diff mbox series

[net-next,02/10] net: dsa: lantiq_gswip: use devres for internal MDIO bus, not ds->user_mii_bus

Message ID 20240104140037.374166-3-vladimir.oltean@nxp.com (mailing list archive)
State Accepted
Commit cd4ba3ecced904cc8e48cde9ae3216889d139789
Delegated to: Netdev Maintainers
Headers show
Series ds->user_mii_bus cleanup (part 1) | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1113 this patch: 1113
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1140 this patch: 1140
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1140 this patch: 1140
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 110 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vladimir Oltean Jan. 4, 2024, 2 p.m. UTC
This driver does not need any of the functionalities that make
ds->user_mii_bus special. Those use cases are listed here:
https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/

It just makes use of ds->user_mii_bus only as storage for its own MDIO
bus, which otherwise has no connection to the framework. This is because:

- the gswip driver only probes on OF: it fails if of_device_get_match_data()
  returns NULL

- when the child OF node of the MDIO bus is absent, no MDIO bus is
  registered at all, not even by the DSA framework. In order for that to
  have happened, the gswip driver would have needed to provide
  ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
  not.

We can break the connection between the gswip driver and the DSA
framework and still preserve the same functionality.

Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
to mdiobus"), MDIO buses take ownership of the OF node handled to them,
and release it on their own. The gswip driver no longer needs to do
this.

Combine that with devres, and we no longer need to keep track of
anything for teardown purposes.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/lantiq_gswip.c | 69 +++++++++++++++-------------------
 1 file changed, 31 insertions(+), 38 deletions(-)

Comments

Alvin Šipraga Jan. 4, 2024, 3:53 p.m. UTC | #1
On Thu, Jan 04, 2024 at 04:00:29PM +0200, Vladimir Oltean wrote:
> This driver does not need any of the functionalities that make
> ds->user_mii_bus special. Those use cases are listed here:
> https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/
> 
> It just makes use of ds->user_mii_bus only as storage for its own MDIO
> bus, which otherwise has no connection to the framework. This is because:
> 
> - the gswip driver only probes on OF: it fails if of_device_get_match_data()
>   returns NULL
> 
> - when the child OF node of the MDIO bus is absent, no MDIO bus is
>   registered at all, not even by the DSA framework. In order for that to
>   have happened, the gswip driver would have needed to provide
>   ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
>   not.
> 
> We can break the connection between the gswip driver and the DSA
> framework and still preserve the same functionality.
> 
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), MDIO buses take ownership of the OF node handled to them,
> and release it on their own. The gswip driver no longer needs to do
> this.
> 
> Combine that with devres, and we no longer need to keep track of
> anything for teardown purposes.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/lantiq_gswip.c | 69 +++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 38 deletions(-)
Florian Fainelli Jan. 4, 2024, 5:36 p.m. UTC | #2
On 1/4/24 06:00, Vladimir Oltean wrote:
> This driver does not need any of the functionalities that make
> ds->user_mii_bus special. Those use cases are listed here:
> https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/
> 
> It just makes use of ds->user_mii_bus only as storage for its own MDIO
> bus, which otherwise has no connection to the framework. This is because:
> 
> - the gswip driver only probes on OF: it fails if of_device_get_match_data()
>    returns NULL
> 
> - when the child OF node of the MDIO bus is absent, no MDIO bus is
>    registered at all, not even by the DSA framework. In order for that to
>    have happened, the gswip driver would have needed to provide
>    ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
>    not.
> 
> We can break the connection between the gswip driver and the DSA
> framework and still preserve the same functionality.
> 
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), MDIO buses take ownership of the OF node handled to them,
> and release it on their own. The gswip driver no longer needs to do
> this.
> 
> Combine that with devres, and we no longer need to keep track of
> anything for teardown purposes.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Luiz Angelo Daros de Luca Jan. 5, 2024, 2:43 a.m. UTC | #3
> This driver does not need any of the functionalities that make
> ds->user_mii_bus special. Those use cases are listed here:
> https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/
>
> It just makes use of ds->user_mii_bus only as storage for its own MDIO
> bus, which otherwise has no connection to the framework. This is because:
>
> - the gswip driver only probes on OF: it fails if of_device_get_match_data()
>   returns NULL
>
> - when the child OF node of the MDIO bus is absent, no MDIO bus is
>   registered at all, not even by the DSA framework. In order for that to
>   have happened, the gswip driver would have needed to provide
>   ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
>   not.
>
> We can break the connection between the gswip driver and the DSA
> framework and still preserve the same functionality.
>
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), MDIO buses take ownership of the OF node handled to them,
> and release it on their own. The gswip driver no longer needs to do
> this.
>
> Combine that with devres, and we no longer need to keep track of
> anything for teardown purposes.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
diff mbox series

