[PATCH v2 0/2] gpio: introduce hog properties with less ambiguity

[PATCH v2 0/2] gpio: introduce hog properties with less ambiguity

[Uwe Kleine-König]

Hello,

this is another approach after
https://lore.kernel.org/linux-gpio/20210503210526.43455-1-u.kleine-koenig@pengutronix.de
two years ago. I switched back to "active" and "inactive" from
"asserted" and "deasserted". The poll about the naming is ambigous, but
I think with a slight preference of active/inactive over
asserted/deasserted (with my unbiased self preferring active/inactive,
too :-)

Rob Herring acked patch #1 and Linus Walleij acked patch #2 of the
asserted/deasserted variant back then, I dropped these as the change is
too relevant IMHO.

In the previous post I had some inconsistencies that should be gone now.

And Andy wrote:
> I like the idea (with active-inactive terms), but...
> 
> It’s a half baked solution. Please use fwnode to cover all possible
> providers.

which I didn't address because I want to change what is currently
supported and not introduce new support. Adding fwnode is orthogonal to
this renaming. I didn't find any other more general implementation that
would affect more than "of" for output-high and output-low. Please tell me
if I missed something.

Best regards
Uwe

Uwe Kleine-König (2):
  dt-bindings: gpio: introduce hog properties with less ambiguity
  gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

 Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
 drivers/gpio/gpiolib-of.c                       | 10 ++++++++--
 include/linux/gpio/consumer.h                   | 14 ++++++++++----
 3 files changed, 29 insertions(+), 11 deletions(-)


base-commit: 8c33787278ca8db73ad7d23f932c8c39b9f6e543
-- 
2.39.2

[PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Uwe Kleine-König]

For active low lines the semantic of output-low and output-high is hard
to grasp because there is a double negation involved and so output-low
is actually a request to drive the line high (aka inactive).

So introduce output-inactive and output-active with the same semantic as
output-low and output-high respectively have today, but with a more
sensible name.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index d82c32217fff..2f037bbd3ffa 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -209,15 +209,21 @@ Required properties:
 - gpios:      Store the GPIO information (id, flags, ...) for each GPIO to
 	      affect. Shall contain an integer multiple of the number of cells
 	      specified in its parent node (GPIO controller node).
+
 Only one of the following properties scanned in the order shown below.
 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.
+- input:             A property specifying to set the GPIO direction as input.
+- output-inactive:   A property specifying to set the GPIO direction as output
+		     with the inactive value (depending on the line's polarity,
+		     which is active-high by default)
+- output-active:     A property specifying to set the GPIO direction as output
+		     with the active value.
+
+For backwards compatibility "output-low" and "output-high" are supported as
+aliases for "output-inactive" and "output-active" respectively. Their usage is
+misleading for active-low outputs, so their use is discouraged.
 
 Optional properties:
 - line-name:  The GPIO label name. If not present the node name is used.
-- 
2.39.2

[PATCH v2 2/2] gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

[Uwe Kleine-König]

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(-)

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

Re: [PATCH v2 0/2] gpio: introduce hog properties with less ambiguity

[Kent Gibson]

On Tue, May 30, 2023 at 05:19:44PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this is another approach after
> https://lore.kernel.org/linux-gpio/20210503210526.43455-1-u.kleine-koenig@pengutronix.de
> two years ago. I switched back to "active" and "inactive" from
> "asserted" and "deasserted". The poll about the naming is ambigous, but
> I think with a slight preference of active/inactive over
> asserted/deasserted (with my unbiased self preferring active/inactive,
> too :-)
> 

FWIW, this makes sense to me too - the active/inactive naming is used in
both the GPIO uAPI and libgpiod v2, so it would be consistent with that,
if nothing else.

Bart, just wondering if gpio-sim should support the aliases as well?
I realise they don't support active-low, so polarity isn't an issue, and
it could even be confusing to support the alias, but just throwing it
out there...

Cheers,
Kent.

Re: [PATCH v2 0/2] gpio: introduce hog properties with less ambiguity

[Bartosz Golaszewski]

