gpio: uapi: clarify default_values being logical

  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.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 include/uapi/linux/gpio.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
  

On Sun, Feb 11, 2024 at 12:14 PM Kent Gibson <warthog618@gmail.com> 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?
  

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@gmail.com> 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?

Cheers,
Kent.
  

On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <warthog618@gmail.com> 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@gmail.com> 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@gmail.com>
  

On Mon, Feb 12, 2024 at 11:28 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <warthog618@gmail.com> 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@gmail.com> 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@gmail.com>

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

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?


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

..

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)."


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)."

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

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@gmail.com> wrote:
> >
> > On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <warthog618@gmail.com> 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@gmail.com> 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@gmail.com>
>
> Also
> "The values are boolean, zero for low, nonzero for high."
> https://docs.kernel.org/driver-api/gpio/consumer.html
>
> 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?
>
>
> On https://docs.kernel.org/driver-api/gpio/
> "get
> returns value for signal "offset", 0=low, 1=high, or negative error
>
> ...
>
> 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)."
>
>
> 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)."
>
> So, can you re-read all of it for high/low asserted/deasserted,
> active/inactive and amend accordingly?
>

Well that got out of control quickly - I was only considering userspace
documentation, not internals nor code comments.

So, no, not today.  Looks like you've got the internals covered though.

Cheers,
Kent.
  

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@gmail.com> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <warthog618@gmail.com> 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@gmail.com> 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@gmail.com>
> >

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.
  

On Tue, Feb 13, 2024 at 12:34 PM Kent Gibson <warthog618@gmail.com> wrote:
> 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@gmail.com> wrote:

..

> 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.

..

> > > 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.

Thanks for taking care of this!
  

On Sun, Feb 11, 2024 at 11:14 AM Kent Gibson <warthog618@gmail.com> 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.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>

After the fixes pointed out by others:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
  

On Sun, Feb 11, 2024 at 06:14:21PM +0800, Kent Gibson 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.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>

Just checking where we are at with this patch, given you've merged the
two documentation patches that followed on from it.

I realize you have bigger fish to fry at the moment, so sorry for any
distraction, but I just want to ensure there isn't something you are
expecting from me or it hasn't fallen through the cracks.

Cheers,
Kent.
  

On Thu, Feb 15, 2024 at 2:07 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sun, Feb 11, 2024 at 06:14:21PM +0800, Kent Gibson 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.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
>
> Just checking where we are at with this patch, given you've merged the
> two documentation patches that followed on from it.
>
> I realize you have bigger fish to fry at the moment, so sorry for any
> distraction, but I just want to ensure there isn't something you are
> expecting from me or it hasn't fallen through the cracks.
>
> Cheers,
> Kent.

Now queued. I had skimmed through the discussion and was under the
impression that you'll be sending a v2 of this one, sorry.

Bart