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

[Alvin Šipraga]

On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote:
> +void rtl83xx_reset_assert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	ret = reset_control_assert(priv->reset_ctl);
> +	if (!ret)
> +		return;

If priv->reset_ctl is NULL - i.e. if no DT property is specified - then
this will always return early and the GPIO will not be asserted.

> +
> +	dev_warn(priv->dev,
> +		 "Failed to assert the switch reset control: %pe\n",
> +		 ERR_PTR(ret));

You only log an error if the reset controller assert fails, but not if
the GPIO assert fails. Why the unequal treatment?

I suggest keeping it simple:

void rtl83xx_reset_assert(struct realtek_priv *priv)
{
  int ret;

  ret = reset_control_assert(priv->reset_ctl);
  if (ret)
    dev_warn(priv->dev, "failed to assert reset control: %d\n", ret);

  ret = gpiod_set_value(priv->reset, false);
  if (ret)
    dev_warn(priv->dev, "failed to assert reset GPIO: %d\n", ret);
}

or even drop the warnings altogether.

> +
> +	gpiod_set_value(priv->reset, true);
> +}
> +
> +void rtl83xx_reset_deassert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	ret = reset_control_deassert(priv->reset_ctl);
> +	if (!ret)
> +		return;
> +
> +	dev_warn(priv->dev,
> +		 "Failed to deassert the switch reset control: %pe\n",
> +		 ERR_PTR(ret));
> +
> +	gpiod_set_value(priv->reset, false);
> +}

Same comments apply to this function. Just deassert both.

> +
>  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>");
>  MODULE_AUTHOR("Linus Walleij <linus.walleij@xxxxxxxxxx>");
>  MODULE_DESCRIPTION("Realtek DSA switches common module");
> diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> index 0ddff33df6b0..c8a0ff8fd75e 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.h
> +++ b/drivers/net/dsa/realtek/rtl83xx.h
> @@ -18,5 +18,7 @@ int rtl83xx_register_switch(struct realtek_priv *priv);
>  void rtl83xx_unregister_switch(struct realtek_priv *priv);
>  void rtl83xx_shutdown(struct realtek_priv *priv);
>  void rtl83xx_remove(struct realtek_priv *priv);
> +void rtl83xx_reset_assert(struct realtek_priv *priv);
> +void rtl83xx_reset_deassert(struct realtek_priv *priv);
>  
>  #endif /* _RTL83XX_H */
> 
> -- 
> 2.43.0
>