[net-next PATCH v3 1/3] net: phy: extend PHY package API to support multiple global address

[net-next PATCH v3 1/3] net: phy: extend PHY package API to support multiple global address

[Christian Marangi]

Current API for PHY package are limited to single address to configure
global settings for the PHY package.

It was found that some PHY package (for example the qca807x, a PHY
package that is shipped with a bundle of 5 PHY) requires multiple PHY
address to configure global settings. An example scenario is a PHY that
have a dedicated PHY for PSGMII/serdes calibrarion and have a specific
PHY in the package where the global PHY mode is set and affects every
other PHY in the package.

Change the API in the following way:
- Change phy_package_join() to take the base addr of the PHY package
  instead of the global PHY addr.
- Make __/phy_package_write/read() require an additional arg that
  select what global PHY address to use by passing the offset from the
  base addr passed on phy_package_join().

Each user of this API is updated to follow this new implementation
following a pattern where an enum is defined to declare the offset of the
addr.

We also drop the check if shared is defined as any user of the
phy_package_read/write is expected to use phy_package_join first. Misuse
of this will correctly trigger a kernel panic for NULL pointer
exception.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v2:
- Make kernel panic if shared is not init (bugged scenario)
- Fix some confusing comments

 drivers/net/phy/bcm54140.c       | 16 +++++++++----
 drivers/net/phy/mscc/mscc.h      |  5 ++++
 drivers/net/phy/mscc/mscc_main.c |  4 ++--
 drivers/net/phy/phy_device.c     | 35 ++++++++++++++-------------
 include/linux/phy.h              | 41 +++++++++++++++++++-------------
 5 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index d43076592f81..2eea3d09b1e6 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -128,6 +128,10 @@
 #define BCM54140_DEFAULT_DOWNSHIFT 5
 #define BCM54140_MAX_DOWNSHIFT 9
 
+enum bcm54140_global_phy {
+	BCM54140_BASE_ADDR = 0,
+};
+
 struct bcm54140_priv {
 	int port;
 	int base_addr;
@@ -429,11 +433,13 @@ static int bcm54140_base_read_rdb(struct phy_device *phydev, u16 rdb)
 	int ret;
 
 	phy_lock_mdio_bus(phydev);
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __phy_package_read(phydev, MII_BCM54XX_RDB_DATA);
+	ret = __phy_package_read(phydev, BCM54140_BASE_ADDR,
+				 MII_BCM54XX_RDB_DATA);
 
 out:
 	phy_unlock_mdio_bus(phydev);
@@ -446,11 +452,13 @@ static int bcm54140_base_write_rdb(struct phy_device *phydev,
 	int ret;
 
 	phy_lock_mdio_bus(phydev);
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_DATA, val);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_DATA, val);
 
 out:
 	phy_unlock_mdio_bus(phydev);
diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 7a962050a4d4..6a3d8a754eb8 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -416,6 +416,11 @@ struct vsc8531_private {
  * gpio_lock: used for PHC operations. Common for all PHYs as the load/save GPIO
  * is shared.
  */
+
+enum vsc85xx_global_phy {
+	VSC88XX_BASE_ADDR = 0,
+};
+
 struct vsc85xx_shared_private {
 	struct mutex gpio_lock;
 };
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 4171f01d34e5..6f74ce0ab1aa 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -711,7 +711,7 @@ int phy_base_write(struct phy_device *phydev, u32 regnum, u16 val)
 		dump_stack();
 	}
 
-	return __phy_package_write(phydev, regnum, val);
+	return __phy_package_write(phydev, VSC88XX_BASE_ADDR, regnum, val);
 }
 
 /* phydev->bus->mdio_lock should be locked when using this function */
@@ -722,7 +722,7 @@ int phy_base_read(struct phy_device *phydev, u32 regnum)
 		dump_stack();
 	}
 
