[2/3] Input: gpio-keys - allow setting wakeup interrupt trigger type in DT

Message ID 20180209115510.11868-3-jeffy.chen@rock-chips.com
State Changes Requested
Headers show
Series
  • gpio-keys: Add support for specifying wakeup interrupt trigger type
Related show

Commit Message

JeffyChen Feb. 9, 2018, 11:55 a.m.
Allow specifying a different interrupt trigger type for wakeup when
using the gpio-keys input device as a wakeup source.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 Documentation/devicetree/bindings/input/gpio-keys.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Brian Norris Feb. 9, 2018, 11:42 p.m. | #1
Hi Jeffy,

On Fri, Feb 09, 2018 at 07:55:09PM +0800, Jeffy Chen wrote:
> Allow specifying a different interrupt trigger type for wakeup when
> using the gpio-keys input device as a wakeup source.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
>  Documentation/devicetree/bindings/input/gpio-keys.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
> index a94940481e55..61926cef708f 100644
> --- a/Documentation/devicetree/bindings/input/gpio-keys.txt
> +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
> @@ -26,6 +26,15 @@ Optional subnode-properties:
>  	  If not specified defaults to 5.
>  	- wakeup-source: Boolean, button can wake-up the system.
>  			 (Legacy property supported: "gpio-key,wakeup")
> +	- wakeup-trigger-type: Specifies the interrupt trigger type for wakeup.
> +		 The value is defined in <dt-bindings/interrupt-controller/irq.h>

