[PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Guenter Roeck]

The gpio include file and the gpio documentation declare and document
GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
and related functions. However, the flag is not evaluated or used.

Check the flag in gpio_request_one and set the gpio internal flag
FLAG_ACTIVE_LOW if it is set.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/gpio/gpiolib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 2ebc907..43d9e34 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1842,6 +1842,9 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (flags & GPIOF_OPEN_SOURCE)
 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
+	if (flags & GPIOF_ACTIVE_LOW)
+		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+
 	if (flags & GPIOF_DIR_IN)
 		err = gpiod_direction_input(desc);
 	else
-- 
1.9.1

Re: [PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Alexandre Courbot]

On Thu, Jul 17, 2014 at 8:11 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> The gpio include file and the gpio documentation declare and document
> GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
> and related functions. However, the flag is not evaluated or used.
>
> Check the flag in gpio_request_one and set the gpio internal flag
> FLAG_ACTIVE_LOW if it is set.

What is the point since the integer GPIO API has no clue of the
active-low status of a GPIO? It is only used by the gpiod and sysfs
interfaces.

>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/gpio/gpiolib.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 2ebc907..43d9e34 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1842,6 +1842,9 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
>         if (flags & GPIOF_OPEN_SOURCE)
>                 set_bit(FLAG_OPEN_SOURCE, &desc->flags);
>
> +       if (flags & GPIOF_ACTIVE_LOW)
> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +
>         if (flags & GPIOF_DIR_IN)
>                 err = gpiod_direction_input(desc);
>         else
> --
> 1.9.1
>

Re: [PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Guenter Roeck]

On 07/16/2014 11:09 PM, Alexandre Courbot wrote:
> On Thu, Jul 17, 2014 at 8:11 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> The gpio include file and the gpio documentation declare and document
>> GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
>> and related functions. However, the flag is not evaluated or used.
>>
>> Check the flag in gpio_request_one and set the gpio internal flag
>> FLAG_ACTIVE_LOW if it is set.
>
> What is the point since the integer GPIO API has no clue of the
> active-low status of a GPIO? It is only used by the gpiod and sysfs
> interfaces.
>

One can use gpio_request_one() to export a gpio pin to user space from
the kernel. That code path does use the flag, as you point out yourself above.

One could also argue that the integer gpio API _should_ support this as well,
but that is a different question.

Guenter

>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/gpio/gpiolib.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 2ebc907..43d9e34 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1842,6 +1842,9 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
>>          if (flags & GPIOF_OPEN_SOURCE)
>>                  set_bit(FLAG_OPEN_SOURCE, &desc->flags);
>>
>> +       if (flags & GPIOF_ACTIVE_LOW)
>> +               set_bit(FLAG_ACTIVE_LOW, &desc->flags);
>> +
>>          if (flags & GPIOF_DIR_IN)
>>                  err = gpiod_direction_input(desc);
>>          else
>> --
>> 1.9.1
>>
>
>

Re: [PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Alexandre Courbot]

On Thu, Jul 17, 2014 at 3:37 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/16/2014 11:09 PM, Alexandre Courbot wrote:
>>
>> On Thu, Jul 17, 2014 at 8:11 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> The gpio include file and the gpio documentation declare and document
>>> GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
>>> and related functions. However, the flag is not evaluated or used.
>>>
>>> Check the flag in gpio_request_one and set the gpio internal flag
>>> FLAG_ACTIVE_LOW if it is set.
>>
>>
>> What is the point since the integer GPIO API has no clue of the
>> active-low status of a GPIO? It is only used by the gpiod and sysfs
>> interfaces.
>>
>
> One can use gpio_request_one() to export a gpio pin to user space from
> the kernel. That code path does use the flag, as you point out yourself
> above.

Ok, in that case I suppose it makes sense.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

> One could also argue that the integer gpio API _should_ support this as
> well,
> but that is a different question.

Probably not going to happen. The integer GPIO interface is deprecated
and users who need new features should seriously consider switching to
gpiod.

Re: [PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Guenter Roeck]

