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

[Vladimir Oltean <olteanv@gmail.com>]

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?