[net-next 0/2] net: dsa: realtek: Introduce realtek_common, load variants on demand

[net-next 0/2] net: dsa: realtek: Introduce realtek_common, load variants on demand

[Luiz Angelo Daros de Luca]

The current driver comprises two interface modules (SMI and MDIO) and
two family/variant modules (RTL8365MB and RTL8366RB). While the
interface modules are independent and can be loaded only when necessary,
the symbols from variant modules are utilized by interface modules. This
requires loading all enabled variants simultaneously or disabling them
at build time. This approach, although simple, does not scale well,
particularly with the addition of more switch variants (e.g., RTL8366B),
resulting in loaded but unused modules.

This patch series refactors the Realtek DSA switch code by introducing a
common module shared by all existing interfaces (SMI or MDIO) and switch
variant modules (RTL8365MB and RTL8366RB). The common module primarily
contains parts of the probe code common to both interfaces, with the
possibility of incorporating more bits from variants in the future.

The relationship between interfaces and variant modules is also
modified. Variant modules now register themselves in realtek_common
using the compatible string as the key, which is also added as the
module alias. When an interface module probes a device, it utilizes the
matching string to both request the module, loading it if necessary, and
obtain the required variant reference.

Tested with a RTL8367S (RTL8365MB) using MDIO interface and a RTL8366RB
(RTL8366) with SMI interface.

Regards,

Luiz

--

Changelog:

RFC->v1
- Drop the reset-controller patches. It will be submitted in a different
  series.
- Renamed 'var' to 'variant'
- Used 'compatible' as MODULE_ALIAS; avoided static module names in
  macros
- Dropped tmp variable in realtek_variant_get()
- realtek_variant_get() now exclusively uses the compatible string
- Removed clk_delay, cmd_{read,write) from realtek_priv as they are
  already in realtek_variant
- Moved mdc/mdio from realtek_common_probe() to realtek_smi_probe()
- Moved module_realtek_variant() macro to realtek-common.h
- module_realtek_variant() now also generates MODULE_ALIAS()
- Avoided repeating error code with dev_err_probe()
- Removed unused realtek_variant.info
- Used realtek_common_(un)lock() in chip variant modules

[net-next 1/2] net: dsa: realtek: create realtek-common

[Luiz Angelo Daros de Luca]

Some code can be shared between both interface modules (MDIO and SMI)
and among variants. Currently, 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/realtek/Makefile         |   1 +
 drivers/net/dsa/realtek/realtek-common.c | 127 +++++++++++++++++++++++
 drivers/net/dsa/realtek/realtek-common.h |  16 +++
 drivers/net/dsa/realtek/realtek-mdio.c   | 115 +++-----------------
 drivers/net/dsa/realtek/realtek-smi.c    | 127 +++--------------------
 drivers/net/dsa/realtek/realtek.h        |   5 +-
 drivers/net/dsa/realtek/rtl8365mb.c      |   9 +-
 drivers/net/dsa/realtek/rtl8366rb.c      |   9 +-
 8 files changed, 185 insertions(+), 224 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..1b733ac56560
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -0,0 +1,127 @@
+// 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->variant = var;
+	priv->ops = var->ops;
+	priv->chip_data = (void *)priv + sizeof(*priv);
+
+	dev_set_drvdata(dev, priv);
+	spin_lock_init(&priv->lock);
+
+	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 = &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..b865e11955ca 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,94 +129,31 @@ 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");
-	}
-
 	ret = priv->ops->detect(priv);
 	if (ret) {
 		dev_err(dev, "unable to detect switch\n");
 		return ret;
 	}
 
-	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
-	if (!priv->ds)
-		return -ENOMEM;
-
-	priv->ds->dev = dev;
+	priv->ds->ops = priv->variant->ds_ops_mdio;
 	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\n");
 		return ret;
 	}
 
@@ -243,11 +167,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 +182,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..2aebcbe0425f 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -40,12 +40,13 @@
 #include <linux/if_bridge.h>
 
 #include "realtek.h"
+#include "realtek-common.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
 
 static inline void realtek_smi_clk_delay(struct realtek_priv *priv)
 {
-	ndelay(priv->clk_delay);
+	ndelay(priv->variant->clk_delay);
 }
 
 static void realtek_smi_start(struct realtek_priv *priv)
@@ -208,7 +209,7 @@ static int realtek_smi_read_reg(struct realtek_priv *priv, u32 addr, u32 *data)
 	realtek_smi_start(priv);
 
 	/* Send READ command */
-	ret = realtek_smi_write_byte(priv, priv->cmd_read);
+	ret = realtek_smi_write_byte(priv, priv->variant->cmd_read);
 	if (ret)
 		goto out;
 
@@ -249,7 +250,7 @@ static int realtek_smi_write_reg(struct realtek_priv *priv,
 	realtek_smi_start(priv);
 
 	/* Send WRITE command */
-	ret = realtek_smi_write_byte(priv, priv->cmd_write);
+	ret = realtek_smi_write_byte(priv, priv->variant->cmd_write);
 	if (ret)
 		goto out;
 
@@ -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,26 @@ 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->setup_interface = realtek_smi_setup_mdio;
-	priv->write_reg_noack = realtek_smi_write_reg_noack;
-
-	dev_set_drvdata(dev, priv);
-	spin_lock_init(&priv->lock);
-
-	/* TODO: if power is software controlled, set up any regulators here */
-
-	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->reset)) {
-		dev_err(dev, "failed to get RESET GPIO\n");
-		return PTR_ERR(priv->reset);
-	}
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
-		dev_dbg(dev, "asserted RESET\n");
-		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
-		msleep(REALTEK_HW_START_DELAY);
-		dev_dbg(dev, "deasserted RESET\n");
-	}
+	priv = realtek_common_probe(dev, realtek_smi_regmap_config,
+				    realtek_smi_nolock_regmap_config);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
 
 	/* Fetch MDIO pins */
 	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->mdc))
 		return PTR_ERR(priv->mdc);
+
 	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->mdio))
 		return PTR_ERR(priv->mdio);
 
-	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
+	priv->setup_interface = realtek_smi_setup_mdio;
+	priv->write_reg_noack = realtek_smi_write_reg_noack;
 
 	ret = priv->ops->detect(priv);
 	if (ret) {
@@ -489,20 +424,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->ops = priv->variant->ds_ops_smi;
 	priv->ds->num_ports = priv->num_ports;
-	priv->ds->priv = priv;
 
-	priv->ds->ops = var->ds_ops_smi;
 	ret = dsa_register_switch(priv->ds);
 	if (ret) {
 		dev_err_probe(dev, ret, "unable to register switch\n");
 		return ret;
 	}
+
 	return 0;
 }
 
@@ -513,13 +443,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 +458,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..fbbdf538908e 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -58,9 +58,6 @@ struct realtek_priv {
 	struct mii_bus		*bus;
 	int			mdio_addr;
 
-	unsigned int		clk_delay;
-	u8			cmd_read;
-	u8			cmd_write;
 	spinlock_t		lock; /* Locks around command writes */
 	struct dsa_switch	*ds;
 	struct irq_domain	*irqdomain;
@@ -79,6 +76,8 @@ struct realtek_priv {
 	int			vlan_enabled;
 	int			vlan4k_enabled;
 
+	const struct realtek_variant *variant;
+
 	char			buf[4096];
 	void			*chip_data; /* Per-chip extra variant data */
 };
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 0875e4fc9f57..9b18774e988c 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -101,6 +101,7 @@
 #include <linux/if_vlan.h>
 
 #include "realtek.h"
+#include "realtek-common.h"
 
 /* Family-specific data and limits */
 #define RTL8365MB_PHYADDRMAX		7
@@ -689,7 +690,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
 	u32 val;
 	int ret;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = rtl8365mb_phy_poll_busy(priv);
 	if (ret)
@@ -722,7 +723,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
 	*data = val & 0xFFFF;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return ret;
 }
@@ -733,7 +734,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy,
 	u32 val;
 	int ret;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = rtl8365mb_phy_poll_busy(priv);
 	if (ret)
@@ -764,7 +765,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy,
 		goto out;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index b39b719a5b8f..1ac2fd098242 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -22,6 +22,7 @@
 #include <linux/regmap.h>
 
 #include "realtek.h"
