lkml.org 
[lkml]   [2020]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v9 12/20] gpiolib: cdev: support setting debounce
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@gmail.com> 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);
> >

\
 
 \ /
  Last update: 2020-09-24 09:49    [W:0.084 / U:7.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site