[RFC net-next 0/5] refactor realtek switches and add reset controller

[RFC net-next 0/5] refactor realtek switches and add reset controller

[Luiz Angelo Daros de Luca]

This patch series might be a bit too hefty, and I'm thinking of splitting it into two series. It all began as a patch to add reset-controller as a way to reset the switch, but the duplicated code for both sparked a discussion about a shared code base, which led to what you get here. This driver will be used in some devices with tight resources, both in storage and RAM.

The current driver has two interface modules (SMI and MDIO) and two family/variant modules (RTL8365MB and RTL8366RB). The interface modules are independent and can be loaded only when necessary. But, they refer to symbols from both variant modules, which means they have to be loaded together or disabled at build time. It's a simple approach but doesn't scale well over time, especially if you add more switch variants (like RTL8366B). Also, it's unlikely there'll ever be a device using switches from different families simultaneously. The variant modules are much larger than the interface modules, so it makes sense to load only the needed code.

The first part involves reworking the Realtek DSA switch code. It introduces a common module shared by all existing interfaces (SMI or MDIO) and switch family modules (RTL8365MB and RTL8366RB). This module will mainly have parts of the probe code common to both interfaces, but more bits from variants might move over in the future. This common module is also a way for variant modules to register themselves for interface modules. The idea came from how DSA requests tag modules at runtime when necessary.

The second part is about the original reset controller code, along with the first two binding patches. It'll only affect the new common module.

This series was tested with RTL8366RB using SMI and RTL8367S (rtl8365mb) with MDIO. Both seemed to work as expected.

Checkpatch is flagging some long function declarations (realtek_common_probe and realtek_variant_get). I checked existing kernel code and noticed differe

Regards,

Luiz

[RFC net-next 1/5] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Luiz Angelo Daros de Luca]

The 'reset-gpios' should not be mandatory. although they might be
required for some devices if the switch reset was left asserted by a
previous driver, such as the bootloader.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: devicetree@vger.kernel.org
Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/net/dsa/realtek.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cce692f57b08..46e113df77c8 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -127,7 +127,6 @@ else:
     - mdc-gpios
     - mdio-gpios
     - mdio
-    - reset-gpios
 
 required:
   - compatible
-- 
2.42.1

[RFC net-next 2/5] dt-bindings: net: dsa: realtek: add reset controller

[Luiz Angelo Daros de Luca]

Realtek switches can use a reset controller instead of reset-gpios.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: devicetree@vger.kernel.org
Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 Documentation/devicetree/bindings/net/dsa/realtek.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index 46e113df77c8..70b6bda3cf98 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -59,6 +59,9 @@ properties:
     description: GPIO to be used to reset the whole device
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
   realtek,disable-leds:
     type: boolean
     description: |
-- 
2.42.1

[RFC net-next 3/5] net: dsa: realtek: create realtek-common

[Luiz Angelo Daros de Luca]

Some code can be shared between both interface modules (MDIO and SMI)
and amongst variants. For now, these interface functions are shared:

- realtek_common_lock
- realtek_common_unlock
- realtek_common_probe
- realtek_common_remove

The reset during probe was moved to the last moment before a variant
detects the switch. This way, we avoid a reset if anything else fails.

The symbols from variants used in of_match_table are now in a single
match table in realtek-common, used by both interfaces.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Makefile         |   1 +
 drivers/net/dsa/realtek/realtek-common.c | 139 +++++++++++++++++++++++
 drivers/net/dsa/realtek/realtek-common.h |  16 +++
 drivers/net/dsa/realtek/realtek-mdio.c   | 116 +++----------------
 drivers/net/dsa/realtek/realtek-smi.c    | 127 +++------------------
 drivers/net/dsa/realtek/realtek.h        |   2 +
 6 files changed, 184 insertions(+), 217 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..5e0c1ef200a3 100644
--- a/drivers/net/dsa/realtek/Makefile
+++ b/drivers/net/dsa/realtek/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_NET_DSA_REALTEK)		+= 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..36f8b60771be
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -0,0 +1,139 @@
+// 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;
+	struct device_node *np;
+	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->clk_delay = var->clk_delay;
+	priv->cmd_read = var->cmd_read;
+	priv->cmd_write = var->cmd_write;
+	priv->variant = var;
+	priv->ops = var->ops;
+	priv->chip_data = (void *)priv + sizeof(*priv);
+
+	dev_set_drvdata(dev, priv);
+	spin_lock_init(&priv->lock);
+
+	/* Fetch MDIO pins */
+	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->mdc))
+		return ERR_CAST(priv->mdc);
+
+	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->mdio))
+		return ERR_CAST(priv->mdio);
+
+	np = dev->of_node;
+
+	priv->leds_disabled = of_property_read_bool(np, "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");
+	}
+
+	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
+	if (!priv->ds)
+		return ERR_PTR(-ENOMEM);
+
+	priv->ds->dev = dev;
+	priv->ds->priv = priv;
+
+	return priv;
+}
+EXPORT_SYMBOL(realtek_common_probe);
+
+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);
+}
+EXPORT_SYMBOL(realtek_common_remove);
+
+const struct of_device_id realtek_common_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 = &rtl8366rb_variant, },
+#endif
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, realtek_common_of_match);
+EXPORT_SYMBOL_GPL(realtek_common_of_match);
+
+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..90a949386518
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-common.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_INTERFACE_H
+#define _REALTEK_INTERFACE_H
+
+#include <linux/regmap.h>
+
+extern const struct of_device_id realtek_common_of_match[];
+
+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);
+void realtek_common_remove(struct realtek_priv *priv);
+
+#endif
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 292e6d087e8b..6f610386c977 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-common.h"
 
 /* Read/write via mdiobus */
 #define REALTEK_MDIO_CTRL0_REG		31
@@ -99,20 +100,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,
@@ -123,8 +110,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 = {
@@ -142,75 +129,19 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
 
 static 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);
-
-	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 = realtek_common_probe(dev, realtek_mdio_regmap_config,
+				    realtek_mdio_nolock_regmap_config);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
 
 	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->write_reg_noack = realtek_mdio_write;
-
-	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");
-	}
+	priv->ds->ops = priv->variant->ds_ops_mdio;
 
 	ret = priv->ops->detect(priv);
 	if (ret) {
@@ -218,18 +149,12 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 		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);
+		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
+			      ERR_PTR(ret));
 		return ret;
 	}
 
@@ -243,11 +168,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
-	dsa_unregister_switch(priv->ds);
-
-	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_common_remove(priv);
 }
 
 static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
@@ -262,21 +183,10 @@ 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,
+		.of_match_table = realtek_common_of_match,
 	},
 	.probe  = realtek_mdio_probe,
 	.remove = realtek_mdio_remove,
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 755546ed8db6..0cf89f9db99e 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-common.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
 
@@ -310,20 +311,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,
@@ -334,8 +321,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 = {
@@ -410,78 +397,18 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 
 static 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 = realtek_common_probe(dev, realtek_smi_regmap_config,
+				    realtek_smi_nolock_regmap_config);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
 
 	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");
-	}
-
-	/* 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->ds->ops = priv->variant->ds_ops_smi;
 
 	ret = priv->ops->detect(priv);
 	if (ret) {
@@ -489,20 +416,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
 		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");
+		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
+			      ERR_PTR(ret));
 		return ret;
 	}
+
 	return 0;
 }
 
@@ -513,13 +435,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
 	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);
+	realtek_common_remove(priv);
 }
 
 static void realtek_smi_shutdown(struct platform_device *pdev)
@@ -534,27 +450,10 @@ 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,
+		.of_match_table = realtek_common_of_match,
 	},
 	.probe  = realtek_smi_probe,
 	.remove_new = realtek_smi_remove,
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 790488e9c667..8d9d546bf5f5 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -79,6 +79,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 */
 };
-- 
2.42.1

[RFC net-next 4/5] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

realtek-common had a hard dependency on both switch variants. That way,
it was not possible to selectively load only one model at runtime. Now
variants are registered at the realtek-common module and interface
modules look for a variant using the compatible string.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-common.c | 125 ++++++++++++++++++++---
 drivers/net/dsa/realtek/realtek-common.h |   3 +
 drivers/net/dsa/realtek/realtek-mdio.c   |   9 +-
 drivers/net/dsa/realtek/realtek-smi.c    |   9 +-
 drivers/net/dsa/realtek/realtek.h        |  36 ++++++-
 drivers/net/dsa/realtek/rtl8365mb.c      |   4 +-
 drivers/net/dsa/realtek/rtl8366rb.c      |   4 +-
 7 files changed, 162 insertions(+), 28 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
index 36f8b60771be..e383db21c776 100644
--- a/drivers/net/dsa/realtek/realtek-common.c
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -1,10 +1,76 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 #include <linux/module.h>
+#include <linux/of_device.h>
 
 #include "realtek.h"
 #include "realtek-common.h"
 
