All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
@ 2018-02-28 12:44 Baolin Wang
  2018-02-28 14:44 ` Arnd Bergmann
  0 siblings, 1 reply; 5+ messages in thread
From: Baolin Wang @ 2018-02-28 12:44 UTC (permalink / raw)
  To: dmitry.torokhov, robh+dt, mark.rutland
  Cc: gregkh, lumotuwe, arvind.yadav.cs, josephl, kstewart,
	pombredanne, tglx, linux-input, devicetree, linux-kernel,
	broonie, linus.walleij, arnd, baolin.wang

On some platforms (such as Spreadtrum platform), the GPIO keys can only
be triggered by level type. So this patch introduces one property to
indicate if the GPIO trigger type is level trigger or edge trigger.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v2:
 - Use 'interrupt' property to indicate the irq type.

Changes since v1:
 - Diable the GPIO irq until reversing the GPIO level type.
---
 drivers/input/keyboard/gpio_keys.c |   32 ++++++++++++++++++++++++++++++--
 include/linux/gpio_keys.h          |    3 +++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 87e613d..9e05c80 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -385,6 +385,20 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
 	struct gpio_button_data *bdata =
 		container_of(work, struct gpio_button_data, work.work);
 
+	if (bdata->button->level_trigger) {
+		unsigned int trigger =
+			irq_get_trigger_type(bdata->irq) & ~IRQF_TRIGGER_MASK;
+		int state = gpiod_get_raw_value_cansleep(bdata->gpiod);
+
+		if (state)
+			trigger |= IRQF_TRIGGER_LOW;
+		else
+			trigger |= IRQF_TRIGGER_HIGH;
+
+		irq_set_irq_type(bdata->irq, trigger);
+		enable_irq(bdata->irq);
+	}
+
 	gpio_keys_gpio_report_event(bdata);
 
 	if (bdata->button->wakeup)
@@ -397,6 +411,9 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 
 	BUG_ON(irq != bdata->irq);
 
+	if (bdata->button->level_trigger)
+		disable_irq_nosync(bdata->irq);
+
 	if (bdata->button->wakeup) {
 		const struct gpio_keys_button *button = bdata->button;
 
@@ -566,7 +583,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func);
 
 		isr = gpio_keys_gpio_isr;
-		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+		if (button->level_trigger)
+			irqflags = gpiod_is_active_low(bdata->gpiod) ?
+				IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH;
+		else
+			irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
 
 	} else {
 		if (!button->irq) {
@@ -696,10 +717,17 @@ static void gpio_keys_close(struct input_dev *input)
 	device_property_read_string(dev, "label", &pdata->name);
 
 	device_for_each_child_node(dev, child) {
-		if (is_of_node(child))
+		if (is_of_node(child)) {
 			button->irq =
 				irq_of_parse_and_map(to_of_node(child), 0);
 
+			if (button->irq)
+				button->level_trigger =
+					irq_get_trigger_type(button->irq) &
+					(IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW) ?
+					true : false;
+		}
+
 		if (fwnode_property_read_u32(child, "linux,code",
 					     &button->code)) {
 			dev_err(dev, "Button without keycode\n");
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index d06bf77..1286136 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -16,6 +16,8 @@
  * @debounce_interval:	debounce ticks interval in msecs
  * @can_disable:	%true indicates that userspace is allowed to
  *			disable button via sysfs
+ * @level_trigger:	indicate if the button's interrupt type is
+ *			level trigger or not
  * @value:		axis value for %EV_ABS
  * @irq:		Irq number in case of interrupt keys
  */
@@ -28,6 +30,7 @@ struct gpio_keys_button {
 	int wakeup;
 	int debounce_interval;
 	bool can_disable;
+	bool level_trigger;
 	int value;
 	unsigned int irq;
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
  2018-02-28 12:44 [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys Baolin Wang
@ 2018-02-28 14:44 ` Arnd Bergmann
  2018-03-01  7:35   ` Baolin Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2018-02-28 14:44 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, gregkh, lumotuwe,
	Arvind Yadav, josephl, kstewart, Philippe Ombredanne,
	Thomas Gleixner, open list:HID CORE LAYER, DTML,
	Linux Kernel Mailing List, Mark Brown, Linus Walleij

On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On some platforms (such as Spreadtrum platform), the GPIO keys can only
> be triggered by level type. So this patch introduces one property to
> indicate if the GPIO trigger type is level trigger or edge trigger.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes since v2:
>  - Use 'interrupt' property to indicate the irq type.
>
> Changes since v1:
>  - Diable the GPIO irq until reversing the GPIO level type.

I've looked at your patch in more detail now, and given it a bit more thought.

I wonder if you could move that logic into your gpiochip/irqchip driver instead.
It seems that what you do in the gpio-keys driver is to emulate edge triggered
behavior on a level triggered irqchip.

If you put the same logic into the gpio driver, you could simply make it
pretend to support an edge trigger on both edges and call into the interrupt
handler whenever the state changes.

        Arnd

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
  2018-02-28 14:44 ` Arnd Bergmann
@ 2018-03-01  7:35   ` Baolin Wang
  2018-03-02  0:44     ` Dmitry Torokhov
  0 siblings, 1 reply; 5+ messages in thread
From: Baolin Wang @ 2018-03-01  7:35 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, gregkh, stephen lu,
	Arvind Yadav, Joseph Lo, Kate Stewart, Philippe Ombredanne,
	Thomas Gleixner, open list:HID CORE LAYER, DTML,
	Linux Kernel Mailing List, Mark Brown, Linus Walleij

