Introduce fwnode_mdiobus_register_phy() to register PHYs on the
mdiobus. From the compatible string, identify whether the PHY is
c45 and based on this create a PHY device instance which is
registered on the mdiobus.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v3:
Introduce two functions to register phy to mdiobus using fwnode
Changes in v2: None
drivers/net/phy/mdio_bus.c | 41 ++++++++++++++++++++++++++++++++++++++
include/linux/mdio.h | 2 ++
2 files changed, 43 insertions(+)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 3e79b96fa344..b51e597c0479 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -106,6 +106,47 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
}
EXPORT_SYMBOL(mdiobus_unregister_device);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr)
+{
+ struct phy_device *phy;
+ bool is_c45 = false;
+ const char *cp;
+ u32 phy_id;
+ int rc;
+
+ fwnode_property_read_string(child, "compatible", &cp);
+ if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
+ is_c45 = true;
+
+ if (!is_c45 && !fwnode_get_phy_id(child, &phy_id))
+ phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+ else
+ phy = get_phy_device(bus, addr, is_c45);
+ if (IS_ERR(phy))
+ return PTR_ERR(phy);
+
+ phy->irq = bus->irq[addr];
+
+ /* Associate the fwnode with the device structure so it
+ * can be looked up later.
+ */
+ phy->mdio.dev.fwnode = child;
+
+ /* All data is now stored in the phy struct, so register it */
+ rc = phy_device_register(phy);
+ if (rc) {
+ phy_device_free(phy);
+ fwnode_handle_put(child);
+ return rc;
+ }
+
+ dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+
+ return 0;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
{
struct mdio_device *mdiodev = bus->mdio_map[addr];
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 917e4bb2ed71..e55609697b42 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -330,6 +330,8 @@ int mdiobus_register_device(struct mdio_device *mdiodev);
int mdiobus_unregister_device(struct mdio_device *mdiodev);
bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr);
/**
* mdio_module_driver() - Helper macro for registering mdio drivers
--
2.17.1
On Tue, May 5, 2020 at 4:30 PM Calvin Johnson
<[email protected]> wrote:
>
> Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> mdiobus. From the compatible string, identify whether the PHY is
> c45 and based on this create a PHY device instance which is
> registered on the mdiobus.
...
> +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> + struct fwnode_handle *child, u32 addr)
> +{
> + struct phy_device *phy;
> + bool is_c45 = false;
Redundant assignment, see below.
> + const char *cp;
> + u32 phy_id;
> + int rc;
> +
> + fwnode_property_read_string(child, "compatible", &cp);
Consider rc = ...; otherwise you will have UB below.
> + if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
UB!
> + is_c45 = true;
is_c45 = !(rc || strcmp(...));
> + if (!is_c45 && !fwnode_get_phy_id(child, &phy_id))
Perhaps positive conditional
if (is_c45 || fwnode_...(...))
get_phy_device();
else
...
> + phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> + else
> + phy = get_phy_device(bus, addr, is_c45);
> + if (IS_ERR(phy))
> + return PTR_ERR(phy);
> +
> + phy->irq = bus->irq[addr];
> +
> + /* Associate the fwnode with the device structure so it
> + * can be looked up later.
> + */
> + phy->mdio.dev.fwnode = child;
> +
> + /* All data is now stored in the phy struct, so register it */
> + rc = phy_device_register(phy);
> + if (rc) {
> + phy_device_free(phy);
> + fwnode_handle_put(child);
Shouldn't mdio.dev.fwnode be put rather than child (yes, I see the assignment)
> + return rc;
> + }
> +
> + dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
On Tue, May 05, 2020 at 05:22:00PM +0300, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 4:30 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> > mdiobus. From the compatible string, identify whether the PHY is
> > c45 and based on this create a PHY device instance which is
> > registered on the mdiobus.
>
> ...
>
> > +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> > + struct fwnode_handle *child, u32 addr)
> > +{
> > + struct phy_device *phy;
>
> > + bool is_c45 = false;
>
> Redundant assignment, see below.
>
> > + const char *cp;
> > + u32 phy_id;
> > + int rc;
> > +
>
> > + fwnode_property_read_string(child, "compatible", &cp);
>
> Consider rc = ...; otherwise you will have UB below.
>
> > + if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
>
> UB!
Thanks for the comments! I've considered all of them.
What is UB, by the way? :)-
Regards
Calvin
On Thu, May 7, 2020 at 10:44 AM Calvin Johnson
<[email protected]> wrote:
>
> On Tue, May 05, 2020 at 05:22:00PM +0300, Andy Shevchenko wrote:
> > On Tue, May 5, 2020 at 4:30 PM Calvin Johnson
> > <[email protected]> wrote:
...
> > > + bool is_c45 = false;
> >
> > Redundant assignment, see below.
> >
> > > + const char *cp;
> > > + u32 phy_id;
> > > + int rc;
> > > + fwnode_property_read_string(child, "compatible", &cp);
> >
> > Consider rc = ...; otherwise you will have UB below.
> >
> > > + if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
> >
> > UB!
>
> Thanks for the comments! I've considered all of them.
> What is UB, by the way? :)-
Undefined Behaviour.
--
With Best Regards,
Andy Shevchenko