Register software nodes and define the device properties.
software-node contains following properties.
  - gpio pin details
  - i2c bus information
  - sfp signals
  - phylink mode

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============
V1 -> V2:
  - SFP GPIO definitions and other macros move from lan743x_main.c to
    lan743x_main.h file
  - Change from "PCI11X1X_" to "PCI11X1X_EVB_PCI11010_" strings for GPIO macros
V0 -> V1:                                                                       
  - No changes

 drivers/net/ethernet/microchip/Kconfig        |   2 +
 drivers/net/ethernet/microchip/lan743x_main.c | 198 +++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  82 ++++++++
 3 files changed, 278 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 2e3eb37a45cd..9c08a4af257a 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -50,6 +50,8 @@ config LAN743X
 	select CRC16
 	select CRC32
 	select PHYLINK
+	select I2C_PCI1XXXX
+	select GP_PCI1XXXX
 	help
 	  Support for the Microchip LAN743x and PCI11x1x families of PCI
 	  Express Ethernet devices
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 20a42a2c7b0e..dc571020ae1b 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -100,8 +100,98 @@ static bool is_pci11x1x_chip(struct lan743x_adapter *adapter)
 	return false;
 }
 
+static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
+					u16 perif_id)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct pci_bus *perif_bus;
+	struct pci_dev *perif_dev;
+	struct pci_dev *br_dev;
+	struct pci_bus *br_bus;
+	struct pci_dev *dev;
+
+	/* PCI11x1x devices' PCIe topology consists of a top level pcie
+	 * switch with up to four downstream ports, some of which have
+	 * integrated endpoints connected to them. One of the downstream ports
+	 * has an embedded single function pcie ethernet controller which is
+	 * handled by this driver. Another downstream port has an
+	 * embedded multifunction pcie endpoint, with four pcie functions
+	 * (the "peripheral controllers": I2C controller, GPIO controller,
+	 * UART controllers, SPIcontrollers)
+	 * The code below navigates the PCI11x1x topology
+	 * to find (by matching its PCI device ID) the peripheral controller
+	 * that should be paired to the embedded ethernet controller.
+	 */
+	br_dev = pci_upstream_bridge(pdev);
+	if (!br_dev) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "upstream bridge not found\n");
+		return br_dev;
+	}
+
+	br_bus = br_dev->bus;
+	list_for_each_entry(dev, &br_bus->devices, bus_list) {
+		if (dev->vendor == PCI1XXXX_VENDOR_ID &&
+		    (dev->device & ~PCI1XXXX_DEV_MASK) ==
+		     PCI1XXXX_BR_PERIF_ID) {
+			perif_bus = dev->subordinate;
+			list_for_each_entry(perif_dev, &perif_bus->devices,
+					    bus_list) {
+				if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
+				    (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
+				     perif_id)
+					return pci_get_drvdata(perif_dev);
+			}
+		}
+	}
+
+	netif_err(adapter, drv, adapter->netdev,
+		  "pci1xxxx peripheral (0x%X) device not found\n", perif_id);
+
+	return NULL;
+}
+
+static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
+{
+	struct pci1xxxx_i2c *i2c_drvdata;
+
+	i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
+	if (!i2c_drvdata)
+		return -EPROBE_DEFER;
+
+	adapter->i2c_adap = &i2c_drvdata->adap;
+	snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
+		 adapter->i2c_adap->name);
+	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
+		  adapter->i2c_adap->name);
+
+	return 0;
+}
+
+static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
+{
+	struct aux_bus_device *aux_bus;
+	struct device *gpio_dev;
+
+	aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
+	if (!aux_bus)
+		return -EPROBE_DEFER;
+
+	gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
+	snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
+		 dev_name(gpio_dev));
+	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
+		  adapter->nodes->gpio_name);
+	return 0;
+}
+
 static void lan743x_pci_cleanup(struct lan743x_adapter *adapter)
 {
+	if (adapter->nodes) {
+		software_node_unregister_node_group(adapter->nodes->group);
+		kfree(adapter->nodes);
+	}
+
 	pci_release_selected_regions(adapter->pdev,
 				     pci_select_bars(adapter->pdev,
 						     IORESOURCE_MEM));
@@ -2888,6 +2978,90 @@ static int lan743x_rx_open(struct lan743x_rx *rx)
 	return ret;
 }
 
