Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB | From | Sergei Shtylyov <> | Date | Tue, 22 Mar 2016 00:56:49 +0300 |
| |
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@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).
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
| ![\](/images/icornerr.gif) |