On 07/17/2014 12:26 AM, Alexandre Courbot wrote:
> On Thu, Jul 17, 2014 at 3:37 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 07/16/2014 11:09 PM, Alexandre Courbot wrote:
>>>
>>> On Thu, Jul 17, 2014 at 8:11 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> The gpio include file and the gpio documentation declare and document
>>>> GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
>>>> and related functions. However, the flag is not evaluated or used.
>>>>
>>>> Check the flag in gpio_request_one and set the gpio internal flag
>>>> FLAG_ACTIVE_LOW if it is set.
>>>
>>>
>>> What is the point since the integer GPIO API has no clue of the
>>> active-low status of a GPIO? It is only used by the gpiod and sysfs
>>> interfaces.
>>>
>>
>> One can use gpio_request_one() to export a gpio pin to user space from
>> the kernel. That code path does use the flag, as you point out yourself
>> above.
>
> Ok, in that case I suppose it makes sense.
>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>
>> One could also argue that the integer gpio API _should_ support this as
>> well,
>> but that is a different question.
>
> Probably not going to happen. The integer GPIO interface is deprecated
> and users who need new features should seriously consider switching to
> gpiod.
>

The new API is unfortunately not equivalent to the old one. For example,
if I understand correctly, gpiod_get is expected to be used instead of
gpio_request_one. That may work nicely in a world with full DT or ACPI
support, but doesn't work as well otherwise unless one drops the notion
of using platform specific drivers built as modules (gpiod_add_lookup_table
is not exported, and there is no remove function).

Specifically, I don't see an easy way to convert mdio-gpio to use the new
model, and that driver could really use support for an API which supports
active-low pins. And even if gpiod_add_lookup_table was supported,
converting a driver like this would be a major pain. Sure, it would be
all easy if there would be a gpiod equivalent to gpio_request_one and
gpio_request_array, but that is not the case. This makes converting
drivers from the old to the new model challenging enough that I suspect
that it won't really happen.

Guenter

Re: [PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Alexandre Courbot]

On Thu, Jul 17, 2014 at 11:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/17/2014 12:26 AM, Alexandre Courbot wrote:
>>
>> On Thu, Jul 17, 2014 at 3:37 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 07/16/2014 11:09 PM, Alexandre Courbot wrote:
>>>>
>>>>
>>>> On Thu, Jul 17, 2014 at 8:11 AM, Guenter Roeck <linux@roeck-us.net>
>>>> wrote:
>>>>>
>>>>>
>>>>> The gpio include file and the gpio documentation declare and document
>>>>> GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
>>>>> and related functions. However, the flag is not evaluated or used.
>>>>>
>>>>> Check the flag in gpio_request_one and set the gpio internal flag
>>>>> FLAG_ACTIVE_LOW if it is set.
>>>>
>>>>
>>>>
>>>> What is the point since the integer GPIO API has no clue of the
>>>> active-low status of a GPIO? It is only used by the gpiod and sysfs
>>>> interfaces.
>>>>
>>>
>>> One can use gpio_request_one() to export a gpio pin to user space from
>>> the kernel. That code path does use the flag, as you point out yourself
>>> above.
>>
>>
>> Ok, in that case I suppose it makes sense.
>>
>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>>> One could also argue that the integer gpio API _should_ support this as
>>> well,
>>> but that is a different question.
>>
>>
>> Probably not going to happen. The integer GPIO interface is deprecated
>> and users who need new features should seriously consider switching to
>> gpiod.
>>
>
> The new API is unfortunately not equivalent to the old one. For example,
> if I understand correctly, gpiod_get is expected to be used instead of
> gpio_request_one.

That is correct. The reason is to have a separate authority to assigns
GPIOs and prevent drivers from arbitrarily requesting any GPIO they
want, as long as everybody sticks to the gpiod interface.

> That may work nicely in a world with full DT or ACPI
> support, but doesn't work as well otherwise unless one drops the notion
> of using platform specific drivers built as modules (gpiod_add_lookup_table
> is not exported, and there is no remove function).
>
> Specifically, I don't see an easy way to convert mdio-gpio to use the new
> model, and that driver could really use support for an API which supports
> active-low pins.

If you want to benefit from the active-low property but cannot use
gpiod_get() for some reason, you can still request a GPIO by
gpio_request_one() and then convert it to a descriptor with
gpio_to_desc(), which is what I suspect your patch will allow you to
do. But the real fix would be to work any limitations that gpiod has
that prevent you from doing what you need.

I am not familiar with mdio-gpio - could you explain what makes it
impossible to convert this driver to the new model in your view?

