Re: [net-next PATCH v7 11/16] net: mdio: Add ACPI support code for mdio

[Calvin Johnson]

On Thu, Mar 11, 2021 at 02:14:37PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 11, 2021 at 8:22 AM Calvin Johnson
> <calvin.johnson@xxxxxxxxxxx> wrote:
> >
> > Define acpi_mdiobus_register() to Register mii_bus and create PHYs for
> > each ACPI child node.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@xxxxxxxxxxx>
> > ---
> >
> > Changes in v7:
> > - Include headers directly used in acpi_mdio.c
> >
> > Changes in v6:
> > - use GENMASK() and ACPI_COMPANION_SET()
> > - some cleanup
> > - remove unwanted header inclusion
> >
> > Changes in v5:
> > - add missing MODULE_LICENSE()
> > - replace fwnode_get_id() with OF and ACPI function calls
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> >  MAINTAINERS                  |  1 +
> >  drivers/net/mdio/Kconfig     |  7 +++++
> >  drivers/net/mdio/Makefile    |  1 +
> >  drivers/net/mdio/acpi_mdio.c | 56 ++++++++++++++++++++++++++++++++++++
> >  include/linux/acpi_mdio.h    | 25 ++++++++++++++++
> >  5 files changed, 90 insertions(+)
> >  create mode 100644 drivers/net/mdio/acpi_mdio.c
> >  create mode 100644 include/linux/acpi_mdio.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 146de41d2656..051377b7fa94 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6680,6 +6680,7 @@ F:        Documentation/devicetree/bindings/net/mdio*
> >  F:     Documentation/devicetree/bindings/net/qca,ar803x.yaml
> >  F:     Documentation/networking/phy.rst
> >  F:     drivers/net/mdio/
> > +F:     drivers/net/mdio/acpi_mdio.c
> >  F:     drivers/net/mdio/fwnode_mdio.c
> >  F:     drivers/net/mdio/of_mdio.c
> >  F:     drivers/net/pcs/
> > diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
> > index 2d5bf5ccffb5..fc8c787b448f 100644
> > --- a/drivers/net/mdio/Kconfig
> > +++ b/drivers/net/mdio/Kconfig
> > @@ -36,6 +36,13 @@ config OF_MDIO
> >         help
> >           OpenFirmware MDIO bus (Ethernet PHY) accessors
> >
> > +config ACPI_MDIO
> > +       def_tristate PHYLIB
> 
> > +       depends on ACPI
> > +       depends on PHYLIB
> 
> Same issue, they are no-ops.
> 
> I guess you have to surround OF and ACPI and FWNODE variants by
> 
> if PHYLIB
> ...
> endif
> 
> This will be an equivalent to depends on PHYLIB
> 
> and put this into Makefile
> 
> ifneq ($(CONFIG_ACPI),)
> obj-$(CONFIG_PHYLIB) += acpi_mdio.o
> endif
> 
> This will give you the rest, i.e. default PHYLIB + depends on ACPI
> 
> Similar for OF
> 

I checked the effect of y/n/m choice of PHYLIB on FWNODE_MDIO.
As expected with def_tristate, whenever PHYLIB changes to y/n/m corresponding
change is reflected on FWNODE_MDIO, also considering the state of other
depending CONFIGS like OF and ACPI.

Symbol: FWNODE_MDIO [=n]
│
  │ Type  : tristate
│
  │ Defined at drivers/net/mdio/Kconfig:22
│
  │   Depends on: NETDEVICES [=y] && MDIO_DEVICE [=y] && ACPI [=y] && OF [=y] &&
PHYLIB [=n]                                                          │
  │ Selects: FIXED_PHY [=n]

Effect is similar for ACPI_MDIO and OF_MDIO.

So instead of above proposed method, I think what you proposed in your earlier
email, i.e, "depends on (ACPI || OF) || COMPILE_TEST" seems to be better for
FWNODE_MDIO.

