Received-SPF: pass (google.com: domain of linux-kernel+bounces-66033-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240213-realtek-reset-v3-0-37837e574713@gmail.com>
 <20240213-realtek-reset-v3-3-37837e574713@gmail.com> <CACRpkdZELbOmZieZTDLfA81VuThM7h399BWtuQuQ6U7o8Xb7LA@mail.gmail.com>
In-Reply-To: <CACRpkdZELbOmZieZTDLfA81VuThM7h399BWtuQuQ6U7o8Xb7LA@mail.gmail.com>
From: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Date: Wed, 14 Feb 2024 19:17:55 -0300
Message-ID: <CAJq09z4wgPo=1_OtA6Y-0O4gLJ2nxy1jdf0BDoZwVmL=TkOdUg@mail.gmail.com>
Subject: Re: [PATCH net-next v3 3/3] net: dsa: realtek: support reset controller
To: Linus Walleij <linus.walleij@linaro.org>
Cc: =?UTF-8?Q?Alvin_=C5=A0ipraga?= <alsi@bang-olufsen.dk>, 
	Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>, 
	Vladimir Oltean <olteanv@gmail.com>, "David S. Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Philipp Zabel <p.zabel@pengutronix.de>, netdev@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

> Hi Luiz,

Hi Linus,

> thanks for your patch!

I'm glad to help ;-)

> On Wed, Feb 14, 2024 at 1:54=E2=80=AFAM Luiz Angelo Daros de Luca
> <luizluca@gmail.com> 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>
> (...)
> > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > +{
> > +       int ret;
> > +
> > +       if (priv->reset_ctl) {
> > +               ret =3D 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: %p=
e\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@linaro.org>
>
> Yours,
> Linus Walleij