[PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Hector Bujanda]

This allows calling gpiod_set_debounce function through char device ioctl.

Signed-off-by: Hector Bujanda <hector.bujanda@digi.com>
---
 drivers/gpio/gpiolib.c    | 12 ++++++++++++
 include/uapi/linux/gpio.h | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 70f0dedca59f..c959c2962f15 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1073,6 +1073,18 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
 		return 0;
+	} else if (cmd == GPIO_SET_DEBOUNCE_IOCTL) {
+		struct gpioline_debounce linedebounce;
+		struct gpio_desc *desc;
+
+		if (copy_from_user(&linedebounce, ip, sizeof(linedebounce)))
+			return -EFAULT;
+		if (linedebounce.line_offset >= gdev->ngpio)
+			return -EINVAL;
+
+		desc = &gdev->descs[linedebounce.line_offset];
+
+		return gpiod_set_debounce(desc, linedebounce.debounce_usec);
 	} else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
 		return linehandle_create(gdev, ip);
 	} else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 1bf6e6df084b..4b092990d4c8 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -53,6 +53,17 @@ struct gpioline_info {
 	char consumer[32];
 };
 
+/**
+ * struct gpioline_debounce - GPIO line debounce
+ * @line_offset: the local offset on this GPIO device, fill this in when
+ * requesting the line information from the kernel
+ * @debounce_usec: debounce in uSeconds to set for this line
+ */
+struct gpioline_debounce {
+	__u32 line_offset;
+	__u32 debounce_usec;
+};
+
 /* Maximum number of requested handles */
 #define GPIOHANDLES_MAX 64
 
@@ -154,5 +165,6 @@ struct gpioevent_data {
 #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
 #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
 #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
+#define GPIO_SET_DEBOUNCE_IOCTL _IOW(0xB4, 0x05, struct gpioline_debounce)
 
 #endif /* _UAPI_GPIO_H_ */
-- 
2.17.1

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Bartosz Golaszewski]

niedz., 19 kwi 2020 o 02:19 Hector Bujanda <hector.bujanda@digi.com> napisał(a):
>
> This allows calling gpiod_set_debounce function through char device ioctl.
>
> Signed-off-by: Hector Bujanda <hector.bujanda@digi.com>
> ---

Hi Hector,

please keep in mind to Cc me on GPIO patches - especially when
touching uAPI. For uAPI you can also Cc Kent Gibson for a second
opinion.

>  drivers/gpio/gpiolib.c    | 12 ++++++++++++
>  include/uapi/linux/gpio.h | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 70f0dedca59f..c959c2962f15 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1073,6 +1073,18 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                 if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
>                         return -EFAULT;
>                 return 0;
> +       } else if (cmd == GPIO_SET_DEBOUNCE_IOCTL) {
> +               struct gpioline_debounce linedebounce;
> +               struct gpio_desc *desc;
> +
> +               if (copy_from_user(&linedebounce, ip, sizeof(linedebounce)))
> +                       return -EFAULT;
> +               if (linedebounce.line_offset >= gdev->ngpio)
> +                       return -EINVAL;
> +
> +               desc = &gdev->descs[linedebounce.line_offset];
> +
> +               return gpiod_set_debounce(desc, linedebounce.debounce_usec);

As Linus pointed out: adding a new ioctl() for this is out of question
- especially if this new ioctl() would be called on the chip file
descriptor. Modifying any config settings can only happen on lines
previously requested too in user-space.

>         } else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
>                 return linehandle_create(gdev, ip);
>         } else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 1bf6e6df084b..4b092990d4c8 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -53,6 +53,17 @@ struct gpioline_info {
>         char consumer[32];
>  };
>
> +/**
> + * struct gpioline_debounce - GPIO line debounce
> + * @line_offset: the local offset on this GPIO device, fill this in when
> + * requesting the line information from the kernel
> + * @debounce_usec: debounce in uSeconds to set for this line
> + */
> +struct gpioline_debounce {
> +       __u32 line_offset;
> +       __u32 debounce_usec;
> +};
> +
>  /* Maximum number of requested handles */
>  #define GPIOHANDLES_MAX 64
>
> @@ -154,5 +165,6 @@ struct gpioevent_data {
>  #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
>  #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
>  #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
> +#define GPIO_SET_DEBOUNCE_IOCTL _IOW(0xB4, 0x05, struct gpioline_debounce)
>
>  #endif /* _UAPI_GPIO_H_ */
> --
> 2.17.1
>

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.

First: this information (debounce setting) isn't exported to
user-space in any way yet. While we can't extend the gpioline_info
structure because there's no padding for future use (unfortunately :(
) we should at least have a flag coming after
GPIOHANDLE_REQUEST_BIAS_DISABLE that would indicate to user-space that
the line is debounced at all e.g. GPIOHANDLE_REQUEST_DEBOUNCED.