+static LIST_HEAD(realtek_variants_list);
+static DEFINE_MUTEX(realtek_variants_lock);
+
+void realtek_variant_register(struct realtek_variant_entry *var_ent)
+{
+	mutex_lock(&realtek_variants_lock);
+	list_add_tail(&var_ent->list, &realtek_variants_list);
+	mutex_unlock(&realtek_variants_lock);
+}
+EXPORT_SYMBOL_GPL(realtek_variant_register);
+
+void realtek_variant_unregister(struct realtek_variant_entry *var_ent)
+{
+	mutex_lock(&realtek_variants_lock);
+	list_del(&var_ent->list);
+	mutex_unlock(&realtek_variants_lock);
+}
+EXPORT_SYMBOL_GPL(realtek_variant_unregister);
+
+const struct realtek_variant *realtek_variant_get(
+		const struct of_device_id *match)
+{
+	const struct realtek_variant *var = ERR_PTR(-ENOENT);
+	struct realtek_variant_entry *var_ent;
+	const char *modname = match->data;
+
+	request_module(modname);
+
+	mutex_lock(&realtek_variants_lock);
+	list_for_each_entry(var_ent, &realtek_variants_list, list) {
+		const struct realtek_variant *tmp = var_ent->variant;
+
+		if (strcmp(match->compatible, var_ent->compatible))
+			continue;
+
+		if (!try_module_get(var_ent->owner))
+			break;
+
+		var = tmp;
+		break;
+	}
+	mutex_unlock(&realtek_variants_lock);
+
+	return var;
+}
+EXPORT_SYMBOL_GPL(realtek_variant_get);
+
+void realtek_variant_put(const struct realtek_variant *var)
+{
+	struct realtek_variant_entry *var_ent;
+
+	mutex_lock(&realtek_variants_lock);
+	list_for_each_entry(var_ent, &realtek_variants_list, list) {
+		if (var_ent->variant != var)
+			continue;
+
+		if (var_ent->owner)
+			module_put(var_ent->owner);
+
+		break;
+	}
+	mutex_unlock(&realtek_variants_lock);
+}
+EXPORT_SYMBOL_GPL(realtek_variant_put);
+
 void realtek_common_lock(void *ctx)
 {
 	struct realtek_priv *priv = ctx;
@@ -25,18 +91,30 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
 		struct regmap_config rc, struct regmap_config rc_nolock)
 {
 	const struct realtek_variant *var;
+	const struct of_device_id *match;
 	struct realtek_priv *priv;
 	struct device_node *np;
 	int ret;
 
-	var = of_device_get_match_data(dev);
-	if (!var)
+	match = of_match_device(dev->driver->of_match_table, dev);
+	if (!match || !match->data)
 		return ERR_PTR(-EINVAL);
 
+	var = realtek_variant_get(match);
+	if (IS_ERR(var)) {
+		ret = PTR_ERR(var);
+		dev_err_probe(dev, ret,
+			      "failed to get module for '%s'. Is '%s' loaded?",
+			      match->compatible, match->data);
+		goto err_variant_put;
+	}
+
 	priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
 			    GFP_KERNEL);
-	if (!priv)
-		return ERR_PTR(-ENOMEM);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto err_variant_put;
+	}
 
 	mutex_init(&priv->map_lock);
 
@@ -45,14 +123,14 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
 	if (IS_ERR(priv->map)) {
 		ret = PTR_ERR(priv->map);
 		dev_err(dev, "regmap init failed: %d\n", ret);
-		return ERR_PTR(ret);
+		goto err_variant_put;
 	}
 
 	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);
+		goto err_variant_put;
 	}
 
 	/* Link forward and backward */
@@ -69,23 +147,27 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
 
 	/* Fetch MDIO pins */
 	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->mdc))
-		return ERR_CAST(priv->mdc);
+
+	if (IS_ERR(priv->mdc)) {
+		ret = PTR_ERR(priv->mdc);
+		goto err_variant_put;
+	}
 
 	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->mdio))
-		return ERR_CAST(priv->mdio);
+	if (IS_ERR(priv->mdio)) {
+		ret = PTR_ERR(priv->mdio);
+		goto err_variant_put;
+	}
 
 	np = dev->of_node;
-
 	priv->leds_disabled = of_property_read_bool(np, "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)) {
+		ret = PTR_ERR(priv->reset);
 		dev_err(dev, "failed to get RESET GPIO\n");
-		return ERR_CAST(priv->reset);
+		goto err_variant_put;
 	}
 	if (priv->reset) {
 		gpiod_set_value(priv->reset, 1);
@@ -97,13 +179,20 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
 	}
 
 	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return ERR_PTR(-ENOMEM);
+	if (!priv->ds) {
+		ret = -ENOMEM;
+		goto err_variant_put;
+	}
 
 	priv->ds->dev = dev;
 	priv->ds->priv = priv;
 
 	return priv;
+
+err_variant_put:
+	realtek_variant_put(var);
+
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL(realtek_common_probe);
 
@@ -116,6 +205,8 @@ void realtek_common_remove(struct realtek_priv *priv)
 	if (priv->user_mii_bus)
 		of_node_put(priv->user_mii_bus->dev.of_node);
 
+	realtek_variant_put(priv->variant);
+
 	/* leave the device reset asserted */
 	if (priv->reset)
 		gpiod_set_value(priv->reset, 1);
@@ -124,10 +215,10 @@ EXPORT_SYMBOL(realtek_common_remove);
 
 const struct of_device_id realtek_common_of_match[] = {
 #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
-	{ .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
+	{ .compatible = "realtek,rtl8366rb", .data = REALTEK_RTL8366RB_MODNAME, },
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
-	{ .compatible = "realtek,rtl8365mb", .data = &rtl8366rb_variant, },
+	{ .compatible = "realtek,rtl8365mb", .data = REALTEK_RTL8365MB_MODNAME, },
 #endif
 	{ /* sentinel */ },
 };
diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
index 90a949386518..089fda2d4fa9 100644
--- a/drivers/net/dsa/realtek/realtek-common.h
+++ b/drivers/net/dsa/realtek/realtek-common.h
@@ -12,5 +12,8 @@ void realtek_common_unlock(void *ctx);
 struct realtek_priv *realtek_common_probe(struct device *dev,
 		struct regmap_config rc, struct regmap_config rc_nolock);
 void realtek_common_remove(struct realtek_priv *priv);
+const struct realtek_variant *realtek_variant_get(
+		const struct of_device_id *match);
+void realtek_variant_put(const struct realtek_variant *var);
 
 #endif
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 6f610386c977..6d81cd88dbe6 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -146,7 +146,7 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 	ret = priv->ops->detect(priv);
 	if (ret) {
 		dev_err(dev, "unable to detect switch\n");
-		return ret;
+		goto err_variant_put;
 	}
 
 	priv->ds->num_ports = priv->num_ports;
@@ -155,10 +155,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 	if (ret) {
 		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
 			      ERR_PTR(ret));
-		return ret;
+		goto err_variant_put;
 	}
 
 	return 0;
+
+err_variant_put:
+	realtek_variant_put(priv->variant);
+
+	return ret;
 }
 
 static void realtek_mdio_remove(struct mdio_device *mdiodev)
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 0cf89f9db99e..a772bb7dbe35 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -413,7 +413,7 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	ret = priv->ops->detect(priv);
 	if (ret) {
 		dev_err(dev, "unable to detect switch\n");
-		return ret;
+		goto err_variant_put;
 	}
 
 	priv->ds->num_ports = priv->num_ports;
@@ -422,10 +422,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
 			      ERR_PTR(ret));
-		return ret;
+		goto err_variant_put;
 	}
 
 	return 0;
+
+err_variant_put:
+	realtek_variant_put(priv->variant);
+
+	return ret;
 }
 
 static void realtek_smi_remove(struct platform_device *pdev)
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 8d9d546bf5f5..f9bd6678e3bd 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -16,6 +16,38 @@
 #define REALTEK_HW_STOP_DELAY		25	/* msecs */
 #define REALTEK_HW_START_DELAY		100	/* msecs */
 
