Re: [PATCH] net: dsa: lan9303: fix reset on probe

[Andrew Lunn]

On Wed, Feb 09, 2022 at 04:34:15PM +0000, Måns Rullgård wrote:
> Andrew Lunn <andrew@xxxxxxx> writes:
> 
> > On Wed, Feb 09, 2022 at 02:54:54PM +0000, Mans Rullgard wrote:
> >> The reset input to the LAN9303 chip is active low, and devicetree
> >> gpio handles reflect this.  Therefore, the gpio should be requested
> >> with an initial state of high in order for the reset signal to be
> >> asserted.  Other uses of the gpio already use the correct polarity.
> >> 
> >> Signed-off-by: Mans Rullgard <mans@xxxxxxxxx>
> >> ---
> >>  drivers/net/dsa/lan9303-core.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> >> index aa1142d6a9f5..2de67708bbd2 100644
> >> --- a/drivers/net/dsa/lan9303-core.c
> >> +++ b/drivers/net/dsa/lan9303-core.c
> >> @@ -1301,7 +1301,7 @@ static int lan9303_probe_reset_gpio(struct lan9303 *chip,
> >>  				     struct device_node *np)
> >>  {
> >>  	chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "reset",
> >> -						   GPIOD_OUT_LOW);
> >> +						   GPIOD_OUT_HIGH);
> >>  	if (IS_ERR(chip->reset_gpio))
> >>  		return PTR_ERR(chip->reset_gpio);
> >
> > lan9303_handle_reset() does a sleep and then releases the reset. I
> > don't see anywhere in the driver which asserts the reset first. So is
> > it actually asserted as part of this getting the GPIO? And if so, does
> > not this change actually break the reset?
> 
> The GPIOD_OUT_xxx flags to gpiod_get() request that the pin be
> configured as output and set to high/low initially.  The GPIOD_OUT_LOW
> currently used by the lan9303 driver together with GPIO_ACTIVE_LOW in
> the devicetrees results in the actual voltage being set high.  The
> driver then sleeps for a bit before setting the gpio value to zero,
> again translated to a high output voltage.  That is, the value set after
> the sleep is the same as it was initially.  This is obviously not the
> intent.

Yes, i agree. I'm just wondering how this worked for whoever
implemented this code. I guess it never actually did a reset, or the
bootloader left the reset already in the asserted state, so that the
gpiod_get() actual deasserted the reset?

Reviewed-by: Andrew Lunn <andrew@xxxxxxx>

    Andrew