On Tue, May 30, 2023 at 5:52 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, May 30, 2023 at 05:19:44PM +0200, Uwe Kleine-König wrote:
> > Hello,
> >
> > this is another approach after
> > https://lore.kernel.org/linux-gpio/20210503210526.43455-1-u.kleine-koenig@pengutronix.de
> > two years ago. I switched back to "active" and "inactive" from
> > "asserted" and "deasserted". The poll about the naming is ambigous, but
> > I think with a slight preference of active/inactive over
> > asserted/deasserted (with my unbiased self preferring active/inactive,
> > too :-)
> >
>
> FWIW, this makes sense to me too - the active/inactive naming is used in
> both the GPIO uAPI and libgpiod v2, so it would be consistent with that,
> if nothing else.
>
> Bart, just wondering if gpio-sim should support the aliases as well?
> I realise they don't support active-low, so polarity isn't an issue, and
> it could even be confusing to support the alias, but just throwing it
> out there...
>

I'm not sure what you need aliases for? Value is only shown, never
stored (where you'd need "active", "inactive" strings).

Bart

Re: [PATCH v2 2/2] gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

Hey Uwe,

On Tue, May 30, 2023 at 05:19:45PM +0200, Uwe Kleine-König wrote:
> For active low lines the semantic of output-low and output-high is hard
> to grasp because there is a double negation involved and so output-low
> is actually a request to drive the line high (aka inactive).
> 
> So introduce output-inactive and output-active with the same semantic as
> output-low and output-high respectively have today, but with a more
> sensible name.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index d82c32217fff..2f037bbd3ffa 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -209,15 +209,21 @@ Required properties:
>  - gpios:      Store the GPIO information (id, flags, ...) for each GPIO to
>  	      affect. Shall contain an integer multiple of the number of cells
>  	      specified in its parent node (GPIO controller node).
> +
>  Only one of the following properties scanned in the order shown below.
>  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.
> +- input:             A property specifying to set the GPIO direction as input.
> +- output-inactive:   A property specifying to set the GPIO direction as output
> +		     with the inactive value (depending on the line's polarity,
> +		     which is active-high by default)
> +- output-active:     A property specifying to set the GPIO direction as output
> +		     with the active value.
> +
> +For backwards compatibility "output-low" and "output-high" are supported as
> +aliases for "output-inactive" and "output-active" respectively. Their usage is
> +misleading for active-low outputs, so their use is discouraged.

Seems like an improvement to me,
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Rob did note that gpio-hog.yaml in dt-schema would need to be updated,
but he's not around right now to approve anything there.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/2] gpio: introduce hog properties with less ambiguity

[Kent Gibson]

On Tue, May 30, 2023 at 09:15:55PM +0200, Bartosz Golaszewski wrote:
> On Tue, May 30, 2023 at 5:52 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, May 30, 2023 at 05:19:44PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > this is another approach after
> > > https://lore.kernel.org/linux-gpio/20210503210526.43455-1-u.kleine-koenig@pengutronix.de
> > > two years ago. I switched back to "active" and "inactive" from
> > > "asserted" and "deasserted". The poll about the naming is ambigous, but
> > > I think with a slight preference of active/inactive over
> > > asserted/deasserted (with my unbiased self preferring active/inactive,
> > > too :-)
> > >
> >
> > FWIW, this makes sense to me too - the active/inactive naming is used in
> > both the GPIO uAPI and libgpiod v2, so it would be consistent with that,
> > if nothing else.
> >
> > Bart, just wondering if gpio-sim should support the aliases as well?
> > I realise they don't support active-low, so polarity isn't an issue, and
> > it could even be confusing to support the alias, but just throwing it
> > out there...
> >
> 
> I'm not sure what you need aliases for? Value is only shown, never
> stored (where you'd need "active", "inactive" strings).
> 

I was thinking wrt the definition in DT - to  allow the switch to
output-active etc throughout.  And I suppose also for configuring the
hogs in configfs, but mainly the DT.

Cheers,
Kent.

Re: [PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Alexander Stein]

Hi,

Am Dienstag, 30. Mai 2023, 17:19:45 CEST schrieb Uwe Kleine-König:
> For active low lines the semantic of output-low and output-high is hard
> to grasp because there is a double negation involved and so output-low
> is actually a request to drive the line high (aka inactive).
> 
> So introduce output-inactive and output-active with the same semantic as
> output-low and output-high respectively have today, but with a more
> sensible name.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
> b/Documentation/devicetree/bindings/gpio/gpio.txt index
> d82c32217fff..2f037bbd3ffa 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -209,15 +209,21 @@ Required properties:
>  - gpios:      Store the GPIO information (id, flags, ...) for each GPIO to
>  	      affect. Shall contain an integer multiple of the number of 
cells
>  	      specified in its parent node (GPIO controller node).
> +
>  Only one of the following properties scanned in the order shown below.
>  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.
> +- input:             A property specifying to set the GPIO direction as
> input. +- output-inactive:   A property specifying to set the GPIO
> direction as output +		     with the inactive value 
(depending on the
> line's polarity, +		     which is active-high by default)
> +- output-active:     A property specifying to set the GPIO direction as
> output +		     with the active value.

I know this is essentially just renaming currently existing properties.
But these mutual exclusive (boolean) properties make it impossible to change 
them in DT overlay. Any ideas how to support changing the output level onGPIO 
hogs in DT overlay?

Despite that, this change looks sensible to me.

Best regards,
Alexander

> +For backwards compatibility "output-low" and "output-high" are supported as
> +aliases for "output-inactive" and "output-active" respectively. Their
> usage is +misleading for active-low outputs, so their use is discouraged.
> 
>  Optional properties:
>  - line-name:  The GPIO label name. If not present the node name is used.


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 2/2] gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2022 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2861 bytes --]

On Wed, May 31, 2023 at 08:18:14AM +0200, Alexander Stein wrote:
> Hi,
> 
> Am Dienstag, 30. Mai 2023, 17:19:45 CEST schrieb Uwe Kleine-König:
> > For active low lines the semantic of output-low and output-high is hard
> > to grasp because there is a double negation involved and so output-low
> > is actually a request to drive the line high (aka inactive).
> > 
> > So introduce output-inactive and output-active with the same semantic as
> > output-low and output-high respectively have today, but with a more
> > sensible name.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
> > b/Documentation/devicetree/bindings/gpio/gpio.txt index
> > d82c32217fff..2f037bbd3ffa 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> > @@ -209,15 +209,21 @@ Required properties:
> >  - gpios:      Store the GPIO information (id, flags, ...) for each GPIO to
> >  	      affect. Shall contain an integer multiple of the number of 
> cells
> >  	      specified in its parent node (GPIO controller node).
> > +
> >  Only one of the following properties scanned in the order shown below.
> >  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.
> > +- input:             A property specifying to set the GPIO direction as
> > input. +- output-inactive:   A property specifying to set the GPIO
> > direction as output +		     with the inactive value 
> (depending on the
> > line's polarity, +		     which is active-high by default)
> > +- output-active:     A property specifying to set the GPIO direction as
> > output +		     with the active value.
> 
> I know this is essentially just renaming currently existing properties.
> But these mutual exclusive (boolean) properties make it impossible to change 
> them in DT overlay. Any ideas how to support changing the output level onGPIO 
> hogs in DT overlay?

The universal way to address this would be to make it possible in dtbos
to delete properties (and nodes).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3011 bytes --]

Hello Conor,

On Tue, May 30, 2023 at 11:20:38PM +0100, Conor Dooley wrote:
> On Tue, May 30, 2023 at 05:19:45PM +0200, Uwe Kleine-König wrote:
> > For active low lines the semantic of output-low and output-high is hard
> > to grasp because there is a double negation involved and so output-low
> > is actually a request to drive the line high (aka inactive).
> > 
> > So introduce output-inactive and output-active with the same semantic as
> > output-low and output-high respectively have today, but with a more
> > sensible name.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> > index d82c32217fff..2f037bbd3ffa 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> > @@ -209,15 +209,21 @@ Required properties:
> >  - gpios:      Store the GPIO information (id, flags, ...) for each GPIO to
> >  	      affect. Shall contain an integer multiple of the number of cells
> >  	      specified in its parent node (GPIO controller node).
> > +
> >  Only one of the following properties scanned in the order shown below.
> >  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.
> > +- input:             A property specifying to set the GPIO direction as input.
> > +- output-inactive:   A property specifying to set the GPIO direction as output
> > +		     with the inactive value (depending on the line's polarity,
> > +		     which is active-high by default)
> > +- output-active:     A property specifying to set the GPIO direction as output
> > +		     with the active value.
> > +
> > +For backwards compatibility "output-low" and "output-high" are supported as
> > +aliases for "output-inactive" and "output-active" respectively. Their usage is
> > +misleading for active-low outputs, so their use is discouraged.
> 
> Seems like an improvement to me,
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks!

> Rob did note that gpio-hog.yaml in dt-schema would need to be updated,

This is a followup-change in a separate repository once the change
under discussion is in mainline, right?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] gpio: introduce hog properties with less ambiguity

