Re: [PATCH v4 12/20] gpiolib: cdev: support setting debounce
On Mon, Aug 17, 2020 at 08:21:58PM +0200, Bartosz Golaszewski wrote:
> On Fri, Aug 14, 2020 at 5:05 AM Kent Gibson wrote:
> >
> > Add support for setting debounce on a line via the GPIO uAPI.
> > Where debounce is not supported by hardware, a software debounce is
> > provided.
> >
> > Signed-off-by: Kent Gibson
> > ---
[snip]
> > + debounce_period = READ_ONCE(desc->debounce_period);
> > + if (debounce_period) {
> > + info->attrs[num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> > + info->attrs[num_attrs].debounce_period = debounce_period;
> > + num_attrs++;
> > + }
> > + info->num_attrs = num_attrs;
>
> AFAICT this (reading it in gpio_desc_to_lineinfo) is the only reason
> to store the debounce period in struct gpio_desc. I'm wondering if we
> can avoid extending this struct only for such uncommon case and store
> it elsewhere. In all other cases where you read or write to it - you
> have access to the underlying edge detector. Would the single-line
> struct line I suggested elsewhere be a good place? On the other hand
> I'm not sure how to get it having only the desc. I need to think about
> it more.
>
Yeah, it is stored there so it can be returned by lineinfo_get() for the
GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL.
And the same applies to any future config fields.
I would also like to not pollute the desc, or anything else in gpiolib,
but wasn't sure where else to put it.
I'm open to suggestions.
Cheers,
Kent.
Re: [PATCH v4 12/20] gpiolib: cdev: support setting debounce
On Fri, Aug 14, 2020 at 5:05 AM Kent Gibson wrote:
>
> Add support for setting debounce on a line via the GPIO uAPI.
> Where debounce is not supported by hardware, a software debounce is
> provided.
>
> Signed-off-by: Kent Gibson
> ---
>
> The implementation of the software debouncer waits for the line to be
> stable for the debounce period before determining if a level change,
> and a corresponding edge event, has occurred. This provides maximum
> protection against glitches, but also introduces a debounce_period
> latency to edge events.
>
> The software debouncer is integrated with the edge detection as it
> utilises the line interrupt, and integration is simpler than getting
> the two to interwork. Where software debounce AND edge detection is
> required, the debouncer provides both.
>
> Due to the tight integration between the debouncer and edge detection,
> and to avoid particular corner cases, it is not allowed to alter the
> debounce value if edge detection is enabled. Changing the debounce with
> edge detection enabled is a very unlikely use case, so it is preferable
> to disallow it rather than complicate the code to allow it.
> Should the user wish to alter the debounce value in such cases they will
> need to release and re-request the line.
>
> Changes for v4:
> - fix handling of mask in line_get_values
>
> Changes for v3:
> - only GPIO_V2 field renaming
>
> Changes for v2:
> - improve documentation on fields shared by threads.
> - use READ_ONCE/WRITE_ONCE for shared fields rather than atomic_t
>which was overkill.
>
> drivers/gpio/gpiolib-cdev.c | 265 +++-
> drivers/gpio/gpiolib.h | 4 +
> 2 files changed, 263 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index de88b7a5ba0f..77fabf815de8 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -22,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "gpiolib.h"
> @@ -395,6 +397,9 @@ static int linehandle_create(struct gpio_device *gdev,
> void __user *ip)
> * for the corresponding line request. Ths is drawn from the @line.
> * @line_seqno: the seqno for the current edge event in the sequence of
> * events for this line.
> + * @work: the worker that implements software debouncing
> + * @sw_debounced: flag indicating if the software debouncer is active
> + * @level: the current debounced physical level of the line
> */
> struct edge_detector {
> struct line *line;
> @@ -406,7 +411,27 @@ struct edge_detector {
> */
> u64 timestamp;
> u32 seqno;
> + /*
> +* line_seqno is used by either edge_irq_thread() or
> +* debounce_work_func() which are themselves mutually exclusive.
> +*/
> u32 line_seqno;
> + /*
> +* -- debouncer specific fields --
> +*/
> + struct delayed_work work;
> + /*
> +* sw_debounce is shared by line_set_config(), which is the only
> +* setter, and line_ioctl(), which can live with a slightly stale
> +* value.
> +*/
> + unsigned int sw_debounced;
> + /*
> +* level is shared by debounce_work_func(), which is the only
> +* setter, and line_ioctl() which can live with a slightly stale
> +* value.
> +*/
> + unsigned int level;
> };
>
> /**
> @@ -523,6 +548,10 @@ static int edge_detector_start(struct edge_detector
> *edet)
> int ret, irq, irqflags = 0;
> struct gpio_desc *desc;
>
> + if (READ_ONCE(edet->sw_debounced))
> + /* debouncer is setup and will provide edge detection */
> + return 0;
> +
> desc = edge_detector_desc(edet);
> irq = gpiod_to_irq(desc);
>
> @@ -554,17 +583,215 @@ static int edge_detector_start(struct edge_detector
> *edet)
> return 0;
> }
>
> +/*
> + * returns the current debounced logical value.
> + */
> +static int debounced_value(struct edge_detector *edet)
> +{
> + int value;
> +
> + /*
> +* minor race - debouncer may be stopped here, so edge_detector_stop
> +* must leave the value unchanged so the following will read the level
> +* from when the debouncer was last running.
> +*/
> + value = READ_ONCE(edet->level);
> +
> + if (test_bit(FLAG_ACTIVE_LOW, _detector_desc(edet)->flags))
> + value = !value;
> +
> + return value;
> +}
> +
> +static irqreturn_t debounce_irq_handler(int irq, void *p)
> +{
> + struct edge_detector *edet = p;
> + struct gpio_desc *desc = edge_detector_desc(edet);
> +
> + mod_delayed_work(system_wq,
> +>work,
> +usecs_to_jiffies(READ_ONCE(desc->debounce_period)));
> +
> + return IRQ_HANDLED;
[PATCH v4 12/20] gpiolib: cdev: support setting debounce
Add support for setting debounce on a line via the GPIO uAPI.
Where debounce is not supported by hardware, a software debounce is
provided.
Signed-off-by: Kent Gibson
---
The implementation of the software debouncer waits for the line to be
stable for the debounce period before determining if a level change,
and a corresponding edge event, has occurred. This provides maximum
protection against glitches, but also introduces a debounce_period
latency to edge events.
The software debouncer is integrated with the edge detection as it
utilises the line interrupt, and integration is simpler than getting
the two to interwork. Where software debounce AND edge detection is
required, the debouncer provides both.
Due to the tight integration between the debouncer and edge detection,
and to avoid particular corner cases, it is not allowed to alter the
debounce value if edge detection is enabled. Changing the debounce with
edge detection enabled is a very unlikely use case, so it is preferable
to disallow it rather than complicate the code to allow it.
Should the user wish to alter the debounce value in such cases they will
need to release and re-request the line.
Changes for v4:
- fix handling of mask in line_get_values
Changes for v3:
- only GPIO_V2 field renaming
Changes for v2:
- improve documentation on fields shared by threads.
- use READ_ONCE/WRITE_ONCE for shared fields rather than atomic_t
which was overkill.
drivers/gpio/gpiolib-cdev.c | 265 +++-
drivers/gpio/gpiolib.h | 4 +
2 files changed, 263 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index de88b7a5ba0f..77fabf815de8 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -22,6 +23,7 @@
#include
#include
#include
+#include
#include
#include "gpiolib.h"
@@ -395,6 +397,9 @@ static int linehandle_create(struct gpio_device *gdev, void
__user *ip)
* for the corresponding line request. Ths is drawn from the @line.
* @line_seqno: the seqno for the current edge event in the sequence of
* events for this line.
+ * @work: the worker that implements software debouncing
+ * @sw_debounced: flag indicating if the software debouncer is active
+ * @level: the current debounced physical level of the line
*/
struct edge_detector {
struct line *line;
@@ -406,7 +411,27 @@ struct edge_detector {
*/
u64 timestamp;
u32 seqno;
+ /*
+* line_seqno is used by either edge_irq_thread() or
+* debounce_work_func() which are themselves mutually exclusive.
+*/
u32 line_seqno;
+ /*
+* -- debouncer specific fields --
+*/
+ struct delayed_work work;
+ /*
+* sw_debounce is shared by line_set_config(), which is the only
+* setter, and line_ioctl(), which can live with a slightly stale
+* value.
+*/
+ unsigned int sw_debounced;
+ /*
+* level is shared by debounce_work_func(), which is the only
+* setter, and line_ioctl() which can live with a slightly stale
+* value.
+*/
+ unsigned int level;
};
/**
@@ -523,6 +548,10 @@ static int edge_detector_start(struct edge_detector *edet)
int ret, irq, irqflags = 0;
struct gpio_desc *desc;
+ if (READ_ONCE(edet->sw_debounced))
+ /* debouncer is setup and will provide edge detection */
+ return 0;
+
desc = edge_detector_desc(edet);
irq = gpiod_to_irq(desc);
@@ -554,17 +583,215 @@ static int edge_detector_start(struct edge_detector
*edet)
return 0;
}
+/*
+ * returns the current debounced logical value.
+ */
+static int debounced_value(struct edge_detector *edet)
+{
+ int value;
+
+ /*
+* minor race - debouncer may be stopped here, so edge_detector_stop
+* must leave the value unchanged so the following will read the level
+* from when the debouncer was last running.
+*/
+ value = READ_ONCE(edet->level);
+
+ if (test_bit(FLAG_ACTIVE_LOW, _detector_desc(edet)->flags))
+ value = !value;
+
+ return value;
+}
+
+static irqreturn_t debounce_irq_handler(int irq, void *p)
+{
+ struct edge_detector *edet = p;
+ struct gpio_desc *desc = edge_detector_desc(edet);
+
+ mod_delayed_work(system_wq,
+>work,
+usecs_to_jiffies(READ_ONCE(desc->debounce_period)));
+
+ return IRQ_HANDLED;
+}
+
+static void debounce_work_func(struct work_struct *work)
+{
+ struct gpio_v2_line_event le;
+ int ret, level;
+ struct edge_detector *edet =
+ container_of(work, struct edge_detector, work.work);
+ struct gpio_desc *desc = edge_detector_desc(edet);
+ struct line *line;
+
+
