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

[Sebastian Frias <sf84@laposte.net>]

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
>