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

[Andrew Lunn]

> @@ -1648,20 +1648,27 @@ 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 addr parameter serves as cookie which has to have the same values

addr has been renamed base_addr.

> + * 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. If your PHY doesn't have global registers
> + * you can just pick any of the PHY addresses.


I would not add this last sentence. We want a clearly defined meaning
of base_addr. Its the lowest address in the package. It does not
matter if its not used, it should still be the lowest address in the
package.

> + * In some special PHY package, multiple PHY are used for global init of

I don't see why they are special.

> -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;
>  
> -	if (!shared)
> +	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
>  		return -EIO;
>  
> -	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
> +	addr = shared->base_addr + addr_offset;
> +	return mdiobus_read(phydev->mdio.bus, addr, regnum);

This might be a little bit more readable:

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 (!shared || addr > PHY_MAX_ADDR)
		return -EIO;

	return mdiobus_read(phydev->mdio.bus, addr, regnum);
}


    Andrew

---
pw-bot: cr