Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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

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