Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB

[Sergei Shtylyov]

On 03/21/2016 11:41 PM, Uwe Kleine-König wrote:

> > > Commit 687908c2b649 ("net: phy: at803x: simplify using
> > > devm_gpiod_get_optional and its 4th argument") introduced a dependency
> > > on GPIOLIB that was not there before.
> > >
> > > This commit removes such dependency by checking the return code and
> > > comparing it against ENOSYS which is returned when GPIOLIB is not
> > > selected.
> > >
> > > Fixes: 687908c2b649 ("net: phy: at803x: simplify using
> > > devm_gpiod_get_optional and its 4th argument")
> > >
> > > Signed-off-by: Sebastian Frias <sf84@xxxxxxxxxxx>
> >
> >     Do you have the PHY that requires the GPIO reset workaround?
> > Askinjg because I have the patch adding the "reset-gpios" prop handling to
> > phylib and your patch made me aware that I'll have to modify this driver in
> > order to do that...
> >
> > > ---
> > >   drivers/net/phy/at803x.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > > index 2174ec9..88b7ff3 100644
> > > --- a/drivers/net/phy/at803x.c
> > > +++ b/drivers/net/phy/at803x.c
> > > @@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
> > >   		return -ENOMEM;
> > >
> > >   	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > -	if (IS_ERR(gpiod_reset))
> > > +	if (PTR_ERR(gpiod_reset) == -ENOSYS)
> > > +		gpiod_reset = NULL;
> > > +	else if (IS_ERR(gpiod_reset))
> >     		return PTR_ERR(gpiod_reset);
> >
> >     My patch basically gets rid of all this code. The thing that worries me
> > is that the driver assumes that the reset singal is active low, despite what
> > the GPIO specifier in the device tree has for the GPIO polarity. In fact, it
> > will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is
> > wrong because the reset signal is active low!
>
> Note that gpio descriptors handle the polarity just fine (i.e. the pin
> is set to 0 after doing gpiod_set_value(1) if the gpio is active low).

   I know. :-)

> But having said that, the driver gets it wrong.
>
> The right sequence to reset a device using a gpio is:
>
> 	gpiod_set_value(priv->gpiod_reset, 1);
> 	msleep(some_time);
> 	gpiod_set_value(priv->gpiod_reset, 0);
>
> and if the gpio is active low, this should be specified in the device
> tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support
> for hardware reset).

   I wonder if that was done before GPIO_ACTIVE_* thing was introduced... 
there are precedents in other MAC drivers that want a separate 
"phy-reset-active-low" or even -"high" prop...

> Best regards
> Uwe

MBR, Sergei