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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, Sep 23, 2020 at 07:27:37PM +0300, Andy Shevchenko wrote:
> 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;
> 
> ?
> 

Or just make the return value a bool?

[snip]
> > +
> > +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;
> }
> 

Not sure it is worth the effort - it would only be used twice - here
and in debounced_value() - which is only a couple of lines itself.

[snip]
> > +
> > +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...
> 

Same answer...

[snip]

> >                 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...
> 

Yeah, I guess I thought that was clearer, though I use the bare form
just below as well, and the bare form seems clear enough to me now, so
will change it for v10.

Cheers,
Kent.

> > +               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);
> >