At the same time as the above: the line state change notifier chain
must be called from gpiod_set_debounce() - in the end: if we export
this information to the user-space, we also need to notify it when it
changes.

Next: the SET_CONFIG ioctl() should be extended to work with lineevent
file descriptors too (of course - not all options would make sense
here and they'd need to be properly filtered).

Finally: we can extend the gpiohandle_config structure with a field
containing the debounce time which would be read by the kernel if the
debounce flag is set in gpiohandle_config.flags.

Does this make sense?

Bart

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Linus Walleij]

On Wed, Apr 29, 2020 at 2:06 PM Bartosz Golaszewski <brgl@bgdev.pl> 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.
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.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Bartosz Golaszewski]

śr., 29 kwi 2020 o 14:38 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Wed, Apr 29, 2020 at 2:06 PM Bartosz Golaszewski <brgl@bgdev.pl> 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

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@bgdev.pl> 
Sent: miércoles, 29 de abril de 2020 15:00
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Bujanda, Hector <Hector.Bujanda@digi.com>; open list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Kent Gibson <warthog618@gmail.com>
Subject: Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

śr., 29 kwi 2020 o 14:38 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Wed, Apr 29, 2020 at 2:06 PM Bartosz Golaszewski <brgl@bgdev.pl> 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

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Kent Gibson]

On Thu, Apr 30, 2020 at 01:32:22PM +0000, Bujanda, Hector wrote:
> 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.
> 

Just to clarify on this point, the reason the SET_CONFIG would have to
be extended to events is not to alter the debounce on the fly but to set
it at all.  Lines are requested as either handles (for outputs or polled inputs) 
or events (for asynchronous edge events on inputs). We cannot extend
either the handle or event request ioctls themselves as there is no provision 
in their data structures for future expansion.  There is in the
SET_CONFIG ioctl - but that doesn't apply to event requests yet...


> 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.
> 

I totally agree with you on the widening scope.

Bart - how do you want to go forward with this?  I'm available to work
on it, in part or full.

Cheers,
Kent.

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Bartosz Golaszewski]

czw., 30 kwi 2020 o 16:58 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Thu, Apr 30, 2020 at 01:32:22PM +0000, Bujanda, Hector wrote:
> > 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.
> >
>
> Just to clarify on this point, the reason the SET_CONFIG would have to
> be extended to events is not to alter the debounce on the fly but to set
> it at all.  Lines are requested as either handles (for outputs or polled inputs)
> or events (for asynchronous edge events on inputs). We cannot extend
> either the handle or event request ioctls themselves as there is no provision
> in their data structures for future expansion.  There is in the
> SET_CONFIG ioctl - but that doesn't apply to event requests yet...
>

Indeed. And as I was thinking about it over the weekend I realized
that exposing a setter for a config option that's not settable at
request-time seems wrong. Together with the lineevent structure which
doesn't work on 64-bit kernel with 32-bit user-space this all makes me
think we should design v2 of several of the ioctl() calls with more
care.

>
> > 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.
> >
>
> I totally agree with you on the widening scope.
>
> Bart - how do you want to go forward with this?  I'm available to work
> on it, in part or full.
>

Personally I'm super busy with my actual job and adding support for
line watch ioctl() to libgpiod ATM. I can't really spare any time on
this. I have some crazy ideas: like storing the debounce time in the
16 most significant bits of the flags field but this is just papering
over bad ABI.

Ideally we'd have to introduce new versions of gpioevent_request,
gpioline_request, gpioline_info and gpioevent_data structs - this time
with enough additional padding and no alignment issues. Then we could
add the debounce properly.

