Re: [net-next PATCH v4 4/6] net: phy: introduce phy_find_by_fwnode() - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Calvin Johnson <calvin.johnson@oss.nxp.com>
Cc: Jeremy Linton <jeremy.linton@arm.com>, Jon <jon@solid-run.com>,
	Cristi Sovaiala <cristian.sovaiala@nxp.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Madalin Bucur <madalin.bucur@oss.nxp.com>,
	linux-acpi@vger.kernel.org, linux.cj@gmail.com,
	netdev@vger.kernel.org
Subject: Re: [net-next PATCH v4 4/6] net: phy: introduce phy_find_by_fwnode()
Date: Thu, 9 Jul 2020 19:14:36 +0100	[thread overview]
Message-ID: <20200709181435.GH1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <20200709175722.5228-5-calvin.johnson@oss.nxp.com>

On Thu, Jul 09, 2020 at 11:27:20PM +0530, Calvin Johnson wrote:
> The PHYs on an mdiobus are probed and registered using mdiobus_register().
> Later, for connecting these PHYs to MAC, the PHYs registered on the
> mdiobus have to be referenced.
> 
> For each MAC node, a property "mdio-handle" is used to reference the
> MDIO bus on which the PHYs are registered. On getting hold of the MDIO
> bus, use phy_find_by_fwnode() to get the PHY connected to the MAC.
> 
> Introduce fwnode_mdio_find_bus() to find the mii_bus that corresponds
> to given mii_bus fwnode.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> 
> ---
> 
> Changes in v4:
> - release fwnode_mdio after use
> - return ERR_PTR instead of NULL
> 
> Changes in v3:
> - introduce fwnode_mdio_find_bus()
> - renamed and improved phy_find_by_fwnode()
> 
> Changes in v2: None
> 
>  drivers/net/phy/mdio_bus.c   | 25 +++++++++++++++++++++++++
>  drivers/net/phy/phy_device.c | 22 ++++++++++++++++++++++
>  include/linux/phy.h          |  2 ++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 3c2749e84f74..dcac8cd8f5cd 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -435,6 +435,31 @@ struct mii_bus *of_mdio_find_bus(struct device_node *mdio_bus_np)
>  }
>  EXPORT_SYMBOL(of_mdio_find_bus);
>  
> +/**
> + * fwnode_mdio_find_bus - Given an mii_bus fwnode, find the mii_bus.
> + * @mdio_bus_fwnode: fwnode of the mii_bus.
> + *
> + * Returns a reference to the mii_bus, or NULL if none found.  The
> + * embedded struct device will have its reference count incremented,
> + * and this must be put once the bus is finished with.
> + *
> + * Because the association of a fwnode and mii_bus is made via
> + * mdiobus_register(), the mii_bus cannot be found before it is
> + * registered with mdiobus_register().
> + *
> + */
> +struct mii_bus *fwnode_mdio_find_bus(struct fwnode_handle *mdio_bus_fwnode)
> +{
> +	struct device *d;
> +
> +	if (!mdio_bus_fwnode)
> +		return NULL;
> +
> +	d = class_find_device_by_fwnode(&mdio_bus_class, mdio_bus_fwnode);
> +	return d ? to_mii_bus(d) : NULL;
> +}
> +EXPORT_SYMBOL(fwnode_mdio_find_bus);
> +
>  /* Walk the list of subnodes of a mdio bus and look for a node that
>   * matches the mdio device's address with its 'reg' property. If
>   * found, set the of_node pointer for the mdio device. This allows
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 7cda95330aea..97a25397348c 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -25,6 +25,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/phy.h>
>  #include <linux/phy_led_triggers.h>
> +#include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/sfp.h>
>  #include <linux/skbuff.h>
> @@ -964,6 +965,27 @@ struct phy_device *phy_find_first(struct mii_bus *bus)
>  }
>  EXPORT_SYMBOL(phy_find_first);
>  
> +struct phy_device *phy_find_by_fwnode(struct fwnode_handle *fwnode)

This should be documented, and I'm not sure that the name is a
particularly good idea.  The way I read this name leads me to believe
that the "fwnode" passed in is the fwnode for the PHY device itself,
rather than something that contains the reference information to lookup
the PHY device.

In other words, it leads me (incorrectly) down the path of assuming
that this function is a fwnode variant of of_phy_find_device().

Since fwnodes cover both ACPI and DT, I think, as this does not
implement the recognised DT style of describing a PHY, it really
should error out if the fwnode is a DT node to prevent it becoming
an unintended DT binding.

Thanks.

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