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

2018-03-12 Thread JeffyChen

Hi Dmitry,

Thanks for your reply.

On 03/11/2018 02:15 AM, Dmitry Torokhov wrote:>> +static int 
gpio_keys_enable_wakeup(struct gpio_button_data *bdata)

> These new helpers need to be __maybe_unused in case we compile on system
> without suspend support.
>
> I also wanted a bit more of error handling, so I ended up with the
> version of the patch below. Can you please try it and let me know if I
> broke it.

sure, the patch you sent works :)

Tested-by: Jeffy Chen 

>
> Thanks.
>
> -- Dmitry
>
> Input: gpio-keys - add support for wakeup event action
>
> From: 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 
> Signed-off-by: Jeffy Chen 
> Reviewed-by: Rob Herring 
> Signed-off-by: Dmitry Torokhov 




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

2018-03-10 Thread Andy Shevchenko
On Sat, Mar 10, 2018 at 8:15 PM, Dmitry Torokhov
 wrote:

> +   while (--i >= 0) {

Just in case

while (i--) {

is slightly better to read.

> +   bdata = &ddata->data[i];
> +   if (bdata->button->wakeup)
> +   gpio_keys_button_disable_wakeup(bdata);
> +   bdata->suspended = false;
> +   }

-- 
With Best Regards,
Andy Shevchenko


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

2018-03-10 Thread Dmitry Torokhov
Hi Jeffy,

On Thu, Mar 08, 2018 at 03:32:11PM +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 
> Signed-off-by: Jeffy Chen 
> ---
> 
> Changes in v5:
> Remove unneeded irq_wake flag as Andy suggested.
> 
> Changes in v4:
> Add dt-binding gpio-keys.h, stop saving irq trigger type, add enable/disable 
> wakeup helpers as Dmitry suggested.
> 
> 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| 63 
> +--
>  include/dt-bindings/input/gpio-keys.h | 13 
>  include/linux/gpio_keys.h |  2 ++
>  3 files changed, 75 insertions(+), 3 deletions(-)
>  create mode 100644 include/dt-bindings/input/gpio-keys.h
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c 
> b/drivers/input/keyboard/gpio_keys.c
> index 87e613dc33b8..f6d5cfd44833 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct gpio_button_data {
>   const struct gpio_keys_button *button;
> @@ -45,6 +46,7 @@ struct gpio_button_data {
>   unsigned int software_debounce; /* in msecs, for GPIO-driven buttons */
>  
>   unsigned int irq;
> + 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,24 @@ 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 ?
> + IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> + break;
> + case EV_ACT_DEASSERTED:
> + bdata->wakeup_trigger_type = active_low ?
> + IRQ_TYPE_EDGE_RISING : IRQ_TYPE_EDGE_FALLING;
> + break;
> + case EV_ACT_ANY:
> + /* fall through */
> + 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 +608,11 @@ static int gpio_keys_setup_key(struct platform_device 
> *pdev,
>  
>   isr = gpio_keys_irq_isr;
>   irqflags = 0;
> +
> + /*
> +  * For IRQ buttons, there is no interrupt for release.
> +  * So we don't need to reconfigure the trigger type for wakeup.
> +  */
>   }
>  
>   bdata->code = &ddata->keymap[idx];
> @@ -718,6 +745,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");
>  
> @@ -845,6 +875,31 @@ static int gpio_keys_probe(struct platform_device *pdev)
>   return 0;
>  }
>  
> +static int gpio_keys_enable_wakeup(struct gpio_button_data *bdata)

These new helpers need to be __maybe_unused in case we compile on system
without suspend support.

I also wanted a bit more of error handling, so I ended up with the
version of the patch below. Can you please try it and let me know if I
broke it.

Thanks.

-- 
Dmitry


Input: gpio-keys - add support for wakeup event action

From: 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 eject

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

2018-03-10 Thread Dmitry Torokhov
On Fri, Mar 09, 2018 at 05:27:21PM -0600, Rob Herring wrote:
> On Thu, Mar 08, 2018 at 03:32:11PM +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 
> > Signed-off-by: Jeffy Chen 
> > ---
> > 
> > Changes in v5:
> > Remove unneeded irq_wake flag as Andy suggested.
> > 
> > Changes in v4:
> > Add dt-binding gpio-keys.h, stop saving irq trigger type, add 
> > enable/disable wakeup helpers as Dmitry suggested.
> > 
> > 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| 63 
> > +--
> >  include/dt-bindings/input/gpio-keys.h | 13 
> 
> This file should be in the binding patch.

No, this is a folly. Both the header, along with the documentation (i.e.
binding doc) should be together with the patch that introduces the
changes. The patches are supposed to be self contained and sufficient to
stand on their own. Neither this header, not the binding doc make
sense without the change to the driver itself.

Next we'll start adding #defines in a separate patch before using
them...

Thanks.

-- 
Dmitry


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

2018-03-09 Thread Rob Herring
On Thu, Mar 08, 2018 at 03:32:11PM +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 
> Signed-off-by: Jeffy Chen 
> ---
> 
> Changes in v5:
> Remove unneeded irq_wake flag as Andy suggested.
> 
> Changes in v4:
> Add dt-binding gpio-keys.h, stop saving irq trigger type, add enable/disable 
> wakeup helpers as Dmitry suggested.
> 
> 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| 63 
> +--
>  include/dt-bindings/input/gpio-keys.h | 13 

This file should be in the binding patch.

>  include/linux/gpio_keys.h |  2 ++
>  3 files changed, 75 insertions(+), 3 deletions(-)
>  create mode 100644 include/dt-bindings/input/gpio-keys.h


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

2018-03-07 Thread 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 
Signed-off-by: Jeffy Chen 
---

Changes in v5:
Remove unneeded irq_wake flag as Andy suggested.

Changes in v4:
Add dt-binding gpio-keys.h, stop saving irq trigger type, add enable/disable 
wakeup helpers as Dmitry suggested.

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| 63 +--
 include/dt-bindings/input/gpio-keys.h | 13 
 include/linux/gpio_keys.h |  2 ++
 3 files changed, 75 insertions(+), 3 deletions(-)
 create mode 100644 include/dt-bindings/input/gpio-keys.h

diff --git a/drivers/input/keyboard/gpio_keys.c 
b/drivers/input/keyboard/gpio_keys.c
index 87e613dc33b8..f6d5cfd44833 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct gpio_button_data {
const struct gpio_keys_button *button;
@@ -45,6 +46,7 @@ struct gpio_button_data {
unsigned int software_debounce; /* in msecs, for GPIO-driven buttons */
 
unsigned int irq;
+   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,24 @@ 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 ?
+   IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
+   break;
+   case EV_ACT_DEASSERTED:
+   bdata->wakeup_trigger_type = active_low ?
+   IRQ_TYPE_EDGE_RISING : IRQ_TYPE_EDGE_FALLING;
+   break;
+   case EV_ACT_ANY:
+   /* fall through */
+   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 +608,11 @@ static int gpio_keys_setup_key(struct platform_device 
*pdev,
 
isr = gpio_keys_irq_isr;
irqflags = 0;
+
+   /*
+* For IRQ buttons, there is no interrupt for release.
+* So we don't need to reconfigure the trigger type for wakeup.
+*/
}
 
bdata->code = &ddata->keymap[idx];
@@ -718,6 +745,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");
 
@@ -845,6 +875,31 @@ static int gpio_keys_probe(struct platform_device *pdev)
return 0;
 }
 
+static int gpio_keys_enable_wakeup(struct gpio_button_data *bdata)
+{
+   int ret;
+
+   ret = enable_irq_wake(bdata->irq);
+   if (ret)
+   return ret;
+
+   if (bdata->wakeup_trigger_type)
+   irq_set_irq_type(bdata->irq, bdata->wakeup_trigger_type);
+
+   return 0;
+}
+
+static void gpio_keys_disable_wakeup(struct gpio_button_data *bdata)
+{
+   /**
+* The trigger type is always both edges for gpio-based keys and we do
+* not support changing wakeup trigger for interrupt-based keys.
+*/
+   if (bdata->wakeup_trigger_type)
+   irq_set_irq_type(bdata->irq, IRQ_TYPE_EDGE_BOTH);
+   disable_irq_wake(bdata->irq);
+}
+
 static int __maybe_unused gpio_keys_suspend(struct device *dev)
 {
struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
@@ -854,8 +909,9 @@ static int __maybe_unused gpio_key