+#define REALTEK_RTL8365MB_MODNAME	"rtl8365mb"
+#define REALTEK_RTL8366RB_MODNAME	"rtl8366"
+
+struct realtek_variant_entry {
+	const struct realtek_variant *variant;
+	const char *compatible;
+	struct module *owner;
+	struct list_head list;
+};
+
+#define module_realtek_variant(__variant, __compatible)			\
+static struct realtek_variant_entry __variant ## _entry = {		\
+	.compatible = __compatible,					\
+	.variant = &(__variant),					\
+	.owner = THIS_MODULE,						\
+};									\
+static int __init realtek_variant_module_init(void)			\
+{									\
+	realtek_variant_register(&__variant ## _entry);			\
+	return 0;							\
+}									\
+module_init(realtek_variant_module_init)				\
+									\
+static void __exit realtek_variant_module_exit(void)			\
+{									\
+	realtek_variant_unregister(&__variant ## _entry);		\
+}									\
+module_exit(realtek_variant_module_exit)
+
+void realtek_variant_register(struct realtek_variant_entry *var_ent);
+void realtek_variant_unregister(struct realtek_variant_entry *var_ent);
+
 struct realtek_ops;
 struct dentry;
 struct inode;
@@ -120,6 +152,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 realtek_variant_info *info;
 	const struct realtek_ops *ops;
 	unsigned int clk_delay;
 	u8 cmd_read;
@@ -146,7 +179,4 @@ void rtl8366_get_strings(struct dsa_switch *ds, int port, u32 stringset,
 int rtl8366_get_sset_count(struct dsa_switch *ds, int port, int sset);
 void rtl8366_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data);
 
-extern const struct realtek_variant rtl8366rb_variant;
-extern const struct realtek_variant rtl8365mb_variant;
-
 #endif /*  _REALTEK_H */
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 0875e4fc9f57..b5b22a4d01eb 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -2163,7 +2163,7 @@ static const struct realtek_ops rtl8365mb_ops = {
 	.phy_write = rtl8365mb_phy_write,
 };
 
-const struct realtek_variant rtl8365mb_variant = {
+static const struct realtek_variant rtl8365mb_variant = {
 	.ds_ops_smi = &rtl8365mb_switch_ops_smi,
 	.ds_ops_mdio = &rtl8365mb_switch_ops_mdio,
 	.ops = &rtl8365mb_ops,
@@ -2172,7 +2172,7 @@ const struct realtek_variant rtl8365mb_variant = {
 	.cmd_write = 0xb8,
 	.chip_data_sz = sizeof(struct rtl8365mb),
 };
-EXPORT_SYMBOL_GPL(rtl8365mb_variant);
+module_realtek_variant(rtl8365mb_variant, "realtek,rtl8365mb");
 
 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..208a8f17a089 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1911,7 +1911,7 @@ static const struct realtek_ops rtl8366rb_ops = {
 	.phy_write	= rtl8366rb_phy_write,
 };
 
-const struct realtek_variant rtl8366rb_variant = {
+static const struct realtek_variant rtl8366rb_variant = {
 	.ds_ops_smi = &rtl8366rb_switch_ops_smi,
 	.ds_ops_mdio = &rtl8366rb_switch_ops_mdio,
 	.ops = &rtl8366rb_ops,
@@ -1920,7 +1920,7 @@ const struct realtek_variant rtl8366rb_variant = {
 	.cmd_write = 0xa8,
 	.chip_data_sz = sizeof(struct rtl8366rb),
 };
-EXPORT_SYMBOL_GPL(rtl8366rb_variant);
+module_realtek_variant(rtl8366rb_variant, "realtek,rtl8366rb");
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Driver for RTL8366RB ethernet switch");
-- 
2.42.1

[RFC net-next 5/5] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

The 'reset-gpios' will not work when the switch reset is controlled by a
reset controller.

Although the reset is optional and the driver performs a soft reset
during setup, if the initial reset state was asserted, the driver will
not detect it.

The reset controller will take precedence over the reset GPIO.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-common.c | 53 +++++++++++++++++++++---
 drivers/net/dsa/realtek/realtek-common.h |  3 ++
 drivers/net/dsa/realtek/realtek.h        |  2 +
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
index e383db21c776..1450e5c206c3 100644
--- a/drivers/net/dsa/realtek/realtek-common.c
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -163,17 +163,25 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
 	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
 
 	/* TODO: if power is software controlled, set up any regulators here */
+	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(priv->reset_ctl)) {
+		ret = PTR_ERR(priv->reset_ctl);
+		dev_err_probe(dev, ret, "failed to get reset control\n");
+		goto err_variant_put;
+	}
+
 	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->reset)) {
 		ret = PTR_ERR(priv->reset);
 		dev_err(dev, "failed to get RESET GPIO\n");
 		goto err_variant_put;
 	}
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
+
+	if (priv->reset_ctl || priv->reset) {
+		realtek_reset_assert(priv);
 		dev_dbg(dev, "asserted RESET\n");
 		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
+		realtek_reset_deassert(priv);
 		msleep(REALTEK_HW_START_DELAY);
 		dev_dbg(dev, "deasserted RESET\n");
 	}
@@ -208,11 +216,46 @@ void realtek_common_remove(struct realtek_priv *priv)
 	realtek_variant_put(priv->variant);
 
 	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_reset_assert(priv);
 }
 EXPORT_SYMBOL(realtek_common_remove);
 
+void realtek_reset_assert(struct realtek_priv *priv)
+{
+	int ret;
+
+	if (priv->reset_ctl) {
+		ret = reset_control_assert(priv->reset_ctl);
+		if (!ret)
+			return;
+
+		dev_warn(priv->dev,
+			 "Failed to assert the switch reset control: %pe\n",
+			 ERR_PTR(ret));
+	}
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, true);
+}
+
+void realtek_reset_deassert(struct realtek_priv *priv)
+{
+	int ret;
+
+	if (priv->reset_ctl) {
+		ret = reset_control_deassert(priv->reset_ctl);
+		if (!ret)
+			return;
+
+		dev_warn(priv->dev,
+			 "Failed to deassert the switch reset control: %pe\n",
+			 ERR_PTR(ret));
+	}
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, false);
+}
+
 const struct of_device_id realtek_common_of_match[] = {
 #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
 	{ .compatible = "realtek,rtl8366rb", .data = REALTEK_RTL8366RB_MODNAME, },
diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
index 089fda2d4fa9..603b4f9891d3 100644
--- a/drivers/net/dsa/realtek/realtek-common.h
+++ b/drivers/net/dsa/realtek/realtek-common.h
@@ -16,4 +16,7 @@ const struct realtek_variant *realtek_variant_get(
 		const struct of_device_id *match);
 void realtek_variant_put(const struct realtek_variant *var);
 
+void realtek_reset_assert(struct realtek_priv *priv);
+void realtek_reset_deassert(struct realtek_priv *priv);
+
 #endif
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index f9bd6678e3bd..86f33327155b 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
 #include <net/dsa.h>
+#include <linux/reset.h>
 
 #define REALTEK_HW_STOP_DELAY		25	/* msecs */
 #define REALTEK_HW_START_DELAY		100	/* msecs */
@@ -80,6 +81,7 @@ struct rtl8366_vlan_4k {
 
 struct realtek_priv {
 	struct device		*dev;
+	struct reset_control    *reset_ctl;
 	struct gpio_desc	*reset;
 	struct gpio_desc	*mdc;
 	struct gpio_desc	*mdio;
-- 
2.42.1

Re: [RFC net-next 4/5] net: dsa: realtek: load switch variants on demand

[kernel test robot]

Hi Luiz,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Luiz-Angelo-Daros-de-Luca/dt-bindings-net-dsa-realtek-reset-gpios-is-not-required/20231112-055916
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231111215647.4966-5-luizluca%40gmail.com
patch subject: [RFC net-next 4/5] net: dsa: realtek: load switch variants on demand
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20231112/202311121144.6m9zKmIa-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231112/202311121144.6m9zKmIa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311121144.6m9zKmIa-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   hppa-linux-ld: drivers/mtd/nand/raw/nand_base.o: in function `nand_do_write_ops':
>> (.text+0x4a24): undefined reference to `.L874'
--
   drivers/net/dsa/realtek/realtek-common.c: In function 'realtek_common_probe':
>> drivers/net/dsa/realtek/realtek-common.c:107:68: warning: format '%s' expects argument of type 'char *', but argument 5 has type 'const void *' [-Wformat=]
     107 |                               "failed to get module for '%s'. Is '%s' loaded?",
         |                                                                   ~^
         |                                                                    |
         |                                                                    char *
         |                                                                   %p
     108 |                               match->compatible, match->data);
         |                                                  ~~~~~~~~~~~        
         |                                                       |
         |                                                       const void *


vim +107 drivers/net/dsa/realtek/realtek-common.c

    89	
    90	struct realtek_priv *realtek_common_probe(struct device *dev,
    91			struct regmap_config rc, struct regmap_config rc_nolock)
    92	{
    93		const struct realtek_variant *var;
    94		const struct of_device_id *match;
    95		struct realtek_priv *priv;
    96		struct device_node *np;
    97		int ret;
    98	
    99		match = of_match_device(dev->driver->of_match_table, dev);
   100		if (!match || !match->data)
   101			return ERR_PTR(-EINVAL);
   102	
   103		var = realtek_variant_get(match);
   104		if (IS_ERR(var)) {
   105			ret = PTR_ERR(var);
   106			dev_err_probe(dev, ret,
 > 107				      "failed to get module for '%s'. Is '%s' loaded?",
   108				      match->compatible, match->data);
   109			goto err_variant_put;
   110		}
   111	
   112		priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
   113				    GFP_KERNEL);
   114		if (!priv) {
   115			ret = -ENOMEM;
   116			goto err_variant_put;
   117		}
   118	
   119		mutex_init(&priv->map_lock);
   120	
   121		rc.lock_arg = priv;
   122		priv->map = devm_regmap_init(dev, NULL, priv, &rc);
   123		if (IS_ERR(priv->map)) {
   124			ret = PTR_ERR(priv->map);
   125			dev_err(dev, "regmap init failed: %d\n", ret);
   126			goto err_variant_put;
   127		}
   128	
   129		priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock);
   130		if (IS_ERR(priv->map_nolock)) {
   131			ret = PTR_ERR(priv->map_nolock);
   132			dev_err(dev, "regmap init failed: %d\n", ret);
   133			goto err_variant_put;
   134		}
   135	
   136		/* Link forward and backward */
   137		priv->dev = dev;
   138		priv->clk_delay = var->clk_delay;
   139		priv->cmd_read = var->cmd_read;
   140		priv->cmd_write = var->cmd_write;
   141		priv->variant = var;
   142		priv->ops = var->ops;
   143		priv->chip_data = (void *)priv + sizeof(*priv);
   144	
   145		dev_set_drvdata(dev, priv);
   146		spin_lock_init(&priv->lock);
   147	
   148		/* Fetch MDIO pins */
   149		priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
   150	
   151		if (IS_ERR(priv->mdc)) {
   152			ret = PTR_ERR(priv->mdc);
   153			goto err_variant_put;
   154		}
   155	
   156		priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
   157		if (IS_ERR(priv->mdio)) {
   158			ret = PTR_ERR(priv->mdio);
   159			goto err_variant_put;
   160		}
   161	
   162		np = dev->of_node;
   163		priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
   164	
   165		/* TODO: if power is software controlled, set up any regulators here */
   166		priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
   167		if (IS_ERR(priv->reset)) {
   168			ret = PTR_ERR(priv->reset);
   169			dev_err(dev, "failed to get RESET GPIO\n");
   170			goto err_variant_put;
   171		}
   172		if (priv->reset) {
   173			gpiod_set_value(priv->reset, 1);
   174			dev_dbg(dev, "asserted RESET\n");
   175			msleep(REALTEK_HW_STOP_DELAY);
   176			gpiod_set_value(priv->reset, 0);
   177			msleep(REALTEK_HW_START_DELAY);
   178			dev_dbg(dev, "deasserted RESET\n");
   179		}
   180	
   181		priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
   182		if (!priv->ds) {
   183			ret = -ENOMEM;
   184			goto err_variant_put;
   185		}
   186	
   187		priv->ds->dev = dev;
   188		priv->ds->priv = priv;
   189	
   190		return priv;
   191	
   192	err_variant_put:
   193		realtek_variant_put(var);
   194	
   195		return ERR_PTR(ret);
   196	}
   197	EXPORT_SYMBOL(realtek_common_probe);
   198	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC net-next 1/5] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Krzysztof Kozlowski]

On 11/11/2023 22:51, Luiz Angelo Daros de Luca wrote:
> The 'reset-gpios' should not be mandatory. although they might be
> required for some devices if the switch reset was left asserted by a
> previous driver, such as the bootloader.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Acked-by: Rob Herring <robh@kernel.org>

If this is first RFC, how did you get the Acks? If this is not v1,
provide changelog.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Best regards,
Krzysztof

Re: [RFC net-next 3/5] net: dsa: realtek: create realtek-common

