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

[Vladimir Oltean <olteanv@gmail.com>]

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

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

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

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

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

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

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