Re: [libgpiod v2][PATCH 0/4] core: add gpiod_request_lines

[Kent Gibson <warthog618@xxxxxxxxx>]

On Sat, Apr 02, 2022 at 02:47:31PM +0200, Bartosz Golaszewski wrote:
> On Thu, Mar 31, 2022 at 3:12 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > In my review of the CXX bindings I suggested a top-level version of
> > Chip.request_lines(), and possibly a C API version as well, so here
> > is the C version, plus a couple of semi-related tweaks I made along
> > the way.
> >
> > The first patch adds the gpiod_request_lines().
> > Patch 3 migrates the appropriate tools.
> > Patch 4 minimizes the lifetimes of objects in the tools as I've
> > previously seen confusion over how long lived objects need to be.
> > Patch 2 is just a rename cos "inexistent" looks weird to me.
> > Strictly speaking it is fine, but unless there is a problem with
> > using "nonexistent" I would go with the latter.
> >
> > This series may be require my unsigned values patch.
> >
> > Cheers,
> > Kent.
> >
> > Kent Gibson (4):
> >   core: add gpiod_request_lines
> >   tools: rename inexistent to nonexistent
> >   tools: migrate to gpiod_request_lines
> >   tools: minimize object lifetimes
> >
> >  include/gpiod.h            | 15 ++++++++
> >  lib/line-request.c         | 17 +++++++++
> >  tests/tests-line-request.c | 73 ++++++++++++++++++++++++++++++++++++++
> >  tools/gpio-tools-test.bats |  4 +--
> >  tools/gpiodetect.c         |  2 +-
> >  tools/gpiofind.c           |  2 +-
> >  tools/gpioget.c            | 25 +++++++------
> >  tools/gpioinfo.c           |  4 +--
> >  tools/gpiomon.c            | 16 ++++-----
> >  tools/gpioset.c            | 18 +++++-----
> >  tools/tools-common.c       | 50 ++++++++++----------------
> >  tools/tools-common.h       |  5 +--
> >  12 files changed, 164 insertions(+), 67 deletions(-)
> >
> > --
> > 2.35.1
> >
> 
> Ugh, I didn't respond under the C++ review in time before you spent
> time on this. :/
> 

Not a problem - it was just something to fill in time.

> I don't agree with this change. For C API I think the intention for v2
> was to avoid having all kinds of high-level helpers and limit the
> number of functions to only those that are necessary to fully leverage
> the kernel uAPI and this one isn't necessary. I think we discussed it
> multiple times and agreed that the C library needs to be minimal this
> time.
> 
> For C++ and Python the issue is irrelevant because you can do:
> 
> auto request = gpiod::Chip("/dev/gpiochip0").request_lines(req_cfg, line_cfg);
> 
> or
> 
> request = gpiod.Chip("/dev/gpiochip0").request_lines(req_cfg, line_cfg)
> 

The point isn't being able to do a one liner, the point is to remove the
chip object from the set of concerns for the basic use case, and so
provide an easier on-ramp for the casual user.

> respectively and achieve the same result while still using a one-liner.
> 
> Unless there's a *really* good reason to do this, it's a NAK from my side.
> 

That's fine.

Cheers,
Kent.