[Krzysztof Kozlowski]

On 11/11/2023 22:51, Luiz Angelo Daros de Luca wrote:
>  	ret = priv->ops->detect(priv);
>  	if (ret) {
> @@ -218,18 +149,12 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  		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);
> +		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> +			      ERR_PTR(ret));

This is some weird code. Why do you print ret twice? This was not
explained in the commit msg.


...
> -	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");
> +		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> +			      ERR_PTR(ret));

Same problem.

Best regards,
Krzysztof

Re: [RFC net-next 1/5] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Linus Walleij]

On Sat, Nov 11, 2023 at 10:57 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> The 'reset-gpios' should not be mandatory. although they might be
> required for some devices if the switch reset was left asserted by a
> previous driver, such as the bootloader.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Acked-by: Rob Herring <robh@kernel.org>

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

Yours,
Linus Walleij

Re: [RFC net-next 2/5] dt-bindings: net: dsa: realtek: add reset controller

[Linus Walleij]

On Sat, Nov 11, 2023 at 10:57 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> Realtek switches can use a reset controller instead of reset-gpios.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>

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

Yours,
Linus Walleij

Re: [RFC net-next 3/5] net: dsa: realtek: create realtek-common

[Linus Walleij]

On Sat, Nov 11, 2023 at 10:57 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> Some code can be shared between both interface modules (MDIO and SMI)
> and amongst variants. For now, these interface functions are shared:
>
> - realtek_common_lock
> - realtek_common_unlock
> - realtek_common_probe
> - realtek_common_remove
>
> The reset during probe was moved to the last moment before a variant
> detects the switch. This way, we avoid a reset if anything else fails.
>
> The symbols from variants used in of_match_table are now in a single
> match table in realtek-common, used by both interfaces.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

As Krzysztof explained the dev_err_probe() call already prints
ret. With that fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC net-next 1/5] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Luiz Angelo Daros de Luca]

> On 11/11/2023 22:51, Luiz Angelo Daros de Luca wrote:
> > The 'reset-gpios' should not be mandatory. although they might be
> > required for some devices if the switch reset was left asserted by a
> > previous driver, such as the bootloader.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > Acked-by: Rob Herring <robh@kernel.org>
>
> If this is first RFC, how did you get the Acks? If this is not v1,
> provide changelog.

Sorry Krzysztof, I might not have handled it correctly. Let me try to fix that.

This RFC is based on a v1/v2 series that morphed into this one.

https://lists.openwall.net/netdev/2023/10/24/348
https://lists.openwall.net/netdev/2023/10/27/257

> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
>
> Best regards,
> Krzysztof
>

Re: [RFC net-next 3/5] net: dsa: realtek: create realtek-common

[Luiz Angelo Daros de Luca]

> > Some code can be shared between both interface modules (MDIO and SMI)
> > and amongst variants. For now, these interface functions are shared:
> >
> > - realtek_common_lock
> > - realtek_common_unlock
> > - realtek_common_probe
> > - realtek_common_remove
> >
> > The reset during probe was moved to the last moment before a variant
> > detects the switch. This way, we avoid a reset if anything else fails.
> >
> > The symbols from variants used in of_match_table are now in a single
> > match table in realtek-common, used by both interfaces.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
>
> As Krzysztof explained the dev_err_probe() call already prints
> ret. With that fixed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks Krzysztof and Linus. I'll be fixed.

Is it ok to keep the series as a whole or should I split it?

Regards,

Luiz

Re: [RFC net-next 3/5] net: dsa: realtek: create realtek-common

[Alvin Šipraga]

Hi Luiz,

On Mon, Nov 13, 2023 at 05:41:49PM -0300, Luiz Angelo Daros de Luca wrote:
> > > Some code can be shared between both interface modules (MDIO and SMI)
> > > and amongst variants. For now, these interface functions are shared:
> > >
> > > - realtek_common_lock
> > > - realtek_common_unlock
> > > - realtek_common_probe
> > > - realtek_common_remove
> > >
> > > The reset during probe was moved to the last moment before a variant
> > > detects the switch. This way, we avoid a reset if anything else fails.
> > >
> > > The symbols from variants used in of_match_table are now in a single
> > > match table in realtek-common, used by both interfaces.
> > >
> > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> >
> > As Krzysztof explained the dev_err_probe() call already prints
> > ret. With that fixed:
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thanks Krzysztof and Linus. I'll be fixed.
> 
> Is it ok to keep the series as a whole or should I split it?

I would prefer that you split it and send the realtek-common series first.
My 2c.

Sorry for not reviewing yet but I'll take a look tomorrow. Cheers!

Kind regards,
Alvin

Re: [RFC net-next 3/5] net: dsa: realtek: create realtek-common

[Alvin Šipraga]

On Sat, Nov 11, 2023 at 06:51:06PM -0300, Luiz Angelo Daros de Luca wrote:
> Some code can be shared between both interface modules (MDIO and SMI)
> and amongst variants. For now, these interface functions are shared:
> 
> - realtek_common_lock
> - realtek_common_unlock
> - realtek_common_probe
> - realtek_common_remove
> 
> The reset during probe was moved to the last moment before a variant
> detects the switch. This way, we avoid a reset if anything else fails.
> 
> The symbols from variants used in of_match_table are now in a single
> match table in realtek-common, used by both interfaces.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/Makefile         |   1 +
>  drivers/net/dsa/realtek/realtek-common.c | 139 +++++++++++++++++++++++
>  drivers/net/dsa/realtek/realtek-common.h |  16 +++
>  drivers/net/dsa/realtek/realtek-mdio.c   | 116 +++----------------
>  drivers/net/dsa/realtek/realtek-smi.c    | 127 +++------------------
>  drivers/net/dsa/realtek/realtek.h        |   2 +
>  6 files changed, 184 insertions(+), 217 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..5e0c1ef200a3 100644
> --- a/drivers/net/dsa/realtek/Makefile
> +++ b/drivers/net/dsa/realtek/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_NET_DSA_REALTEK)		+= 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..36f8b60771be
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-common.c
> @@ -0,0 +1,139 @@
> +// 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);

Shouldn't you update the raw mutex_{lock,unlock}() calls in the chip drivers to
use these common functions as well?

> +
> +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;
> +	struct device_node *np;
> +	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->clk_delay = var->clk_delay;
> +	priv->cmd_read = var->cmd_read;
> +	priv->cmd_write = var->cmd_write;

These are only used by the SMI interface. Since you are storing a pointer to var
in priv anyway, maybe you can remove this part and update the SMI code to use
priv->var->clk_delay etc. Because this is not really common.

The same goes for a couple of other things inside the realtek_priv struct,
e.g. mdio_addr. At the risk of making things too hairy, perhaps you could have
an interface_data pointer just like there is a chip_data pointer. Then the
per-interface stuff can be stored there and the common code is truly common.

> +	priv->variant = var;
> +	priv->ops = var->ops;
> +	priv->chip_data = (void *)priv + sizeof(*priv);
> +
> +	dev_set_drvdata(dev, priv);
> +	spin_lock_init(&priv->lock);
> +
> +	/* Fetch MDIO pins */
> +	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->mdc))
> +		return ERR_CAST(priv->mdc);
> +
> +	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->mdio))
> +		return ERR_CAST(priv->mdio);

Also not common, but specific to the SMI interface driver.

> +
> +	np = dev->of_node;
> +
> +	priv->leds_disabled = of_property_read_bool(np, "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");
> +	}
> +
> +	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> +	if (!priv->ds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv->ds->dev = dev;
> +	priv->ds->priv = priv;

Any reason why you left the assignment of ds->num_ports in the interface
drivers?

> +
> +	return priv;
> +}
> +EXPORT_SYMBOL(realtek_common_probe);
> +
> +void realtek_common_remove(struct realtek_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	dsa_unregister_switch(priv->ds);

It seems a little unbalanced to me that the interface driver's probe function is
responsible for calling dsa_register_switch(), but the common code calls
dsa_unregister_switch().

I understand that you have already done a lot here - all you wanted was to
support reset controllers after all :) - so you don't need to do all this
stuff I am suggesting if you don't want to. But for the parts that you do touch,
please try to keep some balance so that subsequent refactoring is easier.

> +	if (priv->user_mii_bus)
> +		of_node_put(priv->user_mii_bus->dev.of_node);

Similarly this is only used by the SMI interface driver. I think there is a
general need for balance here - not that it was great to begin with. There is
already a setup_interface op in realtek_priv, which is called in the DSA setup
op of the chip drivers. Intuitively I would then expect a teardown_interface op
in realtek_priv to be called in the DSA teardown op of the chip drivers.

> +
> +	/* leave the device reset asserted */
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, 1);
> +}
> +EXPORT_SYMBOL(realtek_common_remove);
> +
> +const struct of_device_id realtek_common_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 = &rtl8366rb_variant, },

Copy-paste error? .data = &rtl8365mb_variant.