This would of course add a lot of cruft to the uAPI code. I'd start by
moving it out of drivers/gpio/gpiolib.c into a new file:
drivers/gpio/gpiolib-cdev.c. This way we'd have everything related to
the character device in one place. It would make it easier to: a) add
a config option for disabling it entirely and b) add a config option
to disable the v1 of the ioctl()s.

I also Cc'ed Andy who may have some better ideas.

Linus: about the software-debounce you mentioned: do you think it
somehow plugs the hole we identified here?

Bart

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Kent Gibson]

On Mon, May 04, 2020 at 12:31:57PM +0200, Bartosz Golaszewski wrote:
> czw., 30 kwi 2020 o 16:58 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Thu, Apr 30, 2020 at 01:32:22PM +0000, Bujanda, Hector wrote:
> > > 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.
> > >
> >
> > Just to clarify on this point, the reason the SET_CONFIG would have to
> > be extended to events is not to alter the debounce on the fly but to set
> > it at all.  Lines are requested as either handles (for outputs or polled inputs)
> > or events (for asynchronous edge events on inputs). We cannot extend
> > either the handle or event request ioctls themselves as there is no provision
> > in their data structures for future expansion.  There is in the
> > SET_CONFIG ioctl - but that doesn't apply to event requests yet...
> >
> 
> Indeed. And as I was thinking about it over the weekend I realized
> that exposing a setter for a config option that's not settable at
> request-time seems wrong. Together with the lineevent structure which
> doesn't work on 64-bit kernel with 32-bit user-space this all makes me
> think we should design v2 of several of the ioctl() calls with more
> care.
> 
> >
> > > 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.
> > >
> >
> > I totally agree with you on the widening scope.
> >
> > Bart - how do you want to go forward with this?  I'm available to work
> > on it, in part or full.
> >
> 
> Personally I'm super busy with my actual job and adding support for
> line watch ioctl() to libgpiod ATM. I can't really spare any time on
> this. I have some crazy ideas: like storing the debounce time in the
> 16 most significant bits of the flags field but this is just papering
> over bad ABI.
> 
> Ideally we'd have to introduce new versions of gpioevent_request,
> gpioline_request, gpioline_info and gpioevent_data structs - this time
> with enough additional padding and no alignment issues. Then we could
> add the debounce properly.
> 

Agreed - adding setting only via the SET_CONFIG ioctl is a bit of a
hack, and a v2 of the uAPI is required to add it to the requests, and
to add the debounce value to the info.

> This would of course add a lot of cruft to the uAPI code. I'd start by
> moving it out of drivers/gpio/gpiolib.c into a new file:
> drivers/gpio/gpiolib-cdev.c. This way we'd have everything related to
> the character device in one place. It would make it easier to: a) add
> a config option for disabling it entirely and b) add a config option
> to disable the v1 of the ioctl()s.
> 

Ok, that is widening the scope of the change again, but I'm still willing
to have a go at it.  Wrt a v2, I was considering re-organising how the
flag field works, generally using it to indicate the presence of other
fields in the request, rather than trying to encode a capability just in
the flags.  e.g. direction, drive, bias and debounce would each have a
single flag as well as a field, or fields, containing their actual value.

For requests and sets, the flags indicate the capabilities being
explicitly set.  For info it would indicate the fields populated.

This would simplify and clarify what combinations of flags actually make
sense and what is actually being set or left unchanged.

Polarity (ActiveLow) is a bit different, as it is purely a binary and is
always set, one way or the other, so it could remain just a flag.

If we need to add a new capability then we use the next bit in the flags
and some of the padding reserved for future use for the field(s).

Does that make sense?

I am also wondering if we could merge the handle and event requests by
making edge detection just another capability?  Maybe there is something
I'm missing here.  What was the rationale for them being separate?

Cheers,
Kent.

> I also Cc'ed Andy who may have some better ideas.
> 
> Linus: about the software-debounce you mentioned: do you think it
> somehow plugs the hole we identified here?
> 
> Bart

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Linus Walleij]

On Thu, May 7, 2020 at 5:40 AM Kent Gibson <warthog618@gmail.com> wrote:

