| Message ID | 20180302035102.10084-2-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > -- 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 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.
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. > -- 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
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 */
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(+)