+#include "realtek-common.h"
 
 #define RTL8366RB_PORT_NUM_CPU		5
 #define RTL8366RB_NUM_PORTS		6
@@ -1705,7 +1706,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
 	if (phy > RTL8366RB_PHY_NO_MAX)
 		return -EINVAL;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
 			   RTL8366RB_PHY_CTRL_READ);
@@ -1733,7 +1734,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
 		phy, regnum, reg, val);
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return ret;
 }
@@ -1747,7 +1748,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 	if (phy > RTL8366RB_PHY_NO_MAX)
 		return -EINVAL;
 
-	mutex_lock(&priv->map_lock);
+	realtek_common_lock(priv);
 
 	ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
 			   RTL8366RB_PHY_CTRL_WRITE);
@@ -1764,7 +1765,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
 		goto out;
 
 out:
-	mutex_unlock(&priv->map_lock);
+	realtek_common_unlock(priv);
 
 	return ret;
 }
-- 
2.42.1

Re: [net-next 1/2] net: dsa: realtek: create realtek-common

[Vladimir Oltean]

On Fri, Nov 17, 2023 at 08:50:00PM -0300, Luiz Angelo Daros de Luca wrote:
> +struct realtek_priv *realtek_common_probe(struct device *dev,
> +		struct regmap_config rc, struct regmap_config rc_nolock)

nitpick: A more conventional way of aligning the arguments would be:

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->variant = var;
> +	priv->ops = var->ops;
> +	priv->chip_data = (void *)priv + sizeof(*priv);
> +
> +	dev_set_drvdata(dev, priv);
> +	spin_lock_init(&priv->lock);
> +
> +	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);

realtek_common_remove() should not undo things that realtek_common_probe()
never did.

Re: [net-next 1/2] net: dsa: realtek: create realtek-common

[Vladimir Oltean]

On Fri, Nov 17, 2023 at 08:50:00PM -0300, Luiz Angelo Daros de Luca wrote:
> +void realtek_common_remove(struct realtek_priv *priv)
> +{
> +	if (!priv)
> +		return;

I'd leave this check out of realtek_common_remove(). You also have it in
the smi/mdio callers.

> +
> +	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);

[net-next 2/2] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

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

The variant modules use the same compatible string as the module alias.
This way, an interface module can use the matching compatible string to
both load the module and get the variant reference.

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

diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
index 1b733ac56560..cdd8d77c20d9 100644
--- a/drivers/net/dsa/realtek/realtek-common.c
+++ b/drivers/net/dsa/realtek/realtek-common.c
@@ -1,10 +1,72 @@
 // 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 *variant_entry)
+{
+	mutex_lock(&realtek_variants_lock);
+	list_add_tail(&variant_entry->list, &realtek_variants_list);
+	mutex_unlock(&realtek_variants_lock);
+}
+EXPORT_SYMBOL_GPL(realtek_variant_register);
+
+void realtek_variant_unregister(struct realtek_variant_entry *variant_entry)
+{
+	mutex_lock(&realtek_variants_lock);
+	list_del(&variant_entry->list);
+	mutex_unlock(&realtek_variants_lock);
+}
+EXPORT_SYMBOL_GPL(realtek_variant_unregister);
+
+const struct realtek_variant *realtek_variant_get(const char *compatible)
+{
+	const struct realtek_variant *variant = ERR_PTR(-ENOENT);
+	struct realtek_variant_entry *variant_entry;
+
+	request_module(compatible);
+
+	mutex_lock(&realtek_variants_lock);
+	list_for_each_entry(variant_entry, &realtek_variants_list, list) {
+		if (strcmp(compatible, variant_entry->compatible))
+			continue;
+
+		if (!try_module_get(variant_entry->owner))
+			break;
+
+		variant = variant_entry->variant;
+		break;
+	}
+	mutex_unlock(&realtek_variants_lock);
+
+	return variant;
+}
+EXPORT_SYMBOL_GPL(realtek_variant_get);
+
+void realtek_variant_put(const struct realtek_variant *var)
+{
+	struct realtek_variant_entry *variant_entry;
+
+	mutex_lock(&realtek_variants_lock);
+	list_for_each_entry(variant_entry, &realtek_variants_list, list) {
+		if (variant_entry->variant != var)
+			continue;
+
+		if (variant_entry->owner)
+			module_put(variant_entry->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 +87,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)
 		return ERR_PTR(-EINVAL);
 
+	var = realtek_variant_get(match->compatible);
+	if (IS_ERR(var)) {
+		ret = PTR_ERR(var);
+		dev_err_probe(dev, ret,
+			      "failed to get module for alias '%s'",
+			      match->compatible);
+		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 +119,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,11 +143,11 @@ 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 = 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);
@@ -85,13 +159,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);
 
@@ -104,6 +185,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);
@@ -112,10 +195,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", },
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
-	{ .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, },
+	{ .compatible = "realtek,rtl8365mb", },
 #endif
 	{ /* sentinel */ },
 };
diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
index 90a949386518..6de4991d8b5c 100644
--- a/drivers/net/dsa/realtek/realtek-common.h
+++ b/drivers/net/dsa/realtek/realtek-common.h
@@ -5,6 +5,37 @@
 
 #include <linux/regmap.h>
 
+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);				\
+									\
+MODULE_ALIAS(__compatible)
+
+void realtek_variant_register(struct realtek_variant_entry *variant_entry);
+void realtek_variant_unregister(struct realtek_variant_entry *variant_entry);
+
 extern const struct of_device_id realtek_common_of_match[];
 
 void realtek_common_lock(void *ctx);
@@ -12,5 +43,7 @@ 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 char *compatible);
+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 b865e11955ca..c447dd815a59 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -145,7 +145,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->ops = priv->variant->ds_ops_mdio;
@@ -154,10 +154,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 	ret = dsa_register_switch(priv->ds);
 	if (ret) {
 		dev_err_probe(dev, ret, "unable to register switch\n");
-		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 2aebcbe0425f..e50b3c6203e6 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -408,12 +408,16 @@ static int realtek_smi_probe(struct platform_device *pdev)
 
 	/* Fetch MDIO pins */
 	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
-	if (IS_ERR(priv->mdc))
-		return PTR_ERR(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 PTR_ERR(priv->mdio);
+	if (IS_ERR(priv->mdio)) {
+		ret = PTR_ERR(priv->mdc);
+		goto err_variant_put;
+	}
 
 	priv->setup_interface = realtek_smi_setup_mdio;
 	priv->write_reg_noack = realtek_smi_write_reg_noack;
@@ -421,7 +425,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->ops = priv->variant->ds_ops_smi;
@@ -430,10 +434,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	ret = dsa_register_switch(priv->ds);
 	if (ret) {
 		dev_err_probe(dev, ret, "unable to register switch\n");
-		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 fbbdf538908e..267a1dc02080 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -143,7 +143,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 9b18774e988c..fb214dd717f0 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -2164,7 +2164,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,
@@ -2173,7 +2173,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 1ac2fd098242..143c57c69ace 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1912,7 +1912,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,
@@ -1921,7 +1921,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

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Vladimir Oltean]

On Fri, Nov 17, 2023 at 08:50:01PM -0300, Luiz Angelo Daros de Luca wrote:
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index b865e11955ca..c447dd815a59 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -145,7 +145,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->ops = priv->variant->ds_ops_mdio;
> @@ -154,10 +154,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  	ret = dsa_register_switch(priv->ds);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "unable to register switch\n");
> -		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)

This is not so great at all from an API presentation point of view - the
fact that the caller needs to know that realtek_variant_put() undoes
the effect of realtek_common_probe().

You said you don't like too many abstractions, and fair enough, but
maybe we could have
- realtek_common_probe_pre(), realtek_common_remove_pre()
- realtek_common_probe_post(), realtek_common_remove_post()

which leads to even more code sharing from probe(), as well as an
opportunity to have a clearly matched unwind function for everything
that is done in probe()?

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Krzysztof Kozlowski]

On 18/11/2023 00:50, Luiz Angelo Daros de Luca wrote:
> realtek-common had a hard dependency on both switch variants. As a
> result, it was not possible to selectively load only one model at
> runtime. Now, variants are registered in the realtek-common module, and
> interface modules look for a variant using the compatible string.

...

> diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> index 90a949386518..6de4991d8b5c 100644
> --- a/drivers/net/dsa/realtek/realtek-common.h
> +++ b/drivers/net/dsa/realtek/realtek-common.h
> @@ -5,6 +5,37 @@
>  
>  #include <linux/regmap.h>
>  
> +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);				\
> +									\
> +MODULE_ALIAS(__compatible)

No, why do you need it? You should not need MODULE_ALIAS() in normal
cases. If you need it, usually it means your device ID table is wrong
(e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is
not a substitute for incomplete ID table.

Entire abstraction/macro is pointless and make the code less readable.


Best regards,
Krzysztof

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Vladimir Oltean]

Hi Krzysztof,

On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote:
> No, why do you need it? You should not need MODULE_ALIAS() in normal
> cases. If you need it, usually it means your device ID table is wrong
> (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is
> not a substitute for incomplete ID table.
> 
> Entire abstraction/macro is pointless and make the code less readable.

Are you saying that the line

MODULE_DEVICE_TABLE(of, realtek_common_of_match);

should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and
rtl8366rb.c, but not in realtek-common.c?

There are 5 kernel modules involved, 2 for interfaces and 2 for switches.

Even if the same OF device ID table could be used to load multiple
modules, I'm not sure
(a) how to avoid loading the interface driver which will not be used
    (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI
    connected switch)
(b) how to ensure that the drivers are loaded in the right order, i.e.
    the switch drivers are loaded before the interface drivers

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Krzysztof Kozlowski]

