Re: Documentation for line_config PULL_UP, effect on line_event edges and line_request values?

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Jun 03, 2024 at 05:48:05PM -0500, David C. Rankin wrote:
> All,
>

Well hello again.

>   First post, so only shoot me once if the rational is documented somewhere.

rationale

> I've been working with the gpio_v2 uABI (fantastic piece of work), but I've
> run into a very confusing combination using gpio_line_get_values where the
> line is PULL_UP makes going down "RISING" and going up "FALLING" and a .bits
> value of 1 is for zero voltage and a value of 0 for normal line voltage. I
> think I've digested it correctly, but I'm wondering if there is a better way
> to handle this and whether the chardev.html doc should be updated to further
> explain this behavior?
>

Quite possibly - it is easy to overlook what you consider obvious when
documenting, like line values being logical, not physical.  That seems
to be the key point you are missing.

>   The confusing part comes from what is defined as "active" and what is
> defined as "inactive" when the line is active low, e.g.
>

Line values are logical, and the documentation uses the terminology
active/inactive to emphasise this throughout.  That applies to getting
and setting values, and the polarity of edges.  You will note the
documentation never uses the hi/lo terminology - as that indicates physical
values.

OTOH bias (pull) is always physical.  I've never seen it used otherwise.
A pull-up is always a bias towards Vdd, and a pull-down is a bias towards GND.
That is how it is defined in hardware, and that is independent of whether
a line is considered active low.

The ACTIVE_LOW flag controls the mapping between physical and logical.
When set, the logical is the inverse of the physical.  When cleared
physical and logical are the same.
The ACTIVE_LOW flag does not change how bias is interpreted - those are
always physical.  Having bias also change polarity would be even more
confusing - have you ever seen a pull-up on a schematic referred to as a
pull-down because the line is active low?

>  /* gpio_v2 line config, line request and line values, read defaults set */
>   struct gpio_v2_line_config linecfg = {
>                               .flags =  GPIO_V2_LINE_FLAG_ACTIVE_LOW      |
>                                         GPIO_V2_LINE_FLAG_INPUT           |
>                                         GPIO_V2_LINE_FLAG_EDGE_RISING     |
>                                         GPIO_V2_LINE_FLAG_EDGE_FALLING    |
>                                         GPIO_V2_LINE_FLAG_BIAS_PULL_UP,
>                               .num_attrs = 1 };
>
>   In this case the configured line_request is passed to a thread for reading
> where the interest is in both the edges and the line_values.
>
>   The documentation at
> https://docs.kernel.org/userspace-api/gpio/chardev.html is helpful, but it
> is silent on how the ...FLAG_ACTIVE_LOW/...PULL_UP basically inverts

It is just the ACTIVE_LOW flag.  And as you quote below, when the
ACTIVE_LOW flag is set "line active state is physical low".

Btw, if you are going to provide links then use references, with the link
itself at the bottom of your email e.g.[1].

> everything so that catching the ...FLAG_EDGE_RISING is actually the edge
> going from normal line voltage to zero (normally the falling edge of a
> waveform), how the value retrieved by gpio_line_get_values() then reports
> bit Hi (1) for the zero voltage state. The ...FLAG_EDGE_FALLING is then
> triggered when voltage goes from zero back to normal line voltage (normally
> the rising edge) and the value reported by gpio_line_get_values() is then Lo
> (0) at line voltage.
>

Does highlighting that line values are logical help?

