Re: [libgpiod][PATCH] core: Basic port to uAPI v2

[Kent Gibson <warthog618@gmail.com>]

On Fri, Oct 02, 2020 at 04:48:02PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 2, 2020 at 9:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Port existing implementation from GPIO uAPI v1 to v2.
> > The libgpiod external interface remains unchanged, only the internal
> > implementation switches from uAPI v1 to v2.
> 
> Cool!
> 

Not terribly - I needed a libgpiod-based testbed for my kernel
changes, and this was the easiest way to do it.
By happy coincidence that aligns with the path Bart wants to take for
libgpiod 2.0, so here we are.

> > This is a minimal port - uAPI v2 features are only used where it
> > simplifies the implementation, specifically multiple events on a bulk can
> > now be handled directly by the kernel in a single v2 line request rather
> > than being emulated by multiple v1 event requests.
> 
> ...
> 
> > +       if (config->flags & GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE)
> > +               bias_flags++;
> > +       if (config->flags & GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP)
> > +               bias_flags++;
> > +       if (config->flags & GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN)
> > +               bias_flags++;
> > +       if (bias_flags > 1)
> > +               return false;
> 
> Seems to me like an emulation of hweight(), although I don't know if
> we have in standard libraries such.
> 

Yup, same here, and it was quicker at the time to just write this than go
looking...


> ...
> 
> > -       if (values) {
> > -               for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
> > -                       data.values[i] = (uint8_t)!!values[i];
> > +       for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++) {
> > +               lines_bitmap_set_bit(&lv.mask, i);
> > +               lines_bitmap_assign_bit(&lv.bits, i, values && values[i]);
> >         }
> 
> Hmm... What about
>        for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
>                lines_bitmap_set_bit(&lv.mask, i);
> 
>   if (values) {
>        for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
>                lines_bitmap_assign_bit(&lv.bits, i, values[i]);
>   }
> 
> ?
> 

I find mine easier to read.
Yours may be slightly faster though.  And maybe larger?

> ...
> 
> >         /*
> >          * 16 is the maximum number of events the kernel can store in the FIFO
> >          * so we can allocate the buffer on the stack.
> > +        *
> > +        * NOTE: This is no longer strictly true for uAPI v2.  While 16 is
> > +        * the default for single line, a request with multiple lines will
> 
> for a single
> 
> > +        * have a larger buffer.  So need to rethink the allocation here,
> 
> So we (I, ...?) need
> 

The purpose of this patch is to move what work I'd already done on
libgpiod in my tree, in order to test the kernel changes, into Bart's
libgpiod tree, where it can be used as the basis of 2.0 development.

So that note is a reminder for Bart, or whoever gets there first...
But leaving it vague is still grammatically correct.

Cheers,
Kent.