Re: [PATCH] net: Phy: Add PHY lookup support on MDIO bus in case of ACPI probe

[Vikas Singh <vikas.singh@puresoftware.com>]

On Sat, Aug 1, 2020 at 3:08 PM Russell King - ARM Linux admin
<linux@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 !!