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

[PATCH 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>
---
Hello,

I already sent this patch back in July and Linus (Walleij) liked the
patch but asked for an implementation. For that I added the second patch
now.

Best regards
Uwe

 Documentation/devicetree/bindings/gpio/gpio.txt | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index a8895d339bfe..1061c346a619 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -196,11 +196,16 @@ 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-deasserted: 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-asserted:   A property specifying to set the GPIO direction as output
+		     with the active value.
+
+For backwards compatibility "output-low" and "output-high" should be supported
+as aliases for "output-deasserted" and "output-asserted" 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.30.2

[PATCH 2/2] gpio: use "asserted" and "deasserted" instead of "high" and "low"

[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 "asserted" and "deasserted" 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-asserted;
	}

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 baf0153b7bca..89e852da6a61 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -624,10 +624,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-deasserted"))
+		*dflags |= GPIOD_OUT_DEASSERTED;
+	else if (of_property_read_bool(np, "output-asserted"))
+		*dflags |= GPIOD_OUT_ASSERTED;
 	else if (of_property_read_bool(np, "output-low"))
-		*dflags |= GPIOD_OUT_LOW;
+		/* misleading alias for output-deasserted */
+		*dflags |= GPIOD_OUT_DEASSERTED;
 	else if (of_property_read_bool(np, "output-high"))
-		*dflags |= GPIOD_OUT_HIGH;
+		/* misleading alias for output-asserted */
+		*dflags |= GPIOD_OUT_ASSERTED;
 	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 c73b25bc9213..b40cc19cf419 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -49,11 +49,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_DEASSERTED = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT,
+	GPIOD_OUT_ASSERTED = 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_DEASSERTED_OPEN_DRAIN = GPIOD_OUT_DEASSERTED | GPIOD_FLAGS_BIT_OPEN_DRAIN,
+	GPIOD_OUT_ASSERTED_OPEN_DRAIN = GPIOD_OUT_ASSERTED | GPIOD_FLAGS_BIT_OPEN_DRAIN,
+
+	/* old names that are confusing in combination with active-low GPIOs */
+	GPIOD_OUT_LOW = GPIOD_OUT_DEASSERTED,
+	GPIOD_OUT_HIGH = GPIOD_OUT_ASSERTED,
+	GPIOD_OUT_LOW_OPEN_DRAIN = GPIOD_OUT_DEASSERTED_OPEN_DRAIN,
+	GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_ASSERTED_OPEN_DRAIN,
 };
 
 #ifdef CONFIG_GPIOLIB
-- 
2.30.2

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

[Kent Gibson]

On Mon, May 03, 2021 at 11:05:26PM +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).
> 

+1 on clarifying the naming.

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

You use active/inactive here, but then asserted/deasserted in the patch.
My preference would be the active/inactive, which has more of a level
feel, over the asserted/deasserted which feels more like an edge.

And you still use active/inactive in the descriptions, so now we have all
three naming schemes in the mix.  

What made you change?

Cheers,
Kent.

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I already sent this patch back in July and Linus (Walleij) liked the
> patch but asked for an implementation. For that I added the second patch
> now.
> 
> Best regards
> Uwe
> 
>  Documentation/devicetree/bindings/gpio/gpio.txt | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index a8895d339bfe..1061c346a619 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -196,11 +196,16 @@ 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-deasserted: 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-asserted:   A property specifying to set the GPIO direction as output
> +		     with the active value.
> +
> +For backwards compatibility "output-low" and "output-high" should be supported
> +as aliases for "output-deasserted" and "output-asserted" 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.30.2
> 

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

[Uwe Kleine-König]

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

Hello,

On Tue, May 04, 2021 at 10:55:46AM +0800, Kent Gibson wrote:
> On Mon, May 03, 2021 at 11:05:26PM +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).
> 
> +1 on clarifying the naming.
> 
> > 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.
> > 
> 
> You use active/inactive here, but then asserted/deasserted in the patch.

oops, this is an oversight.

> My preference would be the active/inactive, which has more of a level
> feel, over the asserted/deasserted which feels more like an edge.
> 
> And you still use active/inactive in the descriptions, so now we have all
> three naming schemes in the mix.  
> 
> What made you change?

I had active/inactive first, but Linux Walleij requested
asserted/deasserted:

https://lore.kernel.org/r/CACRpkdbccHbhYcCyPiSoA7+zGXBtbL_LwLkPB3vQDyOqkTA7EQ@mail.gmail.com

While I like active/inactive better than asserted/deasserted, the latter
is still way better than high/low, so I didn't discuss.

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 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Kent Gibson]

On Tue, May 04, 2021 at 11:14:59AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, May 04, 2021 at 10:55:46AM +0800, Kent Gibson wrote:
> > On Mon, May 03, 2021 at 11:05:26PM +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).
> > 
> > +1 on clarifying the naming.
> > 
> > > 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.
> > > 
> > 
> > You use active/inactive here, but then asserted/deasserted in the patch.
> 
> oops, this is an oversight.
> 
> > My preference would be the active/inactive, which has more of a level
> > feel, over the asserted/deasserted which feels more like an edge.
> > 
> > And you still use active/inactive in the descriptions, so now we have all
> > three naming schemes in the mix.  
> > 
> > What made you change?
> 
> I had active/inactive first, but Linux Walleij requested
> asserted/deasserted:
> 
> https://lore.kernel.org/r/CACRpkdbccHbhYcCyPiSoA7+zGXBtbL_LwLkPB3vQDyOqkTA7EQ@mail.gmail.com
> 