-	return __phy_package_read(phydev, regnum);
+	return __phy_package_read(phydev, VSC88XX_BASE_ADDR, regnum);
 }
 
 u32 vsc85xx_csr_read(struct phy_device *phydev,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 478126f6b5bc..424cbb13de13 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1648,20 +1648,22 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
 /**
  * phy_package_join - join a common PHY group
  * @phydev: target phy_device struct
- * @addr: cookie and PHY address for global register access
+ * @base_addr: cookie and base PHY address of PHY package for offset
+ *   calculation of global register access
  * @priv_size: if non-zero allocate this amount of bytes for private data
  *
  * This joins a PHY group and provides a shared storage for all phydevs in
  * this group. This is intended to be used for packages which contain
  * more than one PHY, for example a quad PHY transceiver.
  *
- * The addr parameter serves as a cookie which has to have the same value
- * for all members of one group and as a PHY address to access generic
- * registers of a PHY package. Usually, one of the PHY addresses of the
- * different PHYs in the package provides access to these global registers.
+ * The base_addr parameter serves as cookie which has to have the same values
+ * for all members of one group and as the base PHY address of the PHY package
+ * for offset calculation to access generic registers of a PHY package.
+ * Usually, one of the PHY addresses of the different PHYs in the package
+ * provides access to these global registers.
  * The address which is given here, will be used in the phy_package_read()
- * and phy_package_write() convenience functions. If your PHY doesn't have
- * global registers you can just pick any of the PHY addresses.
+ * and phy_package_write() convenience functions as base and added to the
+ * passed offset in those functions.
  *
  * This will set the shared pointer of the phydev to the shared storage.
  * If this is the first call for a this cookie the shared storage will be
@@ -1671,17 +1673,17 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
  * Returns < 1 on error, 0 on success. Esp. calling phy_package_join()
  * with the same cookie but a different priv_size is an error.
  */
-int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size)
+int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size)
 {
 	struct mii_bus *bus = phydev->mdio.bus;
 	struct phy_package_shared *shared;
 	int ret;
 
-	if (addr < 0 || addr >= PHY_MAX_ADDR)
+	if (base_addr < 0 || base_addr >= PHY_MAX_ADDR)
 		return -EINVAL;
 
 	mutex_lock(&bus->shared_lock);
-	shared = bus->shared[addr];
+	shared = bus->shared[base_addr];
 	if (!shared) {
 		ret = -ENOMEM;
 		shared = kzalloc(sizeof(*shared), GFP_KERNEL);
@@ -1693,9 +1695,9 @@ int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size)
 				goto err_free;
 			shared->priv_size = priv_size;
 		}
-		shared->addr = addr;
+		shared->base_addr = base_addr;
 		refcount_set(&shared->refcnt, 1);
-		bus->shared[addr] = shared;
+		bus->shared[base_addr] = shared;
 	} else {
 		ret = -EINVAL;
 		if (priv_size && priv_size != shared->priv_size)
@@ -1733,7 +1735,7 @@ void phy_package_leave(struct phy_device *phydev)
 		return;
 
 	if (refcount_dec_and_mutex_lock(&shared->refcnt, &bus->shared_lock)) {
-		bus->shared[shared->addr] = NULL;
+		bus->shared[shared->base_addr] = NULL;
 		mutex_unlock(&bus->shared_lock);
 		kfree(shared->priv);
 		kfree(shared);
@@ -1752,7 +1754,8 @@ static void devm_phy_package_leave(struct device *dev, void *res)
  * devm_phy_package_join - resource managed phy_package_join()
  * @dev: device that is registering this PHY package
  * @phydev: target phy_device struct
- * @addr: cookie and PHY address for global register access
+ * @base_addr: cookie and base PHY address of PHY package for offset
+ *   calculation of global register access
  * @priv_size: if non-zero allocate this amount of bytes for private data
  *
  * Managed phy_package_join(). Shared storage fetched by this function,
@@ -1760,7 +1763,7 @@ static void devm_phy_package_leave(struct device *dev, void *res)
  * phy_package_join() for more information.
  */
 int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
-			  int addr, size_t priv_size)
+			  int base_addr, size_t priv_size)
 {
 	struct phy_device **ptr;
 	int ret;
@@ -1770,7 +1773,7 @@ int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
 	if (!ptr)
 		return -ENOMEM;
 
-	ret = phy_package_join(phydev, addr, priv_size);
+	ret = phy_package_join(phydev, base_addr, priv_size);
 
 	if (!ret) {
 		*ptr = phydev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e5f1f41e399c..51702e349d83 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -327,7 +327,8 @@ struct mdio_bus_stats {
 
 /**
  * struct phy_package_shared - Shared information in PHY packages
- * @addr: Common PHY address used to combine PHYs in one package
+ * @base_addr: Base PHY address of PHY package used to combine PHYs
+ *   in one package and for offset calculation of phy_package_read/write
  * @refcnt: Number of PHYs connected to this shared data
  * @flags: Initialization of PHY package
  * @priv_size: Size of the shared private data @priv
@@ -338,7 +339,7 @@ struct mdio_bus_stats {
  * phy_package_leave().
  */
 struct phy_package_shared {
-	int addr;
+	int base_addr;
 	refcount_t refcnt;
 	unsigned long flags;
 	size_t priv_size;
@@ -1972,10 +1973,10 @@ int phy_ethtool_get_link_ksettings(struct net_device *ndev,
 int phy_ethtool_set_link_ksettings(struct net_device *ndev,
 				   const struct ethtool_link_ksettings *cmd);
 int phy_ethtool_nway_reset(struct net_device *ndev);
-int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size);
+int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size);
 void phy_package_leave(struct phy_device *phydev);
 int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
-			  int addr, size_t priv_size);
+			  int base_addr, size_t priv_size);
 
 int __init mdio_bus_init(void);
 void mdio_bus_exit(void);
@@ -1998,46 +1999,54 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config,
 		       struct netlink_ext_ack *extack);
 
