diff mbox series

[libgpiod,10/16] core: provide gpiod_line_config_set_output_values()

Message ID 20230113215210.616812-11-brgl@bgdev.pl
State New
Headers show
Series [libgpiod,01/16] README: update for libgpiod v2 | expand

Commit Message

Bartosz Golaszewski Jan. 13, 2023, 9:52 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Currently if user wants to use the same settings for a set of requested
lines with the exception of the output value - they need to go through
hoops by updating the line settings object and adding it one by one to
the line config. Provide a helper function that allows to set a global
list of output values that override the settings. For details on the
interface: see documentation in this commit.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/gpiod.h            | 27 +++++++++++++++
 lib/line-config.c          | 60 +++++++++++++++++++++++++++++++---
 tests/gpiod-test-helpers.h | 10 ++++++
 tests/tests-line-config.c  | 67 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 160 insertions(+), 4 deletions(-)

Comments

Kent Gibson Jan. 16, 2023, 12:15 a.m. UTC | #1 
On Fri, Jan 13, 2023 at 10:52:04PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Currently if user wants to use the same settings for a set of requested
> lines with the exception of the output value - they need to go through
> hoops by updating the line settings object and adding it one by one to
> the line config. Provide a helper function that allows to set a global
> list of output values that override the settings. For details on the
> interface: see documentation in this commit.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  include/gpiod.h            | 27 +++++++++++++++
>  lib/line-config.c          | 60 +++++++++++++++++++++++++++++++---
>  tests/gpiod-test-helpers.h | 10 ++++++
>  tests/tests-line-config.c  | 67 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 160 insertions(+), 4 deletions(-)
> 
> diff --git a/include/gpiod.h b/include/gpiod.h
> index 8cede47..c552135 100644
> --- a/include/gpiod.h
> +++ b/include/gpiod.h
> @@ -785,6 +785,33 @@ struct gpiod_line_settings *
>  gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
>  				    unsigned int offset);
>  
> +/**
> + * @brief Set output values for a number of lines.
> + * @param config Line config object.
> + * @param values Buffer containing the output values.
> + * @param num_values Number of values in the buffer.
> + * @return 0 on success, -1 on error.
> + *
> + * This is a helper that allows users to set multiple (potentially different)
> + * output values at once while using the same line settings object. Instead of
> + * modifying the output value in the settings object and calling
> + * ::gpiod_line_config_add_line_settings multiple times, we can specify the
> + * settings, add them for a set of offsets and then call this function to
> + * set the output values.
> + *

A helper such as this did cross my mind when updating gpioset, but I
didn't consider it worth the effort then, and still don't.

So the user has to set ALL output values, or more correctly the first num_values,
right? i.e. it is the block form.

This helper is only helpful if the user already has ALL the values in the
required array format, as is the case for gpioset, else they need to build
the array to pass - in which case they may as well call
gpiod_line_config_add_line_settings() for each line.
So is it really that helpful?

The sparse form would be more generally useful, particularly in the
bindings. i.e. they should accept a mapping from offsets to values rather
than the ordered list.  Though, again, those may be best implemented with
multiple calls to ::gpiod_line_config_add_line_settings rather than
jumping through the hoops of constructing the parameters for another helper
like this one.

> + * Values set by this function override whatever values were specified in the
> + * regular line settings.
> + *
> + * The order of the output values in the array corresponds with the order in
> + * which offsets were added by ::gpiod_line_config_add_line_settings. For
> + * example calling add_settings([1, 3]) and add_settings([2, 4]) and then
> + * calling this function with the following logicall values : [0, 1, 0, 1]
> + * will result in the following offset->value mapping: 1->0, 2->0, 3->1, 4->1.
> + */
> +int gpiod_line_config_set_output_values(struct gpiod_line_config *config,
> +					const enum gpiod_line_value *values,
> +					size_t num_values);
> +

But if you do keep it...

Use documentation from gpiod_line_request_set_values(), suitably modified,
to describe ordering - it is clearer and does not imply that the user has
to independently know the order lines were added - it is in the order
provided by gpiod_line_config_get_offsets().

