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 <s...@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/ |