-static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
+static inline int phy_package_read(struct phy_device *phydev,
+				   unsigned int addr_offset, u32 regnum)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+	return mdiobus_read(phydev->mdio.bus, addr, regnum);
 }
 
-static inline int __phy_package_read(struct phy_device *phydev, u32 regnum)
+static inline int __phy_package_read(struct phy_device *phydev,
+				     unsigned int addr_offset, u32 regnum)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return __mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+	return __mdiobus_read(phydev->mdio.bus, addr, regnum);
 }
 
 static inline int phy_package_write(struct phy_device *phydev,
-				    u32 regnum, u16 val)
+				    unsigned int addr_offset, u32 regnum,
+				    u16 val)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+	return mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
 static inline int __phy_package_write(struct phy_device *phydev,
-				      u32 regnum, u16 val)
+				      unsigned int addr_offset, u32 regnum,
+				      u16 val)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
 
-	if (!shared)
+	if (addr >= PHY_MAX_ADDR)
 		return -EIO;
 
-	return __mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
 static inline bool __phy_package_set_once(struct phy_device *phydev,
-- 
2.40.1

[net-next PATCH v3 2/3] net: phy: restructure __phy_write/read_mmd to helper and phydev user

[Christian Marangi]

Restructure phy_write_mmd and phy_read_mmd to implement generic helper
for direct mdiobus access for mmd and use these helper for phydev user.

This is needed in preparation of PHY package API that requires generic
access to the mdiobus and are deatched from phydev struct but instead
access them based on PHY package base_addr and offsets.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v3:
- Move to phy-core.c instead of inline in phy.h
Changes v2:
- Introduce this patch

 drivers/net/phy/phy-core.c | 64 ++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 966c93cbe616..b729ac8b2640 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -540,6 +540,28 @@ static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
 			devad | MII_MMD_CTRL_NOINCR);
 }
 
+static int mmd_phy_read(struct mii_bus *bus, int phy_addr, bool is_c45,
+			int devad, u32 regnum)
+{
+	if (is_c45)
+		return __mdiobus_c45_read(bus, phy_addr, devad, regnum);
+
+	mmd_phy_indirect(bus, phy_addr, devad, regnum);
+	/* Read the content of the MMD's selected register */
+	return __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
+}
+
+static int mmd_phy_write(struct mii_bus *bus, int phy_addr, bool is_c45,
+			 int devad, u32 regnum, u16 val)
+{
+	if (is_c45)
+		return __mdiobus_c45_write(bus, phy_addr, devad, regnum, val);
+
+	mmd_phy_indirect(bus, phy_addr, devad, regnum);
+	/* Write the data into MMD's selected register */
+	return __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
+}
+
 /**
  * __phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
@@ -551,26 +573,14 @@ static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
  */
 int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
 {
-	int val;
-
 	if (regnum > (u16)~0 || devad > 32)
 		return -EINVAL;
 
-	if (phydev->drv && phydev->drv->read_mmd) {
-		val = phydev->drv->read_mmd(phydev, devad, regnum);
-	} else if (phydev->is_c45) {
-		val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
-					 devad, regnum);
-	} else {
-		struct mii_bus *bus = phydev->mdio.bus;
-		int phy_addr = phydev->mdio.addr;
+	if (phydev->drv && phydev->drv->read_mmd)
+		return phydev->drv->read_mmd(phydev, devad, regnum);
 
-		mmd_phy_indirect(bus, phy_addr, devad, regnum);
-
-		/* Read the content of the MMD's selected register */
-		val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
-	}
-	return val;
+	return mmd_phy_read(phydev->mdio.bus, phydev->mdio.addr,
+			    phydev->is_c45, devad, regnum);
 }
 EXPORT_SYMBOL(__phy_read_mmd);
 
