| Message ID | 20230530151946.2317748-3-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: introduce hog properties with less ambiguity | expand |
On Tue, May 30, 2023 at 6:19 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > For active-low GPIOs the currently available nomenclature requires > regular explaination to the non-enlightened folks, e.g. because a hog explanation > defined as: > > someline { > gpio-hog; > gpios = <24 GPIO_ACTIVE_LOW>; > output-high; > } > > results in the line being set to the physical low level. > > So use the terms "active" and "inactive" which are less ambigous and ambiguous > keep the old names as synonyms. The above example can now be written as: > > someline { > gpio-hog; > gpios = <24 GPIO_ACTIVE_LOW>; > output-active; > } > > where it is less surprising that the output is set to a low level. As I said before, this does not cover the ACPI case. Consider providing an fwnode interface for them and then reuse in OF and/or ACPI if necessary. ... > + GPIOD_OUT_LOW_OPEN_DRAIN = GPIOD_OUT_INACTIVE_OPEN_DRAIN, > + GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_ACTIVE_OPEN_DRAIN, This one is an interesting case, because depending on the transistor polarity this may be active GND or VDD. All the same for OPEN_SOURCE which seems not defined (but should be equivalent to the opposite to the _DRAIN cases).
Hello Andy, On Wed, May 31, 2023 at 12:51:34AM +0300, Andy Shevchenko wrote: > On Tue, May 30, 2023 at 6:19 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > For active-low GPIOs the currently available nomenclature requires > > regular explaination to the non-enlightened folks, e.g. because a hog > > explanation > > > defined as: > > > > someline { > > gpio-hog; > > gpios = <24 GPIO_ACTIVE_LOW>; > > output-high; > > } > > > > results in the line being set to the physical low level. > > > > So use the terms "active" and "inactive" which are less ambigous and > > ambiguous Damn, I should configure my editor to enable spell checking automatically. Thanks. > > keep the old names as synonyms. The above example can now be written as: > > > > someline { > > gpio-hog; > > gpios = <24 GPIO_ACTIVE_LOW>; > > output-active; > > } > > > > where it is less surprising that the output is set to a low level. > > As I said before, this does not cover the ACPI case. Consider I don't understand that concern. Currently there is nothing for ACPI that parses "output-high" et al. So you want me to introduce support for hogs defined by ACPI to fix the strange semantic for dt-defined hogs? What am I missing? > > + GPIOD_OUT_LOW_OPEN_DRAIN = GPIOD_OUT_INACTIVE_OPEN_DRAIN, > > + GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_ACTIVE_OPEN_DRAIN, > > This one is an interesting case, because depending on the transistor > polarity this may be active GND or VDD. All the same for OPEN_SOURCE > which seems not defined (but should be equivalent to the opposite to > the _DRAIN cases). This is (also) orthogonal to my change, right? Best regards Uwe
On Wed, May 31, 2023 at 9:58 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Wed, May 31, 2023 at 12:51:34AM +0300, Andy Shevchenko wrote: > > On Tue, May 30, 2023 at 6:19 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: ... > > As I said before, this does not cover the ACPI case. Consider > > I don't understand that concern. Currently there is nothing for ACPI > that parses "output-high" et al. This is not true. > So you want me to introduce support for > hogs defined by ACPI to fix the strange semantic for dt-defined hogs? > What am I missing? https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpiolib-acpi.c#L1262 ... > > > + GPIOD_OUT_LOW_OPEN_DRAIN = GPIOD_OUT_INACTIVE_OPEN_DRAIN, > > > + GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_ACTIVE_OPEN_DRAIN, > > > > This one is an interesting case, because depending on the transistor > > polarity this may be active GND or VDD. All the same for OPEN_SOURCE > > which seems not defined (but should be equivalent to the opposite to > > the _DRAIN cases). > > This is (also) orthogonal to my change, right? Maybe yes, maybe no. Depends on what we want with this semantics regarding OS/OD/OC/OE. Strictly speaking all four should be defined. But it brings a lot of duplication. I dunno.
Hello, On Wed, May 31, 2023 at 12:51:15PM +0300, Andy Shevchenko wrote: > On Wed, May 31, 2023 at 9:58 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, May 31, 2023 at 12:51:34AM +0300, Andy Shevchenko wrote: > > > On Tue, May 30, 2023 at 6:19 PM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > ... > > > > As I said before, this does not cover the ACPI case. Consider > > > > I don't understand that concern. Currently there is nothing for ACPI > > that parses "output-high" et al. > > This is not true. > > > So you want me to introduce support for > > hogs defined by ACPI to fix the strange semantic for dt-defined hogs? > > What am I missing? > > https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpiolib-acpi.c#L1262 Ah, that was the necessary hint. Adding the aliases there would be a third patch in this series, right? Best regards Uwe
On Wed, May 31, 2023 at 1:06 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Wed, May 31, 2023 at 12:51:15PM +0300, Andy Shevchenko wrote: > > On Wed, May 31, 2023 at 9:58 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: ... > > https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpiolib-acpi.c#L1262 > > Ah, that was the necessary hint. Adding the aliases there would be a > third patch in this series, right? No. Just split out that one to gpiolib main code, since it's already using fwnode API, update OF code to use it, and modify it and then we are done.
On Wed, May 31, 2023 at 3:21 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, May 31, 2023 at 1:06 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, May 31, 2023 at 12:51:15PM +0300, Andy Shevchenko wrote: > > > On Wed, May 31, 2023 at 9:58 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > ... > > > > https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpiolib-acpi.c#L1262 > > > > Ah, that was the necessary hint. Adding the aliases there would be a > > third patch in this series, right? > > No. Just split out that one to gpiolib main code, since it's already > using fwnode API, update OF code to use it, and modify it and then we > are done. Ideally there shouldn't be HOG handling in the OF nor in the ACPI code, just in the main library.
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 1436cdb5fa26..45fc1e4dbc40 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -698,10 +698,16 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np, if (of_property_read_bool(np, "input")) *dflags |= GPIOD_IN; + else if (of_property_read_bool(np, "output-inactive")) + *dflags |= GPIOD_OUT_INACTIVE; + else if (of_property_read_bool(np, "output-active")) + *dflags |= GPIOD_OUT_ACTIVE; else if (of_property_read_bool(np, "output-low")) - *dflags |= GPIOD_OUT_LOW; + /* misleading alias for output-deasserted */ + *dflags |= GPIOD_OUT_INACTIVE; else if (of_property_read_bool(np, "output-high")) - *dflags |= GPIOD_OUT_HIGH; + /* misleading alias for output-asserted */ + *dflags |= GPIOD_OUT_ACTIVE; else { pr_warn("GPIO line %d (%pOFn): no hogging state specified, bailing out\n", desc_to_gpio(desc), np); diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 1c4385a00f88..3e953a1960f4 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -47,11 +47,17 @@ struct gpio_descs { enum gpiod_flags { GPIOD_ASIS = 0, GPIOD_IN = GPIOD_FLAGS_BIT_DIR_SET, - GPIOD_OUT_LOW = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT, - GPIOD_OUT_HIGH = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT | + GPIOD_OUT_INACTIVE = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT, + GPIOD_OUT_ACTIVE = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT | GPIOD_FLAGS_BIT_DIR_VAL, - GPIOD_OUT_LOW_OPEN_DRAIN = GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_OPEN_DRAIN, - GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_OPEN_DRAIN, + GPIOD_OUT_INACTIVE_OPEN_DRAIN = GPIOD_OUT_INACTIVE | GPIOD_FLAGS_BIT_OPEN_DRAIN, + GPIOD_OUT_ACTIVE_OPEN_DRAIN = GPIOD_OUT_ACTIVE | GPIOD_FLAGS_BIT_OPEN_DRAIN, + + /* old names that are confusing in combination with active-low GPIOs */ + GPIOD_OUT_LOW = GPIOD_OUT_INACTIVE, + GPIOD_OUT_HIGH = GPIOD_OUT_ACTIVE, + GPIOD_OUT_LOW_OPEN_DRAIN = GPIOD_OUT_INACTIVE_OPEN_DRAIN, + GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_ACTIVE_OPEN_DRAIN, }; #ifdef CONFIG_GPIOLIB
For active-low GPIOs the currently available nomenclature requires regular explaination to the non-enlightened folks, e.g. because a hog defined as: someline { gpio-hog; gpios = <24 GPIO_ACTIVE_LOW>; output-high; } results in the line being set to the physical low level. So use the terms "active" and "inactive" which are less ambigous and keep the old names as synonyms. The above example can now be written as: someline { gpio-hog; gpios = <24 GPIO_ACTIVE_LOW>; output-active; } where it is less surprising that the output is set to a low level. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/gpio/gpiolib-of.c | 10 ++++++++-- include/linux/gpio/consumer.h | 14 ++++++++++---- 2 files changed, 18 insertions(+), 6 deletions(-)