[LINUX PATCH v2] gpio_keys: Added support to read the IRQ_FLAGS from devicetree

[LINUX PATCH v2] gpio_keys: Added support to read the IRQ_FLAGS from devicetree

[Nava kishore Manne]

This patch adds the support to read the IRQ_FLAGS from the device
instead of hard code the flags in gpio_keys_setup_key().

example gpio-keys DT node:

gpio-keys {
	compatible = "gpio-keys";
	#address-cells = <1>;
	#size-cells = <0>;
	autorepeat;
		sw14 {
			label = "sw14";
			gpios = <&gpio0 12 1>;
			/*
			 * Triggering Type:
			 *
			 * 1 - edge rising
			 * 2 - edge falling
			 * 4 - level active high
			 * 8 - level active low
			 *
			 */
			linux,code = <108>; /* down */
			gpio-key,wakeup;
			autorepeat;
		};
	};

Signed-off-by: Nava kishore Manne <navam@xilinx.com>
---
Changes for v2:
		-None

 drivers/input/keyboard/gpio_keys.c | 8 ++++----
 include/linux/gpio_keys.h          | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index bef317f..07b50ad 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -497,7 +497,7 @@ 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;
+		irqflags = button->irq_flags;
 
 	} else {
 		if (!button->irq) {
@@ -630,11 +630,10 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 
 	i = 0;
 	for_each_child_of_node(node, pp) {
-		enum of_gpio_flags flags;
 
 		button = &pdata->buttons[i++];
 
-		button->gpio = of_get_gpio_flags(pp, 0, &flags);
+		button->gpio = of_get_gpio_flags(pp, 0, &button->irq_flags);
 		if (button->gpio < 0) {
 			error = button->gpio;
 			if (error != -ENOENT) {
@@ -645,7 +644,8 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 				return ERR_PTR(error);
 			}
 		} else {
-			button->active_low = flags & OF_GPIO_ACTIVE_LOW;
+			button->active_low = button->irq_flags
+						& OF_GPIO_ACTIVE_LOW;
 		}
 
 		button->irq = irq_of_parse_and_map(pp, 0);
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index ee2d8c6..0aeecea 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -31,6 +31,7 @@ struct gpio_keys_button {
 	bool can_disable;
 	int value;
 	unsigned int irq;
+	unsigned int irq_flags;
 	struct gpio_desc *gpiod;
 };
 
-- 
2.1.2

Re: [LINUX PATCH v2] gpio_keys: Added support to read the IRQ_FLAGS from devicetree

[Linus Walleij]

On Mon, Apr 4, 2016 at 11:56 AM, Nava kishore Manne
<nava.manne@xilinx.com> wrote:

> This patch adds the support to read the IRQ_FLAGS from the device
> instead of hard code the flags in gpio_keys_setup_key().

NACK

>                 sw14 {
>                         label = "sw14";
>                         gpios = <&gpio0 12 1>;
>                         /*
>                          * Triggering Type:
>                          *
>                          * 1 - edge rising
>                          * 2 - edge falling
>                          * 4 - level active high
>                          * 8 - level active low
>                          *
>                          */

You are completely violating the existing GPIO flags from
include/dt-bindings/gpio/gpio.h

As you will see, for a twocell GPIO flags are already
clearly defined for 0,1,2 and 3. (Bit 0 & 1).

Further, these IRQ edge/level flags already exist in
include/dt-bindings/interrupt-controller/irq.h
but you should not be using those either, because they
do not mix with a GPIO specifier, it's a bit like oil and water.

The standard GPIO bindings already has
GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
which makes it pretty clear that a GPIO line marked
as GPIO_ACTIVE_HIGH should trigger either on
rising edge or level active high and vice versa.

The only information you could *possibly* lack is
whether the IRQ should be edge or level triggered.

But level triggered GPIO buttons *does* *not* *make*
*sense* *at* *all*.

Think about it:

The IRQ line goes level high or low because a user
pressed a button with his/her thumb. Then that is wired
in as a level IRQ. So what are we going to do? Wait in
the interrupt handler until the user removes his/her
thumb?

Level IRQs on GPIOs only makes sense for devices
off-chip where you can talk to the device and ACK the
interrupt, and in this case "talk" does not mean wire
up a speaker telling the user to remove the thumb from
the button because we have recieved the interrupt, albeit
that would be the real-world analogy.

Please tell us what you are actually trying to solve.

Yours,
Linus Walleij

RE: [LINUX PATCH v2] gpio_keys: Added support to read the IRQ_FLAGS from devicetree

[Nava kishore Manne]

Hi Linus walleij,

	One of Our gpio-controller was supporting only edge rising   interrupts. For that reason I implementing the below logic to read the interrupt trigger level from the DT. If it is wrong could you please provide the pointer to solve this issue?

Regards,
Navakishore.



> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Monday, April 04, 2016 4:38 PM
> To: Nava kishore Manne
> Cc: Dmitry Torokhov; Andersson, Björn; Nava kishore Manne; Peng Fan;
> Linux Input; linux-kernel@vger.kernel.org
> Subject: Re: [LINUX PATCH v2] gpio_keys: Added support to read the
> IRQ_FLAGS from devicetree
> 
> On Mon, Apr 4, 2016 at 11:56 AM, Nava kishore Manne
> <nava.manne@xilinx.com> wrote:
> 
> > This patch adds the support to read the IRQ_FLAGS from the device
> > instead of hard code the flags in gpio_keys_setup_key().
> 
> NACK
> 
> >                 sw14 {
> >                         label = "sw14";
> >                         gpios = <&gpio0 12 1>;
> >                         /*
> >                          * Triggering Type:
> >                          *
> >                          * 1 - edge rising
> >                          * 2 - edge falling
> >                          * 4 - level active high
> >                          * 8 - level active low
> >                          *
> >                          */
> 
> You are completely violating the existing GPIO flags from include/dt-
> bindings/gpio/gpio.h
> 
> As you will see, for a twocell GPIO flags are already clearly defined for 0,1,2
> and 3. (Bit 0 & 1).
> 
> Further, these IRQ edge/level flags already exist in include/dt-
> bindings/interrupt-controller/irq.h
> but you should not be using those either, because they do not mix with a
> GPIO specifier, it's a bit like oil and water.
> 
> The standard GPIO bindings already has
> GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> which makes it pretty clear that a GPIO line marked as GPIO_ACTIVE_HIGH
> should trigger either on rising edge or level active high and vice versa.
> 
> The only information you could *possibly* lack is whether the IRQ should be
> edge or level triggered.
> 
> But level triggered GPIO buttons *does* *not* *make*
> *sense* *at* *all*.
> 
> Think about it:
> 
> The IRQ line goes level high or low because a user pressed a button with
> his/her thumb. Then that is wired in as a level IRQ. So what are we going to
> do? Wait in the interrupt handler until the user removes his/her thumb?
> 
> Level IRQs on GPIOs only makes sense for devices off-chip where you can
> talk to the device and ACK the interrupt, and in this case "talk" does not
> mean wire up a speaker telling the user to remove the thumb from the
> button because we have recieved the interrupt, albeit that would be the
> real-world analogy.
> 
> Please tell us what you are actually trying to solve.


	One of Our gpio-controller was supporting only edge rising   interrupts. For that reason I implementing the below logic to read the interrupt trigger level from the DT. If it is wrong could you please provide the pointer to solve this issue?

Regards,
Navakishore.

> 
> Yours,
> Linus Walleij

Re: [LINUX PATCH v2] gpio_keys: Added support to read the IRQ_FLAGS from devicetree

[Dmitry Torokhov]

On Wed, Apr 06, 2016 at 11:32:55AM +0000, Nava kishore Manne wrote:
> > -----Original Message-----
> > From: Linus Walleij [mailto:linus.walleij@linaro.org]
> > Sent: Monday, April 04, 2016 4:38 PM
> > To: Nava kishore Manne
> > Cc: Dmitry Torokhov; Andersson, Björn; Nava kishore Manne; Peng Fan;
> > Linux Input; linux-kernel@vger.kernel.org
> > Subject: Re: [LINUX PATCH v2] gpio_keys: Added support to read the
> > IRQ_FLAGS from devicetree
> > 
> > On Mon, Apr 4, 2016 at 11:56 AM, Nava kishore Manne
> > <nava.manne@xilinx.com> wrote:
> > 
> > > This patch adds the support to read the IRQ_FLAGS from the device
> > > instead of hard code the flags in gpio_keys_setup_key().
> > 
> > NACK
> > 
> > >                 sw14 {
> > >                         label = "sw14";
> > >                         gpios = <&gpio0 12 1>;
> > >                         /*
> > >                          * Triggering Type:
> > >                          *
> > >                          * 1 - edge rising
> > >                          * 2 - edge falling
> > >                          * 4 - level active high
> > >                          * 8 - level active low
> > >                          *
> > >                          */
> > 
> > You are completely violating the existing GPIO flags from include/dt-
> > bindings/gpio/gpio.h
> > 
> > As you will see, for a twocell GPIO flags are already clearly defined for 0,1,2
> > and 3. (Bit 0 & 1).
> > 
> > Further, these IRQ edge/level flags already exist in include/dt-
> > bindings/interrupt-controller/irq.h
> > but you should not be using those either, because they do not mix with a
> > GPIO specifier, it's a bit like oil and water.
> > 
> > The standard GPIO bindings already has
> > GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> > which makes it pretty clear that a GPIO line marked as GPIO_ACTIVE_HIGH
> > should trigger either on rising edge or level active high and vice versa.
> > 
> > The only information you could *possibly* lack is whether the IRQ should be
> > edge or level triggered.
> > 
> > But level triggered GPIO buttons *does* *not* *make*
> > *sense* *at* *all*.
> > 
> > Think about it:
> > 
> > The IRQ line goes level high or low because a user pressed a button with
> > his/her thumb. Then that is wired in as a level IRQ. So what are we going to
> > do? Wait in the interrupt handler until the user removes his/her thumb?
> > 
> > Level IRQs on GPIOs only makes sense for devices off-chip where you can
> > talk to the device and ACK the interrupt, and in this case "talk" does not
> > mean wire up a speaker telling the user to remove the thumb from the
> > button because we have recieved the interrupt, albeit that would be the
> > real-world analogy.
> > 
> > Please tell us what you are actually trying to solve.
> 
> 
> One of Our gpio-controller was supporting only edge rising interrupts.
> For that reason I implementing the below logic to read the interrupt
> trigger level from the DT. If it is wrong could you please provide the
> pointer to solve this issue?

How will you handle key releases if you can only signal key presses?
gpio-keys driver needs to be notified about both edges.

Thanks.

-- 
Dmitry