On 28 February 2018 at 22:44, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On some platforms (such as Spreadtrum platform), the GPIO keys can only
>> be triggered by level type. So this patch introduces one property to
>> indicate if the GPIO trigger type is level trigger or edge trigger.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes since v2:
>>  - Use 'interrupt' property to indicate the irq type.
>>
>> Changes since v1:
>>  - Diable the GPIO irq until reversing the GPIO level type.
>
> I've looked at your patch in more detail now, and given it a bit more thought.
>
> I wonder if you could move that logic into your gpiochip/irqchip driver instead.
> It seems that what you do in the gpio-keys driver is to emulate edge triggered
> behavior on a level triggered irqchip.
>
> If you put the same logic into the gpio driver, you could simply make it
> pretend to support an edge trigger on both edges and call into the interrupt
> handler whenever the state changes.
>

That is really a good suggestion, which can avoid duplicate level
reverse logic in different drivers. So this patch can be simplified
just adding one trigger_type to indicate the interrupt type (not
always edge trigger). Thanks for your suggestion.

-- 
Baolin.wang
Best Regards

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
  2018-03-01  7:35   ` Baolin Wang
@ 2018-03-02  0:44     ` Dmitry Torokhov
  2018-03-02  3:10       ` Baolin Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Torokhov @ 2018-03-02  0:44 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Arnd Bergmann, Rob Herring, Mark Rutland, gregkh, stephen lu,
	Arvind Yadav, Joseph Lo, Kate Stewart, Philippe Ombredanne,
	Thomas Gleixner, open list:HID CORE LAYER, DTML,
	Linux Kernel Mailing List, Mark Brown, Linus Walleij

On Thu, Mar 01, 2018 at 03:35:23PM +0800, Baolin Wang wrote:
> On 28 February 2018 at 22:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
> >> On some platforms (such as Spreadtrum platform), the GPIO keys can only
> >> be triggered by level type. So this patch introduces one property to
> >> indicate if the GPIO trigger type is level trigger or edge trigger.
> >>
> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >> ---
> >> Changes since v2:
> >>  - Use 'interrupt' property to indicate the irq type.
> >>
> >> Changes since v1:
> >>  - Diable the GPIO irq until reversing the GPIO level type.
> >
> > I've looked at your patch in more detail now, and given it a bit more thought.
> >
> > I wonder if you could move that logic into your gpiochip/irqchip driver instead.
> > It seems that what you do in the gpio-keys driver is to emulate edge triggered
> > behavior on a level triggered irqchip.
> >
> > If you put the same logic into the gpio driver, you could simply make it
> > pretend to support an edge trigger on both edges and call into the interrupt
> > handler whenever the state changes.
> >
> 
> That is really a good suggestion, which can avoid duplicate level
> reverse logic in different drivers. So this patch can be simplified
> just adding one trigger_type to indicate the interrupt type (not
> always edge trigger). Thanks for your suggestion.

No, there is no need to add trigger type. The gpio-keys driver expects
trigger with both edges, falling and rising. If your GPIO chip does not
support it natively, you need to emulate edge trigger via level
interrupts by reprogramming trigger from active low to active high and
back on the fly. This should be done in the gpiochip/irqchip driver.
There is no need to change gpio-keys driver.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys
  2018-03-02  0:44     ` Dmitry Torokhov
@ 2018-03-02  3:10       ` Baolin Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Baolin Wang @ 2018-03-02  3:10 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Arnd Bergmann, Rob Herring, Mark Rutland, gregkh, stephen lu,
	Arvind Yadav, Joseph Lo, Kate Stewart, Philippe Ombredanne,
	Thomas Gleixner, open list:HID CORE LAYER, DTML,
	Linux Kernel Mailing List, Mark Brown, Linus Walleij

On 2 March 2018 at 08:44, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Thu, Mar 01, 2018 at 03:35:23PM +0800, Baolin Wang wrote:
>> On 28 February 2018 at 22:44, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> >> On some platforms (such as Spreadtrum platform), the GPIO keys can only
>> >> be triggered by level type. So this patch introduces one property to
>> >> indicate if the GPIO trigger type is level trigger or edge trigger.
>> >>
>> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> >> ---
>> >> Changes since v2:
>> >>  - Use 'interrupt' property to indicate the irq type.
>> >>
>> >> Changes since v1:
>> >>  - Diable the GPIO irq until reversing the GPIO level type.
>> >
>> > I've looked at your patch in more detail now, and given it a bit more thought.
>> >
>> > I wonder if you could move that logic into your gpiochip/irqchip driver instead.
>> > It seems that what you do in the gpio-keys driver is to emulate edge triggered
>> > behavior on a level triggered irqchip.
>> >
>> > If you put the same logic into the gpio driver, you could simply make it
>> > pretend to support an edge trigger on both edges and call into the interrupt
>> > handler whenever the state changes.
>> >
>>
>> That is really a good suggestion, which can avoid duplicate level
>> reverse logic in different drivers. So this patch can be simplified
>> just adding one trigger_type to indicate the interrupt type (not
>> always edge trigger). Thanks for your suggestion.
>
> No, there is no need to add trigger type. The gpio-keys driver expects
> trigger with both edges, falling and rising. If your GPIO chip does not
> support it natively, you need to emulate edge trigger via level
> interrupts by reprogramming trigger from active low to active high and
> back on the fly. This should be done in the gpiochip/irqchip driver.
> There is no need to change gpio-keys driver.

Yes, you are right, It can work on my platform after modifying my GPIO
driver like you and Arnd's suggestion. Thanks for all your help.

-- 
Baolin.wang
Best Regards

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-03-02  3:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-28 12:44 [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys Baolin Wang
2018-02-28 14:44 ` Arnd Bergmann
2018-03-01  7:35   ` Baolin Wang
2018-03-02  0:44     ` Dmitry Torokhov
2018-03-02  3:10       ` Baolin Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.