On 20/11/2023 14:48, Vladimir Oltean wrote:
> Hi Krzysztof,
> 
> On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote:
>> No, why do you need it? You should not need MODULE_ALIAS() in normal
>> cases. If you need it, usually it means your device ID table is wrong
>> (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is
>> not a substitute for incomplete ID table.
>>
>> Entire abstraction/macro is pointless and make the code less readable.
> 
> Are you saying that the line
> 
> MODULE_DEVICE_TABLE(of, realtek_common_of_match);
> 
> should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and
> rtl8366rb.c, but not in realtek-common.c?

Driver should use MODULE_DEVICE_TABLE() for the table not MODULE_ALIAS()
for each entry. I don't judge where should it be put. I just dislike
usage of aliases as a incomplete-substitute of proper table.

> 
> There are 5 kernel modules involved, 2 for interfaces and 2 for switches.
> 
> Even if the same OF device ID table could be used to load multiple
> modules, I'm not sure
> (a) how to avoid loading the interface driver which will not be used
>     (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI
>     connected switch)
> (b) how to ensure that the drivers are loaded in the right order, i.e.
>     the switch drivers are loaded before the interface drivers

I am sorry, I do not understand the problem. The MODULE_DEVICE_TABLE and
MODULE_ALIAS create exactly the same behavior. How any of above would
happen with table but not with alias having exactly the same compatibles?

Best regards,
Krzysztof

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

Hi Krzysztof,

> > On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote:
> >> No, why do you need it? You should not need MODULE_ALIAS() in normal
> >> cases. If you need it, usually it means your device ID table is wrong
> >> (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is
> >> not a substitute for incomplete ID table.
> >>
> >> Entire abstraction/macro is pointless and make the code less readable.
> >
> > Are you saying that the line
> >
> > MODULE_DEVICE_TABLE(of, realtek_common_of_match);
> >
> > should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and
> > rtl8366rb.c, but not in realtek-common.c?
>
> Driver should use MODULE_DEVICE_TABLE() for the table not MODULE_ALIAS()
> for each entry. I don't judge where should it be put. I just dislike
> usage of aliases as a incomplete-substitute of proper table.

MODULE_ALIAS() is in use here because of its relation with modprobe,
not inside the kernel.
MODULE_DEVICE_TABLE is also in use but it does not seem to generate
any information usable by modprobe.

> >
> > There are 5 kernel modules involved, 2 for interfaces and 2 for switches.
> >
> > Even if the same OF device ID table could be used to load multiple
> > modules, I'm not sure
> > (a) how to avoid loading the interface driver which will not be used
> >     (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI
> >     connected switch)
> > (b) how to ensure that the drivers are loaded in the right order, i.e.
> >     the switch drivers are loaded before the interface drivers
>
> I am sorry, I do not understand the problem. The MODULE_DEVICE_TABLE and
> MODULE_ALIAS create exactly the same behavior. How any of above would
> happen with table but not with alias having exactly the same compatibles?

Realtek switches can be managed through different interfaces. In the
current kernel implementation, there is an MDIO driver (realtek-mdio)
for switches connected to the MDIO bus, and a platform driver
implementing the SMI protocol (a simple GPIO bit-bang).

The actual switch logic is implemented in two different switch
family/variant modules: rtl8365mb and rtl8366 (currently only for
rtl8366rb). As of today, both Realtek interface modules directly
reference each switch variant symbol, creating a hard dependency and
forcing the interface module to load both switch family variants. Each
interface module provides the same compatible strings for both
variants, but I haven't investigated whether this is problematic or
not. It appears that there is no mechanism to autoload modules based
on compatible strings from the device tree, and each interface module
is a different type of driver.

This patch set accomplishes two things: it moves some shared code to a
new Realtek common module and changes the hard dependency between
interface and variant modules into a more dynamic relation. Each
variant module registers itself in realtek-common, and interface
modules look for the appropriate variant. However, as interface
modules do not directly depend on variant modules, they might not have
been loaded yet, causing the driver probe to fail.

The solution I opted for was to request the module during the
interface probe (similar to what happens with DSA tag modules),
triggering a userland "modprobe XXXX." Even without the variant module
loaded, we know the compatible string that matched the interface
driver. We can also have some extra info from match data, but I chose
to simply keep using the compatible string. The issue is how to get
the "XXXX" for modprobe. For DSA tags, module names are generated
according to a fixed rule based on the tag name. However, the switch
variants do not have a fixed relation between module name and switch
families (actually, there is not even a switch family name). I could
(and did in a previous RFC) use the match data to inform each module
name. However, it adds a non-obvious relation between the module name
(defined in Makefile) and a string in code. I was starting to
implement a lookup table to match compatible strings to their module
names when I realized that I was just mapping a string to a module
name, something like what module alias already does. That's when the
MODULE_ALIAS("<the compatible string>") was introduced.

After this discussion, I have some questions:

I'm declaring the of_device_id match table in realtek-common as it is
the same for both interfaces. I also moved MODULE_DEVICE_TABLE(of,
realtek_common_of_match) to realtek-common. Should I keep the
MODULE_DEVICE_TABLE() on each interface module (referencing the same
table), or is it okay to keep it in the realtek-common module? If I
need to move it to each interface module, can I reuse a shared
of_device_id match table declared in realtek-common?

If MODULE_ALIAS is not an acceptable solution, what would be the right
one? Go back to the static mapping between the compatible string and
the module name or is there a better solution?

Regards,

Luiz

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Krzysztof Kozlowski]

On 21/11/2023 15:40, Luiz Angelo Daros de Luca wrote:
> Hi Krzysztof,
> 
>>> On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote:
>>>> No, why do you need it? You should not need MODULE_ALIAS() in normal
>>>> cases. If you need it, usually it means your device ID table is wrong
>>>> (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is
>>>> not a substitute for incomplete ID table.
>>>>
>>>> Entire abstraction/macro is pointless and make the code less readable.
>>>
>>> Are you saying that the line
>>>
>>> MODULE_DEVICE_TABLE(of, realtek_common_of_match);
>>>
>>> should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and
>>> rtl8366rb.c, but not in realtek-common.c?
>>
>> Driver should use MODULE_DEVICE_TABLE() for the table not MODULE_ALIAS()
>> for each entry. I don't judge where should it be put. I just dislike
>> usage of aliases as a incomplete-substitute of proper table.
> 
> MODULE_ALIAS() is in use here because of its relation with modprobe,
> not inside the kernel.

The same as MODULE_DEVICE_TABLE.

> MODULE_DEVICE_TABLE is also in use but it does not seem to generate
> any information usable by modprobe.

It does, exactly the same from functional point of view... which
information are you missing?

> 
>>>
>>> There are 5 kernel modules involved, 2 for interfaces and 2 for switches.
>>>
>>> Even if the same OF device ID table could be used to load multiple
>>> modules, I'm not sure
>>> (a) how to avoid loading the interface driver which will not be used
>>>     (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI
>>>     connected switch)
>>> (b) how to ensure that the drivers are loaded in the right order, i.e.
>>>     the switch drivers are loaded before the interface drivers
>>
>> I am sorry, I do not understand the problem. The MODULE_DEVICE_TABLE and
>> MODULE_ALIAS create exactly the same behavior. How any of above would
>> happen with table but not with alias having exactly the same compatibles?
> 
> Realtek switches can be managed through different interfaces. In the
> current kernel implementation, there is an MDIO driver (realtek-mdio)
> for switches connected to the MDIO bus, and a platform driver
> implementing the SMI protocol (a simple GPIO bit-bang).
> 
> The actual switch logic is implemented in two different switch
> family/variant modules: rtl8365mb and rtl8366 (currently only for
> rtl8366rb). As of today, both Realtek interface modules directly
> reference each switch variant symbol, creating a hard dependency and
> forcing the interface module to load both switch family variants. Each
> interface module provides the same compatible strings for both
> variants, but I haven't investigated whether this is problematic or
> not. It appears that there is no mechanism to autoload modules based
> on compatible strings from the device tree, and each interface module
> is a different type of driver.

??? So entire Linux kernel is broken? Somehow autoloading modules based
on compatible strings from the device tree works for every other driver
properly using tables... So again, I am repeating:
stop using MODULE_ALIAS for missing/incomplete tables

> 
> This patch set accomplishes two things: it moves some shared code to a

Which should not... One thing per patch.


> new Realtek common module and changes the hard dependency between
> interface and variant modules into a more dynamic relation. Each
> variant module registers itself in realtek-common, and interface
> modules look for the appropriate variant. However, as interface
> modules do not directly depend on variant modules, they might not have
> been loaded yet, causing the driver probe to fail.
> 
> The solution I opted for was to request the module during the
> interface probe (similar to what happens with DSA tag modules),
> triggering a userland "modprobe XXXX." Even without the variant module
> loaded, we know the compatible string that matched the interface
> driver. We can also have some extra info from match data, but I chose
> to simply keep using the compatible string. The issue is how to get

How is this even related to the problem? Please respond with concise
messages.

> the "XXXX" for modprobe. For DSA tags, module names are generated
> according to a fixed rule based on the tag name. However, the switch
> variants do not have a fixed relation between module name and switch
> families (actually, there is not even a switch family name). I could
> (and did in a previous RFC) use the match data to inform each module
> name. However, it adds a non-obvious relation between the module name
> (defined in Makefile) and a string in code. I was starting to
> implement a lookup table to match compatible strings to their module
> names when I realized that I was just mapping a string to a module
> name, something like what module alias already does. That's when the
> MODULE_ALIAS("<the compatible string>") was introduced.
> 
> After this discussion, I have some questions:
> 
> I'm declaring the of_device_id match table in realtek-common as it is
> the same for both interfaces. I also moved MODULE_DEVICE_TABLE(of,
> realtek_common_of_match) to realtek-common. Should I keep the
> MODULE_DEVICE_TABLE() on each interface module (referencing the same
> table), or is it okay to keep it in the realtek-common module? If I

Why would you have the same compatible entries in different modules? You
do understand that device node will become populated on first bind (bind
of the first device)?

> need to move it to each interface module, can I reuse a shared
> of_device_id match table declared in realtek-common?
> 
> If MODULE_ALIAS is not an acceptable solution, what would be the right
> one? Go back to the static mapping between the compatible string and
> the module name or is there a better solution?


Best regards,
Krzysztof

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Krzysztof Kozlowski]

On 21/11/2023 23:15, Krzysztof Kozlowski wrote:
> How is this even related to the problem? Please respond with concise
> messages.
> 
>> the "XXXX" for modprobe. For DSA tags, module names are generated
>> according to a fixed rule based on the tag name. However, the switch
>> variants do not have a fixed relation between module name and switch
>> families (actually, there is not even a switch family name). I could
>> (and did in a previous RFC) use the match data to inform each module
>> name. However, it adds a non-obvious relation between the module name
>> (defined in Makefile) and a string in code. I was starting to
>> implement a lookup table to match compatible strings to their module
>> names when I realized that I was just mapping a string to a module
>> name, something like what module alias already does. That's when the
>> MODULE_ALIAS("<the compatible string>") was introduced.
>>
>> After this discussion, I have some questions:
>>
>> I'm declaring the of_device_id match table in realtek-common as it is
>> the same for both interfaces. I also moved MODULE_DEVICE_TABLE(of,
>> realtek_common_of_match) to realtek-common. Should I keep the
>> MODULE_DEVICE_TABLE() on each interface module (referencing the same
>> table), or is it okay to keep it in the realtek-common module? If I
> 
> Why would you have the same compatible entries in different modules? You
> do understand that device node will become populated on first bind (bind
> of the first device)?
> 
>> need to move it to each interface module, can I reuse a shared
>> of_device_id match table declared in realtek-common?
>>
>> If MODULE_ALIAS is not an acceptable solution, what would be the right
>> one? Go back to the static mapping between the compatible string and
>> the module name or is there a better solution?

Probably the solution is to make the design the same as for all other
complex drivers supporting more than one bus. If your ID table is
defined in modules A and B, then their loading should not depend on
aliases put in some additional "common" module. We solved this many
times for devices residing on multiple buses (e.g. I2C and SPI), so why
this has to be done in reverse order?

If you ask what would be the acceptable solution, then my answer is: do
the same as for most of other drivers, do not reinvent stuff like
putting same ID table or module alias in two modules. The table is
defined only once in each driver being loaded on uevent. From that
driver you probe whatever device you have, including calling any common
code, subprobes, subvariants etc.

Best regards,
Krzysztof

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

> On 21/11/2023 23:15, Krzysztof Kozlowski wrote:
> > How is this even related to the problem? Please respond with concise
> > messages.
> >
> >> the "XXXX" for modprobe. For DSA tags, module names are generated
> >> according to a fixed rule based on the tag name. However, the switch
> >> variants do not have a fixed relation between module name and switch
> >> families (actually, there is not even a switch family name). I could
> >> (and did in a previous RFC) use the match data to inform each module
> >> name. However, it adds a non-obvious relation between the module name
> >> (defined in Makefile) and a string in code. I was starting to
> >> implement a lookup table to match compatible strings to their module
> >> names when I realized that I was just mapping a string to a module
> >> name, something like what module alias already does. That's when the
> >> MODULE_ALIAS("<the compatible string>") was introduced.
> >>
> >> After this discussion, I have some questions:
> >>
> >> I'm declaring the of_device_id match table in realtek-common as it is
> >> the same for both interfaces. I also moved MODULE_DEVICE_TABLE(of,
> >> realtek_common_of_match) to realtek-common. Should I keep the
> >> MODULE_DEVICE_TABLE() on each interface module (referencing the same
> >> table), or is it okay to keep it in the realtek-common module? If I
> >
> > Why would you have the same compatible entries in different modules? You
> > do understand that device node will become populated on first bind (bind
> > of the first device)?
> >
> >> need to move it to each interface module, can I reuse a shared
> >> of_device_id match table declared in realtek-common?
> >>
> >> If MODULE_ALIAS is not an acceptable solution, what would be the right
> >> one? Go back to the static mapping between the compatible string and
> >> the module name or is there a better solution?
>
> Probably the solution is to make the design the same as for all other
> complex drivers supporting more than one bus. If your ID table is
> defined in modules A and B, then their loading should not depend on
> aliases put in some additional "common" module. We solved this many
> times for devices residing on multiple buses (e.g. I2C and SPI), so why
> this has to be done in reverse order?
>
> If you ask what would be the acceptable solution, then my answer is: do
> the same as for most of other drivers, do not reinvent stuff like
> putting same ID table or module alias in two modules. The table is
> defined only once in each driver being loaded on uevent. From that
> driver you probe whatever device you have, including calling any common
> code, subprobes, subvariants etc.

Thank you, Krzysztof. I didn't design the driver in question; I was
just trying to cooperate.

Linus, Alvin, these are originally your work. I might have
misunderstood something and would appreciate some help in this
discussion.

I noticed that drivers like bmp280, inv_mpu6050, and adxl345 share a
common core with two interface drivers, both declaring the same
compatible strings. Is it an issue to have the same compatible string
in different modules? Is there a better reference to follow?

In realtek DSA drivers, the "interface" modules (realtek-smi and
realtek-mdio) are the actual drivers, containing compatible strings.
The subdriver/variant/family modules are just the switch logic with no
driver declaration. Currently, all subdrivers are loaded, which might
not be necessary as a device may not use two different realtek switch
families. Ideally, only one should be loaded. Can we request a module
at runtime, and is it acceptable to use MODULE_ALIAS() for a
"non-driver module" to make that easier?

The driver works without the module alias/request if it was loaded
before the interface driver. I aim to make it work in more scenarios.
These drivers are for embedded systems, so handling uevents might not
be feasible. I used the compatible string to avoid creating a new
string for the family name, but I could use anything else like
"rtl8366rb" only when the module name does not match the model used by
the compatible string. I didn't expect it to be used by anything
monitoring uevents.

For the driver structure, we have a few options:

1) Merge everything into a single realtek-switch, declaring the
compatible strings once and implementing it as both a platform and an
mdio driver.
2) Have a module for each interface (realtek-smi and realtek-mdio),
both repeating the compatible strings for all families, and load the
family as an interface dependency (the current approach).
3) Introduce a realtek-common module, a module for each interface,
both repeating the compatible strings for all families, and load the
family as an interface dependency (the first patch in this series).
4) Introduce a realtek-common module, a module for each interface,
both repeating the compatible strings for all families, and expect the
family module to be already loaded.
5) Introduce a realtek-common module, a module for each interface,
both repeating the compatible strings for all families, and request
the family module to be loaded (the last patch in this series).
6) Introduce a realtek-common module, merging everything from
realtek-smi and realtek-mdio but the driver declarations and implement
each family as a single module that works both as a platform and an
mdio driver.
7) Introduce a realtek-common module, merging everything from
realtek-smi and realtek-mdio but the driver declarations and create
two modules for each family as real drivers, repeating the compatible
strings on each family (e.g., {rtl8366rb, rtl8365mb}_{smi,mdio}.ko).