Patch

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 3494ad854cf6..a514e6c78c38 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -505,26 +505,34 @@  static int gswip_mdio_rd(struct mii_bus *bus, int addr, int reg)
 	return gswip_mdio_r(priv, GSWIP_MDIO_READ);
 }
 
-static int gswip_mdio(struct gswip_priv *priv, struct device_node *mdio_np)
+static int gswip_mdio(struct gswip_priv *priv)
 {
-	struct dsa_switch *ds = priv->ds;
+	struct device_node *mdio_np, *switch_np = priv->dev->of_node;
+	struct device *dev = priv->dev;
+	struct mii_bus *bus;
 	int err;
 
-	ds->user_mii_bus = mdiobus_alloc();
-	if (!ds->user_mii_bus)
-		return -ENOMEM;
+	mdio_np = of_get_compatible_child(switch_np, "lantiq,xrx200-mdio");
+	if (!mdio_np)
+		return 0;
 
-	ds->user_mii_bus->priv = priv;
-	ds->user_mii_bus->read = gswip_mdio_rd;
-	ds->user_mii_bus->write = gswip_mdio_wr;
-	ds->user_mii_bus->name = "lantiq,xrx200-mdio";
-	snprintf(ds->user_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
-		 dev_name(priv->dev));
-	ds->user_mii_bus->parent = priv->dev;
+	bus = devm_mdiobus_alloc(dev);
+	if (!bus) {
+		err = -ENOMEM;
+		goto out_put_node;
+	}
 
-	err = of_mdiobus_register(ds->user_mii_bus, mdio_np);
-	if (err)
-		mdiobus_free(ds->user_mii_bus);
+	bus->priv = priv;
+	bus->read = gswip_mdio_rd;
+	bus->write = gswip_mdio_wr;
+	bus->name = "lantiq,xrx200-mdio";
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));
+	bus->parent = priv->dev;
+
+	err = devm_of_mdiobus_register(dev, bus, mdio_np);
+
+out_put_node:
+	of_node_put(mdio_np);
 
 	return err;
 }
@@ -2093,9 +2101,9 @@  static int gswip_gphy_fw_list(struct gswip_priv *priv,
 
 static int gswip_probe(struct platform_device *pdev)
 {
-	struct gswip_priv *priv;
-	struct device_node *np, *mdio_np, *gphy_fw_np;
+	struct device_node *np, *gphy_fw_np;
 	struct device *dev = &pdev->dev;
+	struct gswip_priv *priv;
 	int err;
 	int i;
 	u32 version;
@@ -2162,19 +2170,16 @@  static int gswip_probe(struct platform_device *pdev)
 	}
 
 	/* bring up the mdio bus */
-	mdio_np = of_get_compatible_child(dev->of_node, "lantiq,xrx200-mdio");
-	if (mdio_np) {
-		err = gswip_mdio(priv, mdio_np);
-		if (err) {
-			dev_err(dev, "mdio probe failed\n");
-			goto put_mdio_node;
-		}
+	err = gswip_mdio(priv);
+	if (err) {
+		dev_err(dev, "mdio probe failed\n");
+		goto gphy_fw_remove;
 	}
 
 	err = dsa_register_switch(priv->ds);
 	if (err) {
 		dev_err(dev, "dsa switch register failed: %i\n", err);
-		goto mdio_bus;
+		goto gphy_fw_remove;
 	}
 	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
 		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
@@ -2193,13 +2198,7 @@  static int gswip_probe(struct platform_device *pdev)
 disable_switch:
 	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
 	dsa_unregister_switch(priv->ds);
-mdio_bus:
-	if (mdio_np) {
-		mdiobus_unregister(priv->ds->user_mii_bus);
-		mdiobus_free(priv->ds->user_mii_bus);
-	}
-put_mdio_node:
-	of_node_put(mdio_np);
+gphy_fw_remove:
 	for (i = 0; i < priv->num_gphy_fw; i++)
 		gswip_gphy_fw_remove(priv, &priv->gphy_fw[i]);
 	return err;
@@ -2218,12 +2217,6 @@  static void gswip_remove(struct platform_device *pdev)
 
 	dsa_unregister_switch(priv->ds);
 
-	if (priv->ds->user_mii_bus) {
-		mdiobus_unregister(priv->ds->user_mii_bus);
-		of_node_put(priv->ds->user_mii_bus->dev.of_node);
-		mdiobus_free(priv->ds->user_mii_bus);
-	}
-
 	for (i = 0; i < priv->num_gphy_fw; i++)
 		gswip_gphy_fw_remove(priv, &priv->gphy_fw[i]);
 }