[Bartosz Golaszewski]

On Wed, May 31, 2023 at 2:55 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, May 30, 2023 at 09:15:55PM +0200, Bartosz Golaszewski wrote:
> > On Tue, May 30, 2023 at 5:52 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Tue, May 30, 2023 at 05:19:44PM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > >
> > > > this is another approach after
> > > > https://lore.kernel.org/linux-gpio/20210503210526.43455-1-u.kleine-koenig@pengutronix.de
> > > > two years ago. I switched back to "active" and "inactive" from
> > > > "asserted" and "deasserted". The poll about the naming is ambigous, but
> > > > I think with a slight preference of active/inactive over
> > > > asserted/deasserted (with my unbiased self preferring active/inactive,
> > > > too :-)
> > > >
> > >
> > > FWIW, this makes sense to me too - the active/inactive naming is used in
> > > both the GPIO uAPI and libgpiod v2, so it would be consistent with that,
> > > if nothing else.
> > >
> > > Bart, just wondering if gpio-sim should support the aliases as well?
> > > I realise they don't support active-low, so polarity isn't an issue, and
> > > it could even be confusing to support the alias, but just throwing it
> > > out there...
> > >
> >
> > I'm not sure what you need aliases for? Value is only shown, never
> > stored (where you'd need "active", "inactive" strings).
> >
>
> I was thinking wrt the definition in DT - to  allow the switch to
> output-active etc throughout.  And I suppose also for configuring the
> hogs in configfs, but mainly the DT.
>

But gpio-sim doesn't handle DT separately - it reuses the gpiolib-of
code and pretty much works like regular DT drivers.

Bart

Re: [PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

On Wed, May 31, 2023 at 09:03:51AM +0200, Uwe Kleine-König wrote:

> > Rob did note that gpio-hog.yaml in dt-schema would need to be updated,
> 
> This is a followup-change in a separate repository once the change
> under discussion is in mainline, right?

Yeah, https://github.com/devicetree-org/dt-schema

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/2] gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] gpio: use "active" and "inactive" instead of "high" and "low" for output hogs

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Rob Herring]

