Messages in this thread | | | Date | Tue, 06 Mar 2018 15:44:15 +0800 | From | JeffyChen <> | Subject | Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event action |
| |
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. >
| |