> > This would of course add a lot of cruft to the uAPI code. I'd start by
> > moving it out of drivers/gpio/gpiolib.c into a new file:
> > drivers/gpio/gpiolib-cdev.c. This way we'd have everything related to
> > the character device in one place. It would make it easier to: a) add
> > a config option for disabling it entirely and b) add a config option
> > to disable the v1 of the ioctl()s.
> >
>
> Ok, that is widening the scope of the change again, but I'm still willing
> to have a go at it.

I'm very happy if you work on it because you did a great job
with gpiolib so far!

Yours,
Linus Walleij

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Linus Walleij]

On Mon, May 4, 2020 at 12:32 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Ideally we'd have to introduce new versions of gpioevent_request,
> gpioline_request, gpioline_info and gpioevent_data structs - this time
> with enough additional padding and no alignment issues. Then we could
> add the debounce properly.

Hm that sounds massive. Is it really that bad?

> This would of course add a lot of cruft to the uAPI code. I'd start by
> moving it out of drivers/gpio/gpiolib.c into a new file:
> drivers/gpio/gpiolib-cdev.c. This way we'd have everything related to
> the character device in one place. It would make it easier to: a) add
> a config option for disabling it entirely and b) add a config option
> to disable the v1 of the ioctl()s.

Its good to break out for code maintenance no matter what we do
with it :)

I would however not make it in any way totally optional, because the
big win with the character device over the legacy sysfs is to always
be available.

> Linus: about the software-debounce you mentioned: do you think it
> somehow plugs the hole we identified here?

Hm, I don't quite understand what the hole is I guess...

Linus

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Kent Gibson]

On Tue, May 12, 2020 at 07:55:42PM +0200, Linus Walleij wrote:
> On Mon, May 4, 2020 at 12:32 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 

I hope Bart doesn't mind if I jump in here, but I've started working on
this so hopefully I can address most of your points...

> > Ideally we'd have to introduce new versions of gpioevent_request,
> > gpioline_request, gpioline_info and gpioevent_data structs - this time
> > with enough additional padding and no alignment issues. Then we could
> > add the debounce properly.
> 
> Hm that sounds massive. Is it really that bad?
> 

Agreed - it is massive - we end up replacing the majority of the
existing structs and ioctls.

If we want to be able to set debounce in the request(s), not just in
SET_CONFIG, then we need new requests as there is no room in the
existing.  If we want to be able to report that config in the info then
we need new infos for the same reason.  The info_changed contains an
info so that has to change as well. And the event_data has a 32/64bit
alignment issue so it was already up for replacement.

So it could be worse, but not much.

> > This would of course add a lot of cruft to the uAPI code. I'd start by
> > moving it out of drivers/gpio/gpiolib.c into a new file:
> > drivers/gpio/gpiolib-cdev.c. This way we'd have everything related to
> > the character device in one place. It would make it easier to: a) add
> > a config option for disabling it entirely and b) add a config option
> > to disable the v1 of the ioctl()s.
> 
> Its good to break out for code maintenance no matter what we do
> with it :)
>

It definitely is, and I'll submit a patch soon, that hopefully can be
applied immediately before the next dev window opens, to do just that.

> I would however not make it in any way totally optional, because the
> big win with the character device over the legacy sysfs is to always
> be available.
> 

And if you build it into your kernel, which will be the default, it
still will be.

But maybe there are specific applications that don't need cdev and
would be interested in reducing kernel bloat?

> > Linus: about the software-debounce you mentioned: do you think it
> > somehow plugs the hole we identified here?
> 
> Hm, I don't quite understand what the hole is I guess...
> 

I'll leave this one for Bart - the more I re-read the thread the less
certain I am as well.

I will note that Bart correctly mentioned that the uapi doesn't return
an error if the user requests bias that is not supported by the driver
- gpio_set_bias absorbs the error.

That isn't by intent - it is the way gpiod_direction_input
behaved before I added bias to cdev. It was left that way as I was
unsure on the broader implications of changing it, and wasn't keen on
implementing a cdev specific gpiod_direction_input either.
I'm open to suggestions if you would like to change that.

Cheers,
Kent.

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Kent Gibson]

On Wed, Apr 29, 2020 at 02:38:13PM +0200, Linus Walleij wrote:
> On Wed, Apr 29, 2020 at 2:06 PM Bartosz Golaszewski <brgl@bgdev.pl> 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.
> 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.
> 

