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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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!