Allow wakeup_trigger to be defined per gpio button. Currently, all
gpio buttons are set up as IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
It may be more appropriate to only wake the system on one edge, for example
if the gpio is for a Lid Switch.
Signed-off-by: Benson Leung <[email protected]>
---
.../devicetree/bindings/gpio/gpio_keys.txt | 7 +++++++
drivers/input/keyboard/gpio_keys.c | 23 ++++++++++++++++++++--
include/linux/gpio_keys.h | 3 +++
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
index 5c2c021..243f569 100644
--- a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -20,6 +20,13 @@ Optional subnode-properties:
- debounce-interval: Debouncing interval time in milliseconds.
If not specified defaults to 5.
- gpio-key,wakeup: Boolean, button can wake-up the system.
+ - gpio-key,wakeup-trigger : Specifies the type of wakeup behavior.
+ <1> == Rising Edge Trigger
+ <2> == Falling Edge Trigger
+ <3> == Both Rising and Falling Edge Trigger
+ <4> == Level High Trigger
+ <8> == Level Low Trigger
+ If not specified, defaults to <3> == Both Rising and Falling.
Example nodes:
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 440ce32..28b6992 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -40,6 +40,7 @@ struct gpio_button_data {
spinlock_t lock;
bool disabled;
bool key_pressed;
+ unsigned long irqflags;
};
struct gpio_keys_drvdata {
@@ -493,6 +494,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
irqflags = 0;
}
+ bdata->irqflags = irqflags;
+
input_set_capability(input, button->type ?: EV_KEY, button->code);
/*
@@ -643,6 +646,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
if (of_property_read_u32(pp, "debounce-interval",
&button->debounce_interval))
button->debounce_interval = 5;
+
+ of_property_read_u32(pp, "gpio-key,wakeup-trigger",
+ &button->wakeup_trigger);
}
if (pdata->nbuttons == 0) {
@@ -811,8 +817,14 @@ static int 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)
+ if (bdata->button->wakeup) {
+ unsigned int flags;
+ flags = bdata->button->wakeup_trigger &
+ IRQF_TRIGGER_MASK;
+ if (flags && flags != bdata->irqflags)
+ irq_set_irq_type(bdata->irq, flags);
enable_irq_wake(bdata->irq);
+ }
}
} else {
mutex_lock(&input->mutex);
@@ -834,8 +846,15 @@ static int 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)
+ if (bdata->button->wakeup) {
+ unsigned int flags;
+ flags = bdata->button->wakeup_trigger &
+ IRQF_TRIGGER_MASK;
disable_irq_wake(bdata->irq);
+ if (flags && flags != bdata->irqflags)
+ irq_set_irq_type(bdata->irq,
+ bdata->irqflags);
+ }
}
} else {
mutex_lock(&input->mutex);
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index a7e977f..d8d5a4f 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -15,6 +15,9 @@ struct gpio_keys_button {
bool can_disable;
int value; /* axis value for EV_ABS */
unsigned int irq; /* Irq number in case of interrupt keys */
+
+ /* trigger value (IRQF_TRIGGER_*) while used as a wake-up source */
+ unsigned int wakeup_trigger;
};
struct gpio_keys_platform_data {
--
1.8.4
Benson,
On Fri, Sep 13, 2013 at 2:52 PM, Benson Leung <[email protected]> wrote:
> Allow wakeup_trigger to be defined per gpio button. Currently, all
> gpio buttons are set up as IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
> It may be more appropriate to only wake the system on one edge, for example
> if the gpio is for a Lid Switch.
>
> Signed-off-by: Benson Leung <[email protected]>
As discussed out of band, I'd tend to put the masking/error checking
in gpio_keys_get_devtree_pdata() then just rely on non-DT users not to
pass in nonsense, but I don't feel that strongly about it, so:
Reviewed-by: Doug Anderson <[email protected]>
Hi Benson,
On Friday 13 of September 2013 14:52:40 Benson Leung wrote:
> Allow wakeup_trigger to be defined per gpio button. Currently, all
> gpio buttons are set up as IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
> It may be more appropriate to only wake the system on one edge, for
> example if the gpio is for a Lid Switch.
>
> Signed-off-by: Benson Leung <[email protected]>
> ---
> .../devicetree/bindings/gpio/gpio_keys.txt | 7 +++++++
> drivers/input/keyboard/gpio_keys.c | 23
> ++++++++++++++++++++-- include/linux/gpio_keys.h
> | 3 +++
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> b/Documentation/devicetree/bindings/gpio/gpio_keys.txt index
> 5c2c021..243f569 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio_keys.txt
> @@ -20,6 +20,13 @@ Optional subnode-properties:
> - debounce-interval: Debouncing interval time in milliseconds.
> If not specified defaults to 5.
> - gpio-key,wakeup: Boolean, button can wake-up the system.
> + - gpio-key,wakeup-trigger : Specifies the type of wakeup behavior.
> + <1> == Rising Edge Trigger
> + <2> == Falling Edge Trigger
> + <3> == Both Rising and Falling Edge Trigger
> + <4> == Level High Trigger
> + <8> == Level Low Trigger
> + If not specified, defaults to <3> == Both Rising and Falling.
I don't like two things in this patch.
First is that this looks completely like a configuration option, not
hardware description, so it's not something that should be put into DT.
Especially that users might want to use another wake-up trigger depending
on their use cases. I'd rather see this as a sysfs entry.
Another thing is that this driver assumes that key events are indicated by
edges on the GPIO line, so I don't think level trigger setting make any
sense here. I'd rather allow three settings here (through a sysfs knob) -
key down, key up, key down or up.
Best regards,
Tomasz
Hi Tomasz,
On Sat, Sep 14, 2013 at 5:16 AM, Tomasz Figa <[email protected]> wrote:
> I don't like two things in this patch.
>
> First is that this looks completely like a configuration option, not
> hardware description, so it's not something that should be put into DT.
> Especially that users might want to use another wake-up trigger depending
> on their use cases. I'd rather see this as a sysfs entry.
>
> Another thing is that this driver assumes that key events are indicated by
> edges on the GPIO line, so I don't think level trigger setting make any
> sense here. I'd rather allow three settings here (through a sysfs knob) -
> key down, key up, key down or up.
Thanks for your feedback. I agree with your suggestions. I will move
this from platform data and DT to a sysfs property with three settings
: rising, falling, both.
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]