Re: [net-next PATCH v2 0/3] Introduce new APIs to support phylink and phy layers

[Russell King - ARM Linux admin]

On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote:
> On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote:
> > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote:
> > > Following functions are defined:
> > >   phylink_fwnode_phy_connect()
> > >   phylink_device_phy_connect()
> > >   fwnode_phy_find_device()
> > >   device_phy_find_device()
> > >   fwnode_get_phy_node()
> > > 
> > > First two help in connecting phy to phylink instance.
> > > Next two help in finding a phy on a mdiobus.
> > > Last one helps in getting phy_node from a fwnode.
> > > 
> > > Changes in v2:
> > >   move phy code from base/property.c to net/phy/phy_device.c
> > >   replace acpi & of code to get phy-handle with fwnode_find_reference
> > >   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> > > 
> > > Calvin Johnson (3):
> > >   device property: Introduce phy related fwnode functions
> > >   net: phy: alphabetically sort header includes
> > >   phylink: Introduce phylink_fwnode_phy_connect()
> > 
> > Thanks for this, but there's more work that needs to be done here.  I
> > also think that we must have an ack from ACPI people before this can be
> > accepted - you are in effect proposing a new way for representing PHYs
> > in ACPI.
> 
> Thanks for your review.
> 
> Agree that we need an ack from ACPI people.
> However, I don't think it is a completely new way as similar acpi approach to
> get phy-handle is already in place.
> Please see this:
> https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832

That was added by:

commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34
Author: Iyappan Subramanian <isubramanian@xxxxxxx>
Date:   Mon Jul 25 17:12:41 2016 -0700

    drivers: net: xgene: Add backward compatibility

    This patch adds xgene_enet_check_phy_hanlde() function that checks whether
    MDIO driver is probed successfully and sets pdata->mdio_driver to true.
    If MDIO driver is not probed, ethernet driver falls back to backward
    compatibility mode.

    Since enum xgene_enet_cmd is used by MDIO driver, removing this from
    ethernet driver.

    Signed-off-by: Iyappan Subramanian <isubramanian@xxxxxxx>
    Tested-by: Fushen Chen <fchen@xxxxxxx>
    Tested-by: Toan Le <toanle@xxxxxxx>
    Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>

The commit message says nothing about adding ACPI stuff, and searching
the 'net for the posting of this patch seems to suggest that it wasn't
obviously copied to any ACPI people:

    https://lists.openwall.net/netdev/2016/07/26/11

Annoyingly, searching for:

    "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org

doesn't find it on lore, so can't get the full headers and therefore
addresses.

So, yes, there's another driver using it, but the ACPI folk probably
never got a look-in on that instance.  Even if they had been copied,
the patch description is probably sufficiently poor that they wouldn't
have read the patch.

I'd say there's questions over whether ACPI people will find this an
acceptable approach.

Given that your patch moves this from one driver to a subsystem thing,
it needs to be ratified by ACPI people, because it's effectively
becoming a standardised way to represent a PHY in ACPI.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up