Cheers,
Kent.
Bartosz Golaszewski Jan. 16, 2023, 10:23 p.m. UTC | #2 
On Mon, Jan 16, 2023 at 1:15 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jan 13, 2023 at 10:52:04PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Currently if user wants to use the same settings for a set of requested
> > lines with the exception of the output value - they need to go through
> > hoops by updating the line settings object and adding it one by one to
> > the line config. Provide a helper function that allows to set a global
> > list of output values that override the settings. For details on the
> > interface: see documentation in this commit.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  include/gpiod.h            | 27 +++++++++++++++
> >  lib/line-config.c          | 60 +++++++++++++++++++++++++++++++---
> >  tests/gpiod-test-helpers.h | 10 ++++++
> >  tests/tests-line-config.c  | 67 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 160 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/gpiod.h b/include/gpiod.h
> > index 8cede47..c552135 100644
> > --- a/include/gpiod.h
> > +++ b/include/gpiod.h
> > @@ -785,6 +785,33 @@ struct gpiod_line_settings *
> >  gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
> >                                   unsigned int offset);
> >
> > +/**
> > + * @brief Set output values for a number of lines.
> > + * @param config Line config object.
> > + * @param values Buffer containing the output values.
> > + * @param num_values Number of values in the buffer.
> > + * @return 0 on success, -1 on error.
> > + *
> > + * This is a helper that allows users to set multiple (potentially different)
> > + * output values at once while using the same line settings object. Instead of
> > + * modifying the output value in the settings object and calling
> > + * ::gpiod_line_config_add_line_settings multiple times, we can specify the
> > + * settings, add them for a set of offsets and then call this function to
> > + * set the output values.
> > + *
>
> A helper such as this did cross my mind when updating gpioset, but I
> didn't consider it worth the effort then, and still don't.
>
> So the user has to set ALL output values, or more correctly the first num_values,
> right? i.e. it is the block form.
>
> This helper is only helpful if the user already has ALL the values in the
> required array format, as is the case for gpioset, else they need to build
> the array to pass - in which case they may as well call
> gpiod_line_config_add_line_settings() for each line.
> So is it really that helpful?

It seems to me that building/adding a separate settings object for
every line is quite cumbersome (I know that C is cumbersome in itself
but still...). IMO more so than just storing values in an array. And
in many cases the user may already know the right set of values in
which case it boils down to `static const enum gpiod_line_value vals[]
= { ... };`

This is also pretty much the equivalent of the
gpiod_line_request_set_values() function just run at request-time.

>
> The sparse form would be more generally useful, particularly in the
> bindings. i.e. they should accept a mapping from offsets to values rather
> than the ordered list.  Though, again, those may be best implemented with
> multiple calls to ::gpiod_line_config_add_line_settings rather than
> jumping through the hoops of constructing the parameters for another helper
> like this one.
>

I'd argue that this should be implemented in bindings using the new function.

> > + * Values set by this function override whatever values were specified in the
> > + * regular line settings.
> > + *
> > + * The order of the output values in the array corresponds with the order in
> > + * which offsets were added by ::gpiod_line_config_add_line_settings. For
> > + * example calling add_settings([1, 3]) and add_settings([2, 4]) and then
> > + * calling this function with the following logicall values : [0, 1, 0, 1]
> > + * will result in the following offset->value mapping: 1->0, 2->0, 3->1, 4->1.
> > + */
> > +int gpiod_line_config_set_output_values(struct gpiod_line_config *config,
> > +                                     const enum gpiod_line_value *values,
> > +                                     size_t num_values);
> > +
>
> But if you do keep it...
>
> Use documentation from gpiod_line_request_set_values(), suitably modified,
> to describe ordering - it is clearer and does not imply that the user has
> to independently know the order lines were added - it is in the order
> provided by gpiod_line_config_get_offsets().
>

I'd like to have some version of that. Maybe keep what I proposed for
C but have something more elaborate (with mappings?) for bindings? I
need to think about it tomorrow.

Bart
diff mbox series

Patch

diff --git a/include/gpiod.h b/include/gpiod.h
index 8cede47..c552135 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -785,6 +785,33 @@  struct gpiod_line_settings *
 gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
 				    unsigned int offset);
 