> +#endif
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, realtek_common_of_match);
> +EXPORT_SYMBOL_GPL(realtek_common_of_match);
> +
> +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..90a949386518
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-common.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _REALTEK_INTERFACE_H
> +#define _REALTEK_INTERFACE_H
> +
> +#include <linux/regmap.h>
> +
> +extern const struct of_device_id realtek_common_of_match[];
> +
> +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);
> +void realtek_common_remove(struct realtek_priv *priv);
> +
> +#endif
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..6f610386c977 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-common.h"
>  
>  /* Read/write via mdiobus */
>  #define REALTEK_MDIO_CTRL0_REG		31
> @@ -99,20 +100,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,
> @@ -123,8 +110,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 = {
> @@ -142,75 +129,19 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
>  
>  static 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);
> -
> -	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 = realtek_common_probe(dev, realtek_mdio_regmap_config,
> +				    realtek_mdio_nolock_regmap_config);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
>  
>  	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->write_reg_noack = realtek_mdio_write;
> -
> -	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");
> -	}
> +	priv->ds->ops = priv->variant->ds_ops_mdio;
>  
>  	ret = priv->ops->detect(priv);
>  	if (ret) {
> @@ -218,18 +149,12 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  		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);
> +		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> +			      ERR_PTR(ret));
>  		return ret;
>  	}
>  
> @@ -243,11 +168,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return;
>  
> -	dsa_unregister_switch(priv->ds);
> -
> -	/* leave the device reset asserted */
> -	if (priv->reset)
> -		gpiod_set_value(priv->reset, 1);
> +	realtek_common_remove(priv);
>  }
>  
>  static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> @@ -262,21 +183,10 @@ 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,
> +		.of_match_table = realtek_common_of_match,
>  	},
>  	.probe  = realtek_mdio_probe,
>  	.remove = realtek_mdio_remove,
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 755546ed8db6..0cf89f9db99e 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-common.h"
>  
>  #define REALTEK_SMI_ACK_RETRY_COUNT		5
>  
> @@ -310,20 +311,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,
> @@ -334,8 +321,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 = {
> @@ -410,78 +397,18 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  
>  static 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 = realtek_common_probe(dev, realtek_smi_regmap_config,
> +				    realtek_smi_nolock_regmap_config);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
>  
>  	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");
> -	}
> -
> -	/* 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->ds->ops = priv->variant->ds_ops_smi;
>  
>  	ret = priv->ops->detect(priv);
>  	if (ret) {
> @@ -489,20 +416,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  		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");
> +		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> +			      ERR_PTR(ret));
>  		return ret;
>  	}
> +
>  	return 0;
>  }
>  
> @@ -513,13 +435,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
>  	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);
> +	realtek_common_remove(priv);
>  }
>  
>  static void realtek_smi_shutdown(struct platform_device *pdev)
> @@ -534,27 +450,10 @@ 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,
> +		.of_match_table = realtek_common_of_match,
>  	},
>  	.probe  = realtek_smi_probe,
>  	.remove_new = realtek_smi_remove,
> diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> index 790488e9c667..8d9d546bf5f5 100644
> --- a/drivers/net/dsa/realtek/realtek.h
> +++ b/drivers/net/dsa/realtek/realtek.h
> @@ -79,6 +79,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 */
>  };
> -- 
> 2.42.1
>

Re: [RFC net-next 4/5] net: dsa: realtek: load switch variants on demand

[Alvin Šipraga]

On Sat, Nov 11, 2023 at 06:51:07PM -0300, Luiz Angelo Daros de Luca wrote:
> realtek-common had a hard dependency on both switch variants. That way,
> it was not possible to selectively load only one model at runtime. Now
> variants are registered at the realtek-common module and interface
> modules look for a variant using the compatible string.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/realtek-common.c | 125 ++++++++++++++++++++---
>  drivers/net/dsa/realtek/realtek-common.h |   3 +
>  drivers/net/dsa/realtek/realtek-mdio.c   |   9 +-
>  drivers/net/dsa/realtek/realtek-smi.c    |   9 +-
>  drivers/net/dsa/realtek/realtek.h        |  36 ++++++-
>  drivers/net/dsa/realtek/rtl8365mb.c      |   4 +-
>  drivers/net/dsa/realtek/rtl8366rb.c      |   4 +-
>  7 files changed, 162 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> index 36f8b60771be..e383db21c776 100644
> --- a/drivers/net/dsa/realtek/realtek-common.c
> +++ b/drivers/net/dsa/realtek/realtek-common.c
> @@ -1,10 +1,76 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  
>  #include "realtek.h"
>  #include "realtek-common.h"
>  
> +static LIST_HEAD(realtek_variants_list);
> +static DEFINE_MUTEX(realtek_variants_lock);
> +
> +void realtek_variant_register(struct realtek_variant_entry *var_ent)
> +{
> +	mutex_lock(&realtek_variants_lock);
> +	list_add_tail(&var_ent->list, &realtek_variants_list);
> +	mutex_unlock(&realtek_variants_lock);
> +}
> +EXPORT_SYMBOL_GPL(realtek_variant_register);
> +
> +void realtek_variant_unregister(struct realtek_variant_entry *var_ent)
> +{
> +	mutex_lock(&realtek_variants_lock);
> +	list_del(&var_ent->list);
> +	mutex_unlock(&realtek_variants_lock);
> +}
> +EXPORT_SYMBOL_GPL(realtek_variant_unregister);
> +
> +const struct realtek_variant *realtek_variant_get(
> +		const struct of_device_id *match)
> +{
> +	const struct realtek_variant *var = ERR_PTR(-ENOENT);
> +	struct realtek_variant_entry *var_ent;
> +	const char *modname = match->data;
> +
> +	request_module(modname);
> +
> +	mutex_lock(&realtek_variants_lock);
> +	list_for_each_entry(var_ent, &realtek_variants_list, list) {
> +		const struct realtek_variant *tmp = var_ent->variant;
> +
> +		if (strcmp(match->compatible, var_ent->compatible))
> +			continue;
> +
> +		if (!try_module_get(var_ent->owner))
> +			break;
> +
> +		var = tmp;
> +		break;
> +	}

Why not:

list_for_each_entry(...) {
  if (strcmp(...))
    continue;

  if (!try_module_get(...))
    break;

  var = var_ent->variant;
  break;
}

no need for tmp.

Maybe also rename var to variant? tmp, var, foo, etc. have somewhat throw-away
variable connotations... ;)

> +	mutex_unlock(&realtek_variants_lock);
> +
> +	return var;
> +}
> +EXPORT_SYMBOL_GPL(realtek_variant_get);
> +
> +void realtek_variant_put(const struct realtek_variant *var)
> +{
> +	struct realtek_variant_entry *var_ent;
> +
> +	mutex_lock(&realtek_variants_lock);
> +	list_for_each_entry(var_ent, &realtek_variants_list, list) {
> +		if (var_ent->variant != var)
> +			continue;
> +
> +		if (var_ent->owner)
> +			module_put(var_ent->owner);
> +
> +		break;
> +	}
> +	mutex_unlock(&realtek_variants_lock);
> +}
> +EXPORT_SYMBOL_GPL(realtek_variant_put);
> +
>  void realtek_common_lock(void *ctx)
>  {
>  	struct realtek_priv *priv = ctx;
> @@ -25,18 +91,30 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
>  		struct regmap_config rc, struct regmap_config rc_nolock)
>  {
>  	const struct realtek_variant *var;
> +	const struct of_device_id *match;
>  	struct realtek_priv *priv;
>  	struct device_node *np;
>  	int ret;
>  
> -	var = of_device_get_match_data(dev);
> -	if (!var)
> +	match = of_match_device(dev->driver->of_match_table, dev);
> +	if (!match || !match->data)
>  		return ERR_PTR(-EINVAL);
>  
> +	var = realtek_variant_get(match);
> +	if (IS_ERR(var)) {
> +		ret = PTR_ERR(var);
> +		dev_err_probe(dev, ret,
> +			      "failed to get module for '%s'. Is '%s' loaded?",
> +			      match->compatible, match->data);
> +		goto err_variant_put;
> +	}
> +
>  	priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
>  			    GFP_KERNEL);
> -	if (!priv)
> -		return ERR_PTR(-ENOMEM);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err_variant_put;
> +	}
>  
>  	mutex_init(&priv->map_lock);
>  
> @@ -45,14 +123,14 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
>  	if (IS_ERR(priv->map)) {
>  		ret = PTR_ERR(priv->map);
>  		dev_err(dev, "regmap init failed: %d\n", ret);
> -		return ERR_PTR(ret);
> +		goto err_variant_put;
>  	}
>  
>  	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);
> +		goto err_variant_put;
>  	}
>  
>  	/* Link forward and backward */
> @@ -69,23 +147,27 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
>  
>  	/* Fetch MDIO pins */
>  	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
> -	if (IS_ERR(priv->mdc))
> -		return ERR_CAST(priv->mdc);
> +
> +	if (IS_ERR(priv->mdc)) {
> +		ret = PTR_ERR(priv->mdc);
> +		goto err_variant_put;
> +	}
>  
>  	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
> -	if (IS_ERR(priv->mdio))
> -		return ERR_CAST(priv->mdio);
> +	if (IS_ERR(priv->mdio)) {
> +		ret = PTR_ERR(priv->mdio);
> +		goto err_variant_put;
> +	}
>  
>  	np = dev->of_node;
> -
>  	priv->leds_disabled = of_property_read_bool(np, "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)) {
> +		ret = PTR_ERR(priv->reset);
>  		dev_err(dev, "failed to get RESET GPIO\n");
> -		return ERR_CAST(priv->reset);
> +		goto err_variant_put;
>  	}
>  	if (priv->reset) {
>  		gpiod_set_value(priv->reset, 1);
> @@ -97,13 +179,20 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
>  	}
>  
>  	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> -	if (!priv->ds)
> -		return ERR_PTR(-ENOMEM);
> +	if (!priv->ds) {
> +		ret = -ENOMEM;
> +		goto err_variant_put;
> +	}
>  
>  	priv->ds->dev = dev;
>  	priv->ds->priv = priv;
>  
>  	return priv;
> +
> +err_variant_put:
> +	realtek_variant_put(var);
> +
> +	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL(realtek_common_probe);
>  
> @@ -116,6 +205,8 @@ void realtek_common_remove(struct realtek_priv *priv)
>  	if (priv->user_mii_bus)
>  		of_node_put(priv->user_mii_bus->dev.of_node);
>  
> +	realtek_variant_put(priv->variant);
> +
>  	/* leave the device reset asserted */
>  	if (priv->reset)
>  		gpiod_set_value(priv->reset, 1);
> @@ -124,10 +215,10 @@ EXPORT_SYMBOL(realtek_common_remove);
>  
>  const struct of_device_id realtek_common_of_match[] = {
>  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
> -	{ .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
> +	{ .compatible = "realtek,rtl8366rb", .data = REALTEK_RTL8366RB_MODNAME, },
>  #endif
>  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
> -	{ .compatible = "realtek,rtl8365mb", .data = &rtl8366rb_variant, },
> +	{ .compatible = "realtek,rtl8365mb", .data = REALTEK_RTL8365MB_MODNAME, },
>  #endif
>  	{ /* sentinel */ },
>  };
> diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> index 90a949386518..089fda2d4fa9 100644
> --- a/drivers/net/dsa/realtek/realtek-common.h
> +++ b/drivers/net/dsa/realtek/realtek-common.h
> @@ -12,5 +12,8 @@ void realtek_common_unlock(void *ctx);
>  struct realtek_priv *realtek_common_probe(struct device *dev,
>  		struct regmap_config rc, struct regmap_config rc_nolock);
>  void realtek_common_remove(struct realtek_priv *priv);
> +const struct realtek_variant *realtek_variant_get(
> +		const struct of_device_id *match);
> +void realtek_variant_put(const struct realtek_variant *var);
>  
>  #endif
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 6f610386c977..6d81cd88dbe6 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -146,7 +146,7 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  	ret = priv->ops->detect(priv);
>  	if (ret) {
>  		dev_err(dev, "unable to detect switch\n");
> -		return ret;
> +		goto err_variant_put;
>  	}
>  
>  	priv->ds->num_ports = priv->num_ports;
> @@ -155,10 +155,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  	if (ret) {
>  		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
>  			      ERR_PTR(ret));
> -		return ret;
> +		goto err_variant_put;
>  	}
>  
>  	return 0;
> +
> +err_variant_put:
> +	realtek_variant_put(priv->variant);
> +
> +	return ret;
>  }
>  
>  static void realtek_mdio_remove(struct mdio_device *mdiodev)
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 0cf89f9db99e..a772bb7dbe35 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -413,7 +413,7 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  	ret = priv->ops->detect(priv);
>  	if (ret) {
>  		dev_err(dev, "unable to detect switch\n");
> -		return ret;
> +		goto err_variant_put;
>  	}
>  
>  	priv->ds->num_ports = priv->num_ports;
> @@ -422,10 +422,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
>  			      ERR_PTR(ret));
> -		return ret;
> +		goto err_variant_put;
>  	}
>  
>  	return 0;
> +
> +err_variant_put:
> +	realtek_variant_put(priv->variant);
> +
> +	return ret;
>  }
>  
>  static void realtek_smi_remove(struct platform_device *pdev)
> diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> index 8d9d546bf5f5..f9bd6678e3bd 100644
> --- a/drivers/net/dsa/realtek/realtek.h
> +++ b/drivers/net/dsa/realtek/realtek.h
> @@ -16,6 +16,38 @@
>  #define REALTEK_HW_STOP_DELAY		25	/* msecs */
>  #define REALTEK_HW_START_DELAY		100	/* msecs */
>  
> +#define REALTEK_RTL8365MB_MODNAME	"rtl8365mb"
> +#define REALTEK_RTL8366RB_MODNAME	"rtl8366"

