| Message ID | 1459763770-2296-1-git-send-email-navam@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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 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
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.
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; };
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(-)