[PATCH v3 0/3] gpio-keys: Add support for specifying wakeup event action

[PATCH v3 0/3] gpio-keys: Add support for specifying wakeup event action

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

Changes in v3:
Adding more comments as Brian suggested.

Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.
Specify wakeup event action instead of irq trigger type as Brian
suggested.
Specify wakeup event action instead of irq trigger type as Brian
suggested.

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

 .../devicetree/bindings/input/gpio-keys.txt        |  8 +++++
 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts  |  2 ++
 drivers/input/keyboard/gpio_keys.c                 | 39 ++++++++++++++++++++++
 include/linux/gpio_keys.h                          |  2 ++
 include/uapi/linux/input-event-codes.h             |  9 +++++
 5 files changed, 60 insertions(+)

-- 
2.11.0

[PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action

[Jeffy Chen]

Add support for specifying event actions to trigger 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>
---

Changes in v3:
Adding more comments as Brian suggested.

Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

 drivers/input/keyboard/gpio_keys.c     | 39 ++++++++++++++++++++++++++++++++++
 include/linux/gpio_keys.h              |  2 ++
 include/uapi/linux/input-event-codes.h |  9 ++++++++
 3 files changed, 50 insertions(+)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 87e613dc33b8..607f3960c886 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -45,6 +45,8 @@ struct gpio_button_data {
 	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
 
 	unsigned int irq;
+	unsigned int irq_trigger_type;
+	unsigned int wakeup_trigger_type;
 	spinlock_t lock;
 	bool disabled;
 	bool key_pressed;
@@ -540,6 +542,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	}
 
 	if (bdata->gpiod) {
+		int active_low = gpiod_is_active_low(bdata->gpiod);
+
 		if (button->debounce_interval) {
 			error = gpiod_set_debounce(bdata->gpiod,
 					button->debounce_interval * 1000);
@@ -568,6 +572,22 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		isr = gpio_keys_gpio_isr;
 		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
 
+		switch (button->wakeup_event_action) {
+		case EV_ACT_ASSERTED:
+			bdata->wakeup_trigger_type = active_low ?
+				IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
+			break;
+		case EV_ACT_DEASSERTED:
+			bdata->wakeup_trigger_type = active_low ?
+				IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
+			break;
+		default:
+			/*
+			 * For other cases, we are OK letting suspend/resume
+			 * not reconfigure the trigger type.
+			 */
+			break;
+		}
 	} else {
 		if (!button->irq) {
 			dev_err(dev, "Found button without gpio or irq\n");
@@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 
 		isr = gpio_keys_irq_isr;
 		irqflags = 0;
+
+		/*
+		 * For IRQ buttons, the irq trigger type for press and release
+		 * are the same. So we don't need to reconfigure the trigger
+		 * type for wakeup.
+		 */
 	}
 
 	bdata->code = &ddata->keymap[idx];
@@ -618,6 +644,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 +746,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-event-action",
+					 &button->wakeup_event_action);
+
 		button->can_disable =
 			fwnode_property_read_bool(child, "linux,can-disable");
 
@@ -854,6 +885,10 @@ 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 && bdata->wakeup_trigger_type)
+				irq_set_irq_type(bdata->irq,
+						 bdata->wakeup_trigger_type);
 			if (bdata->button->wakeup)
 				enable_irq_wake(bdata->irq);
 			bdata->suspended = true;
@@ -878,6 +913,10 @@ 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 && bdata->wakeup_trigger_type)
+				irq_set_irq_type(bdata->irq,
+						 bdata->irq_trigger_type);
 			if (bdata->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..7160df54a6fe 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_event_action:	event action to trigger wakeup
  * @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_event_action;
 	int debounce_interval;
 	bool can_disable;
 	int value;
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 53fbae27b280..d7917b0bd438 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -32,6 +32,15 @@
 #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
 
 /*
+ * Event action types
+ */
+#define EV_ACT_ANY			0x00	/* asserted or deasserted */
+#define EV_ACT_ASSERTED			0x01	/* asserted */
+#define EV_ACT_DEASSERTED		0x02	/* deasserted */
+#define EV_ACT_MAX			0x02
+#define EV_ACT_CNT			(EV_ACT_MAX+1)
+
+/*
  * Event types
  */
 
-- 
2.11.0

[PATCH v3 2/3] Input: gpio-keys - allow setting wakeup event action in DT

[Jeffy Chen]

Allow specifying event actions to trigger wakeup when using the
gpio-keys input device as a wakeup source.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v3: None
Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

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

diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt
index a94940481e55..996ce84352cb 100644
--- a/Documentation/devicetree/bindings/input/gpio-keys.txt
+++ b/Documentation/devicetree/bindings/input/gpio-keys.txt
@@ -26,6 +26,14 @@ 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-event-action: 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 defined in linux-event-codes.h:
+		EV_ACT_ASSERTED		- asserted
+		EV_ACT_DEASSERTED	- deasserted
+		EV_ACT_ANY		- both asserted and deasserted
 	- 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

[PATCH v3 3/3] arm64: dts: rockchip: Avoid wakeup when inserting the pen

[Jeffy Chen]

Add wakeup event action for Pen Insert gpio key, to avoid wakeup when
inserting the pen.

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

Changes in v3: None
Changes in v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
index 191a6bcb1704..ef2de0057c5c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
@@ -134,6 +134,8 @@
 		gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
 		linux,code = <SW_PEN_INSERTED>;
 		linux,input-type = <EV_SW>;
+		/* Wakeup only when ejecting */
+		wakeup-event-action = <EV_ACT_DEASSERTED>;
 		wakeup-source;
 	};
 };
