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@xxxxxxxx> wrote:
> 
> > This allows calling gpiod_set_debounce function through char device ioctl.
> >
> > Signed-off-by: Hector Bujanda <hector.bujanda@xxxxxxxx>
> 
> (...)
> > +/**
> > + * 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.