>   The header gpio.h does provide more help. There the description of the
> attribute flags does indicate that rising will trigger on transition from
> (inactive to active) edges and falling will trigger on (active to inactive)
> edges, e.g.
>
> /**
>  * enum gpio_v2_line_flag - &struct gpio_v2_line_attribute.flags values
>  ...
>  * @GPIO_V2_LINE_FLAG_ACTIVE_LOW: line active state is physical low
>  ...

Yup, when that flag is set logical is the inverse of physical.
Not clear enough?

>
>  * @GPIO_V2_LINE_FLAG_EDGE_RISING: line detects rising (inactive to active)
>  * edges
>  * @GPIO_V2_LINE_FLAG_EDGE_FALLING: line detects falling (active to
>  * inactive) edges
>  ...

So that does not makes it clear that the edge definitions are based on
logical values?

>
>   Where there is a little ambiguity is in the comment for
> gpio_v2_line_values related to active/inactive .bits values. Taken together
> with the flags comment it's reasonably clear that active/inactive are as
> used in flags and not as commonly used (e.g. inactive - zero - low, active -
> non-zero - high). The comment reads:
>
> /**
>  * struct gpio_v2_line_values - Values of GPIO lines
>  * @bits: a bitmap containing the value of the lines, set to 1 for active
>  * and 0 for inactive.
>  ...
>

Again, logical values.

>   To make sure I was interpreting "active"/"inactive" and the effect on what
> is RISING and FALLING edge and .bits values I wrote a short program for the
> Raspberry Pi to catch the edges and values on button press and release and
> display the results. The results were indeed that the active RISING edge was
> the transition from line-voltage to zero, with a .bits value of 1 (Hi) for
> voltage zero, and on button release the inactive FALLING edge was the
> transition from zero to line-voltage with a .bits value of 0 (Lo) for line
> voltage.
>
>   (if interested the short test program and Makefile for the Pi are at https://github.com/drankinatty/pi-wo-root/tree/master/tst/gpio_v2_button_value)
>

Sorry, I haven't ipaid much attention to your code - it is easier and clearer
to use the libgpiod command line tools (gpioset/gpioget/gpiomon/gpioinfo)
for examples.  They support all the flag combinations you care to test.

If something isn't working the way you expect then an example using those
tools would be very helpful.

>   My questions are:
>
>  1. is there any thread or document squirreled away that contains a
> discussion of how this rational was arrived at?
>

Logical values are reported, not physical, to allow users to invert the
sense of active low lines.  And given that, logical values have to be
used throughout for consistency.

If you don't want that then don't set active low, so logical IS physical.

I doubt that warranted a detailed discussion.

>  2. should the documentation at
> https://docs.kernel.org/userspace-api/gpio/chardev.html be updated to add
> the behavior for "active"/"inactive" and what flag this is dependent on
> (PULL_UP, ACTIVE_LOW or BOTH?) If so, I'd be glad to take a crack at the
> write-up and pass it to whoever for review and revision. (just let me know
> who the right person is to send it to if interested) The chardev.html seems
> like the right place for it rather than having to also locate and read
> gpio.h to find "active" and "inactive" (especially for newer users using
> latest libgpio for Pi, etc.. based on gpio_v2)
>

To update the documentation you would edit the appropriate file(s) and
submit a patch to this list, as well as those indicated in the
MAINTAINERS list (you can use linux/scripts/get_maintainers.pl on your patch
to find them).

>  3. is the expected programming approach to query the line config so that
> the .bits values can be interpreted as either 1 (Hi or Lo), or 0 (Lo or Hi)?
>

You are coming from the Pi world so there is a good chance you have a set
and forget mindset, but that is not how it is intended to be used.

The approach is that YOU are expected to know how YOU configured the
flags for the line.  You have to configure the line as part of requesting
it - even if you only want to read it, so you definitely configured it.
Why did you forget?

And if you always want physical values then never set ACTIVE_LOW.

>  (I guess that was where the biggest confusion was -- that a .bits value 0
> didn't mean no voltage, and vice-versa)
>

Again, they are logical values, not voltages.

>   Don't take this as a knock on the implementation, I think gpio_v2 is a
> stroke of genius, especially the debounce, but rather these were the parts
> that really were a bit difficult to suss out of the documentation and it may
> be helpful to include further explanation in the chardev.html pages
> explaining this a bit further.
>
> NOTE Also:
>
>   Links for the lists in
> https://docs.kernel.org/process/submitting-patches.html under the "Select
> the recipients for your patch" heading still point to
> http://vger.kernel.org/vger-lists.html (Majordomo)
>

Not sure what your point is.  The MAINTAINERS list is the definitive place
to find the appropriate list - "The best place to look for mailing lists is
in the MAINTAINERS file packaged with the kernel source."[1].

That doc is itself part of the kernel documentation, so suggest a patch to
the appropriate list - it is out of the scope of this one.


I'll have another read of the GPIO documentation with this in mind and
see where logical/physical line values can be clarified.

Cheers,
Kent.

[1] https://docs.kernel.org/process/2.Process.html#mailing-lists