Active-low behavior in gpiolib

Active-low behavior in gpiolib

[Alexandre Courbot]

Hi everyone,

While preparing the v2 of the descriptor-based GPIO interface (gpiod),
I stumbled upon this point that looks like some inconsistency in the
current interface.

gpiolib.c defines the following flags that can influence the gpio
output: FLAG_ACTIVE_LOW, FLAG_OPEN_DRAIN, and FLAG_OPEN_SOURCE.
FLAG_OPEN_DRAIN and FLAG_OPEN_SOURCE, when set, take effet on all set
operations no matter where they come from.

FLAG_ACTIVE_LOW, on the other hand, is *only* set and used through
sysfs operations. Independently, the active low property can be
specified in gpio phandles and retrieved by
of_get_(named_)gpio_flags() - only it has to be kept in a separate
variable which must then be checked everytime one wants to change the
GPIO value to set the correct level.

I find it rather confusing and wonder if we could not come with a more
consistent behavior for the gpiod interface. In particular:
- the active low property could be specified when declaring GPIO
mappings (either through DT or platform data)
- gpiod_get() would retrieve it transparently and set it as a
permanent property of the GPIO
- gpiod_set/get_value() would take the active low property into
account when setting the value - making these functions work with the
actual *value* (as their name states) instead of the physical line
level.

The old gpio_* interface would still behave as usual, of course.

This would make gpiolib behavior more consistent IMHO, at the price of
a semantic difference between the otherwise very similar gpio and
gpiod interfaces. Any thought for or against this? If we go this way
and decide to allow gpiod to diverge, are there other things we are
not satisfied with about the current gpio interface and that we want
to improve?

Thanks,
Alex.

Re: Active-low behavior in gpiolib

[Linus Walleij]

On Wed, Jun 5, 2013 at 7:22 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> FLAG_ACTIVE_LOW, on the other hand, is *only* set and used through
> sysfs operations. Independently, the active low property can be
> specified in gpio phandles and retrieved by
> of_get_(named_)gpio_flags() - only it has to be kept in a separate
> variable which must then be checked everytime one wants to change the
> GPIO value to set the correct level.

Well it is designed as a sysfs-only thing according to the comment:
drivers/gpio/gpiolib.c:#define FLAG_ACTIVE_LOW  6       /* sysfs value
has active low */

So I think your actual question is whether it should also be
enabled for the kernel-internal interfaces.

Currently we have things like this:

/**
 * struct mmci_platform_data - platform configuration for the MMCI
 * (also known as PL180) block.
(...)
 * @gpio_cd: read this GPIO pin to detect card insertion
 * @cd_invert: true if the gpio_cd pin value is active low
(...)
        int     gpio_cd;
        bool    cd_invert;

So the knowledge of whether a certain GPIO is active high or low
is spread out through drivers, and the API only drives the line in a
very explicit way.

I am uncertain whether it is wise to move this into gpiolib for
in-kernel consumers, it is mainly a convention of where the
knowledge should sit. In the mentioned case, the flags will still
be needed because we need to have the information there to
set the flag toward the gpiolib API anyway.

So the only thing gained is that gpiolib gets some knowledge
of how the pin is used. But what is the gain of that?

One could argue that this property should not have been
added to the sysfs ABI for the same reason but that cannot
be changed.

The ABI addition of polarity was done when the GPIOlib was
orphaned, by the way, so it wasn't really reviewed. :-(
I wonder if the author's mail address is still working.

Yours,
Linus Walleij

Re: Active-low behavior in gpiolib

[Stephen Warren]

On 06/07/2013 01:10 AM, Linus Walleij wrote:
> On Wed, Jun 5, 2013 at 7:22 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> 
>> FLAG_ACTIVE_LOW, on the other hand, is *only* set and used through
>> sysfs operations. Independently, the active low property can be
>> specified in gpio phandles and retrieved by
>> of_get_(named_)gpio_flags() - only it has to be kept in a separate
>> variable which must then be checked everytime one wants to change the
>> GPIO value to set the correct level.
> 
> Well it is designed as a sysfs-only thing according to the comment:
> drivers/gpio/gpiolib.c:#define FLAG_ACTIVE_LOW  6       /* sysfs value
> has active low */
> 
> So I think your actual question is whether it should also be
> enabled for the kernel-internal interfaces.
> 
> Currently we have things like this:
> 
> /**
>  * struct mmci_platform_data - platform configuration for the MMCI
>  * (also known as PL180) block.
> (...)
>  * @gpio_cd: read this GPIO pin to detect card insertion
>  * @cd_invert: true if the gpio_cd pin value is active low
> (...)
>         int     gpio_cd;
>         bool    cd_invert;
> 
> So the knowledge of whether a certain GPIO is active high or low
> is spread out through drivers, and the API only drives the line in a
> very explicit way.
...
> So the only thing gained is that gpiolib gets some knowledge
> of how the pin is used. But what is the gain of that?

Well, the big gain I see is that every driver needs to somehow find out
whether the GPIO is active-low or not, and store a separate flag for it,
and XOR all data with that flag. That's not a massive amount of code,
but it's really rather annoying. Why can't we just tell gpiolib what the
polarity is, and let it deal with it. In the DT case, this "tell gpiolib
what the polarity is" would be 100% hidden inside the DT parsing/mapping
(of_xlate) code. For non-DT, presumably this could be a flag that the
board file or platform code programs into gpiolib early on.

If you're worried about this hiding too much information, and that some
code might want to know what's really going on rather than the logical
values, perhaps:

gpiod_[gs]et() - return/accept logical values
gpiod_[gs]et_raw() - return the actual electrical level

gpiod_[gs]et() might also hide details of
open-drain/collector/source/... too, and do automatic tri-stating based
on the level of output that was desired?

Re: Active-low behavior in gpiolib

[Alexandre Courbot]

On Fri, Jun 7, 2013 at 4:10 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Well it is designed as a sysfs-only thing according to the comment:
> drivers/gpio/gpiolib.c:#define FLAG_ACTIVE_LOW  6       /* sysfs value
> has active low */
>
> So I think your actual question is whether it should also be
> enabled for the kernel-internal interfaces.

