[PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[Luiz Angelo Daros de Luca]

The current driver consists of two interface modules (SMI and MDIO) and
two family/variant modules (RTL8365MB and RTL8366RB). The SMI and MDIO
modules serve as the platform and MDIO drivers, respectively, calling
functions from the variant modules. In this setup, one interface module
can be loaded independently of the other, but both variants must be
loaded (if not disabled at build time) for any type of interface. This
approach doesn't scale well, especially with the addition of more switch
variants (e.g., RTL8366B), leading to loaded but unused modules.
Additionally, this also seems upside down, as the specific driver code
normally depends on the more generic functions and not the other way
around.

The series begins by removing an unused function pointer at
realtek_ops->cleanup.

Each variant module was converted into real drivers, serving as both a
platform driver (for switches connected using the SMI interface) and an
MDIO driver (for MDIO-connected switches). The relationship between the
variant and interface modules is reversed, with the variant module now
calling both interface functions (if not disabled at build time). While
in most devices only one interface is likely used, the interface code is
significantly smaller than a variant module, consuming fewer resources
than the previous code. With variant modules now functioning as real
drivers, compatible strings are published only in a single variant
module, preventing conflicts.

The patch series introduces a new common module for functions shared by
both variants. This module also absorbs the two previous interface
modules, as they would always be loaded anyway.

The series relocates the user MII driver from realtek-smi to common. It
is now used by MDIO-connected switches instead of the generic DSA
driver. There's a change in how this driver locates the MDIO node. It
now only searches for a child node named "mdio", which is required by
both interfaces in binding docs.

The dsa_switch in realtek_priv->ds is now embedded in the struct. It is
always in use and avoids dynamic memory allocation.

Testing has been performed with an RTL8367S (rtl8365mb) using MDIO
interface and an RTL8366RB (rtl8366) with SMI interface.

Luiz

---

Changes:

v2-v3:
1) Look for the MDIO bus searching for a child node named "mdio" instead
   of the compatible string.
2) Removed the check for a phy-handle in ports. ds->user_mii_bus will
   not be used anymore.
3) Dropped comments for realtek_common_{probe,register_switch}
4) Fixed a compile error in "net: dsa: OF-ware slave_mii_bus"
5) Used the wrapper realtek_smi_driver_register instead of
   platform_driver_register

v1-v2:
1)  Renamed realtek_common module to realtek-dsa.
2)  Removed the warning when the MDIO node is not named "mdio."
3)  ds->user_mii_bus is only assigned if all user ports do not have a
    phy-handle.
4)  of_node_put is now back to the driver remove method.
5)  Renamed realtek_common_probe_{pre,post} to
    realtek_common_{probe,register_switch}.
6)  Added some comments for realtek_common_{probe,register_switch}.
7)  Using dev_err_probe whenever possible.
8)  Embedded priv->ds into realtek_priv, removing its dynamic
    allocation.
9)  Fixed realtek-common.h macros.
10) Save and check the return value in functions, even when it is the
    last one.
11) Added the #if expression as a comment to #else and #endif in header
    files.
12) Unregister the platform and the MDIO driver in the reverse order
    they are registered.
13) Unregister the first driver if the second one failed to register.
14) Added the revert patch for "net: dsa: OF-ware slave_mii_bus."

[PATCH net-next v3 1/8] net: dsa: realtek: drop cleanup from realtek_ops

[Luiz Angelo Daros de Luca]

It was never used and never referenced.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/dsa/realtek/realtek.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 790488e9c667..e9ee778665b2 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -91,7 +91,6 @@ struct realtek_ops {
 	int	(*detect)(struct realtek_priv *priv);
 	int	(*reset_chip)(struct realtek_priv *priv);
 	int	(*setup)(struct realtek_priv *priv);
-	void	(*cleanup)(struct realtek_priv *priv);
 	int	(*get_mib_counter)(struct realtek_priv *priv,
 				   int port,
 				   struct rtl8366_mib_counter *mib,
-- 
2.43.0

[PATCH net-next v3 2/8] net: dsa: realtek: convert variants into real drivers

[Luiz Angelo Daros de Luca]

Previously, the interface modules realtek-smi and realtek-mdio served as
a platform and an MDIO driver, respectively. Each interface module
redundantly specified the same compatible strings for both variants and
referenced symbols from the variants.

Now, each variant module has been transformed into a unified driver
serving both as a platform and an MDIO driver. This modification
reverses the relationship between the interface and variant modules,
with the variant module now utilizing symbols from the interface
modules.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Kconfig        | 20 +++-------
 drivers/net/dsa/realtek/realtek-mdio.c | 33 ++++------------
 drivers/net/dsa/realtek/realtek-mdio.h | 48 +++++++++++++++++++++++
 drivers/net/dsa/realtek/realtek-smi.c  | 38 ++++--------------
 drivers/net/dsa/realtek/realtek-smi.h  | 48 +++++++++++++++++++++++
 drivers/net/dsa/realtek/rtl8365mb.c    | 54 +++++++++++++++++++++++++-
 drivers/net/dsa/realtek/rtl8366rb.c    | 54 +++++++++++++++++++++++++-
 7 files changed, 222 insertions(+), 73 deletions(-)
 create mode 100644 drivers/net/dsa/realtek/realtek-mdio.h
 create mode 100644 drivers/net/dsa/realtek/realtek-smi.h

diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
index 060165a85fb7..9d182fde11b4 100644
--- a/drivers/net/dsa/realtek/Kconfig
+++ b/drivers/net/dsa/realtek/Kconfig
@@ -16,37 +16,29 @@ menuconfig NET_DSA_REALTEK
 if NET_DSA_REALTEK
 
 config NET_DSA_REALTEK_MDIO
-	tristate "Realtek MDIO interface driver"
+	tristate "Realtek MDIO interface support"
 	depends on OF
-	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
-	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
-	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
 	help
 	  Select to enable support for registering switches configured
 	  through MDIO.
 
 config NET_DSA_REALTEK_SMI
-	tristate "Realtek SMI interface driver"
+	tristate "Realtek SMI interface support"
 	depends on OF
-	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
-	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
-	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
 	help
 	  Select to enable support for registering switches connected
 	  through SMI.
 
 config NET_DSA_REALTEK_RTL8365MB
-	tristate "Realtek RTL8365MB switch subdriver"
-	imply NET_DSA_REALTEK_SMI
-	imply NET_DSA_REALTEK_MDIO
+	tristate "Realtek RTL8365MB switch driver"
+	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
 	select NET_DSA_TAG_RTL8_4
 	help
 	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
 
 config NET_DSA_REALTEK_RTL8366RB
-	tristate "Realtek RTL8366RB switch subdriver"
-	imply NET_DSA_REALTEK_SMI
-	imply NET_DSA_REALTEK_MDIO
+	tristate "Realtek RTL8366RB switch driver"
+	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
 	select NET_DSA_TAG_RTL4_A
 	help
 	  Select to enable support for Realtek RTL8366RB.
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 292e6d087e8b..58966d0625c8 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -25,6 +25,7 @@
 #include <linux/regmap.h>
 
 #include "realtek.h"
+#include "realtek-mdio.h"
 
 /* Read/write via mdiobus */
 #define REALTEK_MDIO_CTRL0_REG		31
@@ -140,7 +141,7 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
 	.disable_locking = true,
 };
 
-static int realtek_mdio_probe(struct mdio_device *mdiodev)
+int realtek_mdio_probe(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv;
 	struct device *dev = &mdiodev->dev;
@@ -235,8 +236,9 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(realtek_mdio_probe);
 
-static void realtek_mdio_remove(struct mdio_device *mdiodev)
+void realtek_mdio_remove(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
 
@@ -249,8 +251,9 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
 	if (priv->reset)
 		gpiod_set_value(priv->reset, 1);
 }
+EXPORT_SYMBOL_GPL(realtek_mdio_remove);
 
-static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
+void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
 
@@ -261,29 +264,7 @@ static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 
 	dev_set_drvdata(&mdiodev->dev, NULL);
 }
-
-static const struct of_device_id realtek_mdio_of_match[] = {
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
-	{ .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
-#endif
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
-	{ .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, },
-#endif
-	{ /* sentinel */ },
-};
-MODULE_DEVICE_TABLE(of, realtek_mdio_of_match);
-
-static struct mdio_driver realtek_mdio_driver = {
-	.mdiodrv.driver = {
-		.name = "realtek-mdio",
-		.of_match_table = realtek_mdio_of_match,
-	},
-	.probe  = realtek_mdio_probe,
-	.remove = realtek_mdio_remove,
-	.shutdown = realtek_mdio_shutdown,
-};
-
-mdio_module_driver(realtek_mdio_driver);
+EXPORT_SYMBOL_GPL(realtek_mdio_shutdown);
 
 MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
diff --git a/drivers/net/dsa/realtek/realtek-mdio.h b/drivers/net/dsa/realtek/realtek-mdio.h
new file mode 100644
index 000000000000..ee70f6a5b8ff
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-mdio.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_MDIO_H
+#define _REALTEK_MDIO_H
+
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO)
+
+static inline int realtek_mdio_driver_register(struct mdio_driver *drv)
+{
+	return mdio_driver_register(drv);
+}
+
+static inline void realtek_mdio_driver_unregister(struct mdio_driver *drv)
+{
+	mdio_driver_unregister(drv);
+}
+
+int realtek_mdio_probe(struct mdio_device *mdiodev);
+void realtek_mdio_remove(struct mdio_device *mdiodev);
+void realtek_mdio_shutdown(struct mdio_device *mdiodev);
+
+#else /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO) */
+
+static inline int realtek_mdio_driver_register(struct mdio_driver *drv)
+{
+	return 0;
+}
+
+static inline void realtek_mdio_driver_unregister(struct mdio_driver *drv)
+{
+}
+
+static inline int realtek_mdio_probe(struct mdio_device *mdiodev)
+{
+	return -ENOENT;
+}
+
+static inline void realtek_mdio_remove(struct mdio_device *mdiodev)
+{
+}
+
+static inline void realtek_mdio_shutdown(struct mdio_device *mdiodev)
+{
+}
+
+#endif /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO) */
+
+#endif /* _REALTEK_MDIO_H */
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 755546ed8db6..31ac409acfd0 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -40,6 +40,7 @@
 #include <linux/if_bridge.h>
 
 #include "realtek.h"
+#include "realtek-smi.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
 
@@ -408,7 +409,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	return ret;
 }
 
