> 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