@@ -607,28 +617,14 @@ EXPORT_SYMBOL(phy_read_mmd);
  */
 int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 {
-	int ret;
-
 	if (regnum > (u16)~0 || devad > 32)
 		return -EINVAL;
 
-	if (phydev->drv && phydev->drv->write_mmd) {
-		ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
-	} else if (phydev->is_c45) {
-		ret = __mdiobus_c45_write(phydev->mdio.bus, phydev->mdio.addr,
-					  devad, regnum, val);
-	} else {
-		struct mii_bus *bus = phydev->mdio.bus;
-		int phy_addr = phydev->mdio.addr;
+	if (phydev->drv && phydev->drv->write_mmd)
+		return phydev->drv->write_mmd(phydev, devad, regnum, val);
 
-		mmd_phy_indirect(bus, phy_addr, devad, regnum);
-
-		/* Write the data into MMD's selected register */
-		__mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
-
-		ret = 0;
-	}
-	return ret;
+	return mmd_phy_write(phydev->mdio.bus, phydev->mdio.addr,
+			     phydev->is_c45, devad, regnum, val);
 }
 EXPORT_SYMBOL(__phy_write_mmd);
 
-- 
2.40.1

[net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Christian Marangi]

Some PHY in PHY package may require to read/write MMD regs to correctly
configure the PHY package.

Add support for these additional required function in both lock and no
lock variant.

It's assumed that the entire PHY package is either C22 or C45. We use
C22 or C45 way of writing/reading to mmd regs based on the passed phydev
whether it's C22 or C45.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v3:
- Move in phy-core.c from phy.h
- Base c45 from phydev
Changes v2:
- Rework to use newly introduced helper
- Add common check for regnum and devad

 drivers/net/phy/phy-core.c | 136 +++++++++++++++++++++++++++++++++++++
 include/linux/phy.h        |  33 +++++++++
 2 files changed, 169 insertions(+)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index b729ac8b2640..b5868282def1 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -650,6 +650,142 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 }
 EXPORT_SYMBOL(phy_write_mmd);
 
