Message ID | 20200419002220.105338-1-hector.bujanda@digi.com |
---|---|
State | New |
Headers | show |
Series | gpiolib: add GPIO_SET_DEBOUNCE_IOCTL | expand |
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
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.
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_ */
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(+)