On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson <calvin.johnson@xxxxxxxxxxx> 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; > +} -- With Best Regards, Andy Shevchenko