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

[Rob Herring]

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@xxxxxxxxxxxxxx>
> > ---
> > 
> >  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
> > 
> >