+static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct lan743x_sw_nodes *nodes;
+	struct software_node *swnodes;
+	int ret;
+	u32 id;
+
+	nodes = kzalloc(sizeof(*nodes), GFP_KERNEL);
+	if (!nodes)
+		return -ENOMEM;
+
+	adapter->nodes = nodes;
+
+	ret = pci1xxxx_gpio_dev_get(adapter);
+	if (ret < 0)
+		return ret;
+
+	ret = pci1xxxx_i2c_adapter_get(adapter);
+	if (ret < 0)
+		return ret;
+
+	id = (pdev->bus->number << 8) | pdev->devfn;
+	snprintf(nodes->sfp_name, sizeof(nodes->sfp_name),
+		 "sfp-%d", id);
+	snprintf(nodes->phylink_name, sizeof(nodes->phylink_name),
+		 "mchp-pci1xxxx-phylink-%d", id);
+
+	swnodes = nodes->swnodes;
+
+	nodes->gpio_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names",
+						     "default");
+	swnodes[SWNODE_GPIO] = NODE_PROP(nodes->gpio_name, nodes->gpio_props);
+
+	nodes->tx_fault_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							 PCI11X1X_EVB_PCI11010_TX_FAULT_GPIO,
+							 GPIO_ACTIVE_HIGH);
+	nodes->tx_disable_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							   PCI11X1X_EVB_PCI11010_TX_DIS_GPIO,
+							   GPIO_ACTIVE_HIGH);
+	nodes->mod_def0_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							 PCI11X1X_EVB_PCI11010_MOD_DEF0_GPIO,
+							 GPIO_ACTIVE_LOW);
+	nodes->los_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+						    PCI11X1X_EVB_PCI11010_LOS_GPIO,
+						    GPIO_ACTIVE_HIGH);
+	nodes->rate_sel0_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							  PCI11X1X_EVB_PCI11010_RATE_SEL0_GPIO,
+							  GPIO_ACTIVE_HIGH);
+
+	nodes->i2c_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names", "default");
+	swnodes[SWNODE_I2C] = NODE_PROP(nodes->i2c_name, nodes->i2c_props);
+	nodes->i2c_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_I2C]);
+
+	nodes->sfp_props[0] = PROPERTY_ENTRY_STRING("compatible", "sff,sfp");
+	nodes->sfp_props[1] = PROPERTY_ENTRY_REF_ARRAY("i2c-bus",
+						       nodes->i2c_ref);
+	nodes->sfp_props[2] = PROPERTY_ENTRY_REF_ARRAY("tx-fault-gpios",
+						       nodes->tx_fault_ref);
+	nodes->sfp_props[3] = PROPERTY_ENTRY_REF_ARRAY("tx-disable-gpios",
+						       nodes->tx_disable_ref);
+	nodes->sfp_props[4] = PROPERTY_ENTRY_REF_ARRAY("mod-def0-gpios",
+						       nodes->mod_def0_ref);
+	nodes->sfp_props[5] = PROPERTY_ENTRY_REF_ARRAY("los-gpios",
+						       nodes->los_ref);
+	nodes->sfp_props[6] = PROPERTY_ENTRY_REF_ARRAY("rate-select0-gpios",
+						       nodes->rate_sel0_ref);
+	swnodes[SWNODE_SFP] = NODE_PROP(nodes->sfp_name, nodes->sfp_props);
+	nodes->sfp_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_SFP]);
+	nodes->phylink_props[0] = PROPERTY_ENTRY_STRING("managed",
+							"in-band-status");
+	nodes->phylink_props[1] = PROPERTY_ENTRY_REF_ARRAY("sfp",
+							   nodes->sfp_ref);
+	swnodes[SWNODE_PHYLINK] = NODE_PROP(nodes->phylink_name,
+					    nodes->phylink_props);
+
+	nodes->group[SWNODE_GPIO] = &swnodes[SWNODE_GPIO];
+	nodes->group[SWNODE_I2C] = &swnodes[SWNODE_I2C];
+	nodes->group[SWNODE_SFP] = &swnodes[SWNODE_SFP];
+	nodes->group[SWNODE_PHYLINK] = &swnodes[SWNODE_PHYLINK];
+
+	return software_node_register_node_group(nodes->group);
+}
+
 static int lan743x_phylink_sgmii_config(struct lan743x_adapter *adapter)
 {
 	u32 sgmii_ctl;
@@ -3105,6 +3279,7 @@ static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
 static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
+	struct fwnode_handle *fwnode = NULL;
 	struct phylink *pl;
 
 	adapter->phylink_config.dev = &netdev->dev;
@@ -3138,7 +3313,13 @@ static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 		phy_interface_set_rgmii(adapter->phylink_config.supported_interfaces);
 	}
 
