| Message ID | 20210621173028.3541424-3-mw@semihalf.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI MDIO support for Marvell controllers | expand |
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. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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> > Thanks, Marcin
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
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 #endif /* __LINUX_FWNODE_MDIO_H */ diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index 1becb1a731f6..ae0bf71a9932 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -7,8 +7,10 @@ */ #include <linux/acpi.h> +#include <linux/acpi_mdio.h> #include <linux/fwnode_mdio.h> #include <linux/of.h> +#include <linux/of_mdio.h> #include <linux/phy.h> MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>"); @@ -142,3 +144,23 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, return 0; } EXPORT_SYMBOL(fwnode_mdiobus_register_phy); + +/** + * 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; +} +EXPORT_SYMBOL(fwnode_mdiobus_register);
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(+)