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

[Uwe Kleine-König]

Hello Sergei,

On Mon, Mar 21, 2016 at 11:15:13PM +0300, Sergei Shtylyov wrote:
> On 03/16/2016 08:25 PM, Sebastian Frias 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).

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).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |