[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240125104524.vfkoztu4kcabxdlc@skbuf>
Date: Thu, 25 Jan 2024 12:45:24 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: netdev@...r.kernel.org, linus.walleij@...aro.org, alsi@...g-olufsen.dk,
andrew@...n.ch, f.fainelli@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
arinc.unal@...nc9.com, ansuelsmth@...il.com
Subject: Re: [PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx
module
On Tue, Jan 23, 2024 at 06:55:57PM -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:
>
> - rtl83xx_lock
> - rtl83xx_unlock
> - rtl83xx_probe
> - rtl83xx_register_switch
For API uniformity, I would also introduce rtl83xx_unregister_switch()
to make it clear that there is a teardown step associated with this.
> - rtl83xx_remove
No rtl83xx_shutdown()?
You could centralize the comments from the mdio and smi interfaces into
the rtl83xx common layer.
>
> 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@...il.com>
> ---
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 57bd5d8814c2..26b8371ecc87 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -303,3 +194,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_mdio_shutdown, REALTEK_DSA);
> MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
> MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(REALTEK_DSA);
> +
Stray blank line at the end of the file.
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 274dd96b099c..840b1a835d07 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -575,3 +473,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_smi_shutdown, REALTEK_DSA);
> MODULE_AUTHOR("Linus Walleij <linus.walleij@...aro.org>");
> MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
> MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(REALTEK_DSA);
> +
Likewise.
> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> new file mode 100644
> index 000000000000..57d185226b03
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/rtl83xx.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/module.h>
> +
> +#include "realtek.h"
> +#include "rtl83xx.h"
> +
> +/**
> + * rtl83xx_lock() - Locks the mutex used by regmaps
> + * @ctx: realtek_priv pointer
> + *
> + * This function is passed to regmap to be used as the lock function.
> + * It is also used externally to block regmap before executing multiple
> + * operations that must happen in sequence (which will use
> + * realtek_priv.map_nolock instead).
> + *
> + * Context: Any context. Holds priv->map_lock lock.
Not "any context", but "sleepable context". You cannot acquire a mutex
in atomic context. Actually this applies across the board in this patch
set. The entry points into DSA also take mutex_lock(&dsa2_mutex), so
this also applies to its callers' context..
> + * Return: nothing
> + */
> +void rtl83xx_lock(void *ctx)
> +{
> + struct realtek_priv *priv = ctx;
> +
> + mutex_lock(&priv->map_lock);
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_lock, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_unlock() - Unlocks the mutex used by regmaps
> + * @ctx: realtek_priv pointer
> + *
> + * This function unlocks the lock acquired by rtl83xx_lock.
> + *
> + * Context: Any context. Releases priv->map_lock lock
> + * Return: nothing
> + */
> +void rtl83xx_unlock(void *ctx)
> +{
> + struct realtek_priv *priv = ctx;
> +
> + mutex_unlock(&priv->map_lock);
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_probe() - probe a Realtek switch
> + * @dev: the device being probed
> + * @interface_info: reg read/write methods for a specific management interface.
Leave this description more open-ended, otherwise it will have to be
modified when struct realtek_interface_info gains more fields.
> + *
> + * This function initializes realtek_priv and read data from the device-tree
s/read/reads/
s/device-tree/device tree/
> + * node. The switch is hard resetted if a method is provided.
> + *
> + * Context: Any context.
> + * Return: Pointer to the realtek_priv or ERR_PTR() in case of failure.
> + *
> + * The realtek_priv pointer does not need to be freed as it is controlled by
> + * devres.
> + *
> + */
> +struct realtek_priv *
> +rtl83xx_probe(struct device *dev,
> + const struct realtek_interface_info *interface_info)
> +{
> + const struct realtek_variant *var;
> + struct realtek_priv *priv;
> + struct regmap_config rc = {
> + .reg_bits = 10, /* A4..A0 R4..R0 */
> + .val_bits = 16,
> + .reg_stride = 1,
> + .max_register = 0xffff,
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .reg_read = interface_info->reg_read,
> + .reg_write = interface_info->reg_write,
> + .cache_type = REGCACHE_NONE,
> + .lock = rtl83xx_lock,
> + .unlock = rtl83xx_unlock,
Too many levels of indentation.
> + };
> + 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);
> + }
> +
> + 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);
> + }
> +
> + /* 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_NS_GPL(rtl83xx_probe, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_register_switch() - detects and register a switch
> + * @priv: realtek_priv pointer
> + *
> + * This function first checks the switch chip ID and register a DSA
> + * switch.
> + *
> + * Context: Any context. Takes and releases priv->map_lock.
> + * Return: 0 on success, negative value for failure.
> + */
> +int rtl83xx_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_NS_GPL(rtl83xx_register_switch, REALTEK_DSA);
> +
> +/**
> + * rtl83xx_remove() - Cleanup a realtek switch driver
> + * @ctx: realtek_priv pointer
s/ctx/priv/
> + *
> + * If a method is provided, this function asserts the hard reset of the switch
> + * in order to avoid leaking traffic when the driver is gone.
> + *
> + * Context: Any context.
> + * Return: nothing
> + */
> +void rtl83xx_remove(struct realtek_priv *priv)
> +{
> + if (!priv)
> + return;
> +
> + /* leave the device reset asserted */
> + if (priv->reset)
> + gpiod_set_value(priv->reset, 1);
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
> +
> +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> +MODULE_DESCRIPTION("Realtek DSA switches common module");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> new file mode 100644
> index 000000000000..9eb8197a58fa
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/rtl83xx.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _RTL83XX_H
> +#define _RTL83XX_H
> +
> +#include <linux/regmap.h>
I don't think anything from this header needs regmap.h?
For testing what is needed, you can create a dummy.c file which includes
only rtl83xx.h, and add just the necessary headers so that dummy.c
builds cleanly.
> +
> +struct realtek_interface_info {
> + int (*reg_read)(void *ctx, u32 reg, u32 *val);
> + int (*reg_write)(void *ctx, u32 reg, u32 val);
> +};
> +
> +void rtl83xx_lock(void *ctx);
> +void rtl83xx_unlock(void *ctx);
> +struct realtek_priv *
> +rtl83xx_probe(struct device *dev,
> + const struct realtek_interface_info *interface_info);
> +int rtl83xx_register_switch(struct realtek_priv *priv);
> +void rtl83xx_remove(struct realtek_priv *priv);
> +
> +#endif /* _RTL83XX_H */
> --
> 2.43.0
>
Powered by blists - more mailing lists