Re: [net-next RFC PATCH 0/3] net: phy: detach PHY driver OPs from phy_driver struct

[Russell King (Oracle)]

On Sun, Feb 18, 2024 at 12:22:45AM +0100, Christian Marangi wrote:
> On Sat, Feb 17, 2024 at 07:53:08PM +0000, Russell King (Oracle) wrote:
> > Would it make more sense instead of this big churn, to instead
> > introduce into struct phy_driver:
> > 
> > 	struct mdio_device_id	*ids;
> > 
> > which would then allow a phy_driver structure to be matched by
> > several device IDs?
> 
> Yes that was an alternative idea, but is it good to then have 2 way to
> declare PHY ID?
> 
> Also the name should be changed... Maybe an array of a struct PHY_ID,
> name that ends with a sentinel?

We already have arrays of mdio_device_id in every driver, whether they
can be re-used or not is a separate issue. In any case, merely adding
support for this (only build tested patch below) is a much smaller
change.

 drivers/net/phy/phy_device.c | 64 +++++++++++++++++++++++++++++++++-----------
 include/linux/phy.h          | 16 ++++++++++-
 2 files changed, 63 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2e7d5bfb338e..7e02bf51a2a5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -522,12 +522,51 @@ static int phy_scan_fixups(struct phy_device *phydev)
 	return 0;
 }
 
+static const struct mdio_device_id *
+phy_driver_match_id(struct phy_driver *phydrv, u32 id)
+{
+	const struct mdio_device_id *ids = phydrv->ids;
+
+	if (ids) {
+		while (ids->phy_id) {
+			if (phy_id_compare(id, ids->phy_id, ids->phy_id_mask))
+				return ids;
+			ids++;
+		}
+	}
+
+	if (phy_id_compare(id, phydrv->id.phy_id, phydrv->id.phy_id_mask))
+		return &phydrv->id;
+
+	return NULL;
+}
+
+static const struct mdio_device_id *
+phy_driver_match(struct phy_driver *phydrv, struct phy_device *phydev)
+{
+	const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids);
+	const struct mdio_device_id *id;
+	int i;
+
+	if (!phydev->is_c45)
+		return phy_driver_match_id(phydrv, phydev->phy_id);
+
+	for (i = 1; i < num_ids; i++) {
+		if (phydev->c45_ids.device_ids[i] == 0xffffffff)
+			continue;
+
+		id = phy_driver_match_id(phydrv, phydev->c45_ids.device_ids[i]);
+		if (id)
+			return id;
+	}
+
+	return NULL;
+}
+
 static int phy_bus_match(struct device *dev, struct device_driver *drv)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 	struct phy_driver *phydrv = to_phy_driver(drv);
-	const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids);
-	int i;
 
 	if (!(phydrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY))
 		return 0;
@@ -535,20 +574,7 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
 	if (phydrv->match_phy_device)
 		return phydrv->match_phy_device(phydev);
 
-	if (phydev->is_c45) {
-		for (i = 1; i < num_ids; i++) {
-			if (phydev->c45_ids.device_ids[i] == 0xffffffff)
-				continue;
-
-			if (phy_id_compare(phydev->c45_ids.device_ids[i],
-					   phydrv->phy_id, phydrv->phy_id_mask))
-				return 1;
-		}
-		return 0;
-	} else {
-		return phy_id_compare(phydev->phy_id, phydrv->phy_id,
-				      phydrv->phy_id_mask);
-	}
+	return !!phy_driver_match(phydrv, phydev);
 }
 
 static ssize_t
@@ -3311,6 +3337,7 @@ static int phy_probe(struct device *dev)
 	int err = 0;
 
 	phydev->drv = phydrv;
+	phydev->drv_id = phy_driver_match(phydrv, phydev);
 
 	/* Disable the interrupt if the PHY doesn't support it
 	 * but the interrupt is still a valid one
@@ -3485,6 +3512,11 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
+	if (!new_driver->id.phy_id) {
+		new_driver->id.phy_id = new_driver->phy_id;
+		new_driver->id.phy_id_mask = new_driver->phy_id_mask;
+	}
+
 	retval = driver_register(&new_driver->mdiodrv.driver);
 	if (retval) {
 		pr_err("%s: Error %d in registering driver\n",
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fd8dbea9b4d9..2f2ebbd41535 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -542,6 +542,7 @@ struct macsec_ops;
  *
  * @mdio: MDIO bus this PHY is on
  * @drv: Pointer to the driver for this PHY instance
+ * @drv_id: The matched driver ID for this PHY instance
  * @devlink: Create a link between phy dev and mac dev, if the external phy
  *           used by current mac interface is managed by another mac interface.
  * @phy_id: UID for this device found during discovery
@@ -639,6 +640,7 @@ struct phy_device {
 	/* Information about the PHY type */
 	/* And management functions */
 	const struct phy_driver *drv;
+	const struct mdio_device_id *drv_id;
 
 	struct device_link *devlink;
 
@@ -882,6 +884,9 @@ struct phy_led {
  * struct phy_driver - Driver structure for a particular PHY type
  *
  * @mdiodrv: Data common to all MDIO devices
+ * @ids: array of mdio device IDs to match this driver (terminated with
+ *   zero phy_id)
+ * @id: mdio device ID to match
  * @phy_id: The result of reading the UID registers of this PHY
  *   type, and ANDing them with the phy_id_mask.  This driver
  *   only works for PHYs with IDs which match this field
@@ -903,6 +908,8 @@ struct phy_led {
  */
 struct phy_driver {
 	struct mdio_driver_common mdiodrv;
+	const struct mdio_device_id *ids;
+	struct mdio_device_id id;
 	u32 phy_id;
 	char *name;
 	u32 phy_id_mask;
@@ -1203,7 +1210,14 @@ static inline bool phy_id_compare(u32 id1, u32 id2, u32 mask)
  */
 static inline bool phydev_id_compare(struct phy_device *phydev, u32 id)
 {
-	return phy_id_compare(id, phydev->phy_id, phydev->drv->phy_id_mask);
+	u32 mask;
+
+	if (phydev->drv_id)
+		mask = phydev->drv_id->phy_id_mask;
+	else
+		mask = phydev->drv->phy_id_mask;
+
+	return phy_id_compare(id, phydev->phy_id, mask);
 }
 
 /* A Structure for boards to register fixups with the PHY Lib */

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