gpiolib: fix incorrect IRQ requesting of an active-low lineevent

When a pin is active-low, logical trigger edge should be inverted
to match the same interrupt opportunity.

For example, a button pushed trigger falling edge in ACTIVE_HIGH
case; in ACTIVE_LOW case, the button pushed trigger rising edge.
For user space the IRQ requesting doesn't need to do any
modification except to configuring GPIOHANDLE_REQUEST_ACTIVE_LOW.

Signed-off-by: Michael Wu <michael.wu@vatics.com>
---
 drivers/gpio/gpiolib.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Hi Bartosz,

For example, there is a button which drives level to be low when it is pushed, and drivers level to be high when it is released.
We want to catch the event when the button is pushed.

In user space we configure a line event with the following code:

req.handleflags = GPIOHANDLE_REQUEST_INPUT;
req.eventflags = GPIOEVENT_REQUEST_FALLING_EDGE;

and we hope to get "falling" events by reading the device node:

while (1) {
	read(fd, &dat,sizeof(dat));
	if (dat.id == 0) {
		printf("button pushed\n");
	}
}

Run the same logic on another board which the polarity of the button is inverted. The button drives level to be high when it is pushed.
For the inverted level case, we have to add flag GPIOHANDLE_REQUEST_ACTIVE_LOW:

req.handleflags = GPIOHANDLE_REQUEST_INPUT | GPIOHANDLE_REQUEST_ACTIVE_LOW;
req.eventflags = GPIOEVENT_REQUEST_FALLING_EDGE;

At the result, there are no any events been caught when the button is pushed.
By the way, button releasing will emit a "falling" event.


Sincerely,

Michael Wu 


-----Original Message-----
From: Bartosz Golaszewski [mailto:bgolaszewski@baylibre.com] 
Sent: Friday, July 05, 2019 5:33 PM
To: Michael.Wu(吳忠益)
Cc: Linus Walleij; linux-gpio; LKML; mvp.kutali@gmail.com
Subject: Re: [PATCH] gpiolib: fix incorrect IRQ requesting of an active-low lineevent

pt., 5 lip 2019 o 11:30 Michael Wu <michael.wu@vatics.com> napisał(a):
>
> When a pin is active-low, logical trigger edge should be inverted
> to match the same interrupt opportunity.
>
> For example, a button pushed trigger falling edge in ACTIVE_HIGH
> case; in ACTIVE_LOW case, the button pushed trigger rising edge.
> For user space the IRQ requesting doesn't need to do any
> modification except to configuring GPIOHANDLE_REQUEST_ACTIVE_LOW.
>
> Signed-off-by: Michael Wu <michael.wu@vatics.com>
> ---
>  drivers/gpio/gpiolib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e013d417a936..b98466a05091 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -956,9 +956,11 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>         }
>
>         if (eflags & GPIOEVENT_REQUEST_RISING_EDGE)
> -               irqflags |= IRQF_TRIGGER_RISING;
> +               irqflags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
> +                       IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
>         if (eflags & GPIOEVENT_REQUEST_FALLING_EDGE)
> -               irqflags |= IRQF_TRIGGER_FALLING;
> +               irqflags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
> +                       IRQ_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>         irqflags |= IRQF_ONESHOT;
>
>         INIT_KFIFO(le->events);
> --
> 2.17.1
>

Is this something that causes a bug in user-space? Any scenario to reproduce it?

Bart

pt., 5 lip 2019 o 12:35 <Michael.Wu@vatics.com> napisał(a):
>
> Hi Bartosz,
>
> For example, there is a button which drives level to be low when it is pushed, and drivers level to be high when it is released.
> We want to catch the event when the button is pushed.
>
> In user space we configure a line event with the following code:
>
> req.handleflags = GPIOHANDLE_REQUEST_INPUT;
> req.eventflags = GPIOEVENT_REQUEST_FALLING_EDGE;
>
> and we hope to get "falling" events by reading the device node:
>
> while (1) {
>         read(fd, &dat,sizeof(dat));
>         if (dat.id == 0) {
>                 printf("button pushed\n");
>         }
> }
>
> Run the same logic on another board which the polarity of the button is inverted. The button drives level to be high when it is pushed.
> For the inverted level case, we have to add flag GPIOHANDLE_REQUEST_ACTIVE_LOW:
>
> req.handleflags = GPIOHANDLE_REQUEST_INPUT | GPIOHANDLE_REQUEST_ACTIVE_LOW;
> req.eventflags = GPIOEVENT_REQUEST_FALLING_EDGE;
>
> At the result, there are no any events been caught when the button is pushed.
> By the way, button releasing will emit a "falling" event.
>
>
> Sincerely,
>
> Michael Wu

First: please don't top-post on the mailing list.

Second: have you even built the version you sent? Because I'm getting this:

drivers/gpio/gpiolib.c: In function ‘lineevent_create’:
drivers/gpio/gpiolib.c:963:4: error: ‘IRQ_TRIGGER_RISING’ undeclared
(first use in this function)
    IRQ_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
    ^~~~~~~~~~~~~~~~~~

And third: after fixing the define, this indeed looks like a bug and
I'll need to add a test for that to libgpiod once it's upstream.
Strange we didn't catch it before.

Please send a fixed version and add a Cc tag for stable. Nice catch!

Best regards
Bartosz Golaszewski

On Fri, Jul 5, 2019 at 12:35 PM <Michael.Wu@vatics.com> wrote:

> For example, there is a button which drives level to be low when it is pushed, and drivers level to be high when it is released.
> We want to catch the event when the button is pushed.
>
> In user space we configure a line event with the following code:
>
> req.handleflags = GPIOHANDLE_REQUEST_INPUT;
> req.eventflags = GPIOEVENT_REQUEST_FALLING_EDGE;

But *THIS* is the case that should have
GPIOHANDLE_REQUEST_ACTIVE_LOW, because you push
the button to activate it (it is inactive when not pushed).

Also this should have GPIOEVENT_REQUEST_RISING_EDGE.

> Run the same logic on another board which the polarity of the button is inverted. The button drives level to be high when it is pushed.
> For the inverted level case, we have to add flag GPIOHANDLE_REQUEST_ACTIVE_LOW:
>
> req.handleflags = GPIOHANDLE_REQUEST_INPUT | GPIOHANDLE_REQUEST_ACTIVE_LOW;
> req.eventflags = GPIOEVENT_REQUEST_FALLING_EDGE;

This one should not be active low.

And also have GPIOEVENT_REQUEST_RISING_EDGE.

However I agree that the semantic should change as in the
patch, it makes most logical sense.

The reason it looks as it does is because GPIO line values
and interrupts are two separate subsystems inside the kernel
with their own flags (as you've seen).

But you are right, userspace has no idea about that and should
not have to care.

Yours,
Linus Walleij