| Message ID | 20220331011141.53489-1-warthog618@gmail.com |
|---|---|
| Headers | show |
| Series | core: add gpiod_request_lines | expand |
On Thu, Mar 31, 2022 at 3:12 AM Kent Gibson <warthog618@gmail.com> 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. :/ 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) 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. Bart
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@gmail.com> 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.