Do you really want to codify interrupt triggers here? It seems like most
of the information about edge vs. level is already codified elsewhere,
so this becomes a little redundant. And in fact, some bindings may be
specifying a "gpio", not technically an interrupt (at least not
directly), so it feels weird to apply IRQ_* flags to them right here.
Anyway, I think he only piece you really want to describe here is, do we
wake on "event asserted", "event deasserted", or both. (The "none" case
would just mean you shouldn't have the "wakeup-source" property.)

So maybe:

	wakeup-trigger-type: Specifies whether the key should wake the
	system when asserted, when deasserted, or both. This property is
	only valid for keys that wake up the system (e.g., when the
	"wakeup-source" property is also provided). Supported values
	are:
	  1: asserted
	  2: deasserted
	  3: both asserted and deasserted

? We could still make macros out of those, if we want
(input/linux-event-codes.h?). And then leave it up to the driver to
determine how to translate that into the appropriate edge or level
triggers.

Brian

> +		 Only the following flags are supported:
> +			IRQ_TYPE_NONE
> +			IRQ_TYPE_EDGE_RISING
> +			IRQ_TYPE_EDGE_FALLING
> +			IRQ_TYPE_EDGE_BOTH
> +			IRQ_TYPE_LEVEL_HIGH
> +			IRQ_TYPE_LEVEL_LOW
>  	- linux,can-disable: Boolean, indicates that button is connected
>  	  to dedicated (not shared) interrupt which can be disabled to
>  	  suppress events from the button.
> -- 
> 2.11.0
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
JeffyChen Feb. 10, 2018, 12:06 a.m. | #2
Hi Brian,

Thanks for your reply.

On 02/10/2018 07:42 AM, Brian Norris wrote:
> Hi Jeffy,
>
> On Fri, Feb 09, 2018 at 07:55:09PM +0800, Jeffy Chen wrote:
>> Allow specifying a different interrupt trigger type for wakeup when
>> using the gpio-keys input device as a wakeup source.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>>   Documentation/devicetree/bindings/input/gpio-keys.txt | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
>> index a94940481e55..61926cef708f 100644
>> --- a/Documentation/devicetree/bindings/input/gpio-keys.txt
>> +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
>> @@ -26,6 +26,15 @@ Optional subnode-properties:
>>   	  If not specified defaults to 5.
>>   	- wakeup-source: Boolean, button can wake-up the system.
>>   			 (Legacy property supported: "gpio-key,wakeup")
>> +	- wakeup-trigger-type: Specifies the interrupt trigger type for wakeup.
>> +		 The value is defined in <dt-bindings/interrupt-controller/irq.h>
>
> Do you really want to codify interrupt triggers here? It seems like most
> of the information about edge vs. level is already codified elsewhere,
> so this becomes a little redundant. And in fact, some bindings may be
> specifying a "gpio", not technically an interrupt (at least not
> directly), so it feels weird to apply IRQ_* flags to them right here.
> Anyway, I think he only piece you really want to describe here is, do we
> wake on "event asserted", "event deasserted", or both. (The "none" case
> would just mean you shouldn't have the "wakeup-source" property.)
>
> So maybe:
>
> 	wakeup-trigger-type: Specifies whether the key should wake the
> 	system when asserted, when deasserted, or both. This property is
> 	only valid for keys that wake up the system (e.g., when the
> 	"wakeup-source" property is also provided). Supported values
> 	are:
> 	  1: asserted
> 	  2: deasserted
> 	  3: both asserted and deasserted
>
> ? We could still make macros out of those, if we want
> (input/linux-event-codes.h?). And then leave it up to the driver to
> determine how to translate that into the appropriate edge or level
> triggers.
make sense, will do it in the next version.
>
> Brian
>
>> +		 Only the following flags are supported:
>> +			IRQ_TYPE_NONE
>> +			IRQ_TYPE_EDGE_RISING
>> +			IRQ_TYPE_EDGE_FALLING
>> +			IRQ_TYPE_EDGE_BOTH
>> +			IRQ_TYPE_LEVEL_HIGH
>> +			IRQ_TYPE_LEVEL_LOW
>>   	- linux,can-disable: Boolean, indicates that button is connected
>>   	  to dedicated (not shared) interrupt which can be disabled to
>>   	  suppress events from the button.
>> --
>> 2.11.0
>>
>>
>
>
>


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Feb. 18, 2018, 11:34 p.m. | #3
On Fri, Feb 09, 2018 at 03:42:47PM -0800, Brian Norris wrote:
> Hi Jeffy,
> 
> On Fri, Feb 09, 2018 at 07:55:09PM +0800, Jeffy Chen wrote:
> > Allow specifying a different interrupt trigger type for wakeup when
> > using the gpio-keys input device as a wakeup source.
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> > 
> >  Documentation/devicetree/bindings/input/gpio-keys.txt | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
> > index a94940481e55..61926cef708f 100644
> > --- a/Documentation/devicetree/bindings/input/gpio-keys.txt
> > +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
> > @@ -26,6 +26,15 @@ Optional subnode-properties:
> >  	  If not specified defaults to 5.
> >  	- wakeup-source: Boolean, button can wake-up the system.
> >  			 (Legacy property supported: "gpio-key,wakeup")
> > +	- wakeup-trigger-type: Specifies the interrupt trigger type for wakeup.
> > +		 The value is defined in <dt-bindings/interrupt-controller/irq.h>
> 
> Do you really want to codify interrupt triggers here? It seems like most
> of the information about edge vs. level is already codified elsewhere,
> so this becomes a little redundant. And in fact, some bindings may be
> specifying a "gpio", not technically an interrupt (at least not
> directly), so it feels weird to apply IRQ_* flags to them right here.
> Anyway, I think he only piece you really want to describe here is, do we
> wake on "event asserted", "event deasserted", or both. (The "none" case
> would just mean you shouldn't have the "wakeup-source" property.)
> 
> So maybe:
> 
> 	wakeup-trigger-type: Specifies whether the key should wake the
> 	system when asserted, when deasserted, or both. This property is
> 	only valid for keys that wake up the system (e.g., when the
> 	"wakeup-source" property is also provided). Supported values
> 	are:
> 	  1: asserted

As wakeup is an IRQ, that's assumed.

> 	  2: deasserted

Just invert the flags for the IRQ.

> 	  3: both asserted and deasserted

I don't see what would be the usecase. But wouldn't this be any edge 
(because level certainly doesn't make sense)?

> 
> ? We could still make macros out of those, if we want
> (input/linux-event-codes.h?). And then leave it up to the driver to
> determine how to translate that into the appropriate edge or level
> triggers.
> 
> Brian
> 
> > +		 Only the following flags are supported:
> > +			IRQ_TYPE_NONE
> > +			IRQ_TYPE_EDGE_RISING
> > +			IRQ_TYPE_EDGE_FALLING
> > +			IRQ_TYPE_EDGE_BOTH
> > +			IRQ_TYPE_LEVEL_HIGH
> > +			IRQ_TYPE_LEVEL_LOW
> >  	- linux,can-disable: Boolean, indicates that button is connected
> >  	  to dedicated (not shared) interrupt which can be disabled to
> >  	  suppress events from the button.
> > -- 
> > 2.11.0
> > 
> > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Norris Feb. 21, 2018, 5:38 a.m. | #4
Hi,

On Sun, Feb 18, 2018 at 05:34:53PM -0600, Rob Herring wrote:
> On Fri, Feb 09, 2018 at 03:42:47PM -0800, Brian Norris wrote:
> > On Fri, Feb 09, 2018 at 07:55:09PM +0800, Jeffy Chen wrote:
> > > Allow specifying a different interrupt trigger type for wakeup when
> > > using the gpio-keys input device as a wakeup source.
> > > 
> > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > > ---
> > > 
> > >  Documentation/devicetree/bindings/input/gpio-keys.txt | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
> > > index a94940481e55..61926cef708f 100644
> > > --- a/Documentation/devicetree/bindings/input/gpio-keys.txt
> > > +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
> > > @@ -26,6 +26,15 @@ Optional subnode-properties:
> > >  	  If not specified defaults to 5.
> > >  	- wakeup-source: Boolean, button can wake-up the system.
> > >  			 (Legacy property supported: "gpio-key,wakeup")
> > > +	- wakeup-trigger-type: Specifies the interrupt trigger type for wakeup.
> > > +		 The value is defined in <dt-bindings/interrupt-controller/irq.h>
> > 
> > Do you really want to codify interrupt triggers here? It seems like most
> > of the information about edge vs. level is already codified elsewhere,
> > so this becomes a little redundant. And in fact, some bindings may be
> > specifying a "gpio", not technically an interrupt (at least not
> > directly), so it feels weird to apply IRQ_* flags to them right here.
> > Anyway, I think he only piece you really want to describe here is, do we
> > wake on "event asserted", "event deasserted", or both. (The "none" case
> > would just mean you shouldn't have the "wakeup-source" property.)
> > 
> > So maybe:
> > 
> > 	wakeup-trigger-type: Specifies whether the key should wake the
> > 	system when asserted, when deasserted, or both. This property is
> > 	only valid for keys that wake up the system (e.g., when the
> > 	"wakeup-source" property is also provided). Supported values
> > 	are:
> > 	  1: asserted
> 
> As wakeup is an IRQ, that's assumed.
> 
> > 	  2: deasserted
> 
> Just invert the flags for the IRQ.

What? Wouldn't that change the meaning of the key?

> > 	  3: both asserted and deasserted
> 
> I don't see what would be the usecase. But wouldn't this be any edge 
> (because level certainly doesn't make sense)?

Well, #3 is how the driver happens to currently interpret the binding ;)
I believe the idea is that you get wakeup triggers on all events
(pressed or released). I'm not actually sure why though, since it
doesn't really make for a good use case. (We want both edges in S0, but
not really in S3.)

For some background: the case that inspired this is SW_PEN_INSERTED. We
want to receive events on both edges (PEN_INSERTED = "asserted"; and
!PEN_INSERTED = "deasserted", meaning pen ejected) in S0 (e.g., to
show/hide special UI menus when we think the pen is "in use"). But in
S3, we tend to want to wake only when the pen is ejected. We can't
invert the IRQ, because then ejection and insertion get swapped...
Also, per the above, the current wakeup condition is for both edges. We
want to override that.

Brian

> > 
> > ? We could still make macros out of those, if we want
> > (input/linux-event-codes.h?). And then leave it up to the driver to
> > determine how to translate that into the appropriate edge or level
> > triggers.
> > 
> > Brian
> > 
> > > +		 Only the following flags are supported:
> > > +			IRQ_TYPE_NONE
> > > +			IRQ_TYPE_EDGE_RISING
> > > +			IRQ_TYPE_EDGE_FALLING
> > > +			IRQ_TYPE_EDGE_BOTH
> > > +			IRQ_TYPE_LEVEL_HIGH
> > > +			IRQ_TYPE_LEVEL_LOW
> > >  	- linux,can-disable: Boolean, indicates that button is connected
> > >  	  to dedicated (not shared) interrupt which can be disabled to
> > >  	  suppress events from the button.
> > > -- 
> > > 2.11.0
> > > 
> > > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
index a94940481e55..61926cef708f 100644
--- a/Documentation/devicetree/bindings/input/gpio-keys.txt
+++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
@@ -26,6 +26,15 @@  Optional subnode-properties:
 	  If not specified defaults to 5.
 	- wakeup-source: Boolean, button can wake-up the system.
 			 (Legacy property supported: "gpio-key,wakeup")
+	- wakeup-trigger-type: Specifies the interrupt trigger type for wakeup.
+		 The value is defined in <dt-bindings/interrupt-controller/irq.h>
+		 Only the following flags are supported:
+			IRQ_TYPE_NONE
+			IRQ_TYPE_EDGE_RISING
+			IRQ_TYPE_EDGE_FALLING
+			IRQ_TYPE_EDGE_BOTH
+			IRQ_TYPE_LEVEL_HIGH
+			IRQ_TYPE_LEVEL_LOW
 	- linux,can-disable: Boolean, indicates that button is connected
 	  to dedicated (not shared) interrupt which can be disabled to
 	  suppress events from the button.