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

[PATCH v2 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 v2:
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                 | 27 ++++++++++++++++++++++
 include/linux/gpio_keys.h                          |  2 ++
 include/uapi/linux/input-event-codes.h             |  9 ++++++++
 5 files changed, 48 insertions(+)

-- 
2.11.0

[PATCH v2 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 v2:
Specify wakeup event action instead of irq trigger type as Brian
suggested.

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

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 87e613dc33b8..5c57339d3999 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,16 @@ 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;
+		}
 	} else {
 		if (!button->irq) {
 			dev_err(dev, "Found button without gpio or irq\n");
@@ -618,6 +632,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 +734,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 +873,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 +901,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

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

[Brian Norris]

Hi Jeffy,

On Sat, Feb 10, 2018 at 07:09:05PM +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 v2:
> Specify wakeup event action instead of irq trigger type as Brian
> suggested.
> 
>  drivers/input/keyboard/gpio_keys.c     | 27 +++++++++++++++++++++++++++
>  include/linux/gpio_keys.h              |  2 ++
>  include/uapi/linux/input-event-codes.h |  9 +++++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 87e613dc33b8..5c57339d3999 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,16 @@ 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;

What about EV_ACT_ANY? And default case? I think for ANY, we're OK
letting suspend/resume not reconfigure the trigger type, but maybe a
comment here to note that?

> +		}
>  	} else {

What about the 'else' case? Shouldn't we try to handle that?

Brian

>  		if (!button->irq) {
>  			dev_err(dev, "Found button without gpio or irq\n");
> @@ -618,6 +632,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 +734,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 +873,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 +901,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
> 
> 

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

[Enric Balletbo i Serra]

Hi Jeffy,

On 12/02/18 23:13, Brian Norris wrote:
> Hi Jeffy,
> 
> On Sat, Feb 10, 2018 at 07:09:05PM +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 v2:
>> Specify wakeup event action instead of irq trigger type as Brian
>> suggested.
>>
>>  drivers/input/keyboard/gpio_keys.c     | 27 +++++++++++++++++++++++++++
>>  include/linux/gpio_keys.h              |  2 ++
>>  include/uapi/linux/input-event-codes.h |  9 +++++++++
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>> index 87e613dc33b8..5c57339d3999 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,16 @@ 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;
> 
> What about EV_ACT_ANY? And default case? I think for ANY, we're OK
> letting suspend/resume not reconfigure the trigger type, but maybe a
> comment here to note that?
> 
>> +		}
>>  	} else {
> 
> What about the 'else' case? Shouldn't we try to handle that?
> 
> Brian
> 
>>  		if (!button->irq) {
>>  			dev_err(dev, "Found button without gpio or irq\n");
>> @@ -618,6 +632,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 +734,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 +873,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 +901,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
>>
>>

Not sure if you were aware but there is also some discussion related to this,
maybe we can join the efforts?

v1: https://patchwork.kernel.org/patch/10208261/
v2: https://patchwork.kernel.org/patch/10211147/

Best regards,
 Enric

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

[Brian Norris]

Hi Enric,

On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote:
> On 12/02/18 23:13, Brian Norris wrote:
> > On Sat, Feb 10, 2018 at 07:09:05PM +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 v2:
> >> Specify wakeup event action instead of irq trigger type as Brian
> >> suggested.
[...]
> Not sure if you were aware but there is also some discussion related to this,
> maybe we can join the efforts?
> 
> v1: https://patchwork.kernel.org/patch/10208261/
> v2: https://patchwork.kernel.org/patch/10211147/

Thanks for the pointers. IIUC, that's talking about a different problem:
how to utilize a GPIO key in level-triggered mode. That touches similar
code, but it doesn't really have anything to do with configuring a
different wakeup trigger type.

The two patches would need to be reconciled, if they both are going to
be merged. But otherwise, I think they're perfectly fine to be separate.

Brian

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

[Enric Balletbo Serra]

Hi,

2018-02-13 19:25 GMT+01:00 Brian Norris <briannorris@chromium.org>:
> Hi Enric,
>
> On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote:
>> On 12/02/18 23:13, Brian Norris wrote:
>> > On Sat, Feb 10, 2018 at 07:09:05PM +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 v2:
>> >> Specify wakeup event action instead of irq trigger type as Brian
>> >> suggested.
> [...]
>> Not sure if you were aware but there is also some discussion related to this,
>> maybe we can join the efforts?
>>
>> v1: https://patchwork.kernel.org/patch/10208261/
>> v2: https://patchwork.kernel.org/patch/10211147/
>
> Thanks for the pointers. IIUC, that's talking about a different problem:
> how to utilize a GPIO key in level-triggered mode. That touches similar
> code, but it doesn't really have anything to do with configuring a
> different wakeup trigger type.
>

Right, sorry. I see now what you are doing.

> The two patches would need to be reconciled, if they both are going to
> be merged. But otherwise, I think they're perfectly fine to be separate.
>

Yes, that's why I got confused, I had those patches applied on my tree
and when I tried to apply these failed and I wrongly assumed that were
doing the same. Waiting to test the third version ;)

Thanks,
 Enric

> Brian

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

[JeffyChen]

Hi Enric,

Thanks for your reply.

On 02/14/2018 06:25 AM, Enric Balletbo Serra wrote:
> Hi,
>
> 2018-02-13 19:25 GMT+01:00 Brian Norris <briannorris@chromium.org>:
>> Hi Enric,
>>
>> On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote:
>>> On 12/02/18 23:13, Brian Norris wrote:
>>>> On Sat, Feb 10, 2018 at 07:09:05PM +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 v2:
>>>>> Specify wakeup event action instead of irq trigger type as Brian
>>>>> suggested.
>> [...]
>>> Not sure if you were aware but there is also some discussion related to this,
>>> maybe we can join the efforts?
>>>
>>> v1: https://patchwork.kernel.org/patch/10208261/
>>> v2: https://patchwork.kernel.org/patch/10211147/
>>
>> Thanks for the pointers. IIUC, that's talking about a different problem:
>> how to utilize a GPIO key in level-triggered mode. That touches similar
>> code, but it doesn't really have anything to do with configuring a
>> different wakeup trigger type.
>>
>
> Right, sorry. I see now what you are doing.
>
>> The two patches would need to be reconciled, if they both are going to
>> be merged. But otherwise, I think they're perfectly fine to be separate.
>>
>
> Yes, that's why I got confused, I had those patches applied on my tree
> and when I tried to apply these failed and I wrongly assumed that were
> doing the same. Waiting to test the third version ;)
right, they are not related, and i should add the level irq case after 
that series merged :)