+/**
+ * __phy_package_read_mmd - Convenience function for reading a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to read from
+ * @regnum: The register on the MMD to read
+ *
+ * Same rules as for __phy_read();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+int __phy_package_read_mmd(struct phy_device *phydev,
+			   unsigned int addr_offset, int devad,
+			   u32 regnum)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
+
+	if (addr >= PHY_MAX_ADDR)
+		return -EIO;
+
+	if (regnum > (u16)~0 || devad > 32)
+		return -EINVAL;
+
+	return mmd_phy_read(phydev->mdio.bus, addr, phydev->is_c45, devad,
+			    regnum);
+}
+EXPORT_SYMBOL(__phy_package_read_mmd);
+
+/**
+ * phy_package_read_mmd - Convenience function for reading a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to read from
+ * @regnum: The register on the MMD to read
+ *
+ * Same rules as for phy_read();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+int phy_package_read_mmd(struct phy_device *phydev,
+			 unsigned int addr_offset, int devad,
+			 u32 regnum)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
+	int val;
+
+	if (addr >= PHY_MAX_ADDR)
+		return -EIO;
+
+	if (regnum > (u16)~0 || devad > 32)
+		return -EINVAL;
+
+	phy_lock_mdio_bus(phydev);
+	val = mmd_phy_read(phydev->mdio.bus, addr, phydev->is_c45, devad,
+			   regnum);
+	phy_unlock_mdio_bus(phydev);
+
+	return val;
+}
+EXPORT_SYMBOL(phy_package_read_mmd);
+
+/**
+ * __phy_package_write_mmd - Convenience function for writing a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to read from
+ * @regnum: The register on the MMD to read
+ * @val: value to write to @regnum
+ *
+ * Same rules as for __phy_write();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+int __phy_package_write_mmd(struct phy_device *phydev,
+			    unsigned int addr_offset, int devad,
+			    u32 regnum, u16 val)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
+
+	if (addr >= PHY_MAX_ADDR)
+		return -EIO;
+
+	if (regnum > (u16)~0 || devad > 32)
+		return -EINVAL;
+
+	return mmd_phy_write(phydev->mdio.bus, addr, phydev->is_c45, devad,
+			     regnum, val);
+}
+EXPORT_SYMBOL(__phy_package_write_mmd);
+
+/**
+ * phy_package_write_mmd - Convenience function for writing a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to read from
+ * @regnum: The register on the MMD to read
+ * @val: value to write to @regnum
+ *
+ * Same rules as for phy_write();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+int phy_package_write_mmd(struct phy_device *phydev,
+			  unsigned int addr_offset, int devad,
+			  u32 regnum, u16 val)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	int addr = shared->base_addr + addr_offset;
+	int ret;
+
+	if (addr >= PHY_MAX_ADDR)
+		return -EIO;
+
+	if (regnum > (u16)~0 || devad > 32)
+		return -EINVAL;
+
+	phy_lock_mdio_bus(phydev);
+	ret = mmd_phy_write(phydev->mdio.bus, addr, phydev->is_c45, devad,
+			    regnum, val);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+EXPORT_SYMBOL(phy_package_write_mmd);
+
 /**
  * phy_modify_changed - Function for modifying a PHY register
  * @phydev: the phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 51702e349d83..41e0698a3685 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2049,6 +2049,39 @@ static inline int __phy_package_write(struct phy_device *phydev,
 	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
+/*
+ * __phy_package_read_mmd - Convenience function for reading a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ */
+int __phy_package_read_mmd(struct phy_device *phydev,
+			   unsigned int addr_offset, int devad,
+			   u32 regnum);
+/*
+ * phy_package_read_mmd - Convenience function for reading a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ */
+int phy_package_read_mmd(struct phy_device *phydev,
+			 unsigned int addr_offset, int devad,
+			 u32 regnum);
+/*
+ * __phy_package_write_mmd - Convenience function for writing a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ */
+int __phy_package_write_mmd(struct phy_device *phydev,
+			    unsigned int addr_offset, int devad,
+			    u32 regnum, u16 val);
+/*
+ * phy_package_write_mmd - Convenience function for writing a register
+ * on an MMD on a given PHY using the PHY package base addr, added of
+ * the addr_offset value.
+ */
+int phy_package_write_mmd(struct phy_device *phydev,
+			  unsigned int addr_offset, int devad,
+			  u32 regnum, u16 val);
+
 static inline bool __phy_package_set_once(struct phy_device *phydev,
 					  unsigned int b)
 {
-- 
2.40.1

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Jakub Kicinski]

On Tue, 28 Nov 2023 14:36:30 +0100 Christian Marangi wrote:
> +/**
> + * phy_package_write_mmd - Convenience function for writing a register
> + * on an MMD on a given PHY using the PHY package base addr, added of
> + * the addr_offset value.
> + * @phydev: The phy_device struct
> + * @addr_offset: The offset to be added to PHY package base_addr
> + * @devad: The MMD to read from
> + * @regnum: The register on the MMD to read
> + * @val: value to write to @regnum
> + *
> + * Same rules as for phy_write();
> + *
> + * NOTE: It's assumed that the entire PHY package is either C22 or C45.
> + */

> +/*
> + * phy_package_write_mmd - Convenience function for writing a register
> + * on an MMD on a given PHY using the PHY package base addr, added of
> + * the addr_offset value.
> + */
> +int phy_package_write_mmd(struct phy_device *phydev,
> +			  unsigned int addr_offset, int devad,
> +			  u32 regnum, u16 val);

Hm, I see there's some precedent here already for this duplicated
semi-kdoc. It seems a bit unusual. If I was looking for kdoc and 
found the header one I'd probably not look at the source file at all.

Andrew, WDYT?

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Andrew Lunn]

On Mon, Dec 04, 2023 at 06:17:52PM -0800, Jakub Kicinski wrote:
> On Tue, 28 Nov 2023 14:36:30 +0100 Christian Marangi wrote:
> > +/**
> > + * phy_package_write_mmd - Convenience function for writing a register
> > + * on an MMD on a given PHY using the PHY package base addr, added of
> > + * the addr_offset value.
> > + * @phydev: The phy_device struct
> > + * @addr_offset: The offset to be added to PHY package base_addr
> > + * @devad: The MMD to read from
> > + * @regnum: The register on the MMD to read
> > + * @val: value to write to @regnum
> > + *
> > + * Same rules as for phy_write();
> > + *
> > + * NOTE: It's assumed that the entire PHY package is either C22 or C45.
> > + */
> 
> > +/*
> > + * phy_package_write_mmd - Convenience function for writing a register
> > + * on an MMD on a given PHY using the PHY package base addr, added of
> > + * the addr_offset value.
> > + */
> > +int phy_package_write_mmd(struct phy_device *phydev,
> > +			  unsigned int addr_offset, int devad,
> > +			  u32 regnum, u16 val);
> 
> Hm, I see there's some precedent here already for this duplicated
> semi-kdoc. It seems a bit unusual. If I was looking for kdoc and 
> found the header one I'd probably not look at the source file at all.
> 
> Andrew, WDYT?