Shall we go ahead with this?

Regards
Calvin

> > +       help
> > +         ACPI MDIO bus (Ethernet PHY) accessors
> > +
> >  if MDIO_BUS
> >
> >  config MDIO_DEVRES
> > diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
> > index ea5390e2ef84..e8b739a3df1c 100644
> > --- a/drivers/net/mdio/Makefile
> > +++ b/drivers/net/mdio/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  # Makefile for Linux MDIO bus drivers
> >
> > +obj-$(CONFIG_ACPI_MDIO)                += acpi_mdio.o
> >  obj-$(CONFIG_FWNODE_MDIO)      += fwnode_mdio.o
> >  obj-$(CONFIG_OF_MDIO)          += of_mdio.o
> >
> > diff --git a/drivers/net/mdio/acpi_mdio.c b/drivers/net/mdio/acpi_mdio.c
> > new file mode 100644
> > index 000000000000..60a86e3fc246
> > --- /dev/null
> > +++ b/drivers/net/mdio/acpi_mdio.c
> > @@ -0,0 +1,56 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ACPI helpers for the MDIO (Ethernet PHY) API
> > + *
> > + * This file provides helper functions for extracting PHY device information
> > + * out of the ACPI ASL and using it to populate an mii_bus.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/acpi_mdio.h>
> > +#include <linux/bits.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/fwnode_mdio.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +
> > +MODULE_AUTHOR("Calvin Johnson <calvin.johnson@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > +
> > +/**
> > + * acpi_mdiobus_register - Register mii_bus and create PHYs from the ACPI ASL.
> > + * @mdio: pointer to mii_bus structure
> > + * @fwnode: pointer to fwnode of MDIO bus.
> > + *
> > + * This function registers the mii_bus structure and registers a phy_device
> > + * for each child node of @fwnode.
> > + */
> > +int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> > +{
> > +       struct fwnode_handle *child;
> > +       u32 addr;
> > +       int ret;
> > +
> > +       /* Mask out all PHYs from auto probing. */
> > +       mdio->phy_mask = GENMASK(31, 0);
> > +       ret = mdiobus_register(mdio);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ACPI_COMPANION_SET(&mdio->dev, to_acpi_device_node(fwnode));
> > +
> > +       /* Loop over the child nodes and register a phy_device for each PHY */
> > +       fwnode_for_each_child_node(fwnode, child) {
> > +               ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > +               if (ret || addr >= PHY_MAX_ADDR)
> > +                       continue;
> > +
> > +               ret = fwnode_mdiobus_register_phy(mdio, child, addr);
> > +               if (ret == -ENODEV)
> > +                       dev_err(&mdio->dev,
> > +                               "MDIO device at address %d is missing.\n",
> > +                               addr);
> > +       }
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(acpi_mdiobus_register);
> > diff --git a/include/linux/acpi_mdio.h b/include/linux/acpi_mdio.h
> > new file mode 100644
> > index 000000000000..748d261fe2f9
> > --- /dev/null
> > +++ b/include/linux/acpi_mdio.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * ACPI helper for the MDIO (Ethernet PHY) API
> > + */
> > +
> > +#ifndef __LINUX_ACPI_MDIO_H
> > +#define __LINUX_ACPI_MDIO_H
> > +
> > +#include <linux/phy.h>
> > +
> > +#if IS_ENABLED(CONFIG_ACPI_MDIO)
> > +int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode);
> > +#else /* CONFIG_ACPI_MDIO */
> > +static inline int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> > +{
> > +       /*
> > +        * Fall back to mdiobus_register() function to register a bus.
> > +        * This way, we don't have to keep compat bits around in drivers.
> > +        */
> > +
> > +       return mdiobus_register(mdio);
> > +}
> > +#endif
> > +
> > +#endif /* __LINUX_ACPI_MDIO_H */
> > --
> > 2.17.1
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko