diff mbox series

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

Message ID 20210503210526.43455-1-u.kleine-koenig@pengutronix.de
State New
Headers show
Series [1/2] dt-bindings: gpio: introduce hog properties with less ambiguity | expand

Commit Message

Uwe Kleine-König May 3, 2021, 9:05 p.m. UTC
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(-)

Comments

Kent Gibson May 4, 2021, 2:55 a.m. UTC | #1
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

>
Uwe Kleine-König May 4, 2021, 9:14 a.m. UTC | #2
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/ |
Kent Gibson May 4, 2021, 10:24 a.m. UTC | #3
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.
Uwe Kleine-König May 4, 2021, 10:56 a.m. UTC | #4
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
Linus Walleij May 6, 2021, 12:35 p.m. UTC | #5
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
Linus Walleij May 6, 2021, 12:37 p.m. UTC | #6
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
Kent Gibson May 6, 2021, 3:34 p.m. UTC | #7
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.
Rob Herring (Arm) May 6, 2021, 6:31 p.m. UTC | #8
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
>
diff mbox series

Patch

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.