I tend to agree. These functions should be documented once in kdoc,
and only once. I don't really care if its in the header, or the C
code, but not both.

      Andrew

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Christian Marangi]

On Tue, Dec 05, 2023 at 03:37:55AM +0100, Andrew Lunn wrote:
> On Mon, Dec 04, 2023 at 06:17:52PM -0800, Jakub Kicinski wrote:
> > On Tue, 28 Nov 2023 14:36:30 +0100 Christian Marangi wrote:
> > > +/**
> > > + * phy_package_write_mmd - Convenience function for writing a register
> > > + * on an MMD on a given PHY using the PHY package base addr, added of
> > > + * the addr_offset value.
> > > + * @phydev: The phy_device struct
> > > + * @addr_offset: The offset to be added to PHY package base_addr
> > > + * @devad: The MMD to read from
> > > + * @regnum: The register on the MMD to read
> > > + * @val: value to write to @regnum
> > > + *
> > > + * Same rules as for phy_write();
> > > + *
> > > + * NOTE: It's assumed that the entire PHY package is either C22 or C45.
> > > + */
> > 
> > > +/*
> > > + * phy_package_write_mmd - Convenience function for writing a register
> > > + * on an MMD on a given PHY using the PHY package base addr, added of
> > > + * the addr_offset value.
> > > + */
> > > +int phy_package_write_mmd(struct phy_device *phydev,
> > > +			  unsigned int addr_offset, int devad,
> > > +			  u32 regnum, u16 val);
> > 
> > Hm, I see there's some precedent here already for this duplicated
> > semi-kdoc. It seems a bit unusual. If I was looking for kdoc and 
> > found the header one I'd probably not look at the source file at all.
> > 
> > Andrew, WDYT?
> 
> I tend to agree. These functions should be documented once in kdoc,
> and only once. I don't really care if its in the header, or the C
> code, but not both.
>

Ok just to make sure, I should keep the kdoc in the .c and drop them in
.h ? (or should I move the more complete kdoc in .c to .h and remove
kdoc in .c?)

I followed the pattern for the other API but I get they are very old
code.

-- 
	Ansuel

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Andrew Lunn]

> > I tend to agree. These functions should be documented once in kdoc,
> > and only once. I don't really care if its in the header, or the C
> > code, but not both.
> >
> 
> Ok just to make sure, I should keep the kdoc in the .c and drop them in
> .h ? (or should I move the more complete kdoc in .c to .h and remove
> kdoc in .c?)

Please put the kdoc in the header file and remove if from the .c file.

Thanks
	Andrew

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Russell King (Oracle)]

On Tue, Dec 05, 2023 at 03:54:04PM +0100, Andrew Lunn wrote:
> > > I tend to agree. These functions should be documented once in kdoc,
> > > and only once. I don't really care if its in the header, or the C
> > > code, but not both.
> > >
> > 
> > Ok just to make sure, I should keep the kdoc in the .c and drop them in
> > .h ? (or should I move the more complete kdoc in .c to .h and remove
> > kdoc in .c?)
> 
> Please put the kdoc in the header file and remove if from the .c file.

phy-core.c follows the style that the kdoc is in the .c file rather
than the header file.

I've raised this before in other subsystems, and it's suggested that
it's better to have it in the .c file. I guess the reason is that it's
more obvious that the function is documented when modifying it, so
there's a higher probability that the kdoc will get updated when the
function is altered.

I guess a question to ask is how often do people modify a function and
then check the header for any documentation?

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

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Jakub Kicinski]

On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
> I've raised this before in other subsystems, and it's suggested that
> it's better to have it in the .c file. I guess the reason is that it's
> more obvious that the function is documented when modifying it, so
> there's a higher probability that the kdoc will get updated when the
> function is altered.

Plus I think people using IDEs (i.e. not me) may use the "jump to
definition" functionality, to find the doc? 