>
> Thanks,
>   Enric
>
>> Brian
>
>
>

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

[JeffyChen]

Hi Brian,

Thanks for your reply.

On 02/13/2018 06:13 AM, Brian Norris wrote:
>> >
>> >  	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,16 @@ 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;
> What about EV_ACT_ANY? And default case? I think for ANY, we're OK
> letting suspend/resume not reconfigure the trigger type, but maybe a
> comment here to note that?
right, will add comment in the next version.
>
>> >+		}
>> >  	} else {
> What about the 'else' case? Shouldn't we try to handle that?
i think the else case is for irq key, which would generate down and up 
events in one irq, so it would use the same trigger type for all these 3 
cases.

i'll add comment in the next version too.
>
> Brian
>

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

[Brian Norris]

Hi,

On Fri, Feb 23, 2018 at 06:04:22PM +0800, Jeffy Chen wrote:
> On 02/13/2018 06:13 AM, Brian Norris wrote:
> > > >
> > > >  	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,16 @@ 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;
> > What about EV_ACT_ANY? And default case? I think for ANY, we're OK
> > letting suspend/resume not reconfigure the trigger type, but maybe a
> > comment here to note that?
> right, will add comment in the next version.
> > 
> > > >+		}
> > > >  	} else {
> > What about the 'else' case? Shouldn't we try to handle that?
> i think the else case is for irq key, which would generate down and up
> events in one irq, so it would use the same trigger type for all these 3
> cases.

Not necessarily. It uses whatever trigger was provided in
platform/DT/etc. data. You could retrieve that with
irq_get_trigger_type() and try to interpret that. Or you could just
outlaw using that combination (e.g., in the binding documentation).

Brian

> i'll add comment in the next version too.
> > 
> > Brian
> > 
> 
> 

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

[JeffyChen]

Hi Brain,

Thanks for your reply.

On 03/02/2018 10:32 AM, Brian Norris wrote:
>>> > >What about the 'else' case? Shouldn't we try to handle that?
>> >i think the else case is for irq key, which would generate down and up
>> >events in one irq, so it would use the same trigger type for all these 3
>> >cases.
> Not necessarily. It uses whatever trigger was provided in
> platform/DT/etc. data. You could retrieve that with
> irq_get_trigger_type() and try to interpret that. Or you could just
> outlaw using that combination (e.g., in the binding documentation).
i think for the IRQ button case the assert/deassert/any are using the 
same irq trigger type, so it should be ok to leave the wakeup trigger 
type to be 0(not reconfigure the trigger type)...

i've made a v3 to add a comment about that, but forgot to send it :(

>
> Brian
>

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

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

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

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

[Rob Herring]

On Sat, Feb 10, 2018 at 07:09:06PM +0800, Jeffy Chen wrote:
> Allow specifying event actions to trigger wakeup when using the
> gpio-keys input device as a wakeup source.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> 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(+)

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 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 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 v2 0/3] gpio-keys: Add support for specifying wakeup event action

[Heiko Stübner]

Hi Jeffy,

Am Samstag, 10. Februar 2018, 12:09:04 CET schrieb 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 v2:
> 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

The recipient-selection for the mail thread seems to be a bit off and
I only got the cover-letter and patch 3/3.
Therefore I won't really see if and when the Input-changes get
accepted. So please ping me once that has happened so I can pick up
the rockchip dts change for it.


Thanks
Heiko

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

[JeffyChen]

Hi Heiko,

Thanks for your reply :)

On 02/14/2018 09:39 PM, Heiko Stübner wrote:
> Hi Jeffy,
>
> Am Samstag, 10. Februar 2018, 12:09:04 CET schrieb 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 v2:
>> 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
>
> The recipient-selection for the mail thread seems to be a bit off and
> I only got the cover-letter and patch 3/3.
> Therefore I won't really see if and when the Input-changes get
> accepted. So please ping me once that has happened so I can pick up
> the rockchip dts change for it.
oops, sorry, i'll add you to the CC list in the next version :)

>
>
> Thanks
> Heiko
>
>
>