Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi Brain, Thanks for your reply. On 03/02/2018 10:32 AM, Brian Norris wrote: > >What about the 'else' case? Shouldn't we try to handle that? >i think the else case is for irq key, which would generate down and up >events in one irq, so it would use the same trigger type for all these 3 >cases. Not necessarily. It uses whatever trigger was provided in platform/DT/etc. data. You could retrieve that with irq_get_trigger_type() and try to interpret that. Or you could just outlaw using that combination (e.g., in the binding documentation). i think for the IRQ button case the assert/deassert/any are using the same irq trigger type, so it should be ok to leave the wakeup trigger type to be 0(not reconfigure the trigger type)... i've made a v3 to add a comment about that, but forgot to send it :( Brian
Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi, On Fri, Feb 23, 2018 at 06:04:22PM +0800, Jeffy Chen wrote: > On 02/13/2018 06:13 AM, Brian Norris wrote: > > > > > > > > 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,16 @@ 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; > > What about EV_ACT_ANY? And default case? I think for ANY, we're OK > > letting suspend/resume not reconfigure the trigger type, but maybe a > > comment here to note that? > right, will add comment in the next version. > > > > > >+} > > > > } else { > > What about the 'else' case? Shouldn't we try to handle that? > i think the else case is for irq key, which would generate down and up > events in one irq, so it would use the same trigger type for all these 3 > cases. Not necessarily. It uses whatever trigger was provided in platform/DT/etc. data. You could retrieve that with irq_get_trigger_type() and try to interpret that. Or you could just outlaw using that combination (e.g., in the binding documentation). Brian > i'll add comment in the next version too. > > > > Brian > > > >
Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi Enric, Thanks for your reply. On 02/14/2018 06:25 AM, Enric Balletbo Serra wrote: Hi, 2018-02-13 19:25 GMT+01:00 Brian Norris : Hi Enric, On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote: On 12/02/18 23:13, Brian Norris wrote: On Sat, Feb 10, 2018 at 07:09:05PM +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 v2: Specify wakeup event action instead of irq trigger type as Brian suggested. [...] Not sure if you were aware but there is also some discussion related to this, maybe we can join the efforts? v1: https://patchwork.kernel.org/patch/10208261/ v2: https://patchwork.kernel.org/patch/10211147/ Thanks for the pointers. IIUC, that's talking about a different problem: how to utilize a GPIO key in level-triggered mode. That touches similar code, but it doesn't really have anything to do with configuring a different wakeup trigger type. Right, sorry. I see now what you are doing. The two patches would need to be reconciled, if they both are going to be merged. But otherwise, I think they're perfectly fine to be separate. Yes, that's why I got confused, I had those patches applied on my tree and when I tried to apply these failed and I wrongly assumed that were doing the same. Waiting to test the third version ;) right, they are not related, and i should add the level irq case after that series merged :) Thanks, Enric Brian
Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi Brian, Thanks for your reply. On 02/13/2018 06:13 AM, Brian Norris wrote: > >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,16 @@ 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; What about EV_ACT_ANY? And default case? I think for ANY, we're OK letting suspend/resume not reconfigure the trigger type, but maybe a comment here to note that? right, will add comment in the next version. >+ } >} else { What about the 'else' case? Shouldn't we try to handle that? i think the else case is for irq key, which would generate down and up events in one irq, so it would use the same trigger type for all these 3 cases. i'll add comment in the next version too. Brian
Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi, 2018-02-13 19:25 GMT+01:00 Brian Norris : > Hi Enric, > > On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote: >> On 12/02/18 23:13, Brian Norris wrote: >> > On Sat, Feb 10, 2018 at 07:09:05PM +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 v2: >> >> Specify wakeup event action instead of irq trigger type as Brian >> >> suggested. > [...] >> Not sure if you were aware but there is also some discussion related to this, >> maybe we can join the efforts? >> >> v1: https://patchwork.kernel.org/patch/10208261/ >> v2: https://patchwork.kernel.org/patch/10211147/ > > Thanks for the pointers. IIUC, that's talking about a different problem: > how to utilize a GPIO key in level-triggered mode. That touches similar > code, but it doesn't really have anything to do with configuring a > different wakeup trigger type. > Right, sorry. I see now what you are doing. > The two patches would need to be reconciled, if they both are going to > be merged. But otherwise, I think they're perfectly fine to be separate. > Yes, that's why I got confused, I had those patches applied on my tree and when I tried to apply these failed and I wrongly assumed that were doing the same. Waiting to test the third version ;) Thanks, Enric > Brian
Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi Enric, On Tue, Feb 13, 2018 at 11:40:44AM +0100, Enric Balletbo i Serra wrote: > On 12/02/18 23:13, Brian Norris wrote: > > On Sat, Feb 10, 2018 at 07:09:05PM +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 v2: > >> Specify wakeup event action instead of irq trigger type as Brian > >> suggested. [...] > Not sure if you were aware but there is also some discussion related to this, > maybe we can join the efforts? > > v1: https://patchwork.kernel.org/patch/10208261/ > v2: https://patchwork.kernel.org/patch/10211147/ Thanks for the pointers. IIUC, that's talking about a different problem: how to utilize a GPIO key in level-triggered mode. That touches similar code, but it doesn't really have anything to do with configuring a different wakeup trigger type. The two patches would need to be reconciled, if they both are going to be merged. But otherwise, I think they're perfectly fine to be separate. Brian
Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi Jeffy, On 12/02/18 23:13, Brian Norris wrote: > Hi Jeffy, > > On Sat, Feb 10, 2018 at 07:09:05PM +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 v2: >> Specify wakeup event action instead of irq trigger type as Brian >> suggested. >> >> drivers/input/keyboard/gpio_keys.c | 27 +++ >> include/linux/gpio_keys.h | 2 ++ >> include/uapi/linux/input-event-codes.h | 9 + >> 3 files changed, 38 insertions(+) >> >> diff --git a/drivers/input/keyboard/gpio_keys.c >> b/drivers/input/keyboard/gpio_keys.c >> index 87e613dc33b8..5c57339d3999 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,16 @@ 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; > > What about EV_ACT_ANY? And default case? I think for ANY, we're OK > letting suspend/resume not reconfigure the trigger type, but maybe a > comment here to note that? > >> +} >> } else { > > What about the 'else' case? Shouldn't we try to handle that? > > Brian > >> if (!button->irq) { >> dev_err(dev, "Found button without gpio or irq\n"); >> @@ -618,6 +632,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 +734,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 +873,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 +901,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/linu
Re: [PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
Hi Jeffy, On Sat, Feb 10, 2018 at 07:09:05PM +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 v2: > Specify wakeup event action instead of irq trigger type as Brian > suggested. > > drivers/input/keyboard/gpio_keys.c | 27 +++ > include/linux/gpio_keys.h | 2 ++ > include/uapi/linux/input-event-codes.h | 9 + > 3 files changed, 38 insertions(+) > > diff --git a/drivers/input/keyboard/gpio_keys.c > b/drivers/input/keyboard/gpio_keys.c > index 87e613dc33b8..5c57339d3999 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,16 @@ 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; What about EV_ACT_ANY? And default case? I think for ANY, we're OK letting suspend/resume not reconfigure the trigger type, but maybe a comment here to note that? > + } > } else { What about the 'else' case? Shouldn't we try to handle that? Brian > if (!button->irq) { > dev_err(dev, "Found button without gpio or irq\n"); > @@ -618,6 +632,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 +734,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 +873,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 +901,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 b
[PATCH v2 1/3] Input: gpio-keys - add support for wakeup event action
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 v2: Specify wakeup event action instead of irq trigger type as Brian suggested. drivers/input/keyboard/gpio_keys.c | 27 +++ include/linux/gpio_keys.h | 2 ++ include/uapi/linux/input-event-codes.h | 9 + 3 files changed, 38 insertions(+) diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 87e613dc33b8..5c57339d3999 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,16 @@ 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; + } } else { if (!button->irq) { dev_err(dev, "Found button without gpio or irq\n"); @@ -618,6 +632,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 +734,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 +873,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 +901,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 @@ s