TBH I thought putting kdoc in the C source was documented in the coding
style, but I can't find any mention of it now.

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Russell King (Oracle)]

On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote:
> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
> > I've raised this before in other subsystems, and it's suggested that
> > it's better to have it in the .c file. I guess the reason is that it's
> > more obvious that the function is documented when modifying it, so
> > there's a higher probability that the kdoc will get updated when the
> > function is altered.
> 
> Plus I think people using IDEs (i.e. not me) may use the "jump to
> definition" functionality, to find the doc? 
> 
> TBH I thought putting kdoc in the C source was documented in the coding
> style, but I can't find any mention of it now.

Well, in Documentation/doc-guide/kernel-doc.rst:

  The function and type kernel-doc comments should be placed just before
  the function or type being described in order to maximise the chance
  that somebody changing the code will also change the documentation.

That implies (but not explicitly) that it should be at the function
definition site, since "changing the code" is used as an argument as
I did in my previous email.

Secondly, this document goes on to give an example of running
scripts/kernel-doc on a .c file.

Thirdly, there are seven references in this document of kernel-doc
in .c files, and only one for kernel-doc in a .h file. So this suggests
that "it will be in a .c file" isn't a rule (it can't be because of
documenting structures!)

So let's not get hung up on whether it should be in .c or .h because I
think that isn't relevant. Instead, I think it's about "it should be at
the definition site" - that being a structure definition or a function
definition, and not at a function prototype.

The only exception I can think of is the style I've used in
linux/phylink.h for the _method_ definitions which look like function
prototypes - that's just a work-around because one can't kernel-doc
the structure-of-function-pointers and document the function parameters
without jumping through that hoop, and it would be silly to document
the methods in some random driver!

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

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Jeff Johnson]

On 12/5/2023 8:11 AM, Russell King (Oracle) wrote:
> On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote:
>> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
>>> I've raised this before in other subsystems, and it's suggested that
>>> it's better to have it in the .c file. I guess the reason is that it's
>>> more obvious that the function is documented when modifying it, so
>>> there's a higher probability that the kdoc will get updated when the
>>> function is altered.
>>
>> Plus I think people using IDEs (i.e. not me) may use the "jump to
>> definition" functionality, to find the doc? 
>>
>> TBH I thought putting kdoc in the C source was documented in the coding
>> style, but I can't find any mention of it now.
> 
> Well, in Documentation/doc-guide/kernel-doc.rst:
> 
>   The function and type kernel-doc comments should be placed just before
>   the function or type being described in order to maximise the chance
>   that somebody changing the code will also change the documentation.
> 
> That implies (but not explicitly) that it should be at the function
> definition site, since "changing the code" is used as an argument as
> I did in my previous email.
> 
> Secondly, this document goes on to give an example of running
> scripts/kernel-doc on a .c file.
> 
> Thirdly, there are seven references in this document of kernel-doc
> in .c files, and only one for kernel-doc in a .h file. So this suggests
> that "it will be in a .c file" isn't a rule (it can't be because of
> documenting structures!)
> 
> So let's not get hung up on whether it should be in .c or .h because I
> think that isn't relevant. Instead, I think it's about "it should be at
> the definition site" - that being a structure definition or a function
> definition, and not at a function prototype.
> 
> The only exception I can think of is the style I've used in
> linux/phylink.h for the _method_ definitions which look like function
> prototypes - that's just a work-around because one can't kernel-doc
> the structure-of-function-pointers and document the function parameters
> without jumping through that hoop, and it would be silly to document
> the methods in some random driver!
> 

The Linux Kernel philosophy of documenting functions instead of
prototypes has always bothered me since I'm "old school" and am
ingrained with the software engineering philosophy that you document
interfaces, not implementations. This was reinforced early in my career
by working on multiple projects in different programming languages using
processes outlined in DOD-STD-2167A, and for some projects, especially
ones written in Ada, the header files were the design and the documentation.

This philosophy was further enforced when working with closed source
projects (Windows, IOS, VxWorks) where all the documentation was
contained in shared header files.

So in my experience a function prototype IS the function definition, and
the actual function is just the implementation of that definition.

But that thinking obviously isn't shared by others.

/jeff

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Russell King (Oracle)]