-static int realtek_smi_probe(struct platform_device *pdev)
+int realtek_smi_probe(struct platform_device *pdev)
 {
 	const struct realtek_variant *var;
 	struct device *dev = &pdev->dev;
@@ -505,8 +506,9 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(realtek_smi_probe);
 
-static void realtek_smi_remove(struct platform_device *pdev)
+void realtek_smi_remove(struct platform_device *pdev)
 {
 	struct realtek_priv *priv = platform_get_drvdata(pdev);
 
@@ -521,8 +523,9 @@ static void realtek_smi_remove(struct platform_device *pdev)
 	if (priv->reset)
 		gpiod_set_value(priv->reset, 1);
 }
+EXPORT_SYMBOL_GPL(realtek_smi_remove);
 
-static void realtek_smi_shutdown(struct platform_device *pdev)
+void realtek_smi_shutdown(struct platform_device *pdev)
 {
 	struct realtek_priv *priv = platform_get_drvdata(pdev);
 
@@ -533,34 +536,7 @@ static void realtek_smi_shutdown(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 }
-
-static const struct of_device_id realtek_smi_of_match[] = {
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
-	{
-		.compatible = "realtek,rtl8366rb",
-		.data = &rtl8366rb_variant,
-	},
-#endif
-#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
-	{
-		.compatible = "realtek,rtl8365mb",
-		.data = &rtl8365mb_variant,
-	},
-#endif
-	{ /* sentinel */ },
-};
-MODULE_DEVICE_TABLE(of, realtek_smi_of_match);
-
-static struct platform_driver realtek_smi_driver = {
-	.driver = {
-		.name = "realtek-smi",
-		.of_match_table = realtek_smi_of_match,
-	},
-	.probe  = realtek_smi_probe,
-	.remove_new = realtek_smi_remove,
-	.shutdown = realtek_smi_shutdown,
-};
-module_platform_driver(realtek_smi_driver);
+EXPORT_SYMBOL_GPL(realtek_smi_shutdown);
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
diff --git a/drivers/net/dsa/realtek/realtek-smi.h b/drivers/net/dsa/realtek/realtek-smi.h
new file mode 100644
index 000000000000..ea49a2edd3c8
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-smi.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_SMI_H
+#define _REALTEK_SMI_H
+
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI)
+
+static inline int realtek_smi_driver_register(struct platform_driver *drv)
+{
+	return platform_driver_register(drv);
+}
+
+static inline void realtek_smi_driver_unregister(struct platform_driver *drv)
+{
+	platform_driver_unregister(drv);
+}
+
+int realtek_smi_probe(struct platform_device *pdev);
+void realtek_smi_remove(struct platform_device *pdev);
+void realtek_smi_shutdown(struct platform_device *pdev);
+
+#else /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI) */
+
+static inline int realtek_smi_driver_register(struct platform_driver *drv)
+{
+	return 0;
+}
+
+static inline void realtek_smi_driver_unregister(struct platform_driver *drv)
+{
+}
+
+static inline int realtek_smi_probe(struct platform_device *pdev)
+{
+	return -ENOENT;
+}
+
+static inline void realtek_smi_remove(struct platform_device *pdev)
+{
+}
+
+static inline void realtek_smi_shutdown(struct platform_device *pdev)
+{
+}
+
+#endif /* IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI) */
+
+#endif  /* _REALTEK_SMI_H */
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 0875e4fc9f57..a4d0abd5a6bf 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -101,6 +101,8 @@
 #include <linux/if_vlan.h>
 
 #include "realtek.h"
+#include "realtek-smi.h"
+#include "realtek-mdio.h"
 
 /* Family-specific data and limits */
 #define RTL8365MB_PHYADDRMAX		7
@@ -2172,7 +2174,57 @@ const struct realtek_variant rtl8365mb_variant = {
 	.cmd_write = 0xb8,
 	.chip_data_sz = sizeof(struct rtl8365mb),
 };
-EXPORT_SYMBOL_GPL(rtl8365mb_variant);
+
+static const struct of_device_id rtl8365mb_of_match[] = {
+	{ .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rtl8365mb_of_match);
+
+static struct platform_driver rtl8365mb_smi_driver = {
+	.driver = {
+		.name = "rtl8365mb-smi",
+		.of_match_table = rtl8365mb_of_match,
+	},
+	.probe  = realtek_smi_probe,
+	.remove_new = realtek_smi_remove,
+	.shutdown = realtek_smi_shutdown,
+};
+
+static struct mdio_driver rtl8365mb_mdio_driver = {
+	.mdiodrv.driver = {
+		.name = "rtl8365mb-mdio",
+		.of_match_table = rtl8365mb_of_match,
+	},
+	.probe  = realtek_mdio_probe,
+	.remove = realtek_mdio_remove,
+	.shutdown = realtek_mdio_shutdown,
+};
+
+static int rtl8365mb_init(void)
+{
+	int ret;
+
+	ret = realtek_mdio_driver_register(&rtl8365mb_mdio_driver);
+	if (ret)
+		return ret;
+
+	ret = realtek_smi_driver_register(&rtl8365mb_smi_driver);
+	if (ret) {
+		realtek_mdio_driver_unregister(&rtl8365mb_mdio_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(rtl8365mb_init);
+
+static void __exit rtl8365mb_exit(void)
+{
+	realtek_smi_driver_unregister(&rtl8365mb_smi_driver);
+	realtek_mdio_driver_unregister(&rtl8365mb_mdio_driver);
+}
+module_exit(rtl8365mb_exit);
 
 MODULE_AUTHOR("Alvin Šipraga <alsi@bang-olufsen.dk>");
 MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch");
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index b39b719a5b8f..cc2fd636ec23 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -22,6 +22,8 @@
 #include <linux/regmap.h>
 
 #include "realtek.h"
+#include "realtek-smi.h"
+#include "realtek-mdio.h"
 
 #define RTL8366RB_PORT_NUM_CPU		5
 #define RTL8366RB_NUM_PORTS		6
@@ -1920,7 +1922,57 @@ const struct realtek_variant rtl8366rb_variant = {
 	.cmd_write = 0xa8,
 	.chip_data_sz = sizeof(struct rtl8366rb),
 };
-EXPORT_SYMBOL_GPL(rtl8366rb_variant);
+
+static const struct of_device_id rtl8366rb_of_match[] = {
+	{ .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rtl8366rb_of_match);
+
+static struct platform_driver rtl8366rb_smi_driver = {
+	.driver = {
+		.name = "rtl8366rb-smi",
+		.of_match_table = rtl8366rb_of_match,
+	},
+	.probe  = realtek_smi_probe,
+	.remove_new = realtek_smi_remove,
+	.shutdown = realtek_smi_shutdown,
+};
+
+static struct mdio_driver rtl8366rb_mdio_driver = {
+	.mdiodrv.driver = {
+		.name = "rtl8366rb-mdio",
+		.of_match_table = rtl8366rb_of_match,
+	},
+	.probe  = realtek_mdio_probe,
+	.remove = realtek_mdio_remove,
+	.shutdown = realtek_mdio_shutdown,
+};
+
+static int rtl8366rb_init(void)
+{
+	int ret;
+
+	ret = realtek_mdio_driver_register(&rtl8366rb_mdio_driver);
+	if (ret)
+		return ret;
+
+	ret = realtek_smi_driver_register(&rtl8366rb_smi_driver);
+	if (ret) {
+		realtek_mdio_driver_unregister(&rtl8366rb_mdio_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(rtl8366rb_init);
+
+static void __exit rtl8366rb_exit(void)
+{
+	realtek_smi_driver_unregister(&rtl8366rb_smi_driver);
+	realtek_mdio_driver_unregister(&rtl8366rb_mdio_driver);
+}
+module_exit(rtl8366rb_exit);
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for RTL8366RB ethernet switch");
-- 
2.43.0

[PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Luiz Angelo Daros de Luca]

Some code can be shared between both interface modules (MDIO and SMI)
and among variants. These interface functions migrated to a common
module:

- realtek_common_lock
- realtek_common_unlock
- realtek_common_probe
- realtek_common_register_switch
- realtek_common_remove

The reset during probe was moved to the end of the common probe. This way,
we avoid a reset if anything else fails.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Makefile         |   2 +
 drivers/net/dsa/realtek/realtek-common.c | 132 +++++++++++++++++++++++
 drivers/net/dsa/realtek/realtek-common.h |  16 +++
 drivers/net/dsa/realtek/realtek-mdio.c   | 114 +++-----------------
 drivers/net/dsa/realtek/realtek-smi.c    | 117 ++++----------------
 drivers/net/dsa/realtek/realtek.h        |   6 +-
 drivers/net/dsa/realtek/rtl8365mb.c      |   9 +-
 drivers/net/dsa/realtek/rtl8366rb.c      |   9 +-
 8 files changed, 194 insertions(+), 211 deletions(-)
 create mode 100644 drivers/net/dsa/realtek/realtek-common.c
 create mode 100644 drivers/net/dsa/realtek/realtek-common.h

diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
index 0aab57252a7c..f4f9c6340d5f 100644
--- a/drivers/net/dsa/realtek/Makefile
+++ b/drivers/net/dsa/realtek/Makefile
@@ -1,4 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_NET_DSA_REALTEK)		+= realtek-dsa.o
+realtek-dsa-objs			:= realtek-common.o
 obj-$(CONFIG_NET_DSA_REALTEK_MDIO) 	+= realtek-mdio.o
 obj-$(CONFIG_NET_DSA_REALTEK_SMI) 	+= realtek-smi.o
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
new file mode 100644
index 000000000000..80b37e5fe780
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/module.h>
+
+#include "realtek.h"
+#include "realtek-common.h"
+
+void realtek_common_lock(void *ctx)
+{
+	struct realtek_priv *priv = ctx;
+
+	mutex_lock(&priv->map_lock);
+}
+EXPORT_SYMBOL_GPL(realtek_common_lock);
+
+void realtek_common_unlock(void *ctx)
+{
+	struct realtek_priv *priv = ctx;
+
+	mutex_unlock(&priv->map_lock);
+}
+EXPORT_SYMBOL_GPL(realtek_common_unlock);
+
+struct realtek_priv *
+realtek_common_probe(struct device *dev, struct regmap_config rc,
+		     struct regmap_config rc_nolock)
+{
+	const struct realtek_variant *var;
+	struct realtek_priv *priv;
+	int ret;
+
+	var = of_device_get_match_data(dev);
+	if (!var)
+		return ERR_PTR(-EINVAL);
+
+	priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
+			    GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&priv->map_lock);
+
+	rc.lock_arg = priv;
+	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
+	if (IS_ERR(priv->map)) {
+		ret = PTR_ERR(priv->map);
+		dev_err(dev, "regmap init failed: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock);
+	if (IS_ERR(priv->map_nolock)) {
+		ret = PTR_ERR(priv->map_nolock);
+		dev_err(dev, "regmap init failed: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	/* Link forward and backward */
+	priv->dev = dev;
+	priv->variant = var;
+	priv->ops = var->ops;
+	priv->chip_data = (void *)priv + sizeof(*priv);
+
+	dev_set_drvdata(dev, priv);
+	spin_lock_init(&priv->lock);
+
+	priv->leds_disabled = of_property_read_bool(dev->of_node,
+						    "realtek,disable-leds");
+
+	/* TODO: if power is software controlled, set up any regulators here */
+
+	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->reset)) {
+		dev_err(dev, "failed to get RESET GPIO\n");
+		return ERR_CAST(priv->reset);
+	}
+	if (priv->reset) {
+		gpiod_set_value(priv->reset, 1);
+		dev_dbg(dev, "asserted RESET\n");
+		msleep(REALTEK_HW_STOP_DELAY);
+		gpiod_set_value(priv->reset, 0);
+		msleep(REALTEK_HW_START_DELAY);
+		dev_dbg(dev, "deasserted RESET\n");
+	}
+
+	return priv;
+}
+EXPORT_SYMBOL(realtek_common_probe);
+
+int realtek_common_register_switch(struct realtek_priv *priv)
+{
+	int ret;
+
+	ret = priv->ops->detect(priv);
+	if (ret) {
+		dev_err_probe(priv->dev, ret, "unable to detect switch\n");
+		return ret;
+	}
+
+	priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL);
+	if (!priv->ds)
+		return -ENOMEM;
+
+	priv->ds->priv = priv;
+	priv->ds->dev = priv->dev;
+	priv->ds->ops = priv->ds_ops;
+	priv->ds->num_ports = priv->num_ports;
+
+	ret = dsa_register_switch(priv->ds);
+	if (ret) {
+		dev_err_probe(priv->dev, ret, "unable to register switch\n");
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(realtek_common_register_switch);
+
+void realtek_common_remove(struct realtek_priv *priv)
+{
+	if (!priv)
+		return;
+
+	/* leave the device reset asserted */
+	if (priv->reset)
+		gpiod_set_value(priv->reset, 1);
+}
+EXPORT_SYMBOL(realtek_common_remove);
+
+MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
+MODULE_DESCRIPTION("Realtek DSA switches common module");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
new file mode 100644
index 000000000000..518d091ff496
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-common.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_COMMON_H
+#define _REALTEK_COMMON_H
+
+#include <linux/regmap.h>
+
+void realtek_common_lock(void *ctx);
+void realtek_common_unlock(void *ctx);
+struct realtek_priv *
+realtek_common_probe(struct device *dev, struct regmap_config rc,
+		     struct regmap_config rc_nolock);
+int realtek_common_register_switch(struct realtek_priv *priv);
+void realtek_common_remove(struct realtek_priv *priv);
+
+#endif /* _REALTEK_COMMON_H */
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 58966d0625c8..1eed09ab3aa1 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -26,6 +26,7 @@
 
 #include "realtek.h"
 #include "realtek-mdio.h"
+#include "realtek-common.h"
 
 /* Read/write via mdiobus */
 #define REALTEK_MDIO_CTRL0_REG		31
@@ -100,20 +101,6 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
 	return ret;
 }
 
-static void realtek_mdio_lock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_lock(&priv->map_lock);
-}
-
-static void realtek_mdio_unlock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_unlock(&priv->map_lock);
-}
-
 static const struct regmap_config realtek_mdio_regmap_config = {
 	.reg_bits = 10, /* A4..A0 R4..R0 */
 	.val_bits = 16,
@@ -124,8 +111,8 @@ static const struct regmap_config realtek_mdio_regmap_config = {
 	.reg_read = realtek_mdio_read,
 	.reg_write = realtek_mdio_write,
 	.cache_type = REGCACHE_NONE,
-	.lock = realtek_mdio_lock,
-	.unlock = realtek_mdio_unlock,
+	.lock = realtek_common_lock,
+	.unlock = realtek_common_unlock,
 };
 
 static const struct regmap_config realtek_mdio_nolock_regmap_config = {
@@ -143,96 +130,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
 
 int realtek_mdio_probe(struct mdio_device *mdiodev)
 {
-	struct realtek_priv *priv;
 	struct device *dev = &mdiodev->dev;
-	const struct realtek_variant *var;
-	struct regmap_config rc;
-	struct device_node *np;
+	struct realtek_priv *priv;
 	int ret;
 
-	var = of_device_get_match_data(dev);
-	if (!var)
-		return -EINVAL;
-
-	priv = devm_kzalloc(&mdiodev->dev,
-			    size_add(sizeof(*priv), var->chip_data_sz),
-			    GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	mutex_init(&priv->map_lock);
+	priv = realtek_common_probe(dev, realtek_mdio_regmap_config,
+				    realtek_mdio_nolock_regmap_config);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
 
-	rc = realtek_mdio_regmap_config;
-	rc.lock_arg = priv;
-	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map)) {
-		ret = PTR_ERR(priv->map);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
-
-	rc = realtek_mdio_nolock_regmap_config;
-	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map_nolock)) {
-		ret = PTR_ERR(priv->map_nolock);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
-
-	priv->mdio_addr = mdiodev->addr;
 	priv->bus = mdiodev->bus;
-	priv->dev = &mdiodev->dev;
-	priv->chip_data = (void *)priv + sizeof(*priv);
-
-	priv->clk_delay = var->clk_delay;
-	priv->cmd_read = var->cmd_read;
-	priv->cmd_write = var->cmd_write;
-	priv->ops = var->ops;
-
+	priv->mdio_addr = mdiodev->addr;
 	priv->write_reg_noack = realtek_mdio_write;
+	priv->ds_ops = priv->variant->ds_ops_mdio;
 
-	np = dev->of_node;
-
-	dev_set_drvdata(dev, priv);
-
-	/* TODO: if power is software controlled, set up any regulators here */
-	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
-
-	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->reset)) {
-		dev_err(dev, "failed to get RESET GPIO\n");
-		return PTR_ERR(priv->reset);
-	}
-
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
-		dev_dbg(dev, "asserted RESET\n");
-		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
-		msleep(REALTEK_HW_START_DELAY);
-		dev_dbg(dev, "deasserted RESET\n");
-	}
-
-	ret = priv->ops->detect(priv);
-	if (ret) {
-		dev_err(dev, "unable to detect switch\n");
-		return ret;
-	}
-
-	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return -ENOMEM;
-
-	priv->ds->dev = dev;
-	priv->ds->num_ports = priv->num_ports;
-	priv->ds->priv = priv;
-	priv->ds->ops = var->ds_ops_mdio;
-
-	ret = dsa_register_switch(priv->ds);
-	if (ret) {
-		dev_err(priv->dev, "unable to register switch ret = %d\n", ret);
+	ret = realtek_common_register_switch(priv);
+	if (ret)
 		return ret;
-	}
 
 	return 0;
 }
@@ -247,9 +161,7 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
 
 	dsa_unregister_switch(priv->ds);
 
-	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_common_remove(priv);
 }
 EXPORT_SYMBOL_GPL(realtek_mdio_remove);
 
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 31ac409acfd0..fc54190839cf 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -41,12 +41,13 @@
 
 #include "realtek.h"
 #include "realtek-smi.h"
+#include "realtek-common.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
 
 static inline void realtek_smi_clk_delay(struct realtek_priv *priv)
 {
-	ndelay(priv->clk_delay);
+	ndelay(priv->variant->clk_delay);
 }
 
 static void realtek_smi_start(struct realtek_priv *priv)
@@ -209,7 +210,7 @@ static int realtek_smi_read_reg(struct realtek_priv *priv, u32 addr, u32 *data)
 	realtek_smi_start(priv);
 
 	/* Send READ command */
-	ret = realtek_smi_write_byte(priv, priv->cmd_read);
+	ret = realtek_smi_write_byte(priv, priv->variant->cmd_read);
 	if (ret)
 		goto out;
 
@@ -250,7 +251,7 @@ static int realtek_smi_write_reg(struct realtek_priv *priv,
 	realtek_smi_start(priv);
 
 	/* Send WRITE command */
-	ret = realtek_smi_write_byte(priv, priv->cmd_write);
+	ret = realtek_smi_write_byte(priv, priv->variant->cmd_write);
 	if (ret)
 		goto out;
 
@@ -311,20 +312,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val)
 	return realtek_smi_read_reg(priv, reg, val);
 }
 
-static void realtek_smi_lock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_lock(&priv->map_lock);
-}
-
-static void realtek_smi_unlock(void *ctx)
-{
-	struct realtek_priv *priv = ctx;
-
-	mutex_unlock(&priv->map_lock);
-}
-
 static const struct regmap_config realtek_smi_regmap_config = {
 	.reg_bits = 10, /* A4..A0 R4..R0 */
 	.val_bits = 16,
@@ -335,8 +322,8 @@ static const struct regmap_config realtek_smi_regmap_config = {
 	.reg_read = realtek_smi_read,
 	.reg_write = realtek_smi_write,
 	.cache_type = REGCACHE_NONE,
-	.lock = realtek_smi_lock,
-	.unlock = realtek_smi_unlock,
+	.lock = realtek_common_lock,
+	.unlock = realtek_common_unlock,
 };
 
 static const struct regmap_config realtek_smi_nolock_regmap_config = {
@@ -411,99 +398,32 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 
 int realtek_smi_probe(struct platform_device *pdev)
 {
-	const struct realtek_variant *var;
 	struct device *dev = &pdev->dev;
 	struct realtek_priv *priv;
-	struct regmap_config rc;
-	struct device_node *np;
 	int ret;
 
-	var = of_device_get_match_data(dev);
-	np = dev->of_node;
-
-	priv = devm_kzalloc(dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-	priv->chip_data = (void *)priv + sizeof(*priv);
-
-	mutex_init(&priv->map_lock);
-
-	rc = realtek_smi_regmap_config;
-	rc.lock_arg = priv;
-	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map)) {
-		ret = PTR_ERR(priv->map);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
-
-	rc = realtek_smi_nolock_regmap_config;
-	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map_nolock)) {
-		ret = PTR_ERR(priv->map_nolock);
-		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ret;
-	}
-
-	/* Link forward and backward */
-	priv->dev = dev;
-	priv->clk_delay = var->clk_delay;
-	priv->cmd_read = var->cmd_read;
-	priv->cmd_write = var->cmd_write;
-	priv->ops = var->ops;
-
-	priv->setup_interface = realtek_smi_setup_mdio;
-	priv->write_reg_noack = realtek_smi_write_reg_noack;
-
-	dev_set_drvdata(dev, priv);
-	spin_lock_init(&priv->lock);
-
-	/* TODO: if power is software controlled, set up any regulators here */
-
-	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->reset)) {
-		dev_err(dev, "failed to get RESET GPIO\n");
-		return PTR_ERR(priv->reset);
-	}
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
-		dev_dbg(dev, "asserted RESET\n");
-		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
-		msleep(REALTEK_HW_START_DELAY);
-		dev_dbg(dev, "deasserted RESET\n");
-	}
+	priv = realtek_common_probe(dev, realtek_smi_regmap_config,
+				    realtek_smi_nolock_regmap_config);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
 
 	/* Fetch MDIO pins */
 	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->mdc))
 		return PTR_ERR(priv->mdc);
