Re: [libgpiod v2][PATCH v3 2/3] line-config: expose the override logic to users

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Mar 03, 2022 at 10:18:35AM +0100, Bartosz Golaszewski wrote:
> We've already added getters for line-config but without exposing some
> parts of the internal logic of the object, the user can't really get
> the full picture and inspect the contents. This patch reworks the
> accessors further by providing access to the underlying override
> mechanism.
> 
> For every setting, we expose a getter and setter for the default value
> as well as a set of four functions for setting, getting, clearing and
> checking per-offset overrides.
> 
> An override can initially have the same value as the defaults but will
> retain the overridden value should the defaults change.
> 
> We also complete the API by providing functions that allow to retrieve
> the overridden offsets and their corresponding property types.
> 
> This way the caller can fully inspect the line_config and high-level
> language bindings can provide stringification methods.
> 
> While at it: we fix a couple bugs in the implementation of struct
> line_config and add new constructors that take a variable list of
> arguments.
> 

The variadic constructor is new for patch v3.
It bundles default constructor + default mutators, so doesn't add
functionality that wasn't already available - it just makes it
accessible via a single function call.
Is the variadic form beneficial for bindings, say Python, where you
would prefer not to be making a bunch of C calls?
Or is this just sugar?

No major objection, just curious about the rationale for adding it.

[snip]

> +/**
> + * @brief Get the total number of overridden settings currently stored by this
> + *        line config object.
> + * @param config Line config object.
> + * @return Number of individual overridden settings.
> + */
> +unsigned int
> +gpiod_line_config_get_num_overrides(struct gpiod_line_config *config);
> +
> +/**
> + * @brief Get the list of overridden offsets and the corresponding types of
> + *        overridden settings.
> + * @param config Line config object.
> + * @param offsets Array to store the overidden offsets. Must hold at least the
> + *                number of unsigned integers returned by
> + *                ::gpiod_line_config_get_output_value_offset.
> + * @param props Array to store the types of overridden settings. Must hold at
> + *              least the number of integers returned by
> + *              gpiod_line_config_get_output_value_offset.
> + */

The purpose of the offsets and props arrays is not clear.
Clarify that you are returning a list of (offset,prop), split across the
two arrays.
Replace them with a single array of (offset,prop) unless there is
a good reason to keep them separate?

Guessing it should be gpiod_line_config_get_num_overrides(), not
gpiod_line_config_get_output_value_offset() which returns 0 or 1, or
even better -1 for inputs?

[snip]

> +GPIOD_API unsigned int
> +gpiod_line_config_get_num_overrides(struct gpiod_line_config *config)
> +{
> +	struct override_config *override;
> +	unsigned int i, j, count = 0;
>  
> -	errno = ENXIO;
> +	for (i = 0; i < GPIO_V2_LINES_MAX; i++) {
> +		override = &config->overrides[i];
> +
> +		if (override_used(override)) {
> +			for (j = 0; j < NUM_OVERRIDE_FLAGS; j++) {
> +				if (override->override_flags &
> +				    override_flag_list[j])
> +					count++;
> +			}
> +		}
> +	}
> +
> +	return count;
> +}
> +

Using GPIO_V2_LINES_MAX for the size of the overrides array is
confusing, and the two should be de-coupled so you can more easily resize
the array if necessary.
Provide a NUM_OVERRIDES_MAX, or similar, and use that when referring
to the size of the overrides array.

> +static int override_flag_to_prop(int flag)
> +{
> +	switch (flag) {
> +	case OVERRIDE_FLAG_DIRECTION:
> +		return GPIOD_LINE_CONFIG_PROP_DIRECTION;
> +	case OVERRIDE_FLAG_EDGE:
> +		return GPIOD_LINE_CONFIG_PROP_EDGE;
> +	case OVERRIDE_FLAG_BIAS:
> +		return GPIOD_LINE_CONFIG_PROP_BIAS;
> +	case OVERRIDE_FLAG_DRIVE:
> +		return GPIOD_LINE_CONFIG_PROP_DRIVE;
> +	case OVERRIDE_FLAG_ACTIVE_LOW:
> +		return GPIOD_LINE_CONFIG_PROP_ACTIVE_LOW;
> +	case OVERRIDE_FLAG_DEBOUNCE_PERIOD:
> +		return GPIOD_LINE_CONFIG_PROP_DEBOUNCE_PERIOD;
> +	case OVERRIDE_FLAG_CLOCK:
> +		return GPIOD_LINE_CONFIG_PROP_EVENT_CLOCK;
> +	case OVERRIDE_FLAG_OUTPUT_VALUE:
> +		return GPIOD_LINE_CONFIG_PROP_OUTPUT_VALUE;
> +	}
> +
> +	/* Can't happen. */
>  	return -1;
>  }
>  
> +GPIOD_API void
> +gpiod_line_config_get_overrides(struct gpiod_line_config *config,
> +				unsigned int *offsets, int *props)
> +{
> +	struct override_config *override;
> +	unsigned int i, j, count = 0;
> +
> +	for (i = 0; i < GPIO_V2_LINES_MAX; i++) {
> +		override = &config->overrides[i];
> +
> +		if (override_used(override)) {
> +			for (j = 0; j < NUM_OVERRIDE_FLAGS; j++) {
> +				if (override->override_flags &
> +				    override_flag_list[j]) {
> +					offsets[count] = override->offset;
> +					props[count] = override_flag_to_prop(
> +							override_flag_list[j]);
> +					count++;
> +				}
> +			}
> +		}
> +	}
> +}
> +

Return the count?
Would be a bit redundant, as the user needs to call 
gpiod_line_config_get_num_overrides() to size the offsets and props
arrays beforehand, but the usual patten when writing into a passed array
is to return the number of elements written.

Cheers,
Kent.