Yes, that's my question.

> Currently we have things like this:
>
> /**
>  * struct mmci_platform_data - platform configuration for the MMCI
>  * (also known as PL180) block.
> (...)
>  * @gpio_cd: read this GPIO pin to detect card insertion
>  * @cd_invert: true if the gpio_cd pin value is active low
> (...)
>         int     gpio_cd;
>         bool    cd_invert;
>
> So the knowledge of whether a certain GPIO is active high or low
> is spread out through drivers, and the API only drives the line in a
> very explicit way.
>
> I am uncertain whether it is wise to move this into gpiolib for
> in-kernel consumers, it is mainly a convention of where the
> knowledge should sit. In the mentioned case, the flags will still
> be needed because we need to have the information there to
> set the flag toward the gpiolib API anyway.
>
> So the only thing gained is that gpiolib gets some knowledge
> of how the pin is used. But what is the gain of that?

Well, my problem is clearly highlighted by the example you mention,
which is a very common usage pattern especially for DT-obtained GPIOs
which can have this OF_GPIO_ACTIVE_LOW flag. E.g. we have a lot of
this:

    gpio = of_get_named_gpio_flags(np, "foo-gpios", 0, &flags);
    active_low = flags & OF_GPIO_ACTIVE_LOW ? 1 : 0;
    ...
    state = (gpio_get_value(gpio) ? 1 : 0) ^ active_low;
    ...
    gpio_set_value(gpio, (value ? 1 : 0) ^ active_low;

which could be simplified into this:

    gpio = of_get_named_gpiod_flags(np, "foo-gpios", 0);
    ...
    state = gpiod_get_value(gpio);
    ...
    gpiod_set_value(gpio, value);

Board design makes it possible to turn any GPIO active low for any
(good or bad) reason, thus the drivers, which interact with gpiolib,
are more likely to want to set the GPIO logical value (taking the
active low parameter into account) rather than its raw level.

In the end, drivers that are not using an active_low flag are most
likely doing so just because they never encountered such a situation.
In practice, I suspect nearly all drivers want to be aware of the
active-low property.

Since gpiolib is already half-aware of this property, through the
sysfs active_low node and the OF_GPIO_ACTIVE_LOW DT property, why not
complete the circle and ease the life of driver writers?

As Stephen wrote, an alternative set of functions to get/set the
actual raw value of the line could be provided, but my understanding
is that an overwhelming majority of GPIO users would benefit from
having the active low property being handled transparently.

Alex.