Re: [PATCH] gpio: uapi: clarify default_values being logical

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Feb 12, 2024 at 05:56:07PM +0800, Kent Gibson wrote:
> On Mon, Feb 12, 2024 at 11:44:02AM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 11:28 AM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > > On Sun, Feb 11, 2024 at 06:58:14PM +0200, Andy Shevchenko wrote:
> > > > > On Sun, Feb 11, 2024 at 12:14 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > > > >
> > > > > > The documentation for default_values mentions high/low which can be
> > > > > > confusing, particularly when the ACTIVE_LOW flag is set.
> > > > > >
> > > > > > Replace high/low with active/inactive to clarify that the values are
> > > > > > logical not physical.
> > > > > >
> > > > > > Similarly, clarify the interpretation of values in struct gpiohandle_data.
> > > > >
> > > > > I'm not against this particular change, but I want the entire GPIO
> > > > > documentation to be aligned in the terminology aspect. Is this the
> > > > > case after this patch? I.o.w. have we replaced all leftovers?
> > > >
> > > > Agreed. Those are the last remnants of the low/high terminolgy that I am
> > > > aware of, certainly the last in gpio.h.
> > > >
> > > > Having a closer look to double check...
> > > >
> > > > Ah - it is still used in Documentation/userspace-api/gpio/sysfs.rst -
> > > > not somewhere I go very often.
> > > > Would you like that updated in a separate patch?
> > >
> > > Yes, please. For this one
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> >

In response after re-reading these docs:

> > Also
> > "The values are boolean, zero for low, nonzero for high."
> > https://docs.kernel.org/driver-api/gpio/consumer.html
> >

That one is logical and should be changed.

> > And there as well
> > "With this, all the gpiod_set_(array)_value_xxx() functions interpret
> > the parameter "value" as "asserted" ("1") or "de-asserted" ("0")."
> > So, should we use asserted-deasserted?
> >

We should use active/inactive rather than asserted/de-asserted. This is
the only place that terminology is used - which is ironic as it is this
section (_active_low_semantics) that explicitly describes the
physical/logical mapping.

> >
> > On https://docs.kernel.org/driver-api/gpio/
> > "get
> > returns value for signal "offset", 0=low, 1=high, or negative error
> >
> > ...

The struct gpio_chip interface is physical, not logical - the active low
conversion is handled in gpiolib, so this (driver.h) is correct as is.

> >
> > reg_set
> > output set register (out=high) for generic GPIO
> >
> > reg_clr
> > output clear register (out=low) for generic GPIO"
> > (Not sure about the last two, though)
> >
> > https://docs.kernel.org/driver-api/gpio/intro.html
> > "Output values are writable (high=1, low=0)."
> >

I read that to be physical values, so good as is.

> >
> > A-ha, here is the section about this:
> > https://docs.kernel.org/driver-api/gpio/intro.html#active-high-and-active-low.
> >
> >
> > On https://docs.kernel.org/driver-api/gpio/drivers-on-gpio.html
> > "ledtrig-gpio: drivers/leds/trigger/ledtrig-gpio.c will provide a LED
> > trigger, i.e. a LED will turn on/off in response to a GPIO line going
> > high or low (and that LED may in turn use the leds-gpio as per
> > above)."
> >

Ditto - physical values.

> > So, can you re-read all of it for high/low asserted/deasserted,
> > active/inactive and amend accordingly?
> >
>

So, from these, consumer.rst is the only file requiring any change.
I'll submit a patch for that shortly.

Cheers,
Kent.