On Tue, May 30, 2023 at 05:19:45PM +0200, Uwe Kleine-König wrote:
> For active low lines the semantic of output-low and output-high is hard
> to grasp because there is a double negation involved and so output-low
> is actually a request to drive the line high (aka inactive).
> 
> So introduce output-inactive and output-active with the same semantic as
> output-low and output-high respectively have today, but with a more
> sensible name.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)

Please add this to the schemas in dtschema. Everything in gpio.txt 
should be removed. If that hasn't been done, it's because what's here 
needs to be relicensed. Looks like you need TI's and Geert's permission 
for the hog part. Anything from Grant, Linaro or NVIDIA I've gotten 
permission for as well.

Rob

Re: [PATCH v2 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Geert Uytterhoeven]

Hi Alexander,

On Wed, May 31, 2023 at 8:19 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
> Am Dienstag, 30. Mai 2023, 17:19:45 CEST schrieb Uwe Kleine-König:
> > For active low lines the semantic of output-low and output-high is hard
> > to grasp because there is a double negation involved and so output-low
> > is actually a request to drive the line high (aka inactive).
> >
> > So introduce output-inactive and output-active with the same semantic as
> > output-low and output-high respectively have today, but with a more
> > sensible name.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/gpio/gpio.txt | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
> > b/Documentation/devicetree/bindings/gpio/gpio.txt index
> > d82c32217fff..2f037bbd3ffa 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> > @@ -209,15 +209,21 @@ Required properties:
> >  - gpios:      Store the GPIO information (id, flags, ...) for each GPIO to
> >             affect. Shall contain an integer multiple of the number of
> cells
> >             specified in its parent node (GPIO controller node).
> > +
> >  Only one of the following properties scanned in the order shown below.
> >  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.
> > +- input:             A property specifying to set the GPIO direction as
> > input. +- output-inactive:   A property specifying to set the GPIO
> > direction as output +              with the inactive value
> (depending on the
> > line's polarity, +                 which is active-high by default)
> > +- output-active:     A property specifying to set the GPIO direction as
> > output +                   with the active value.
>
> I know this is essentially just renaming currently existing properties.
> But these mutual exclusive (boolean) properties make it impossible to change
> them in DT overlay. Any ideas how to support changing the output level onGPIO
> hogs in DT overlay?

That's a good point. And despite it not working, people do try to
stick e.g. /delete-node/ in .dtso files...

I assume you can sort of remove the existing hog subnode by adding
status = "disabled" in the DT overlay, and adding a new subnode to
configure the new output level?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds