| Message ID | 20231223005253.17891-4-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: variants to drivers, interfaces to a common module | expand |
On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote: > Some code can be shared between both interface modules (MDIO and SMI) > and among variants. These interface functions migrated to a common > module: > > - realtek_common_lock > - realtek_common_unlock > - realtek_common_probe > - realtek_common_register_switch > - realtek_common_remove > > The reset during probe was moved to the end of the common probe. This way, > we avoid a reset if anything else fails. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c > new file mode 100644 > index 000000000000..80b37e5fe780 > --- /dev/null > +++ b/drivers/net/dsa/realtek/realtek-common.c > @@ -0,0 +1,132 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +#include <linux/module.h> > + > +#include "realtek.h" > +#include "realtek-common.h" > + > +void realtek_common_lock(void *ctx) > +{ > + struct realtek_priv *priv = ctx; > + > + mutex_lock(&priv->map_lock); > +} > +EXPORT_SYMBOL_GPL(realtek_common_lock); Would you mind adding some kernel-doc comments above each of these exported functions? https://docs.kernel.org/doc-guide/kernel-doc.html says "Every function that is exported to loadable modules using EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc comment. Functions and data structures in header files which are intended to be used by modules should also have kernel-doc comments." It is something I only recently started paying attention to, so we don't have consistency in this regard. But we should try to adhere to this practice for code we change. > + > +void realtek_common_unlock(void *ctx) > +{ > + struct realtek_priv *priv = ctx; > + > + mutex_unlock(&priv->map_lock); > +} > +EXPORT_SYMBOL_GPL(realtek_common_unlock); > + > +struct realtek_priv * > +realtek_common_probe(struct device *dev, struct regmap_config rc, > + struct regmap_config rc_nolock) Could you use "const struct regmap_config *" as the data types here, to avoid two on-stack variable copies? Regmap will copy the config structures anyway. > +{ > + const struct realtek_variant *var; > + struct realtek_priv *priv; > + int ret; > + > + var = of_device_get_match_data(dev); > + if (!var) > + return ERR_PTR(-EINVAL); > + > + priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz), > + GFP_KERNEL); > + if (!priv) > + return ERR_PTR(-ENOMEM); > + > + mutex_init(&priv->map_lock); > + > + rc.lock_arg = priv; > + priv->map = devm_regmap_init(dev, NULL, priv, &rc); > + if (IS_ERR(priv->map)) { > + ret = PTR_ERR(priv->map); > + dev_err(dev, "regmap init failed: %d\n", ret); > + return ERR_PTR(ret); > + } > + > + priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock); > + if (IS_ERR(priv->map_nolock)) { > + ret = PTR_ERR(priv->map_nolock); > + dev_err(dev, "regmap init failed: %d\n", ret); > + return ERR_PTR(ret); > + } > + > + /* Link forward and backward */ > + priv->dev = dev; > + priv->variant = var; > + priv->ops = var->ops; > + priv->chip_data = (void *)priv + sizeof(*priv); > + > + dev_set_drvdata(dev, priv); > + spin_lock_init(&priv->lock); > + > + priv->leds_disabled = of_property_read_bool(dev->of_node, > + "realtek,disable-leds"); > + > + /* TODO: if power is software controlled, set up any regulators here */ > + > + priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(priv->reset)) { > + dev_err(dev, "failed to get RESET GPIO\n"); > + return ERR_CAST(priv->reset); > + } > + if (priv->reset) { > + gpiod_set_value(priv->reset, 1); > + dev_dbg(dev, "asserted RESET\n"); > + msleep(REALTEK_HW_STOP_DELAY); > + gpiod_set_value(priv->reset, 0); > + msleep(REALTEK_HW_START_DELAY); > + dev_dbg(dev, "deasserted RESET\n"); > + } > + > + return priv; > +} > +EXPORT_SYMBOL(realtek_common_probe); > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > index e9ee778665b2..fbd0616c1df3 100644 > --- a/drivers/net/dsa/realtek/realtek.h > +++ b/drivers/net/dsa/realtek/realtek.h > @@ -58,11 +58,9 @@ struct realtek_priv { > struct mii_bus *bus; > int mdio_addr; > > - unsigned int clk_delay; > - u8 cmd_read; > - u8 cmd_write; > spinlock_t lock; /* Locks around command writes */ > struct dsa_switch *ds; > + const struct dsa_switch_ops *ds_ops; > struct irq_domain *irqdomain; > bool leds_disabled; > > @@ -79,6 +77,8 @@ struct realtek_priv { > int vlan_enabled; > int vlan4k_enabled; > > + const struct realtek_variant *variant; > + > char buf[4096]; > void *chip_data; /* Per-chip extra variant data */ > }; Can the changes to struct realtek_priv be a separate patch, to clarify what is being changed, and to leave the noisy code movement more isolated?
Em seg., 8 de jan. de 2024 às 11:00, Vladimir Oltean <olteanv@gmail.com> escreveu: > > On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote: > > Some code can be shared between both interface modules (MDIO and SMI) > > and among variants. These interface functions migrated to a common > > module: > > > > - realtek_common_lock > > - realtek_common_unlock > > - realtek_common_probe > > - realtek_common_register_switch > > - realtek_common_remove > > > > The reset during probe was moved to the end of the common probe. This way, > > we avoid a reset if anything else fails. > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c > > new file mode 100644 > > index 000000000000..80b37e5fe780 > > --- /dev/null > > +++ b/drivers/net/dsa/realtek/realtek-common.c > > @@ -0,0 +1,132 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +#include <linux/module.h> > > + > > +#include "realtek.h" > > +#include "realtek-common.h" > > + > > +void realtek_common_lock(void *ctx) > > +{ > > + struct realtek_priv *priv = ctx; > > + > > + mutex_lock(&priv->map_lock); > > +} > > +EXPORT_SYMBOL_GPL(realtek_common_lock); > > Would you mind adding some kernel-doc comments above each of these > exported functions? https://docs.kernel.org/doc-guide/kernel-doc.html > says "Every function that is exported to loadable modules using > EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc comment. > Functions and data structures in header files which are intended to be > used by modules should also have kernel-doc comments." > > It is something I only recently started paying attention to, so we don't > have consistency in this regard. But we should try to adhere to this > practice for code we change. > Sure. I'll pay attention to that too. > > + > > +void realtek_common_unlock(void *ctx) > > +{ > > + struct realtek_priv *priv = ctx; > > + > > + mutex_unlock(&priv->map_lock); > > +} > > +EXPORT_SYMBOL_GPL(realtek_common_unlock); > > + > > +struct realtek_priv * > > +realtek_common_probe(struct device *dev, struct regmap_config rc, > > + struct regmap_config rc_nolock) > > Could you use "const struct regmap_config *" as the data types here, to > avoid two on-stack variable copies? Regmap will copy the config structures > anyway. I could do that for rc_nolock but not for rc as we need to modify it before passing to regmap. I would still need to duplicate rc, either using the stack or heap. What would be the best option? 1) pass two pointers and copy one to stack 2) pass two pointers and copy one to heap 3) pass two structs (as it is today) 4) pass one pointer and one struct The old code was using 1) and I'm inclined to adopt it and save a hundred and so bytes from the stack, although 2) would save even more. > > +{ > > + const struct realtek_variant *var; > > + struct realtek_priv *priv; > > + int ret; > > + > > + var = of_device_get_match_data(dev); > > + if (!var) > > + return ERR_PTR(-EINVAL); > > + > > + priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz), > > + GFP_KERNEL); > > + if (!priv) > > + return ERR_PTR(-ENOMEM); > > + > > + mutex_init(&priv->map_lock); > > + > > + rc.lock_arg = priv; > > + priv->map = devm_regmap_init(dev, NULL, priv, &rc); > > + if (IS_ERR(priv->map)) { > > + ret = PTR_ERR(priv->map); > > + dev_err(dev, "regmap init failed: %d\n", ret); > > + return ERR_PTR(ret); > > + } > > + > > + priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock); > > + if (IS_ERR(priv->map_nolock)) { > > + ret = PTR_ERR(priv->map_nolock); > > + dev_err(dev, "regmap init failed: %d\n", ret); > > + return ERR_PTR(ret); > > + } > > + > > + /* Link forward and backward */ > > + priv->dev = dev; > > + priv->variant = var; > > + priv->ops = var->ops; > > + priv->chip_data = (void *)priv + sizeof(*priv); > > + > > + dev_set_drvdata(dev, priv); > > + spin_lock_init(&priv->lock); > > + > > + priv->leds_disabled = of_property_read_bool(dev->of_node, > > + "realtek,disable-leds"); > > + > > + /* TODO: if power is software controlled, set up any regulators here */ > > + > > + priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(priv->reset)) { > > + dev_err(dev, "failed to get RESET GPIO\n"); > > + return ERR_CAST(priv->reset); > > + } > > + if (priv->reset) { > > + gpiod_set_value(priv->reset, 1); > > + dev_dbg(dev, "asserted RESET\n"); > > + msleep(REALTEK_HW_STOP_DELAY); > > + gpiod_set_value(priv->reset, 0); > > + msleep(REALTEK_HW_START_DELAY); > > + dev_dbg(dev, "deasserted RESET\n"); > > + } > > + > > + return priv; > > +} > > +EXPORT_SYMBOL(realtek_common_probe); > > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > > index e9ee778665b2..fbd0616c1df3 100644 > > --- a/drivers/net/dsa/realtek/realtek.h > > +++ b/drivers/net/dsa/realtek/realtek.h > > @@ -58,11 +58,9 @@ struct realtek_priv { > > struct mii_bus *bus; > > int mdio_addr; > > > > - unsigned int clk_delay; > > - u8 cmd_read; > > - u8 cmd_write; > > spinlock_t lock; /* Locks around command writes */ > > struct dsa_switch *ds; > > + const struct dsa_switch_ops *ds_ops; > > struct irq_domain *irqdomain; > > bool leds_disabled; > > > > @@ -79,6 +77,8 @@ struct realtek_priv { > > int vlan_enabled; > > int vlan4k_enabled; > > > > + const struct realtek_variant *variant; > > + > > char buf[4096]; > > void *chip_data; /* Per-chip extra variant data */ > > }; > > Can the changes to struct realtek_priv be a separate patch, to > clarify what is being changed, and to leave the noisy code movement > more isolated? Sure, although it will not be a patch that makes sense by itself. If it helps with the review, I'll split it. We can fold it back if needed. Regards, Luiz
On Tue, Jan 09, 2024 at 02:05:29AM -0300, Luiz Angelo Daros de Luca wrote: > > > +struct realtek_priv * > > > +realtek_common_probe(struct device *dev, struct regmap_config rc, > > > + struct regmap_config rc_nolock) > > > > Could you use "const struct regmap_config *" as the data types here, to > > avoid two on-stack variable copies? Regmap will copy the config structures > > anyway. > > I could do that for rc_nolock but not for rc as we need to modify it > before passing to regmap. I would still need to duplicate rc, either > using the stack or heap. What would be the best option? > > 1) pass two pointers and copy one to stack > 2) pass two pointers and copy one to heap > 3) pass two structs (as it is today) > 4) pass one pointer and one struct > > The old code was using 1) and I'm inclined to adopt it and save a > hundred and so bytes from the stack, although 2) would save even more. I didn't notice the "rc.lock_arg = priv" assignment... I'm not sure what you mean by "copy to heap". Perform a dynamic memory allocation? Also, the old code was not using exactly 1). It copied both the normal and the nolock regmap config to an on-stack local variable, even though only the normal regmap config had to be copied (to be fixed up). I went back to study the 4 regmap configs, and only the reg_read() and reg_write() methods differ between SMI and MDIO. The rest seems boilerplate that can be dynamically constructed by realtek_common_probe(). Sure, spelling out 4 regmap_config structures is more flexible, but do we need that flexibility? What if realtek_common_probe() takes just the reg_read() and reg_write() function prototypes as arguments, rather than pointers to regmap_config structures it then has to fix up? > > > +EXPORT_SYMBOL(realtek_common_probe); > > > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > > > index e9ee778665b2..fbd0616c1df3 100644 > > > --- a/drivers/net/dsa/realtek/realtek.h > > > +++ b/drivers/net/dsa/realtek/realtek.h > > > @@ -58,11 +58,9 @@ struct realtek_priv { > > > struct mii_bus *bus; > > > int mdio_addr; > > > > > > - unsigned int clk_delay; > > > - u8 cmd_read; > > > - u8 cmd_write; > > > spinlock_t lock; /* Locks around command writes */ > > > struct dsa_switch *ds; > > > + const struct dsa_switch_ops *ds_ops; > > > struct irq_domain *irqdomain; > > > bool leds_disabled; > > > > > > @@ -79,6 +77,8 @@ struct realtek_priv { > > > int vlan_enabled; > > > int vlan4k_enabled; > > > > > > + const struct realtek_variant *variant; > > > + > > > char buf[4096]; > > > void *chip_data; /* Per-chip extra variant data */ > > > }; > > > > Can the changes to struct realtek_priv be a separate patch, to > > clarify what is being changed, and to leave the noisy code movement > > more isolated? > > Sure, although it will not be a patch that makes sense by itself. If > it helps with the review, I'll split it. We can fold it back if > needed. Well, I don't mean only the changes to the private structure, but also the code changes that accompany them. As Andrew usually says, what you want is lots of small patches that are each obviously correct, where there is only one thing being changed. Code movement with small renames is trivial to review. Consolidation of two identical code paths in a single function is also possible to follow. The insertion of a new variable and its usage is also easy to review. The removal of a variable, the same. But superimposing them all into a single patch makes everything much more difficult to follow.
> On Tue, Jan 09, 2024 at 02:05:29AM -0300, Luiz Angelo Daros de Luca wrote: > > > > +struct realtek_priv * > > > > +realtek_common_probe(struct device *dev, struct regmap_config rc, > > > > + struct regmap_config rc_nolock) > > > > > > Could you use "const struct regmap_config *" as the data types here, to > > > avoid two on-stack variable copies? Regmap will copy the config structures > > > anyway. > > > > I could do that for rc_nolock but not for rc as we need to modify it > > before passing to regmap. I would still need to duplicate rc, either > > using the stack or heap. What would be the best option? > > > > 1) pass two pointers and copy one to stack > > 2) pass two pointers and copy one to heap > > 3) pass two structs (as it is today) > > 4) pass one pointer and one struct > > > > The old code was using 1) and I'm inclined to adopt it and save a > > hundred and so bytes from the stack, although 2) would save even more. > > I didn't notice the "rc.lock_arg = priv" assignment... > > I'm not sure what you mean by "copy to heap". Perform a dynamic memory > allocation? Yes. However, I guess the stack can handle that structure in this context. > Also, the old code was not using exactly 1). It copied both the normal > and the nolock regmap config to an on-stack local variable, even though > only the normal regmap config had to be copied (to be fixed up). > > I went back to study the 4 regmap configs, and only the reg_read() and > reg_write() methods differ between SMI and MDIO. The rest seems boilerplate > that can be dynamically constructed by realtek_common_probe(). Sure, > spelling out 4 regmap_config structures is more flexible, but do we need > that flexibility? What if realtek_common_probe() takes just the > reg_read() and reg_write() function prototypes as arguments, rather than > pointers to regmap_config structures it then has to fix up? IMHO, the constant regmap_config looks cleaner than a sequence of assignments. However, we don't actually need 4 of them. As we already have a writable regmap_config in stack (to assign lock_arg), we can reuse the same struct and simply set disable_locking. It makes the regmap ignore all locking fields and we don't even need to unset them for map_nolock. Something like this: realtek_common_probe(struct device *dev, const struct regmap_config *rc_base) { (...) rc = *rc_base; rc.lock_arg = priv; priv->map = devm_regmap_init(dev, NULL, priv, &rc); if (IS_ERR(priv->map)) { ret = PTR_ERR(priv->map); dev_err(dev, "regmap init failed: %d\n", ret); return ERR_PTR(ret); } rc.disable_locking = true; priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc); if (IS_ERR(priv->map_nolock)) { ret = PTR_ERR(priv->map_nolock); dev_err(dev, "regmap init failed: %d\n", ret); return ERR_PTR(ret); } It has a cleaner function signature and we can remove the _nolock constants as well. The regmap configs still have some room for improvement, like moving from interfaces to variants, but this series is already too big. We can leave that as it is. > > > > +EXPORT_SYMBOL(realtek_common_probe); > > > > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > > > > index e9ee778665b2..fbd0616c1df3 100644 > > > > --- a/drivers/net/dsa/realtek/realtek.h > > > > +++ b/drivers/net/dsa/realtek/realtek.h > > > > @@ -58,11 +58,9 @@ struct realtek_priv { > > > > struct mii_bus *bus; > > > > int mdio_addr; > > > > > > > > - unsigned int clk_delay; > > > > - u8 cmd_read; > > > > - u8 cmd_write; > > > > spinlock_t lock; /* Locks around command writes */ > > > > struct dsa_switch *ds; > > > > + const struct dsa_switch_ops *ds_ops; > > > > struct irq_domain *irqdomain; > > > > bool leds_disabled; > > > > > > > > @@ -79,6 +77,8 @@ struct realtek_priv { > > > > int vlan_enabled; > > > > int vlan4k_enabled; > > > > > > > > + const struct realtek_variant *variant; > > > > + > > > > char buf[4096]; > > > > void *chip_data; /* Per-chip extra variant data */ > > > > }; > > > > > > Can the changes to struct realtek_priv be a separate patch, to > > > clarify what is being changed, and to leave the noisy code movement > > > more isolated? > > > > Sure, although it will not be a patch that makes sense by itself. If > > it helps with the review, I'll split it. We can fold it back if > > needed. > > Well, I don't mean only the changes to the private structure, but also > the code changes that accompany them. > > As Andrew usually says, what you want is lots of small patches that are > each obviously correct, where there is only one thing being changed. > Code movement with small renames is trivial to review. Consolidation of > two identical code paths in a single function is also possible to follow. > The insertion of a new variable and its usage is also easy to review. > The removal of a variable, the same. But superimposing them all into a > single patch makes everything much more difficult to follow. This case in particular might be hard to justify in the commit message other than "preliminary work". I'll split it as it makes review much easier. this series will grow from 7 to 10 patches, even after dropping the revert patch. Regards, Luiz
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);
On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote: > +EXPORT_SYMBOL_GPL(realtek_common_lock); > +EXPORT_SYMBOL_GPL(realtek_common_unlock); > +EXPORT_SYMBOL(realtek_common_probe); > +EXPORT_SYMBOL(realtek_common_register_switch); > +EXPORT_SYMBOL(realtek_common_remove); Is there any reason for the lack of consistency between GPL and non-GPL symbols? Also, I don't like too much the naming of symbols like "realtek_common_probe", exported to the entire kernel. I wonder if it would be better to drop the word "common" altogether, and use EXPORT_SYMBOL_NS_GPL(*, REALTEK_DSA) + MODULE_IMPORT_NS(REALTEK_DSA) instead of plain EXPORT_SYMBOL_GPL()?
On Thu, Jan 11, 2024 at 11:41:48AM +0200, Vladimir Oltean wrote: > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h > index 518d091ff496..71fc43d8d90a 100644 > --- a/drivers/net/dsa/realtek/realtek-common.h > +++ b/drivers/net/dsa/realtek/realtek-common.h > @@ -5,11 +5,16 @@ > > #include <linux/regmap.h> > > +struct realtek_common_info { > + int (*reg_read)(void *ctx, u32 reg, u32 *val); > + int (*reg_write)(void *ctx, u32 reg, u32 val); > +}; > + Yes, this is good. Makes it easier to expand later if necessary, too. > void realtek_common_lock(void *ctx); > void realtek_common_unlock(void *ctx); > struct realtek_priv * > -realtek_common_probe(struct device *dev, struct regmap_config rc, > - struct regmap_config rc_nolock); > +realtek_common_probe(struct device *dev, > + const struct realtek_common_info *info); > int realtek_common_register_switch(struct realtek_priv *priv); > void realtek_common_remove(struct realtek_priv *priv);
> On Fri, Dec 22, 2023 at 09:46:31PM -0300, Luiz Angelo Daros de Luca wrote: > > +EXPORT_SYMBOL_GPL(realtek_common_lock); > > +EXPORT_SYMBOL_GPL(realtek_common_unlock); > > +EXPORT_SYMBOL(realtek_common_probe); > > +EXPORT_SYMBOL(realtek_common_register_switch); > > +EXPORT_SYMBOL(realtek_common_remove); > > Is there any reason for the lack of consistency between GPL and non-GPL > symbols? No. I might have just copied the string from the wrong example. > Also, I don't like too much the naming of symbols like "realtek_common_probe", > exported to the entire kernel. I wonder if it would be better to drop > the word "common" altogether, and use EXPORT_SYMBOL_NS_GPL(*, REALTEK_DSA) + > MODULE_IMPORT_NS(REALTEK_DSA) instead of plain EXPORT_SYMBOL_GPL()? Introducing a namespace seems to be a nice ideia. Let the series grow :-) What do you mean by dropping the "common"? Use "realtek_probe" or "realtek_dsa_probe"? Regards, Luiz
On Thu, Jan 11, 2024 at 04:53:01PM -0300, Luiz Angelo Daros de Luca wrote: > What do you mean by dropping the "common"? Use "realtek_probe" or > "realtek_dsa_probe"? Yes, I meant "realtek_probe()" etc. It's just that the word "common" has no added value in function names, and I can't come up with something good to replace it with.
Em qui., 11 de jan. de 2024 às 06:41, Vladimir Oltean <olteanv@gmail.com> escreveu: > > On Thu, Jan 11, 2024 at 03:20:10AM -0300, Luiz Angelo Daros de Luca wrote: > > IMHO, the constant regmap_config looks cleaner than a sequence of > > assignments. However, we don't actually need 4 of them. > > As we already have a writable regmap_config in stack (to assign > > lock_arg), we can reuse the same struct and simply set > > disable_locking. > > It makes the regmap ignore all locking fields and we don't even need > > to unset them for map_nolock. Something like this: > > > > realtek_common_probe(struct device *dev, const struct regmap_config *rc_base) > > { > > > > (...) > > > > rc = *rc_base; > > rc.lock_arg = priv; > > priv->map = devm_regmap_init(dev, NULL, priv, &rc); > > if (IS_ERR(priv->map)) { > > ret = PTR_ERR(priv->map); > > dev_err(dev, "regmap init failed: %d\n", ret); > > return ERR_PTR(ret); > > } > > > > rc.disable_locking = true; > > priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc); > > if (IS_ERR(priv->map_nolock)) { > > ret = PTR_ERR(priv->map_nolock); > > dev_err(dev, "regmap init failed: %d\n", ret); > > return ERR_PTR(ret); > > } > > > > It has a cleaner function signature and we can remove the _nolock > > constants as well. > > > > The regmap configs still have some room for improvement, like moving > > from interfaces to variants, but this series is already too big. We > > can leave that as it is. > > I was thinking something like this, does it look bad? > > From 2e462507171ed0fd8393598842dc0f7e6c50d499 Mon Sep 17 00:00:00 2001 > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Date: Thu, 11 Jan 2024 11:38:17 +0200 > Subject: [PATCH] realtek_common_info > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/realtek/realtek-common.c | 35 ++++++++++++++++++------ > drivers/net/dsa/realtek/realtek-common.h | 9 ++++-- > drivers/net/dsa/realtek/realtek-mdio.c | 27 ++---------------- > drivers/net/dsa/realtek/realtek-smi.c | 35 ++++-------------------- > 4 files changed, 41 insertions(+), 65 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c > index 80b37e5fe780..bd6b04922b6d 100644 > --- a/drivers/net/dsa/realtek/realtek-common.c > +++ b/drivers/net/dsa/realtek/realtek-common.c > @@ -22,10 +22,21 @@ void realtek_common_unlock(void *ctx) > EXPORT_SYMBOL_GPL(realtek_common_unlock); > > struct realtek_priv * > -realtek_common_probe(struct device *dev, struct regmap_config rc, > - struct regmap_config rc_nolock) > +realtek_common_probe(struct device *dev, > + const struct realtek_common_info *info) > { > const struct realtek_variant *var; > + struct regmap_config rc = { > + .reg_bits = 10, /* A4..A0 R4..R0 */ > + .val_bits = 16, > + .reg_stride = 1, > + /* PHY regs are at 0x8000 */ > + .max_register = 0xffff, > + .reg_format_endian = REGMAP_ENDIAN_BIG, > + .reg_read = info->reg_read, > + .reg_write = info->reg_write, > + .cache_type = REGCACHE_NONE, > + }; > struct realtek_priv *priv; > int ret; > > @@ -40,17 +51,23 @@ realtek_common_probe(struct device *dev, struct regmap_config rc, > > mutex_init(&priv->map_lock); > > - rc.lock_arg = priv; > - priv->map = devm_regmap_init(dev, NULL, priv, &rc); > - if (IS_ERR(priv->map)) { > - ret = PTR_ERR(priv->map); > + /* Initialize the non-locking regmap first */ > + rc.disable_locking = true; > + priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc); > + if (IS_ERR(priv->map_nolock)) { > + ret = PTR_ERR(priv->map_nolock); > dev_err(dev, "regmap init failed: %d\n", ret); > return ERR_PTR(ret); > } > > - priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock); > - if (IS_ERR(priv->map_nolock)) { > - ret = PTR_ERR(priv->map_nolock); > + /* Then the locking regmap */ > + rc.disable_locking = false; > + rc.lock = realtek_common_lock; > + rc.unlock = realtek_common_unlock; > + rc.lock_arg = priv; > + priv->map = devm_regmap_init(dev, NULL, priv, &rc); > + if (IS_ERR(priv->map)) { > + ret = PTR_ERR(priv->map); > dev_err(dev, "regmap init failed: %d\n", ret); > return ERR_PTR(ret); > } > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h > index 518d091ff496..71fc43d8d90a 100644 > --- a/drivers/net/dsa/realtek/realtek-common.h > +++ b/drivers/net/dsa/realtek/realtek-common.h > @@ -5,11 +5,16 @@ > > #include <linux/regmap.h> > > +struct realtek_common_info { > + int (*reg_read)(void *ctx, u32 reg, u32 *val); > + int (*reg_write)(void *ctx, u32 reg, u32 val); > +}; > + > void realtek_common_lock(void *ctx); > void realtek_common_unlock(void *ctx); > struct realtek_priv * > -realtek_common_probe(struct device *dev, struct regmap_config rc, > - struct regmap_config rc_nolock); > +realtek_common_probe(struct device *dev, > + const struct realtek_common_info *info); > int realtek_common_register_switch(struct realtek_priv *priv); > void realtek_common_remove(struct realtek_priv *priv); > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 1eed09ab3aa1..8725cd1b027b 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -101,31 +101,9 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) > return ret; > } > > -static const struct regmap_config realtek_mdio_regmap_config = { > - .reg_bits = 10, /* A4..A0 R4..R0 */ > - .val_bits = 16, > - .reg_stride = 1, > - /* PHY regs are at 0x8000 */ > - .max_register = 0xffff, > - .reg_format_endian = REGMAP_ENDIAN_BIG, > +static const struct realtek_common_info realtek_mdio_info = { > .reg_read = realtek_mdio_read, > .reg_write = realtek_mdio_write, > - .cache_type = REGCACHE_NONE, > - .lock = realtek_common_lock, > - .unlock = realtek_common_unlock, > -}; > - > -static const struct regmap_config realtek_mdio_nolock_regmap_config = { > - .reg_bits = 10, /* A4..A0 R4..R0 */ > - .val_bits = 16, > - .reg_stride = 1, > - /* PHY regs are at 0x8000 */ > - .max_register = 0xffff, > - .reg_format_endian = REGMAP_ENDIAN_BIG, > - .reg_read = realtek_mdio_read, > - .reg_write = realtek_mdio_write, > - .cache_type = REGCACHE_NONE, > - .disable_locking = true, > }; > > int realtek_mdio_probe(struct mdio_device *mdiodev) > @@ -134,8 +112,7 @@ int realtek_mdio_probe(struct mdio_device *mdiodev) > struct realtek_priv *priv; > int ret; > > - priv = realtek_common_probe(dev, realtek_mdio_regmap_config, > - realtek_mdio_nolock_regmap_config); > + priv = realtek_common_probe(dev, &realtek_mdio_info); > if (IS_ERR(priv)) > return PTR_ERR(priv); > > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index fc54190839cf..7697dc66e5e8 100644 > --- a/drivers/net/dsa/realtek/realtek-smi.c > +++ b/drivers/net/dsa/realtek/realtek-smi.c > @@ -312,33 +312,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val) > return realtek_smi_read_reg(priv, reg, val); > } > > -static const struct regmap_config realtek_smi_regmap_config = { > - .reg_bits = 10, /* A4..A0 R4..R0 */ > - .val_bits = 16, > - .reg_stride = 1, > - /* PHY regs are at 0x8000 */ > - .max_register = 0xffff, > - .reg_format_endian = REGMAP_ENDIAN_BIG, > - .reg_read = realtek_smi_read, > - .reg_write = realtek_smi_write, > - .cache_type = REGCACHE_NONE, > - .lock = realtek_common_lock, > - .unlock = realtek_common_unlock, > -}; > - > -static const struct regmap_config realtek_smi_nolock_regmap_config = { > - .reg_bits = 10, /* A4..A0 R4..R0 */ > - .val_bits = 16, > - .reg_stride = 1, > - /* PHY regs are at 0x8000 */ > - .max_register = 0xffff, > - .reg_format_endian = REGMAP_ENDIAN_BIG, > - .reg_read = realtek_smi_read, > - .reg_write = realtek_smi_write, > - .cache_type = REGCACHE_NONE, > - .disable_locking = true, > -}; > - > static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum) > { > struct realtek_priv *priv = bus->priv; > @@ -396,14 +369,18 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) > return ret; > } > > +static const struct realtek_common_info realtek_smi_info = { > + .reg_read = realtek_smi_read, > + .reg_write = realtek_smi_write, > +}; > + > int realtek_smi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct realtek_priv *priv; > int ret; > > - priv = realtek_common_probe(dev, realtek_smi_regmap_config, > - realtek_smi_nolock_regmap_config); > + priv = realtek_common_probe(dev, &realtek_smi_info); > if (IS_ERR(priv)) > return PTR_ERR(priv); > > -- > 2.34.1 > I don't have a strong opinion on how the regmap config is prepared. I'll adopt your suggestion, with some minor changes. Regmap config for realtek dsa driver should be composed of values that (should) depend on the variant, the management interface (read/write) and some common values. Today it is only dependent on the interface and common values (with register range being a forced coincidence). However, that is a topic for another series. We do need to stop this series at some point. It already has 11 patches (and not counting the 2 bindings + 1 code that are actually my target). We already have some nice features for delivery. Regards, Luiz
> On Thu, Jan 11, 2024 at 04:53:01PM -0300, Luiz Angelo Daros de Luca wrote: > > What do you mean by dropping the "common"? Use "realtek_probe" or > > "realtek_dsa_probe"? > > Yes, I meant "realtek_probe()" etc. It's just that the word "common" has > no added value in function names, and I can't come up with something > good to replace it with. I didn't like realtek_probe() either. It is too generic, although now in a specific name space. How about replacing all realtek_common with rtl83xx? I guess all rtl83xx chips are switch controllers, even though some of them have an embedded mips core. Or we could include a prefix/suffix as well like rtl83xx_dsa or realtek_dsa.. Regards, Luiz
On Sat, Jan 13, 2024 at 06:38:39PM -0300, Luiz Angelo Daros de Luca wrote: > I didn't like realtek_probe() either. It is too generic, although now > in a specific name space. > How about replacing all realtek_common with rtl83xx? I guess all > rtl83xx chips are switch controllers, even though some of them have an > embedded mips core. > Or we could include a prefix/suffix as well like rtl83xx_dsa or realtek_dsa.. I don't have a problem with just rtl83xx.
diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile index 0aab57252a7c..f4f9c6340d5f 100644 --- a/drivers/net/dsa/realtek/Makefile +++ b/drivers/net/dsa/realtek/Makefile @@ -1,4 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_NET_DSA_REALTEK) += realtek-dsa.o +realtek-dsa-objs := realtek-common.o obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c new file mode 100644 index 000000000000..80b37e5fe780 --- /dev/null +++ b/drivers/net/dsa/realtek/realtek-common.c @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include <linux/module.h> + +#include "realtek.h" +#include "realtek-common.h" + +void realtek_common_lock(void *ctx) +{ + struct realtek_priv *priv = ctx; + + mutex_lock(&priv->map_lock); +} +EXPORT_SYMBOL_GPL(realtek_common_lock); + +void realtek_common_unlock(void *ctx) +{ + struct realtek_priv *priv = ctx; + + mutex_unlock(&priv->map_lock); +} +EXPORT_SYMBOL_GPL(realtek_common_unlock); + +struct realtek_priv * +realtek_common_probe(struct device *dev, struct regmap_config rc, + struct regmap_config rc_nolock) +{ + const struct realtek_variant *var; + struct realtek_priv *priv; + int ret; + + var = of_device_get_match_data(dev); + if (!var) + return ERR_PTR(-EINVAL); + + priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz), + GFP_KERNEL); + if (!priv) + return ERR_PTR(-ENOMEM); + + mutex_init(&priv->map_lock); + + rc.lock_arg = priv; + priv->map = devm_regmap_init(dev, NULL, priv, &rc); + if (IS_ERR(priv->map)) { + ret = PTR_ERR(priv->map); + dev_err(dev, "regmap init failed: %d\n", ret); + return ERR_PTR(ret); + } + + priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock); + if (IS_ERR(priv->map_nolock)) { + ret = PTR_ERR(priv->map_nolock); + dev_err(dev, "regmap init failed: %d\n", ret); + return ERR_PTR(ret); + } + + /* Link forward and backward */ + priv->dev = dev; + priv->variant = var; + priv->ops = var->ops; + priv->chip_data = (void *)priv + sizeof(*priv); + + dev_set_drvdata(dev, priv); + spin_lock_init(&priv->lock); + + priv->leds_disabled = of_property_read_bool(dev->of_node, + "realtek,disable-leds"); + + /* TODO: if power is software controlled, set up any regulators here */ + + priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(priv->reset)) { + dev_err(dev, "failed to get RESET GPIO\n"); + return ERR_CAST(priv->reset); + } + if (priv->reset) { + gpiod_set_value(priv->reset, 1); + dev_dbg(dev, "asserted RESET\n"); + msleep(REALTEK_HW_STOP_DELAY); + gpiod_set_value(priv->reset, 0); + msleep(REALTEK_HW_START_DELAY); + dev_dbg(dev, "deasserted RESET\n"); + } + + return priv; +} +EXPORT_SYMBOL(realtek_common_probe); + +int realtek_common_register_switch(struct realtek_priv *priv) +{ + int ret; + + ret = priv->ops->detect(priv); + if (ret) { + dev_err_probe(priv->dev, ret, "unable to detect switch\n"); + return ret; + } + + priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL); + if (!priv->ds) + return -ENOMEM; + + priv->ds->priv = priv; + priv->ds->dev = priv->dev; + priv->ds->ops = priv->ds_ops; + priv->ds->num_ports = priv->num_ports; + + ret = dsa_register_switch(priv->ds); + if (ret) { + dev_err_probe(priv->dev, ret, "unable to register switch\n"); + return ret; + } + + return 0; +} +EXPORT_SYMBOL(realtek_common_register_switch); + +void realtek_common_remove(struct realtek_priv *priv) +{ + if (!priv) + return; + + /* leave the device reset asserted */ + if (priv->reset) + gpiod_set_value(priv->reset, 1); +} +EXPORT_SYMBOL(realtek_common_remove); + +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); +MODULE_DESCRIPTION("Realtek DSA switches common module"); +MODULE_LICENSE("GPL"); diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h new file mode 100644 index 000000000000..518d091ff496 --- /dev/null +++ b/drivers/net/dsa/realtek/realtek-common.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ + +#ifndef _REALTEK_COMMON_H +#define _REALTEK_COMMON_H + +#include <linux/regmap.h> + +void realtek_common_lock(void *ctx); +void realtek_common_unlock(void *ctx); +struct realtek_priv * +realtek_common_probe(struct device *dev, struct regmap_config rc, + struct regmap_config rc_nolock); +int realtek_common_register_switch(struct realtek_priv *priv); +void realtek_common_remove(struct realtek_priv *priv); + +#endif /* _REALTEK_COMMON_H */ diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 58966d0625c8..1eed09ab3aa1 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -26,6 +26,7 @@ #include "realtek.h" #include "realtek-mdio.h" +#include "realtek-common.h" /* Read/write via mdiobus */ #define REALTEK_MDIO_CTRL0_REG 31 @@ -100,20 +101,6 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) return ret; } -static void realtek_mdio_lock(void *ctx) -{ - struct realtek_priv *priv = ctx; - - mutex_lock(&priv->map_lock); -} - -static void realtek_mdio_unlock(void *ctx) -{ - struct realtek_priv *priv = ctx; - - mutex_unlock(&priv->map_lock); -} - static const struct regmap_config realtek_mdio_regmap_config = { .reg_bits = 10, /* A4..A0 R4..R0 */ .val_bits = 16, @@ -124,8 +111,8 @@ static const struct regmap_config realtek_mdio_regmap_config = { .reg_read = realtek_mdio_read, .reg_write = realtek_mdio_write, .cache_type = REGCACHE_NONE, - .lock = realtek_mdio_lock, - .unlock = realtek_mdio_unlock, + .lock = realtek_common_lock, + .unlock = realtek_common_unlock, }; static const struct regmap_config realtek_mdio_nolock_regmap_config = { @@ -143,96 +130,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = { int realtek_mdio_probe(struct mdio_device *mdiodev) { - struct realtek_priv *priv; struct device *dev = &mdiodev->dev; - const struct realtek_variant *var; - struct regmap_config rc; - struct device_node *np; + struct realtek_priv *priv; int ret; - var = of_device_get_match_data(dev); - if (!var) - return -EINVAL; - - priv = devm_kzalloc(&mdiodev->dev, - size_add(sizeof(*priv), var->chip_data_sz), - GFP_KERNEL); - if (!priv) - return -ENOMEM; - - mutex_init(&priv->map_lock); + priv = realtek_common_probe(dev, realtek_mdio_regmap_config, + realtek_mdio_nolock_regmap_config); + if (IS_ERR(priv)) + return PTR_ERR(priv); - rc = realtek_mdio_regmap_config; - rc.lock_arg = priv; - priv->map = devm_regmap_init(dev, NULL, priv, &rc); - if (IS_ERR(priv->map)) { - ret = PTR_ERR(priv->map); - dev_err(dev, "regmap init failed: %d\n", ret); - return ret; - } - - rc = realtek_mdio_nolock_regmap_config; - priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc); - if (IS_ERR(priv->map_nolock)) { - ret = PTR_ERR(priv->map_nolock); - dev_err(dev, "regmap init failed: %d\n", ret); - return ret; - } - - priv->mdio_addr = mdiodev->addr; priv->bus = mdiodev->bus; - priv->dev = &mdiodev->dev; - priv->chip_data = (void *)priv + sizeof(*priv); - - priv->clk_delay = var->clk_delay; - priv->cmd_read = var->cmd_read; - priv->cmd_write = var->cmd_write; - priv->ops = var->ops; - + priv->mdio_addr = mdiodev->addr; priv->write_reg_noack = realtek_mdio_write; + priv->ds_ops = priv->variant->ds_ops_mdio; - np = dev->of_node; - - dev_set_drvdata(dev, priv); - - /* TODO: if power is software controlled, set up any regulators here */ - priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); - - priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); - if (IS_ERR(priv->reset)) { - dev_err(dev, "failed to get RESET GPIO\n"); - return PTR_ERR(priv->reset); - } - - if (priv->reset) { - gpiod_set_value(priv->reset, 1); - dev_dbg(dev, "asserted RESET\n"); - msleep(REALTEK_HW_STOP_DELAY); - gpiod_set_value(priv->reset, 0); - msleep(REALTEK_HW_START_DELAY); - dev_dbg(dev, "deasserted RESET\n"); - } - - ret = priv->ops->detect(priv); - if (ret) { - dev_err(dev, "unable to detect switch\n"); - return ret; - } - - priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL); - if (!priv->ds) - return -ENOMEM; - - priv->ds->dev = dev; - priv->ds->num_ports = priv->num_ports; - priv->ds->priv = priv; - priv->ds->ops = var->ds_ops_mdio; - - ret = dsa_register_switch(priv->ds); - if (ret) { - dev_err(priv->dev, "unable to register switch ret = %d\n", ret); + ret = realtek_common_register_switch(priv); + if (ret) return ret; - } return 0; } @@ -247,9 +161,7 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) dsa_unregister_switch(priv->ds); - /* leave the device reset asserted */ - if (priv->reset) - gpiod_set_value(priv->reset, 1); + realtek_common_remove(priv); } EXPORT_SYMBOL_GPL(realtek_mdio_remove); diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 31ac409acfd0..fc54190839cf 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -41,12 +41,13 @@ #include "realtek.h" #include "realtek-smi.h" +#include "realtek-common.h" #define REALTEK_SMI_ACK_RETRY_COUNT 5 static inline void realtek_smi_clk_delay(struct realtek_priv *priv) { - ndelay(priv->clk_delay); + ndelay(priv->variant->clk_delay); } static void realtek_smi_start(struct realtek_priv *priv) @@ -209,7 +210,7 @@ static int realtek_smi_read_reg(struct realtek_priv *priv, u32 addr, u32 *data) realtek_smi_start(priv); /* Send READ command */ - ret = realtek_smi_write_byte(priv, priv->cmd_read); + ret = realtek_smi_write_byte(priv, priv->variant->cmd_read); if (ret) goto out; @@ -250,7 +251,7 @@ static int realtek_smi_write_reg(struct realtek_priv *priv, realtek_smi_start(priv); /* Send WRITE command */ - ret = realtek_smi_write_byte(priv, priv->cmd_write); + ret = realtek_smi_write_byte(priv, priv->variant->cmd_write); if (ret) goto out; @@ -311,20 +312,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val) return realtek_smi_read_reg(priv, reg, val); } -static void realtek_smi_lock(void *ctx) -{ - struct realtek_priv *priv = ctx; - - mutex_lock(&priv->map_lock); -} - -static void realtek_smi_unlock(void *ctx) -{ - struct realtek_priv *priv = ctx; - - mutex_unlock(&priv->map_lock); -} - static const struct regmap_config realtek_smi_regmap_config = { .reg_bits = 10, /* A4..A0 R4..R0 */ .val_bits = 16, @@ -335,8 +322,8 @@ static const struct regmap_config realtek_smi_regmap_config = { .reg_read = realtek_smi_read, .reg_write = realtek_smi_write, .cache_type = REGCACHE_NONE, - .lock = realtek_smi_lock, - .unlock = realtek_smi_unlock, + .lock = realtek_common_lock, + .unlock = realtek_common_unlock, }; static const struct regmap_config realtek_smi_nolock_regmap_config = { @@ -411,99 +398,32 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) int realtek_smi_probe(struct platform_device *pdev) { - const struct realtek_variant *var; struct device *dev = &pdev->dev; struct realtek_priv *priv; - struct regmap_config rc; - struct device_node *np; int ret; - var = of_device_get_match_data(dev); - np = dev->of_node; - - priv = devm_kzalloc(dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL); - if (!priv) - return -ENOMEM; - priv->chip_data = (void *)priv + sizeof(*priv); - - mutex_init(&priv->map_lock); - - rc = realtek_smi_regmap_config; - rc.lock_arg = priv; - priv->map = devm_regmap_init(dev, NULL, priv, &rc); - if (IS_ERR(priv->map)) { - ret = PTR_ERR(priv->map); - dev_err(dev, "regmap init failed: %d\n", ret); - return ret; - } - - rc = realtek_smi_nolock_regmap_config; - priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc); - if (IS_ERR(priv->map_nolock)) { - ret = PTR_ERR(priv->map_nolock); - dev_err(dev, "regmap init failed: %d\n", ret); - return ret; - } - - /* Link forward and backward */ - priv->dev = dev; - priv->clk_delay = var->clk_delay; - priv->cmd_read = var->cmd_read; - priv->cmd_write = var->cmd_write; - priv->ops = var->ops; - - priv->setup_interface = realtek_smi_setup_mdio; - priv->write_reg_noack = realtek_smi_write_reg_noack; - - dev_set_drvdata(dev, priv); - spin_lock_init(&priv->lock); - - /* TODO: if power is software controlled, set up any regulators here */ - - priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); - if (IS_ERR(priv->reset)) { - dev_err(dev, "failed to get RESET GPIO\n"); - return PTR_ERR(priv->reset); - } - if (priv->reset) { - gpiod_set_value(priv->reset, 1); - dev_dbg(dev, "asserted RESET\n"); - msleep(REALTEK_HW_STOP_DELAY); - gpiod_set_value(priv->reset, 0); - msleep(REALTEK_HW_START_DELAY); - dev_dbg(dev, "deasserted RESET\n"); - } + priv = realtek_common_probe(dev, realtek_smi_regmap_config, + realtek_smi_nolock_regmap_config); + if (IS_ERR(priv)) + return PTR_ERR(priv); /* Fetch MDIO pins */ priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW); if (IS_ERR(priv->mdc)) return PTR_ERR(priv->mdc); + priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW); if (IS_ERR(priv->mdio)) return PTR_ERR(priv->mdio); - priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); + priv->write_reg_noack = realtek_smi_write_reg_noack; + priv->setup_interface = realtek_smi_setup_mdio; + priv->ds_ops = priv->variant->ds_ops_smi; - ret = priv->ops->detect(priv); - if (ret) { - dev_err(dev, "unable to detect switch\n"); + ret = realtek_common_register_switch(priv); + if (ret) return ret; - } - - priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL); - if (!priv->ds) - return -ENOMEM; - priv->ds->dev = dev; - priv->ds->num_ports = priv->num_ports; - priv->ds->priv = priv; - - priv->ds->ops = var->ds_ops_smi; - ret = dsa_register_switch(priv->ds); - if (ret) { - dev_err_probe(dev, ret, "unable to register switch\n"); - return ret; - } return 0; } EXPORT_SYMBOL_GPL(realtek_smi_probe); @@ -516,12 +436,11 @@ void realtek_smi_remove(struct platform_device *pdev) return; dsa_unregister_switch(priv->ds); + if (priv->user_mii_bus) of_node_put(priv->user_mii_bus->dev.of_node); - /* leave the device reset asserted */ - if (priv->reset) - gpiod_set_value(priv->reset, 1); + realtek_common_remove(priv); } EXPORT_SYMBOL_GPL(realtek_smi_remove); diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index e9ee778665b2..fbd0616c1df3 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -58,11 +58,9 @@ struct realtek_priv { struct mii_bus *bus; int mdio_addr; - unsigned int clk_delay; - u8 cmd_read; - u8 cmd_write; spinlock_t lock; /* Locks around command writes */ struct dsa_switch *ds; + const struct dsa_switch_ops *ds_ops; struct irq_domain *irqdomain; bool leds_disabled; @@ -79,6 +77,8 @@ struct realtek_priv { int vlan_enabled; int vlan4k_enabled; + const struct realtek_variant *variant; + char buf[4096]; void *chip_data; /* Per-chip extra variant data */ }; diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index a4d0abd5a6bf..972a73662641 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -103,6 +103,7 @@ #include "realtek.h" #include "realtek-smi.h" #include "realtek-mdio.h" +#include "realtek-common.h" /* Family-specific data and limits */ #define RTL8365MB_PHYADDRMAX 7 @@ -691,7 +692,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy, u32 val; int ret; - mutex_lock(&priv->map_lock); + realtek_common_lock(priv); ret = rtl8365mb_phy_poll_busy(priv); if (ret) @@ -724,7 +725,7 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy, *data = val & 0xFFFF; out: - mutex_unlock(&priv->map_lock); + realtek_common_unlock(priv); return ret; } @@ -735,7 +736,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, u32 val; int ret; - mutex_lock(&priv->map_lock); + realtek_common_lock(priv); ret = rtl8365mb_phy_poll_busy(priv); if (ret) @@ -766,7 +767,7 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, goto out; out: - mutex_unlock(&priv->map_lock); + realtek_common_unlock(priv); return 0; } diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c index cc2fd636ec23..e60a0a81d426 100644 --- a/drivers/net/dsa/realtek/rtl8366rb.c +++ b/drivers/net/dsa/realtek/rtl8366rb.c @@ -24,6 +24,7 @@ #include "realtek.h" #include "realtek-smi.h" #include "realtek-mdio.h" +#include "realtek-common.h" #define RTL8366RB_PORT_NUM_CPU 5 #define RTL8366RB_NUM_PORTS 6 @@ -1707,7 +1708,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum) if (phy > RTL8366RB_PHY_NO_MAX) return -EINVAL; - mutex_lock(&priv->map_lock); + realtek_common_lock(priv); ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG, RTL8366RB_PHY_CTRL_READ); @@ -1735,7 +1736,7 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum) phy, regnum, reg, val); out: - mutex_unlock(&priv->map_lock); + realtek_common_unlock(priv); return ret; } @@ -1749,7 +1750,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, if (phy > RTL8366RB_PHY_NO_MAX) return -EINVAL; - mutex_lock(&priv->map_lock); + realtek_common_lock(priv); ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG, RTL8366RB_PHY_CTRL_WRITE); @@ -1766,7 +1767,7 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, goto out; out: - mutex_unlock(&priv->map_lock); + realtek_common_unlock(priv); return ret; }
Some code can be shared between both interface modules (MDIO and SMI) and among variants. These interface functions migrated to a common module: - realtek_common_lock - realtek_common_unlock - realtek_common_probe - realtek_common_register_switch - realtek_common_remove The reset during probe was moved to the end of the common probe. This way, we avoid a reset if anything else fails. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/Makefile | 2 + drivers/net/dsa/realtek/realtek-common.c | 132 +++++++++++++++++++++++ drivers/net/dsa/realtek/realtek-common.h | 16 +++ drivers/net/dsa/realtek/realtek-mdio.c | 114 +++----------------- drivers/net/dsa/realtek/realtek-smi.c | 117 ++++---------------- drivers/net/dsa/realtek/realtek.h | 6 +- drivers/net/dsa/realtek/rtl8365mb.c | 9 +- drivers/net/dsa/realtek/rtl8366rb.c | 9 +- 8 files changed, 194 insertions(+), 211 deletions(-) create mode 100644 drivers/net/dsa/realtek/realtek-common.c create mode 100644 drivers/net/dsa/realtek/realtek-common.h