Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1362134 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-03-21 21:20 +0100
    Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2016-03-21 21:50 +0100
      Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-03-21 23:00 +0100
      Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB Sebastian Frias <sf84@laposte.net> - 2016-03-22 16:00 +0100
    Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB Sebastian Frias <sf84@laposte.net> - 2016-03-22 15:40 +0100

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

Hello.

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!
    Can you point me to an actual device tree using this erratic PHY?

WBR, Sergei

[toc] | [next] | [standalone]

#1362143

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/  |

[toc] | [prev] | [next] | [standalone]

#1362169

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

[toc] | [prev] | [next] | [standalone]

#1362803

Hi,

On 03/21/2016 09:41 PM, Uwe Kleine-König wrote:
>>    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).
> 

Isn't that source of bugs?
What about using some #define (or probably better, an enum)?, something
like:

    gpiod_set_value(gpiod, GPIO_SET_VALUE_ACTIVE)
    gpiod_set_value(gpiod, GPIO_SET_VALUE_INACTIVE)
    gpiod_set_value(gpiod, GPIO_SET_VALUE_TRISTATE)

then, somebody reading the code would have to stop and think what do
these mean.
IIUC, currently the "0" or "1" can easily be confused with the actual
logical value of the GPIO.

gpiod_set_value() could also return an int with the actual value it
applied to the GPIO.
For example: if gpiod is active low, gpiod_set_value(gpiod,
GPIO_SET_VALUE_ACTIVE) would return 0;
Conversely, if gpiod is active high, gpiod_set_value(gpiod,
GPIO_SET_VALUE_ACTIVE) would return 1;

Best regards,

Sebastian

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

[toc] | [prev] | [next] | [standalone]

#1362784

Hi Sergei,

On 03/21/2016 09:15 PM, Sergei Shtylyov wrote:
> 
>    Do you have the PHY that requires the GPIO reset workaround?

Unfortunately (or luckily :-) ) I don't have the faulty PHY, sorry.

Best regards,

Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web