-- 
2.11.0

Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action

[Brian Norris]

Hi Jeffy,

On Fri, Mar 02, 2018 at 11:51:00AM +0800, Jeffy Chen wrote:
> Add support for specifying event actions to trigger 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>
> ---
> 
> Changes in v3:
> Adding more comments as Brian suggested.
> 
> Changes in v2:
> Specify wakeup event action instead of irq trigger type as Brian
> suggested.
> 
>  drivers/input/keyboard/gpio_keys.c     | 39 ++++++++++++++++++++++++++++++++++
>  include/linux/gpio_keys.h              |  2 ++
>  include/uapi/linux/input-event-codes.h |  9 ++++++++
>  3 files changed, 50 insertions(+)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 87e613dc33b8..607f3960c886 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c

...

> @@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  
>  		isr = gpio_keys_irq_isr;
>  		irqflags = 0;
> +
> +		/*
> +		 * For IRQ buttons, the irq trigger type for press and release
> +		 * are the same. So we don't need to reconfigure the trigger
> +		 * type for wakeup.
> +		 */

Ah, I think I misunderstood what the (non-GPIO) IRQ-based trigger was
doing. It's just emulating an instantaneous press/release (or, with a
configurable release delay), so there isn't really a way to say "only
wake me on one edge".

So that still doesn't really mean we use the "same" trigger for all
wakeup trigger types. It really just means we can't support any
customization here -- we wake based on whatever the IRQ represents
(which today is probably just EV_ACT_ANY).

I guess it's fine to just ignore the wakeup-trigger-type property here,
but it would also be equally valid to reject it outright I think.

Anyway, that's only a mild complaint. For the whole series:

Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>

Thanks!

>  	}
>  
>  	bdata->code = &ddata->keymap[idx];
...
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 53fbae27b280..d7917b0bd438 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -32,6 +32,15 @@
>  #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
>  
>  /*
> + * Event action types
> + */
> +#define EV_ACT_ANY			0x00	/* asserted or deasserted */
> +#define EV_ACT_ASSERTED			0x01	/* asserted */
> +#define EV_ACT_DEASSERTED		0x02	/* deasserted */
> +#define EV_ACT_MAX			0x02
> +#define EV_ACT_CNT			(EV_ACT_MAX+1)
> +
> +/*
>   * Event types
>   */
>  
> -- 
> 2.11.0
> 
> 

Re: [PATCH v3 3/3] arm64: dts: rockchip: Avoid wakeup when inserting the pen

[Enric Balletbo Serra]

Hi Jeffy,

Thanks for the patch.

2018-03-02 4:51 GMT+01:00 Jeffy Chen <jeffy.chen@rock-chips.com>:
> Add wakeup event action for Pen Insert gpio key, to avoid wakeup when
> inserting the pen.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> Changes in v3: None
> Changes in v2:
> Specify wakeup event action instead of irq trigger type as Brian
> suggested.
>
>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> index 191a6bcb1704..ef2de0057c5c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> @@ -134,6 +134,8 @@
>                 gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
>                 linux,code = <SW_PEN_INSERTED>;
>                 linux,input-type = <EV_SW>;
> +               /* Wakeup only when ejecting */
> +               wakeup-event-action = <EV_ACT_DEASSERTED>;
>                 wakeup-source;
>         };
>  };
> --
> 2.11.0
>
>

Works as expected on my Kevin

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action

[Dmitry Torokhov]

Hi Jeffy,