I'm looking at implementing the software debounce you suggest here,
using the gpio_keys example as a starting point, but find that there are
actually two different debounce strategies in gpio_keys. For gpio pins
that don't support debounce in hw, it uses mod_delayed_work to put off
reading the new value until the line has settled (i.e. no interrupts
received) for the debounce period.
For non-gpio lines it uses timers to poll the line at the debounce
period.

You mention timers for the gpio pins that cannot provide debounce, 
so I'm confused. Could you clarify which strategy you have in mind?

I've also had a quick look at the possibility of providing the software
debounce for in-kernel consumers.  Are you anticipating new API for
that?  e.g. allowing consumers to request gpio events?  Cos, gpio_keys
grabs the irq itself - and that would bypass the software debouncer,
or even conflict with it.

Thanks,
Kent.

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Linus Walleij]

On Mon, May 25, 2020 at 4:22 AM Kent Gibson <warthog618@gmail.com> wrote:

> You mention timers for the gpio pins that cannot provide debounce,
> so I'm confused. Could you clarify which strategy you have in mind?

My idea is that the callback gpiod_set_debounce() for in-kernel consumers
should more or less always return success. Either the hardware does
the debounce,  or gpiolib sets up a timer.

> I've also had a quick look at the possibility of providing the software
> debounce for in-kernel consumers.

That is where I think it should start.

>  Are you anticipating new API for
> that?  e.g. allowing consumers to request gpio events?  Cos, gpio_keys
> grabs the irq itself - and that would bypass the software debouncer,
> or even conflict with it.

It may be hard or impossible.

I suppose gpiolib would have to steal or intercept the interrupt
by using e.g. IRQF_SHARED and then just return IRQ_HANDLED
on the first IRQ so the underlying irq handler does not get called.

After the timer times out it needs to retrigger the IRQ.

So the consuming driver would se a "debounced and ready"
IRQ so when it gets this IRQ it knows for sure there is
no bounciness on it because gpiolib already took care
of that.

The above is in no way trivial, but it follows the design pattern
of "narrow and deep" APIs.

Failure is an option! Sorry if I push too complex ideas.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Kent Gibson]

On Mon, May 25, 2020 at 02:17:41PM +0200, Linus Walleij wrote:
> On Mon, May 25, 2020 at 4:22 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> > You mention timers for the gpio pins that cannot provide debounce,
> > so I'm confused. Could you clarify which strategy you have in mind?
> 
> My idea is that the callback gpiod_set_debounce() for in-kernel consumers
> should more or less always return success. Either the hardware does
> the debounce,  or gpiolib sets up a timer.
> 
> > I've also had a quick look at the possibility of providing the software
> > debounce for in-kernel consumers.
> 
> That is where I think it should start.
> 
> >  Are you anticipating new API for
> > that?  e.g. allowing consumers to request gpio events?  Cos, gpio_keys
> > grabs the irq itself - and that would bypass the software debouncer,
> > or even conflict with it.
> 
> It may be hard or impossible.
> 
> I suppose gpiolib would have to steal or intercept the interrupt
> by using e.g. IRQF_SHARED and then just return IRQ_HANDLED
> on the first IRQ so the underlying irq handler does not get called.
> 

And how would gpiolib ensure that it was first in the chain?

> After the timer times out it needs to retrigger the IRQ.
> 
> So the consuming driver would se a "debounced and ready"
> IRQ so when it gets this IRQ it knows for sure there is
> no bounciness on it because gpiolib already took care
> of that.
> 
> The above is in no way trivial, but it follows the design pattern
> of "narrow and deep" APIs.
> 

Totally agree with the concept - just trying to work out how to
implement it seemlessly given the existing API and usage, and given my
limited knowledge of the kernel internals.

> Failure is an option! Sorry if I push too complex ideas.
> 

I'm not as concerned about complexity as I am about fragility.

I don't see any problem adding debounce for gpiolib-cdev.
Adding a more complete solution to gpiolib itself is certainly
non-trivial, if it is possible at all. 

The path I'll probably be taking is adding a debouncer to gpiolib-cdev,
so at least we have a solution for userspace, then take a longer look at
the more general solution.