+
 	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->mdio))
 		return PTR_ERR(priv->mdio);
 
-	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
+	priv->write_reg_noack = realtek_smi_write_reg_noack;
+	priv->setup_interface = realtek_smi_setup_mdio;
+	priv->ds_ops = priv->variant->ds_ops_smi;
 
-	ret = priv->ops->detect(priv);
-	if (ret) {
-		dev_err(dev, "unable to detect switch\n");
+	ret = realtek_common_register_switch(priv);
+	if (ret)
 		return ret;
-	}
-
-	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return -ENOMEM;
 
-	priv->ds->dev = dev;
-	priv->ds->num_ports = priv->num_ports;
-	priv->ds->priv = priv;
-
-	priv->ds->ops = var->ds_ops_smi;
-	ret = dsa_register_switch(priv->ds);
-	if (ret) {
-		dev_err_probe(dev, ret, "unable to register switch\n");
-		return ret;
-	}
 	return 0;
 }
 EXPORT_SYMBOL_GPL(realtek_smi_probe);
@@ -516,12 +436,11 @@ void realtek_smi_remove(struct platform_device *pdev)
 		return;
 
 	dsa_unregister_switch(priv->ds);
+
 	if (priv->user_mii_bus)
 		of_node_put(priv->user_mii_bus->dev.of_node);
 
-	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_common_remove(priv);
 }
 EXPORT_SYMBOL_GPL(realtek_smi_remove);
 
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index e9ee778665b2..fbd0616c1df3 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -58,11 +58,9 @@ struct realtek_priv {
 	struct mii_bus		*bus;
 	int			mdio_addr;
 
-	unsigned int		clk_delay;
-	u8			cmd_read;
-	u8			cmd_write;
 	spinlock_t		lock; /* Locks around command writes */
 	struct dsa_switch	*ds;
+	const struct dsa_switch_ops *ds_ops;
 	struct irq_domain	*irqdomain;
 	bool			leds_disabled;
 
@@ -79,6 +77,8 @@ struct realtek_priv {
 	int			vlan_enabled;
 	int			vlan4k_enabled;
 
+	const struct realtek_variant *variant;
+
 	char			buf[4096];
 	void			*chip_data; /* Per-chip extra variant data */
 };
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index a4d0abd5a6bf..972a73662641 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -103,6 +103,7 @@
 #include "realtek.h"
 #include "realtek-smi.h"
 #include "realtek-mdio.h"
+#include "realtek-common.h"
 
 /* Family-specific data and limits */
 #define RTL8365MB_PHYADDRMAX		7
@@ -691,7 +692,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
 	u32 val;
 	int ret;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = rtl8365mb_phy_poll_busy(priv);
 	if (ret)
@@ -724,7 +725,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
 	*data = val & 0xFFFF;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return ret;
 }
@@ -735,7 +736,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy,
 	u32 val;
 	int ret;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = rtl8365mb_phy_poll_busy(priv);
 	if (ret)
@@ -766,7 +767,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy,
 		goto out;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index cc2fd636ec23..e60a0a81d426 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -24,6 +24,7 @@
 #include "realtek.h"
 #include "realtek-smi.h"
 #include "realtek-mdio.h"
+#include "realtek-common.h"
 
 #define RTL8366RB_PORT_NUM_CPU		5
 #define RTL8366RB_NUM_PORTS		6
@@ -1707,7 +1708,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
 	if (phy > RTL8366RB_PHY_NO_MAX)
 		return -EINVAL;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
 			   RTL8366RB_PHY_CTRL_READ);
@@ -1735,7 +1736,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
 		phy, regnum, reg, val);
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return ret;
 }
@@ -1749,7 +1750,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	if (phy > RTL8366RB_PHY_NO_MAX)
 		return -EINVAL;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
 			   RTL8366RB_PHY_CTRL_WRITE);
@@ -1766,7 +1767,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 		goto out;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return ret;
 }
-- 
2.43.0

[PATCH net-next v3 4/8] net: dsa: realtek: merge common and interface modules into realtek-dsa

[Luiz Angelo Daros de Luca]

Since realtek-common and realtek-{smi,mdio} are always loaded together,
we can optimize resource usage by consolidating them into a single
module.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Kconfig          | 4 ++--
 drivers/net/dsa/realtek/Makefile         | 7 ++++---
 drivers/net/dsa/realtek/realtek-common.c | 1 +
 drivers/net/dsa/realtek/realtek-mdio.c   | 4 ----
 drivers/net/dsa/realtek/realtek-smi.c    | 4 ----
 5 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
index 9d182fde11b4..6989972eebc3 100644
--- a/drivers/net/dsa/realtek/Kconfig
+++ b/drivers/net/dsa/realtek/Kconfig
@@ -16,14 +16,14 @@ menuconfig NET_DSA_REALTEK
 if NET_DSA_REALTEK
 
 config NET_DSA_REALTEK_MDIO
-	tristate "Realtek MDIO interface support"
+	bool "Realtek MDIO interface support"
 	depends on OF
 	help
 	  Select to enable support for registering switches configured
 	  through MDIO.
 
 config NET_DSA_REALTEK_SMI
-	tristate "Realtek SMI interface support"
+	bool "Realtek SMI interface support"
 	depends on OF
 	help
 	  Select to enable support for registering switches connected
diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
index f4f9c6340d5f..cea0e761d20f 100644
--- a/drivers/net/dsa/realtek/Makefile
+++ b/drivers/net/dsa/realtek/Makefile
@@ -1,8 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_NET_DSA_REALTEK)		+= realtek-dsa.o
-realtek-dsa-objs			:= realtek-common.o
-obj-$(CONFIG_NET_DSA_REALTEK_MDIO) 	+= realtek-mdio.o
-obj-$(CONFIG_NET_DSA_REALTEK_SMI) 	+= realtek-smi.o
+realtek-dsa-objs-y			:= realtek-common.o
+realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
+realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
+realtek-dsa-objs			:= $(realtek-dsa-objs-y)
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
 rtl8366-objs 				:= rtl8366-core.o rtl8366rb.o
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
index 80b37e5fe780..dcf859ea78ab 100644
--- a/drivers/net/dsa/realtek/realtek-common.c
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -128,5 +128,6 @@ void realtek_common_remove(struct realtek_priv *priv)
 EXPORT_SYMBOL(realtek_common_remove);
 
 MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Realtek DSA switches common module");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 1eed09ab3aa1..967f6c1e8df0 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -177,7 +177,3 @@ void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 	dev_set_drvdata(&mdiodev->dev, NULL);
 }
 EXPORT_SYMBOL_GPL(realtek_mdio_shutdown);
-
-MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
-MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
-MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index fc54190839cf..2b2c6e34bae5 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -456,7 +456,3 @@ void realtek_smi_shutdown(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 }
 EXPORT_SYMBOL_GPL(realtek_smi_shutdown);
-
-MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
-MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
-MODULE_LICENSE("GPL");
-- 
2.43.0

[PATCH net-next v3 5/8] net: dsa: realtek: get internal MDIO node by name

[Luiz Angelo Daros de Luca]

The binding docs requires for SMI-connected devices that the switch
must have a child node named "mdio" and with a compatible string of
"realtek,smi-mdio". Meanwile, for MDIO-connected switches, the binding
docs only requires a child node named "mdio".

This patch changes the driver to use the common denominator for both
interfaces, looking for the MDIO node by name, ignoring the compatible
string.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-smi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 2b2c6e34bae5..9100b583ddc5 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -360,7 +360,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	struct device_node *mdio_np;
 	int ret;
 
