| Message ID | 20200505132905.10276-6-calvin.johnson@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Introduce new fwnode based APIs to support phylink and phy layers | expand |
On Tue, May 5, 2020 at 4:30 PM Calvin Johnson <calvin.johnson@oss.nxp.com> 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; > +}
On Tue, May 05, 2020 at 05:22:00PM +0300, Andy Shevchenko wrote: > On Tue, May 5, 2020 at 4:30 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> 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 <calvin.johnson@oss.nxp.com> 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 > > <calvin.johnson@oss.nxp.com> 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.
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
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 <calvin.johnson@oss.nxp.com> --- 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(+)