| Message ID | 20201215164315.3666-7-calvin.johnson@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 846 this patch: 839 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis WARNING: line length of 87 exceeds 80 columns |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 746 this patch: 739 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Dec 15, 2020 at 6:44 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 mii_timestamper *mii_ts; > + struct phy_device *phy; > + const char *cp; > + bool is_c45; > + u32 phy_id; > + int rc; > + if (is_of_node(child)) { > + mii_ts = of_find_mii_timestamper(to_of_node(child)); > + if (IS_ERR(mii_ts)) > + return PTR_ERR(mii_ts); > + } Perhaps mii_ts = of_find_mii_timestamper(to_of_node(child)); > + > + rc = fwnode_property_read_string(child, "compatible", &cp); > + is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45")); > + > + if (is_c45 || fwnode_get_phy_id(child, &phy_id)) > + phy = get_phy_device(bus, addr, is_c45); > + else > + phy = phy_device_create(bus, addr, phy_id, 0, NULL); > + if (IS_ERR(phy)) { > + if (mii_ts && is_of_node(child)) > + unregister_mii_timestamper(mii_ts); if (!IS_ERR_OR_NULL(mii_ts)) ... However it points to the question why unregister() doesn't handle the above cases. I would expect unconditional call to unregister() here. > + return PTR_ERR(phy); > + } > + > + if (is_acpi_node(child)) { > + 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(phy->mdio.dev.fwnode); > + return rc; > + } > + > + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); > + } else if (is_of_node(child)) { > + rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr); > + if (rc) { > + if (mii_ts) > + unregister_mii_timestamper(mii_ts); Ditto. > + phy_device_free(phy); > + return rc; > + } > + > + /* phy->mii_ts may already be defined by the PHY driver. A > + * mii_timestamper probed via the device tree will still have > + * precedence. > + */ > + if (mii_ts) > + phy->mii_ts = mii_ts; How is that defined? Do you need to do something with an old pointer? > + } > + return 0; > +}
On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote: > On Tue, Dec 15, 2020 at 6:44 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 mii_timestamper *mii_ts; > > + struct phy_device *phy; > > + const char *cp; > > + bool is_c45; > > + u32 phy_id; > > + int rc; > > > + if (is_of_node(child)) { > > + mii_ts = of_find_mii_timestamper(to_of_node(child)); > > + if (IS_ERR(mii_ts)) > > + return PTR_ERR(mii_ts); > > + } > > Perhaps > > mii_ts = of_find_mii_timestamper(to_of_node(child)); > > > + > > + rc = fwnode_property_read_string(child, "compatible", &cp); > > + is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45")); > > + > > + if (is_c45 || fwnode_get_phy_id(child, &phy_id)) > > + phy = get_phy_device(bus, addr, is_c45); > > + else > > + phy = phy_device_create(bus, addr, phy_id, 0, NULL); > > + if (IS_ERR(phy)) { > > > + if (mii_ts && is_of_node(child)) > > + unregister_mii_timestamper(mii_ts); > > if (!IS_ERR_OR_NULL(mii_ts)) > ... > > However it points to the question why unregister() doesn't handle the > above cases. > I would expect unconditional call to unregister() here. This is following the logic defined in of_mdiobus_register_phy(). https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/of_mdio.c#L107 mii_ts = of_find_mii_timestamper(child); if (IS_ERR(mii_ts)) return PTR_ERR(mii_ts); I think the logic above is correct because this function returns only if of_find_mii_timestamper() returns error. On NULL return, it proceeds further. > > > + return PTR_ERR(phy); > > + } > > + > > + if (is_acpi_node(child)) { > > + 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(phy->mdio.dev.fwnode); > > + return rc; > > + } > > + > > + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); > > + } else if (is_of_node(child)) { > > + rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr); > > + if (rc) { > > > + if (mii_ts) > > + unregister_mii_timestamper(mii_ts); > > Ditto. > > > + phy_device_free(phy); > > + return rc; > > + } > > + > > + /* phy->mii_ts may already be defined by the PHY driver. A > > + * mii_timestamper probed via the device tree will still have > > + * precedence. > > + */ > > > + if (mii_ts) > > + phy->mii_ts = mii_ts; > > How is that defined? Do you need to do something with an old pointer? As the comment says, I think PHY drivers which got invoked before calling of_mdiobus_register_phy() may have defined phy->mii_ts. > > > + } > > + return 0; > > +} > > -- > With Best Regards, > Andy Shevchenko
On Fri, Dec 18, 2020 at 7:34 AM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: ... > > > + /* phy->mii_ts may already be defined by the PHY driver. A > > > + * mii_timestamper probed via the device tree will still have > > > + * precedence. > > > + */ > > > > > + if (mii_ts) > > > + phy->mii_ts = mii_ts; > > > > How is that defined? Do you need to do something with an old pointer? > > As the comment says, I think PHY drivers which got invoked before calling > of_mdiobus_register_phy() may have defined phy->mii_ts. What I meant here is that the old pointer might need some care (freeing?). > > > + }
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 2b42e46066b4..3361a1a86e97 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -106,6 +106,72 @@ 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 mii_timestamper *mii_ts; + struct phy_device *phy; + const char *cp; + bool is_c45; + u32 phy_id; + int rc; + + if (is_of_node(child)) { + mii_ts = of_find_mii_timestamper(to_of_node(child)); + if (IS_ERR(mii_ts)) + return PTR_ERR(mii_ts); + } + + rc = fwnode_property_read_string(child, "compatible", &cp); + is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45")); + + if (is_c45 || fwnode_get_phy_id(child, &phy_id)) + phy = get_phy_device(bus, addr, is_c45); + else + phy = phy_device_create(bus, addr, phy_id, 0, NULL); + if (IS_ERR(phy)) { + if (mii_ts && is_of_node(child)) + unregister_mii_timestamper(mii_ts); + return PTR_ERR(phy); + } + + if (is_acpi_node(child)) { + 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(phy->mdio.dev.fwnode); + return rc; + } + + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); + } else if (is_of_node(child)) { + rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr); + if (rc) { + if (mii_ts) + unregister_mii_timestamper(mii_ts); + phy_device_free(phy); + return rc; + } + + /* phy->mii_ts may already be defined by the PHY driver. A + * mii_timestamper probed via the device tree will still have + * precedence. + */ + if (mii_ts) + phy->mii_ts = mii_ts; + } + 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 dbd69b3d170b..f138ec333725 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -368,6 +368,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 v2: None drivers/net/phy/mdio_bus.c | 66 ++++++++++++++++++++++++++++++++++++++ include/linux/mdio.h | 2 ++ 2 files changed, 68 insertions(+)