[PATCH 0/3] gpio-keys: Add support for specifying wakeup interrupt trigger type

[PATCH 0/3] gpio-keys: Add support for specifying wakeup interrupt trigger type

[Jeffy Chen]

On chromebook kevin, we are using gpio-keys for pen insert event. But
we only want it to wakeup the system when ejecting the pen.

So we may need to change the interrupt trigger type during suspending.


Jeffy Chen (3):
  Input: gpio-keys - add support for wakeup interrupt trigger type
  Input: gpio-keys - allow setting wakeup interrupt trigger type in DT
  arm64: dts: rockchip: Avoid wakeup when inserting the pen

 .../devicetree/bindings/input/gpio-keys.txt          |  9 +++++++++
 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts    |  2 ++
 drivers/input/keyboard/gpio_keys.c                   | 20 ++++++++++++++++++--
 include/linux/gpio_keys.h                            |  2 ++
 4 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH 1/3] Input: gpio-keys - add support for wakeup interrupt trigger type

[Jeffy Chen]

Add support for specifying a different interrupt trigger type for wakeup
when using the gpio-keys input device as a wakeup source.

This would allow the device to configure when to wakeup the system. For
example a gpio-keys input device for pen insert, may only want to wakeup
the system when ejecting the pen.

Suggested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/input/keyboard/gpio_keys.c | 20 ++++++++++++++++++--
 include/linux/gpio_keys.h          |  2 ++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 87e613dc33b8..5e5c93b75a71 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -45,6 +45,7 @@ struct gpio_button_data {
 	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
 
 	unsigned int irq;
+	unsigned int irq_trigger_type;
 	spinlock_t lock;
 	bool disabled;
 	bool key_pressed;
@@ -618,6 +619,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		return error;
 	}
 
+	bdata->irq_trigger_type = irq_get_trigger_type(bdata->irq);
+
 	return 0;
 }
 
@@ -718,6 +721,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 			/* legacy name */
 			fwnode_property_read_bool(child, "gpio-key,wakeup");
 
+		fwnode_property_read_u32(child, "wakeup-trigger-type",
+					 &button->wakeup_trigger_type);
+
 		button->can_disable =
 			fwnode_property_read_bool(child, "linux,can-disable");
 
@@ -854,7 +860,12 @@ static int __maybe_unused gpio_keys_suspend(struct device *dev)
 	if (device_may_wakeup(dev)) {
 		for (i = 0; i < ddata->pdata->nbuttons; i++) {
 			struct gpio_button_data *bdata = &ddata->data[i];
-			if (bdata->button->wakeup)
+			const struct gpio_keys_button *button = bdata->button;
+
+			if (button->wakeup && button->wakeup_trigger_type)
+				irq_set_irq_type(bdata->irq,
+						 button->wakeup_trigger_type);
+			if (button->wakeup)
 				enable_irq_wake(bdata->irq);
 			bdata->suspended = true;
 		}
@@ -878,7 +889,12 @@ static int __maybe_unused gpio_keys_resume(struct device *dev)
 	if (device_may_wakeup(dev)) {
 		for (i = 0; i < ddata->pdata->nbuttons; i++) {
 			struct gpio_button_data *bdata = &ddata->data[i];
-			if (bdata->button->wakeup)
+			const struct gpio_keys_button *button = bdata->button;
+
+			if (button->wakeup && button->wakeup_trigger_type)
+				irq_set_irq_type(bdata->irq,
+						 bdata->irq_trigger_type);
+			if (button->wakeup)
 				disable_irq_wake(bdata->irq);
 			bdata->suspended = false;
 		}
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index d06bf77400f1..9b7e7137f768 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -13,6 +13,7 @@ struct device;
  * @desc:		label that will be attached to button's gpio
  * @type:		input event type (%EV_KEY, %EV_SW, %EV_ABS)
  * @wakeup:		configure the button as a wake-up source
+ * @wakeup_trigger_type:	wakeup interrupt trigger type
  * @debounce_interval:	debounce ticks interval in msecs
  * @can_disable:	%true indicates that userspace is allowed to
  *			disable button via sysfs
@@ -26,6 +27,7 @@ struct gpio_keys_button {
 	const char *desc;
 	unsigned int type;
 	int wakeup;
+	int wakeup_trigger_type;
 	int debounce_interval;
 	bool can_disable;
 	int value;
-- 
2.11.0

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

[Jeffy Chen]

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

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

[Brian Norris]

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

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

[JeffyChen]

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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>>
>>   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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html