The solution is a little baroque but I see the benefit. This however seems
rather brittle. I don't have a good idea of how to improve this but maybe
somebody else will chime in.

> +
> +struct realtek_variant_entry {
> +	const struct realtek_variant *variant;
> +	const char *compatible;
> +	struct module *owner;
> +	struct list_head list;
> +};
> +
> +#define module_realtek_variant(__variant, __compatible)			\
> +static struct realtek_variant_entry __variant ## _entry = {		\
> +	.compatible = __compatible,					\
> +	.variant = &(__variant),					\
> +	.owner = THIS_MODULE,						\
> +};									\
> +static int __init realtek_variant_module_init(void)			\
> +{									\
> +	realtek_variant_register(&__variant ## _entry);			\
> +	return 0;							\
> +}									\
> +module_init(realtek_variant_module_init)				\
> +									\
> +static void __exit realtek_variant_module_exit(void)			\
> +{									\
> +	realtek_variant_unregister(&__variant ## _entry);		\
> +}									\
> +module_exit(realtek_variant_module_exit)
> +
> +void realtek_variant_register(struct realtek_variant_entry *var_ent);
> +void realtek_variant_unregister(struct realtek_variant_entry *var_ent);
> +
>  struct realtek_ops;
>  struct dentry;
>  struct inode;
> @@ -120,6 +152,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 realtek_variant_info *info;

Unused member variable.

Re: [RFC net-next 1/5] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Alvin Šipraga]

On Sat, Nov 11, 2023 at 06:51:04PM -0300, Luiz Angelo Daros de Luca wrote:
> The 'reset-gpios' should not be mandatory. although they might be
> required for some devices if the switch reset was left asserted by a
> previous driver, such as the bootloader.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Acked-by: Rob Herring <robh@kernel.org>

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

> ---
>  Documentation/devicetree/bindings/net/dsa/realtek.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cce692f57b08..46e113df77c8 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -127,7 +127,6 @@ else:
>      - mdc-gpios
>      - mdio-gpios
>      - mdio
> -    - reset-gpios
>  
>  required:
>    - compatible
> -- 
> 2.42.1
>

Re: [RFC net-next 2/5] dt-bindings: net: dsa: realtek: add reset controller

[Alvin Šipraga]

On Sat, Nov 11, 2023 at 06:51:05PM -0300, Luiz Angelo Daros de Luca wrote:
> Realtek switches can use a reset controller instead of reset-gpios.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>

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

> ---
>  Documentation/devicetree/bindings/net/dsa/realtek.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index 46e113df77c8..70b6bda3cf98 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -59,6 +59,9 @@ properties:
>      description: GPIO to be used to reset the whole device
>      maxItems: 1
>  
> +  resets:
> +    maxItems: 1
> +
>    realtek,disable-leds:
>      type: boolean
>      description: |
> -- 
> 2.42.1
>

Re: [RFC net-next 2/5] dt-bindings: net: dsa: realtek: add reset controller

[Rob Herring]

On Sat, 11 Nov 2023 18:51:05 -0300, Luiz Angelo Daros de Luca wrote:
> Realtek switches can use a reset controller instead of reset-gpios.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/realtek.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [RFC net-next 4/5] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

> On Sat, Nov 11, 2023 at 06:51:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > realtek-common had a hard dependency on both switch variants. That way,
> > it was not possible to selectively load only one model at runtime. Now
> > variants are registered at the realtek-common module and interface
> > modules look for a variant using the compatible string.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> >  drivers/net/dsa/realtek/realtek-common.c | 125 ++++++++++++++++++++---
> >  drivers/net/dsa/realtek/realtek-common.h |   3 +
> >  drivers/net/dsa/realtek/realtek-mdio.c   |   9 +-
> >  drivers/net/dsa/realtek/realtek-smi.c    |   9 +-
> >  drivers/net/dsa/realtek/realtek.h        |  36 ++++++-
> >  drivers/net/dsa/realtek/rtl8365mb.c      |   4 +-
> >  drivers/net/dsa/realtek/rtl8366rb.c      |   4 +-
> >  7 files changed, 162 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> > index 36f8b60771be..e383db21c776 100644
> > --- a/drivers/net/dsa/realtek/realtek-common.c
> > +++ b/drivers/net/dsa/realtek/realtek-common.c
> > @@ -1,10 +1,76 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >
> >  #include <linux/module.h>
> > +#include <linux/of_device.h>
> >
> >  #include "realtek.h"
> >  #include "realtek-common.h"
> >
> > +static LIST_HEAD(realtek_variants_list);
> > +static DEFINE_MUTEX(realtek_variants_lock);
> > +
> > +void realtek_variant_register(struct realtek_variant_entry *var_ent)
> > +{
> > +     mutex_lock(&realtek_variants_lock);
> > +     list_add_tail(&var_ent->list, &realtek_variants_list);
> > +     mutex_unlock(&realtek_variants_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_variant_register);
> > +
> > +void realtek_variant_unregister(struct realtek_variant_entry *var_ent)
> > +{
> > +     mutex_lock(&realtek_variants_lock);
> > +     list_del(&var_ent->list);
> > +     mutex_unlock(&realtek_variants_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_variant_unregister);
> > +
> > +const struct realtek_variant *realtek_variant_get(
> > +             const struct of_device_id *match)
> > +{
> > +     const struct realtek_variant *var = ERR_PTR(-ENOENT);
> > +     struct realtek_variant_entry *var_ent;
> > +     const char *modname = match->data;
> > +
> > +     request_module(modname);
> > +
> > +     mutex_lock(&realtek_variants_lock);
> > +     list_for_each_entry(var_ent, &realtek_variants_list, list) {
> > +             const struct realtek_variant *tmp = var_ent->variant;
> > +
> > +             if (strcmp(match->compatible, var_ent->compatible))
> > +                     continue;
> > +
> > +             if (!try_module_get(var_ent->owner))
> > +                     break;
> > +
> > +             var = tmp;
> > +             break;
> > +     }
>
> Why not:
>
> list_for_each_entry(...) {
>   if (strcmp(...))
>     continue;
>
>   if (!try_module_get(...))
>     break;
>
>   var = var_ent->variant;
>   break;
> }
>
> no need for tmp.
>
> Maybe also rename var to variant? tmp, var, foo, etc. have somewhat throw-away
> variable connotations... ;)

You are right. That tmp came from dsa tag.c. At first, it had some use
as the match was using fields from the variant. However, once I opted
to use the compatible string, variant became just the result.

> > +     mutex_unlock(&realtek_variants_lock);
> > +
> > +     return var;
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_variant_get);
> > +
> > +void realtek_variant_put(const struct realtek_variant *var)
> > +{
> > +     struct realtek_variant_entry *var_ent;
> > +
> > +     mutex_lock(&realtek_variants_lock);
> > +     list_for_each_entry(var_ent, &realtek_variants_list, list) {
> > +             if (var_ent->variant != var)
> > +                     continue;
> > +
> > +             if (var_ent->owner)
> > +                     module_put(var_ent->owner);
> > +
> > +             break;
> > +     }
> > +     mutex_unlock(&realtek_variants_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_variant_put);
> > +
> >  void realtek_common_lock(void *ctx)
> >  {
> >       struct realtek_priv *priv = ctx;
> > @@ -25,18 +91,30 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
> >               struct regmap_config rc, struct regmap_config rc_nolock)
> >  {
> >       const struct realtek_variant *var;
> > +     const struct of_device_id *match;
> >       struct realtek_priv *priv;
> >       struct device_node *np;
> >       int ret;
> >
> > -     var = of_device_get_match_data(dev);
> > -     if (!var)
> > +     match = of_match_device(dev->driver->of_match_table, dev);
> > +     if (!match || !match->data)
> >               return ERR_PTR(-EINVAL);
> >
> > +     var = realtek_variant_get(match);
> > +     if (IS_ERR(var)) {
> > +             ret = PTR_ERR(var);
> > +             dev_err_probe(dev, ret,
> > +                           "failed to get module for '%s'. Is '%s' loaded?",
> > +                           match->compatible, match->data);
> > +             goto err_variant_put;
> > +     }
> > +
> >       priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz),
> >                           GFP_KERNEL);
> > -     if (!priv)
> > -             return ERR_PTR(-ENOMEM);
> > +     if (!priv) {
> > +             ret = -ENOMEM;
> > +             goto err_variant_put;
> > +     }
> >
> >       mutex_init(&priv->map_lock);
> >
> > @@ -45,14 +123,14 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
> >       if (IS_ERR(priv->map)) {
> >               ret = PTR_ERR(priv->map);
> >               dev_err(dev, "regmap init failed: %d\n", ret);
> > -             return ERR_PTR(ret);
> > +             goto err_variant_put;
> >       }
> >
> >       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);
> > +             goto err_variant_put;
> >       }
> >
> >       /* Link forward and backward */
> > @@ -69,23 +147,27 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
> >
> >       /* Fetch MDIO pins */
> >       priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
> > -     if (IS_ERR(priv->mdc))
> > -             return ERR_CAST(priv->mdc);
> > +
> > +     if (IS_ERR(priv->mdc)) {
> > +             ret = PTR_ERR(priv->mdc);
> > +             goto err_variant_put;
> > +     }
> >
> >       priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
> > -     if (IS_ERR(priv->mdio))
> > -             return ERR_CAST(priv->mdio);
> > +     if (IS_ERR(priv->mdio)) {
> > +             ret = PTR_ERR(priv->mdio);
> > +             goto err_variant_put;
> > +     }
> >
> >       np = dev->of_node;
> > -
> >       priv->leds_disabled = of_property_read_bool(np, "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)) {
> > +             ret = PTR_ERR(priv->reset);
> >               dev_err(dev, "failed to get RESET GPIO\n");
> > -             return ERR_CAST(priv->reset);
> > +             goto err_variant_put;
> >       }
> >       if (priv->reset) {
> >               gpiod_set_value(priv->reset, 1);
> > @@ -97,13 +179,20 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
> >       }
> >
> >       priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> > -     if (!priv->ds)
> > -             return ERR_PTR(-ENOMEM);
> > +     if (!priv->ds) {
> > +             ret = -ENOMEM;
> > +             goto err_variant_put;
> > +     }
> >
> >       priv->ds->dev = dev;
> >       priv->ds->priv = priv;
> >
> >       return priv;
> > +
> > +err_variant_put:
> > +     realtek_variant_put(var);
> > +
> > +     return ERR_PTR(ret);
> >  }
> >  EXPORT_SYMBOL(realtek_common_probe);
> >
> > @@ -116,6 +205,8 @@ void realtek_common_remove(struct realtek_priv *priv)
> >       if (priv->user_mii_bus)
> >               of_node_put(priv->user_mii_bus->dev.of_node);
> >
> > +     realtek_variant_put(priv->variant);
> > +
> >       /* leave the device reset asserted */
> >       if (priv->reset)
> >               gpiod_set_value(priv->reset, 1);
> > @@ -124,10 +215,10 @@ EXPORT_SYMBOL(realtek_common_remove);
> >
> >  const struct of_device_id realtek_common_of_match[] = {
> >  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB)
> > -     { .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, },
> > +     { .compatible = "realtek,rtl8366rb", .data = REALTEK_RTL8366RB_MODNAME, },
> >  #endif
> >  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
> > -     { .compatible = "realtek,rtl8365mb", .data = &rtl8366rb_variant, },
> > +     { .compatible = "realtek,rtl8365mb", .data = REALTEK_RTL8365MB_MODNAME, },
> >  #endif
> >       { /* sentinel */ },
> >  };
> > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> > index 90a949386518..089fda2d4fa9 100644
> > --- a/drivers/net/dsa/realtek/realtek-common.h
> > +++ b/drivers/net/dsa/realtek/realtek-common.h
> > @@ -12,5 +12,8 @@ void realtek_common_unlock(void *ctx);
> >  struct realtek_priv *realtek_common_probe(struct device *dev,
> >               struct regmap_config rc, struct regmap_config rc_nolock);
> >  void realtek_common_remove(struct realtek_priv *priv);
> > +const struct realtek_variant *realtek_variant_get(
> > +             const struct of_device_id *match);
> > +void realtek_variant_put(const struct realtek_variant *var);
> >
> >  #endif
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 6f610386c977..6d81cd88dbe6 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -146,7 +146,7 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >       ret = priv->ops->detect(priv);
> >       if (ret) {
> >               dev_err(dev, "unable to detect switch\n");
> > -             return ret;
> > +             goto err_variant_put;
> >       }
> >
> >       priv->ds->num_ports = priv->num_ports;
> > @@ -155,10 +155,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >       if (ret) {
> >               dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> >                             ERR_PTR(ret));
> > -             return ret;
> > +             goto err_variant_put;
> >       }
> >
> >       return 0;
> > +
> > +err_variant_put:
> > +     realtek_variant_put(priv->variant);
> > +
> > +     return ret;
> >  }
> >
> >  static void realtek_mdio_remove(struct mdio_device *mdiodev)
> > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> > index 0cf89f9db99e..a772bb7dbe35 100644
> > --- a/drivers/net/dsa/realtek/realtek-smi.c
> > +++ b/drivers/net/dsa/realtek/realtek-smi.c
> > @@ -413,7 +413,7 @@ static int realtek_smi_probe(struct platform_device *pdev)
> >       ret = priv->ops->detect(priv);
> >       if (ret) {
> >               dev_err(dev, "unable to detect switch\n");
> > -             return ret;
> > +             goto err_variant_put;
> >       }
> >
> >       priv->ds->num_ports = priv->num_ports;
> > @@ -422,10 +422,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
> >       if (ret) {
> >               dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> >                             ERR_PTR(ret));
> > -             return ret;
> > +             goto err_variant_put;
> >       }
> >
> >       return 0;
> > +
> > +err_variant_put:
> > +     realtek_variant_put(priv->variant);
> > +
> > +     return ret;
> >  }
> >
> >  static void realtek_smi_remove(struct platform_device *pdev)
> > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> > index 8d9d546bf5f5..f9bd6678e3bd 100644
> > --- a/drivers/net/dsa/realtek/realtek.h
> > +++ b/drivers/net/dsa/realtek/realtek.h
> > @@ -16,6 +16,38 @@
> >  #define REALTEK_HW_STOP_DELAY                25      /* msecs */
> >  #define REALTEK_HW_START_DELAY               100     /* msecs */
> >
> > +#define REALTEK_RTL8365MB_MODNAME    "rtl8365mb"
> > +#define REALTEK_RTL8366RB_MODNAME    "rtl8366"
>
> The solution is a little baroque but I see the benefit. This however seems
> rather brittle. I don't have a good idea of how to improve this but maybe
> somebody else will chime in.