+/**
+ * @brief Set output values for a number of lines.
+ * @param config Line config object.
+ * @param values Buffer containing the output values.
+ * @param num_values Number of values in the buffer.
+ * @return 0 on success, -1 on error.
+ *
+ * This is a helper that allows users to set multiple (potentially different)
+ * output values at once while using the same line settings object. Instead of
+ * modifying the output value in the settings object and calling
+ * ::gpiod_line_config_add_line_settings multiple times, we can specify the
+ * settings, add them for a set of offsets and then call this function to
+ * set the output values.
+ *
+ * Values set by this function override whatever values were specified in the
+ * regular line settings.
+ *
+ * The order of the output values in the array corresponds with the order in
+ * which offsets were added by ::gpiod_line_config_add_line_settings. For
+ * example calling add_settings([1, 3]) and add_settings([2, 4]) and then
+ * calling this function with the following logicall values : [0, 1, 0, 1]
+ * will result in the following offset->value mapping: 1->0, 2->0, 3->1, 4->1.
+ */
+int gpiod_line_config_set_output_values(struct gpiod_line_config *config,
+					const enum gpiod_line_value *values,
+					size_t num_values);
+
 /**
  * @brief Get the number of configured line offsets.
  * @param config Line config object.
diff --git a/lib/line-config.c b/lib/line-config.c
index b00e5e6..901eb02 100644
--- a/lib/line-config.c
+++ b/lib/line-config.c
@@ -25,6 +25,8 @@  struct per_line_config {
 struct gpiod_line_config {
 	struct per_line_config line_configs[LINES_MAX];
 	size_t num_configs;
+	enum gpiod_line_value output_values[LINES_MAX];
+	size_t num_output_values;
 	struct settings_node *sref_list;
 };
 
@@ -136,23 +138,60 @@  GPIOD_API struct gpiod_line_settings *
 gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
 				    unsigned int offset)
 {
+	struct gpiod_line_settings *settings;
 	struct per_line_config *per_line;
 	size_t i;
+	int ret;
 
 	assert(config);
 
 	for (i = 0; i < config->num_configs; i++) {
 		per_line = &config->line_configs[i];
 
-		if (per_line->offset == offset)
-			return gpiod_line_settings_copy(
+		if (per_line->offset == offset) {
+			settings = gpiod_line_settings_copy(
 					per_line->node->settings);
+			if (!settings)
+				return NULL;
+
+			/*
+			 * If a global output value was set for this line - use
+			 * it and override the one stored in settings.
+			 */
+			if (config->num_output_values > i) {
+				ret = gpiod_line_settings_set_output_value(
+						settings,
+						config->output_values[i]);
+				if (ret) {
+					gpiod_line_settings_free(settings);
+					return NULL;
+				}
+			}
+
+			return settings;
+		}
 	}
 
 	errno = ENOENT;
 	return NULL;
 }
 
+GPIOD_API int
+gpiod_line_config_set_output_values(struct gpiod_line_config *config,
+				    const enum gpiod_line_value *values,
+				    size_t num_values)
+{
+	if (num_values > LINES_MAX) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	memcpy(config->output_values, values, num_values * sizeof(*values));
+	config->num_output_values = num_values;
+
+	return 0;
+}
+
 GPIOD_API size_t
 gpiod_line_config_get_num_configured_offsets(struct gpiod_line_config *config)
 {
@@ -209,6 +248,13 @@  static bool has_at_least_one_output_direction(struct gpiod_line_config *config)
 	return false;
 }
 
+static void set_output_value(uint64_t *vals, size_t bit,
+			     enum gpiod_line_value value)
+{
+	gpiod_line_mask_assign_bit(vals, bit,
+				   value == GPIOD_LINE_VALUE_ACTIVE ? 1 : 0);
+}
+
 static void set_kernel_output_values(uint64_t *mask, uint64_t *vals,
 				     struct gpiod_line_config *config)
 {
@@ -230,8 +276,14 @@  static void set_kernel_output_values(uint64_t *mask, uint64_t *vals,
 		gpiod_line_mask_set_bit(mask, i);
 		value = gpiod_line_settings_get_output_value(
 			per_line->node->settings);
-		gpiod_line_mask_assign_bit(
-			vals, i, value == GPIOD_LINE_VALUE_ACTIVE ? 1 : 0);
+		set_output_value(vals, i, value);
+	}
+
+	/* "Global" output values override the ones from per-line settings. */
+	for (i = 0; i < config->num_output_values; i++) {
+		gpiod_line_mask_set_bit(mask, i);
+		value = config->output_values[i];
+		set_output_value(vals, i, value);
 	}
 }
 
diff --git a/tests/gpiod-test-helpers.h b/tests/gpiod-test-helpers.h
index fb3fd7d..760949e 100644
--- a/tests/gpiod-test-helpers.h
+++ b/tests/gpiod-test-helpers.h
@@ -136,6 +136,16 @@  G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_edge_event_buffer,
 		_settings; \
 	})
 
