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@xxxxxxxxxx]
> > 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxx> 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