I need some way to map "this CHIP requires module XXX" in order to let
it request the module. DSA tags, for example, have a module format
based on the tag name. Here, rtl8365mb matches the compatible string
but rtl8366rb doesn't. We discussed in the past to keep a single
compatible string for each driver when we dropped the "rtl8367s"
string. We could migrate the rtl8366-core to realtek-common and rename
the module from rtl8366 to rtl8366rb.

Anyway, I'll try another solution for now. How about
MODULE_ALIAS("realtek,rtl8365mb")? It seems to work nicely.

> > +
> > +struct realtek_variant_entry {
> > +     const struct realtek_variant *variant;
> > +     const char *compatible;
> > +     struct module *owner;
> > +     struct list_head list;
> > +};
> > +
> > +#define module_realtek_variant(__variant, __compatible)                      \
> > +static struct realtek_variant_entry __variant ## _entry = {          \
> > +     .compatible = __compatible,                                     \
> > +     .variant = &(__variant),                                        \
> > +     .owner = THIS_MODULE,                                           \
> > +};                                                                   \
> > +static int __init realtek_variant_module_init(void)                  \
> > +{                                                                    \
> > +     realtek_variant_register(&__variant ## _entry);                 \
> > +     return 0;                                                       \
> > +}                                                                    \
> > +module_init(realtek_variant_module_init)                             \
> > +                                                                     \
> > +static void __exit realtek_variant_module_exit(void)                 \
> > +{                                                                    \
> > +     realtek_variant_unregister(&__variant ## _entry);               \
> > +}                                                                    \
> > +module_exit(realtek_variant_module_exit)
> > +
> > +void realtek_variant_register(struct realtek_variant_entry *var_ent);
> > +void realtek_variant_unregister(struct realtek_variant_entry *var_ent);
> > +
> >  struct realtek_ops;
> >  struct dentry;
> >  struct inode;
> > @@ -120,6 +152,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 realtek_variant_info *info;
>
> Unused member variable.

Removed.

Thanks Alvin, I might send a new series with 3/5 and 4/5 soon with the
changes after some more tests.

Regards,

Luiz

Re: [RFC net-next 3/5] net: dsa: realtek: create realtek-common

[Luiz Angelo Daros de Luca]

> > Some code can be shared between both interface modules (MDIO and SMI)
> > and amongst variants. For now, these interface functions are shared:
> >
> > - realtek_common_lock
> > - realtek_common_unlock
> > - realtek_common_probe
> > - realtek_common_remove
> >
> > The reset during probe was moved to the last moment before a variant
> > detects the switch. This way, we avoid a reset if anything else fails.
> >
> > The symbols from variants used in of_match_table are now in a single
> > match table in realtek-common, used by both interfaces.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> >  drivers/net/dsa/realtek/Makefile         |   1 +
> >  drivers/net/dsa/realtek/realtek-common.c | 139 +++++++++++++++++++++++
> >  drivers/net/dsa/realtek/realtek-common.h |  16 +++
> >  drivers/net/dsa/realtek/realtek-mdio.c   | 116 +++----------------
> >  drivers/net/dsa/realtek/realtek-smi.c    | 127 +++------------------
> >  drivers/net/dsa/realtek/realtek.h        |   2 +
> >  6 files changed, 184 insertions(+), 217 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..5e0c1ef200a3 100644
> > --- a/drivers/net/dsa/realtek/Makefile
> > +++ b/drivers/net/dsa/realtek/Makefile
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_NET_DSA_REALTEK)                += 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..36f8b60771be
> > --- /dev/null
> > +++ b/drivers/net/dsa/realtek/realtek-common.c
> > @@ -0,0 +1,139 @@
> > +// 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);
>
> Shouldn't you update the raw mutex_{lock,unlock}() calls in the chip drivers to
> use these common functions as well?

I wasn't focusing on chip drivers for now but, yes, they should. There
might be more code to be shared between those chip drivers but I don't
want to touch that for now.

> > +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;
> > +     struct device_node *np;
> > +     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->clk_delay = var->clk_delay;
> > +     priv->cmd_read = var->cmd_read;
> > +     priv->cmd_write = var->cmd_write;
>
> These are only used by the SMI interface. Since you are storing a pointer to var
> in priv anyway, maybe you can remove this part and update the SMI code to use
> priv->var->clk_delay etc. Because this is not really common.

Yes. variants are statically allocated and there is no reason to copy
those static values to a dynamic allocated structure.
I'll drop them and use the variant reference.

> The same goes for a couple of other things inside the realtek_priv struct,
> e.g. mdio_addr. At the risk of making things too hairy, perhaps you could have
> an interface_data pointer just like there is a chip_data pointer. Then the
> per-interface stuff can be stored there and the common code is truly common.

I think these are all fields used only by MDIO interface:

       struct mii_bus          *bus;
       int                     mdio_addr;
       struct mii_bus          *user_mii_bus;
       spinlock_t              lock;

And these are for SMI:

       struct gpio_desc        *mdc;
       struct gpio_desc        *mdio;

I don't know the kernel internal memory allocator but I believe that a
couple of unused fields in a structure might be better than using
kmalloc twice.

> > +     priv->variant = var;
> > +     priv->ops = var->ops;
> > +     priv->chip_data = (void *)priv + sizeof(*priv);
> > +
> > +     dev_set_drvdata(dev, priv);
> > +     spin_lock_init(&priv->lock);
> > +
> > +     /* Fetch MDIO pins */
> > +     priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
> > +     if (IS_ERR(priv->mdc))
> > +             return ERR_CAST(priv->mdc);
> > +
> > +     priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
> > +     if (IS_ERR(priv->mdio))
> > +             return ERR_CAST(priv->mdio);
>
> Also not common, but specific to the SMI interface driver.

I was trying to do all the device-tree ops before actually resetting
the switch just to let it possibly fail sooner than the msleep().
However, a failed device-tree prop that would break probing is just
too rare in a production system to care. I'll move it to realtek-smi.

> > +
> > +     np = dev->of_node;
> > +
> > +     priv->leds_disabled = of_property_read_bool(np, "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");
> > +     }
> > +
> > +     priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> > +     if (!priv->ds)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     priv->ds->dev = dev;
> > +     priv->ds->priv = priv;
>
> Any reason why you left the assignment of ds->num_ports in the interface
> drivers?