-	mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-mdio");
+	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
 	if (!mdio_np) {
 		dev_err(priv->dev, "no MDIO bus node\n");
 		return -ENODEV;
-- 
2.43.0

[PATCH net-next v3 6/8] net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa

[Luiz Angelo Daros de Luca]

In the user MDIO driver, despite numerous references to SMI, including
its compatible string, there's nothing inherently specific about the SMI
interface in the user MDIO bus. Consequently, the code has been migrated
to the common module. All references to SMI have been eliminated.

The realtek-mdio will now use this driver instead of the generic DSA
driver ("dsa user smi"), which should not be used with OF[1].

The driver now looks for the MDIO node searchking for a child node named
"mdio" instead of using the compatible string. This requirement is
already present in binding docs for both interfaces.

The line assigning dev.of_node in mdio_bus has been removed since the
subsequent of_mdiobus_register will always overwrite it.

ds->user_mii_bus is not assigned anymore[2]. It should work as before as
long as the switch ports have a valid phy-handle property.

With a single ds_ops for both interfaces, the ds_ops in realtek_priv is
no longer necessary. Now, the realtek_variant.ds_ops can be used
directly.

The realtek_priv.setup_interface() has been removed as we can directly
call the new common function.

The switch unregistration and the MDIO node decrement in refcount were
moved into realtek_common_remove() as both interfaces now need to put
the MDIO node.

[1] https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/
[2] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-common.c | 65 +++++++++++++++++++++-
 drivers/net/dsa/realtek/realtek-common.h |  1 +
 drivers/net/dsa/realtek/realtek-mdio.c   |  6 ---
 drivers/net/dsa/realtek/realtek-smi.c    | 68 ------------------------
 drivers/net/dsa/realtek/realtek.h        |  5 +-
 drivers/net/dsa/realtek/rtl8365mb.c      | 49 +++--------------
 drivers/net/dsa/realtek/rtl8366rb.c      | 52 +++---------------
 7 files changed, 78 insertions(+), 168 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
index dcf859ea78ab..03fbc80f42b4 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,63 @@ 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;
+	int ret;
+
+	mdio_np = of_get_child_by_name(priv->dev->of_node, "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 = "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;
+
+	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
+	if (ret) {
+		dev_err(priv->dev, "unable to register MDIO bus %s\n",
+			priv->user_mii_bus->id);
+		goto err_put_node;
+	}
+
+	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(struct device *dev, struct regmap_config rc,
 		     struct regmap_config rc_nolock)
@@ -103,7 +161,7 @@ int realtek_common_register_switch(struct realtek_priv *priv)
 
 	priv->ds->priv = priv;
 	priv->ds->dev = priv->dev;
-	priv->ds->ops = priv->ds_ops;
+	priv->ds->ops = priv->variant->ds_ops;
 	priv->ds->num_ports = priv->num_ports;
 
 	ret = dsa_register_switch(priv->ds);
@@ -121,6 +179,11 @@ void realtek_common_remove(struct realtek_priv *priv)
 	if (!priv)
 		return;
 
+	dsa_unregister_switch(priv->ds);
+
+	if (priv->user_mii_bus)
+		of_node_put(priv->user_mii_bus->dev.of_node);
+
 	/* leave the device reset asserted */
 	if (priv->reset)
 		gpiod_set_value(priv->reset, 1);
diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
index 518d091ff496..b1c2a50d85cd 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(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 967f6c1e8df0..e2b5432eeb26 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -142,7 +142,6 @@ 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;
 
 	ret = realtek_common_register_switch(priv);
 	if (ret)
@@ -156,11 +155,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
 
-	if (!priv)
-		return;
-
-	dsa_unregister_switch(priv->ds);
-
 	realtek_common_remove(priv);
 }
 EXPORT_SYMBOL_GPL(realtek_mdio_remove);
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 9100b583ddc5..383689163057 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_child_by_name(priv->dev->of_node, "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->dev.of_node = mdio_np;
-	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);
-	if (ret) {
-		dev_err(priv->dev, "unable to register MDIO bus %s\n",
-			priv->user_mii_bus->id);
-		goto err_put_node;
-	}
-
-	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;
@@ -417,8 +359,6 @@ 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;
 
 	ret = realtek_common_register_switch(priv);
 	if (ret)
@@ -432,14 +372,6 @@ void realtek_smi_remove(struct platform_device *pdev)
 {
 	struct realtek_priv *priv = platform_get_drvdata(pdev);
 
-	if (!priv)
-		return;
-
-	dsa_unregister_switch(priv->ds);
-
-	if (priv->user_mii_bus)
-		of_node_put(priv->user_mii_bus->dev.of_node);
-
 	realtek_common_remove(priv);
 }
 EXPORT_SYMBOL_GPL(realtek_smi_remove);
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index fbd0616c1df3..7af6dcc1bb24 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -60,7 +60,6 @@ struct realtek_priv {
 
 	spinlock_t		lock; /* Locks around command writes */
 	struct dsa_switch	*ds;
-	const struct dsa_switch_ops *ds_ops;
 	struct irq_domain	*irqdomain;
 	bool			leds_disabled;
 
@@ -71,7 +70,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 +113,7 @@ 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;
 	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 972a73662641..81ec8f6f69c6 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -828,17 +828,6 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	return 0;
 }
 
-static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
-{
-	return rtl8365mb_phy_read(ds->priv, phy, regnum);
-}
-
-static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
-				   u16 val)
-{
-	return rtl8365mb_phy_write(ds->priv, phy, regnum, val);
-}
-
 static const struct rtl8365mb_extint *
 rtl8365mb_get_port_extint(struct realtek_priv *priv, int port)
 {
@@ -2017,12 +2006,10 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 	if (ret)
 		goto out_teardown_irq;
 
-	if (priv->setup_interface) {
-		ret = priv->setup_interface(ds);
-		if (ret) {
-			dev_err(priv->dev, "could not set up MDIO bus\n");
-			goto out_teardown_irq;
-		}
+	ret = realtek_common_setup_user_mdio(ds);
+	if (ret) {
+		dev_err(priv->dev, "could not set up MDIO bus\n");
+		goto out_teardown_irq;
 	}
 
 	/* Start statistics counter polling */
@@ -2116,28 +2103,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
-	.get_tag_protocol = rtl8365mb_get_tag_protocol,
-	.change_tag_protocol = rtl8365mb_change_tag_protocol,
-	.setup = rtl8365mb_setup,
-	.teardown = rtl8365mb_teardown,
-	.phylink_get_caps = rtl8365mb_phylink_get_caps,
-	.phylink_mac_config = rtl8365mb_phylink_mac_config,
-	.phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
-	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
-	.port_stp_state_set = rtl8365mb_port_stp_state_set,
-	.get_strings = rtl8365mb_get_strings,
-	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
-	.get_sset_count = rtl8365mb_get_sset_count,
-	.get_eth_phy_stats = rtl8365mb_get_phy_stats,
-	.get_eth_mac_stats = rtl8365mb_get_mac_stats,
-	.get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats,
-	.get_stats64 = rtl8365mb_get_stats64,
-	.port_change_mtu = rtl8365mb_port_change_mtu,
-	.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 = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
 	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
@@ -2146,8 +2112,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.phylink_mac_config = rtl8365mb_phylink_mac_config,
 	.phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
 	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
-	.phy_read = rtl8365mb_dsa_phy_read,
-	.phy_write = rtl8365mb_dsa_phy_write,
 	.port_stp_state_set = rtl8365mb_port_stp_state_set,
 	.get_strings = rtl8365mb_get_strings,
 	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
@@ -2167,8 +2131,7 @@ 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 = &rtl8365mb_switch_ops,
 	.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 e60a0a81d426..56619aa592ec 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1027,12 +1027,10 @@ 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 (ret) {
-			dev_err(priv->dev, "could not set up MDIO bus\n");
-			return -ENODEV;
-		}
+	ret = realtek_common_setup_user_mdio(ds);
+	if (ret) {
+		dev_err(priv->dev, "could not set up MDIO bus\n");
+		return -ENODEV;
 	}
 
 	return 0;
@@ -1772,17 +1770,6 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	return ret;
 }
 
-static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
-{
-	return rtl8366rb_phy_read(ds->priv, phy, regnum);
-}
-
-static int rtl8366rb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
-				   u16 val)
-{
-	return rtl8366rb_phy_write(ds->priv, phy, regnum, val);
-}
-
 static int rtl8366rb_reset_chip(struct realtek_priv *priv)
 {
 	int timeout = 10;
@@ -1848,35 +1835,9 @@ static int rtl8366rb_detect(struct realtek_priv *priv)
 	return 0;
 }
 
-static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
-	.get_tag_protocol = rtl8366_get_tag_protocol,
-	.setup = rtl8366rb_setup,
-	.phylink_get_caps = rtl8366rb_phylink_get_caps,
-	.phylink_mac_link_up = rtl8366rb_mac_link_up,
-	.phylink_mac_link_down = rtl8366rb_mac_link_down,
-	.get_strings = rtl8366_get_strings,
-	.get_ethtool_stats = rtl8366_get_ethtool_stats,
-	.get_sset_count = rtl8366_get_sset_count,
-	.port_bridge_join = rtl8366rb_port_bridge_join,
-	.port_bridge_leave = rtl8366rb_port_bridge_leave,
-	.port_vlan_filtering = rtl8366rb_vlan_filtering,
-	.port_vlan_add = rtl8366_vlan_add,
-	.port_vlan_del = rtl8366_vlan_del,
-	.port_enable = rtl8366rb_port_enable,
-	.port_disable = rtl8366rb_port_disable,
-	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
-	.port_bridge_flags = rtl8366rb_port_bridge_flags,
-	.port_stp_state_set = rtl8366rb_port_stp_state_set,
-	.port_fast_age = rtl8366rb_port_fast_age,
-	.port_change_mtu = rtl8366rb_change_mtu,
-	.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 = {
 	.get_tag_protocol = rtl8366_get_tag_protocol,
 	.setup = rtl8366rb_setup,
-	.phy_read = rtl8366rb_dsa_phy_read,
-	.phy_write = rtl8366rb_dsa_phy_write,
 	.phylink_get_caps = rtl8366rb_phylink_get_caps,
 	.phylink_mac_link_up = rtl8366rb_mac_link_up,
 	.phylink_mac_link_down = rtl8366rb_mac_link_down,
@@ -1915,8 +1876,7 @@ 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 = &rtl8366rb_switch_ops,
 	.ops = &rtl8366rb_ops,
 	.clk_delay = 10,
 	.cmd_read = 0xa9,
-- 
2.43.0

[PATCH net-next v3 7/8] net: dsa: realtek: embed dsa_switch into realtek_priv

[Luiz Angelo Daros de Luca]

To eliminate the need for a second memory allocation for dsa_switch, it
has been embedded within realtek_priv.

Suggested-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-common.c | 16 ++++++----------
 drivers/net/dsa/realtek/realtek-mdio.c   |  2 +-
 drivers/net/dsa/realtek/realtek-smi.c    |  2 +-
 drivers/net/dsa/realtek/realtek.h        |  2 +-
 drivers/net/dsa/realtek/rtl8365mb.c      | 12 ++++++------
 drivers/net/dsa/realtek/rtl8366rb.c      |  2 +-
 6 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
index 03fbc80f42b4..99c9270fd3e7 100644
--- a/drivers/net/dsa/realtek/realtek-common.c
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -155,16 +155,12 @@ int realtek_common_register_switch(struct realtek_priv *priv)
 		return ret;
 	}
 
-	priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return -ENOMEM;
+	priv->ds.priv = priv;
+	priv->ds.dev = priv->dev;
+	priv->ds.ops = priv->variant->ds_ops;
+	priv->ds.num_ports = priv->num_ports;
 
-	priv->ds->priv = priv;
-	priv->ds->dev = priv->dev;
-	priv->ds->ops = priv->variant->ds_ops;
-	priv->ds->num_ports = priv->num_ports;
-
-	ret = dsa_register_switch(priv->ds);
+	ret = dsa_register_switch(&priv->ds);
 	if (ret) {
 		dev_err_probe(priv->dev, ret, "unable to register switch\n");
 		return ret;
@@ -179,7 +175,7 @@ void realtek_common_remove(struct realtek_priv *priv)
 	if (!priv)
 		return;
 
-	dsa_unregister_switch(priv->ds);
+	dsa_unregister_switch(&priv->ds);
 
 	if (priv->user_mii_bus)
 		of_node_put(priv->user_mii_bus->dev.of_node);
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index e2b5432eeb26..0305b2f69b41 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -166,7 +166,7 @@ void realtek_mdio_shutdown(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
-	dsa_switch_shutdown(priv->ds);
+	dsa_switch_shutdown(&priv->ds);
 
 	dev_set_drvdata(&mdiodev->dev, NULL);
 }
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 383689163057..fa5a4c5e4210 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -383,7 +383,7 @@ void realtek_smi_shutdown(struct platform_device *pdev)
 	if (!priv)
 		return;
 
-	dsa_switch_shutdown(priv->ds);
+	dsa_switch_shutdown(&priv->ds);
 
 	platform_set_drvdata(pdev, NULL);
 }
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 7af6dcc1bb24..0217b8032c01 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -59,7 +59,7 @@ struct realtek_priv {
 	int			mdio_addr;
 
 	spinlock_t		lock; /* Locks around command writes */
-	struct dsa_switch	*ds;
+	struct dsa_switch	ds;
 	struct irq_domain	*irqdomain;
 	bool			leds_disabled;
 
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 81ec8f6f69c6..e710b636042e 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -880,7 +880,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
 	if (!extint)
 		return -ENODEV;
 
-	dp = dsa_to_port(priv->ds, port);
+	dp = dsa_to_port(&priv->ds, port);
 	dn = dp->dn;
 
 	/* Set the RGMII TX/RX delay
@@ -1543,7 +1543,7 @@ static void rtl8365mb_stats_setup(struct realtek_priv *priv)
 	for (i = 0; i < priv->num_ports; i++) {
 		struct rtl8365mb_port *p = &mb->ports[i];
 
-		if (dsa_is_unused_port(priv->ds, i))
+		if (dsa_is_unused_port(&priv->ds, i))
 			continue;
 
 		/* Per-port spinlock to protect the stats64 data */
@@ -1564,7 +1564,7 @@ static void rtl8365mb_stats_teardown(struct realtek_priv *priv)
 	for (i = 0; i < priv->num_ports; i++) {
 		struct rtl8365mb_port *p = &mb->ports[i];
 
-		if (dsa_is_unused_port(priv->ds, i))
+		if (dsa_is_unused_port(&priv->ds, i))
 			continue;
 
 		cancel_delayed_work_sync(&p->mib_work);
@@ -1963,7 +1963,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		dev_info(priv->dev, "no interrupt support\n");
 
 	/* Configure CPU tagging */
-	dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
+	dsa_switch_for_each_cpu_port(cpu_dp, &priv->ds) {
 		cpu->mask |= BIT(cpu_dp->index);
 
 		if (cpu->trap_port == RTL8365MB_MAX_NUM_PORTS)
@@ -1978,7 +1978,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 	for (i = 0; i < priv->num_ports; i++) {
 		struct rtl8365mb_port *p = &mb->ports[i];
 
-		if (dsa_is_unused_port(priv->ds, i))
+		if (dsa_is_unused_port(&priv->ds, i))
 			continue;
 
 		/* Forward only to the CPU */
@@ -1995,7 +1995,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		 * ports will still forward frames to the CPU despite being
 		 * administratively down by default.
 		 */
-		rtl8365mb_port_stp_state_set(priv->ds, i, BR_STATE_DISABLED);
+		rtl8365mb_port_stp_state_set(&priv->ds, i, BR_STATE_DISABLED);
 
 		/* Set up per-port private data */
 		p->priv = priv;
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 56619aa592ec..f5b32c77db7f 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1662,7 +1662,7 @@ static int rtl8366rb_set_mc_index(struct realtek_priv *priv, int port, int index
 	 * not drop any untagged or C-tagged frames. Make sure to update the
 	 * filtering setting.
 	 */
-	if (dsa_port_is_vlan_filtering(dsa_to_port(priv->ds, port)))
+	if (dsa_port_is_vlan_filtering(dsa_to_port(&priv->ds, port)))
 		ret = rtl8366rb_drop_untagged(priv, port, !pvid_enabled);
 
 	return ret;
-- 
2.43.0

[PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

[Luiz Angelo Daros de Luca]

This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1.

The use of user_mii_bus is inappropriate when the hardware is described
with a device-tree [1].

Since all drivers currently implementing ds_switch_ops.phy_{read,write}
were not updated to utilize the MDIO information from OF with the
generic "dsa user mii", they might not be affected by this change.

[1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 net/dsa/dsa.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ac7be864e80d..09d2f5d4b3dd 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -15,7 +15,6 @@
 #include <linux/slab.h>
 #include <linux/rtnetlink.h>
 #include <linux/of.h>
-#include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <net/dsa_stubs.h>
 #include <net/sch_generic.h>
@@ -626,7 +625,6 @@ static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds)
 
 static int dsa_switch_setup(struct dsa_switch *ds)
 {
-	struct device_node *dn;
 	int err;
 
 	if (ds->setup)
@@ -666,10 +664,7 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 
 		dsa_user_mii_bus_init(ds);
 
-		dn = of_get_child_by_name(ds->dev->of_node, "mdio");
-
-		err = of_mdiobus_register(ds->user_mii_bus, dn);
-		of_node_put(dn);
+		err = mdiobus_register(ds->user_mii_bus);
 		if (err < 0)
 			goto free_user_mii_bus;
 	}
-- 
2.43.0

Re: [PATCH net-next v3 1/8] net: dsa: realtek: drop cleanup from realtek_ops

[Florian Fainelli]

On 12/23/2023 1:46 AM, Luiz Angelo Daros de Luca wrote:
> It was never used and never referenced.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net-next v3 1/8] net: dsa: realtek: drop cleanup from realtek_ops

[Linus Walleij]

On Sat, Dec 23, 2023 at 1:53 AM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> It was never used and never referenced.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

[Arınç ÜNAL]

On 23.12.2023 03:46, Luiz Angelo Daros de Luca wrote:
> This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1.
> 
> The use of user_mii_bus is inappropriate when the hardware is described
> with a device-tree [1].
> 
> Since all drivers currently implementing ds_switch_ops.phy_{read,write}
> were not updated to utilize the MDIO information from OF with the
> generic "dsa user mii", they might not be affected by this change.
> 
> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u

This doesn't make sense to me: "The use of user_mii_bus is inappropriate
when the hardware is described with a device-tree." I think this is the
correct statement: "The use of user_mii_bus is inappropriate when the MDIO
bus of the switch is described on the device-tree."

The patch log is also not clear on why this leads to the removal of
OF-based registration from the DSA core driver.

I would change the patch log here to something like this:

net: dsa: do not populate user_mii_bus when switch MDIO bus is described

The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
switch is described on the device-tree [1].

To keep things simple, make [all subdrivers that control a switch [with
MDIO bus] probed on OF] register the bus on the subdriver.

The subdrivers that control switches [with MDIO bus] probed on OF must
follow this logic to support all cases properly:

No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if "interrupt-controller" is defined at
the switch node.

Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if ["interrupt-controller" is defined at
the switch node and "interrupts" is defined at the PHY nodes under the
switch MDIO bus node].

There can be a case where ds->user_mii_bus is not populated, but
ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls a
switch probed on OF, and it lets the DSA core driver to populate
ds->user_mii_bus and register the bus. We don't want this to happen.
Therefore, ds_switch_ops.phy_{read,write} should be only used on the
subdrivers that control switches probed on platform_data.

With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
dsa_switch_setup() remains in use only for switches probed on
platform_data. Therefore, remove OF-based registration as it's useless now.

---

I think we should do all this in a single patch. I've done it on the MT7530
DSA subdriver which I maintain.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 391c4dbdff42..bbd230a73ead 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2155,15 +2155,21 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
  {
  	struct dsa_switch *ds = priv->ds;
  	struct device *dev = priv->dev;
+	struct device_node *np, *mnp;
  	struct mii_bus *bus;
  	static int idx;
  	int ret;
  
+	np = priv->dev->of_node;
+	mnp = of_get_child_by_name(np, "mdio");
+
  	bus = devm_mdiobus_alloc(dev);
  	if (!bus)
  		return -ENOMEM;
  
-	ds->user_mii_bus = bus;
+	if (mnp == NULL)
+		ds->user_mii_bus = bus;
+
  	bus->priv = priv;
  	bus->name = KBUILD_MODNAME "-mii";
  	snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
@@ -2174,10 +2180,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
  	bus->parent = dev;
  	bus->phy_mask = ~ds->phys_mii_mask;
  
-	if (priv->irq)
+	if (priv->irq && mnp == NULL)
  		mt7530_setup_mdio_irq(priv);
  
-	ret = devm_mdiobus_register(dev, bus);
+	ret = devm_of_mdiobus_register(dev, bus, mnp);
+	of_node_put(mnp);
  	if (ret) {
  		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
  		if (priv->irq)

Arınç

Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

[Arınç ÜNAL]

On 30.12.2023 10:18, Arınç ÜNAL wrote:
> On 23.12.2023 03:46, Luiz Angelo Daros de Luca wrote:
>> This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1.
>>
>> The use of user_mii_bus is inappropriate when the hardware is described
>> with a device-tree [1].
>>
>> Since all drivers currently implementing ds_switch_ops.phy_{read,write}
>> were not updated to utilize the MDIO information from OF with the
>> generic "dsa user mii", they might not be affected by this change.
>>
>> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u
> 
> This doesn't make sense to me: "The use of user_mii_bus is inappropriate
> when the hardware is described with a device-tree." I think this is the
> correct statement: "The use of user_mii_bus is inappropriate when the MDIO
> bus of the switch is described on the device-tree."
> 
> The patch log is also not clear on why this leads to the removal of
> OF-based registration from the DSA core driver.
> 
> I would change the patch log here to something like this:
> 
> net: dsa: do not populate user_mii_bus when switch MDIO bus is described
> 
> The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
> switch is described on the device-tree [1].
> 
> To keep things simple, make [all subdrivers that control a switch [with
> MDIO bus] probed on OF] register the bus on the subdriver.
> 
> The subdrivers that control switches [with MDIO bus] probed on OF must
> follow this logic to support all cases properly:
> 
> No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO
> bus, set the interrupts for PHYs if "interrupt-controller" is defined at
> the switch node.
> 
> Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO
> bus, set the interrupts for PHYs if ["interrupt-controller" is defined at
> the switch node and "interrupts" is defined at the PHY nodes under the
> switch MDIO bus node].
> 
> There can be a case where ds->user_mii_bus is not populated, but
> ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls a
> switch probed on OF, and it lets the DSA core driver to populate
> ds->user_mii_bus and register the bus. We don't want this to happen.
> Therefore, ds_switch_ops.phy_{read,write} should be only used on the
> subdrivers that control switches probed on platform_data.
> 
> With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
> dsa_switch_setup() remains in use only for switches probed on
> platform_data. Therefore, remove OF-based registration as it's useless now.
> 
> ---
> 
> I think we should do all this in a single patch. I've done it on the MT7530
> DSA subdriver which I maintain.

Actually, there's no need to drag this patch further by including the
improvement of handling the MDIO bus on all relevant subdrivers.

That said, I'd like to submit this patch myself, if it is OK by everyone
here.

Here's the patch log I've prepared:

net: dsa: do not populate user_mii_bus when switch MDIO bus is described

The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
switch is described on the device-tree [1].

To keep things simple, make [all subdrivers that control switches [with
MDIO bus] probed on OF] register the bus on the subdriver. This is already
the case on all of these subdrivers.

There can be a case where ds->user_mii_bus is not populated, but
ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls
switches probed on OF, and it lets the DSA core driver to populate
ds->user_mii_bus and register the bus. We don't want this to happen.
Therefore, ds_switch_ops.phy_{read,write} should be only used on the
subdrivers that control switches probed on platform_data.

With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
dsa_switch_setup() remains in use only for switches probed on
platform_data. Therefore, remove OF-based registration as it's useless now.

Currently, none of the subdrivers that control switches [with MDIO bus]
probed on OF implements ds_switch_ops.phy_{read,write} so no subdriver will
be affected by this patch.

The subdrivers that control switches [with MDIO bus] probed on OF must
follow this logic to support all cases properly:

No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if "interrupt-controller" is defined at
the switch node. This case should only be covered for the switches which
their dt-bindings documentation didn't document the MDIO bus from the
start. This is to keep supporting the device trees that do not describe the
MDIO bus on the device tree but the MDIO bus is being used nonetheless.

Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if ["interrupt-controller" is defined at
the switch node and "interrupts" is defined at the PHY nodes under the
switch MDIO bus node].

Let's leave the improvement of handling the MDIO bus on relevant subdrivers
to future patches.

Arınç

Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

[Arınç ÜNAL]

On 30.12.2023 18:56, Arınç ÜNAL wrote:
> On 30.12.2023 10:18, Arınç ÜNAL wrote:
>> I think we should do all this in a single patch. I've done it on the MT7530
>> DSA subdriver which I maintain.
> 
> Actually, there's no need to drag this patch further by including the
> improvement of handling the MDIO bus on all relevant subdrivers.
> 
> That said, I'd like to submit this patch myself, if it is OK by everyone
> here.
> 
> Here's the patch log I've prepared:
> 
> net: dsa: do not populate user_mii_bus when switch MDIO bus is described
> 
> The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
> switch is described on the device-tree [1].
> 
> To keep things simple, make [all subdrivers that control switches [with
> MDIO bus] probed on OF] register the bus on the subdriver. This is already
> the case on all of these subdrivers.
> 
> There can be a case where ds->user_mii_bus is not populated, but
> ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls
> switches probed on OF, and it lets the DSA core driver to populate
> ds->user_mii_bus and register the bus. We don't want this to happen.
> Therefore, ds_switch_ops.phy_{read,write} should be only used on the
> subdrivers that control switches probed on platform_data.
> 
> With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
> dsa_switch_setup() remains in use only for switches probed on
> platform_data. Therefore, remove OF-based registration as it's useless now.
> 
> Currently, none of the subdrivers that control switches [with MDIO bus]
> probed on OF implements ds_switch_ops.phy_{read,write} so no subdriver will
> be affected by this patch.

It looks like this patch will cause the MDIO bus of the switches probed on
OF which are controlled by these subdrivers to only be registered
non-OF-based.

drivers/net/dsa/b53/b53_common.c
drivers/net/dsa/lan9303-core.c
drivers/net/dsa/vitesse-vsc73xx-core.c

These subdrivers let the DSA driver register the bus OF-based or
non-OF-based:
- ds->ops->phy_read() and ds->ops->phy_write() are present.
- ds->user_mii_bus is not populated.

Not being able to register the bus OF-based may cause issues. There is an
example for the switch on the MT7988 SoC which is controlled by the MT7530
DSA subdriver. Being able to reference the PHYs on the switch MDIO bus is
mandatory on MT7988 as calibration data from NVMEM for each PHY is
required.

I suggest that we hold off on this patch until these subdrivers are made to
be capable of registering the MDIO bus as OF-based on their own.

Arınç

Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

[Luiz Angelo Daros de Luca]

Hi Arinç,

> It looks like this patch will cause the MDIO bus of the switches probed on
> OF which are controlled by these subdrivers to only be registered
> non-OF-based.
>
> drivers/net/dsa/b53/b53_common.c
> drivers/net/dsa/lan9303-core.c
> drivers/net/dsa/vitesse-vsc73xx-core.c
>
> These subdrivers let the DSA driver register the bus OF-based or
> non-OF-based:
> - ds->ops->phy_read() and ds->ops->phy_write() are present.
> - ds->user_mii_bus is not populated.

I checked the changes on those drivers since
fe7324b932222574a0721b80e72c6c5fe57960d1 and nothing indicates that
they were changing anything related to the user mii bus. I also
checked bindings for the mdio node requirement. None of them mentioned
the mdio node.

> Not being able to register the bus OF-based may cause issues. There is an
> example for the switch on the MT7988 SoC which is controlled by the MT7530
> DSA subdriver. Being able to reference the PHYs on the switch MDIO bus is
> mandatory on MT7988 as calibration data from NVMEM for each PHY is
> required.
>
> I suggest that we hold off on this patch until these subdrivers are made to
> be capable of registering the MDIO bus as OF-based on their own.

We might be over cautious keeping this for more time after the realtek
refactoring gets merged. The using OF with the generic user mii bus
driver is just a broken design and probably not in use. Anyway, it is
not a requirement for the series. If there is no objection, I can drop
it.

I would like to send v4 with the OF node handling simplified by the
change in the MDIO API. However, I'm reluctant to send mostly the same
code without any reviews.

Regards,

Luiz

Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

[Arınç ÜNAL]

On 8.01.2024 07:44, Luiz Angelo Daros de Luca wrote:
> Hi Arinç,
> 
>> It looks like this patch will cause the MDIO bus of the switches probed on
>> OF which are controlled by these subdrivers to only be registered
>> non-OF-based.
>>
>> drivers/net/dsa/b53/b53_common.c
>> drivers/net/dsa/lan9303-core.c
>> drivers/net/dsa/vitesse-vsc73xx-core.c
>>
>> These subdrivers let the DSA driver register the bus OF-based or
>> non-OF-based:
>> - ds->ops->phy_read() and ds->ops->phy_write() are present.
>> - ds->user_mii_bus is not populated.
> 
> I checked the changes on those drivers since
> fe7324b932222574a0721b80e72c6c5fe57960d1 and nothing indicates that
> they were changing anything related to the user mii bus. I also
> checked bindings for the mdio node requirement. None of them mentioned
> the mdio node.

Ok, we need to specifically mention the latter on the patch log.

> 
>> Not being able to register the bus OF-based may cause issues. There is an
>> example for the switch on the MT7988 SoC which is controlled by the MT7530
>> DSA subdriver. Being able to reference the PHYs on the switch MDIO bus is
>> mandatory on MT7988 as calibration data from NVMEM for each PHY is
>> required.
>>
>> I suggest that we hold off on this patch until these subdrivers are made to
>> be capable of registering the MDIO bus as OF-based on their own.
> 
> We might be over cautious keeping this for more time after the realtek
> refactoring gets merged. The using OF with the generic user mii bus
> driver is just a broken design and probably not in use. Anyway, it is
> not a requirement for the series. If there is no objection, I can drop
> it.

Sounds good to me. I'd like to take this patch off your hands.

> 
> I would like to send v4 with the OF node handling simplified by the
> change in the MDIO API. However, I'm reluctant to send mostly the same
> code without any reviews.

I suppose our conversation here will remind Vladimir and Alvin to review.

Arınç

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Vladimir Oltean]

On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote:
> Some code can be shared between both interface modules (MDIO and SMI)
> and among variants. These interface functions migrated to a common
> module:
> 
> - realtek_common_lock
> - realtek_common_unlock
> - realtek_common_probe
> - realtek_common_register_switch
> - realtek_common_remove
> 
> The reset during probe was moved to the end of the common probe. This way,
> we avoid a reset if anything else fails.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
> diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> new file mode 100644
> index 000000000000..80b37e5fe780
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-common.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/module.h>
> +
> +#include "realtek.h"
> +#include "realtek-common.h"
> +
> +void realtek_common_lock(void *ctx)
> +{
> +	struct realtek_priv *priv = ctx;
> +
> +	mutex_lock(&priv->map_lock);
> +}
> +EXPORT_SYMBOL_GPL(realtek_common_lock);

Would you mind adding some kernel-doc comments above each of these
exported functions? https://docs.kernel.org/doc-guide/kernel-doc.html
says "Every function that is exported to loadable modules using
EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc comment.
Functions and data structures in header files which are intended to be
used by modules should also have kernel-doc comments."

It is something I only recently started paying attention to, so we don't
have consistency in this regard. But we should try to adhere to this
practice for code we change.

> +
> +void realtek_common_unlock(void *ctx)
> +{
> +	struct realtek_priv *priv = ctx;
> +
> +	mutex_unlock(&priv->map_lock);
> +}
> +EXPORT_SYMBOL_GPL(realtek_common_unlock);
> +
> +struct realtek_priv *
> +realtek_common_probe(struct device *dev, struct regmap_config rc,
> +		     struct regmap_config rc_nolock)

Could you use "const struct regmap_config *" as the data types here, to
avoid two on-stack variable copies? Regmap will copy the config structures
anyway.

> +{
> +	const struct realtek_variant *var;
> +	struct realtek_priv *priv;
> +	int ret;
> +
> +	var = of_device_get_match_data(dev);
> +	if (!var)
> +		return ERR_PTR(-EINVAL);
> +
> +	priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_init(&priv->map_lock);
> +
> +	rc.lock_arg = priv;
> +	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> +	if (IS_ERR(priv->map)) {
> +		ret = PTR_ERR(priv->map);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock);
> +	if (IS_ERR(priv->map_nolock)) {
> +		ret = PTR_ERR(priv->map_nolock);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	/* Link forward and backward */
> +	priv->dev = dev;
> +	priv->variant = var;
> +	priv->ops = var->ops;
> +	priv->chip_data = (void *)priv + sizeof(*priv);
> +
> +	dev_set_drvdata(dev, priv);
> +	spin_lock_init(&priv->lock);
> +
> +	priv->leds_disabled = of_property_read_bool(dev->of_node,
> +						    "realtek,disable-leds");
> +
> +	/* TODO: if power is software controlled, set up any regulators here */
> +
> +	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->reset)) {
> +		dev_err(dev, "failed to get RESET GPIO\n");
> +		return ERR_CAST(priv->reset);
> +	}
> +	if (priv->reset) {
> +		gpiod_set_value(priv->reset, 1);
> +		dev_dbg(dev, "asserted RESET\n");
> +		msleep(REALTEK_HW_STOP_DELAY);
> +		gpiod_set_value(priv->reset, 0);
> +		msleep(REALTEK_HW_START_DELAY);
> +		dev_dbg(dev, "deasserted RESET\n");
> +	}
> +
> +	return priv;
> +}
> +EXPORT_SYMBOL(realtek_common_probe);
> diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> index e9ee778665b2..fbd0616c1df3 100644
> --- a/drivers/net/dsa/realtek/realtek.h
> +++ b/drivers/net/dsa/realtek/realtek.h
> @@ -58,11 +58,9 @@ struct realtek_priv {
>  	struct mii_bus		*bus;
>  	int			mdio_addr;
>  
> -	unsigned int		clk_delay;
> -	u8			cmd_read;
> -	u8			cmd_write;
>  	spinlock_t		lock; /* Locks around command writes */
>  	struct dsa_switch	*ds;
> +	const struct dsa_switch_ops *ds_ops;
>  	struct irq_domain	*irqdomain;
>  	bool			leds_disabled;
>  
> @@ -79,6 +77,8 @@ struct realtek_priv {
>  	int			vlan_enabled;
>  	int			vlan4k_enabled;
>  
> +	const struct realtek_variant *variant;
> +
>  	char			buf[4096];
>  	void			*chip_data; /* Per-chip extra variant data */
>  };

Can the changes to struct realtek_priv be a separate patch, to
clarify what is being changed, and to leave the noisy code movement
more isolated?

Re: [PATCH net-next v3 4/8] net: dsa: realtek: merge common and interface modules into realtek-dsa

[Vladimir Oltean]

On Fri, Dec 22, 2023 at 09:46:32PM -0300, Luiz Angelo Daros de Luca wrote:
> diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
> index f4f9c6340d5f..cea0e761d20f 100644
> --- a/drivers/net/dsa/realtek/Makefile
> +++ b/drivers/net/dsa/realtek/Makefile
> @@ -1,8 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_NET_DSA_REALTEK)		+= realtek-dsa.o
> -realtek-dsa-objs			:= realtek-common.o
> -obj-$(CONFIG_NET_DSA_REALTEK_MDIO) 	+= realtek-mdio.o
> -obj-$(CONFIG_NET_DSA_REALTEK_SMI) 	+= realtek-smi.o
> +realtek-dsa-objs-y			:= realtek-common.o
> +realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
> +realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
> +realtek-dsa-objs			:= $(realtek-dsa-objs-y)
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
>  rtl8366-objs 				:= rtl8366-core.o rtl8366rb.o
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o

Does "realtek-dsa-objs-y" have any particular meaning in the Kbuild
system, or is it just a random variable name?

Am I the only one for whom this is clearer in intent?

diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
index cea0e761d20f..418f8bff77b8 100644
--- a/drivers/net/dsa/realtek/Makefile
+++ b/drivers/net/dsa/realtek/Makefile
@@ -1,9 +1,15 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_NET_DSA_REALTEK)		+= realtek-dsa.o
-realtek-dsa-objs-y			:= realtek-common.o
-realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
-realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
-realtek-dsa-objs			:= $(realtek-dsa-objs-y)
+realtek-dsa-objs			:= realtek-common.o
+
+ifdef CONFIG_NET_DSA_REALTEK_MDIO
+realtek-dsa-objs += realtek-mdio.o
+endif
+
+ifdef CONFIG_NET_DSA_REALTEK_SMI
+realtek-dsa-objs += realtek-smi.o
+endif
+
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
 rtl8366-objs 				:= rtl8366-core.o rtl8366rb.o
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o

Re: [PATCH net-next v3 5/8] net: dsa: realtek: get internal MDIO node by name

[Vladimir Oltean]

On Fri, Dec 22, 2023 at 09:46:33PM -0300, Luiz Angelo Daros de Luca wrote:
> The binding docs requires for SMI-connected devices that the switch
> must have a child node named "mdio" and with a compatible string of
> "realtek,smi-mdio". Meanwile, for MDIO-connected switches, the binding
> docs only requires a child node named "mdio".
> 
> This patch changes the driver to use the common denominator for both
> interfaces, looking for the MDIO node by name, ignoring the compatible
> string.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v3 6/8] net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa

[Vladimir Oltean]

On Fri, Dec 22, 2023 at 09:46:34PM -0300, Luiz Angelo Daros de Luca wrote:
> In the user MDIO driver, despite numerous references to SMI, including
> its compatible string, there's nothing inherently specific about the SMI
> interface in the user MDIO bus. Consequently, the code has been migrated
> to the common module. All references to SMI have been eliminated.
> 
> The realtek-mdio will now use this driver instead of the generic DSA
> driver ("dsa user smi"), which should not be used with OF[1].
> 
> The driver now looks for the MDIO node searchking for a child node named
> "mdio" instead of using the compatible string. This requirement is
> already present in binding docs for both interfaces.
> 
> The line assigning dev.of_node in mdio_bus has been removed since the
> subsequent of_mdiobus_register will always overwrite it.
> 
> ds->user_mii_bus is not assigned anymore[2]. It should work as before as
> long as the switch ports have a valid phy-handle property.
> 
> With a single ds_ops for both interfaces, the ds_ops in realtek_priv is
> no longer necessary. Now, the realtek_variant.ds_ops can be used
> directly.
> 
> The realtek_priv.setup_interface() has been removed as we can directly
> call the new common function.
> 
> The switch unregistration and the MDIO node decrement in refcount were
> moved into realtek_common_remove() as both interfaces now need to put
> the MDIO node.

Can you please make all these individual changes separate patches,
some before moving the code to realtek_common, and some after?
There's a lot of stuff happening at once here.

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Luiz Angelo Daros de Luca]

Em seg., 8 de jan. de 2024 às 11:00, Vladimir Oltean
<olteanv@gmail.com> escreveu:
>
> On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote:
> > Some code can be shared between both interface modules (MDIO and SMI)
> > and among variants. These interface functions migrated to a common
> > module:
> >
> > - realtek_common_lock
> > - realtek_common_unlock
> > - realtek_common_probe
> > - realtek_common_register_switch
> > - realtek_common_remove
> >
> > The reset during probe was moved to the end of the common probe. This way,
> > we avoid a reset if anything else fails.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> > new file mode 100644
> > index 000000000000..80b37e5fe780
> > --- /dev/null
> > +++ b/drivers/net/dsa/realtek/realtek-common.c
> > @@ -0,0 +1,132 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <linux/module.h>
> > +
> > +#include "realtek.h"
> > +#include "realtek-common.h"
> > +
> > +void realtek_common_lock(void *ctx)
> > +{
> > +     struct realtek_priv *priv = ctx;
> > +
> > +     mutex_lock(&priv->map_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_common_lock);
>
> Would you mind adding some kernel-doc comments above each of these
> exported functions? https://docs.kernel.org/doc-guide/kernel-doc.html
> says "Every function that is exported to loadable modules using
> EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc comment.
> Functions and data structures in header files which are intended to be
> used by modules should also have kernel-doc comments."
>
> It is something I only recently started paying attention to, so we don't
> have consistency in this regard. But we should try to adhere to this
> practice for code we change.
>

Sure. I'll pay attention to that too.

> > +
> > +void realtek_common_unlock(void *ctx)
> > +{
> > +     struct realtek_priv *priv = ctx;
> > +
> > +     mutex_unlock(&priv->map_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_common_unlock);
> > +
> > +struct realtek_priv *
> > +realtek_common_probe(struct device *dev, struct regmap_config rc,
> > +                  struct regmap_config rc_nolock)
>
> Could you use "const struct regmap_config *" as the data types here, to
> avoid two on-stack variable copies? Regmap will copy the config structures
> anyway.

I could do that for rc_nolock but not for rc as we need to modify it
before passing to regmap. I would still need to duplicate rc, either
using the stack or heap. What would be the best option?

1) pass two pointers and copy one to stack
2) pass two pointers and copy one to heap
3) pass two structs (as it is today)
4) pass one pointer and one struct

The old code was using 1) and I'm inclined to adopt it and save a
hundred and so bytes from the stack, although 2) would save even more.