Re: [PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Guenter Roeck]

On 07/21/2014 09:30 PM, Alexandre Courbot wrote:
> On Thu, Jul 17, 2014 at 11:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 07/17/2014 12:26 AM, Alexandre Courbot wrote:
>>>
>>> On Thu, Jul 17, 2014 at 3:37 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 07/16/2014 11:09 PM, Alexandre Courbot wrote:
>>>>>
>>>>>
>>>>> On Thu, Jul 17, 2014 at 8:11 AM, Guenter Roeck <linux@roeck-us.net>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> The gpio include file and the gpio documentation declare and document
>>>>>> GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
>>>>>> and related functions. However, the flag is not evaluated or used.
>>>>>>
>>>>>> Check the flag in gpio_request_one and set the gpio internal flag
>>>>>> FLAG_ACTIVE_LOW if it is set.
>>>>>
>>>>>
>>>>>
>>>>> What is the point since the integer GPIO API has no clue of the
>>>>> active-low status of a GPIO? It is only used by the gpiod and sysfs
>>>>> interfaces.
>>>>>
>>>>
>>>> One can use gpio_request_one() to export a gpio pin to user space from
>>>> the kernel. That code path does use the flag, as you point out yourself
>>>> above.
>>>
>>>
>>> Ok, in that case I suppose it makes sense.
>>>
>>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>>>
>>>> One could also argue that the integer gpio API _should_ support this as
>>>> well,
>>>> but that is a different question.
>>>
>>>
>>> Probably not going to happen. The integer GPIO interface is deprecated
>>> and users who need new features should seriously consider switching to
>>> gpiod.
>>>
>>
>> The new API is unfortunately not equivalent to the old one. For example,
>> if I understand correctly, gpiod_get is expected to be used instead of
>> gpio_request_one.
>
> That is correct. The reason is to have a separate authority to assigns
> GPIOs and prevent drivers from arbitrarily requesting any GPIO they
> want, as long as everybody sticks to the gpiod interface.
>
>> That may work nicely in a world with full DT or ACPI
>> support, but doesn't work as well otherwise unless one drops the notion
>> of using platform specific drivers built as modules (gpiod_add_lookup_table
>> is not exported, and there is no remove function).
>>
>> Specifically, I don't see an easy way to convert mdio-gpio to use the new
>> model, and that driver could really use support for an API which supports
>> active-low pins.
>
> If you want to benefit from the active-low property but cannot use
> gpiod_get() for some reason, you can still request a GPIO by
> gpio_request_one() and then convert it to a descriptor with
> gpio_to_desc(), which is what I suspect your patch will allow you to
> do. But the real fix would be to work any limitations that gpiod has
> that prevent you from doing what you need.
>
> I am not familiar with mdio-gpio - could you explain what makes it
> impossible to convert this driver to the new model in your view?
>

gpiod_get has the notion of 'con_id'. That is all fine if you have
a system with acpi or devicetree to set this up, but the platform
data fallback is pretty clumsy (at least in my opinion). To convert
mdio-gpio such that it can provide the required platform data to gpio
would complicate the code so much that the conversion would not be worth
it. It would also require to convert all drivers _using_ mdio-gpio to the
new platform data format required by gpiod_get, or the mdio-gpio driver
would have to do some kind of internal conversion, both of which would
make the code even more complicated (and less likely to get it accepted).

My current solution is exactly what you suggested - to use gpio_request_one()
followed by gpio_to_desc(). I plan to submit the necessary patches
once this patch has been accepted.

Thanks,
Guenter

Re: [PATCH] gpio: Add support for GPIOF_ACTIVE_LOW to gpio_request_one

[Alexandre Courbot]

