| Message ID | 20250410-open-drain-source-tests-v1-1-a062d2280cc5@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [libgpiod] tests: uapi: add test-cases for open-drain and open-source emulation | expand |
On Thu, Apr 10, 2025 at 12:17 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > The kernel GPIO subsystem can emulate open-drain and open-source by not > actively driving the line for active and inactive output values > respectively. The kernel does it by setting the line to input in these > cases but this still must be reported as output to user-space. Add new > test-cases that verify this behavior. Thanks, that's indeed a good idea! Reviewed-by: Andy Shevchenko <andy@kernel.org>
On Thu, Apr 10, 2025 at 11:17:47AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > The kernel GPIO subsystem can emulate open-drain and open-source by not > actively driving the line for active and inactive output values > respectively. The kernel does it by setting the line to input in these > cases but this still must be reported as output to user-space. Add new > test-cases that verify this behavior. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > Andy's comment on a GPIOLIB patch made me realize it's a good idea to > add tests for open-drain and open-source emulation in the kernel where > we don't actively drive the line for active and inactive values > respectively. > --- > tests/tests-kernel-uapi.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > > diff --git a/tests/tests-kernel-uapi.c b/tests/tests-kernel-uapi.c > index ff220fc..5955fac 100644 > --- a/tests/tests-kernel-uapi.c > +++ b/tests/tests-kernel-uapi.c > @@ -110,3 +110,90 @@ GPIOD_TEST_CASE(enable_debounce_then_edge_detection) > > g_assert_cmpuint(ts_falling, >, ts_rising); > } > + > +GPIOD_TEST_CASE(open_drain_emulation) > +{ > + static const guint offset = 2; > + > + g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL); > + g_autoptr(struct_gpiod_chip) chip = NULL; > + g_autoptr(struct_gpiod_line_settings) settings = NULL; > + g_autoptr(struct_gpiod_line_config) line_cfg = NULL; > + g_autoptr(struct_gpiod_line_request) request = NULL; > + g_autoptr(struct_gpiod_line_info) info = NULL; > + gint ret; > + > + chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim)); > + settings = gpiod_test_create_line_settings_or_fail(); > + line_cfg = gpiod_test_create_line_config_or_fail(); > + > + gpiod_line_settings_set_direction(settings, > + GPIOD_LINE_DIRECTION_OUTPUT); > + gpiod_line_settings_set_drive(settings, GPIOD_LINE_DRIVE_OPEN_DRAIN); > + gpiod_test_line_config_add_line_settings_or_fail(line_cfg, &offset, 1, > + settings); > + request = gpiod_test_chip_request_lines_or_fail(chip, NULL, line_cfg); > + > + ret = gpiod_line_request_set_value(request, offset, > + GPIOD_LINE_VALUE_ACTIVE); > + g_assert_cmpint(ret, ==, 0); > + gpiod_test_return_if_failed(); > + > + /* > + * The open-drain emulation in the kernel will set the line's direction > + * to input but NOT set FLAG_IS_OUT. Let's verify the direction is > + * still reported as output. > + */ My understanding is that FLAG_IS_OUT is always set for output lines, even if the direction is set to input for the emulation. To quote gpiod_direction_output(): set_output_flag: /* * When emulating open-source or open-drain functionalities by not * actively driving the line (setting mode to input) we still need to * set the IS_OUT flag or otherwise we won't be able to set the line * value anymore. */ if (ret == 0) set_bit(FLAG_IS_OUT, &desc->flags); return ret; Cheers, Kent.
On Thu, Apr 10, 2025 at 08:34:37PM +0300, Andy Shevchenko wrote: > On Thu, Apr 10, 2025 at 12:17 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > The kernel GPIO subsystem can emulate open-drain and open-source by not > > actively driving the line for active and inactive output values > > respectively. The kernel does it by setting the line to input in these > > cases but this still must be reported as output to user-space. Add new > > test-cases that verify this behavior. > > Thanks, that's indeed a good idea! > Reviewed-by: Andy Shevchenko <andy@kernel.org> > FWIW, the test suite[1] I wrote for my Go library along with the uAPI v2 also covers these cases, and more - particularly error cases. Not that I've run it over Bart's changes yet, but it is my first stop when checking for uAPI breakage. It is written in Go, as there obviously was no libgpiod v2 at the time. You can run the whole uapi test suite yourself by grabbing that repo and Go and... ~/go-gpiocdev/uapi$ go test -c ~/go-gpiocdev/uapi$ sudo ./uapi.test --test.v I believe the cases above are covered (mostly) by $ sudo ./uapi.test --test.v --test.run "TestGetLine$/output_[drain|source]" === RUN TestGetLine === RUN TestGetLine/output_drain === RUN TestGetLine/output_source --- PASS: TestGetLine (0.01s) --- PASS: TestGetLine/output_drain (0.00s) --- PASS: TestGetLine/output_source (0.00s) PASS which request the line then check that the reported info corresponds. I don't seem to have any tests that cover setting the value of open-drain/open-source outputs yet, so I might have to add some. Though my existing set value tests check the value reported by the gpio-sim, which will behave differently for emulated outputs... Which probably explains why I didn't test that already - it is getting into the bowels of gpiolib so it is really testing gpiolib behaviour, not just the uAPI. (I do have a few kernel specific tests, but they have generally been added to test specific bugs that were visible though the uAPI.) On that point, it might also be handy if we could turn open-drain/open-source support on and off in the gpio-sim (per sim), so tests can control if they are getting the emulation, or not. Cheers, Kent. [1] https://github.com/warthog618/go-gpiocdev
On Fri, 11 Apr 2025 at 03:34, Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Apr 10, 2025 at 11:17:47AM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > The kernel GPIO subsystem can emulate open-drain and open-source by not > > actively driving the line for active and inactive output values > > respectively. The kernel does it by setting the line to input in these > > cases but this still must be reported as output to user-space. Add new > > test-cases that verify this behavior. > > > > + ret = gpiod_line_request_set_value(request, offset, > > + GPIOD_LINE_VALUE_ACTIVE); > > + g_assert_cmpint(ret, ==, 0); > > + gpiod_test_return_if_failed(); > > + > > + /* > > + * The open-drain emulation in the kernel will set the line's direction > > + * to input but NOT set FLAG_IS_OUT. Let's verify the direction is > > + * still reported as output. > > + */ > > My understanding is that FLAG_IS_OUT is always set for output lines, > even if the direction is set to input for the emulation. > Of course, it's a typo. It should have said: does NOT clear FLAG_IS_OUT. Bartosz
diff --git a/tests/tests-kernel-uapi.c b/tests/tests-kernel-uapi.c index ff220fc..5955fac 100644 --- a/tests/tests-kernel-uapi.c +++ b/tests/tests-kernel-uapi.c @@ -110,3 +110,90 @@ GPIOD_TEST_CASE(enable_debounce_then_edge_detection) g_assert_cmpuint(ts_falling, >, ts_rising); } + +GPIOD_TEST_CASE(open_drain_emulation) +{ + static const guint offset = 2; + + g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL); + g_autoptr(struct_gpiod_chip) chip = NULL; + g_autoptr(struct_gpiod_line_settings) settings = NULL; + g_autoptr(struct_gpiod_line_config) line_cfg = NULL; + g_autoptr(struct_gpiod_line_request) request = NULL; + g_autoptr(struct_gpiod_line_info) info = NULL; + gint ret; + + chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim)); + settings = gpiod_test_create_line_settings_or_fail(); + line_cfg = gpiod_test_create_line_config_or_fail(); + + gpiod_line_settings_set_direction(settings, + GPIOD_LINE_DIRECTION_OUTPUT); + gpiod_line_settings_set_drive(settings, GPIOD_LINE_DRIVE_OPEN_DRAIN); + gpiod_test_line_config_add_line_settings_or_fail(line_cfg, &offset, 1, + settings); + request = gpiod_test_chip_request_lines_or_fail(chip, NULL, line_cfg); + + ret = gpiod_line_request_set_value(request, offset, + GPIOD_LINE_VALUE_ACTIVE); + g_assert_cmpint(ret, ==, 0); + gpiod_test_return_if_failed(); + + /* + * The open-drain emulation in the kernel will set the line's direction + * to input but NOT set FLAG_IS_OUT. Let's verify the direction is + * still reported as output. + */ + info = gpiod_test_chip_get_line_info_or_fail(chip, offset); + g_assert_cmpint(gpiod_line_info_get_direction(info), ==, + GPIOD_LINE_DIRECTION_OUTPUT); + g_assert_cmpint(gpiod_line_info_get_drive(info), ==, + GPIOD_LINE_DRIVE_OPEN_DRAIN); + + /* + * The actual line is not being actively driven, so check that too on + * the gpio-sim end. + */ + g_assert_cmpint(g_gpiosim_chip_get_value(sim, offset), ==, + G_GPIOSIM_VALUE_INACTIVE); +} + +GPIOD_TEST_CASE(open_source_emulation) +{ + static const guint offset = 2; + + g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL); + g_autoptr(struct_gpiod_chip) chip = NULL; + g_autoptr(struct_gpiod_line_settings) settings = NULL; + g_autoptr(struct_gpiod_line_config) line_cfg = NULL; + g_autoptr(struct_gpiod_line_request) request = NULL; + g_autoptr(struct_gpiod_line_info) info = NULL; + gint ret; + + chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim)); + settings = gpiod_test_create_line_settings_or_fail(); + line_cfg = gpiod_test_create_line_config_or_fail(); + + gpiod_line_settings_set_direction(settings, + GPIOD_LINE_DIRECTION_OUTPUT); + gpiod_line_settings_set_drive(settings, GPIOD_LINE_DRIVE_OPEN_SOURCE); + gpiod_test_line_config_add_line_settings_or_fail(line_cfg, &offset, 1, + settings); + request = gpiod_test_chip_request_lines_or_fail(chip, NULL, line_cfg); + + ret = gpiod_line_request_set_value(request, offset, + GPIOD_LINE_VALUE_INACTIVE); + g_assert_cmpint(ret, ==, 0); + gpiod_test_return_if_failed(); + + /* + * The open-source emulation in the kernel will set the line's direction + * to input but NOT set FLAG_IS_OUT. Let's verify the direction is + * still reported as output. + */ + info = gpiod_test_chip_get_line_info_or_fail(chip, offset); + g_assert_cmpint(gpiod_line_info_get_direction(info), ==, + GPIOD_LINE_DIRECTION_OUTPUT); + g_assert_cmpint(gpiod_line_info_get_drive(info), ==, + GPIOD_LINE_DRIVE_OPEN_SOURCE); +}