On Fri, Mar 02, 2018 at 11:51:00AM +0800, Jeffy Chen wrote:
> Add support for specifying event actions to trigger 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>
> ---
> 
> Changes in v3:
> Adding more comments as Brian suggested.
> 
> Changes in v2:
> Specify wakeup event action instead of irq trigger type as Brian
> suggested.
> 
>  drivers/input/keyboard/gpio_keys.c     | 39 ++++++++++++++++++++++++++++++++++
>  include/linux/gpio_keys.h              |  2 ++
>  include/uapi/linux/input-event-codes.h |  9 ++++++++
>  3 files changed, 50 insertions(+)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 87e613dc33b8..607f3960c886 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -45,6 +45,8 @@ struct gpio_button_data {
>  	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
>  
>  	unsigned int irq;
> +	unsigned int irq_trigger_type;
> +	unsigned int wakeup_trigger_type;
>  	spinlock_t lock;
>  	bool disabled;
>  	bool key_pressed;
> @@ -540,6 +542,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  	}
>  
>  	if (bdata->gpiod) {
> +		int active_low = gpiod_is_active_low(bdata->gpiod);
> +
>  		if (button->debounce_interval) {
>  			error = gpiod_set_debounce(bdata->gpiod,
>  					button->debounce_interval * 1000);
> @@ -568,6 +572,22 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  		isr = gpio_keys_gpio_isr;
>  		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
>  
> +		switch (button->wakeup_event_action) {
> +		case EV_ACT_ASSERTED:
> +			bdata->wakeup_trigger_type = active_low ?
> +				IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;

				IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;

> +			break;
> +		case EV_ACT_DEASSERTED:
> +			bdata->wakeup_trigger_type = active_low ?
> +				IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
> +			break;

		case EV_ACT_ANY:
> +		default:
> +			/*
> +			 * For other cases, we are OK letting suspend/resume
> +			 * not reconfigure the trigger type.
> +			 */
> +			break;
> +		}
>  	} else {
>  		if (!button->irq) {
>  			dev_err(dev, "Found button without gpio or irq\n");
> @@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  
>  		isr = gpio_keys_irq_isr;
>  		irqflags = 0;
> +
> +		/*
> +		 * For IRQ buttons, the irq trigger type for press and release
> +		 * are the same. So we don't need to reconfigure the trigger
> +		 * type for wakeup.

That is not entirely accurate. Interrupt triggers button press, which is
followed by either immediate or delayed release. There is no interrupt
for release.

> +		 */
>  	}
>  
>  	bdata->code = &ddata->keymap[idx];
> @@ -618,6 +644,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  		return error;
>  	}
>  
> +	bdata->irq_trigger_type = irq_get_trigger_type(bdata->irq);

Why do we need to store the trigger type? It is always both edges for
gpio-based keys and we do not support changing wakeup trigger for
interrupt-based keys.

> +
>  	return 0;
>  }
>  
> @@ -718,6 +746,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-event-action",
> +					 &button->wakeup_event_action);
> +
>  		button->can_disable =
>  			fwnode_property_read_bool(child, "linux,can-disable");
>  
> @@ -854,6 +885,10 @@ 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 && bdata->wakeup_trigger_type)
> +				irq_set_irq_type(bdata->irq,
> +						 bdata->wakeup_trigger_type);

			if (bdata->button->wakeup) {
				if (bdata->wakeup_trigger_type) {
					error = ...;
				}

				enable_irq_wake(bdata->irq);
			}

Might need to be split into a helper; if you add error handling to
enable_irq_wake() that woudl be great too.

>  			if (bdata->button->wakeup)
>  				enable_irq_wake(bdata->irq);
>  			bdata->suspended = true;
> @@ -878,6 +913,10 @@ 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 && bdata->wakeup_trigger_type)
> +				irq_set_irq_type(bdata->irq,
> +						 bdata->irq_trigger_type);

Just use IRQ_TYPE_EDGE_BOTH.

