Received-SPF: pass (google.com: domain of linux-kernel+bounces-67936-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 16 Feb 2024 03:15:45 +0200
From: Vladimir Oltean <olteanv@gmail.com>
To: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Alvin =?utf-8?Q?=C5=A0ipraga?= <alsi@bang-olufsen.dk>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@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
Subject: Re: [PATCH net-next v3 3/3] net: dsa: realtek: support reset
 controller
Message-ID: <20240216011545.7fmup2qkqikvb7ro@skbuf>
References: <20240213-realtek-reset-v3-0-37837e574713@gmail.com>
 <20240213-realtek-reset-v3-3-37837e574713@gmail.com>
 <CACRpkdZELbOmZieZTDLfA81VuThM7h399BWtuQuQ6U7o8Xb7LA@mail.gmail.com>
 <CAJq09z4wgPo=1_OtA6Y-0O4gLJ2nxy1jdf0BDoZwVmL=TkOdUg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJq09z4wgPo=1_OtA6Y-0O4gLJ2nxy1jdf0BDoZwVmL=TkOdUg@mail.gmail.com>

On Wed, Feb 14, 2024 at 07:17:55PM -0300, Luiz Angelo Daros de Luca wrote:
> > On Wed, Feb 14, 2024 at 1:54 AM 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 = 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.

In the defense of Linus' comment, your proposed code did not do what you
seem to have intended anyway. If priv->reset_ctl was non-NULL, it fell
through and potentially ran gpiod_set_value() anyway - there was no
early "return". So, the simplification comment was predictable.