Date: Fri, 8 Dec 2023 11:05:02 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>, "andrew@...n.ch"
	<andrew@...n.ch>, "f.fainelli@...il.com" <f.fainelli@...il.com>,
	"olteanv@...il.com" <olteanv@...il.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 6/7] net: dsa: realtek: migrate user_mii setup to
 common

On Fri, Dec 08, 2023 at 01:41:42AM -0300, Luiz Angelo Daros de Luca wrote:
> Although there are many mentions to SMI in in the user mdio driver,
> including its compatible string, there is nothing special about the SMI
> interface in the user mdio bus. That way, the code was migrated to the
> common module.
> 
> All references to SMI were removed, except for the compatible string
> that will still work but warn about using the mdio node name instead.
> 
> The variant ds_ops_{smi,mdio} fields were rename to, respectively,
> ds_ops_custom_user_mdio and ds_ops_default_user_mdio.
> 
> The priv->setup_interface() is also gone. If the ds_ops defines
> phy_read/write, it means DSA will handle the user_mii_bus. We don't need
> to check in another place. Also, with the function that would define
> setup_interface() in common, we can call it directly.

What is default?  It's MDIO. What is custom? It's SMI right now. If
there is another interface (SPI) then we may need a third way. Frankly I
do not see the benefit of this change, sorry.

> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
>  drivers/net/dsa/realtek/realtek-common.c | 67 ++++++++++++++++++++++++
>  drivers/net/dsa/realtek/realtek-common.h |  1 +
>  drivers/net/dsa/realtek/realtek-mdio.c   |  2 +-
>  drivers/net/dsa/realtek/realtek-smi.c    | 61 +--------------------
>  drivers/net/dsa/realtek/realtek.h        |  5 +-
>  drivers/net/dsa/realtek/rtl8365mb.c      | 12 ++---
>  drivers/net/dsa/realtek/rtl8366rb.c      | 12 ++---
>  7 files changed, 84 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> index 73c25d114dd3..64a55cb1ea05 100644
> --- a/drivers/net/dsa/realtek/realtek-common.c
> +++ b/drivers/net/dsa/realtek/realtek-common.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  #include <linux/module.h>
> +#include <linux/of_mdio.h>
>  
>  #include "realtek.h"
>  #include "realtek-common.h"
> @@ -21,6 +22,72 @@ void realtek_common_unlock(void *ctx)
>  }
>  EXPORT_SYMBOL_GPL(realtek_common_unlock);
>  
> +static int realtek_common_user_mdio_read(struct mii_bus *bus, int addr,
> +					 int regnum)
> +{
> +	struct realtek_priv *priv = bus->priv;
> +
> +	return priv->ops->phy_read(priv, addr, regnum);
> +}
> +
> +static int realtek_common_user_mdio_write(struct mii_bus *bus, int addr,
> +					  int regnum, u16 val)
> +{
> +	struct realtek_priv *priv = bus->priv;
> +
> +	return priv->ops->phy_write(priv, addr, regnum, val);
> +}
> +
> +int realtek_common_setup_user_mdio(struct dsa_switch *ds)
> +{
> +	struct realtek_priv *priv =  ds->priv;
> +	struct device_node *mdio_np;
> +	const char compatible = "realtek,smi-mdio";

const char *compatible

> +	int ret;
> +
> +	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
> +	if (!mdio_np) {
> +		mdio_np = of_get_compatible_child(priv->dev->of_node, compatible);
> +		if (!mdio_np) {
> +			dev_err(priv->dev, "no MDIO bus node\n");
> +			return -ENODEV;
> +		}
> +		dev_warn(priv->dev,
> +			 "Rename node '%s' to 'mdio' and remove compatible '%s'",
> +			 mdio_np->name, compatible);
> +	}
> +
> +	priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
> +	if (!priv->user_mii_bus) {
> +		ret = -ENOMEM;
> +		goto err_put_node;
> +	}
> +	priv->user_mii_bus->priv = priv;
> +	priv->user_mii_bus->name = "Realtek user MII";
> +	priv->user_mii_bus->read = realtek_common_user_mdio_read;
> +	priv->user_mii_bus->write = realtek_common_user_mdio_write;
> +	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "Realtek-%d",
> +		 ds->index);
> +	priv->user_mii_bus->parent = priv->dev;
> +	ds->user_mii_bus = priv->user_mii_bus;
> +
> +	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
> +	of_node_put(mdio_np);
> +	if (ret) {
> +		dev_err(priv->dev, "unable to register MDIO bus %s\n",
> +			priv->user_mii_bus->id);
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +err_put_node:
> +	of_node_put(mdio_np);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(realtek_common_setup_user_mdio);
> +
>  struct realtek_priv *
>  realtek_common_probe_pre(struct device *dev, struct regmap_config rc,
>  			 struct regmap_config rc_nolock)
> diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> index 405bd0d85d2b..4f8c66167b15 100644
> --- a/drivers/net/dsa/realtek/realtek-common.h
> +++ b/drivers/net/dsa/realtek/realtek-common.h
> @@ -7,6 +7,7 @@
>  
>  void realtek_common_lock(void *ctx);
>  void realtek_common_unlock(void *ctx);
> +int realtek_common_setup_user_mdio(struct dsa_switch *ds);
>  struct realtek_priv *
>  realtek_common_probe_pre(struct device *dev, struct regmap_config rc,
>  			 struct regmap_config rc_nolock);
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index bb5bff719ae9..37a41bab20b4 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -141,7 +141,7 @@ int realtek_mdio_probe(struct mdio_device *mdiodev)
>  	priv->bus = mdiodev->bus;
>  	priv->mdio_addr = mdiodev->addr;
>  	priv->write_reg_noack = realtek_mdio_write;
> -	priv->ds_ops = priv->variant->ds_ops_mdio;
> +	priv->ds_ops = priv->variant->ds_ops_default_user_mdio;
>  
>  	return realtek_common_probe_post(priv);
>  
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 1ca2aa784d24..84dde2123b09 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -31,7 +31,6 @@
>  #include <linux/spinlock.h>
>  #include <linux/skbuff.h>
>  #include <linux/of.h>
> -#include <linux/of_mdio.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/platform_device.h>
> @@ -339,63 +338,6 @@ static const struct regmap_config realtek_smi_nolock_regmap_config = {
>  	.disable_locking = true,
>  };
>  
> -static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum)
> -{
> -	struct realtek_priv *priv = bus->priv;
> -
> -	return priv->ops->phy_read(priv, addr, regnum);
> -}
> -
> -static int realtek_smi_mdio_write(struct mii_bus *bus, int addr, int regnum,
> -				  u16 val)
> -{
> -	struct realtek_priv *priv = bus->priv;
> -
> -	return priv->ops->phy_write(priv, addr, regnum, val);
> -}
> -
> -static int realtek_smi_setup_mdio(struct dsa_switch *ds)
> -{
> -	struct realtek_priv *priv =  ds->priv;
> -	struct device_node *mdio_np;
> -	int ret;
> -
> -	mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
> -	if (!mdio_np) {
> -		dev_err(priv->dev, "no MDIO bus node\n");
> -		return -ENODEV;
> -	}
> -
> -	priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
> -	if (!priv->user_mii_bus) {
> -		ret = -ENOMEM;
> -		goto err_put_node;
> -	}
> -	priv->user_mii_bus->priv = priv;
> -	priv->user_mii_bus->name = "SMI user MII";
> -	priv->user_mii_bus->read = realtek_smi_mdio_read;
> -	priv->user_mii_bus->write = realtek_smi_mdio_write;
> -	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d",
> -		 ds->index);
> -	priv->user_mii_bus->parent = priv->dev;
> -	ds->user_mii_bus = priv->user_mii_bus;
> -
> -	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
> -	of_node_put(mdio_np);
> -	if (ret) {
> -		dev_err(priv->dev, "unable to register MDIO bus %s\n",
> -			priv->user_mii_bus->id);
> -		return ret;
> -	}
> -
> -	return 0;
> -
> -err_put_node:
> -	of_node_put(mdio_np);
> -
> -	return ret;
> -}
> -
>  int realtek_smi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -416,8 +358,7 @@ int realtek_smi_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->mdio);
>  
>  	priv->write_reg_noack = realtek_smi_write_reg_noack;
> -	priv->setup_interface = realtek_smi_setup_mdio;
> -	priv->ds_ops = priv->variant->ds_ops_smi;
> +	priv->ds_ops = priv->variant->ds_ops_custom_user_mdio;
>  
>  	return realtek_common_probe_post(priv);
>  }
> diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> index fbd0616c1df3..3fa8479c396f 100644
> --- a/drivers/net/dsa/realtek/realtek.h
> +++ b/drivers/net/dsa/realtek/realtek.h
> @@ -71,7 +71,6 @@ struct realtek_priv {
>  	struct rtl8366_mib_counter *mib_counters;
>  
>  	const struct realtek_ops *ops;
> -	int			(*setup_interface)(struct dsa_switch *ds);
>  	int			(*write_reg_noack)(void *ctx, u32 addr, u32 data);
>  
>  	int			vlan_enabled;
> @@ -115,8 +114,8 @@ struct realtek_ops {
>  };
>  
>  struct realtek_variant {
> -	const struct dsa_switch_ops *ds_ops_smi;
> -	const struct dsa_switch_ops *ds_ops_mdio;
> +	const struct dsa_switch_ops *ds_ops_default_user_mdio;
> +	const struct dsa_switch_ops *ds_ops_custom_user_mdio;
>  	const struct realtek_ops *ops;
>  	unsigned int clk_delay;
>  	u8 cmd_read;
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index ac848b965f84..a52fb07504b5 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -2017,8 +2017,8 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  	if (ret)
>  		goto out_teardown_irq;
>  
> -	if (priv->setup_interface) {
> -		ret = priv->setup_interface(ds);
> +	if (!priv->ds_ops->phy_read) {
> +		ret = realtek_common_setup_user_mdio(ds);
>  		if (ret) {
>  			dev_err(priv->dev, "could not set up MDIO bus\n");
>  			goto out_teardown_irq;
> @@ -2116,7 +2116,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  	return 0;
>  }
>  
> -static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
> +static const struct dsa_switch_ops rtl8365mb_switch_ops_custom_user_mdio = {
>  	.get_tag_protocol = rtl8365mb_get_tag_protocol,
>  	.change_tag_protocol = rtl8365mb_change_tag_protocol,
>  	.setup = rtl8365mb_setup,
> @@ -2137,7 +2137,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>  	.port_max_mtu = rtl8365mb_port_max_mtu,
>  };
>  
> -static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
> +static const struct dsa_switch_ops rtl8365mb_switch_ops_default_user_mdio = {
>  	.get_tag_protocol = rtl8365mb_get_tag_protocol,
>  	.change_tag_protocol = rtl8365mb_change_tag_protocol,
>  	.setup = rtl8365mb_setup,
> @@ -2167,8 +2167,8 @@ static const struct realtek_ops rtl8365mb_ops = {
>  };
>  
>  const struct realtek_variant rtl8365mb_variant = {
> -	.ds_ops_smi = &rtl8365mb_switch_ops_smi,
> -	.ds_ops_mdio = &rtl8365mb_switch_ops_mdio,
> +	.ds_ops_default_user_mdio = &rtl8365mb_switch_ops_default_user_mdio,
> +	.ds_ops_custom_user_mdio = &rtl8365mb_switch_ops_custom_user_mdio,
>  	.ops = &rtl8365mb_ops,
>  	.clk_delay = 10,
>  	.cmd_read = 0xb9,
> diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
> index 1cc4de3cf54f..9b6997574d2c 100644
> --- a/drivers/net/dsa/realtek/rtl8366rb.c
> +++ b/drivers/net/dsa/realtek/rtl8366rb.c
> @@ -1027,8 +1027,8 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>  	if (ret)
>  		dev_info(priv->dev, "no interrupt support\n");
>  
> -	if (priv->setup_interface) {
> -		ret = priv->setup_interface(ds);
> +	if (!priv->ds_ops->phy_read) {
> +		ret = realtek_common_setup_user_mdio(ds);
>  		if (ret) {
>  			dev_err(priv->dev, "could not set up MDIO bus\n");
>  			return -ENODEV;
> @@ -1848,7 +1848,7 @@ static int rtl8366rb_detect(struct realtek_priv *priv)
>  	return 0;
>  }
>  
> -static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
> +static const struct dsa_switch_ops rtl8366rb_switch_ops_custom_user_mdio = {
>  	.get_tag_protocol = rtl8366_get_tag_protocol,
>  	.setup = rtl8366rb_setup,
>  	.phylink_get_caps = rtl8366rb_phylink_get_caps,
> @@ -1872,7 +1872,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
>  	.port_max_mtu = rtl8366rb_max_mtu,
>  };
>  
> -static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = {
> +static const struct dsa_switch_ops rtl8366rb_switch_ops_default_user_mdio = {
>  	.get_tag_protocol = rtl8366_get_tag_protocol,
>  	.setup = rtl8366rb_setup,
>  	.phy_read = rtl8366rb_dsa_phy_read,
> @@ -1915,8 +1915,8 @@ static const struct realtek_ops rtl8366rb_ops = {
>  };
>  
>  const struct realtek_variant rtl8366rb_variant = {
> -	.ds_ops_smi = &rtl8366rb_switch_ops_smi,
> -	.ds_ops_mdio = &rtl8366rb_switch_ops_mdio,
> +	.ds_ops_default_user_mdio = &rtl8366rb_switch_ops_default_user_mdio,
> +	.ds_ops_custom_user_mdio = &rtl8366rb_switch_ops_custom_user_mdio,
>  	.ops = &rtl8366rb_ops,
>  	.clk_delay = 10,
>  	.cmd_read = 0xa9,
> -- 
> 2.43.0
>

Powered by blists - more mailing lists