> > +{
> > +     const struct realtek_variant *var;
> > +     struct realtek_priv *priv;
> > +     int ret;
> > +
> > +     var = of_device_get_match_data(dev);
> > +     if (!var)
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
> > +                         GFP_KERNEL);
> > +     if (!priv)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     mutex_init(&priv->map_lock);
> > +
> > +     rc.lock_arg = priv;
> > +     priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> > +     if (IS_ERR(priv->map)) {
> > +             ret = PTR_ERR(priv->map);
> > +             dev_err(dev, "regmap init failed: %d\n", ret);
> > +             return ERR_PTR(ret);
> > +     }
> > +
> > +     priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock);
> > +     if (IS_ERR(priv->map_nolock)) {
> > +             ret = PTR_ERR(priv->map_nolock);
> > +             dev_err(dev, "regmap init failed: %d\n", ret);
> > +             return ERR_PTR(ret);
> > +     }
> > +
> > +     /* Link forward and backward */
> > +     priv->dev = dev;
> > +     priv->variant = var;
> > +     priv->ops = var->ops;
> > +     priv->chip_data = (void *)priv + sizeof(*priv);
> > +
> > +     dev_set_drvdata(dev, priv);
> > +     spin_lock_init(&priv->lock);
> > +
> > +     priv->leds_disabled = of_property_read_bool(dev->of_node,
> > +                                                 "realtek,disable-leds");
> > +
> > +     /* TODO: if power is software controlled, set up any regulators here */
> > +
> > +     priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(priv->reset)) {
> > +             dev_err(dev, "failed to get RESET GPIO\n");
> > +             return ERR_CAST(priv->reset);
> > +     }
> > +     if (priv->reset) {
> > +             gpiod_set_value(priv->reset, 1);
> > +             dev_dbg(dev, "asserted RESET\n");
> > +             msleep(REALTEK_HW_STOP_DELAY);
> > +             gpiod_set_value(priv->reset, 0);
> > +             msleep(REALTEK_HW_START_DELAY);
> > +             dev_dbg(dev, "deasserted RESET\n");
> > +     }
> > +
> > +     return priv;
> > +}
> > +EXPORT_SYMBOL(realtek_common_probe);
> > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> > index e9ee778665b2..fbd0616c1df3 100644
> > --- a/drivers/net/dsa/realtek/realtek.h
> > +++ b/drivers/net/dsa/realtek/realtek.h
> > @@ -58,11 +58,9 @@ struct realtek_priv {
> >       struct mii_bus          *bus;
> >       int                     mdio_addr;
> >
> > -     unsigned int            clk_delay;
> > -     u8                      cmd_read;
> > -     u8                      cmd_write;
> >       spinlock_t              lock; /* Locks around command writes */
> >       struct dsa_switch       *ds;
> > +     const struct dsa_switch_ops *ds_ops;
> >       struct irq_domain       *irqdomain;
> >       bool                    leds_disabled;
> >
> > @@ -79,6 +77,8 @@ struct realtek_priv {
> >       int                     vlan_enabled;
> >       int                     vlan4k_enabled;
> >
> > +     const struct realtek_variant *variant;
> > +
> >       char                    buf[4096];
> >       void                    *chip_data; /* Per-chip extra variant data */
> >  };
>
> Can the changes to struct realtek_priv be a separate patch, to
> clarify what is being changed, and to leave the noisy code movement
> more isolated?

Sure, although it will not be a patch that makes sense by itself. If
it helps with the review, I'll split it. We can fold it back if
needed.

Regards,

Luiz

Re: [PATCH net-next v3 4/8] net: dsa: realtek: merge common and interface modules into realtek-dsa

[Luiz Angelo Daros de Luca]

> > +++ b/drivers/net/dsa/realtek/Makefile
> > @@ -1,8 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_NET_DSA_REALTEK)                += realtek-dsa.o
> > -realtek-dsa-objs                     := realtek-common.o
> > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO)   += realtek-mdio.o
> > -obj-$(CONFIG_NET_DSA_REALTEK_SMI)    += realtek-smi.o
> > +realtek-dsa-objs-y                   := realtek-common.o
> > +realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
> > +realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
> > +realtek-dsa-objs                     := $(realtek-dsa-objs-y)
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
> >  rtl8366-objs                                 := rtl8366-core.o rtl8366rb.o
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
>
> Does "realtek-dsa-objs-y" have any particular meaning in the Kbuild
> system, or is it just a random variable name?
>
> Am I the only one for whom this is clearer in intent?
>
> diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
> index cea0e761d20f..418f8bff77b8 100644
> --- a/drivers/net/dsa/realtek/Makefile
> +++ b/drivers/net/dsa/realtek/Makefile
> @@ -1,9 +1,15 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_NET_DSA_REALTEK)          += realtek-dsa.o
> -realtek-dsa-objs-y                     := realtek-common.o
> -realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
> -realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
> -realtek-dsa-objs                       := $(realtek-dsa-objs-y)
> +realtek-dsa-objs                       := realtek-common.o
> +
> +ifdef CONFIG_NET_DSA_REALTEK_MDIO
> +realtek-dsa-objs += realtek-mdio.o
> +endif
> +
> +ifdef CONFIG_NET_DSA_REALTEK_SMI
> +realtek-dsa-objs += realtek-smi.o
> +endif
> +
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
>  rtl8366-objs                           := rtl8366-core.o rtl8366rb.o
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o

