Date: Mon, 21 Mar 2016 21:41:11 +0100
From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
	<u.kleine-koenig@pengutronix.de>
To: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: Sebastian Frias <sf84@laposte.net>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        lkml <linux-kernel@vger.kernel.org>, mason <slash.tmp@free.fr>,
        Daniel Mack <zonque@gmail.com>
Subject: Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
Message-ID: <20160321204111.GB6191@pengutronix.de>
References: <56E99727.9040702@laposte.net>
 <56F05651.3030706@cogentembedded.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <56F05651.3030706@cogentembedded.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2545
Lines: 68

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@laposte.net>
> 
>    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/  |