+#define gpiod_test_line_config_set_output_values_or_fail(_line_cfg, _values, \
+							 _num_values) \
+	do { \
+		gint _ret = gpiod_line_config_set_output_values(_line_cfg, \
+								_values, \
+								_num_values); \
+		g_assert_cmpint(_ret, ==, 0); \
+		gpiod_test_return_if_failed(); \
+	} while (0)
+
 #define gpiod_test_create_request_config_or_fail() \
 	({ \
 		struct gpiod_request_config *_config = \
diff --git a/tests/tests-line-config.c b/tests/tests-line-config.c
index 7f5e4b1..78a4632 100644
--- a/tests/tests-line-config.c
+++ b/tests/tests-line-config.c
@@ -238,3 +238,70 @@  GPIOD_TEST_CASE(get_null_offsets)
 							       NULL, 10);
 	g_assert_cmpuint(num_offsets, ==, 0);
 }
+
+GPIOD_TEST_CASE(set_global_output_values)
+{
+	static const guint offsets[] = { 0, 1, 2, 3 };
+	static const enum gpiod_line_value values[] = {
+		GPIOD_LINE_VALUE_ACTIVE,
+		GPIOD_LINE_VALUE_INACTIVE,
+		GPIOD_LINE_VALUE_ACTIVE,
+		GPIOD_LINE_VALUE_INACTIVE,
+	};
+
+	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 4, NULL);
+	g_autoptr(struct_gpiod_line_settings) settings = NULL;
+	g_autoptr(struct_gpiod_line_config) config = NULL;
+	g_autoptr(struct_gpiod_line_request) request = NULL;
+	g_autoptr(struct_gpiod_chip) chip = NULL;
+
+	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
+	settings = gpiod_test_create_line_settings_or_fail();
+	config = gpiod_test_create_line_config_or_fail();
+
+	gpiod_line_settings_set_direction(settings,
+					  GPIOD_LINE_DIRECTION_OUTPUT);
+	gpiod_test_line_config_add_line_settings_or_fail(config, offsets, 4,
+							 settings);
+	gpiod_test_line_config_set_output_values_or_fail(config, values, 4);
+
+	request = gpiod_test_request_lines_or_fail(chip, NULL, config);
+
+	g_assert_cmpint(g_gpiosim_chip_get_value(sim, 0), ==,
+			GPIOD_LINE_VALUE_ACTIVE);
+	g_assert_cmpint(g_gpiosim_chip_get_value(sim, 1), ==,
+			GPIOD_LINE_VALUE_INACTIVE);
+	g_assert_cmpint(g_gpiosim_chip_get_value(sim, 2), ==,
+			GPIOD_LINE_VALUE_ACTIVE);
+	g_assert_cmpint(g_gpiosim_chip_get_value(sim, 3), ==,
+			GPIOD_LINE_VALUE_INACTIVE);
+}
+
+GPIOD_TEST_CASE(read_back_global_output_values)
+{
+	static const guint offsets[] = { 0, 1, 2, 3 };
+	static const enum gpiod_line_value values[] = {
+		GPIOD_LINE_VALUE_ACTIVE,
+		GPIOD_LINE_VALUE_INACTIVE,
+		GPIOD_LINE_VALUE_ACTIVE,
+		GPIOD_LINE_VALUE_INACTIVE,
+	};
+
+	g_autoptr(struct_gpiod_line_settings) settings = NULL;
+	g_autoptr(struct_gpiod_line_settings) retrieved = NULL;
+	g_autoptr(struct_gpiod_line_config) config = NULL;
+
+	settings = gpiod_test_create_line_settings_or_fail();
+	config = gpiod_test_create_line_config_or_fail();
+
+	gpiod_line_settings_set_direction(settings,
+					  GPIOD_LINE_DIRECTION_OUTPUT);
+	gpiod_line_settings_set_output_value(settings, GPIOD_LINE_VALUE_ACTIVE);
+	gpiod_test_line_config_add_line_settings_or_fail(config, offsets, 4,
+							 settings);
+	gpiod_test_line_config_set_output_values_or_fail(config, values, 4);
+
+	retrieved = gpiod_test_line_config_get_line_settings_or_fail(config, 1);
+	g_assert_cmpint(gpiod_line_settings_get_output_value(retrieved), ==,
+			GPIOD_LINE_VALUE_INACTIVE);
+}