>  			if (bdata->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..7160df54a6fe 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_event_action:	event action to trigger wakeup
>   * @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_event_action;
>  	int debounce_interval;
>  	bool can_disable;
>  	int value;
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 53fbae27b280..d7917b0bd438 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -32,6 +32,15 @@
>  #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
>  
>  /*
> + * Event action types
> + */
> +#define EV_ACT_ANY			0x00	/* asserted or deasserted */
> +#define EV_ACT_ASSERTED			0x01	/* asserted */
> +#define EV_ACT_DEASSERTED		0x02	/* deasserted */

These do not belong here: they are of no interest to userspace but
simply a driver specific quirk. If you want to share symbolic names add
include/dt-bindings/input/gpio-keys.h


> +#define EV_ACT_MAX			0x02
> +#define EV_ACT_CNT			(EV_ACT_MAX+1)

These are not needed at all: you are not defining input bitmasks.

> +
> +/*
>   * Event types
>   */
>  
> -- 
> 2.11.0
> 
> 

Thanks.

-- 
Dmitry

Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action

[JeffyChen]

Hi Dmitry,

Thanks for your review.

On 03/06/2018 08:30 AM, Dmitry Torokhov wrote:
>> >+		switch (button->wakeup_event_action) {
>> >+		case EV_ACT_ASSERTED:
>> >+			bdata->wakeup_trigger_type = active_low ?
>> >+				IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
> 				IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
>
ok, will fix in next version
>> >+			break;
>> >+		case EV_ACT_DEASSERTED:
>> >+			bdata->wakeup_trigger_type = active_low ?
>> >+				IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>> >+			break;
> 		case EV_ACT_ANY:
ok, will fix in next version
>> >+		default:
>> >+			/*
>> >+			 * For other cases, we are OK letting suspend/resume
>> >+			 * not reconfigure the trigger type.
>> >+			 */
>> >+			break;
>> >+		}
>> >  	} else {
>> >  		if (!button->irq) {
>> >  			dev_err(dev, "Found button without gpio or irq\n");
>> >@@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>> >
>> >  		isr = gpio_keys_irq_isr;
>> >  		irqflags = 0;
>> >+
>> >+		/*
>> >+		 * For IRQ buttons, the irq trigger type for press and release
>> >+		 * are the same. So we don't need to reconfigure the trigger
>> >+		 * type for wakeup.
> That is not entirely accurate. Interrupt triggers button press, which is
> followed by either immediate or delayed release. There is no interrupt
> for release.
ok, will fix the comment
>
>> >+		 */
>> >  	}
>> >
>> >  	bdata->code = &ddata->keymap[idx];
>> >@@ -618,6 +644,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>> >  		return error;
>> >  	}
>> >
>> >+	bdata->irq_trigger_type = irq_get_trigger_type(bdata->irq);
> Why do we need to store the trigger type? It is always both edges for
> gpio-based keys and we do not support changing wakeup trigger for
> interrupt-based keys.
right, this is not needed.
>
>> >+
>> >  	return 0;
>> >  }
>> >
>> >@@ -718,6 +746,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-event-action",
>> >+					 &button->wakeup_event_action);
>> >+
>> >  		button->can_disable =
>> >  			fwnode_property_read_bool(child, "linux,can-disable");
>> >
>> >@@ -854,6 +885,10 @@ 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 && bdata->wakeup_trigger_type)
>> >+				irq_set_irq_type(bdata->irq,
>> >+						 bdata->wakeup_trigger_type);
> 			if (bdata->button->wakeup) {
> 				if (bdata->wakeup_trigger_type) {
> 					error = ...;
> 				}
>
> 				enable_irq_wake(bdata->irq);
> 			}
>
> Might need to be split into a helper; if you add error handling to
> enable_irq_wake() that woudl be great too.
ok, will do that.
>
>> >  			if (bdata->button->wakeup)
>> >  				enable_irq_wake(bdata->irq);
>> >  			bdata->suspended = true;
>> >@@ -878,6 +913,10 @@ 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 && bdata->wakeup_trigger_type)
>> >+				irq_set_irq_type(bdata->irq,
>> >+						 bdata->irq_trigger_type);
> Just use IRQ_TYPE_EDGE_BOTH.
>
>> >  			if (bdata->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..7160df54a6fe 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_event_action:	event action to trigger wakeup
>> >   * @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_event_action;
>> >  	int debounce_interval;
>> >  	bool can_disable;
>> >  	int value;
>> >diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
>> >index 53fbae27b280..d7917b0bd438 100644
>> >--- a/include/uapi/linux/input-event-codes.h
>> >+++ b/include/uapi/linux/input-event-codes.h
>> >@@ -32,6 +32,15 @@
>> >  #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
>> >
>> >  /*
>> >+ * Event action types
>> >+ */
>> >+#define EV_ACT_ANY			0x00	/* asserted or deasserted */
>> >+#define EV_ACT_ASSERTED			0x01	/* asserted */
>> >+#define EV_ACT_DEASSERTED		0x02	/* deasserted */
> These do not belong here: they are of no interest to userspace but
> simply a driver specific quirk. If you want to share symbolic names add
> include/dt-bindings/input/gpio-keys.h
ok
>
>
>> >+#define EV_ACT_MAX			0x02
>> >+#define EV_ACT_CNT			(EV_ACT_MAX+1)
> These are not needed at all: you are not defining input bitmasks.
>