Thanks - I'd missed that.

I don't suppose you happen to have a link to the gpiod_set_value()
discussion that Linus mentions?

> While I like active/inactive better than asserted/deasserted, the latter
> is still way better than high/low, so I didn't discuss.
> 

As a native English speaker, I find deasserted to be awkward - though it
is the appropriate negative of asserted in this context.

And there is no escaping the naming of the active-low, so I'm curious to
know if there is a good reason not to go with active/inactive.

Cheers,
Kent.

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

[Uwe Kleine-König]

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

Hello,

On Tue, May 04, 2021 at 06:24:54PM +0800, Kent Gibson wrote:
> On Tue, May 04, 2021 at 11:14:59AM +0200, Uwe Kleine-König wrote:
> > On Tue, May 04, 2021 at 10:55:46AM +0800, Kent Gibson wrote:
> > > On Mon, May 03, 2021 at 11:05:26PM +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).
> > > 
> > > +1 on clarifying the naming.
> > > 
> > > > 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.
> > > > 
> > > 
> > > You use active/inactive here, but then asserted/deasserted in the patch.
> > 
> > oops, this is an oversight.
> > 
> > > My preference would be the active/inactive, which has more of a level
> > > feel, over the asserted/deasserted which feels more like an edge.
> > > 
> > > And you still use active/inactive in the descriptions, so now we have all
> > > three naming schemes in the mix.  
> > > 
> > > What made you change?
> > 
> > I had active/inactive first, but Linux Walleij requested
> > asserted/deasserted:
> > 
> > https://lore.kernel.org/r/CACRpkdbccHbhYcCyPiSoA7+zGXBtbL_LwLkPB3vQDyOqkTA7EQ@mail.gmail.com
> 
> Thanks - I'd missed that.
> 
> I don't suppose you happen to have a link to the gpiod_set_value()
> discussion that Linus mentions?

I found https://lore.kernel.org/linux-gpio/CACRpkdZAm5AML6cfrX_VrzyADASj1rsVXC3zwtfdo+aRSgX7fQ@mail.gmail.com/
but not that other thread Linus mentions there. I would have expected
https://lore.kernel.org/linux-gpio/?q=GPIO_OUT_ASSERTED to find it, but
it doesn't.

> > While I like active/inactive better than asserted/deasserted, the latter
> > is still way better than high/low, so I didn't discuss.
> > 
> 
> As a native English speaker, I find deasserted to be awkward - though it
> is the appropriate negative of asserted in this context.
> 
> And there is no escaping the naming of the active-low, so I'm curious to

Ack, we shouldn't rename that to assert-low :-)

> know if there is a good reason not to go with active/inactive.

Linus: So we're already 3 out of 3 who would like active/inactive better
than asserted/deasserted. I'm curious about your preference, too.

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 1/2] dt-bindings: gpio: introduce hog properties with less ambiguity

[Linus Walleij]

On Tue, May 4, 2021 at 12:56 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
but not active
> > know if there is a good reason not to go with active/inactive.
>
> Linus: So we're already 3 out of 3 who would like active/inactive better
> than asserted/deasserted. I'm curious about your preference, too.

I suppose it depends on where you come from. In electronics
the terms asserted/deasserted is commonly used and
that is where I'm coming from. Maybe just the materials
I've been subjected to, who knows.

Yours,
Linus Walleij

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

[Kent Gibson]

On Thu, May 06, 2021 at 02:35:41PM +0200, Linus Walleij wrote:
> On Tue, May 4, 2021 at 12:56 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> but not active
> > > know if there is a good reason not to go with active/inactive.
> >
> > Linus: So we're already 3 out of 3 who would like active/inactive better
> > than asserted/deasserted. I'm curious about your preference, too.
> 
> I suppose it depends on where you come from. In electronics
> the terms asserted/deasserted is commonly used and
> that is where I'm coming from. Maybe just the materials
> I've been subjected to, who knows.
> 

I also come from electronics and, depending on context, deasserted can
also mean the line is set to high impedance. Here we are trying to
indicate that the line is actively driven to the inactive state, so
using output-deasserted would be more open to misinterpretation than
output-inactive, no?

Cheers,
Kent.

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

[Linus Walleij]

On Mon, May 3, 2021 at 11:06 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> 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>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Rob Herring]

On Mon, May 03, 2021 at 11:05:26PM +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>
> ---
> Hello,
> 
> I already sent this patch back in July and Linus (Walleij) liked the
> patch but asked for an implementation. For that I added the second patch
> now.
> 
> Best regards
> Uwe
> 
>  Documentation/devicetree/bindings/gpio/gpio.txt | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

The schema in dtschema will need to be updated too. Really, probably all 
or most of gpio.txt needs to be moved there if not already. But for now,

Acked-by: Rob Herring <robh@kernel.org>

> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index a8895d339bfe..1061c346a619 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -196,11 +196,16 @@ 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-deasserted: 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-asserted:   A property specifying to set the GPIO direction as output
> +		     with the active value.
> +
> +For backwards compatibility "output-low" and "output-high" should be supported
> +as aliases for "output-deasserted" and "output-asserted" 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.30.2
>