-	pl = phylink_create(&adapter->phylink_config, NULL,
+	if (adapter->nodes) {
+		fwnode = software_node_fwnode(adapter->nodes->group[SWNODE_PHYLINK]);
+		if (!fwnode)
+			return -ENODEV;
+	}
+
+	pl = phylink_create(&adapter->phylink_config, fwnode,
 			    adapter->phy_interface, &lan743x_phylink_mac_ops);
 
 	if (IS_ERR(pl)) {
@@ -3511,9 +3692,18 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 	if (ret)
 		return ret;
 
-	ret = lan743x_phy_init(adapter);
-	if (ret)
-		return ret;
+	if (adapter->is_sfp_support_en && !adapter->nodes) {
+		ret = lan743x_swnodes_register(adapter);
+		if (ret) {
+			netdev_err(adapter->netdev,
+				   "failed to register software nodes\n");
+			return ret;
+		}
+	} else {
+		ret = lan743x_phy_init(adapter);
+		if (ret)
+			return ret;
+	}
 
 	ret = lan743x_ptp_init(adapter);
 	if (ret)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index f7e96496600b..bf0d0f285e39 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -6,6 +6,10 @@
 
 #include <linux/phy.h>
 #include <linux/phylink.h>
+#include <linux/property.h>
+#include <linux/i2c.h>
+#include <linux/gpio/machine.h>
+#include <linux/auxiliary_bus.h>
 #include "lan743x_ptp.h"
 
 #define DRIVER_AUTHOR   "Bryan Whitehead <Bryan.Whitehead@microchip.com>"
@@ -1047,6 +1051,40 @@ enum lan743x_sgmii_lsd {
 
 #define MAC_SUPPORTED_WAKES  (WAKE_BCAST | WAKE_UCAST | WAKE_MCAST | \
 			      WAKE_MAGIC | WAKE_ARP)
+
+enum lan743x_swnodes {
+	SWNODE_GPIO = 0,
+	SWNODE_I2C,
+	SWNODE_SFP,
+	SWNODE_PHYLINK,
+	SWNODE_MAX
+};
+
+#define I2C_DRV_NAME		48
+#define GPIO_DRV_NAME		32
+#define SFP_NODE_NAME		32
+#define PHYLINK_NODE_NAME	32
+
+struct lan743x_sw_nodes {
+	char gpio_name[GPIO_DRV_NAME];
+	char i2c_name[I2C_DRV_NAME];
+	char sfp_name[SFP_NODE_NAME];
+	char phylink_name[PHYLINK_NODE_NAME];
+	struct property_entry gpio_props[1];
+	struct property_entry i2c_props[1];
+	struct property_entry sfp_props[8];
+	struct property_entry phylink_props[2];
+	struct software_node_ref_args i2c_ref[1];
+	struct software_node_ref_args tx_fault_ref[1];
+	struct software_node_ref_args tx_disable_ref[1];
+	struct software_node_ref_args mod_def0_ref[1];
+	struct software_node_ref_args los_ref[1];
+	struct software_node_ref_args rate_sel0_ref[1];
+	struct software_node_ref_args sfp_ref[1];
+	struct software_node swnodes[SWNODE_MAX];
+	const struct software_node *group[SWNODE_MAX + 1];
+};
+
 struct lan743x_adapter {
 	struct net_device       *netdev;
 	struct mii_bus		*mdiobus;
@@ -1089,6 +1127,8 @@ struct lan743x_adapter {
 	phy_interface_t		phy_interface;
 	struct phylink		*phylink;
 	struct phylink_config	phylink_config;
+	struct lan743x_sw_nodes	*nodes;
+	struct i2c_adapter	*i2c_adap;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
@@ -1202,6 +1242,48 @@ struct lan743x_rx_buffer_info {
 #define RX_PROCESS_RESULT_NOTHING_TO_DO     (0)
 #define RX_PROCESS_RESULT_BUFFER_RECEIVED   (1)
 
+#define PCI1XXXX_VENDOR_ID      0x1055
+#define PCI1XXXX_BR_PERIF_ID    0xA00C
+#define PCI1XXXX_PERIF_I2C_ID   0xA003
+#define PCI1XXXX_PERIF_GPIO_ID  0xA005
+#define PCI1XXXX_DEV_MASK       GENMASK(7, 4)
+
+#define PCI11X1X_EVB_PCI11010_TX_FAULT_GPIO  46
+#define PCI11X1X_EVB_PCI11010_TX_DIS_GPIO    47
+#define PCI11X1X_EVB_PCI11010_RATE_SEL0_GPIO 48
+#define PCI11X1X_EVB_PCI11010_LOS_GPIO       49
+#define PCI11X1X_EVB_PCI11010_MOD_DEF0_GPIO  51
+
+#define NODE_PROP(_NAME, _PROP)         \
+	((const struct software_node) { \
+		.name = _NAME,          \
+		.properties = _PROP,    \
+	})
+
+struct pci1xxxx_i2c {
+	struct completion i2c_xfer_done;
+	bool i2c_xfer_in_progress;
+	struct i2c_adapter adap;
+	void __iomem *i2c_base;
+	u32 freq;
+	u32 flags;
+};
+
+struct gp_aux_data_type {
+	int irq_num;
+	resource_size_t region_start;
+	resource_size_t region_length;
+};
+
+struct auxiliary_device_wrapper {
+	struct auxiliary_device aux_dev;
+	struct gp_aux_data_type gp_aux_data;
+};
+
+struct aux_bus_device {
+	struct auxiliary_device_wrapper *aux_device_wrapper[2];
+};
+
 u32 lan743x_csr_read(struct lan743x_adapter *adapter, int offset);
 void lan743x_csr_write(struct lan743x_adapter *adapter, int offset, u32 data);
 int lan743x_hs_syslock_acquire(struct lan743x_adapter *adapter, u16 timeout);
-- 
2.34.1

Hi Raju,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Lakkaraju/net-lan743x-Add-SFP-support-check-flag/20240912-002444
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240911161054.4494-3-Raju.Lakkaraju%40microchip.com
patch subject: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp
config: x86_64-randconfig-003-20240914 (https://download.01.org/0day-ci/archive/20240915/202409150110.dSOZKgpK-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409150110.dSOZKgpK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409150110.dSOZKgpK-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: devm_i2c_add_adapter
   >>> referenced by i2c-mchp-pci1xxxx.c:1182 (drivers/i2c/busses/i2c-mchp-pci1xxxx.c:1182)
   >>>               drivers/i2c/busses/i2c-mchp-pci1xxxx.o:(pci1xxxx_i2c_probe_pci) in archive vmlinux.a

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GP_PCI1XXXX
   Depends on [n]: PCI [=y] && GPIOLIB [=y] && NVMEM_SYSFS [=n]
   Selected by [y]:
   - LAN743X [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_MICROCHIP [=y] && PCI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]
   WARNING: unmet direct dependencies detected for I2C_PCI1XXXX
   Depends on [m]: I2C [=m] && HAS_IOMEM [=y] && PCI [=y]
   Selected by [y]:
   - LAN743X [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_MICROCHIP [=y] && PCI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> index 2e3eb37a45cd..9c08a4af257a 100644
> --- a/drivers/net/ethernet/microchip/Kconfig
> +++ b/drivers/net/ethernet/microchip/Kconfig
> @@ -50,6 +50,8 @@ config LAN743X
>  	select CRC16
>  	select CRC32
>  	select PHYLINK
> +	select I2C_PCI1XXXX
> +	select GP_PCI1XXXX

GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
is not in 6.11-rc7. Is it in gpio-next?

> +static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
> +					u16 perif_id)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct pci_bus *perif_bus;
> +	struct pci_dev *perif_dev;
> +	struct pci_dev *br_dev;
> +	struct pci_bus *br_bus;
> +	struct pci_dev *dev;
> +
> +	/* PCI11x1x devices' PCIe topology consists of a top level pcie
> +	 * switch with up to four downstream ports, some of which have
> +	 * integrated endpoints connected to them. One of the downstream ports
> +	 * has an embedded single function pcie ethernet controller which is
> +	 * handled by this driver. Another downstream port has an
> +	 * embedded multifunction pcie endpoint, with four pcie functions
> +	 * (the "peripheral controllers": I2C controller, GPIO controller,
> +	 * UART controllers, SPIcontrollers)
> +	 * The code below navigates the PCI11x1x topology
> +	 * to find (by matching its PCI device ID) the peripheral controller
> +	 * that should be paired to the embedded ethernet controller.
> +	 */
> +	br_dev = pci_upstream_bridge(pdev);
> +	if (!br_dev) {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "upstream bridge not found\n");
> +		return br_dev;
> +	}
> +
> +	br_bus = br_dev->bus;
> +	list_for_each_entry(dev, &br_bus->devices, bus_list) {
> +		if (dev->vendor == PCI1XXXX_VENDOR_ID &&
> +		    (dev->device & ~PCI1XXXX_DEV_MASK) ==
> +		     PCI1XXXX_BR_PERIF_ID) {
> +			perif_bus = dev->subordinate;
> +			list_for_each_entry(perif_dev, &perif_bus->devices,
> +					    bus_list) {
> +				if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
> +				    (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
> +				     perif_id)
> +					return pci_get_drvdata(perif_dev);
> +			}
> +		}
> +	}

It would be good to have the PCI Maintainers review of this. Maybe
pull this out into a patch of its own and Cc: Bjorn Helgaas
<bhelgaas@google.com>

	Andrew

Hi Andrew,

The 09/11/2024 19:17, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> > index 2e3eb37a45cd..9c08a4af257a 100644
> > --- a/drivers/net/ethernet/microchip/Kconfig
> > +++ b/drivers/net/ethernet/microchip/Kconfig
> > @@ -50,6 +50,8 @@ config LAN743X
> >       select CRC16
> >       select CRC32
> >       select PHYLINK
> > +     select I2C_PCI1XXXX
> > +     select GP_PCI1XXXX
> 
> GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
> is not in 6.11-rc7. Is it in gpio-next?
> 

Yes. But GPIO driver developer use this.
I have to use the same here.

It's exist in 6.11-rc6
drivers/misc/mchp_pci1xxxx/Kconfig
This was defined along back.

>Is it in gpio-next? 
No. available in net-next

> > +static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
> > +                                     u16 perif_id)
> > +{
> > +     struct pci_dev *pdev = adapter->pdev;
> > +     struct pci_bus *perif_bus;
> > +     struct pci_dev *perif_dev;
> > +     struct pci_dev *br_dev;
> > +     struct pci_bus *br_bus;
> > +     struct pci_dev *dev;
> > +
> > +     /* PCI11x1x devices' PCIe topology consists of a top level pcie
> > +      * switch with up to four downstream ports, some of which have
> > +      * integrated endpoints connected to them. One of the downstream ports
> > +      * has an embedded single function pcie ethernet controller which is
> > +      * handled by this driver. Another downstream port has an
> > +      * embedded multifunction pcie endpoint, with four pcie functions
> > +      * (the "peripheral controllers": I2C controller, GPIO controller,
> > +      * UART controllers, SPIcontrollers)
> > +      * The code below navigates the PCI11x1x topology
> > +      * to find (by matching its PCI device ID) the peripheral controller
> > +      * that should be paired to the embedded ethernet controller.
> > +      */
> > +     br_dev = pci_upstream_bridge(pdev);
> > +     if (!br_dev) {
> > +             netif_err(adapter, drv, adapter->netdev,
> > +                       "upstream bridge not found\n");
> > +             return br_dev;
> > +     }
> > +
> > +     br_bus = br_dev->bus;
> > +     list_for_each_entry(dev, &br_bus->devices, bus_list) {
> > +             if (dev->vendor == PCI1XXXX_VENDOR_ID &&
> > +                 (dev->device & ~PCI1XXXX_DEV_MASK) ==
> > +                  PCI1XXXX_BR_PERIF_ID) {
> > +                     perif_bus = dev->subordinate;
> > +                     list_for_each_entry(perif_dev, &perif_bus->devices,
> > +                                         bus_list) {
> > +                             if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
> > +                                 (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
> > +                                  perif_id)
> > +                                     return pci_get_drvdata(perif_dev);
> > +                     }
> > +             }
> > +     }
> 
> It would be good to have the PCI Maintainers review of this. Maybe
> pull this out into a patch of its own and Cc: Bjorn Helgaas
> <bhelgaas@google.com>

Sure. I will add Cc: Bjorn Helgaas.

> 
>         Andrew

-- 
Thanks,                                                                         
Raju

On Thu, Sep 12, 2024 at 12:08:40PM +0530, Raju Lakkaraju wrote:
> Hi Andrew,
> 
> The 09/11/2024 19:17, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > > diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> > > index 2e3eb37a45cd..9c08a4af257a 100644
> > > --- a/drivers/net/ethernet/microchip/Kconfig
> > > +++ b/drivers/net/ethernet/microchip/Kconfig
> > > @@ -50,6 +50,8 @@ config LAN743X
> > >       select CRC16
> > >       select CRC32
> > >       select PHYLINK
> > > +     select I2C_PCI1XXXX
> > > +     select GP_PCI1XXXX
> > 
> > GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
> > is not in 6.11-rc7. Is it in gpio-next?
> > 
> 
> Yes. But GPIO driver developer use this.

And the GPIO Maintainer accepted this, despite it not being the same
as every other GPIO driver?

Ah, there is no Acked-by: from anybody i recognise as a GPIO
maintainer. Was it even reviewed by GPIO people? And why is it hiding
in driver/misc? I don't see any reason it cannot be in drivers/gpio,
which is where i looked for it. There are other auxiliary_driver in
drivers/gpio.

> I have to use the same here.

Unfortunately, i have to agree, for the moment.

But it would be good to clean it up. I _think_ mchp_pci1xxxx_gpio.c
can be moved into driver/gpio, given the expected Kconfig symbol
GPIO_PCI1XXXX and depends on GP_PCI1XXXX. It would then also get
reviewed by the GPIO Maintainers, which you probably want.

	Andrew

On Thu, Sep 12, 2024 at 05:19:37PM +0200, Andrew Lunn wrote:
> On Thu, Sep 12, 2024 at 12:08:40PM +0530, Raju Lakkaraju wrote:
> > Hi Andrew,
> > 
> > The 09/11/2024 19:17, Andrew Lunn wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > > diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> > > > index 2e3eb37a45cd..9c08a4af257a 100644
> > > > --- a/drivers/net/ethernet/microchip/Kconfig
> > > > +++ b/drivers/net/ethernet/microchip/Kconfig
> > > > @@ -50,6 +50,8 @@ config LAN743X
> > > >       select CRC16
> > > >       select CRC32
> > > >       select PHYLINK
> > > > +     select I2C_PCI1XXXX
> > > > +     select GP_PCI1XXXX
> > > 
> > > GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
> > > is not in 6.11-rc7. Is it in gpio-next?
> > > 
> > 
> > Yes. But GPIO driver developer use this.
> 
> And the GPIO Maintainer accepted this, despite it not being the same
> as every other GPIO driver?
> 
> Ah, there is no Acked-by: from anybody i recognise as a GPIO
> maintainer. Was it even reviewed by GPIO people? And why is it hiding
> in driver/misc? I don't see any reason it cannot be in drivers/gpio,
> which is where i looked for it. There are other auxiliary_driver in
> drivers/gpio.


What's worse is that the Link: in the commit adding it doesn't
work!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Le 11/09/2024 à 18:10, Raju Lakkaraju a écrit :
> Register software nodes and define the device properties.
> software-node contains following properties.
>    - gpio pin details
>    - i2c bus information
>    - sfp signals
>    - phylink mode
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>

Hi,

...

> +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> +{
> +	struct pci1xxxx_i2c *i2c_drvdata;
> +
> +	i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
> +	if (!i2c_drvdata)
> +		return -EPROBE_DEFER;
> +
> +	adapter->i2c_adap = &i2c_drvdata->adap;
> +	snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
> +		 adapter->i2c_adap->name);

strscpy() ?

> +	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> +		  adapter->i2c_adap->name);
> +
> +	return 0;
> +}
> +
> +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> +{
> +	struct aux_bus_device *aux_bus;
> +	struct device *gpio_dev;
> +
> +	aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> +	if (!aux_bus)
> +		return -EPROBE_DEFER;
> +
> +	gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> +	snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> +		 dev_name(gpio_dev));

strscpy() ?

> +	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> +		  adapter->nodes->gpio_name);
> +	return 0;
> +}
> +

...

Hi Christophe,

The 09/11/2024 18:54, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Le 11/09/2024 à 18:10, Raju Lakkaraju a écrit :
> > Register software nodes and define the device properties.
> > software-node contains following properties.
> >    - gpio pin details
> >    - i2c bus information
> >    - sfp signals
> >    - phylink mode
> > 
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> 
> Hi,
> 
> ...
> 
> > +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct pci1xxxx_i2c *i2c_drvdata;
> > +
> > +     i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
> > +     if (!i2c_drvdata)
> > +             return -EPROBE_DEFER;
> > +
> > +     adapter->i2c_adap = &i2c_drvdata->adap;
> > +     snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
> > +              adapter->i2c_adap->name);
> 
> strscpy() ?
> 

Accepted. I will fix.
Here snprintf( ) does not take any format string, we can use strscpy( ).
 
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->i2c_adap->name);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct aux_bus_device *aux_bus;
> > +     struct device *gpio_dev;
> > +
> > +     aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> > +     if (!aux_bus)
> > +             return -EPROBE_DEFER;
> > +
> > +     gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> > +     snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> > +              dev_name(gpio_dev));
> 
> strscpy() ?
> 

Accepted. I will fix.
 
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->i2c_adap->name);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct aux_bus_device *aux_bus;
> > +     struct device *gpio_dev;
> > +
> > +     aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> > +     if (!aux_bus)
> > +             return -EPROBE_DEFER;
> > +
> > +     gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> > +     snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> > +              dev_name(gpio_dev));
> 
> strscpy() ?

Accepted. I will fix.
 
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->i2c_adap->name);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct aux_bus_device *aux_bus;
> > +     struct device *gpio_dev;
> > +
> > +     aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> > +     if (!aux_bus)
> > +             return -EPROBE_DEFER;
> > +
> > +     gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> > +     snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> > +              dev_name(gpio_dev));
> 
> strscpy() ?
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->nodes->gpio_name);
> > +     return 0;
> > +}
> > +
> 
> ...

-- 
Thanks,                                                                         
Raju