Cheers,
Kent.

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Linus Walleij]

On Mon, May 25, 2020 at 5:17 PM Kent Gibson <warthog618@gmail.com> wrote:

> > I suppose gpiolib would have to steal or intercept the interrupt
> > by using e.g. IRQF_SHARED and then just return IRQ_HANDLED
> > on the first IRQ so the underlying irq handler does not get called.
>
> And how would gpiolib ensure that it was first in the chain?

I don't know.

> Totally agree with the concept - just trying to work out how to
> implement it seemlessly given the existing API and usage, and given my
> limited knowledge of the kernel internals.

The irqchip maintainers certainly know the answer for the question
of shared interrupts at least.

> > Failure is an option! Sorry if I push too complex ideas.
>
> I'm not as concerned about complexity as I am about fragility.
>
> I don't see any problem adding debounce for gpiolib-cdev.
> Adding a more complete solution to gpiolib itself is certainly
> non-trivial, if it is possible at all.

I agree. It's just that I perceive it as more elegant if we can do that.

> The path I'll probably be taking is adding a debouncer to gpiolib-cdev,
> so at least we have a solution for userspace, then take a longer look at
> the more general solution.

That's fine! Thanks for looking into this.

Linus Walleij

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Andy Shevchenko]

On Wed, Apr 29, 2020 at 3:09 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> niedz., 19 kwi 2020 o 02:19 Hector Bujanda <hector.bujanda@digi.com> napisał(a):

...

> Finally: we can extend the gpiohandle_config structure with a field
> containing the debounce time which would be read by the kernel if the
> debounce flag is set in gpiohandle_config.flags.

Don't forget the 64/32 ABI.

The configuration structure has padding / endianess issues

Should be something like

{
u32 x;
u8 y;
u8 padding8[3];
u32 padding32[2*z];
}

I.O.W. each member must have padding / endianess agnostic place along
with whole structure to be multiple of 64-bit words.

-- 
With Best Regards,
Andy Shevchenko

[PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Hector Bujanda]

This allows calling gpiod_set_debounce function through char device ioctl.

Signed-off-by: Hector Bujanda <hector.bujanda@digi.com>
---
 drivers/gpio/gpiolib.c    | 12 ++++++++++++
 include/uapi/linux/gpio.h | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 70f0dedca59f..c959c2962f15 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1073,6 +1073,18 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
 		return 0;
+	} else if (cmd == GPIO_SET_DEBOUNCE_IOCTL) {
+		struct gpioline_debounce linedebounce;
+		struct gpio_desc *desc;
+
+		if (copy_from_user(&linedebounce, ip, sizeof(linedebounce)))
+			return -EFAULT;
+		if (linedebounce.line_offset >= gdev->ngpio)
+			return -EINVAL;
+
+		desc = &gdev->descs[linedebounce.line_offset];
+
+		return gpiod_set_debounce(desc, linedebounce.debounce_usec);
 	} else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
 		return linehandle_create(gdev, ip);
 	} else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 1bf6e6df084b..4b092990d4c8 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -53,6 +53,17 @@ struct gpioline_info {
 	char consumer[32];
 };
 
+/**
+ * struct gpioline_debounce - GPIO line debounce
+ * @line_offset: the local offset on this GPIO device, fill this in when
+ * requesting the line information from the kernel
+ * @debounce_usec: debounce in uSeconds to set for this line
+ */
+struct gpioline_debounce {
+	__u32 line_offset;
+	__u32 debounce_usec;
+};
+
 /* Maximum number of requested handles */
 #define GPIOHANDLES_MAX 64
 
@@ -154,5 +165,6 @@ struct gpioevent_data {
 #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
 #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
 #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
+#define GPIO_SET_DEBOUNCE_IOCTL _IOW(0xB4, 0x05, struct gpioline_debounce)
 
 #endif /* _UAPI_GPIO_H_ */
-- 
2.17.1

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Linus Walleij]

Hi Hector,

thanks for your patch!

On Sun, Apr 19, 2020 at 2:22 AM Hector Bujanda <hector.bujanda@digi.com> wrote:

> This allows calling gpiod_set_debounce function through char device ioctl.
>
> Signed-off-by: Hector Bujanda <hector.bujanda@digi.com>