Solutions 1, 2, and 3 may use more resources than needed. My test
device, for example, cannot handle them. Solution 7 is similar to the
examples I found in the kernel. Solutions 1 and 6 are the only ones
not repeating the same compatible strings in different modules, if
that's a problem.

Regards,

Luiz

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Alvin Šipraga]

On Wed, Nov 22, 2023 at 07:44:44PM -0300, Luiz Angelo Daros de Luca wrote:
> For the driver structure, we have a few options:
> 
> 1) Merge everything into a single realtek-switch, declaring the
> compatible strings once and implementing it as both a platform and an
> mdio driver.
> 2) Have a module for each interface (realtek-smi and realtek-mdio),
> both repeating the compatible strings for all families, and load the
> family as an interface dependency (the current approach).
> 3) Introduce a realtek-common module, a module for each interface,
> both repeating the compatible strings for all families, and load the
> family as an interface dependency (the first patch in this series).

As you say below, this is too much for your test device. Skip.

> 4) Introduce a realtek-common module, a module for each interface,
> both repeating the compatible strings for all families, and expect the
> family module to be already loaded.

The kernel should be able to autoload. Skip.

> 5) Introduce a realtek-common module, a module for each interface,
> both repeating the compatible strings for all families, and request
> the family module to be loaded (the last patch in this series).

I had reservations before about this approach and I think I am not
the only one. Let's consider the other options.

> 6) Introduce a realtek-common module, merging everything from
> realtek-smi and realtek-mdio but the driver declarations and implement
> each family as a single module that works both as a platform and an
> mdio driver.
> 7) Introduce a realtek-common module, merging everything from
> realtek-smi and realtek-mdio but the driver declarations and create
> two modules for each family as real drivers, repeating the compatible
> strings on each family (e.g., {rtl8366rb, rtl8365mb}_{smi,mdio}.ko).

Yeah, something like this. Roughly I think we want:

- generic boilerplate probe/remove and regmap setup code in
  realtek-common.c
- interface code in realtek-{smi,mdio}.c
- chip variant code in rtl8365mb.c and rtl8366rb.c

The module dependencies only need to go upwards, i.e.:

       realtek-common
         ^       ^
         |       |  use symbols realtek_common_probe()
         |       |              realtek_common_remove()
         |       |
 realtek-smi   realtek-mdio 
      ^   ^     ^   ^
      |   |     |   |  use symbols realtek_mdio_probe()
      |    \   /    |              realtek_mdio_remove()
      |     \ /     |
      |      X      |  or symbols realtek_smi_probe()
      |     / \     |             realtek_smi_remove()
      |    /   \    |
      |   /     \   |  or both
      |  /       \  |
  rtl8365mb     rtl8366rb

We already see what realtek_common_probe() etc. look like in your patch
series here. If an interface driver needs to do something specific and
it can't do that before or after calling realtek_common_probe(), then it
can be abstracted away into some realtek_common_info struct and the
interface driver can populate this and pass it along.

The interface driver also ought to provide its own probe function that
the chip variant driver can call. Again, if there is something specific
that needs doing in the middle of this process it can call into some
realtek_{smi,mdio}_info function pointer.

I made similar points in my review of your last series so you can look
there for specific instances where I think this is valuable.

After that, the chip variant driver can do some #if ENABLED() logic to
selectively register itself as either a platform driver (SMI), MDIO
driver (MDIO), or both. The MODULE_DEVICE_TABLE is just used by the bus
driver in question (platform or MDIO) to match with the appropriate
driver. Compatible strings are the same, but it's the same chip after
all. In fact, I don't think you even need two MODULE_DEVICE_TABLEs. You
can pass the same one to both the mdio_driver and platform_driver
structs.

I think one needs to look critically at the data being passed around
between the constituent modules at the moment. Some observations:

- for MDIO, the only thing the chip variant driver needs to give it is a
  detect function and its dsa_switch_ops, so why not:

    struct realtek_mdio_info {
      int (*detect)(struct realtek_priv *priv);
      const struct dsa_switch_ops *ds_ops;
    }

- for SMI, some more info is needed:

    struct realtek_smi_info {
      int (*detect)(struct realtek_priv *priv);
      int (*phy_read)(struct realtek_priv *priv, int phy, int regnum);
      int (*phy_write)(struct realtek_priv *priv, int phy, int regnum,
                       u16 val);
      const struct dsa_switch_ops *ds_ops;
    }

  Yes, there is a lot more in struct realtek_ops, but all of that is
  used by rtl8366-core.c. I think this is trivial to disentangle but it
  might result in a fairly large diff if one tries to remove this from
  struct realtek_ops. My point being you don't need to actually populate
  this for the interface driver itself.

With that we can define reasonable signatures for the interface
drivers' probe functions:

  int realtek_smi_probe(struct platform_device *pdev,
                        const struct realtek_smi_info *info);
  int realtek_mdio_probe(struct mdio_device *mdiodev,
                         const struct realtek_mdio_info *info);
  EXPORT_SYMBOL_GPL(both of these functions);

The remove functions can just take the pdev or mdiodev pointers to keep
things balanced.

Now the interface drivers can populate realtek_priv sufficiently for the
realtek_common_probe() to do its job. 

  struct realtek_interface_info {
      static const struct regmap_config *regmap_config;
      static const struct regmap_config *regmap_config_nolock;
      int (*detect)(struct realtek_priv *priv);
      int (*setup_interface)(struct dsa_switch *ds);
      const struct dsa_switch_ops *ds_ops;
  };

The regmap configs are self explanatory.

The setup_interface function pointer is only needed for SMI, and
currently the chip interface drivers are the ones calling it. It is kind
of ugly that this gets passed all the way up, copied into realtek_priv,
and then called all the way down again... there is probably a more
elegant solution. I am mainly trying to illustrate how to handle the
module knot you are trying to unpick.

Now I recall that Vladimir suggested something more along the lines of
realtek_common_probe_pre() and post, and ditto for remove(). You might
prefer that approach. Something to experiment with. You might need
something like that because there are those GPIOs that SMI wants to
fiddle with, and they need to be ready before calling
dsa_register_switch(), and the priv datastructure is not allocated
before calling the common probe function.

Anyway, with the above suggestion I think one can move the vast majority
of common code into realtek-common while keeping the chip variant
drivers mostly unmodified. All that's needed is to move the
MODULE_DEVICE_TABLES into those files and create custom module init/exit
functions which conditionally register themselves as platform and mdio
drivers. This is what the module_platform_driver() and
mdio_module_driver() macros do - they just make the boilerplate
init/exit for you. But nothing stops you from registering both a struct
platform_driver and a struct mdio_driver. Just guard everything with
#ifs to ensure clean compilation.

The relevant probe functions will look like this (rtl8365mb example):

  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI)
  static const struct realtek_smi_info rtl8365mb_smi_info = {
    .detect = rtl8365mb_detect,
    .phy_read = rtl8365mb_phy_read,
    .phy_write = rtl8365mb_phy_write,
    .ds_ops = rtl8365mb_switch_ops,
  };
  
  int rtl8365mb_smi_probe(struct platform_device *pdev) {
    return realtek_smi_probe(pdev, &rtl8365mb_smi_info);
  }
  #endif
  
  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO)
  static const struct realtek_mdio_info rtl8365mb_mdio_info = {
    .detect = rtl8365mb_detect,
    .ds_ops = rtl8365mb_switch_ops,
  };
  
  int rtl8365mb_mdio_probe(struct mdio_device *mdiodev) {
    return realtek_mdio_probe(mdiodev, &rtl8365mb_mdio_info);
  }
  #endif

One thing I dislike is that all the driver private data is getting
allocated way up in the common driver. It would be nicer if the chip
variant or interface drivers could also allocate their own stuff and
just put it in a priv pointer of the parent. Likewise I was never a fan
of the fact that the chip variant drivers always receiving realtek_priv
pointers rather than pointers to their driver private data. You can see
a lot of diving into chip_data especially in rtl8365mb for this
reason. I regret not addressing that before I added the new driver.

> 
> Solutions 1, 2, and 3 may use more resources than needed. My test
> device, for example, cannot handle them. Solution 7 is similar to the
> examples I found in the kernel. Solutions 1 and 6 are the only ones
> not repeating the same compatible strings in different modules, if
> that's a problem.

You will have noticed that with the above solution, the chip variant
modules will invariably require both interface modules to be loaded if
the kernel is built with both REALTEK_SMI and REALTEK_MDIO. I hope that
your resource-constrained system has enough headroom to accommodate
that. If not then I am afraid you might simply be out of luck.

Kind regards,
Alvin

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

Hi Alvin,

> > 4) Introduce a realtek-common module, a module for each interface,
> > both repeating the compatible strings for all families, and expect the
> > family module to be already loaded.
>
> The kernel should be able to autoload. Skip.
>
> > 5) Introduce a realtek-common module, a module for each interface,
> > both repeating the compatible strings for all families, and request
> > the family module to be loaded (the last patch in this series).
>
> I had reservations before about this approach and I think I am not
> the only one. Let's consider the other options.

I'm not sure if getting/putting a module is a problem or if I can
request it when missing. I would like some options on that specific
topic from the experts. It seems to happen in many places, even in DSA
tag code.

> > 6) Introduce a realtek-common module, merging everything from
> > realtek-smi and realtek-mdio but the driver declarations and implement
> > each family as a single module that works both as a platform and an
> > mdio driver.
> > 7) Introduce a realtek-common module, merging everything from
> > realtek-smi and realtek-mdio but the driver declarations and create
> > two modules for each family as real drivers, repeating the compatible
> > strings on each family (e.g., {rtl8366rb, rtl8365mb}_{smi,mdio}.ko).
>
> Yeah, something like this. Roughly I think we want:

Just to be sure, you are suggesting something like 6), not 7), right?

> - generic boilerplate probe/remove and regmap setup code in
>   realtek-common.c
> - interface code in realtek-{smi,mdio}.c
> - chip variant code in rtl8365mb.c and rtl8366rb.c
>
> The module dependencies only need to go upwards, i.e.:
>
>        realtek-common
>          ^       ^
>          |       |  use symbols realtek_common_probe()
>          |       |              realtek_common_remove()
>          |       |
>  realtek-smi   realtek-mdio
>       ^   ^     ^   ^
>       |   |     |   |  use symbols realtek_mdio_probe()
>       |    \   /    |              realtek_mdio_remove()
>       |     \ /     |
>       |      X      |  or symbols realtek_smi_probe()
>       |     / \     |             realtek_smi_remove()
>       |    /   \    |
>       |   /     \   |  or both
>       |  /       \  |
>   rtl8365mb     rtl8366rb

It makes sense to turn rtl8365mb/rtl8366rb into both a platform and an
MDIO driver, similar to the merge between realtek-mdio/realtek-smi
Vladmir suggested before, with a custom module_init/exit doing the
job. I didn't invest too much time thinking about how data structures
would fit in this new model. realtek_priv would probably be allocated
by the variant modules with little left for interface to probe/setup.

> The setup_interface function pointer is only needed for SMI, and
> currently the chip interface drivers are the ones calling it. It is kind
> of ugly that this gets passed all the way up, copied into realtek_priv,
> and then called all the way down again... there is probably a more
> elegant solution. I am mainly trying to illustrate how to handle the
> module knot you are trying to unpick.

The "realtek_smi_setup_mdio()" used in setup_interface isn't really
necessary (like it happens in realtek-mdio). It could be used (or not)
by both interfaces. The "realtek,smi-mdio" compatible string is
misleading, indicating it might be something specific to the SMI
interface HW while it is just how the driver was implemented. A
"realtek,slave_mdio" or "realtek,user_mii" would be better.

I believe the strange relations between realtek interface modules and
variants tend to go away if we put things in the right place. The
probe will happen mostly in common and variant modules, leaving just a
minor probe job for the interface module called from the variant
driver (using pre/post approach) or from common (using a
realtek_interface_ops). Anyway, I'll leave the discussion of who calls
who after we settle the role of each module.

The most likely proposal is to convert both variant modules from a
subdriver code into a both platform(for SMI) and mdio driver. Probe
will use both realtek_<interface>_probe() and realtek_common_probe()
when appropriated. Variants will call the interface and not the other
way around.

> > Solutions 1, 2, and 3 may use more resources than needed. My test
> > device, for example, cannot handle them. Solution 7 is similar to the
> > examples I found in the kernel. Solutions 1 and 6 are the only ones
> > not repeating the same compatible strings in different modules, if
> > that's a problem.
>
> You will have noticed that with the above solution, the chip variant
> modules will invariably require both interface modules to be loaded if
> the kernel is built with both REALTEK_SMI and REALTEK_MDIO. I hope that
> your resource-constrained system has enough headroom to accommodate
> that. If not then I am afraid you might simply be out of luck.

I wouldn't say it will invariably require both interface modules to be
loaded. The dynamic load would be much simpler if variants request the
interface module as we only have two (at most 3 with a future
realtek-spi) modules. We would just need to call a
realtek_interface_get() and realtek_interface_put() on each respective
probe. The module names will be well-known with no issues with
module_alias.

Thanks for your help, Alvin. I'll wait for a couple of more days for
others to manifest.

Regards,

Luiz

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Vladimir Oltean]

On Mon, Nov 27, 2023 at 07:24:16PM -0300, Luiz Angelo Daros de Luca wrote:
> I'm not sure if getting/putting a module is a problem or if I can
> request it when missing. I would like some options on that specific
> topic from the experts. It seems to happen in many places, even in DSA
> tag code.
> 
> I wouldn't say it will invariably require both interface modules to be
> loaded. The dynamic load would be much simpler if variants request the
> interface module as we only have two (at most 3 with a future
> realtek-spi) modules. We would just need to call a
> realtek_interface_get() and realtek_interface_put() on each respective
> probe. The module names will be well-known with no issues with
> module_alias.
> 
> Thanks for your help, Alvin. I'll wait for a couple of more days for
> others to manifest.

I'm not an expert on this topic either, but Alvin's suggestion makes
sense to have the switch variant drivers be both platform and MDIO
device drivers, and call symbols exported by the interface drivers as
needed.

If you are able to make the variant driver depend on just the interface
driver in use based on some request_module() calls, I don't think that
will be a problem with Krzysztof either, since he just said to not
duplicate the MODULE_DEVICE_TABLE() functionality.

I think it's down to prototyping something and seeing what are the pros
and cons.

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

Hi Vladimir,

