diff mbox series

[net-next,v4,4/6] net: phy: introduce phy_find_by_fwnode()

Message ID 20200709175722.5228-5-calvin.johnson@oss.nxp.com (mailing list archive)
State Superseded, archived
Headers show
Series [net-next,v4,1/6] Documentation: ACPI: DSD: Document MDIO PHY | expand

Commit Message

Calvin Johnson July 9, 2020, 5:57 p.m. UTC
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(+)

Comments

Russell King (Oracle) July 9, 2020, 6:14 p.m. UTC | #1
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.
Andy Shevchenko July 9, 2020, 8:43 p.m. UTC | #2
On Thu, Jul 9, 2020 at 8:57 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> 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.

...

> +struct phy_device *phy_find_by_fwnode(struct fwnode_handle *fwnode)
> +{
> +       struct fwnode_handle *fwnode_mdio;
> +       struct mii_bus *mdio;
> +       int addr;
> +       int err;
> +
> +       fwnode_mdio = fwnode_find_reference(fwnode, "mdio-handle", 0);
> +       mdio = fwnode_mdio_find_bus(fwnode_mdio);
> +       fwnode_handle_put(fwnode_mdio);
> +       if (!mdio)
> +               return ERR_PTR(-ENODEV);

> +       err = fwnode_property_read_u32(fwnode, "phy-channel", &addr);
> +       if (err < 0 || addr < 0 || addr >= PHY_MAX_ADDR)
> +               return ERR_PTR(-EINVAL);

I'm wondering why this is written like above and not like below

err = ...
if (err)
  return ERR_PTR(err);
if (...)
 return ERR_PTR(-E...);

?

> +       return mdiobus_get_phy(mdio, addr);
> +}
diff mbox series

Patch

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)
+{
+	struct fwnode_handle *fwnode_mdio;
+	struct mii_bus *mdio;
+	int addr;
+	int err;
+
+	fwnode_mdio = fwnode_find_reference(fwnode, "mdio-handle", 0);
+	mdio = fwnode_mdio_find_bus(fwnode_mdio);
+	fwnode_handle_put(fwnode_mdio);
+	if (!mdio)
+		return ERR_PTR(-ENODEV);
+
+	err = fwnode_property_read_u32(fwnode, "phy-channel", &addr);
+	if (err < 0 || addr < 0 || addr >= PHY_MAX_ADDR)
+		return ERR_PTR(-EINVAL);
+
+	return mdiobus_get_phy(mdio, addr);
+}
+EXPORT_SYMBOL(phy_find_by_fwnode);
+
 static void phy_link_change(struct phy_device *phydev, bool up)
 {
 	struct net_device *netdev = phydev->attached_dev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0403eb799913..a2ec1c288db0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -334,6 +334,7 @@  static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
 }
 
 struct mii_bus *mdio_find_bus(const char *mdio_name);
+struct mii_bus *fwnode_mdio_find_bus(struct fwnode_handle *mdio_bus_fwnode);
 struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
 
 #define PHY_INTERRUPT_DISABLED	false
@@ -1245,6 +1246,7 @@  int phy_sfp_probe(struct phy_device *phydev,
 struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
 			      phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
+struct phy_device *phy_find_by_fwnode(struct fwnode_handle *fwnode);
 int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface);
 int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,