The priv->num_ports is set by the chip variants during
priv->ops->detect(priv), which requires some priv setting specific for
each interface. In fact, I noticed that when I tested the code.

> > +
> > +     return priv;
> > +}
> > +EXPORT_SYMBOL(realtek_common_probe);
> > +
> > +void realtek_common_remove(struct realtek_priv *priv)
> > +{
> > +     if (!priv)
> > +             return;
> > +
> > +     dsa_unregister_switch(priv->ds);
>
> It seems a little unbalanced to me that the interface driver's probe function is
> responsible for calling dsa_register_switch(), but the common code calls
> dsa_unregister_switch().

Yes, it is. I don't like it but moving the detect+register code to a
realtek_common_{probe2/detect/register} will create a strange function
and splitting them will be too much abstraction for nothing.
It will get a little worse in the 4/5 patch when we need to call
realtek_variant_put() that will run module_put() if the probe fails.
It would be easier if we had a devm_try_module_get() that would handle
the module_put().

> I understand that you have already done a lot here - all you wanted was to
> support reset controllers after all :) - so you don't need to do all this
> stuff I am suggesting if you don't want to. But for the parts that you do touch,
> please try to keep some balance so that subsequent refactoring is easier.
>
> > +     if (priv->user_mii_bus)
> > +             of_node_put(priv->user_mii_bus->dev.of_node);
>
> Similarly this is only used by the SMI interface driver. I think there is a
> general need for balance here - not that it was great to begin with. There is
> already a setup_interface op in realtek_priv, which is called in the DSA setup
> op of the chip drivers. Intuitively I would then expect a teardown_interface op
> in realtek_priv to be called in the DSA teardown op of the chip drivers.

I agree. The same module responsible to register/get should be the one
unregistering/putting whenever possible.

We might need some extra refactoring. I just noticed, for example,
realtek_ops->cleanup is not in use. I believe rtl8366-core.c would
better fit in realtek-common as it is probably usable by rtl8365mb for
vlan support. However, I prefer to merge this smaller series that will
already give the users benefits while it also paves the way for
further refactoring.

> > +
> > +     /* leave the device reset asserted */
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, 1);
> > +}
> > +EXPORT_SYMBOL(realtek_common_remove);
> > +
> > +const struct of_device_id realtek_common_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 = &rtl8366rb_variant, },
>
> Copy-paste error? .data = &rtl8365mb_variant.

Yes. I didn't test this intermediate patch with rtl8365mb. Thanks.


>
> > +#endif
> > +     { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, realtek_common_of_match);
> > +EXPORT_SYMBOL_GPL(realtek_common_of_match);
> > +
> > +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..90a949386518
> > --- /dev/null
> > +++ b/drivers/net/dsa/realtek/realtek-common.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +
> > +#ifndef _REALTEK_INTERFACE_H
> > +#define _REALTEK_INTERFACE_H
> > +
> > +#include <linux/regmap.h>
> > +
> > +extern const struct of_device_id realtek_common_of_match[];
> > +
> > +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);
> > +void realtek_common_remove(struct realtek_priv *priv);
> > +
> > +#endif
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 292e6d087e8b..6f610386c977 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-common.h"
> >
> >  /* Read/write via mdiobus */
> >  #define REALTEK_MDIO_CTRL0_REG               31
> > @@ -99,20 +100,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,
> > @@ -123,8 +110,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 = {
> > @@ -142,75 +129,19 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
> >
> >  static 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);
> > -
> > -     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 = realtek_common_probe(dev, realtek_mdio_regmap_config,
> > +                                 realtek_mdio_nolock_regmap_config);
> > +     if (IS_ERR(priv))
> > +             return PTR_ERR(priv);
> >
> >       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->write_reg_noack = realtek_mdio_write;
> > -
> > -     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");
> > -     }
> > +     priv->ds->ops = priv->variant->ds_ops_mdio;
> >
> >       ret = priv->ops->detect(priv);
> >       if (ret) {
> > @@ -218,18 +149,12 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >               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);
> > +             dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> > +                           ERR_PTR(ret));
> >               return ret;
> >       }
> >
> > @@ -243,11 +168,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
> >       if (!priv)
> >               return;
> >
> > -     dsa_unregister_switch(priv->ds);
> > -
> > -     /* leave the device reset asserted */
> > -     if (priv->reset)
> > -             gpiod_set_value(priv->reset, 1);
> > +     realtek_common_remove(priv);
> >  }
> >
> >  static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> > @@ -262,21 +183,10 @@ 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,
> > +             .of_match_table = realtek_common_of_match,
> >       },
> >       .probe  = realtek_mdio_probe,
> >       .remove = realtek_mdio_remove,
> > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> > index 755546ed8db6..0cf89f9db99e 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-common.h"
> >
> >  #define REALTEK_SMI_ACK_RETRY_COUNT          5
> >
> > @@ -310,20 +311,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,
> > @@ -334,8 +321,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 = {
> > @@ -410,78 +397,18 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
> >
> >  static 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 = realtek_common_probe(dev, realtek_smi_regmap_config,
> > +                                 realtek_smi_nolock_regmap_config);
> > +     if (IS_ERR(priv))
> > +             return PTR_ERR(priv);
> >
> >       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");
> > -     }
> > -
> > -     /* 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->ds->ops = priv->variant->ds_ops_smi;
> >
> >       ret = priv->ops->detect(priv);
> >       if (ret) {
> > @@ -489,20 +416,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
> >               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");
> > +             dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> > +                           ERR_PTR(ret));
> >               return ret;
> >       }
> > +
> >       return 0;
> >  }
> >
> > @@ -513,13 +435,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
> >       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);
> > +     realtek_common_remove(priv);
> >  }
> >
> >  static void realtek_smi_shutdown(struct platform_device *pdev)
> > @@ -534,27 +450,10 @@ 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,
> > +             .of_match_table = realtek_common_of_match,
> >       },
> >       .probe  = realtek_smi_probe,
> >       .remove_new = realtek_smi_remove,
> > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> > index 790488e9c667..8d9d546bf5f5 100644
> > --- a/drivers/net/dsa/realtek/realtek.h
> > +++ b/drivers/net/dsa/realtek/realtek.h
> > @@ -79,6 +79,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 */
> >  };
> > --
> > 2.42.1
> >