On Sat, Apr 18, 2020 at 12:41:16PM +0100, Russell King - ARM Linux admin wrote: > On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote: > > @@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) > > return value; > > } > > > > +/* Extract the clause 22 phy ID from the compatible string of the form > > + * ethernet-phy-idAAAA.BBBB > > This comment is incorrect. What about clause 45 PHYs? Agree. Will correct it. May be we need a comment update for of_get_phy_id() also. https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/of/of_mdio.c#L28 <snip> > > + /* 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; > > You seem to be duplicating the OF implementation in a private driver, > converting it to fwnode. This is not how we develop the Linux kernel. > We fix subsystem problems by fixing the subsystems, not by throwing > what should be subsystem code into private drivers. I've used some part of the of_mdiobus_register_phy(). Looks like some other network drivers using acpi had also taken similar approach. Anyway, I'll try to make it generic and move out to subsystem. Thanks Calvin
