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

[Luiz Angelo Daros de Luca]

> Hi Luiz,

Hi Linus,

> thanks for your patch!

I'm glad to help ;-)

> On Wed, Feb 14, 2024 at 1:54 AM Luiz Angelo Daros de Luca
> <luizluca@xxxxxxxxx> 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@xxxxxxxxx>
> (...)
> > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > +{
> > +       int ret;
> > +
> > +       if (priv->reset_ctl) {
> > +               ret = reset_control_assert(priv->reset_ctl);
>
> Actually, reset_control_assert() is NULL-tolerand (as well as the
> stubs) so you can just issue this unconditionally. If priv->reset_ctl
> is NULL it will just return 0.

The idea was to avoid gpiod_set_value if the reset_control_assert was
configured and worked. However, I don't see a big issue in calling
them both as you don't expect both to be configured.
I'll remove the "ifs not null" and let both be called.

>
> > +               if (!ret)
> > +                       return;
> > +
> > +               dev_warn(priv->dev,
> > +                        "Failed to assert the switch reset control: %pe\n",
> > +                        ERR_PTR(ret));
> > +       }
> > +
> > +       if (priv->reset)
> > +               gpiod_set_value(priv->reset, true);
>
> Same here! Also NULL-tolerant. You do not need to check priv->reset.
> Just issue it.
>
> > +void rtl83xx_reset_deassert(struct realtek_priv *priv)
>
> Same comments for deassert.
>
> With this fixed (the rest looks just fine):
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>
> Yours,
> Linus Walleij