lkml.org 
[lkml]   [2016]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From
Date
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

\
 
 \ /
  Last update: 2016-03-21 23:21    [W:0.081 / U:2.552 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site