Messages in this thread | | | From | Marcin Wojtas <> | Date | Thu, 24 Jun 2021 13:10:04 +0200 | Subject | Re: [net-next: PATCH v3 2/6] net: mdiobus: Introduce fwnode_mdbiobus_register() |
| |
Hi,
czw., 24 cze 2021 o 00:10 Marcin Wojtas <mw@semihalf.com> napisał(a): > > Hi, > > śr., 23 cze 2021 o 22:22 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > On Mon, Jun 21, 2021 at 07:30:24PM +0200, Marcin Wojtas wrote: > > > This patch introduces a new helper function that > > > wraps acpi_/of_ mdiobus_register() and allows its > > > usage via common fwnode_ interface. > > > > > > Fall back to raw mdiobus_register() in case CONFIG_FWNODE_MDIO > > > is not enabled, in order to satisfy compatibility > > > in all future user drivers. > > > > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > > > --- > > > include/linux/fwnode_mdio.h | 12 +++++++++++ > > > drivers/net/mdio/fwnode_mdio.c | 22 ++++++++++++++++++++ > > > 2 files changed, 34 insertions(+) > > > > > > diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h > > > index faf603c48c86..13d4ae8fee0a 100644 > > > --- a/include/linux/fwnode_mdio.h > > > +++ b/include/linux/fwnode_mdio.h > > > @@ -16,6 +16,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, > > > int fwnode_mdiobus_register_phy(struct mii_bus *bus, > > > struct fwnode_handle *child, u32 addr); > > > > > > +int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode); > > > #else /* CONFIG_FWNODE_MDIO */ > > > int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, > > > struct phy_device *phy, > > > @@ -30,6 +31,17 @@ static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus, > > > { > > > return -EINVAL; > > > } > > > + > > > +static inline int fwnode_mdiobus_register(struct mii_bus *bus, > > > + 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 > > > > I looked at this some more, and in the end i decided it was O.K. > > > > > +/** > > > + * fwnode_mdiobus_register - bring up all the PHYs on a given MDIO bus and > > > + * attach them to it. > > > + * @bus: Target MDIO bus. > > > + * @fwnode: Pointer to fwnode of the MDIO controller. > > > + * > > > + * Return values are determined accordingly to acpi_/of_ mdiobus_register() > > > + * operation. > > > + */ > > > +int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode) > > > +{ > > > + if (is_acpi_node(fwnode)) > > > + return acpi_mdiobus_register(bus, fwnode); > > > + else if (is_of_node(fwnode)) > > > + return of_mdiobus_register(bus, to_of_node(fwnode)); > > > + else > > > + return -EINVAL; > > > > I wounder if here you should call mdiobus_register(mdio), rather than > > -EINVAL? > > > > I don't have a strong opinion. > > Currently (and in foreseeable future) we support only DT/ACPI as a > firmware description, reaching the last "else" means something really > wrong. The case of lack of DT/ACPI and the fallback is handled on the > include/linux/fwnode_mdio.h level. > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > >
Unfortunately I have to withdraw this patch, as well as xgmac_mdio one. In case the fwnode_/of_/acpi_mdio are built as modules, we get a cycle dependency during depmod phase of modules_install, eg.:
depmod: ERROR: Cycle detected: fwnode_mdio -> of_mdio -> fwnode_mdio depmod: ERROR: Found 2 modules in dependency cycles!
OR:
depmod: ERROR: Cycle detected: acpi_mdio -> fwnode_mdio -> acpi_mdio depmod: ERROR: Found 2 modules in dependency cycles!
The proper solution would be to merge contents of acpi_mdiobus_register and of_mdiobus_register inside the common fwnode_mdiobus_register (so that the former would only call the latter). However this change seems feasible, but I'd expect it to be a patchset bigger than this one alone and deserves its own thorough review and testing, as it would affect huge amount of current of_mdiobus_register users.
Given above, for now I will resubmit this patchset in the shape as proposed in v1, i.e. using the 'if' condition explicitly in mvmdio driver.
Best regards, Marcin
| |