> > I'm not sure if getting/putting a module is a problem or if I can
> > request it when missing. I would like some options on that specific
> > topic from the experts. It seems to happen in many places, even in DSA
> > tag code.
> >
> > I wouldn't say it will invariably require both interface modules to be
> > loaded. The dynamic load would be much simpler if variants request the
> > interface module as we only have two (at most 3 with a future
> > realtek-spi) modules. We would just need to call a
> > realtek_interface_get() and realtek_interface_put() on each respective
> > probe. The module names will be well-known with no issues with
> > module_alias.
> >
> > Thanks for your help, Alvin. I'll wait for a couple of more days for
> > others to manifest.
>
> I'm not an expert on this topic either, but Alvin's suggestion makes
> sense to have the switch variant drivers be both platform and MDIO
> device drivers, and call symbols exported by the interface drivers as
> needed.

Yes, it does. It looks like the driver was upside down.

> If you are able to make the variant driver depend on just the interface
> driver in use based on some request_module() calls, I don't think that
> will be a problem with Krzysztof either, since he just said to not
> duplicate the MODULE_DEVICE_TABLE() functionality.

The interface modules are quite small, multiple times smaller than the
variant module. It wasn't worth it to load them on demand as the code
to handle that might be close to the interface module size. Indeed, as
we'll have a common module, I think the best solution would be to
merge both interfaces into the common module. It would make things
much simpler: two variant/families modules that require a single
common module. It is also closer to what we see in other DSA drivers.

> I think it's down to prototyping something and seeing what are the pros
> and cons.

I already did that and I'm finishing some tests before submitting it.
It looks like it fits nicely. I avoided some struct refactoring Alvim
suggested to keep the change as small as possible but I went a little
further migrating the user mdio driver to common and use it for both
interfaces.

Regards,

Luiz

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Vladimir Oltean]

On Mon, Nov 27, 2023 at 07:24:16PM -0300, Luiz Angelo Daros de Luca wrote:
> The "realtek_smi_setup_mdio()" used in setup_interface isn't really
> necessary (like it happens in realtek-mdio). It could be used (or not)
> by both interfaces. The "realtek,smi-mdio" compatible string is
> misleading, indicating it might be something specific to the SMI
> interface HW while it is just how the driver was implemented. A
> "realtek,slave_mdio" or "realtek,user_mii" would be better.

The compatible string is about picking a driver for a device. It is
supposed to uniquely describe the register layout and functionality of
that IP block, not its functional role in the kernel. "slave_mdio" and
"user_mii" are too ingrained with "this MDIO controller gives access to
internal PHY ports of DSA slave ports".

Even if the MDIO controller doesn't currently have its own struct device
and driver, you'd have to think of the fact that it could, when picking
an appropriate compatible string.

If you have very specific information that the MDIO controller is based on
some reusable/licensable IP block and there were no modifications made
to it, you could use that compatible string.

Otherwise, another sensible choice would be "realtek,<precise-soc-name>-mdio",
because it leaves room for future extensions with other compatible
strings, more generic or just as specific, that all bind to the same
driver.

It's always good to start being too specific rather than too generic,
because a future Realtek switch might have a different IP block for its
MDIO controller. Then a driver for your existing "realtek,smi-mdio" or
"realtek,slave_mdio" or "realtek,user_mii" compatible string sounds like
it could handle it, but it can't.

You can always add secondary compatible strings to a node, but changing
the existing one breaks the ABI between the kernel and the DT.

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

> On Mon, Nov 27, 2023 at 07:24:16PM -0300, Luiz Angelo Daros de Luca wrote:
> > The "realtek_smi_setup_mdio()" used in setup_interface isn't really
> > necessary (like it happens in realtek-mdio). It could be used (or not)
> > by both interfaces. The "realtek,smi-mdio" compatible string is
> > misleading, indicating it might be something specific to the SMI
> > interface HW while it is just how the driver was implemented. A
> > "realtek,slave_mdio" or "realtek,user_mii" would be better.
>
> The compatible string is about picking a driver for a device. It is
> supposed to uniquely describe the register layout and functionality of
> that IP block, not its functional role in the kernel. "slave_mdio" and
> "user_mii" are too ingrained with "this MDIO controller gives access to
> internal PHY ports of DSA slave ports".
>
> Even if the MDIO controller doesn't currently have its own struct device
> and driver, you'd have to think of the fact that it could, when picking
> an appropriate compatible string.
>
> If you have very specific information that the MDIO controller is based on
> some reusable/licensable IP block and there were no modifications made
> to it, you could use that compatible string.
>
> Otherwise, another sensible choice would be "realtek,<precise-soc-name>-mdio",
> because it leaves room for future extensions with other compatible
> strings, more generic or just as specific, that all bind to the same
> driver.
>
> It's always good to start being too specific rather than too generic,
> because a future Realtek switch might have a different IP block for its
> MDIO controller. Then a driver for your existing "realtek,smi-mdio" or
> "realtek,slave_mdio" or "realtek,user_mii" compatible string sounds like
> it could handle it, but it can't.
>
> You can always add secondary compatible strings to a node, but changing
> the existing one breaks the ABI between the kernel and the DT.

HI Vladmir,

We discussed something about that in the past:

https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/#m04e6cf7d0c1f18b02c2bf40266ada915b1f02f3d

The code is able to handle only a single node and binding docs say it
should be named "mdio". The compatible string wasn't a requirement
since the beginning and I don't think it is worth it to rename the
compatible string. I suggest we simply switch to
of_get_child_by_name() and look for a node named "mdio". If that node
is not found, we can still look for the old compatible string
(backwards compatibility) and probably warn the "user" (targeting not
the end-user but the one creating the DT for a new device).

I don't know how to handle the binding docs as the compatible string
is still a requirement for older kernel versions. Is it ok to update
the device-tree bindings docs in such a way it would break old
drivers? Or should we keep it there until the last LTS kernel
requiring it reaches EOL? As device-tree bindings docs should not
consider how the driver was implemented, I think it would be strange
to have a note like "required by kernel up to 6.x".

Regards,

Luiz

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Vladimir Oltean]

On Thu, Dec 07, 2023 at 04:50:12PM -0300, Luiz Angelo Daros de Luca wrote:
> HI Vladmir,
> 
> We discussed something about that in the past:
> 
> https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/#m04e6cf7d0c1f18b02c2bf40266ada915b1f02f3d
> 
> The code is able to handle only a single node and binding docs say it
> should be named "mdio". The compatible string wasn't a requirement
> since the beginning and I don't think it is worth it to rename the
> compatible string. I suggest we simply switch to
> of_get_child_by_name() and look for a node named "mdio". If that node
> is not found, we can still look for the old compatible string
> (backwards compatibility) and probably warn the "user" (targeting not
> the end-user but the one creating the DT for a new device).
> 
> I don't know how to handle the binding docs as the compatible string
> is still a requirement for older kernel versions. Is it ok to update
> the device-tree bindings docs in such a way it would break old
> drivers? Or should we keep it there until the last LTS kernel
> requiring it reaches EOL? As device-tree bindings docs should not
> consider how the driver was implemented, I think it would be strange
> to have a note like "required by kernel up to 6.x".
> 
> Regards,
> 
> Luiz

And did you ever answer this question?

"And why do you even need to remove the compatible string from the MDIO
node, can't you just ignore it, does it bother you in any way?"

I'm very confused as to what you're after.

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Luiz Angelo Daros de Luca]

> > We discussed something about that in the past:
> >
> > https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/#m04e6cf7d0c1f18b02c2bf40266ada915b1f02f3d
> >
> > The code is able to handle only a single node and binding docs say it
> > should be named "mdio". The compatible string wasn't a requirement
> > since the beginning and I don't think it is worth it to rename the
> > compatible string. I suggest we simply switch to
> > of_get_child_by_name() and look for a node named "mdio". If that node
> > is not found, we can still look for the old compatible string
> > (backwards compatibility) and probably warn the "user" (targeting not
> > the end-user but the one creating the DT for a new device).
> >
> > I don't know how to handle the binding docs as the compatible string
> > is still a requirement for older kernel versions. Is it ok to update
> > the device-tree bindings docs in such a way it would break old
> > drivers? Or should we keep it there until the last LTS kernel
> > requiring it reaches EOL? As device-tree bindings docs should not
> > consider how the driver was implemented, I think it would be strange
> > to have a note like "required by kernel up to 6.x".
> >
> > Regards,
> >
> > Luiz
>
> And did you ever answer this question?
>
> "And why do you even need to remove the compatible string from the MDIO
> node, can't you just ignore it, does it bother you in any way?"
>
> I'm very confused as to what you're after.

