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

Andrew Lunn
2022-02-11T19:28:11Z

On Wed, Feb 09, 2022 at 04:34:15PM +0000, Måns Rullgård wrote:
> Andrew Lunn <andrew@lunn.ch> 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@mansr.com>
> >> ---
> >> 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@lunn.ch>

Andrew