I also prefer ifdef but it was suggested to use the realtek-dsa-objs-y
magic and nobody argued about that. It is an easy fix.

Regards,

Luiz

Re: [PATCH net-next v3 6/8] net: dsa: realtek: migrate user_mii_bus setup to realtek-dsa

[Luiz Angelo Daros de Luca]

> Can you please make all these individual changes separate patches,
> some before moving the code to realtek_common, and some after?
> There's a lot of stuff happening at once here.

Sure. I'll split into 3 parts:

1) change the user mdio bus setup code in place (with some cleanups
related to that)
2) move the code to common, just changing strings
3) make realtek-mdio use it and remove duplicated code.

Regards,

Luiz

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Vladimir Oltean]

On Tue, Jan 09, 2024 at 02:05:29AM -0300, Luiz Angelo Daros de Luca wrote:
> > > +struct realtek_priv *
> > > +realtek_common_probe(struct device *dev, struct regmap_config rc,
> > > +                  struct regmap_config rc_nolock)
> >
> > Could you use "const struct regmap_config *" as the data types here, to
> > avoid two on-stack variable copies? Regmap will copy the config structures
> > anyway.
> 
> I could do that for rc_nolock but not for rc as we need to modify it
> before passing to regmap. I would still need to duplicate rc, either
> using the stack or heap. What would be the best option?
> 
> 1) pass two pointers and copy one to stack
> 2) pass two pointers and copy one to heap
> 3) pass two structs (as it is today)
> 4) pass one pointer and one struct
> 
> The old code was using 1) and I'm inclined to adopt it and save a
> hundred and so bytes from the stack, although 2) would save even more.

I didn't notice the "rc.lock_arg = priv" assignment...

I'm not sure what you mean by "copy to heap". Perform a dynamic memory
allocation?

Also, the old code was not using exactly 1). It copied both the normal
and the nolock regmap config to an on-stack local variable, even though
only the normal regmap config had to be copied (to be fixed up).

I went back to study the 4 regmap configs, and only the reg_read() and
reg_write() methods differ between SMI and MDIO. The rest seems boilerplate
that can be dynamically constructed by realtek_common_probe(). Sure,
spelling out 4 regmap_config structures is more flexible, but do we need
that flexibility? What if realtek_common_probe() takes just the
reg_read() and reg_write() function prototypes as arguments, rather than
pointers to regmap_config structures it then has to fix up?

> > > +EXPORT_SYMBOL(realtek_common_probe);
> > > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> > > index e9ee778665b2..fbd0616c1df3 100644
> > > --- a/drivers/net/dsa/realtek/realtek.h
> > > +++ b/drivers/net/dsa/realtek/realtek.h
> > > @@ -58,11 +58,9 @@ struct realtek_priv {
> > >       struct mii_bus          *bus;
> > >       int                     mdio_addr;
> > >
> > > -     unsigned int            clk_delay;
> > > -     u8                      cmd_read;
> > > -     u8                      cmd_write;
> > >       spinlock_t              lock; /* Locks around command writes */
> > >       struct dsa_switch       *ds;
> > > +     const struct dsa_switch_ops *ds_ops;
> > >       struct irq_domain       *irqdomain;
> > >       bool                    leds_disabled;
> > >
> > > @@ -79,6 +77,8 @@ struct realtek_priv {
> > >       int                     vlan_enabled;
> > >       int                     vlan4k_enabled;
> > >
> > > +     const struct realtek_variant *variant;
> > > +
> > >       char                    buf[4096];
> > >       void                    *chip_data; /* Per-chip extra variant data */
> > >  };
> >
> > Can the changes to struct realtek_priv be a separate patch, to
> > clarify what is being changed, and to leave the noisy code movement
> > more isolated?
> 
> Sure, although it will not be a patch that makes sense by itself. If
> it helps with the review, I'll split it. We can fold it back if
> needed.

Well, I don't mean only the changes to the private structure, but also
the code changes that accompany them.

As Andrew usually says, what you want is lots of small patches that are
each obviously correct, where there is only one thing being changed.
Code movement with small renames is trivial to review. Consolidation of
two identical code paths in a single function is also possible to follow.
The insertion of a new variable and its usage is also easy to review.
The removal of a variable, the same. But superimposing them all into a
single patch makes everything much more difficult to follow.

Re: [PATCH net-next v3 4/8] net: dsa: realtek: merge common and interface modules into realtek-dsa

[Linus Walleij]

On Mon, Jan 8, 2024 at 3:11 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Fri, Dec 22, 2023 at 09:46:32PM -0300, Luiz Angelo Daros de Luca wrote:

> > +realtek-dsa-objs-y                   := realtek-common.o
> > +realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
> > +realtek-dsa-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
> > +realtek-dsa-objs                     := $(realtek-dsa-objs-y)
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
> >  rtl8366-objs                                 := rtl8366-core.o rtl8366rb.o
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
>
> Does "realtek-dsa-objs-y" have any particular meaning in the Kbuild
> system, or is it just a random variable name?

It's a Makefile naming convention, meaning this is always enabled.

linux$ git grep 'objs\-y'  | wc -l
52

Yours,
Linus Walleij

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Luiz Angelo Daros de Luca]

> On Tue, Jan 09, 2024 at 02:05:29AM -0300, Luiz Angelo Daros de Luca wrote:
> > > > +struct realtek_priv *
> > > > +realtek_common_probe(struct device *dev, struct regmap_config rc,
> > > > +                  struct regmap_config rc_nolock)
> > >
> > > Could you use "const struct regmap_config *" as the data types here, to
> > > avoid two on-stack variable copies? Regmap will copy the config structures
> > > anyway.
> >
> > I could do that for rc_nolock but not for rc as we need to modify it
> > before passing to regmap. I would still need to duplicate rc, either
> > using the stack or heap. What would be the best option?
> >
> > 1) pass two pointers and copy one to stack
> > 2) pass two pointers and copy one to heap
> > 3) pass two structs (as it is today)
> > 4) pass one pointer and one struct
> >
> > The old code was using 1) and I'm inclined to adopt it and save a
> > hundred and so bytes from the stack, although 2) would save even more.
>
> I didn't notice the "rc.lock_arg = priv" assignment...
>
> I'm not sure what you mean by "copy to heap". Perform a dynamic memory
> allocation?

Yes. However, I guess the stack can handle that structure in this context.

> Also, the old code was not using exactly 1). It copied both the normal
> and the nolock regmap config to an on-stack local variable, even though
> only the normal regmap config had to be copied (to be fixed up).
>
> I went back to study the 4 regmap configs, and only the reg_read() and
> reg_write() methods differ between SMI and MDIO. The rest seems boilerplate
> that can be dynamically constructed by realtek_common_probe(). Sure,
> spelling out 4 regmap_config structures is more flexible, but do we need
> that flexibility? What if realtek_common_probe() takes just the
> reg_read() and reg_write() function prototypes as arguments, rather than
> pointers to regmap_config structures it then has to fix up?

IMHO, the constant regmap_config looks cleaner than a sequence of
assignments. However, we don't actually need 4 of them.
As we already have a writable regmap_config in stack (to assign
lock_arg), we can reuse the same struct and simply set
disable_locking.
It makes the regmap ignore all locking fields and we don't even need
to unset them for map_nolock. Something like this:

