On Tue, Feb 20, 2024 at 01:30:33PM +0000, Alvin Šipraga wrote: > 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. OK, actually the original code wasn't doing that, so not crucial for this change. It can be done in a follow-up.