Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Oct 24, 2023 at 05:58:04PM -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.
> 
> This is an example of how to use the reset controller:
> 
>         switch {
>                 compatible = "realtek,rtl8366rb";
> 
>                 resets = <&rst 8>;
>                 reset-names = "switch";
> 
> 		...
> 	}

Mix of tabs and spaces here.
Also, examples belong to the dt-schema.

> 
> 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 | 36 +++++++++++++++++++++-----
>  drivers/net/dsa/realtek/realtek-smi.c  | 34 +++++++++++++++++++-----
>  drivers/net/dsa/realtek/realtek.h      |  6 +++++
>  3 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..600124c58c00 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -140,6 +140,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
>  	.disable_locking = true,
>  };
>  
> +static int realtek_mdio_hwreset(struct realtek_priv *priv, bool active)
> +{
> +#ifdef CONFIG_RESET_CONTROLLER
> +	if (priv->reset_ctl) {
> +		if (active)
> +			return reset_control_assert(priv->reset_ctl);
> +		else
> +			return reset_control_deassert(priv->reset_ctl);
> +	}
> +#endif
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, active);
> +
> +	return 0;
> +}
> +

This "bool active" artificially unifies two discrete code paths in the
same function, where the callers are not the same and the implementation
is not the same (given a priv->reset_ctl presence), separated by an "if".

Would it make more sense to have discrete functions, each with its
unique caller, like this?

static int realtek_reset_assert(struct realtek_priv *priv)
{
	if (priv->reset_ctl)
		return reset_control_assert(priv->reset_ctl);

	if (priv->reset)
		gpiod_set_value(priv->reset, 1);

	return 0;
}

static int realtek_reset_deassert(struct realtek_priv *priv)
{
	if (priv->reset_ctl)
		return reset_control_deassert(priv->reset_ctl);

	if (priv->reset)
		gpiod_set_value(priv->reset, 0);

	return 0;
}

Also, you return int but ignore error values everywhere. I guess it
would make more sense to return void, but print warnings within the
reset functions if the calls to the reset control fail.

>  static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv;
> @@ -194,20 +211,26 @@ 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 */

I'm not sure if "power" and "reset" are the same thing...

>  	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
>  
> +#ifdef CONFIG_RESET_CONTROLLER
> +	priv->reset_ctl = devm_reset_control_get(dev, "switch");
> +	if (IS_ERR(priv->reset_ctl)) {
> +		dev_err(dev, "failed to get switch reset control\n");
> +		return PTR_ERR(priv->reset_ctl);

		ret = PTR_ERR(priv->reset_ctl);
		return dev_err_probe(dev, err, "failed to get reset control\n");

This suppresses -EPROBE_DEFER prints.

> +	}
> +#endif
> +
>  	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_hwreset(priv, 1);
>  		dev_dbg(dev, "asserted RESET\n");
>  		msleep(REALTEK_HW_STOP_DELAY);
> -		gpiod_set_value(priv->reset, 0);
> +		realtek_mdio_hwreset(priv, 0);
>  		msleep(REALTEK_HW_START_DELAY);
>  		dev_dbg(dev, "deasserted RESET\n");
>  	}
> @@ -246,8 +269,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_hwreset(priv, 1);

nitpick: "bool" arguments should take "true" or "false".

>  }
>  
>  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..751159d71223 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -408,6 +408,23 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  	return ret;
>  }
>  
> +static int realtek_smi_hwreset(struct realtek_priv *priv, bool active)
> +{
> +#ifdef CONFIG_RESET_CONTROLLER
> +	if (priv->reset_ctl) {
> +		if (active)
> +			return reset_control_assert(priv->reset_ctl);
> +		else
> +			return reset_control_deassert(priv->reset_ctl);
> +	}
> +#endif
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, active);
> +
> +	return 0;
> +}

What is the reason for duplicating realtek_mdio_hwreset()?