realtek_common_probe(struct device *dev, const struct regmap_config *rc_base)
{

       (...)

       rc = *rc_base;
       rc.lock_arg = priv;
       priv->map = devm_regmap_init(dev, NULL, priv, &rc);
       if (IS_ERR(priv->map)) {
               ret = PTR_ERR(priv->map);
               dev_err(dev, "regmap init failed: %d\n", ret);
               return ERR_PTR(ret);
       }

       rc.disable_locking = true;
       priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
       if (IS_ERR(priv->map_nolock)) {
               ret = PTR_ERR(priv->map_nolock);
               dev_err(dev, "regmap init failed: %d\n", ret);
               return ERR_PTR(ret);
       }

It has a cleaner function signature and we can remove the _nolock
constants as well.

The regmap configs still have some room for improvement, like moving
from interfaces to variants, but this series is already too big. We
can leave that as it is.

> > > > +EXPORT_SYMBOL(realtek_common_probe);
> > > > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> > > > index e9ee778665b2..fbd0616c1df3 100644
> > > > --- a/drivers/net/dsa/realtek/realtek.h
> > > > +++ b/drivers/net/dsa/realtek/realtek.h
> > > > @@ -58,11 +58,9 @@ struct realtek_priv {
> > > >       struct mii_bus          *bus;
> > > >       int                     mdio_addr;
> > > >
> > > > -     unsigned int            clk_delay;
> > > > -     u8                      cmd_read;
> > > > -     u8                      cmd_write;
> > > >       spinlock_t              lock; /* Locks around command writes */
> > > >       struct dsa_switch       *ds;
> > > > +     const struct dsa_switch_ops *ds_ops;
> > > >       struct irq_domain       *irqdomain;
> > > >       bool                    leds_disabled;
> > > >
> > > > @@ -79,6 +77,8 @@ struct realtek_priv {
> > > >       int                     vlan_enabled;
> > > >       int                     vlan4k_enabled;
> > > >
> > > > +     const struct realtek_variant *variant;
> > > > +
> > > >       char                    buf[4096];
> > > >       void                    *chip_data; /* Per-chip extra variant data */
> > > >  };
> > >
> > > Can the changes to struct realtek_priv be a separate patch, to
> > > clarify what is being changed, and to leave the noisy code movement
> > > more isolated?
> >
> > Sure, although it will not be a patch that makes sense by itself. If
> > it helps with the review, I'll split it. We can fold it back if
> > needed.
>
> Well, I don't mean only the changes to the private structure, but also
> the code changes that accompany them.
>
> As Andrew usually says, what you want is lots of small patches that are
> each obviously correct, where there is only one thing being changed.
> Code movement with small renames is trivial to review. Consolidation of
> two identical code paths in a single function is also possible to follow.
> The insertion of a new variable and its usage is also easy to review.
> The removal of a variable, the same. But superimposing them all into a
> single patch makes everything much more difficult to follow.

This case in particular might be hard to justify in the commit message
other than "preliminary work". I'll split it as it makes review much
easier. this series will grow from 7 to 10 patches, even after
dropping the revert patch.

Regards,

Luiz

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Vladimir Oltean]

On Thu, Jan 11, 2024 at 03:20:10AM -0300, Luiz Angelo Daros de Luca wrote:
> IMHO, the constant regmap_config looks cleaner than a sequence of
> assignments. However, we don't actually need 4 of them.
> As we already have a writable regmap_config in stack (to assign
> lock_arg), we can reuse the same struct and simply set
> disable_locking.
> It makes the regmap ignore all locking fields and we don't even need
> to unset them for map_nolock. Something like this:
> 
> realtek_common_probe(struct device *dev, const struct regmap_config *rc_base)
> {
> 
>        (...)
> 
>        rc = *rc_base;
>        rc.lock_arg = priv;
>        priv->map = devm_regmap_init(dev, NULL, priv, &rc);
>        if (IS_ERR(priv->map)) {
>                ret = PTR_ERR(priv->map);
>                dev_err(dev, "regmap init failed: %d\n", ret);
>                return ERR_PTR(ret);
>        }
> 
>        rc.disable_locking = true;
>        priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
>        if (IS_ERR(priv->map_nolock)) {
>                ret = PTR_ERR(priv->map_nolock);
>                dev_err(dev, "regmap init failed: %d\n", ret);
>                return ERR_PTR(ret);
>        }
> 
> It has a cleaner function signature and we can remove the _nolock
> constants as well.
> 
> The regmap configs still have some room for improvement, like moving
> from interfaces to variants, but this series is already too big. We
> can leave that as it is.

I was thinking something like this, does it look bad?

From 2e462507171ed0fd8393598842dc0f7e6c50d499 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Thu, 11 Jan 2024 11:38:17 +0200
Subject: [PATCH] realtek_common_info

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/realtek/realtek-common.c | 35 ++++++++++++++++++------
 drivers/net/dsa/realtek/realtek-common.h |  9 ++++--
 drivers/net/dsa/realtek/realtek-mdio.c   | 27 ++----------------
 drivers/net/dsa/realtek/realtek-smi.c    | 35 ++++--------------------
 4 files changed, 41 insertions(+), 65 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
index 80b37e5fe780..bd6b04922b6d 100644
--- a/drivers/net/dsa/realtek/realtek-common.c
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -22,10 +22,21 @@ void realtek_common_unlock(void *ctx)
 EXPORT_SYMBOL_GPL(realtek_common_unlock);
 
 struct realtek_priv *
-realtek_common_probe(struct device *dev, struct regmap_config rc,
-		     struct regmap_config rc_nolock)
+realtek_common_probe(struct device *dev,
+		     const struct realtek_common_info *info)
 {
 	const struct realtek_variant *var;
+	struct regmap_config rc = {
+		.reg_bits = 10, /* A4..A0 R4..R0 */
+		.val_bits = 16,
+		.reg_stride = 1,
+		/* PHY regs are at 0x8000 */
+		.max_register = 0xffff,
+		.reg_format_endian = REGMAP_ENDIAN_BIG,
+		.reg_read = info->reg_read,
+		.reg_write = info->reg_write,
+		.cache_type = REGCACHE_NONE,
+	};
 	struct realtek_priv *priv;
 	int ret;
 
@@ -40,17 +51,23 @@ realtek_common_probe(struct device *dev, struct regmap_config rc,
 
 	mutex_init(&priv->map_lock);
 
-	rc.lock_arg = priv;
-	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
-	if (IS_ERR(priv->map)) {
-		ret = PTR_ERR(priv->map);
+	/* Initialize the non-locking regmap first */
+	rc.disable_locking = true;
+	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
+	if (IS_ERR(priv->map_nolock)) {
+		ret = PTR_ERR(priv->map_nolock);
 		dev_err(dev, "regmap init failed: %d\n", ret);
 		return ERR_PTR(ret);
 	}
 
-	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock);
-	if (IS_ERR(priv->map_nolock)) {
-		ret = PTR_ERR(priv->map_nolock);
+	/* Then the locking regmap */
+	rc.disable_locking = false;
+	rc.lock = realtek_common_lock;
+	rc.unlock = realtek_common_unlock;
+	rc.lock_arg = priv;
+	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
+	if (IS_ERR(priv->map)) {
+		ret = PTR_ERR(priv->map);
 		dev_err(dev, "regmap init failed: %d\n", ret);
 		return ERR_PTR(ret);
 	}
diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
index 518d091ff496..71fc43d8d90a 100644
--- a/drivers/net/dsa/realtek/realtek-common.h
+++ b/drivers/net/dsa/realtek/realtek-common.h
@@ -5,11 +5,16 @@
 
 #include <linux/regmap.h>
 
+struct realtek_common_info {
+	int (*reg_read)(void *ctx, u32 reg, u32 *val);
+	int (*reg_write)(void *ctx, u32 reg, u32 val);
+};
+
 void realtek_common_lock(void *ctx);
 void realtek_common_unlock(void *ctx);
 struct realtek_priv *
-realtek_common_probe(struct device *dev, struct regmap_config rc,
-		     struct regmap_config rc_nolock);
+realtek_common_probe(struct device *dev,
+		     const struct realtek_common_info *info);
 int realtek_common_register_switch(struct realtek_priv *priv);
 void realtek_common_remove(struct realtek_priv *priv);
 
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 1eed09ab3aa1..8725cd1b027b 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -101,31 +101,9 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
 	return ret;
 }
 
-static const struct regmap_config realtek_mdio_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
+static const struct realtek_common_info realtek_mdio_info = {
 	.reg_read = realtek_mdio_read,
 	.reg_write = realtek_mdio_write,
-	.cache_type = REGCACHE_NONE,
-	.lock = realtek_common_lock,
-	.unlock = realtek_common_unlock,
-};
-
-static const struct regmap_config realtek_mdio_nolock_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
-	.reg_read = realtek_mdio_read,
-	.reg_write = realtek_mdio_write,
-	.cache_type = REGCACHE_NONE,
-	.disable_locking = true,
 };
 
 int realtek_mdio_probe(struct mdio_device *mdiodev)
@@ -134,8 +112,7 @@ int realtek_mdio_probe(struct mdio_device *mdiodev)
 	struct realtek_priv *priv;
 	int ret;
 
-	priv = realtek_common_probe(dev, realtek_mdio_regmap_config,
-				    realtek_mdio_nolock_regmap_config);
+	priv = realtek_common_probe(dev, &realtek_mdio_info);
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index fc54190839cf..7697dc66e5e8 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -312,33 +312,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val)
 	return realtek_smi_read_reg(priv, reg, val);
 }
 
-static const struct regmap_config realtek_smi_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
-	.reg_read = realtek_smi_read,
-	.reg_write = realtek_smi_write,
-	.cache_type = REGCACHE_NONE,
-	.lock = realtek_common_lock,
-	.unlock = realtek_common_unlock,
-};
-
-static const struct regmap_config realtek_smi_nolock_regmap_config = {
-	.reg_bits = 10, /* A4..A0 R4..R0 */
-	.val_bits = 16,
-	.reg_stride = 1,
-	/* PHY regs are at 0x8000 */
-	.max_register = 0xffff,
-	.reg_format_endian = REGMAP_ENDIAN_BIG,
-	.reg_read = realtek_smi_read,
-	.reg_write = realtek_smi_write,
-	.cache_type = REGCACHE_NONE,
-	.disable_locking = true,
-};
-
 static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum)
 {
 	struct realtek_priv *priv = bus->priv;
@@ -396,14 +369,18 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	return ret;
 }
 
+static const struct realtek_common_info realtek_smi_info = {
+	.reg_read = realtek_smi_read,
+	.reg_write = realtek_smi_write,
+};
+
 int realtek_smi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct realtek_priv *priv;
 	int ret;
 
-	priv = realtek_common_probe(dev, realtek_smi_regmap_config,
-				    realtek_smi_nolock_regmap_config);
+	priv = realtek_common_probe(dev, &realtek_smi_info);
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
-- 
2.34.1

> This case in particular might be hard to justify in the commit message
> other than "preliminary work". I'll split it as it makes review much
> easier. this series will grow from 7 to 10 patches, even after
> dropping the revert patch.

Preliminary work is fine if you explain a bit in advance why it will be
needed.

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Vladimir Oltean]

On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote:
> +EXPORT_SYMBOL_GPL(realtek_common_lock);
> +EXPORT_SYMBOL_GPL(realtek_common_unlock);
> +EXPORT_SYMBOL(realtek_common_probe);
> +EXPORT_SYMBOL(realtek_common_register_switch);
> +EXPORT_SYMBOL(realtek_common_remove);

Is there any reason for the lack of consistency between GPL and non-GPL
symbols?

Also, I don't like too much the naming of symbols like "realtek_common_probe",
exported to the entire kernel. I wonder if it would be better to drop
the word "common" altogether, and use EXPORT_SYMBOL_NS_GPL(*, REALTEK_DSA) +
MODULE_IMPORT_NS(REALTEK_DSA) instead of plain EXPORT_SYMBOL_GPL()?

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Alvin Šipraga]

On Thu, Jan 11, 2024 at 11:41:48AM +0200, Vladimir Oltean wrote:
> diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> index 518d091ff496..71fc43d8d90a 100644
> --- a/drivers/net/dsa/realtek/realtek-common.h
> +++ b/drivers/net/dsa/realtek/realtek-common.h
> @@ -5,11 +5,16 @@
>  
>  #include <linux/regmap.h>
>  
> +struct realtek_common_info {
> +	int (*reg_read)(void *ctx, u32 reg, u32 *val);
> +	int (*reg_write)(void *ctx, u32 reg, u32 val);
> +};
> +

Yes, this is good. Makes it easier to expand later if necessary, too.

>  void realtek_common_lock(void *ctx);
>  void realtek_common_unlock(void *ctx);
>  struct realtek_priv *
> -realtek_common_probe(struct device *dev, struct regmap_config rc,
> -		     struct regmap_config rc_nolock);
> +realtek_common_probe(struct device *dev,
> +		     const struct realtek_common_info *info);
>  int realtek_common_register_switch(struct realtek_priv *priv);
>  void realtek_common_remove(struct realtek_priv *priv);

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Luiz Angelo Daros de Luca]

> On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote:
> > +EXPORT_SYMBOL_GPL(realtek_common_lock);
> > +EXPORT_SYMBOL_GPL(realtek_common_unlock);
> > +EXPORT_SYMBOL(realtek_common_probe);
> > +EXPORT_SYMBOL(realtek_common_register_switch);
> > +EXPORT_SYMBOL(realtek_common_remove);
>
> Is there any reason for the lack of consistency between GPL and non-GPL
> symbols?

No. I might have just copied the string from the wrong example.

> Also, I don't like too much the naming of symbols like "realtek_common_probe",
> exported to the entire kernel. I wonder if it would be better to drop
> the word "common" altogether, and use EXPORT_SYMBOL_NS_GPL(*, REALTEK_DSA) +
> MODULE_IMPORT_NS(REALTEK_DSA) instead of plain EXPORT_SYMBOL_GPL()?

Introducing a namespace seems to be a nice ideia. Let the series grow :-)

What do you mean by dropping the "common"? Use "realtek_probe" or
"realtek_dsa_probe"?

Regards,

Luiz

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Vladimir Oltean]

On Thu, Jan 11, 2024 at 04:53:01PM -0300, Luiz Angelo Daros de Luca wrote:
> What do you mean by dropping the "common"? Use "realtek_probe" or
> "realtek_dsa_probe"?

Yes, I meant "realtek_probe()" etc. It's just that the word "common" has
no added value in function names, and I can't come up with something
good to replace it with.

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Luiz Angelo Daros de Luca]

