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

Vladimir Oltean
2024-01-08T14:00:08Z

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?