Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Oct 27, 2023 at 04:00:57PM -0300, Luiz Angelo Daros de Luca wrote:
> The 'reset-gpios' will not work when the switch reset is controlled by a
> reset controller.
> 
> Although the reset is optional and the driver performs a soft reset
> during setup, if the initial reset state was asserted, the driver will
> not detect it.
> 
> The reset controller will take precedence over the reset GPIO.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++----
>  drivers/net/dsa/realtek/realtek-smi.c  | 49 ++++++++++++++++++++++---
>  drivers/net/dsa/realtek/realtek.h      |  2 +
>  3 files changed, 89 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..aad94e49d4c9 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
>  	.disable_locking = true,
>  };
>  
> +static void realtek_mdio_reset_assert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_assert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> +				 ret);

Instead of "Error: %i" you can say ".. reset control: %pe\n", ERR_PTR(ret)
which will print the error as a symbolic error name (if CONFIG_SYMBOLIC_ERRNAME=y)
rather than just a numeric value.

Also, I don't know if this is explicit in the coding style, but I
believe it is more consistent if single function calls are enveloped in
curly braces if they span multiple lines, like so:

		if (ret) {
			dev_warn(priv->dev,
				 "Failed to assert the switch reset control: %pe",
				 ERR_PTR(ret));
		}

Also, please note that netdev still prefers the 80 character line limit.

> +
> +		return;
> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, true);
> +}
> +
> +static void realtek_mdio_reset_deassert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_deassert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> +				 ret);
> +
> +		return;

Is there a particular reason why this has to ignore a reset GPIO if
present, rather than fall through, checking for the latter as well?

> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, false);
> +}
> +
>  static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv;
> @@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  
>  	dev_set_drvdata(dev, priv);
>  
> -	/* TODO: if power is software controlled, set up any regulators here */

As Andrew mentions, this commit does not make power software-controlled,
so don't remove this.

>  	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
>  
> +	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> +	if (IS_ERR(priv->reset_ctl)) {
> +		ret = PTR_ERR(priv->reset_ctl);
> +		return dev_err_probe(dev, ret, "failed to get reset control\n");
> +	}
> +
>  	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);
> +	if (priv->reset_ctl || priv->reset) {
> +		realtek_mdio_reset_assert(priv);
>  		dev_dbg(dev, "asserted RESET\n");
>  		msleep(REALTEK_HW_STOP_DELAY);
> -		gpiod_set_value(priv->reset, 0);
> +		realtek_mdio_reset_deassert(priv);
>  		msleep(REALTEK_HW_START_DELAY);
>  		dev_dbg(dev, "deasserted RESET\n");
>  	}
> @@ -246,8 +284,7 @@ static 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_mdio_reset_assert(priv);
>  }
>  
>  static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index bfd11591faf4..a99e53b5b662 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  	return ret;
>  }
>  
> +static void realtek_smi_reset_assert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_assert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> +				 ret);
> +
> +		return;
> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, true);
> +}
> +
> +static void realtek_smi_reset_deassert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_deassert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> +				 ret);
> +
> +		return;
> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, false);
> +}
> +

To respond here, in a single email, to your earlier question (sorry):
https://lore.kernel.org/netdev/CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com/

| Both interface modules, realtek-smi and realtek-mdio, do not share
| code, except for the realtek.h header file. I don't know if it is
| worth it to put the code in a new shared module. What is the best
| practice here? Create a realtek_common.c linked to both modules?

The answer is: I ran "meld" between realtek-mdio.c and realtek-smi.c,
and the probe, remove and shutdown functions are surprisingly similar
already, and perhaps might become even more similar in the future.
I think it is worth introducing a common kernel module for both
interface drivers as a preliminary patch, rather than keeping duplicated
probe/remove/shutdown code.

>  static int realtek_smi_probe(struct platform_device *pdev)
>  {
>  	const struct realtek_variant *var;
> @@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  	dev_set_drvdata(dev, priv);
>  	spin_lock_init(&priv->lock);
>  
> -	/* TODO: if power is software controlled, set up any regulators here */
> +	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> +	if (IS_ERR(priv->reset_ctl)) {
> +		ret = PTR_ERR(priv->reset_ctl);
> +		return dev_err_probe(dev, ret, "failed to get reset control\n");
> +	}
>  
>  	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);
> +	if (priv->reset_ctl || priv->reset) {
> +		realtek_smi_reset_assert(priv);
>  		dev_dbg(dev, "asserted RESET\n");
>  		msleep(REALTEK_HW_STOP_DELAY);
> -		gpiod_set_value(priv->reset, 0);
> +		realtek_smi_reset_deassert(priv);
>  		msleep(REALTEK_HW_START_DELAY);
>  		dev_dbg(dev, "deasserted RESET\n");
>  	}
> @@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
>  		of_node_put(priv->slave_mii_bus->dev.of_node);

slave_mii_bus was renamed to user_mii_bus, and this prevents the
application of the patch currently, so you will need to respin. But I
think net-next is going to close soon for 2 weeks, so either you respin
as RFC or you wait until it reopens.

>  
>  	/* leave the device reset asserted */
> -	if (priv->reset)
> -		gpiod_set_value(priv->reset, 1);
> +	realtek_smi_reset_assert(priv);
>  }
>