RE: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Bujanda, Hector]

Thanks all for your guidance!

First saying that this patch request was sent having our platforms in k4.14 in the way of upgrading to k5.4.
In those versions the commit e588bb1eae31be73fbec2b731be986a7c09635a4 "gpio: add new SET_CONFIG ioctl() to gpio chardev" by Kent Gibson was not available.

I see that you clearly understand the necessity of having a way of configuring debounce from the userspace.
Our platforms make use of hardware debouncing filtering. Up to now we were using the sysfilesystem to let the user handle gpios (including debounce configuration).
We wanted now to get rid of sysfilesystem and start using gpiolib/libgpiod.... but configuring debounce is blocking us.

Now I clearly see (as pointed by Bartosz Golaszewski) that my suggested GPIO_SET_DEBOUNCE_IOCTL is wrong as it hits the chip file descriptor while 'Modifying any config settings can only happen on lines previously requested too in user-space'.

I agree with all that a flag is needed to allow configuring debounce to '0' which has always meant disabling it.

Also agree with 'Kent Gibson' suggestion of  'You might want to add a flag to the GPIOLINE_FLAGs to indicate if debounce is set'.

I have my doubts if it is compulsory to extend debounce configuration to the gpioevent_requests since the debounce value configured by a user is normally linked to a hardware noise in a line; and that does not change from one gpioevent_requests to another. So I think this configuration would be useful but not compulsory.

I agree with Linus Walleij that 'there is a serious user-facing problem here though, because not all GPIO controllers supports debounce'.
Our platforms have native freescale/NXP gpiochips not supporting hardware debounce and our own gpiochips having hardware debounce.
We have also noticed that 'drivers/input/keyboard/gpio_keys.c contains generic debounce code using kernel timers if the GPIO driver cannot provide debouncing'. That feature is not of our interest (because of having hardware debounce filters) but it would clearly be a very good overall functionality.

Having said all above, I wonder how you want to proceed.
Our current development in k5.4 and libgpiod1.4.1 is much behind master... what makes collaboration (and reusability) a bit more complex.
Also I see the implementation requires a bigger picture than I initially expected.
So I wonder if you want me to do the initial steps of the development (what I foresee will require some back and forth) or you prefer implementing all pieces.

Regards, Hector.


-----Original Message-----
From: Bartosz Golaszewski <brgl@xxxxxxxx> 
Sent: miÃrcoles, 29 de abril de 2020 15:00
To: Linus Walleij <linus.walleij@xxxxxxxxxx>
Cc: Bujanda, Hector <Hector.Bujanda@xxxxxxxx>; open list:GPIO SUBSYSTEM <linux-gpio@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Kent Gibson <warthog618@xxxxxxxxx>
Subject: Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

År., 29 kwi 2020 o 14:38 Linus Walleij <linus.walleij@xxxxxxxxxx> napisaÅ(a):
>
> On Wed, Apr 29, 2020 at 2:06 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> > I understand the need to set debounce time to make line events 
> > reliable. As I see it: there'll be a couple steps to add this.
>
> I think there is a serious user-facing problem here though, because 
> not all GPIO controllers supports debounce, so the call may return 
> "nope" (error code).
>
> I think that is unavoidable with things like pull-up/down or drive 
> strength, but for debounce I think we could do better.

For bias we don't return an error if the operation is not supported by the driver.

> drivers/input/keyboard/gpio_keys.c contains generic debounce code 
> using kernel timers if the GPIO driver cannot provide debouncing, and 
> I have thought for a long time that it would be nice if we could do 
> this generic, so that we always provide debouncing if requested, even 
> for in-kernel consumers but most certainly for userspace consumers, 
> else userspace will just start to reinvent this too.
>

Thanks for bringing this to my attention. This definitely looks like something we could pull into gpiolib for others to use.

Bart