Re: [PATCH v9 12/20] gpiolib: cdev: support setting debounce

From: Andy Shevchenko
Date: Wed Sep 23 2020 - 12:27:57 EST

Next message: Mike Rapoport: "[PATCH] mm: remove unused early_pfn_valid()"
Previous message: 周琰杰 (Zhou Yanjie): "[PATCH v6 2/2] PHY: Ingenic: Add USB PHY driver using generic PHY framework."
In reply to: Kent Gibson: "[PATCH v9 12/20] gpiolib: cdev: support setting debounce"
Next in thread: Kent Gibson: "[PATCH v9 13/20] gpio: uapi: document uAPI v1 as deprecated"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Sep 22, 2020 at 5:36 AM Kent Gibson <warthog618@xxxxxxxxx> 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.
>
> 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.

> +static unsigned int debounced_value(struct line *line)
> +{
> + unsigned 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(line->level);
> +

> + if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
> + value = !value;

I'm not sure what this means in terms of unsingned int to be returned.

> + return value;

Shouldn't we rather return 0/1 guaranteed?

Perhaps

if (active_low)
return !value;

return !!value;

?

> +}
> +
> +static irqreturn_t debounce_irq_handler(int irq, void *p)
> +{
> + struct line *line = p;
> +
> + mod_delayed_work(system_wq, &line->work,
> + usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void debounce_work_func(struct work_struct *work)
> +{
> + struct gpio_v2_line_event le;
> + struct line *line = container_of(work, struct line, work.work);
> + struct linereq *lr;
> + int level;
> +
> + level = gpiod_get_raw_value_cansleep(line->desc);
> + if (level < 0) {
> + pr_debug_ratelimited("debouncer failed to read line value\n");
> + return;
> + }
> +
> + if (READ_ONCE(line->level) == level)
> + return;
> +
> + WRITE_ONCE(line->level, level);
> +
> + /* -- edge detection -- */
> + if (!line->eflags)
> + return;

> + /* switch from physical level to logical - if they differ */
> + if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
> + level = !level;

Seems to me a good candidate to have

static inline bool convert_with_active_low_respected(desc, value)
{
if (active_low)
return !value;
return !!value;
}

> + /* ignore edges that are not being monitored */
> + if (((line->eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) && !level) ||
> + ((line->eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) && level))
> + return;
> +
> + /* Do not leak kernel stack to userspace */
> + memset(&le, 0, sizeof(le));
> +
> + lr = line->req;
> + le.timestamp_ns = ktime_get_ns();
> + le.offset = gpio_chip_hwgpio(line->desc);
> + line->line_seqno++;
> + le.line_seqno = line->line_seqno;
> + le.seqno = (lr->num_lines == 1) ?
> + le.line_seqno : atomic_inc_return(&lr->seqno);
> +
> + if (level)
> + /* Emit low-to-high event */
> + le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
> + else
> + /* Emit high-to-low event */
> + le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
> +
> + linereq_put_event(lr, &le);
> +}
> +
> +static int debounce_setup(struct line *line,
> + unsigned int debounce_period_us)
> +{
> + unsigned long irqflags;
> + int ret, level, irq;
> +
> + /* try hardware */
> + ret = gpiod_set_debounce(line->desc, debounce_period_us);
> + if (!ret) {
> + WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> + return ret;
> + }
> + if (ret != -ENOTSUPP)
> + return ret;
> +
> + if (debounce_period_us) {
> + /* setup software debounce */
> + level = gpiod_get_raw_value_cansleep(line->desc);
> + if (level < 0)
> + return level;
> +
> + irq = gpiod_to_irq(line->desc);
> + if (irq <= 0)

Same question about return code...

> + return -ENODEV;
> +
> + WRITE_ONCE(line->level, level);
> + irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
> + ret = request_irq(irq, debounce_irq_handler, irqflags,
> + line->req->label, line);
> + if (ret)
> + return ret;
> +
> + WRITE_ONCE(line->sw_debounced, 1);
> + line->irq = irq;
> + }
> + return 0;
> +}
> +
> +static bool gpio_v2_line_config_debounced(struct gpio_v2_line_config *lc,
> + unsigned int line_idx)
> +{
> + unsigned int i;
> + u64 mask = BIT_ULL(line_idx);
> +
> + for (i = 0; i < lc->num_attrs; i++) {
> + if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_DEBOUNCE) &&
> + (lc->attrs[i].mask & mask))
> + return true;
> + }
> + return false;
> +}
> +
> +static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
> + unsigned int line_idx)
> +{
> + unsigned int i;
> + u64 mask = BIT_ULL(line_idx);
> +
> + for (i = 0; i < lc->num_attrs; i++) {
> + if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_DEBOUNCE) &&
> + (lc->attrs[i].mask & mask))
> + return lc->attrs[i].attr.debounce_period_us;
> + }
> + return 0;
> +}
> +
> static void edge_detector_stop(struct line *line)
> {
> if (line->irq) {
> @@ -578,12 +752,18 @@ static void edge_detector_stop(struct line *line)
> line->irq = 0;
> }
>
> + cancel_delayed_work_sync(&line->work);
> + WRITE_ONCE(line->sw_debounced, 0);
> line->eflags = 0;
> + /* do not change line->level - see comment in debounced_value */
> }
>
> static int edge_detector_setup(struct line *line,
> + struct gpio_v2_line_config *lc,
> + unsigned int line_idx,
> u64 eflags)
> {
> + u32 debounce_period_us;
> unsigned long irqflags = 0;
> int irq, ret;
>
> @@ -594,8 +774,16 @@ static int edge_detector_setup(struct line *line,
> return ret;
> }
> line->eflags = eflags;
> + if (gpio_v2_line_config_debounced(lc, line_idx)) {
> + debounce_period_us = gpio_v2_line_config_debounce_period(lc, line_idx);
> + ret = debounce_setup(line, debounce_period_us);
> + if (ret)
> + return ret;
> + WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> + }
>
> - if (!eflags)
> + /* detection disabled or sw debouncer will provide edge detection */
> + if (!eflags || READ_ONCE(line->sw_debounced))
> return 0;
>
> irq = gpiod_to_irq(line->desc);
> @@ -620,15 +808,31 @@ static int edge_detector_setup(struct line *line,
> return 0;
> }
>
> -static int edge_detector_update(struct line *line, u64 eflags,
> - bool polarity_change)
> +static int edge_detector_update(struct line *line,
> + struct gpio_v2_line_config *lc,
> + unsigned int line_idx,
> + u64 eflags, bool polarity_change)
> {
> - if ((line->eflags == eflags) && !polarity_change)
> + unsigned int debounce_period_us =
> + gpio_v2_line_config_debounce_period(lc, line_idx);
> +
> + if ((line->eflags == eflags) && !polarity_change &&
> + (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
> return 0;
>
> - edge_detector_stop(line);
> + /* sw debounced and still will be...*/

> + if ((debounce_period_us != 0) && READ_ONCE(line->sw_debounced)) {

'( != 0)' are redundant. But I think you want to show that it's not
boolean and we compare to 0...

> + line->eflags = eflags;
> + WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> + return 0;
> + }
> +
> + /* reconfiguring edge detection or sw debounce being disabled */
> + if ((line->irq && !READ_ONCE(line->sw_debounced)) ||
> + (!debounce_period_us && READ_ONCE(line->sw_debounced)))
> + edge_detector_stop(line);
>
> - return edge_detector_setup(line, eflags);
> + return edge_detector_setup(line, lc, line_idx, eflags);
> }
>
> static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
> @@ -726,6 +930,11 @@ static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
> ret = gpio_v2_line_flags_validate(flags);
> if (ret)
> return ret;
> +
> + /* debounce requires explicit input */
> + if (gpio_v2_line_config_debounced(lc, i) &&
> + !(flags & GPIO_V2_LINE_FLAG_INPUT))
> + return -EINVAL;
> }
> return 0;
> }
> @@ -762,7 +971,7 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
> struct gpio_v2_line_values lv;
> DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
> struct gpio_desc **descs;
> - unsigned int i, didx, num_get;
> + unsigned int i, val, didx, num_get;
> int ret;
>
> /* NOTE: It's ok to read values of output lines. */
> @@ -801,7 +1010,11 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
> lv.bits = 0;
> for (didx = 0, i = 0; i < lr->num_lines; i++) {
> if (lv.mask & BIT_ULL(i)) {
> - if (test_bit(didx, vals))
> + if (lr->lines[i].sw_debounced)
> + val = debounced_value(&lr->lines[i]);
> + else
> + val = test_bit(didx, vals);
> + if (val)
> lv.bits |= BIT_ULL(i);
> didx++;
> }
> @@ -905,7 +1118,7 @@ static long linereq_set_config_unlocked(struct linereq *lr,
> if (ret)
> return ret;
>
> - ret = edge_detector_update(&lr->lines[i],
> + ret = edge_detector_update(&lr->lines[i], lc, i,
> flags & GPIO_V2_LINE_EDGE_FLAGS,
> polarity_change);
> if (ret)
> @@ -1099,8 +1312,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
> lr->gdev = gdev;
> get_device(&gdev->dev);
>
> - for (i = 0; i < ulr.num_lines; i++)
> + for (i = 0; i < ulr.num_lines; i++) {
> lr->lines[i].req = lr;
> + WRITE_ONCE(lr->lines[i].sw_debounced, 0);
> + INIT_DELAYED_WORK(&lr->lines[i].work, debounce_work_func);
> + }
>
> /* Make sure this is terminated */
> ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
> @@ -1160,7 +1376,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
> if (ret)
> goto out_free_linereq;
>
> - ret = edge_detector_setup(&lr->lines[i],
> + ret = edge_detector_setup(&lr->lines[i], lc, i,
> flags & GPIO_V2_LINE_EDGE_FLAGS);
> if (ret)
> goto out_free_linereq;
> @@ -1631,6 +1847,8 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> struct gpio_chip *gc = desc->gdev->chip;
> bool ok_for_pinctrl;
> unsigned long flags;
> + u32 debounce_period_us;
> + unsigned int num_attrs = 0;
>
> memset(info, 0, sizeof(*info));
> info->offset = gpio_chip_hwgpio(desc);
> @@ -1691,6 +1909,14 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
> info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING;
>
> + debounce_period_us = READ_ONCE(desc->debounce_period_us);
> + if (debounce_period_us) {
> + info->attrs[num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> + info->attrs[num_attrs].debounce_period_us = debounce_period_us;
> + num_attrs++;
> + }
> + info->num_attrs = num_attrs;
> +
> spin_unlock_irqrestore(&gpio_lock, flags);
> }
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index aa20481e9452..3cdf9effc13a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2097,6 +2097,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
> clear_bit(FLAG_IS_HOGGED, &desc->flags);
> #ifdef CONFIG_OF_DYNAMIC
> desc->hog = NULL;
> +#endif
> +#ifdef CONFIG_GPIO_CDEV
> + WRITE_ONCE(desc->de> --
> 2.28.0
>
bounce_period_us, 0);
> #endif
> ret = true;
> }
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 39b356160937..b674b5bb980e 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -124,6 +124,10 @@ struct gpio_desc {
> #ifdef CONFIG_OF_DYNAMIC
> struct device_node *hog;
> #endif
> +#ifdef CONFIG_GPIO_CDEV
> + /* debounce period in microseconds */
> + unsigned int debounce_period_us;
> +#endif
> };
>
> int gpiod_request(struct gpio_desc *desc, const char *label);

--
With Best Regards,
Andy Shevchenko

Next message: Mike Rapoport: "[PATCH] mm: remove unused early_pfn_valid()"
Previous message: 周琰杰 (Zhou Yanjie): "[PATCH v6 2/2] PHY: Ingenic: Add USB PHY driver using generic PHY framework."
In reply to: Kent Gibson: "[PATCH v9 12/20] gpiolib: cdev: support setting debounce"
Next in thread: Kent Gibson: "[PATCH v9 13/20] gpio: uapi: document uAPI v1 as deprecated"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]