[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <qp6cdwyqzsrprh5vgfllkms3syegdvpnbzdzmiddwfhtudpreq@vfy3z7p6dkfp>
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