On Sat, Aug 1, 2020 at 3:08 PM Russell King - ARM Linux admin <li...@armlinux.org.uk> wrote: > > On Sat, Aug 01, 2020 at 10:23:38AM +0530, Vikas Singh wrote: > > Hi Andrew, > > > > As i have already mentioned that this patch is based on > > https://www.spinics.net/lists/netdev/msg662173.html, > > <https://www.spinics.net/lists/netdev/msg662173.html> > > > > When MDIO bus gets registered itself along with devices on it , the > > function mdiobus_register() inside of_mdiobus_register(), brings > > up all the PHYs on the mdio bus and attach them to the bus with the help > > of_mdiobus_link_mdiodev() inside mdiobus_scan() . > > Additionally it has been discussed with the maintainers that the > > mdiobus_register() function should be capable of handling both ACPI & DTB > > stuff > > without any change to existing implementation. > > Now of_mdiobus_link_mdiodev() inside mdiobus_scan() see if the auto-probed > > phy has a corresponding child in the bus node, and set the "of_node" > > pointer in DT case. > > But lacks to set the "fwnode" pointer in ACPI case which is resulting in > > mdiobus_register() failure as an end result theoretically. > > > > Now this patch set (changes) will attempt to fill this gap and generalise > > the mdiobus_register() implementation for both ACPI & DT with no duplicacy > > or redundancy. > > Please do not top-post. > > What Andrew is asking is why _should_ a DT specific function (which > starts with of_, meaning "open firmware") have anything to do with > ACPI. The function you refer to (of_mdiobus_link_mdiodev()) is only > built when CONFIG_OF_MDIO is enabled, which is again, a DT specific > thing. > > So, why should a DT specific function care about ACPI? > > We're not asking about the fine details, we're asking about the high > level idea that DT functions should know about ACPI. > > The assignment in of_mdiobus_link_mdiodev() to dev->fwnode is not > itself about ACPI, it's about enabling drivers that wish to access > DT properties through the fwnode property APIs can do so. > > IMHO, the right way to go about this is to implement it as a non-DT > function. Given that it is a static function, Andrew may find it > acceptable if you also renamed of_mdiobus_link_mdiodev() as > mdiobus_link_mdiodev() and moved it out of the #ifdef. > > + bus->dev.fwnode = bus->parent->fwnode; > > That should be done elsewhere, not here. of_mdiobus_register() already > ensures that this is appropriately set, and if it isn't, maybe there's > a bug elsewhere. > > Lastly, note that you don't need two loops, one for ACPI and one for > DT (it's a shame there isn't a device_for_each_available_child_node()): > > int addr; > > if (dev->fwnode && !bus->dev.fwnode) > return; > > device_for_each_child_node(&bus->dev, fwnode) { > if (!fwnode_device_is_available(fwnode)) > continue; > > if (is_of_node(fwnode)) > addr = of_mdio_parse_addr(dev, to_of_node(fwnode)); > else if (fwnode_property_read_u32(fwnode, "reg", &addr)) > continue; > > if (addr == mdiodev->addr) { > dev->of_node = to_of_node(fwnode); > dev->fwnode = fwnode; > return; > } > } > > which, I think, will behave identically to the existing implementation > when called in a DT setup, but should also add what you want. > > So, maybe with the above, moving it out from under the ifdef, and > renaming it _may_ be acceptable. This is just a suggestion. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi Russell, Apologies for the late response, I was stuck with other critical stuff !! Thank you so much. Agreed. I will do the suggested changes and send a V2 patch shortly. Thnx !!
