On Tue, Feb 20, 2024 at 09:22:44AM -0300, Luiz Angelo Daros de Luca wrote:
> Hi Alvin,
>
> > 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.
>
> I made a mistake. I should be
>
> if (ret) {
> dev_warn...
> }
>
> not returning on error (as you suggested below).
>
> I was sure I was doing just that... I was surprised to see it as it
> is. I'll recheck my branch with all the integrated changes. It passed
> my tests as when reset is missed, it normally does not matter. Thanks
> for the catch.
>
> >
> > > +
> > > + 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?
>
> Because it does not return a value. There is no way to tell if it failed.
Ah ok, nevermind that part then.
BTW, please use gpiod_set_value_cansleep(). With that I think this is good.
