This allows calling gpiod_set_debounce function through char device ioctl.
Signed-off-by: Hector Bujanda <[email protected]>
---
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
Hi Hector,
thanks for your patch!
On Sun, Apr 19, 2020 at 2:22 AM Hector Bujanda <[email protected]> wrote:
> This allows calling gpiod_set_debounce function through char device ioctl.
>
> Signed-off-by: Hector Bujanda <[email protected]>
(...)
> +/**
> + * 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 <[email protected]> wrote:
>
> > This allows calling gpiod_set_debounce function through char device ioctl.
> >
> > Signed-off-by: Hector Bujanda <[email protected]>
>
> (...)
> > +/**
> > + * 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.
On Mon, May 25, 2020 at 02:17:41PM +0200, Linus Walleij wrote:
> On Mon, May 25, 2020 at 4:22 AM Kent Gibson <[email protected]> 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.
On Mon, May 25, 2020 at 5:17 PM Kent Gibson <[email protected]> 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