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

[Kent Gibson <warthog618@gmail.com>]

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.