On Tue, Dec 05, 2023 at 09:44:05AM -0800, Jeff Johnson wrote:
> On 12/5/2023 8:11 AM, Russell King (Oracle) wrote:
> > On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote:
> >> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
> >>> I've raised this before in other subsystems, and it's suggested that
> >>> it's better to have it in the .c file. I guess the reason is that it's
> >>> more obvious that the function is documented when modifying it, so
> >>> there's a higher probability that the kdoc will get updated when the
> >>> function is altered.
> >>
> >> Plus I think people using IDEs (i.e. not me) may use the "jump to
> >> definition" functionality, to find the doc? 
> >>
> >> TBH I thought putting kdoc in the C source was documented in the coding
> >> style, but I can't find any mention of it now.
> > 
> > Well, in Documentation/doc-guide/kernel-doc.rst:
> > 
> >   The function and type kernel-doc comments should be placed just before
> >   the function or type being described in order to maximise the chance
> >   that somebody changing the code will also change the documentation.
> > 
> > That implies (but not explicitly) that it should be at the function
> > definition site, since "changing the code" is used as an argument as
> > I did in my previous email.
> > 
> > Secondly, this document goes on to give an example of running
> > scripts/kernel-doc on a .c file.
> > 
> > Thirdly, there are seven references in this document of kernel-doc
> > in .c files, and only one for kernel-doc in a .h file. So this suggests
> > that "it will be in a .c file" isn't a rule (it can't be because of
> > documenting structures!)
> > 
> > So let's not get hung up on whether it should be in .c or .h because I
> > think that isn't relevant. Instead, I think it's about "it should be at
> > the definition site" - that being a structure definition or a function
> > definition, and not at a function prototype.
> > 
> > The only exception I can think of is the style I've used in
> > linux/phylink.h for the _method_ definitions which look like function
> > prototypes - that's just a work-around because one can't kernel-doc
> > the structure-of-function-pointers and document the function parameters
> > without jumping through that hoop, and it would be silly to document
> > the methods in some random driver!
> > 
> 
> The Linux Kernel philosophy of documenting functions instead of
> prototypes has always bothered me since I'm "old school" and am
> ingrained with the software engineering philosophy that you document
> interfaces, not implementations. This was reinforced early in my career
> by working on multiple projects in different programming languages using
> processes outlined in DOD-STD-2167A, and for some projects, especially
> ones written in Ada, the header files were the design and the documentation.
> 
> This philosophy was further enforced when working with closed source
> projects (Windows, IOS, VxWorks) where all the documentation was
> contained in shared header files.
> 
> So in my experience a function prototype IS the function definition, and
> the actual function is just the implementation of that definition.
> 
> But that thinking obviously isn't shared by others.

Interestingly, the view that a function prototype is a function
definition does not seem to be shared by w3school, Microsoft, IBM,
and many more.

If we look at the C99 standard, then 6.9.1 Function definitions gives
the syntax as including a compound-statement, which is defined as
requiring the curley braces and contents. Therefore, a function
definition as defined by the C standard includes its body.

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

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Jeff Johnson]

On 12/5/2023 10:14 AM, Russell King (Oracle) wrote:
> On Tue, Dec 05, 2023 at 09:44:05AM -0800, Jeff Johnson wrote:
>> So in my experience a function prototype IS the function definition, and
>> the actual function is just the implementation of that definition.
>>
>> But that thinking obviously isn't shared by others.
> 
> Interestingly, the view that a function prototype is a function
> definition does not seem to be shared by w3school, Microsoft, IBM,
> and many more.
> 
> If we look at the C99 standard, then 6.9.1 Function definitions gives
> the syntax as including a compound-statement, which is defined as
> requiring the curley braces and contents. Therefore, a function
> definition as defined by the C standard includes its body.
> 

Note I was speaking in terms of functional languages in general, not C
specifically. Perhaps I should have used the term "specification"
instead of "definition" (which would align with the Ada terminology).

Having worked with closed-source systems, especially VxWorks, for many
years (where the header files contain all the documentation), it just
seems strange to embed the documentation in the .c files.

/jeff

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Andrew Lunn]

> Having worked with closed-source systems, especially VxWorks, for many
> years (where the header files contain all the documentation), it just
> seems strange to embed the documentation in the .c files.

The key words here might be closed-source. With such black boxes, you
don't have access the sources. You cannot look at the source to
understand how a function works. In the open source world, the
comments partially function as an introduction to reading the code and
understanding what it does. You are also encouraged to change the code
if needed, which in the closed source world you cannot do.

Given this discussion, i now think putting the documentation in the .c
file makes more sense. For the generated documentation it does not
matter, but for the reader of the code, having it in the .c files does
seem to make sense.

     Andrew