On Thu, Sep 12, 2024 at 12:02:57PM +0530, Raju Lakkaraju wrote:
> Hi Christophe,
> 
> The 09/11/2024 18:54, Christophe JAILLET wrote:
> > > +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> > > +{
> > > +     struct pci1xxxx_i2c *i2c_drvdata;
> > > +
> > > +     i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
> > > +     if (!i2c_drvdata)
> > > +             return -EPROBE_DEFER;
> > > +
> > > +     adapter->i2c_adap = &i2c_drvdata->adap;
> > > +     snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
> > > +              adapter->i2c_adap->name);
> > 
> > strscpy() ?
> > 
> 
> Accepted. I will fix.
> Here snprintf( ) does not take any format string, we can use strscpy( ).

As a general tip for safe programming... never use snprintf() as a
"short cut" for copying strings. It may do stuff that you don't
expect!

For example, taking the above case, if "adapter->i2c_adap->name"
contains any % characters, then, as you are passing it as the
_format_ _string_, sprintf() will try to interpret those as printf
escape sequences, and thus _can_ attempt to dereference arguments
that were never passed to snprintf().

If you really want to do this kind of thing, at least write it in
a safe way...

	snprintf(..., "%s", string);

rather than:

	snprintf(..., string);

so that "string" doesn't attempt to be escape-expanded.

Of course, using proper string copying functions that do what you
want in a cheap way is always more preferable to the printf related
functions!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

> If you really want to do this kind of thing, at least write it in
> a safe way...
> 
> 	snprintf(..., "%s", string);
> 
> rather than:
> 
> 	snprintf(..., string);
> 
> so that "string" doesn't attempt to be escape-expanded.

One of the static analysers is complaining about this danger, or it
might be GCC itself if you up the warning level. The kernel hardening
people are replacing all such bad cases, one by one. So we definitely
don't want to add more.

	Andrew