(...)
> +/**
> + * struct gpioline_debounce - GPIO line debounce
> + * @line_offset: the local offset on this GPIO device, fill this in when
> + * requesting the line information from the kernel
> + * @debounce_usec: debounce in uSeconds to set for this line
> + */
> +struct gpioline_debounce {
> +       __u32 line_offset;
> +       __u32 debounce_usec;
> +};
(...)
> @@ -154,5 +165,6 @@ struct gpioevent_data {
>  #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
>  #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
>  #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
> +#define GPIO_SET_DEBOUNCE_IOCTL _IOW(0xB4, 0x05, struct gpioline_debounce)

Please do not define a new ioctl for this: since of commit
e588bb1eae31be73fbec2b731be986a7c09635a4
"gpio: add new SET_CONFIG ioctl() to gpio chardev"
by Kent Gibson we have this:

/**
 * struct gpiohandle_config - Configuration for a GPIO handle request
 * @flags: updated flags for the requested GPIO lines, such as
 * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
 * together
 * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set in flags,
 * this specifies the default output value, should be 0 (low) or
 * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
 * @padding: reserved for future use and should be zero filled
 */
struct gpiohandle_config {
        __u32 flags;
        __u8 default_values[GPIOHANDLES_MAX];
        __u32 padding[4]; /* padding for future use */
};

#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)

Setting debounce is just another type of config, so please use Kent's
work as a base for this.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Kent Gibson]

On Tue, Apr 28, 2020 at 12:35:05PM +0200, Linus Walleij wrote:
> Hi Hector,
> 
> thanks for your patch!
> 
> On Sun, Apr 19, 2020 at 2:22 AM Hector Bujanda <hector.bujanda@digi.com> wrote:
> 
> > This allows calling gpiod_set_debounce function through char device ioctl.
> >
> > Signed-off-by: Hector Bujanda <hector.bujanda@digi.com>
> 
> (...)
> > +/**
> > + * struct gpioline_debounce - GPIO line debounce
> > + * @line_offset: the local offset on this GPIO device, fill this in when
> > + * requesting the line information from the kernel
> > + * @debounce_usec: debounce in uSeconds to set for this line
> > + */
> > +struct gpioline_debounce {
> > +       __u32 line_offset;
> > +       __u32 debounce_usec;
> > +};
> (...)
> > @@ -154,5 +165,6 @@ struct gpioevent_data {
> >  #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
> >  #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
> >  #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
> > +#define GPIO_SET_DEBOUNCE_IOCTL _IOW(0xB4, 0x05, struct gpioline_debounce)
> 
> Please do not define a new ioctl for this: since of commit
> e588bb1eae31be73fbec2b731be986a7c09635a4
> "gpio: add new SET_CONFIG ioctl() to gpio chardev"
> by Kent Gibson we have this:
> 
> /**
>  * struct gpiohandle_config - Configuration for a GPIO handle request
>  * @flags: updated flags for the requested GPIO lines, such as
>  * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
>  * together
>  * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set in flags,
>  * this specifies the default output value, should be 0 (low) or
>  * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
>  * @padding: reserved for future use and should be zero filled
>  */
> struct gpiohandle_config {
>         __u32 flags;
>         __u8 default_values[GPIOHANDLES_MAX];
>         __u32 padding[4]; /* padding for future use */
> };
> 
> #define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
> 
> Setting debounce is just another type of config, so please use Kent's
> work as a base for this.
> 

A few things you might want to keep in mind:

For ABI backward compatibility, a zeroed field should be treated as "don't change".
As a 0 is also used to disable debounce you are going to need a flag
indicating whether the debounce field is present.  I don't think that can
be added to the flags field, as those values are shared with the
gpiohandle_request, but I could well be wrong on that (maybe it can just
be ignored in the request case??).

You might want to add a flag to the GPIOLINE_FLAGs to indicate if
debounce is set.

The GPIOHANDLE_SET_CONFIG_IOCTL/gpiohandle_config only applies to 
gpiohandle_requests, so it does not provide a means to set debounce for 
gpioevent_requests, and you really want debounce for those, right?
I would extend that ioctl to the gpioevent_requests, but I would run it
past Bart or LinusW first - they may have better ideas.

Cheers,
Kent.