| Message ID | 20240211101421.166779-1-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: uapi: clarify default_values being logical | expand |
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
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.
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(-)