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 <[email protected]>
---
include/uapi/linux/gpio.h | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 103cd3c6c81e..f7cb8ae87df7 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -399,8 +399,8 @@ struct gpioline_info_changed {
* a batch of input or output lines, but they must all have the same
* characteristics, i.e. all inputs or all outputs, all active low etc
* @default_values: if the %GPIOHANDLE_REQUEST_OUTPUT is set for a requested
- * line, this specifies the default output value, should be 0 (low) or
- * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
+ * line, this specifies the default output value, should be 0 (inactive) or
+ * 1 (active). Anything other than 0 or 1 will be interpreted as active.
* @consumer_label: a desired consumer label for the selected GPIO line(s)
* such as "my-bitbanged-relay"
* @lines: number of lines requested in this request, i.e. the number of
@@ -426,8 +426,8 @@ struct gpiohandle_request {
* %GPIOHANDLE_REQUEST_OUTPUT, %GPIOHANDLE_REQUEST_ACTIVE_LOW etc, added
* together
* @default_values: if the %GPIOHANDLE_REQUEST_OUTPUT is set in flags,
- * this specifies the default output value, should be 0 (low) or
- * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
+ * this specifies the default output value, should be 0 (inactive) or
+ * 1 (active). Anything other than 0 or 1 will be interpreted as active.
* @padding: reserved for future use and should be zero filled
*
* Note: This struct is part of ABI v1 and is deprecated.
@@ -443,7 +443,8 @@ struct gpiohandle_config {
* struct gpiohandle_data - Information of values on a GPIO handle
* @values: when getting the state of lines this contains the current
* state of a line, when setting the state of lines these should contain
- * the desired target state
+ * the desired target state. States are 0 (inactive) or 1 (active).
+ * When setting, anything other than 0 or 1 will be interpreted as active.
*
* Note: This struct is part of ABI v1 and is deprecated.
* Use ABI v2 and &struct gpio_v2_line_values instead.
--
2.39.2
On Sun, Feb 11, 2024 at 12:14 PM Kent Gibson <[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
On Sun, Feb 11, 2024 at 06:58:14PM +0200, Andy Shevchenko wrote:
> On Sun, Feb 11, 2024 at 12:14 PM Kent Gibson <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 12, 2024 at 11:28 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <[email protected]> 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 <[email protected]> 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 <[email protected]>
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?
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 12, 2024 at 11:44:02AM +0200, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 11:28 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <[email protected]> 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 <[email protected]> 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 <[email protected]>
>
> 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
> > <[email protected]> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 1:13 AM Kent Gibson <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >
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 <[email protected]> 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
> > > <[email protected]> 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!
--
With Best Regards,
Andy Shevchenko
On Sun, Feb 11, 2024 at 11:14 AM Kent Gibson <[email protected]> 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 <[email protected]>
After the fixes pointed out by others:
Acked-by: Linus Walleij <[email protected]>
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> 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