On Tue, Jul 22, 2014 at 2:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/21/2014 09:30 PM, Alexandre Courbot wrote:
>>
>> On Thu, Jul 17, 2014 at 11:34 PM, Guenter Roeck <linux@roeck-us.net>
>> wrote:
>>>
>>> On 07/17/2014 12:26 AM, Alexandre Courbot wrote:
>>>>
>>>>
>>>> On Thu, Jul 17, 2014 at 3:37 PM, Guenter Roeck <linux@roeck-us.net>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 07/16/2014 11:09 PM, Alexandre Courbot wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, Jul 17, 2014 at 8:11 AM, Guenter Roeck <linux@roeck-us.net>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The gpio include file and the gpio documentation declare and document
>>>>>>> GPIOF_ACTIVE_LOW as one of the flags to be passed to gpio_request_one
>>>>>>> and related functions. However, the flag is not evaluated or used.
>>>>>>>
>>>>>>> Check the flag in gpio_request_one and set the gpio internal flag
>>>>>>> FLAG_ACTIVE_LOW if it is set.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> What is the point since the integer GPIO API has no clue of the
>>>>>> active-low status of a GPIO? It is only used by the gpiod and sysfs
>>>>>> interfaces.
>>>>>>
>>>>>
>>>>> One can use gpio_request_one() to export a gpio pin to user space from
>>>>> the kernel. That code path does use the flag, as you point out yourself
>>>>> above.
>>>>
>>>>
>>>>
>>>> Ok, in that case I suppose it makes sense.
>>>>
>>>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>>>>
>>>>> One could also argue that the integer gpio API _should_ support this as
>>>>> well,
>>>>> but that is a different question.
>>>>
>>>>
>>>>
>>>> Probably not going to happen. The integer GPIO interface is deprecated
>>>> and users who need new features should seriously consider switching to
>>>> gpiod.
>>>>
>>>
>>> The new API is unfortunately not equivalent to the old one. For example,
>>> if I understand correctly, gpiod_get is expected to be used instead of
>>> gpio_request_one.
>>
>>
>> That is correct. The reason is to have a separate authority to assigns
>> GPIOs and prevent drivers from arbitrarily requesting any GPIO they
>> want, as long as everybody sticks to the gpiod interface.
>>
>>> That may work nicely in a world with full DT or ACPI
>>> support, but doesn't work as well otherwise unless one drops the notion
>>> of using platform specific drivers built as modules
>>> (gpiod_add_lookup_table
>>> is not exported, and there is no remove function).
>>>
>>> Specifically, I don't see an easy way to convert mdio-gpio to use the new
>>> model, and that driver could really use support for an API which supports
>>> active-low pins.
>>
>>
>> If you want to benefit from the active-low property but cannot use
>> gpiod_get() for some reason, you can still request a GPIO by
>> gpio_request_one() and then convert it to a descriptor with
>> gpio_to_desc(), which is what I suspect your patch will allow you to
>> do. But the real fix would be to work any limitations that gpiod has
>> that prevent you from doing what you need.
>>
>> I am not familiar with mdio-gpio - could you explain what makes it
>> impossible to convert this driver to the new model in your view?
>>
>
> gpiod_get has the notion of 'con_id'. That is all fine if you have
> a system with acpi or devicetree to set this up, but the platform
> data fallback is pretty clumsy (at least in my opinion).

GPIO lookup tables for platform data are still young, and updating it
to something better is still possible if you have ideas on how to
improve it. Actually I am thinking about giving them a second thought
myself.

> To convert
> mdio-gpio such that it can provide the required platform data to gpio
> would complicate the code so much that the conversion would not be worth
> it.

Here "the code" would only consist of platform-dependant lookup
tables, right? Or do you need to build GPIO lookup tables dynamically?

>
> It would also require to convert all drivers _using_ mdio-gpio to the
> new platform data format required by gpiod_get, or the mdio-gpio driver
> would have to do some kind of internal conversion, both of which would
> make the code even more complicated (and less likely to get it accepted).

Maybe not necessarily. The gpiod interface has been designed so that
most existing DT bindings can use it without having to change. For
instance, in Documentation/devicetree/bindings/net/mdio-gpio.txt I see
that GPIOs are assigned that way:

gpios = <&qe_pio_a 11
        &qe_pio_c 6>;

You can get such GPIOs by omitting the con_id argument in
gpiod_get_index(), e.g. gpiod_get_index(dev, NULL, 1) will return the
second GPIO of the array.

All the same you can define lookup tables only containing GPIOs
without a con_id. They will match the same call.

>
> My current solution is exactly what you suggested - to use
> gpio_request_one()
> followed by gpio_to_desc(). I plan to submit the necessary patches
> once this patch has been accepted.

Not ideal, but acceptable as a temporary solution until we find a
better way for you to use gpiod. It's better to use it halfway than
not at all.