| Message ID | 20170604101049.4592-1-uwe@kleine-koenig.org |
|---|---|
| State | New |
| Headers | show |
On Sun, Jun 04, 2017 at 12:10:49PM +0200, Uwe Kleine-König wrote: > Using the following gpio hog: > > { > gpio-hog; > gpios = <17 GPIO_ACTIVE_LOW>; > output-high; > } > > results in the gpio to be set to (physically) low value, which is > surprising. So support "output-active" which makes the semantic more > obvious. The old semantic is still supported for backward compatibility. Ugg. Can't say I'm a fan of the Linux GPIO subsystem inverting the logic for GPIO_ACTIVE_LOW... > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > Hello, > > the next logical step would be to go through all in-tree dts files and > substitute the old properties by the new ones. I can do this as soon as > the idea is discussed and considered to be good. > > Maybe the same idea can be applied to the cpp definitions GPIOD_OUT_LOW > and GPIOD_OUT_HIGH. > > And btw I wondered about the binding that defines what should happen in > this case: > > { > gpio-hog; > gpios = <...>; > output-high; > output-low; > } > > I would have defined as undefined instead of inactive. > > Best regards > Uwe > > Documentation/devicetree/bindings/gpio/gpio.txt | 10 ++++++---- > drivers/gpio/gpiolib-of.c | 4 ++++ > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt > index 68d28f62a6f4..4d29e0b33435 100644 > --- a/Documentation/devicetree/bindings/gpio/gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt > @@ -196,10 +196,12 @@ This means that when multiple properties are present they will be searched > in the order presented below and the first match is taken as the intended > configuration. > - input: A property specifying to set the GPIO direction as input. > -- output-low A property specifying to set the GPIO direction as output with > - the value low. > -- output-high A property specifying to set the GPIO direction as output with > - the value high. > +- output-inactive: A property specifying to set the GPIO direction as output > + using the inactive level as value. > +- output-active: A property specifying to set the GPIO direction as output > + using the active level as value. > +- output-low A deprecated alias for "output-inactive". > +- output-high A deprecated alias for "output-active". The question is was this really what was expected. I guess based on the code it was. I would have expected these gave low or high voltage levels. Seems like Linuxisms defining the binding. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Maybe the same idea can be applied to the cpp definitions GPIOD_OUT_LOW >> and GPIOD_OUT_HIGH. I'd say it was quite bad idea to introduce ACTIVE_LOW thing while still same "high" / "low" names all over the API. Now this is causing confusion in every other use case. The only way to clean up this mess now without breaking things is to rename everything, and deprecate all currently-exisitng identifiers containing "high" and "low" substrings. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 4, 2017 at 12:10 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote: > - input: A property specifying to set the GPIO direction as input. > -- output-low A property specifying to set the GPIO direction as output with > - the value low. > -- output-high A property specifying to set the GPIO direction as output with > - the value high. > +- output-inactive: A property specifying to set the GPIO direction as output > + using the inactive level as value. > +- output-active: A property specifying to set the GPIO direction as output > + using the active level as value. > +- output-low A deprecated alias for "output-inactive". > +- output-high A deprecated alias for "output-active". Wouldn't "output-asserted" / "output-deasserted" be more to the point? If the majority prefers "active" I can live with that too. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 9, 2017 at 12:33 AM, Rob Herring <robh@kernel.org> wrote: > On Sun, Jun 04, 2017 at 12:10:49PM +0200, Uwe Kleine-König wrote: >> Using the following gpio hog: >> >> { >> gpio-hog; >> gpios = <17 GPIO_ACTIVE_LOW>; >> output-high; >> } >> >> results in the gpio to be set to (physically) low value, which is >> surprising. So support "output-active" which makes the semantic more >> obvious. The old semantic is still supported for backward compatibility. > > Ugg. Can't say I'm a fan of the Linux GPIO subsystem inverting the logic > for GPIO_ACTIVE_LOW... Actually it is the other way around, chronologically speaking. From its inception, the GPIO bindings supplied a flag to invert the signal on the consumer phandle. Then some consumers started to screw around with their local inversion properties. This has some historical reasons. <dt-bindings/gpio/gpio.h> is just documenting the historical fact that was introduced in the very first GPIO DT bindings in commit commit d524dac9279b6a41ffdf7ff7958c577f2e387db6 "dt: Move device tree documentation out of powerpc directory" But already the cabal of powerpc developers did this before we started to globalize their DT attempts. It all started (AFAICT) in the MMC bindings. What actually happened was this: commit 3f1c6ebf57b815ad709e89291e446935fee78f75 "powerpc: add mmc-spi-slot bindings" Then used in: commit 754582853120a9ec8b8293b5147b605b1c6a39f1 "powerpc/83xx: add mmc-spi support via the device tree for MPC8323E-RDB from march 31st 2009: + mmc-slot@0 { + compatible = "fsl,mpc8323rdb-mmc-slot", + "mmc-spi-slot"; + reg = <0>; + gpios = <&qe_pio_d 14 1 + &qe_pio_d 15 0>; + voltage-ranges = <3300 3300>; + spi-max-frequency = <50000000>; + }; As you can see the first GPIO line here (card detect) is clearly active low, and that is why it has the magical flag "1". What happens later is more disturbing: commit 50dfe70fe9e216cf356830194630f9a39e498d76 "powerpc: introduce and document sdhci,wp-inverted property for eSDHC" On sep 22, 2009 adds: - clock-frequency : specifies eSDHC base clock frequency. + - sdhci,wp-inverted : (optional) specifies that eSDHC controller + reports inverted write-protect state; These two oxymoronic patches were even written by the same developer. Who is not to blame. This is simply the result of lack of review, and lack of macros clearly showing what is going on. The open coded numerals obscured the meaning of the line inversion flag, so when merging the latter binding this was forgotten. But it is very clear that in the beginning of time, the idea was that consumers should specify any line inversion with a "1" in the second cell of the GPIO specifier. Maybe that initial idea was wrong. Maybe someone who was there back in 2009 could care to tell us how they were thinking. Maybe this was even discussed by the IEEE quarter? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt index 68d28f62a6f4..4d29e0b33435 100644 --- a/Documentation/devicetree/bindings/gpio/gpio.txt +++ b/Documentation/devicetree/bindings/gpio/gpio.txt @@ -196,10 +196,12 @@ This means that when multiple properties are present they will be searched in the order presented below and the first match is taken as the intended configuration. - input: A property specifying to set the GPIO direction as input. -- output-low A property specifying to set the GPIO direction as output with - the value low. -- output-high A property specifying to set the GPIO direction as output with - the value high. +- output-inactive: A property specifying to set the GPIO direction as output + using the inactive level as value. +- output-active: A property specifying to set the GPIO direction as output + using the active level as value. +- output-low A deprecated alias for "output-inactive". +- output-high A deprecated alias for "output-active". Optional properties: - line-name: The GPIO label name. If not present the node name is used. diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 193f15d50bba..c465511eae8a 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -209,6 +209,10 @@ 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_LOW; + else if (of_property_read_bool(np, "output-active")) + *dflags |= GPIOD_OUT_HIGH; else if (of_property_read_bool(np, "output-low")) *dflags |= GPIOD_OUT_LOW; else if (of_property_read_bool(np, "output-high"))
Using the following gpio hog: { gpio-hog; gpios = <17 GPIO_ACTIVE_LOW>; output-high; } results in the gpio to be set to (physically) low value, which is surprising. So support "output-active" which makes the semantic more obvious. The old semantic is still supported for backward compatibility. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Hello, the next logical step would be to go through all in-tree dts files and substitute the old properties by the new ones. I can do this as soon as the idea is discussed and considered to be good. Maybe the same idea can be applied to the cpp definitions GPIOD_OUT_LOW and GPIOD_OUT_HIGH. And btw I wondered about the binding that defines what should happen in this case: { gpio-hog; gpios = <...>; output-high; output-low; } I would have defined as undefined instead of inactive. Best regards Uwe Documentation/devicetree/bindings/gpio/gpio.txt | 10 ++++++---- drivers/gpio/gpiolib-of.c | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-)