Em qui., 11 de jan. de 2024 às 06:41, Vladimir Oltean
<olteanv@gmail.com> escreveu:
>
> On Thu, Jan 11, 2024 at 03:20:10AM -0300, Luiz Angelo Daros de Luca wrote:
> > IMHO, the constant regmap_config looks cleaner than a sequence of
> > assignments. However, we don't actually need 4 of them.
> > As we already have a writable regmap_config in stack (to assign
> > lock_arg), we can reuse the same struct and simply set
> > disable_locking.
> > It makes the regmap ignore all locking fields and we don't even need
> > to unset them for map_nolock. Something like this:
> >
> > realtek_common_probe(struct device *dev, const struct regmap_config *rc_base)
> > {
> >
> >        (...)
> >
> >        rc = *rc_base;
> >        rc.lock_arg = priv;
> >        priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> >        if (IS_ERR(priv->map)) {
> >                ret = PTR_ERR(priv->map);
> >                dev_err(dev, "regmap init failed: %d\n", ret);
> >                return ERR_PTR(ret);
> >        }
> >
> >        rc.disable_locking = true;
> >        priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
> >        if (IS_ERR(priv->map_nolock)) {
> >                ret = PTR_ERR(priv->map_nolock);
> >                dev_err(dev, "regmap init failed: %d\n", ret);
> >                return ERR_PTR(ret);
> >        }
> >
> > It has a cleaner function signature and we can remove the _nolock
> > constants as well.
> >
> > The regmap configs still have some room for improvement, like moving
> > from interfaces to variants, but this series is already too big. We
> > can leave that as it is.
>
> I was thinking something like this, does it look bad?
>
> From 2e462507171ed0fd8393598842dc0f7e6c50d499 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Thu, 11 Jan 2024 11:38:17 +0200
> Subject: [PATCH] realtek_common_info
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/realtek/realtek-common.c | 35 ++++++++++++++++++------
>  drivers/net/dsa/realtek/realtek-common.h |  9 ++++--
>  drivers/net/dsa/realtek/realtek-mdio.c   | 27 ++----------------
>  drivers/net/dsa/realtek/realtek-smi.c    | 35 ++++--------------------
>  4 files changed, 41 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> index 80b37e5fe780..bd6b04922b6d 100644
> --- a/drivers/net/dsa/realtek/realtek-common.c
> +++ b/drivers/net/dsa/realtek/realtek-common.c
> @@ -22,10 +22,21 @@ void realtek_common_unlock(void *ctx)
>  EXPORT_SYMBOL_GPL(realtek_common_unlock);
>
>  struct realtek_priv *
> -realtek_common_probe(struct device *dev, struct regmap_config rc,
> -                    struct regmap_config rc_nolock)
> +realtek_common_probe(struct device *dev,
> +                    const struct realtek_common_info *info)
>  {
>         const struct realtek_variant *var;
> +       struct regmap_config rc = {
> +               .reg_bits = 10, /* A4..A0 R4..R0 */
> +               .val_bits = 16,
> +               .reg_stride = 1,
> +               /* PHY regs are at 0x8000 */
> +               .max_register = 0xffff,
> +               .reg_format_endian = REGMAP_ENDIAN_BIG,
> +               .reg_read = info->reg_read,
> +               .reg_write = info->reg_write,
> +               .cache_type = REGCACHE_NONE,
> +       };
>         struct realtek_priv *priv;
>         int ret;
>
> @@ -40,17 +51,23 @@ realtek_common_probe(struct device *dev, struct regmap_config rc,
>
>         mutex_init(&priv->map_lock);
>
> -       rc.lock_arg = priv;
> -       priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> -       if (IS_ERR(priv->map)) {
> -               ret = PTR_ERR(priv->map);
> +       /* Initialize the non-locking regmap first */
> +       rc.disable_locking = true;
> +       priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
> +       if (IS_ERR(priv->map_nolock)) {
> +               ret = PTR_ERR(priv->map_nolock);
>                 dev_err(dev, "regmap init failed: %d\n", ret);
>                 return ERR_PTR(ret);
>         }
>
> -       priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock);
> -       if (IS_ERR(priv->map_nolock)) {
> -               ret = PTR_ERR(priv->map_nolock);
> +       /* Then the locking regmap */
> +       rc.disable_locking = false;
> +       rc.lock = realtek_common_lock;
> +       rc.unlock = realtek_common_unlock;
> +       rc.lock_arg = priv;
> +       priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> +       if (IS_ERR(priv->map)) {
> +               ret = PTR_ERR(priv->map);
>                 dev_err(dev, "regmap init failed: %d\n", ret);
>                 return ERR_PTR(ret);
>         }
> diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> index 518d091ff496..71fc43d8d90a 100644
> --- a/drivers/net/dsa/realtek/realtek-common.h
> +++ b/drivers/net/dsa/realtek/realtek-common.h
> @@ -5,11 +5,16 @@
>
>  #include <linux/regmap.h>
>
> +struct realtek_common_info {
> +       int (*reg_read)(void *ctx, u32 reg, u32 *val);
> +       int (*reg_write)(void *ctx, u32 reg, u32 val);
> +};
> +
>  void realtek_common_lock(void *ctx);
>  void realtek_common_unlock(void *ctx);
>  struct realtek_priv *
> -realtek_common_probe(struct device *dev, struct regmap_config rc,
> -                    struct regmap_config rc_nolock);
> +realtek_common_probe(struct device *dev,
> +                    const struct realtek_common_info *info);
>  int realtek_common_register_switch(struct realtek_priv *priv);
>  void realtek_common_remove(struct realtek_priv *priv);
>
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 1eed09ab3aa1..8725cd1b027b 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -101,31 +101,9 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
>         return ret;
>  }
>
> -static const struct regmap_config realtek_mdio_regmap_config = {
> -       .reg_bits = 10, /* A4..A0 R4..R0 */
> -       .val_bits = 16,
> -       .reg_stride = 1,
> -       /* PHY regs are at 0x8000 */
> -       .max_register = 0xffff,
> -       .reg_format_endian = REGMAP_ENDIAN_BIG,
> +static const struct realtek_common_info realtek_mdio_info = {
>         .reg_read = realtek_mdio_read,
>         .reg_write = realtek_mdio_write,
> -       .cache_type = REGCACHE_NONE,
> -       .lock = realtek_common_lock,
> -       .unlock = realtek_common_unlock,
> -};
> -
> -static const struct regmap_config realtek_mdio_nolock_regmap_config = {
> -       .reg_bits = 10, /* A4..A0 R4..R0 */
> -       .val_bits = 16,
> -       .reg_stride = 1,
> -       /* PHY regs are at 0x8000 */
> -       .max_register = 0xffff,
> -       .reg_format_endian = REGMAP_ENDIAN_BIG,
> -       .reg_read = realtek_mdio_read,
> -       .reg_write = realtek_mdio_write,
> -       .cache_type = REGCACHE_NONE,
> -       .disable_locking = true,
>  };
>
>  int realtek_mdio_probe(struct mdio_device *mdiodev)
> @@ -134,8 +112,7 @@ int realtek_mdio_probe(struct mdio_device *mdiodev)
>         struct realtek_priv *priv;
>         int ret;
>
> -       priv = realtek_common_probe(dev, realtek_mdio_regmap_config,
> -                                   realtek_mdio_nolock_regmap_config);
> +       priv = realtek_common_probe(dev, &realtek_mdio_info);
>         if (IS_ERR(priv))
>                 return PTR_ERR(priv);
>
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index fc54190839cf..7697dc66e5e8 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -312,33 +312,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val)
>         return realtek_smi_read_reg(priv, reg, val);
>  }
>
> -static const struct regmap_config realtek_smi_regmap_config = {
> -       .reg_bits = 10, /* A4..A0 R4..R0 */
> -       .val_bits = 16,
> -       .reg_stride = 1,
> -       /* PHY regs are at 0x8000 */
> -       .max_register = 0xffff,
> -       .reg_format_endian = REGMAP_ENDIAN_BIG,
> -       .reg_read = realtek_smi_read,
> -       .reg_write = realtek_smi_write,
> -       .cache_type = REGCACHE_NONE,
> -       .lock = realtek_common_lock,
> -       .unlock = realtek_common_unlock,
> -};
> -
> -static const struct regmap_config realtek_smi_nolock_regmap_config = {
> -       .reg_bits = 10, /* A4..A0 R4..R0 */
> -       .val_bits = 16,
> -       .reg_stride = 1,
> -       /* PHY regs are at 0x8000 */
> -       .max_register = 0xffff,
> -       .reg_format_endian = REGMAP_ENDIAN_BIG,
> -       .reg_read = realtek_smi_read,
> -       .reg_write = realtek_smi_write,
> -       .cache_type = REGCACHE_NONE,
> -       .disable_locking = true,
> -};
> -
>  static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum)
>  {
>         struct realtek_priv *priv = bus->priv;
> @@ -396,14 +369,18 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>         return ret;
>  }
>
> +static const struct realtek_common_info realtek_smi_info = {
> +       .reg_read = realtek_smi_read,
> +       .reg_write = realtek_smi_write,
> +};
> +
>  int realtek_smi_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct realtek_priv *priv;
>         int ret;
>
> -       priv = realtek_common_probe(dev, realtek_smi_regmap_config,
> -                                   realtek_smi_nolock_regmap_config);
> +       priv = realtek_common_probe(dev, &realtek_smi_info);
>         if (IS_ERR(priv))
>                 return PTR_ERR(priv);
>
> --
> 2.34.1
>

I don't have a strong opinion on how the regmap config is prepared.
I'll adopt your suggestion, with some minor changes.

Regmap config for realtek dsa driver should be composed of values that
(should) depend on the variant, the management interface (read/write)
and some common values. Today it is only dependent on the interface
and common values (with register range being a forced coincidence).
However, that is a topic for another series. We do need to stop this
series at some point. It already has 11 patches (and not counting the
2 bindings + 1 code that are actually my target). We already have some
nice features for delivery.

Regards,

Luiz

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Luiz Angelo Daros de Luca]

> On Thu, Jan 11, 2024 at 04:53:01PM -0300, Luiz Angelo Daros de Luca wrote:
> > What do you mean by dropping the "common"? Use "realtek_probe" or
> > "realtek_dsa_probe"?
>
> Yes, I meant "realtek_probe()" etc. It's just that the word "common" has
> no added value in function names, and I can't come up with something
> good to replace it with.

I didn't like realtek_probe() either. It is too generic, although now
in a specific name space.
How about replacing all realtek_common with rtl83xx? I guess all
rtl83xx chips are switch controllers, even though some of them have an
embedded mips core.
Or we could include a prefix/suffix as well like rtl83xx_dsa or realtek_dsa..

Regards,

Luiz

Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module

[Vladimir Oltean]

On Sat, Jan 13, 2024 at 06:38:39PM -0300, Luiz Angelo Daros de Luca wrote:
> I didn't like realtek_probe() either. It is too generic, although now
> in a specific name space.
> How about replacing all realtek_common with rtl83xx? I guess all
> rtl83xx chips are switch controllers, even though some of them have an
> embedded mips core.
> Or we could include a prefix/suffix as well like rtl83xx_dsa or realtek_dsa..

I don't have a problem with just rtl83xx.

Re: [PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[Vladimir Oltean]

On Fri, Dec 22, 2023 at 09:46:28PM -0300, Luiz Angelo Daros de Luca wrote:
> The series begins by removing an unused function pointer at
> realtek_ops->cleanup.
> 
> Each variant module was converted into real drivers, serving as both a
> platform driver (for switches connected using the SMI interface) and an
> MDIO driver (for MDIO-connected switches). The relationship between the
> variant and interface modules is reversed, with the variant module now
> calling both interface functions (if not disabled at build time). While
> in most devices only one interface is likely used, the interface code is
> significantly smaller than a variant module, consuming fewer resources
> than the previous code. With variant modules now functioning as real
> drivers, compatible strings are published only in a single variant
> module, preventing conflicts.
> 
> The patch series introduces a new common module for functions shared by
> both variants. This module also absorbs the two previous interface
> modules, as they would always be loaded anyway.
> 
> The series relocates the user MII driver from realtek-smi to common. It
> is now used by MDIO-connected switches instead of the generic DSA
> driver. There's a change in how this driver locates the MDIO node. It
> now only searches for a child node named "mdio", which is required by
> both interfaces in binding docs.
> 
> The dsa_switch in realtek_priv->ds is now embedded in the struct. It is
> always in use and avoids dynamic memory allocation.

git format-patch --cover-letter generates a nice patch series overview
with a diffstat and the commit titles, you should include it next time.

Re: [PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[Arınç ÜNAL]

On 16.01.2024 00:54, Vladimir Oltean wrote:
> git format-patch --cover-letter generates a nice patch series overview
> with a diffstat and the commit titles, you should include it next time.

Thanks a lot for mentioning this. I didn't know this and now that I use it,
it helps a lot.

Arınç

Re: [PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[Linus Walleij]

On Wed, Jan 17, 2024 at 11:26 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> On 16.01.2024 00:54, Vladimir Oltean wrote:

> > git format-patch --cover-letter generates a nice patch series overview
> > with a diffstat and the commit titles, you should include it next time.
>
> Thanks a lot for mentioning this. I didn't know this and now that I use it,
> it helps a lot.

There are some even nicer tools you can use on top, i.e. "b4":
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1

This blog post doesn't mention the magic trick:
b4 prep --set-prefixes net-next

And
b4 trailers -u

Which is what you need to make it an awesome net contribution
patch series tool.

Yours,
Linus Walleij

Re: [PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[Arınç ÜNAL]

On 17.01.2024 15:48, Linus Walleij wrote:
> On Wed, Jan 17, 2024 at 11:26 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>> On 16.01.2024 00:54, Vladimir Oltean wrote:
> 
>>> git format-patch --cover-letter generates a nice patch series overview
>>> with a diffstat and the commit titles, you should include it next time.
>>
>> Thanks a lot for mentioning this. I didn't know this and now that I use it,
>> it helps a lot.
> 
> There are some even nicer tools you can use on top, i.e. "b4":
> https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1
> 
> This blog post doesn't mention the magic trick:
> b4 prep --set-prefixes net-next
> 
> And
> b4 trailers -u
> 
> Which is what you need to make it an awesome net contribution
> patch series tool.

I've spent a day thinking I probably don't need this. I've spent the next
day giving it a go. I need this. Diffstat, the auto formatting of new
version change log, and linking to the previous version on the cover
letter, patch version comparison, and auto adding trailers features make my
life so much easier.

I've had trouble with every mail provider's SMTP server that I've ever used
for submitting patches, so the web endpoint is a godsend. It would've been
great if b4 supported openssh keys to submit patches via the web endpoint.
Patatt at least supports it to sign patches. I've got a single ed25519
openssh keypair I use across all my devices, now I'll have to backup
another key pair. Or create a new key and authenticate with the web
endpoint on each device.

Safe to say, I will submit my next patch series using b4. Thanks for
telling me about this tool Linus!

Arınç

Re: [PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[Konstantin Ryabitsev]

On Sun, Jan 21, 2024 at 01:13:36AM +0300, Arınç ÜNAL wrote:
> I've had trouble with every mail provider's SMTP server that I've ever used
> for submitting patches, so the web endpoint is a godsend. It would've been
> great if b4 supported openssh keys to submit patches via the web endpoint.

The only reason it's not currently supported is because we don't have a recent
enough version of openssh on the system where the endpoint is listening. This
will change in the near future, at which point using ssh keys will be
possible.

> Patatt at least supports it to sign patches. I've got a single ed25519
> openssh keypair I use across all my devices, now I'll have to backup
> another key pair. Or create a new key and authenticate with the web
> endpoint on each device.
> 
> Safe to say, I will submit my next patch series using b4. Thanks for
> telling me about this tool Linus!

\o/

Please feel free to provide any feedback you have to the tools@kernel.org
list.

-K

Re: [PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module

[Luiz Angelo Daros de Luca]

> > I've had trouble with every mail provider's SMTP server that I've ever used
> > for submitting patches, so the web endpoint is a godsend. It would've been
> > great if b4 supported openssh keys to submit patches via the web endpoint.
>
> The only reason it's not currently supported is because we don't have a recent
> enough version of openssh on the system where the endpoint is listening. This
> will change in the near future, at which point using ssh keys will be
> possible.
>
> > Patatt at least supports it to sign patches. I've got a single ed25519
> > openssh keypair I use across all my devices, now I'll have to backup
> > another key pair. Or create a new key and authenticate with the web
> > endpoint on each device.
> >
> > Safe to say, I will submit my next patch series using b4. Thanks for
> > telling me about this tool Linus!
>
> \o/
>
> Please feel free to provide any feedback you have to the tools@kernel.org
> list.
>
> -K

+1 to b4 users: v5 just sent using b4. Great tool.

Regards,

Luiz