| Message ID | 1379109160-32437-1-git-send-email-bleung@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Benson, On Fri, Sep 13, 2013 at 2:52 PM, Benson Leung <bleung@chromium.org> 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 <bleung@chromium.org> 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 <dianders@chromium.org> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <bleung@chromium.org> > --- > .../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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On Sat, Sep 14, 2013 at 5:16 AM, Tomasz Figa <tomasz.figa@gmail.com> 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.
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 {
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 <bleung@chromium.org> --- .../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(-)