The device-tree bindings should delineate the hardware characteristics
rather than specifying the implementation details of a particular
driver. The requirement of an "mdio" node with a compatible string
such as "realtek,smi-mdio" may be misleading, implying a potential
correlation between the host-switch interface (SMI, SPI, or MDIO) and
a specific user MDIO it describes. It's important to note that how we
describe the user mdio could vary for other future switch families,
but not with a distinct management interface.

I am currently conducting tests using the same user MDIO driver for
both realtek-smi and realtek-mdio. However, it's noteworthy that
unlike realtek-smi, the current user MDIO for realtek-mdio does not
require a compatible string; only a node named "mdio". Realtek-mdio is
presently utilizing the generic DSA user MDIO, but you mentioned it's
not considered a "core functionality." I assume this implies I
shouldn't depend on it. That's the reason for my switch to the
existing user MDIO driver from realtek-smi.

Regarding the absence of a compatible string for realtek-mdio, we have
a few options: introducing a new compatible string exclusively for
realtek-mdio, such as "realtek,mdio-mdio"; creating a new generic one
for both interfaces like "realtek,user-mdio" or "rtl836x-user-mdio";
or simply ignore the compatible string, as you suggested. However, if
I opt to ignore it, I presume I should retrieve that node solely based
on the node name. That's what I'm after. Is my understanding correct?

I'll post a new series that is still compatible both with old HW
descriptions and the device-tree bindings. In that way, I'll not touch
the docs. However, given that the compatible string is unnecessary to
describe the hardware, and after we modify the code to disregard it,
it is awkward for the binding documentation to request a compatible
string that serves no purpose. Shouldn't we consider updating this
requirement at some point?

Regards,

Luiz

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Vladimir Oltean]

On Thu, Dec 07, 2023 at 11:46:46PM -0300, Luiz Angelo Daros de Luca wrote:
> The device-tree bindings should delineate the hardware characteristics
> rather than specifying the implementation details of a particular
> driver. The requirement of an "mdio" node with a compatible string
> such as "realtek,smi-mdio" may be misleading, implying a potential
> correlation between the host-switch interface (SMI, SPI, or MDIO) and
> a specific user MDIO it describes. It's important to note that how we
> describe the user mdio could vary for other future switch families,
> but not with a distinct management interface.

Agree, "realtek,smi-mdio" is not a great compatible string. But it is an
established compatible string.

> I am currently conducting tests using the same user MDIO driver for
> both realtek-smi and realtek-mdio. However, it's noteworthy that
> unlike realtek-smi, the current user MDIO for realtek-mdio does not
> require a compatible string; only a node named "mdio". Realtek-mdio is
> presently utilizing the generic DSA user MDIO, but you mentioned it's
> not considered a "core functionality." I assume this implies I
> shouldn't depend on it. That's the reason for my switch to the
> existing user MDIO driver from realtek-smi.
> 
> Regarding the absence of a compatible string for realtek-mdio, we have
> a few options: introducing a new compatible string exclusively for
> realtek-mdio, such as "realtek,mdio-mdio"; creating a new generic one
> for both interfaces like "realtek,user-mdio" or "rtl836x-user-mdio";

The naming choice really looks like the secondary problem here.
But what about "realtek,rtl8365mb-mdio" and "realtek,rtl8366rb-mdio" as
a secondary compatible string for SMI, and primary compatible string for
MDIO-connected switches?

> or simply ignore the compatible string, as you suggested. However, if
> I opt to ignore it, I presume I should retrieve that node solely based
> on the node name. That's what I'm after. Is my understanding correct?

You could do that. There's a very high chance that the node was named
"mdio". The schema says it should be called "mdio", _and_ be compatible
with realtek,smi-mdio. If anyone comes and complains (very unlikely),
just say "hey, even Documentation/devicetree/bindings/net/dsa/realtek-smi.txt
said you should name the node 'mdio'"!

> I'll post a new series that is still compatible both with old HW
> descriptions and the device-tree bindings. In that way, I'll not touch
> the docs.

> However, given that the compatible string is unnecessary to describe
> the hardware, and after we modify the code to disregard it, it is
> awkward for the binding documentation to request a compatible string
> that serves no purpose. Shouldn't we consider updating this
> requirement at some point?
> 
> Regards,
> 
> Luiz

Not everything that is in the device tree has to be used. It is a
description of the hardware, not a rigid set of instructions for what
the OS has to do. The OS still does whatever it wants based on that info.

You can ask anyone about this. See Thomas Petazzoni's slides as just one
example.
https://elinux.org/images/f/f9/Petazzoni-device-tree-dummies_0.pdf

| The Device Tree is really a hardware description language.
| It should describe the hardware layout, and how it works.
| But it should not describe which particular hardware configuration you’re interested in.
| As an example:
| You may describe in the DT whether a particular piece of hardware supports DMA or not.
| But you may not describe in the DT if you want to use DMA or not.

It's a really weak argument for recommending users to remove the compatible
string, thereby deliberately breaking ABI compatibility in one direction,
to literally _no_ benefit.

Compatible strings for MDIO controllers are, in essence, not strange.
They are independent devices which could reasonably be bound to their
own drivers. I don't see what's awkward about this.

Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

[Dan Carpenter]

Hi Luiz,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Luiz-Angelo-Daros-de-Luca/net-dsa-realtek-create-realtek-common/20231118-075444
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231117235140.1178-3-luizluca%40gmail.com
patch subject: [net-next 2/2] net: dsa: realtek: load switch variants on demand
config: mips-randconfig-r081-20231121 (https://download.01.org/0day-ci/archive/20231125/202311251132.QKdGl71R-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231125/202311251132.QKdGl71R-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>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202311251132.QKdGl71R-lkp@intel.com/

smatch warnings:
drivers/net/dsa/realtek/realtek-smi.c:418 realtek_smi_probe() warn: passing zero to 'PTR_ERR'

vim +/PTR_ERR +418 drivers/net/dsa/realtek/realtek-smi.c

d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  398  static int realtek_smi_probe(struct platform_device *pdev)
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  399  {
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  400  	struct device *dev = &pdev->dev;
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28  401  	struct realtek_priv *priv;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  402  	int ret;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  403  
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  404  	priv = realtek_common_probe(dev, realtek_smi_regmap_config,
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  405  				    realtek_smi_nolock_regmap_config);
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  406  	if (IS_ERR(priv))
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  407  		return PTR_ERR(priv);
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  408  
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  409  	/* Fetch MDIO pins */
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28  410  	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  411  	if (IS_ERR(priv->mdc)) {
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  412  		ret = PTR_ERR(priv->mdc);
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  413  		goto err_variant_put;
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  414  	}
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  415  
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28  416  	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  417  	if (IS_ERR(priv->mdio)) {
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17 @418  		ret = PTR_ERR(priv->mdc);

s/mdc/mdio/.

7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  419  		goto err_variant_put;
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  420  	}
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  421  
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  422  	priv->setup_interface = realtek_smi_setup_mdio;
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  423  	priv->write_reg_noack = realtek_smi_write_reg_noack;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  424  
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28  425  	ret = priv->ops->detect(priv);
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  426  	if (ret) {
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  427  		dev_err(dev, "unable to detect switch\n");
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c      Luiz Angelo Daros de Luca 2023-11-17  428  		goto err_variant_put;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  429  	}
d8652956cf37c5 drivers/net/dsa/realtek-smi.c              Linus Walleij             2018-07-14  430  

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

Re: [net-next 0/2] net: dsa: realtek: Introduce realtek_common, load variants on demand

[Luiz Angelo Daros de Luca]

Sorry, I used the wrong prefix. It is missing PATCH. I'll fix it in a
v2 after the first round of reviews.

Re: [net-next 0/2] net: dsa: realtek: Introduce realtek_common, load variants on demand

[Jakub Kicinski]

On Fri, 17 Nov 2023 20:57:34 -0300 Luiz Angelo Daros de Luca wrote:
> Sorry, I used the wrong prefix. It is missing PATCH.

That's fine, patchwork gobbled it up as is, no need to repost.