| Message ID | 20210115103018.27704-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | treewide: remove more cruft and add some improvements | expand |
On Fri, Jan 15, 2021 at 11:30:14AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > When inspecting the current bias setting of a GPIO line, the AS_IS name > of one of the possible values really means that the kernel uAPI can't > determine the bias setting because it didn't set it itself. In this case > it's better to change the name to BIAS_UNKNOWN to reflect that. > Your checkin comment incorporates some of my review comments, which were actually a bit sloppy. While I didn't bother to correct myself for that email, I'd rather the checkin comment be more precise. Specifically, I was conflating gpiolib and the cdev uAPI. If the bias is set via gpiolib then the uAPI will report it correctly. If it is set otherwise then the setting is unknown to gpiolib and therefore the uAPI. And I'm not sure if the DT example that I used in that email was a good one. But say the hardware initialises with pull-up enabled. If it hasn't also been set via gpiolib then it will be reported as unknown. Cheers, Kent.
On Fri, Jan 15, 2021 at 11:30:18AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > In order to avoid any problems with symbols missing from the host linux > kernel headers (for example: if current version of libgpiod supports > features that were added recently to the kernel but the host headers are > outdated and don't export required symbols) let's add the uapi header to > the repository and include it instead of the one in /usr/include/linux. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > configure.ac | 12 +- > lib/Makefile.am | 2 +- > lib/core.c | 3 +- > lib/uapi/gpio.h | 522 ++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 528 insertions(+), 11 deletions(-) > create mode 100644 lib/uapi/gpio.h > [snip] > +enum gpio_v2_line_flag { > + GPIO_V2_LINE_FLAG_USED = _BITULL(0), > + GPIO_V2_LINE_FLAG_ACTIVE_LOW = _BITULL(1), > + GPIO_V2_LINE_FLAG_INPUT = _BITULL(2), > + GPIO_V2_LINE_FLAG_OUTPUT = _BITULL(3), > + GPIO_V2_LINE_FLAG_EDGE_RISING = _BITULL(4), > + GPIO_V2_LINE_FLAG_EDGE_FALLING = _BITULL(5), > + GPIO_V2_LINE_FLAG_OPEN_DRAIN = _BITULL(6), > + GPIO_V2_LINE_FLAG_OPEN_SOURCE = _BITULL(7), > + GPIO_V2_LINE_FLAG_BIAS_PULL_UP = _BITULL(8), > + GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = _BITULL(9), > + GPIO_V2_LINE_FLAG_BIAS_DISABLED = _BITULL(10), > +}; Perhaps include the header from v5.11-rc3 that includes the GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME flag? Unless your intent is for the next libgpiod release to explicitly target v5.10? +1 for including the header, btw. Targetting a specific kernel header simplifies the build. We are always going to have to deal with current apps being run on old kernels, either way, and anyone that cares to use current features has to ensure they have a current kernel, either way. Cheers, Kent.
On Fri, Jan 15, 2021 at 11:30:11AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > This is another batch of improvements to libgpiod before we overhaul the > data structure model. > > The last patch adds the kernel uapi header to the repository so that we > no longer depend on its presence on the host system. > +1 for this set, other than my specific comments. I'm all for a small core C library. And there are more drastic changes to come, so any problems missed here can be picked up there. Cheers, Kent.
On Mon, Jan 18, 2021 at 1:22 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Jan 15, 2021 at 11:30:18AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > In order to avoid any problems with symbols missing from the host linux > > kernel headers (for example: if current version of libgpiod supports > > features that were added recently to the kernel but the host headers are > > outdated and don't export required symbols) let's add the uapi header to > > the repository and include it instead of the one in /usr/include/linux. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > configure.ac | 12 +- > > lib/Makefile.am | 2 +- > > lib/core.c | 3 +- > > lib/uapi/gpio.h | 522 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 528 insertions(+), 11 deletions(-) > > create mode 100644 lib/uapi/gpio.h > > > > [snip] > > +enum gpio_v2_line_flag { > > + GPIO_V2_LINE_FLAG_USED = _BITULL(0), > > + GPIO_V2_LINE_FLAG_ACTIVE_LOW = _BITULL(1), > > + GPIO_V2_LINE_FLAG_INPUT = _BITULL(2), > > + GPIO_V2_LINE_FLAG_OUTPUT = _BITULL(3), > > + GPIO_V2_LINE_FLAG_EDGE_RISING = _BITULL(4), > > + GPIO_V2_LINE_FLAG_EDGE_FALLING = _BITULL(5), > > + GPIO_V2_LINE_FLAG_OPEN_DRAIN = _BITULL(6), > > + GPIO_V2_LINE_FLAG_OPEN_SOURCE = _BITULL(7), > > + GPIO_V2_LINE_FLAG_BIAS_PULL_UP = _BITULL(8), > > + GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = _BITULL(9), > > + GPIO_V2_LINE_FLAG_BIAS_DISABLED = _BITULL(10), > > +}; > > Perhaps include the header from v5.11-rc3 that includes the > GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME flag? > > Unless your intent is for the next libgpiod release to explicitly target > v5.10? > Not at all! I'll just update the header once a new linux version is out. I doubt v2.0 will be done anytime soon and I don't want to rush it. Bart > +1 for including the header, btw. Targetting a specific kernel header > simplifies the build. We are always going to have to deal with current > apps being run on old kernels, either way, and anyone that cares to use > current features has to ensure they have a current kernel, either way. > > Cheers, > Kent.
On Mon, Jan 18, 2021 at 1:07 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Jan 15, 2021 at 11:30:14AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > When inspecting the current bias setting of a GPIO line, the AS_IS name > > of one of the possible values really means that the kernel uAPI can't > > determine the bias setting because it didn't set it itself. In this case > > it's better to change the name to BIAS_UNKNOWN to reflect that. > > > > Your checkin comment incorporates some of my review comments, which were > actually a bit sloppy. While I didn't bother to correct myself for that > email, I'd rather the checkin comment be more precise. > > Specifically, I was conflating gpiolib and the cdev uAPI. If the bias > is set via gpiolib then the uAPI will report it correctly. If it is set > otherwise then the setting is unknown to gpiolib and therefore the uAPI. > > And I'm not sure if the DT example that I used in that email was a good > one. But say the hardware initialises with pull-up enabled. If it hasn't > also been set via gpiolib then it will be reported as unknown. > Which makes me wonder whether we shouldn't add a get_config() callback to drivers in gpiolib for that because some controllers allow you to query their current settings. --- When inspecting the current bias setting of a GPIO line, the AS_IS name of one of the possible values really means that the kernel GPIO subsystem can't determine the bias setting because it didn't set it itself (e.g. the hardware may have internally initialized pull-up or pull-down resistors). In this case it's better to change the name to BIAS_UNKNOWN to reflect that. --- Does this sound good? Bartosz
On Mon, Jan 18, 2021 at 12:40:11PM +0100, Bartosz Golaszewski wrote: > On Mon, Jan 18, 2021 at 1:07 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Fri, Jan 15, 2021 at 11:30:14AM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > When inspecting the current bias setting of a GPIO line, the AS_IS name > > > of one of the possible values really means that the kernel uAPI can't > > > determine the bias setting because it didn't set it itself. In this case > > > it's better to change the name to BIAS_UNKNOWN to reflect that. > > > > > > > Your checkin comment incorporates some of my review comments, which were > > actually a bit sloppy. While I didn't bother to correct myself for that > > email, I'd rather the checkin comment be more precise. > > > > Specifically, I was conflating gpiolib and the cdev uAPI. If the bias > > is set via gpiolib then the uAPI will report it correctly. If it is set > > otherwise then the setting is unknown to gpiolib and therefore the uAPI. > > > > And I'm not sure if the DT example that I used in that email was a good > > one. But say the hardware initialises with pull-up enabled. If it hasn't > > also been set via gpiolib then it will be reported as unknown. > > > > Which makes me wonder whether we shouldn't add a get_config() callback > to drivers in gpiolib for that because some controllers allow you to > query their current settings. > I've thought the same, but until all the pinctrl drivers support it you are still going to be returning unknown. So you still can't provide any guarantee that the information is available. And as such is it any more use than just requiring the user set it explicitly? And if they really care about the bias they will probably set it anyway. > --- > When inspecting the current bias setting of a GPIO line, the AS_IS name > of one of the possible values really means that the kernel GPIO subsystem > can't determine the bias setting because it didn't set it itself (e.g. the > hardware may have internally initialized pull-up or pull-down resistors). > In this case it's better to change the name to BIAS_UNKNOWN to reflect that. > --- > > Does this sound good? > That works for me. Cheers, Kent.
From: Bartosz Golaszewski <bgolaszewski@baylibre.com> This is another batch of improvements to libgpiod before we overhaul the data structure model. The last patch adds the kernel uapi header to the repository so that we no longer depend on its presence on the host system. v1 -> v2: - rename BIAS_NONE to BIAS_UNKNOWN - rename DRIVE_NONE to DRIVE_PUSH_PULL - rename BIAS_DISABLE to BIAS_DISABLED Bartosz Golaszewski (7): treewide: remove helpers for opening chips by name & number treewide: simplify the active-low line property treewide: rename BIAS_AS_IS to BIAS_UNKNOWN treewide: rename BIAS_DISABLE to BIAS_DISABLED treewide: make drive settings an enum bindings: cxx: line: reorder bias mapping entries core: add the kernel uapi header to the repository bindings/cxx/chip.cpp | 41 +- bindings/cxx/examples/gpioinfocxx.cpp | 3 +- bindings/cxx/gpiod.hpp | 61 +-- bindings/cxx/line.cpp | 28 +- bindings/cxx/line_bulk.cpp | 4 +- bindings/cxx/tests/tests-chip.cpp | 97 +---- bindings/cxx/tests/tests-event.cpp | 14 +- bindings/cxx/tests/tests-iter.cpp | 2 +- bindings/cxx/tests/tests-line.cpp | 97 ++--- bindings/python/examples/gpioinfo.py | 4 +- bindings/python/gpiodmodule.c | 187 +++------ bindings/python/tests/gpiod_py_test.py | 177 ++++----- configure.ac | 12 +- include/gpiod.h | 69 +--- lib/Makefile.am | 2 +- lib/core.c | 35 +- lib/helpers.c | 57 --- lib/uapi/gpio.h | 522 +++++++++++++++++++++++++ tests/tests-chip.c | 41 -- tests/tests-event.c | 2 +- tests/tests-line.c | 110 +++--- tools/gpiodetect.c | 2 +- tools/gpiofind.c | 2 +- tools/gpioget.c | 2 +- tools/gpioinfo.c | 30 +- tools/gpiomon.c | 2 +- tools/gpioset.c | 2 +- tools/tools-common.c | 59 ++- tools/tools-common.h | 3 + 29 files changed, 934 insertions(+), 733 deletions(-) create mode 100644 lib/uapi/gpio.h