* [libgpiod v2][PATCH] line-config: rework the internal implementation
@ 2021-09-22 8:11 Bartosz Golaszewski
2021-10-05 4:20 ` Kent Gibson
0 siblings, 1 reply; 3+ messages in thread
From: Bartosz Golaszewski @ 2021-09-22 8:11 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Jack Winch,
Helmut Grohne, Ben Hutchings
Cc: linux-gpio, Bartosz Golaszewski
This reworks how the line config objects work internally. In order to reduce
the size of the gpiod_line_config objects, we switch from using a set number
of override config structures with each storing a list of line offsets to
storing a smaller override object for each of the maximum of 64 lines.
Additionally these internal config structures are now packed and only occupy
the minimum required amount of memory.
The processing of these new overrides has become a bit more complicated but
should actually be more robust wrt corner cases.
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
include/gpiod.h | 42 +--
lib/internal.h | 6 +-
lib/line-config.c | 748 ++++++++++++++++++++++++++--------------------
3 files changed, 439 insertions(+), 357 deletions(-)
diff --git a/include/gpiod.h b/include/gpiod.h
index 44deafc..2a41fca 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -674,8 +674,8 @@ void gpiod_line_config_set_active_low_subset(struct gpiod_line_config *config,
* @note If an offset is used for which no config was provided, the function
* will return the global default value.
*/
-bool gpiod_line_config_is_active_low(struct gpiod_line_config *config,
- unsigned int offset);
+bool gpiod_line_config_get_active_low(struct gpiod_line_config *config,
+ unsigned int offset);
/**
* @brief Set all lines as active-high.
@@ -811,15 +811,6 @@ void gpiod_line_config_set_output_values(struct gpiod_line_config *config,
const unsigned int *offsets,
const int *values);
-/**
- * @brief Get the number of line offsets for which this config object stores
- * output values.
- * @param config Line config object.
- * @return Number of output values currently configured for this object.
- */
-unsigned int
-gpiod_line_config_num_output_values(struct gpiod_line_config *config);
-
/**
* @brief Get the output value configured for a given line.
* @param config Line config object.
@@ -829,35 +820,6 @@ gpiod_line_config_num_output_values(struct gpiod_line_config *config);
int gpiod_line_config_get_output_value(struct gpiod_line_config *config,
unsigned int offset);
-/**
- * @brief Get the output value mapping (offset -> value) at given index.
- * @param config Line config object.
- * @param index Position of the mapping in the internal array.
- * @param offset Buffer for storing the offset of the line.
- * @param value Buffer for storing the value corresponding to the offset.
- * @return Returns 0 on success, -1 if the index is out of range.
- *
- * This function together with ::gpiod_line_config_num_output_values allows to
- * iterate over all output value mappings currently held by this object.
- */
-int gpiod_line_config_get_output_value_index(struct gpiod_line_config *config,
- unsigned int index,
- unsigned int *offset, int *value);
-
-/**
- * @brief Get all output value mappings stored in this config object.
- * @param config Line config object.
- * @param offsets Buffer in which offsets will be stored.
- * @param values Buffer in which values will be stored.
- * @note Both the offsets and values buffers must be able to hold at least the
- * number of elements returned by ::gpiod_line_config_num_output_values.
- *
- * Each offset in the offsets array corresponds to the value in the values
- * array at the same index.
- */
-void gpiod_line_config_get_output_values(struct gpiod_line_config *config,
- unsigned int *offsets, int *values);
-
/**
* @}
*
diff --git a/lib/internal.h b/lib/internal.h
index a5e47e3..32f36b5 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -12,7 +12,11 @@
/* For internal library use only. */
-#define GPIOD_API __attribute__((visibility("default")))
+#define GPIOD_API __attribute__((visibility("default")))
+#define GPIOD_PACKED __attribute__((packed))
+#define GPIOD_UNUSED __attribute__((unused))
+
+#define GPIOD_BIT(nr) (1UL << (nr))
struct gpiod_line_info *
gpiod_line_info_from_kernel(struct gpio_v2_line_info *infobuf);
diff --git a/lib/line-config.c b/lib/line-config.c
index 5f356c3..b99aeef 100644
--- a/lib/line-config.c
+++ b/lib/line-config.c
@@ -11,40 +11,40 @@
#include "internal.h"
struct base_config {
- int direction;
- int edge;
- int drive;
- int bias;
- bool active_low;
- int clock;
- unsigned long debounce_period;
-};
-
-struct secondary_config {
- struct base_config config;
- /* Offsets are sorted and duplicates are removed. */
- unsigned int offsets[GPIO_V2_LINES_MAX];
- unsigned int num_offsets;
-};
-
-struct output_value {
+ unsigned int direction : 2;
+ unsigned int edge : 3;
+ unsigned int drive : 2;
+ unsigned int bias : 3;
+ bool active_low : 1;
+ unsigned int clock : 2;
+ unsigned long debounce_period_us;
+} GPIOD_PACKED;
+
+#define OVERRIDE_FLAG_DIRECTION GPIOD_BIT(0)
+#define OVERRIDE_FLAG_EDGE GPIOD_BIT(1)
+#define OVERRIDE_FLAG_DRIVE GPIOD_BIT(2)
+#define OVERRIDE_FLAG_BIAS GPIOD_BIT(3)
+#define OVERRIDE_FLAG_ACTIVE_LOW GPIOD_BIT(4)
+#define OVERRIDE_FLAG_CLOCK GPIOD_BIT(5)
+#define OVERRIDE_FLAG_DEBOUNCE_PERIOD GPIOD_BIT(6)
+
+/*
+ * Config overriding the defaults for a single line offset. Only flagged
+ * settings are actually overriden for a line.
+ */
+struct override_config {
+ struct base_config base;
unsigned int offset;
- int value;
-};
+ bool value_set : 1;
+ unsigned int value : 1;
+ unsigned int override_flags : 7;
+} GPIOD_PACKED;
struct gpiod_line_config {
bool too_complex;
- struct base_config primary;
- struct secondary_config secondary[GPIO_V2_LINE_NUM_ATTRS_MAX];
- unsigned int num_secondary;
- struct output_value output_values[GPIO_V2_LINES_MAX];
- unsigned int num_output_values;
- /*
- * Used to temporarily store sorted offsets when looking for existing
- * configuration
- */
- unsigned int sorted_offsets[GPIO_V2_LINES_MAX];
- unsigned int num_sorted_offsets;
+ struct base_config defaults;
+ struct override_config overrides[GPIO_V2_LINES_MAX];
+ unsigned int num_overrides;
};
static void init_base_config(struct base_config *config)
@@ -55,7 +55,7 @@ static void init_base_config(struct base_config *config)
config->drive = GPIOD_LINE_DRIVE_PUSH_PULL;
config->active_low = false;
config->clock = GPIOD_LINE_EVENT_CLOCK_MONOTONIC;
- config->debounce_period = 0;
+ config->debounce_period_us = 0;
}
GPIOD_API struct gpiod_line_config *gpiod_line_config_new(void)
@@ -84,112 +84,73 @@ GPIOD_API void gpiod_line_config_reset(struct gpiod_line_config *config)
int i;
memset(config, 0, sizeof(*config));
- init_base_config(&config->primary);
+ init_base_config(&config->defaults);
for (i = 0; i < GPIO_V2_LINE_NUM_ATTRS_MAX; i++)
- init_base_config(&config->secondary[i].config);
-}
-
-static int offset_compare(const void *a_ptr, const void *b_ptr)
-{
- unsigned int a = *((unsigned int *)a_ptr);
- unsigned int b = *((unsigned int *)b_ptr);
-
- return a - b;
+ init_base_config(&config->overrides[i].base);
}
-static void sanitize_offsets(struct gpiod_line_config *config,
- unsigned int num_offsets,
- const unsigned int *offsets)
+static struct override_config *
+get_override_by_offset(struct gpiod_line_config *config, unsigned int offset)
{
- unsigned int i, count, *sorted = config->sorted_offsets;
-
- if (num_offsets == 0 || num_offsets == 1)
- return;
-
- count = num_offsets > GPIO_V2_LINES_MAX ? GPIO_V2_LINES_MAX
- : num_offsets;
- config->num_sorted_offsets = num_offsets;
+ struct override_config *override;
+ unsigned int i;
- memcpy(config->sorted_offsets, offsets, count);
- qsort(sorted, count, sizeof(*sorted), offset_compare);
+ for (i = 0; i < config->num_overrides; i++) {
+ override = &config->overrides[i];
- for (i = 0; i < (count - 1); i++) {
- if (sorted[i] == sorted[i + 1]) {
- if (i < (count - 2))
- memmove(sorted + i + 1, sorted + i + 2,
- sizeof(*sorted) * num_offsets - i);
- config->num_sorted_offsets--;
- }
+ if (override->offset == offset)
+ return override;
}
+
+ return NULL;
}
-static struct secondary_config *
-find_matching_secondary_config(struct gpiod_line_config *config)
+static struct override_config *
+get_new_override(struct gpiod_line_config *config, unsigned int offset)
{
- unsigned int i, *offsets, num_offsets;
- struct secondary_config *secondary;
+ struct override_config *override;
- offsets = config->sorted_offsets;
- num_offsets = config->num_sorted_offsets;
-
- for (i = 0; i < config->num_secondary; i++) {
- secondary = &config->secondary[i];
-
- if (num_offsets != secondary->num_offsets)
- continue;
-
- if (memcmp(secondary->offsets, offsets,
- sizeof(*offsets) * num_offsets) == 0)
- return secondary;
+ if (config->num_overrides == GPIO_V2_LINES_MAX) {
+ config->too_complex = true;
+ return NULL;
}
- return NULL;
+ override = &config->overrides[config->num_overrides++];
+ override->offset = offset;
+
+ return override;
}
-static struct secondary_config *
-get_secondary_config(struct gpiod_line_config *config,
- unsigned int num_offsets, const unsigned int *offsets)
+static struct override_config *
+get_override_config_for_writing(struct gpiod_line_config *config,
+ unsigned int offset)
{
- struct secondary_config *secondary;
+ struct override_config *override;
if (config->too_complex)
return NULL;
- sanitize_offsets(config, num_offsets, offsets);
- secondary = find_matching_secondary_config(config);
- if (!secondary) {
- if (config->num_secondary == GPIO_V2_LINE_NUM_ATTRS_MAX) {
- config->too_complex = true;
+ override = get_override_by_offset(config, offset);
+ if (!override) {
+ override = get_new_override(config, offset);
+ if (!override)
return NULL;
- }
-
- secondary = &config->secondary[config->num_secondary++];
}
- return secondary;
+ return override;
}
static struct base_config *
-get_base_config_for_offset(struct gpiod_line_config *config,
- unsigned int offset)
+get_base_config_for_reading(struct gpiod_line_config *config,
+ unsigned int offset, unsigned int flag)
{
- struct secondary_config *secondary;
- unsigned int i, j;
-
- /*
- * We're looking backwards as the settings get overwritten if set
- * multiple times.
- */
- for (i = config->num_secondary; i > 0; i--) {
- secondary = &config->secondary[i - 1];
+ struct override_config *override;
- for (j = 0; j < secondary->num_offsets; j++) {
- if (secondary->offsets[j] == offset)
- return &secondary->config;
- }
- }
+ override = get_override_by_offset(config, offset);
+ if (!override || !(override->override_flags & flag))
+ return &config->defaults;
- return NULL;
+ return &override->base;
}
static void set_direction(struct base_config *config, int direction)
@@ -209,7 +170,7 @@ static void set_direction(struct base_config *config, int direction)
GPIOD_API void
gpiod_line_config_set_direction(struct gpiod_line_config *config, int direction)
{
- set_direction(&config->primary, direction);
+ set_direction(&config->defaults, direction);
}
GPIOD_API void
@@ -224,13 +185,19 @@ gpiod_line_config_set_direction_subset(struct gpiod_line_config *config,
int direction, unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
- set_direction(&secondary->config, direction);
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
+
+ set_direction(&override->base, direction);
+ override->override_flags |= OVERRIDE_FLAG_DIRECTION;
+ }
}
GPIOD_API int gpiod_line_config_get_direction(struct gpiod_line_config *config,
@@ -238,9 +205,8 @@ GPIOD_API int gpiod_line_config_get_direction(struct gpiod_line_config *config,
{
struct base_config *base;
- base = get_base_config_for_offset(config, offset);
- if (!base)
- return config->primary.direction;
+ base = get_base_config_for_reading(config, offset,
+ OVERRIDE_FLAG_DIRECTION);
return base->direction;
}
@@ -263,7 +229,7 @@ static void set_edge_detection(struct base_config *config, int edge)
GPIOD_API void
gpiod_line_config_set_edge_detection(struct gpiod_line_config *config, int edge)
{
- set_edge_detection(&config->primary, edge);
+ set_edge_detection(&config->defaults, edge);
}
GPIOD_API void
@@ -278,13 +244,19 @@ gpiod_line_config_set_edge_detection_subset(struct gpiod_line_config *config,
int edge, unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
- set_edge_detection(&secondary->config, edge);
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
+
+ set_edge_detection(&override->base, edge);
+ override->override_flags |= OVERRIDE_FLAG_EDGE;
+ }
}
GPIOD_API int
@@ -293,9 +265,7 @@ gpiod_line_config_get_edge_detection(struct gpiod_line_config *config,
{
struct base_config *base;
- base = get_base_config_for_offset(config, offset);
- if (!base)
- return config->primary.edge;
+ base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_EDGE);
return base->edge;
}
@@ -318,7 +288,7 @@ static void set_bias(struct base_config *config, int bias)
GPIOD_API void
gpiod_line_config_set_bias(struct gpiod_line_config *config, int bias)
{
- set_bias(&config->primary, bias);
+ set_bias(&config->defaults, bias);
}
GPIOD_API void
@@ -333,13 +303,19 @@ gpiod_line_config_set_bias_subset(struct gpiod_line_config *config,
int bias, unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
- set_bias(&secondary->config, bias);
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
+
+ set_bias(&override->base, bias);
+ override->override_flags |= OVERRIDE_FLAG_BIAS;
+ }
}
GPIOD_API int gpiod_line_config_get_bias(struct gpiod_line_config *config,
@@ -347,9 +323,7 @@ GPIOD_API int gpiod_line_config_get_bias(struct gpiod_line_config *config,
{
struct base_config *base;
- base = get_base_config_for_offset(config, offset);
- if (!base)
- return config->primary.bias;
+ base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_BIAS);
return base->bias;
}
@@ -371,7 +345,7 @@ static void set_drive(struct base_config *config, int drive)
GPIOD_API void
gpiod_line_config_set_drive(struct gpiod_line_config *config, int drive)
{
- set_drive(&config->primary, drive);
+ set_drive(&config->defaults, drive);
}
GPIOD_API void
@@ -386,13 +360,19 @@ gpiod_line_config_set_drive_subset(struct gpiod_line_config *config,
int drive, unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
- set_drive(&secondary->config, drive);
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
+
+ set_drive(&override->base, drive);
+ override->override_flags |= OVERRIDE_FLAG_DRIVE;
+ }
}
GPIOD_API int gpiod_line_config_get_drive(struct gpiod_line_config *config,
@@ -400,9 +380,7 @@ GPIOD_API int gpiod_line_config_get_drive(struct gpiod_line_config *config,
{
struct base_config *base;
- base = get_base_config_for_offset(config, offset);
- if (!base)
- return config->primary.drive;
+ base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_DRIVE);
return base->drive;
}
@@ -410,7 +388,7 @@ GPIOD_API int gpiod_line_config_get_drive(struct gpiod_line_config *config,
GPIOD_API void
gpiod_line_config_set_active_low(struct gpiod_line_config *config)
{
- config->primary.active_low = true;
+ config->defaults.active_low = true;
}
GPIOD_API void
@@ -425,23 +403,29 @@ gpiod_line_config_set_active_low_subset(struct gpiod_line_config *config,
unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
+
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
- secondary->config.active_low = true;
+ override->base.active_low = true;
+ override->override_flags |= OVERRIDE_FLAG_ACTIVE_LOW;
+ }
}
-GPIOD_API bool gpiod_line_config_is_active_low(struct gpiod_line_config *config,
- unsigned int offset)
+GPIOD_API bool
+gpiod_line_config_get_active_low(struct gpiod_line_config *config,
+ unsigned int offset)
{
struct base_config *base;
- base = get_base_config_for_offset(config, offset);
- if (!base)
- return config->primary.active_low;
+ base = get_base_config_for_reading(config, offset,
+ OVERRIDE_FLAG_ACTIVE_LOW);
return base->active_low;
}
@@ -449,7 +433,7 @@ GPIOD_API bool gpiod_line_config_is_active_low(struct gpiod_line_config *config,
GPIOD_API void
gpiod_line_config_set_active_high(struct gpiod_line_config *config)
{
- config->primary.active_low = false;
+ config->defaults.active_low = false;
}
GPIOD_API void
@@ -464,20 +448,26 @@ gpiod_line_config_set_active_high_subset(struct gpiod_line_config *config,
unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
+
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
- secondary->config.active_low = false;
+ override->base.active_low = false;
+ override->override_flags |= OVERRIDE_FLAG_ACTIVE_LOW;
+ }
}
GPIOD_API void
gpiod_line_config_set_debounce_period_us(struct gpiod_line_config *config,
unsigned long period)
{
- config->primary.debounce_period = period;
+ config->defaults.debounce_period_us = period;
}
GPIOD_API void
@@ -497,13 +487,19 @@ gpiod_line_config_set_debounce_period_us_subset(
unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
+
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
- secondary->config.debounce_period = period;
+ override->base.debounce_period_us = period;
+ override->override_flags |= OVERRIDE_FLAG_DEBOUNCE_PERIOD;
+ }
}
GPIOD_API unsigned long
@@ -512,11 +508,10 @@ gpiod_line_config_get_debounce_us_period(struct gpiod_line_config *config,
{
struct base_config *base;
- base = get_base_config_for_offset(config, offset);
- if (!base)
- return config->primary.debounce_period;
+ base = get_base_config_for_reading(config, offset,
+ OVERRIDE_FLAG_DEBOUNCE_PERIOD);
- return base->debounce_period;
+ return base->debounce_period_us;
}
static void set_event_clock(struct base_config *config, int clock)
@@ -535,7 +530,7 @@ static void set_event_clock(struct base_config *config, int clock)
GPIOD_API void
gpiod_line_config_set_event_clock(struct gpiod_line_config *config, int clock)
{
- set_event_clock(&config->primary, clock);
+ set_event_clock(&config->defaults, clock);
}
GPIOD_API void
@@ -550,43 +545,42 @@ gpiod_line_config_set_event_clock_subset(struct gpiod_line_config *config,
int clock, unsigned int num_offsets,
const unsigned int *offsets)
{
- struct secondary_config *secondary;
+ struct override_config *override;
+ unsigned int i, offset;
- secondary = get_secondary_config(config, num_offsets, offsets);
- if (!secondary)
- return;
+ for (i = 0; i < num_offsets; i++) {
+ offset = offsets[i];
- set_event_clock(&secondary->config, clock);
-}
+ override = get_override_config_for_writing(config, offset);
+ if (!override)
+ return;
-GPIOD_API void
-gpiod_line_config_set_output_value(struct gpiod_line_config *config,
- unsigned int offset, int value)
-{
- gpiod_line_config_set_output_values(config, 1, &offset, &value);
+ set_event_clock(&override->base, clock);
+ }
}
-static int output_value_find_offset(struct gpiod_line_config *config,
- unsigned int offset)
+GPIOD_API int
+gpiod_line_config_get_event_clock(struct gpiod_line_config *config,
+ unsigned int offset)
{
- unsigned int i;
+ struct base_config *base;
- for (i = 0; i < config->num_output_values; i++) {
- if (config->output_values[i].offset == offset)
- return i;
- }
+ base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_CLOCK);
- return -1;
+ return base->clock;
}
-static void set_output_value(struct gpiod_line_config *config, unsigned int idx,
- unsigned int offset, int value, bool inc)
+static void set_output_value(struct override_config *override, int value)
{
- config->output_values[idx].offset = offset;
- config->output_values[idx].value = value;
+ override->value = !!value;
+ override->value_set = true;
+}
- if (inc)
- config->num_output_values++;
+GPIOD_API void
+gpiod_line_config_set_output_value(struct gpiod_line_config *config,
+ unsigned int offset, int value)
+{
+ gpiod_line_config_set_output_values(config, 1, &offset, &value);
}
GPIOD_API void
@@ -595,82 +589,40 @@ gpiod_line_config_set_output_values(struct gpiod_line_config *config,
const unsigned int *offsets,
const int *values)
{
- unsigned int i;
- int pos;
-
- if (config->too_complex)
- return;
+ struct override_config *override;
+ unsigned int i, offset, val;
for (i = 0; i < num_values; i++) {
- pos = output_value_find_offset(config, offsets[i]);
- if (pos < 0) {
- if (config->num_output_values == GPIO_V2_LINES_MAX) {
- /* Too many output values specified. */
- config->too_complex = true;
- return;
- }
+ offset = offsets[i];
+ val = values[i];
- /* Add new output value. */
- set_output_value(config, config->num_output_values,
- offsets[i], values[i], true);
- } else {
- /* Overwrite old value for this offset. */
- set_output_value(config, pos,
- offsets[i], values[i], false);
+ override = get_override_by_offset(config, offset);
+ if (!override) {
+ override = get_new_override(config, offset);
+ if (!override)
+ return;
}
- }
-}
-GPIOD_API unsigned int
-gpiod_line_config_num_output_values(struct gpiod_line_config *config)
-{
- return config->num_output_values;
+ set_output_value(override, val);
+ }
}
GPIOD_API int
gpiod_line_config_get_output_value(struct gpiod_line_config *config,
unsigned int offset)
{
- unsigned int i;
-
- for (i = 0; i < config->num_output_values; i++) {
- if (config->output_values[i].offset == offset)
- return config->output_values[i].value;
- }
+ struct override_config *override;
- errno = ENXIO;
- return -1;
-}
-
-GPIOD_API int
-gpiod_line_config_get_output_value_index(struct gpiod_line_config *config,
- unsigned int index,
- unsigned int *offset, int *value)
-{
- if (index >= config->num_output_values) {
- errno = EINVAL;
+ override = get_override_by_offset(config, offset);
+ if (!override || !override->value_set) {
+ errno = ENXIO;
return -1;
}
- *offset = config->output_values[index].offset;
- *value = config->output_values[index].value;
-
- return 0;
-}
-
-GPIOD_API void
-gpiod_line_config_get_output_values(struct gpiod_line_config *config,
- unsigned int *offsets, int *values)
-{
- unsigned int i;
-
- for (i = 0; i < config->num_output_values; i++) {
- offsets[i] = config->output_values[i].offset;
- values[i] = config->output_values[i].value;
- }
+ return override->value;
}
-static uint64_t gpiod_make_kernel_flags(struct base_config *config)
+static uint64_t make_kernel_flags(const struct base_config *config)
{
uint64_t flags = 0;
@@ -753,48 +705,205 @@ static int set_kernel_output_values(uint64_t *mask, uint64_t *vals,
unsigned int num_lines,
const unsigned int *offsets)
{
- struct output_value *outval;
+ struct override_config *override;
unsigned int i;
int idx;
gpiod_line_mask_zero(mask);
gpiod_line_mask_zero(vals);
- for (i = 0; i < config->num_output_values; i++) {
- outval = &config->output_values[i];
+ for (i = 0; i < config->num_overrides; i++) {
+ override = &config->overrides[i];
+
+ if (override->value_set) {
+ idx = find_bitmap_index(override->offset,
+ num_lines, offsets);
+ if (idx < 0) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ gpiod_line_mask_set_bit(mask, idx);
+ gpiod_line_mask_assign_bit(vals, idx,
+ !!override->value);
+ }
+ }
+
+ return 0;
+}
+
+static bool base_config_flags_are_equal(struct base_config *base,
+ struct override_config *override)
+{
+ if (((override->override_flags & OVERRIDE_FLAG_DIRECTION) &&
+ base->direction != override->base.direction) ||
+ ((override->override_flags & OVERRIDE_FLAG_EDGE) &&
+ base->edge != override->base.edge) ||
+ ((override->override_flags & OVERRIDE_FLAG_DRIVE) &&
+ base->drive != override->base.drive) ||
+ ((override->override_flags & OVERRIDE_FLAG_BIAS) &&
+ base->bias != override->base.bias) ||
+ ((override->override_flags & OVERRIDE_FLAG_ACTIVE_LOW) &&
+ base->active_low != override->base.active_low) ||
+ ((override->override_flags & OVERRIDE_FLAG_CLOCK) &&
+ base->clock != override->base.clock))
+ return false;
+
+ return true;
+}
+
+static bool override_config_flags_are_equal(struct override_config *a,
+ struct override_config *b)
+{
+ if (base_config_flags_are_equal(&a->base, b) &&
+ ((a->override_flags & ~OVERRIDE_FLAG_DEBOUNCE_PERIOD) ==
+ (b->override_flags & ~OVERRIDE_FLAG_DEBOUNCE_PERIOD)))
+ return true;
+
+ return false;
+}
+
+static void set_base_config_flags(struct gpio_v2_line_attribute *attr,
+ struct override_config *override,
+ struct gpiod_line_config *config)
+{
+ struct base_config base;
+
+ memcpy(&base, &config->defaults, sizeof(base));
+
+ if (override->override_flags & OVERRIDE_FLAG_DIRECTION)
+ base.direction = override->base.direction;
+ if (override->override_flags & OVERRIDE_FLAG_EDGE)
+ base.edge = override->base.edge;
+ if (override->override_flags & OVERRIDE_FLAG_BIAS)
+ base.bias = override->base.bias;
+ if (override->override_flags & OVERRIDE_FLAG_DRIVE)
+ base.drive = override->base.drive;
+ if (override->override_flags & OVERRIDE_FLAG_ACTIVE_LOW)
+ base.active_low = override->base.active_low;
+ if (override->override_flags & OVERRIDE_FLAG_CLOCK)
+ base.clock = override->base.clock;
+
+ attr->id = GPIO_V2_LINE_ATTR_ID_FLAGS;
+ attr->flags = make_kernel_flags(&base);
+}
+
+static bool base_debounce_period_is_equal(struct base_config *base,
+ struct override_config *override)
+{
+ if ((override->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD) &&
+ base->debounce_period_us != override->base.debounce_period_us)
+ return false;
+
+ return true;
+}
+
+static bool override_config_debounce_period_is_equal(struct override_config *a,
+ struct override_config *b)
+{
+ if (base_debounce_period_is_equal(&a->base, b) &&
+ ((a->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD) ==
+ (b->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD)))
+ return true;
+
+ return false;
+}
+
+static void
+set_base_config_debounce_period(struct gpio_v2_line_attribute *attr,
+ struct override_config *override,
+ struct gpiod_line_config *config GPIOD_UNUSED)
+{
+ attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
+ attr->debounce_period_us = override->base.debounce_period_us;
+}
+
+static int set_kernel_attr_mask(uint64_t *out, const uint64_t *in,
+ unsigned int num_lines,
+ const unsigned int *offsets,
+ const struct gpiod_line_config *config)
+{
+ unsigned int i, j;
+ int off;
+
+ gpiod_line_mask_zero(out);
+
+ for (i = 0; i < config->num_overrides; i++) {
+ if (!gpiod_line_mask_test_bit(in, i))
+ continue;
+
+ for (j = 0, off = -1; j < num_lines; j++) {
+ if (offsets[j] == config->overrides[i].offset) {
+ off = j;
+ break;
+ }
+ }
- idx = find_bitmap_index(outval->offset, num_lines, offsets);
- if (idx < 0) {
+ if (off < 0) {
errno = EINVAL;
return -1;
}
- gpiod_line_mask_set_bit(mask, idx);
- gpiod_line_mask_assign_bit(vals, idx, !!outval->value);
+ gpiod_line_mask_set_bit(out, off);
}
return 0;
}
-static int set_secondary_mask(uint64_t *mask,
- struct secondary_config *sec_cfg,
- unsigned int num_lines,
- const unsigned int *offsets)
+static int process_overrides(struct gpiod_line_config *config,
+ struct gpio_v2_line_config *cfgbuf,
+ unsigned int *attr_idx,
+ unsigned int num_lines,
+ const unsigned int *offsets,
+ bool (*defaults_equal_func)(struct base_config *,
+ struct override_config *),
+ bool (*override_equal_func)(
+ struct override_config *,
+ struct override_config *),
+ void (*set_func)(struct gpio_v2_line_attribute *,
+ struct override_config *,
+ struct gpiod_line_config *))
{
- unsigned int i;
- int idx;
+ struct gpio_v2_line_config_attribute *attr;
+ uint64_t processed = 0, marked = 0, mask;
+ struct override_config *current, *next;
+ unsigned int i, j;
- gpiod_line_mask_zero(mask);
+ for (i = 0; i < config->num_overrides; i++) {
+ if (gpiod_line_mask_test_bit(&processed, i))
+ continue;
- for (i = 0; i < sec_cfg->num_offsets; i++) {
- idx = find_bitmap_index(sec_cfg->offsets[i],
- num_lines, offsets);
- if (idx < 0) {
- errno = EINVAL;
+ current = &config->overrides[i];
+ gpiod_line_mask_set_bit(&processed, i);
+
+ if (defaults_equal_func(&config->defaults, current))
+ continue;
+
+ marked = 0;
+ gpiod_line_mask_set_bit(&marked, i);
+
+ for (j = i + 1; j < config->num_overrides; j++) {
+ if (gpiod_line_mask_test_bit(&processed, j))
+ continue;
+
+ next = &config->overrides[j];
+
+ if (override_equal_func(current, next)) {
+ gpiod_line_mask_set_bit(&marked, j);
+ gpiod_line_mask_set_bit(&processed, j);
+ }
+ }
+
+ attr = &cfgbuf->attrs[(*attr_idx)++];
+ if (*attr_idx == GPIO_V2_LINE_NUM_ATTRS_MAX) {
+ errno = E2BIG;
return -1;
}
- gpiod_line_mask_set_bit(mask, idx);
+ set_kernel_attr_mask(&mask, &marked,
+ num_lines, offsets, config);
+ attr->mask = mask;
+ set_func(&attr->attr, current, config);
}
return 0;
@@ -806,7 +915,7 @@ int gpiod_line_config_to_kernel(struct gpiod_line_config *config,
const unsigned int *offsets)
{
struct gpio_v2_line_config_attribute *attr;
- struct secondary_config *sec_cfg;
+ struct override_config *override;
unsigned int attr_idx = 0, i;
uint64_t mask, values;
int ret;
@@ -819,59 +928,66 @@ int gpiod_line_config_to_kernel(struct gpiod_line_config *config,
if (config->too_complex)
goto err_2big;
- if (config->num_output_values) {
- if (config->num_output_values > num_lines)
- goto err_2big;
+ /*
+ * First check if we have at least one default output value configured.
+ * If so, let's take one attribute for the default values.
+ */
+ for (i = 0; i < config->num_overrides; i++) {
+ override = &config->overrides[i];
- attr = &cfgbuf->attrs[attr_idx++];
- attr->attr.id = GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES;
+ if (override->value_set) {
+ attr = &cfgbuf->attrs[attr_idx++];
+ attr->attr.id = GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES;
- ret = set_kernel_output_values(&mask, &values, config,
- num_lines, offsets);
- if (ret)
- return ret;
+ ret = set_kernel_output_values(&mask, &values, config,
+ num_lines, offsets);
+ if (ret)
+ return ret;
- attr->attr.values = values;
- attr->mask = mask;
+ attr->attr.values = values;
+ attr->mask = mask;
+
+ break;
+ }
}
- if (config->primary.debounce_period) {
+ /* If we have a default debounce period - use another attribute. */
+ if (config->defaults.debounce_period_us) {
attr = &cfgbuf->attrs[attr_idx++];
attr->attr.id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
- attr->attr.debounce_period_us = config->primary.debounce_period;
+ attr->attr.debounce_period_us =
+ config->defaults.debounce_period_us;
gpiod_line_mask_fill(&mask);
attr->mask = mask;
}
- for (i = 0; i < config->num_secondary; i++, attr_idx++) {
- if (attr_idx == GPIO_V2_LINE_NUM_ATTRS_MAX)
- goto err_2big;
-
- sec_cfg = &config->secondary[i];
- attr = &cfgbuf->attrs[attr_idx];
-
- if (sec_cfg->num_offsets > num_lines)
- goto err_2big;
-
- if (sec_cfg->config.debounce_period) {
- attr->attr.id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
- attr->attr.debounce_period_us =
- sec_cfg->config.debounce_period;
- } else {
- attr->attr.id = GPIO_V2_LINE_ATTR_ID_FLAGS;
-
- attr->attr.flags = gpiod_make_kernel_flags(
- &sec_cfg->config);
- }
+ /*
+ * The overrides are processed independently for regular flags and the
+ * debounce period. We iterate over the configured line overrides. We
+ * first check if the given set of options is equal to the global
+ * defaults. If not, we mark it and iterate over the remaining
+ * overrides looking for ones that have the same config as the one
+ * currently processed. We mark them too and at the end we create a
+ * single kernel attribute with the translated config and the mask
+ * corresponding to all marked overrides. Those are now excluded from
+ * further processing.
+ */
- ret = set_secondary_mask(&mask, sec_cfg, num_lines, offsets);
- if (ret)
- return -1;
+ ret = process_overrides(config, cfgbuf, &attr_idx, num_lines, offsets,
+ base_config_flags_are_equal,
+ override_config_flags_are_equal,
+ set_base_config_flags);
+ if (ret)
+ return -1;
- attr->mask = mask;
- }
+ ret = process_overrides(config, cfgbuf, &attr_idx, num_lines, offsets,
+ base_debounce_period_is_equal,
+ override_config_debounce_period_is_equal,
+ set_base_config_debounce_period);
+ if (ret)
+ return -1;
- cfgbuf->flags = gpiod_make_kernel_flags(&config->primary);
+ cfgbuf->flags = make_kernel_flags(&config->defaults);
cfgbuf->num_attrs = attr_idx;
return 0;
--
2.30.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [libgpiod v2][PATCH] line-config: rework the internal implementation
2021-09-22 8:11 [libgpiod v2][PATCH] line-config: rework the internal implementation Bartosz Golaszewski
@ 2021-10-05 4:20 ` Kent Gibson
2021-10-08 19:15 ` Bartosz Golaszewski
0 siblings, 1 reply; 3+ messages in thread
From: Kent Gibson @ 2021-10-05 4:20 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Andy Shevchenko, Jack Winch, Helmut Grohne,
Ben Hutchings, linux-gpio
On Wed, Sep 22, 2021 at 10:11:00AM +0200, Bartosz Golaszewski wrote:
> This reworks how the line config objects work internally. In order to reduce
> the size of the gpiod_line_config objects, we switch from using a set number
> of override config structures with each storing a list of line offsets to
> storing a smaller override object for each of the maximum of 64 lines.
> Additionally these internal config structures are now packed and only occupy
> the minimum required amount of memory.
>
> The processing of these new overrides has become a bit more complicated but
> should actually be more robust wrt corner cases.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Given the level of rework the diff is harder work than looking at the
resulting code, but it all looks good to me.
A few minor comments scattered below.
> ---
> include/gpiod.h | 42 +--
> lib/internal.h | 6 +-
> lib/line-config.c | 748 ++++++++++++++++++++++++++--------------------
> 3 files changed, 439 insertions(+), 357 deletions(-)
>
> diff --git a/include/gpiod.h b/include/gpiod.h
> index 44deafc..2a41fca 100644
> --- a/include/gpiod.h
> +++ b/include/gpiod.h
> @@ -674,8 +674,8 @@ void gpiod_line_config_set_active_low_subset(struct gpiod_line_config *config,
> * @note If an offset is used for which no config was provided, the function
> * will return the global default value.
> */
> -bool gpiod_line_config_is_active_low(struct gpiod_line_config *config,
> - unsigned int offset);
> +bool gpiod_line_config_get_active_low(struct gpiod_line_config *config,
> + unsigned int offset);
>
> /**
> * @brief Set all lines as active-high.
> @@ -811,15 +811,6 @@ void gpiod_line_config_set_output_values(struct gpiod_line_config *config,
> const unsigned int *offsets,
> const int *values);
>
> -/**
> - * @brief Get the number of line offsets for which this config object stores
> - * output values.
> - * @param config Line config object.
> - * @return Number of output values currently configured for this object.
> - */
> -unsigned int
> -gpiod_line_config_num_output_values(struct gpiod_line_config *config);
> -
> /**
> * @brief Get the output value configured for a given line.
> * @param config Line config object.
> @@ -829,35 +820,6 @@ gpiod_line_config_num_output_values(struct gpiod_line_config *config);
> int gpiod_line_config_get_output_value(struct gpiod_line_config *config,
> unsigned int offset);
>
> -/**
> - * @brief Get the output value mapping (offset -> value) at given index.
> - * @param config Line config object.
> - * @param index Position of the mapping in the internal array.
> - * @param offset Buffer for storing the offset of the line.
> - * @param value Buffer for storing the value corresponding to the offset.
> - * @return Returns 0 on success, -1 if the index is out of range.
> - *
> - * This function together with ::gpiod_line_config_num_output_values allows to
> - * iterate over all output value mappings currently held by this object.
> - */
> -int gpiod_line_config_get_output_value_index(struct gpiod_line_config *config,
> - unsigned int index,
> - unsigned int *offset, int *value);
> -
> -/**
> - * @brief Get all output value mappings stored in this config object.
> - * @param config Line config object.
> - * @param offsets Buffer in which offsets will be stored.
> - * @param values Buffer in which values will be stored.
> - * @note Both the offsets and values buffers must be able to hold at least the
> - * number of elements returned by ::gpiod_line_config_num_output_values.
> - *
> - * Each offset in the offsets array corresponds to the value in the values
> - * array at the same index.
> - */
> -void gpiod_line_config_get_output_values(struct gpiod_line_config *config,
> - unsigned int *offsets, int *values);
> -
> /**
> * @}
> *
> diff --git a/lib/internal.h b/lib/internal.h
> index a5e47e3..32f36b5 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -12,7 +12,11 @@
>
> /* For internal library use only. */
>
> -#define GPIOD_API __attribute__((visibility("default")))
> +#define GPIOD_API __attribute__((visibility("default")))
> +#define GPIOD_PACKED __attribute__((packed))
> +#define GPIOD_UNUSED __attribute__((unused))
> +
> +#define GPIOD_BIT(nr) (1UL << (nr))
>
> struct gpiod_line_info *
> gpiod_line_info_from_kernel(struct gpio_v2_line_info *infobuf);
> diff --git a/lib/line-config.c b/lib/line-config.c
> index 5f356c3..b99aeef 100644
> --- a/lib/line-config.c
> +++ b/lib/line-config.c
> @@ -11,40 +11,40 @@
> #include "internal.h"
>
> struct base_config {
> - int direction;
> - int edge;
> - int drive;
> - int bias;
> - bool active_low;
> - int clock;
> - unsigned long debounce_period;
> -};
> -
> -struct secondary_config {
> - struct base_config config;
> - /* Offsets are sorted and duplicates are removed. */
> - unsigned int offsets[GPIO_V2_LINES_MAX];
> - unsigned int num_offsets;
> -};
> -
> -struct output_value {
> + unsigned int direction : 2;
> + unsigned int edge : 3;
> + unsigned int drive : 2;
> + unsigned int bias : 3;
> + bool active_low : 1;
> + unsigned int clock : 2;
> + unsigned long debounce_period_us;
> +} GPIOD_PACKED;
> +
> +#define OVERRIDE_FLAG_DIRECTION GPIOD_BIT(0)
> +#define OVERRIDE_FLAG_EDGE GPIOD_BIT(1)
> +#define OVERRIDE_FLAG_DRIVE GPIOD_BIT(2)
> +#define OVERRIDE_FLAG_BIAS GPIOD_BIT(3)
> +#define OVERRIDE_FLAG_ACTIVE_LOW GPIOD_BIT(4)
> +#define OVERRIDE_FLAG_CLOCK GPIOD_BIT(5)
> +#define OVERRIDE_FLAG_DEBOUNCE_PERIOD GPIOD_BIT(6)
> +
> +/*
> + * Config overriding the defaults for a single line offset. Only flagged
> + * settings are actually overriden for a line.
> + */
> +struct override_config {
> + struct base_config base;
> unsigned int offset;
> - int value;
> -};
> + bool value_set : 1;
> + unsigned int value : 1;
> + unsigned int override_flags : 7;
> +} GPIOD_PACKED;
>
> struct gpiod_line_config {
> bool too_complex;
> - struct base_config primary;
> - struct secondary_config secondary[GPIO_V2_LINE_NUM_ATTRS_MAX];
> - unsigned int num_secondary;
> - struct output_value output_values[GPIO_V2_LINES_MAX];
> - unsigned int num_output_values;
> - /*
> - * Used to temporarily store sorted offsets when looking for existing
> - * configuration
> - */
> - unsigned int sorted_offsets[GPIO_V2_LINES_MAX];
> - unsigned int num_sorted_offsets;
> + struct base_config defaults;
> + struct override_config overrides[GPIO_V2_LINES_MAX];
> + unsigned int num_overrides;
> };
>
> static void init_base_config(struct base_config *config)
> @@ -55,7 +55,7 @@ static void init_base_config(struct base_config *config)
> config->drive = GPIOD_LINE_DRIVE_PUSH_PULL;
> config->active_low = false;
> config->clock = GPIOD_LINE_EVENT_CLOCK_MONOTONIC;
> - config->debounce_period = 0;
> + config->debounce_period_us = 0;
> }
>
> GPIOD_API struct gpiod_line_config *gpiod_line_config_new(void)
You don't have any change to gpiod_line_config_free() here, but
isn't free() guaranteed to be NULL aware, so you can drop the NULL
check?
If not, couldn't it be flipped to:
if (config)
free(config);
Similarly gpiod_line_config_new() could have
if (config)
gpiod_line_config_reset(config);
and that "if" could be dropped if gpiod_line_config_reset() were NULL
aware.
But the general policy is that gpiod functions are not NULL aware?
> @@ -84,112 +84,73 @@ GPIOD_API void gpiod_line_config_reset(struct gpiod_line_config *config)
> int i;
>
> memset(config, 0, sizeof(*config));
> - init_base_config(&config->primary);
> + init_base_config(&config->defaults);
> for (i = 0; i < GPIO_V2_LINE_NUM_ATTRS_MAX; i++)
> - init_base_config(&config->secondary[i].config);
> -}
> -
> -static int offset_compare(const void *a_ptr, const void *b_ptr)
> -{
> - unsigned int a = *((unsigned int *)a_ptr);
> - unsigned int b = *((unsigned int *)b_ptr);
> -
> - return a - b;
> + init_base_config(&config->overrides[i].base);
> }
>
> -static void sanitize_offsets(struct gpiod_line_config *config,
> - unsigned int num_offsets,
> - const unsigned int *offsets)
> +static struct override_config *
> +get_override_by_offset(struct gpiod_line_config *config, unsigned int offset)
> {
> - unsigned int i, count, *sorted = config->sorted_offsets;
> -
> - if (num_offsets == 0 || num_offsets == 1)
> - return;
> -
> - count = num_offsets > GPIO_V2_LINES_MAX ? GPIO_V2_LINES_MAX
> - : num_offsets;
> - config->num_sorted_offsets = num_offsets;
> + struct override_config *override;
> + unsigned int i;
>
> - memcpy(config->sorted_offsets, offsets, count);
> - qsort(sorted, count, sizeof(*sorted), offset_compare);
> + for (i = 0; i < config->num_overrides; i++) {
> + override = &config->overrides[i];
>
> - for (i = 0; i < (count - 1); i++) {
> - if (sorted[i] == sorted[i + 1]) {
> - if (i < (count - 2))
> - memmove(sorted + i + 1, sorted + i + 2,
> - sizeof(*sorted) * num_offsets - i);
> - config->num_sorted_offsets--;
> - }
> + if (override->offset == offset)
> + return override;
> }
> +
> + return NULL;
> }
>
> -static struct secondary_config *
> -find_matching_secondary_config(struct gpiod_line_config *config)
> +static struct override_config *
> +get_new_override(struct gpiod_line_config *config, unsigned int offset)
> {
> - unsigned int i, *offsets, num_offsets;
> - struct secondary_config *secondary;
> + struct override_config *override;
>
> - offsets = config->sorted_offsets;
> - num_offsets = config->num_sorted_offsets;
> -
> - for (i = 0; i < config->num_secondary; i++) {
> - secondary = &config->secondary[i];
> -
> - if (num_offsets != secondary->num_offsets)
> - continue;
> -
> - if (memcmp(secondary->offsets, offsets,
> - sizeof(*offsets) * num_offsets) == 0)
> - return secondary;
> + if (config->num_overrides == GPIO_V2_LINES_MAX) {
> + config->too_complex = true;
> + return NULL;
> }
>
> - return NULL;
> + override = &config->overrides[config->num_overrides++];
> + override->offset = offset;
> +
> + return override;
> }
>
> -static struct secondary_config *
> -get_secondary_config(struct gpiod_line_config *config,
> - unsigned int num_offsets, const unsigned int *offsets)
> +static struct override_config *
> +get_override_config_for_writing(struct gpiod_line_config *config,
> + unsigned int offset)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
>
> if (config->too_complex)
> return NULL;
>
> - sanitize_offsets(config, num_offsets, offsets);
> - secondary = find_matching_secondary_config(config);
> - if (!secondary) {
> - if (config->num_secondary == GPIO_V2_LINE_NUM_ATTRS_MAX) {
> - config->too_complex = true;
> + override = get_override_by_offset(config, offset);
> + if (!override) {
> + override = get_new_override(config, offset);
> + if (!override)
> return NULL;
> - }
> -
> - secondary = &config->secondary[config->num_secondary++];
> }
>
> - return secondary;
> + return override;
> }
>
> static struct base_config *
> -get_base_config_for_offset(struct gpiod_line_config *config,
> - unsigned int offset)
> +get_base_config_for_reading(struct gpiod_line_config *config,
> + unsigned int offset, unsigned int flag)
> {
> - struct secondary_config *secondary;
> - unsigned int i, j;
> -
> - /*
> - * We're looking backwards as the settings get overwritten if set
> - * multiple times.
> - */
> - for (i = config->num_secondary; i > 0; i--) {
> - secondary = &config->secondary[i - 1];
> + struct override_config *override;
>
> - for (j = 0; j < secondary->num_offsets; j++) {
> - if (secondary->offsets[j] == offset)
> - return &secondary->config;
> - }
> - }
> + override = get_override_by_offset(config, offset);
> + if (!override || !(override->override_flags & flag))
> + return &config->defaults;
>
> - return NULL;
> + return &override->base;
> }
>
Maybe flip the logic around to make it easier to read:
if (override && (override->override_flags & flag))
return &override->base;
return &config->defaults;
Similarly in gpiod_line_config_get_output_value().
> static void set_direction(struct base_config *config, int direction)
In set_direction() you have a specific case (GPIOD_LINE_DIRECTION_AS_IS)
that matches the default behaviour. I generally drop that case and let
the default handle it, but that is just personal preference.
Similarly in other set_XXX switches.
> @@ -209,7 +170,7 @@ static void set_direction(struct base_config *config, int direction)
> GPIOD_API void
> gpiod_line_config_set_direction(struct gpiod_line_config *config, int direction)
> {
> - set_direction(&config->primary, direction);
> + set_direction(&config->defaults, direction);
> }
>
> GPIOD_API void
> @@ -224,13 +185,19 @@ gpiod_line_config_set_direction_subset(struct gpiod_line_config *config,
> int direction, unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
>
Worth the effort given it is only used once?
> - set_direction(&secondary->config, direction);
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
> +
> + set_direction(&override->base, direction);
> + override->override_flags |= OVERRIDE_FLAG_DIRECTION;
> + }
> }
>
> GPIOD_API int gpiod_line_config_get_direction(struct gpiod_line_config *config,
> @@ -238,9 +205,8 @@ GPIOD_API int gpiod_line_config_get_direction(struct gpiod_line_config *config,
> {
> struct base_config *base;
>
> - base = get_base_config_for_offset(config, offset);
> - if (!base)
> - return config->primary.direction;
> + base = get_base_config_for_reading(config, offset,
> + OVERRIDE_FLAG_DIRECTION);
>
> return base->direction;
> }
> @@ -263,7 +229,7 @@ static void set_edge_detection(struct base_config *config, int edge)
> GPIOD_API void
> gpiod_line_config_set_edge_detection(struct gpiod_line_config *config, int edge)
> {
> - set_edge_detection(&config->primary, edge);
> + set_edge_detection(&config->defaults, edge);
> }
>
> GPIOD_API void
> @@ -278,13 +244,19 @@ gpiod_line_config_set_edge_detection_subset(struct gpiod_line_config *config,
> int edge, unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
>
> - set_edge_detection(&secondary->config, edge);
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
> +
> + set_edge_detection(&override->base, edge);
> + override->override_flags |= OVERRIDE_FLAG_EDGE;
> + }
> }
>
> GPIOD_API int
> @@ -293,9 +265,7 @@ gpiod_line_config_get_edge_detection(struct gpiod_line_config *config,
> {
> struct base_config *base;
>
> - base = get_base_config_for_offset(config, offset);
> - if (!base)
> - return config->primary.edge;
> + base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_EDGE);
>
> return base->edge;
> }
> @@ -318,7 +288,7 @@ static void set_bias(struct base_config *config, int bias)
> GPIOD_API void
> gpiod_line_config_set_bias(struct gpiod_line_config *config, int bias)
> {
> - set_bias(&config->primary, bias);
> + set_bias(&config->defaults, bias);
> }
>
> GPIOD_API void
> @@ -333,13 +303,19 @@ gpiod_line_config_set_bias_subset(struct gpiod_line_config *config,
> int bias, unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
>
> - set_bias(&secondary->config, bias);
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
> +
> + set_bias(&override->base, bias);
> + override->override_flags |= OVERRIDE_FLAG_BIAS;
> + }
> }
>
> GPIOD_API int gpiod_line_config_get_bias(struct gpiod_line_config *config,
> @@ -347,9 +323,7 @@ GPIOD_API int gpiod_line_config_get_bias(struct gpiod_line_config *config,
> {
> struct base_config *base;
>
> - base = get_base_config_for_offset(config, offset);
> - if (!base)
> - return config->primary.bias;
> + base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_BIAS);
>
> return base->bias;
> }
> @@ -371,7 +345,7 @@ static void set_drive(struct base_config *config, int drive)
> GPIOD_API void
> gpiod_line_config_set_drive(struct gpiod_line_config *config, int drive)
> {
> - set_drive(&config->primary, drive);
> + set_drive(&config->defaults, drive);
> }
>
> GPIOD_API void
> @@ -386,13 +360,19 @@ gpiod_line_config_set_drive_subset(struct gpiod_line_config *config,
> int drive, unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
>
> - set_drive(&secondary->config, drive);
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
> +
> + set_drive(&override->base, drive);
> + override->override_flags |= OVERRIDE_FLAG_DRIVE;
> + }
> }
>
> GPIOD_API int gpiod_line_config_get_drive(struct gpiod_line_config *config,
> @@ -400,9 +380,7 @@ GPIOD_API int gpiod_line_config_get_drive(struct gpiod_line_config *config,
> {
> struct base_config *base;
>
> - base = get_base_config_for_offset(config, offset);
> - if (!base)
> - return config->primary.drive;
> + base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_DRIVE);
>
> return base->drive;
> }
> @@ -410,7 +388,7 @@ GPIOD_API int gpiod_line_config_get_drive(struct gpiod_line_config *config,
> GPIOD_API void
> gpiod_line_config_set_active_low(struct gpiod_line_config *config)
> {
> - config->primary.active_low = true;
> + config->defaults.active_low = true;
> }
>
> GPIOD_API void
> @@ -425,23 +403,29 @@ gpiod_line_config_set_active_low_subset(struct gpiod_line_config *config,
> unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
> +
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
>
> - secondary->config.active_low = true;
> + override->base.active_low = true;
> + override->override_flags |= OVERRIDE_FLAG_ACTIVE_LOW;
> + }
> }
>
> -GPIOD_API bool gpiod_line_config_is_active_low(struct gpiod_line_config *config,
> - unsigned int offset)
> +GPIOD_API bool
> +gpiod_line_config_get_active_low(struct gpiod_line_config *config,
> + unsigned int offset)
> {
> struct base_config *base;
>
> - base = get_base_config_for_offset(config, offset);
> - if (!base)
> - return config->primary.active_low;
> + base = get_base_config_for_reading(config, offset,
> + OVERRIDE_FLAG_ACTIVE_LOW);
>
> return base->active_low;
> }
> @@ -449,7 +433,7 @@ GPIOD_API bool gpiod_line_config_is_active_low(struct gpiod_line_config *config,
> GPIOD_API void
> gpiod_line_config_set_active_high(struct gpiod_line_config *config)
> {
> - config->primary.active_low = false;
> + config->defaults.active_low = false;
> }
>
> GPIOD_API void
> @@ -464,20 +448,26 @@ gpiod_line_config_set_active_high_subset(struct gpiod_line_config *config,
> unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
> +
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
>
> - secondary->config.active_low = false;
> + override->base.active_low = false;
> + override->override_flags |= OVERRIDE_FLAG_ACTIVE_LOW;
> + }
> }
>
gpiod_line_config_set_active_low_subset() and
gpiod_line_config_set_active_high_subset() could call a common helper
that accepts the active_low as a parameter?
> GPIOD_API void
> gpiod_line_config_set_debounce_period_us(struct gpiod_line_config *config,
> unsigned long period)
> {
> - config->primary.debounce_period = period;
> + config->defaults.debounce_period_us = period;
> }
>
> GPIOD_API void
> @@ -497,13 +487,19 @@ gpiod_line_config_set_debounce_period_us_subset(
> unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
> +
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
>
> - secondary->config.debounce_period = period;
> + override->base.debounce_period_us = period;
> + override->override_flags |= OVERRIDE_FLAG_DEBOUNCE_PERIOD;
> + }
> }
>
> GPIOD_API unsigned long
> @@ -512,11 +508,10 @@ gpiod_line_config_get_debounce_us_period(struct gpiod_line_config *config,
> {
> struct base_config *base;
>
> - base = get_base_config_for_offset(config, offset);
> - if (!base)
> - return config->primary.debounce_period;
> + base = get_base_config_for_reading(config, offset,
> + OVERRIDE_FLAG_DEBOUNCE_PERIOD);
>
> - return base->debounce_period;
> + return base->debounce_period_us;
> }
>
> static void set_event_clock(struct base_config *config, int clock)
> @@ -535,7 +530,7 @@ static void set_event_clock(struct base_config *config, int clock)
> GPIOD_API void
> gpiod_line_config_set_event_clock(struct gpiod_line_config *config, int clock)
> {
> - set_event_clock(&config->primary, clock);
> + set_event_clock(&config->defaults, clock);
> }
>
> GPIOD_API void
> @@ -550,43 +545,42 @@ gpiod_line_config_set_event_clock_subset(struct gpiod_line_config *config,
> int clock, unsigned int num_offsets,
> const unsigned int *offsets)
> {
> - struct secondary_config *secondary;
> + struct override_config *override;
> + unsigned int i, offset;
>
> - secondary = get_secondary_config(config, num_offsets, offsets);
> - if (!secondary)
> - return;
> + for (i = 0; i < num_offsets; i++) {
> + offset = offsets[i];
>
> - set_event_clock(&secondary->config, clock);
> -}
> + override = get_override_config_for_writing(config, offset);
> + if (!override)
> + return;
>
> -GPIOD_API void
> -gpiod_line_config_set_output_value(struct gpiod_line_config *config,
> - unsigned int offset, int value)
> -{
> - gpiod_line_config_set_output_values(config, 1, &offset, &value);
> + set_event_clock(&override->base, clock);
> + }
> }
>
> -static int output_value_find_offset(struct gpiod_line_config *config,
> - unsigned int offset)
> +GPIOD_API int
> +gpiod_line_config_get_event_clock(struct gpiod_line_config *config,
> + unsigned int offset)
> {
> - unsigned int i;
> + struct base_config *base;
>
> - for (i = 0; i < config->num_output_values; i++) {
> - if (config->output_values[i].offset == offset)
> - return i;
> - }
> + base = get_base_config_for_reading(config, offset, OVERRIDE_FLAG_CLOCK);
>
> - return -1;
> + return base->clock;
> }
>
> -static void set_output_value(struct gpiod_line_config *config, unsigned int idx,
> - unsigned int offset, int value, bool inc)
> +static void set_output_value(struct override_config *override, int value)
> {
> - config->output_values[idx].offset = offset;
> - config->output_values[idx].value = value;
> + override->value = !!value;
> + override->value_set = true;
> +}
>
> - if (inc)
> - config->num_output_values++;
> +GPIOD_API void
> +gpiod_line_config_set_output_value(struct gpiod_line_config *config,
> + unsigned int offset, int value)
> +{
> + gpiod_line_config_set_output_values(config, 1, &offset, &value);
> }
>
> GPIOD_API void
> @@ -595,82 +589,40 @@ gpiod_line_config_set_output_values(struct gpiod_line_config *config,
> const unsigned int *offsets,
> const int *values)
> {
> - unsigned int i;
> - int pos;
> -
> - if (config->too_complex)
> - return;
> + struct override_config *override;
> + unsigned int i, offset, val;
>
> for (i = 0; i < num_values; i++) {
> - pos = output_value_find_offset(config, offsets[i]);
> - if (pos < 0) {
> - if (config->num_output_values == GPIO_V2_LINES_MAX) {
> - /* Too many output values specified. */
> - config->too_complex = true;
> - return;
> - }
> + offset = offsets[i];
> + val = values[i];
>
> - /* Add new output value. */
> - set_output_value(config, config->num_output_values,
> - offsets[i], values[i], true);
> - } else {
> - /* Overwrite old value for this offset. */
> - set_output_value(config, pos,
> - offsets[i], values[i], false);
> + override = get_override_by_offset(config, offset);
> + if (!override) {
> + override = get_new_override(config, offset);
> + if (!override)
> + return;
> }
> - }
> -}
>
> -GPIOD_API unsigned int
> -gpiod_line_config_num_output_values(struct gpiod_line_config *config)
> -{
> - return config->num_output_values;
> + set_output_value(override, val);
> + }
> }
>
> GPIOD_API int
> gpiod_line_config_get_output_value(struct gpiod_line_config *config,
> unsigned int offset)
> {
> - unsigned int i;
> -
> - for (i = 0; i < config->num_output_values; i++) {
> - if (config->output_values[i].offset == offset)
> - return config->output_values[i].value;
> - }
> + struct override_config *override;
>
> - errno = ENXIO;
> - return -1;
> -}
> -
> -GPIOD_API int
> -gpiod_line_config_get_output_value_index(struct gpiod_line_config *config,
> - unsigned int index,
> - unsigned int *offset, int *value)
> -{
> - if (index >= config->num_output_values) {
> - errno = EINVAL;
> + override = get_override_by_offset(config, offset);
> + if (!override || !override->value_set) {
> + errno = ENXIO;
> return -1;
> }
>
> - *offset = config->output_values[index].offset;
> - *value = config->output_values[index].value;
> -
> - return 0;
> -}
> -
> -GPIOD_API void
> -gpiod_line_config_get_output_values(struct gpiod_line_config *config,
> - unsigned int *offsets, int *values)
> -{
> - unsigned int i;
> -
> - for (i = 0; i < config->num_output_values; i++) {
> - offsets[i] = config->output_values[i].offset;
> - values[i] = config->output_values[i].value;
> - }
> + return override->value;
> }
>
> -static uint64_t gpiod_make_kernel_flags(struct base_config *config)
> +static uint64_t make_kernel_flags(const struct base_config *config)
> {
> uint64_t flags = 0;
>
> @@ -753,48 +705,205 @@ static int set_kernel_output_values(uint64_t *mask, uint64_t *vals,
> unsigned int num_lines,
> const unsigned int *offsets)
> {
> - struct output_value *outval;
> + struct override_config *override;
> unsigned int i;
> int idx;
>
> gpiod_line_mask_zero(mask);
> gpiod_line_mask_zero(vals);
>
> - for (i = 0; i < config->num_output_values; i++) {
> - outval = &config->output_values[i];
> + for (i = 0; i < config->num_overrides; i++) {
> + override = &config->overrides[i];
> +
> + if (override->value_set) {
> + idx = find_bitmap_index(override->offset,
> + num_lines, offsets);
> + if (idx < 0) {
> + errno = EINVAL;
> + return -1;
> + }
> +
> + gpiod_line_mask_set_bit(mask, idx);
> + gpiod_line_mask_assign_bit(vals, idx,
> + !!override->value);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static bool base_config_flags_are_equal(struct base_config *base,
> + struct override_config *override)
> +{
> + if (((override->override_flags & OVERRIDE_FLAG_DIRECTION) &&
> + base->direction != override->base.direction) ||
> + ((override->override_flags & OVERRIDE_FLAG_EDGE) &&
> + base->edge != override->base.edge) ||
> + ((override->override_flags & OVERRIDE_FLAG_DRIVE) &&
> + base->drive != override->base.drive) ||
> + ((override->override_flags & OVERRIDE_FLAG_BIAS) &&
> + base->bias != override->base.bias) ||
> + ((override->override_flags & OVERRIDE_FLAG_ACTIVE_LOW) &&
> + base->active_low != override->base.active_low) ||
> + ((override->override_flags & OVERRIDE_FLAG_CLOCK) &&
> + base->clock != override->base.clock))
> + return false;
> +
> + return true;
> +}
> +
> +static bool override_config_flags_are_equal(struct override_config *a,
> + struct override_config *b)
> +{
> + if (base_config_flags_are_equal(&a->base, b) &&
> + ((a->override_flags & ~OVERRIDE_FLAG_DEBOUNCE_PERIOD) ==
> + (b->override_flags & ~OVERRIDE_FLAG_DEBOUNCE_PERIOD)))
> + return true;
> +
> + return false;
> +}
> +
> +static void set_base_config_flags(struct gpio_v2_line_attribute *attr,
> + struct override_config *override,
> + struct gpiod_line_config *config)
> +{
> + struct base_config base;
> +
> + memcpy(&base, &config->defaults, sizeof(base));
> +
> + if (override->override_flags & OVERRIDE_FLAG_DIRECTION)
> + base.direction = override->base.direction;
> + if (override->override_flags & OVERRIDE_FLAG_EDGE)
> + base.edge = override->base.edge;
> + if (override->override_flags & OVERRIDE_FLAG_BIAS)
> + base.bias = override->base.bias;
> + if (override->override_flags & OVERRIDE_FLAG_DRIVE)
> + base.drive = override->base.drive;
> + if (override->override_flags & OVERRIDE_FLAG_ACTIVE_LOW)
> + base.active_low = override->base.active_low;
> + if (override->override_flags & OVERRIDE_FLAG_CLOCK)
> + base.clock = override->base.clock;
> +
> + attr->id = GPIO_V2_LINE_ATTR_ID_FLAGS;
> + attr->flags = make_kernel_flags(&base);
> +}
> +
> +static bool base_debounce_period_is_equal(struct base_config *base,
> + struct override_config *override)
> +{
> + if ((override->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD) &&
> + base->debounce_period_us != override->base.debounce_period_us)
> + return false;
> +
> + return true;
> +}
> +
> +static bool override_config_debounce_period_is_equal(struct override_config *a,
> + struct override_config *b)
> +{
> + if (base_debounce_period_is_equal(&a->base, b) &&
> + ((a->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD) ==
> + (b->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD)))
> + return true;
> +
> + return false;
> +}
> +
To improve readability, flip the order here to test the flag equivalence
first.
Particularly wrt the "a" doesn't have debounce overridden case.
> +static void
> +set_base_config_debounce_period(struct gpio_v2_line_attribute *attr,
> + struct override_config *override,
> + struct gpiod_line_config *config GPIOD_UNUSED)
> +{
> + attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> + attr->debounce_period_us = override->base.debounce_period_us;
> +}
> +
> +static int set_kernel_attr_mask(uint64_t *out, const uint64_t *in,
> + unsigned int num_lines,
> + const unsigned int *offsets,
> + const struct gpiod_line_config *config)
> +{
> + unsigned int i, j;
> + int off;
> +
> + gpiod_line_mask_zero(out);
> +
> + for (i = 0; i < config->num_overrides; i++) {
> + if (!gpiod_line_mask_test_bit(in, i))
> + continue;
> +
> + for (j = 0, off = -1; j < num_lines; j++) {
> + if (offsets[j] == config->overrides[i].offset) {
> + off = j;
> + break;
> + }
> + }
>
> - idx = find_bitmap_index(outval->offset, num_lines, offsets);
> - if (idx < 0) {
> + if (off < 0) {
> errno = EINVAL;
> return -1;
> }
>
> - gpiod_line_mask_set_bit(mask, idx);
> - gpiod_line_mask_assign_bit(vals, idx, !!outval->value);
> + gpiod_line_mask_set_bit(out, off);
> }
>
> return 0;
> }
>
> -static int set_secondary_mask(uint64_t *mask,
> - struct secondary_config *sec_cfg,
> - unsigned int num_lines,
> - const unsigned int *offsets)
> +static int process_overrides(struct gpiod_line_config *config,
> + struct gpio_v2_line_config *cfgbuf,
> + unsigned int *attr_idx,
> + unsigned int num_lines,
> + const unsigned int *offsets,
> + bool (*defaults_equal_func)(struct base_config *,
> + struct override_config *),
> + bool (*override_equal_func)(
> + struct override_config *,
> + struct override_config *),
> + void (*set_func)(struct gpio_v2_line_attribute *,
> + struct override_config *,
> + struct gpiod_line_config *))
> {
> - unsigned int i;
> - int idx;
> + struct gpio_v2_line_config_attribute *attr;
> + uint64_t processed = 0, marked = 0, mask;
> + struct override_config *current, *next;
> + unsigned int i, j;
>
> - gpiod_line_mask_zero(mask);
> + for (i = 0; i < config->num_overrides; i++) {
> + if (gpiod_line_mask_test_bit(&processed, i))
> + continue;
>
> - for (i = 0; i < sec_cfg->num_offsets; i++) {
> - idx = find_bitmap_index(sec_cfg->offsets[i],
> - num_lines, offsets);
> - if (idx < 0) {
> - errno = EINVAL;
> + current = &config->overrides[i];
> + gpiod_line_mask_set_bit(&processed, i);
> +
> + if (defaults_equal_func(&config->defaults, current))
> + continue;
> +
> + marked = 0;
> + gpiod_line_mask_set_bit(&marked, i);
> +
> + for (j = i + 1; j < config->num_overrides; j++) {
> + if (gpiod_line_mask_test_bit(&processed, j))
> + continue;
> +
> + next = &config->overrides[j];
> +
> + if (override_equal_func(current, next)) {
> + gpiod_line_mask_set_bit(&marked, j);
> + gpiod_line_mask_set_bit(&processed, j);
> + }
> + }
> +
> + attr = &cfgbuf->attrs[(*attr_idx)++];
> + if (*attr_idx == GPIO_V2_LINE_NUM_ATTRS_MAX) {
> + errno = E2BIG;
> return -1;
> }
>
> - gpiod_line_mask_set_bit(mask, idx);
> + set_kernel_attr_mask(&mask, &marked,
> + num_lines, offsets, config);
> + attr->mask = mask;
> + set_func(&attr->attr, current, config);
> }
>
> return 0;
Don't see anything wrong here, but I'd like to see a bunch of tests to
cover the corner cases you mentioned, as the bulk of the module complexity
is here.
Not that I expect that now.
> @@ -806,7 +915,7 @@ int gpiod_line_config_to_kernel(struct gpiod_line_config *config,
> const unsigned int *offsets)
> {
> struct gpio_v2_line_config_attribute *attr;
> - struct secondary_config *sec_cfg;
> + struct override_config *override;
> unsigned int attr_idx = 0, i;
> uint64_t mask, values;
> int ret;
> @@ -819,59 +928,66 @@ int gpiod_line_config_to_kernel(struct gpiod_line_config *config,
> if (config->too_complex)
> goto err_2big;
>
> - if (config->num_output_values) {
> - if (config->num_output_values > num_lines)
> - goto err_2big;
> + /*
> + * First check if we have at least one default output value configured.
> + * If so, let's take one attribute for the default values.
> + */
> + for (i = 0; i < config->num_overrides; i++) {
> + override = &config->overrides[i];
>
> - attr = &cfgbuf->attrs[attr_idx++];
> - attr->attr.id = GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES;
> + if (override->value_set) {
> + attr = &cfgbuf->attrs[attr_idx++];
> + attr->attr.id = GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES;
>
> - ret = set_kernel_output_values(&mask, &values, config,
> - num_lines, offsets);
> - if (ret)
> - return ret;
> + ret = set_kernel_output_values(&mask, &values, config,
> + num_lines, offsets);
> + if (ret)
> + return ret;
>
> - attr->attr.values = values;
> - attr->mask = mask;
> + attr->attr.values = values;
> + attr->mask = mask;
> +
> + break;
> + }
> }
>
> - if (config->primary.debounce_period) {
> + /* If we have a default debounce period - use another attribute. */
> + if (config->defaults.debounce_period_us) {
> attr = &cfgbuf->attrs[attr_idx++];
> attr->attr.id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> - attr->attr.debounce_period_us = config->primary.debounce_period;
> + attr->attr.debounce_period_us =
> + config->defaults.debounce_period_us;
> gpiod_line_mask_fill(&mask);
> attr->mask = mask;
> }
>
> - for (i = 0; i < config->num_secondary; i++, attr_idx++) {
> - if (attr_idx == GPIO_V2_LINE_NUM_ATTRS_MAX)
> - goto err_2big;
> -
> - sec_cfg = &config->secondary[i];
> - attr = &cfgbuf->attrs[attr_idx];
> -
> - if (sec_cfg->num_offsets > num_lines)
> - goto err_2big;
> -
> - if (sec_cfg->config.debounce_period) {
> - attr->attr.id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> - attr->attr.debounce_period_us =
> - sec_cfg->config.debounce_period;
> - } else {
> - attr->attr.id = GPIO_V2_LINE_ATTR_ID_FLAGS;
> -
> - attr->attr.flags = gpiod_make_kernel_flags(
> - &sec_cfg->config);
> - }
> + /*
> + * The overrides are processed independently for regular flags and the
> + * debounce period. We iterate over the configured line overrides. We
> + * first check if the given set of options is equal to the global
> + * defaults. If not, we mark it and iterate over the remaining
> + * overrides looking for ones that have the same config as the one
> + * currently processed. We mark them too and at the end we create a
> + * single kernel attribute with the translated config and the mask
> + * corresponding to all marked overrides. Those are now excluded from
> + * further processing.
> + */
>
> - ret = set_secondary_mask(&mask, sec_cfg, num_lines, offsets);
> - if (ret)
> - return -1;
> + ret = process_overrides(config, cfgbuf, &attr_idx, num_lines, offsets,
> + base_config_flags_are_equal,
> + override_config_flags_are_equal,
> + set_base_config_flags);
> + if (ret)
> + return -1;
>
> - attr->mask = mask;
> - }
> + ret = process_overrides(config, cfgbuf, &attr_idx, num_lines, offsets,
> + base_debounce_period_is_equal,
> + override_config_debounce_period_is_equal,
> + set_base_config_debounce_period);
> + if (ret)
> + return -1;
>
> - cfgbuf->flags = gpiod_make_kernel_flags(&config->primary);
> + cfgbuf->flags = make_kernel_flags(&config->defaults);
> cfgbuf->num_attrs = attr_idx;
>
> return 0;
> --
> 2.30.1
>
Overall it looks really good to me, so no problem with applying it,
with or without my suggestions.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [libgpiod v2][PATCH] line-config: rework the internal implementation
2021-10-05 4:20 ` Kent Gibson
@ 2021-10-08 19:15 ` Bartosz Golaszewski
0 siblings, 0 replies; 3+ messages in thread
From: Bartosz Golaszewski @ 2021-10-08 19:15 UTC (permalink / raw)
To: Kent Gibson
Cc: Linus Walleij, Andy Shevchenko, Jack Winch, Helmut Grohne,
Ben Hutchings, open list:GPIO SUBSYSTEM
On Tue, Oct 5, 2021 at 6:20 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Sep 22, 2021 at 10:11:00AM +0200, Bartosz Golaszewski wrote:
> > This reworks how the line config objects work internally. In order to reduce
> > the size of the gpiod_line_config objects, we switch from using a set number
> > of override config structures with each storing a list of line offsets to
> > storing a smaller override object for each of the maximum of 64 lines.
> > Additionally these internal config structures are now packed and only occupy
> > the minimum required amount of memory.
> >
> > The processing of these new overrides has become a bit more complicated but
> > should actually be more robust wrt corner cases.
> >
> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
>
> Given the level of rework the diff is harder work than looking at the
> resulting code, but it all looks good to me.
> A few minor comments scattered below.
>
[snip]
> >
> > GPIOD_API struct gpiod_line_config *gpiod_line_config_new(void)
>
> You don't have any change to gpiod_line_config_free() here, but
> isn't free() guaranteed to be NULL aware, so you can drop the NULL
> check?
> If not, couldn't it be flipped to:
>
> if (config)
> free(config);
>
> Similarly gpiod_line_config_new() could have
>
> if (config)
> gpiod_line_config_reset(config);
>
> and that "if" could be dropped if gpiod_line_config_reset() were NULL
> aware.
>
No, you're right, as long as we don't dereference the pointer, we can
drop the check. Here and elsewhere.
> But the general policy is that gpiod functions are not NULL aware?
>
In general yes, but various free functions are NULL-aware.
[snip]
> > static struct base_config *
> > -get_base_config_for_offset(struct gpiod_line_config *config,
> > - unsigned int offset)
> > +get_base_config_for_reading(struct gpiod_line_config *config,
> > + unsigned int offset, unsigned int flag)
> > {
> > - struct secondary_config *secondary;
> > - unsigned int i, j;
> > -
> > - /*
> > - * We're looking backwards as the settings get overwritten if set
> > - * multiple times.
> > - */
> > - for (i = config->num_secondary; i > 0; i--) {
> > - secondary = &config->secondary[i - 1];
> > + struct override_config *override;
> >
> > - for (j = 0; j < secondary->num_offsets; j++) {
> > - if (secondary->offsets[j] == offset)
> > - return &secondary->config;
> > - }
> > - }
> > + override = get_override_by_offset(config, offset);
> > + if (!override || !(override->override_flags & flag))
> > + return &config->defaults;
> >
> > - return NULL;
> > + return &override->base;
> > }
> >
>
> Maybe flip the logic around to make it easier to read:
>
> if (override && (override->override_flags & flag))
> return &override->base;
>
> return &config->defaults;
>
> Similarly in gpiod_line_config_get_output_value().
>
Done.
> > static void set_direction(struct base_config *config, int direction)
>
> In set_direction() you have a specific case (GPIOD_LINE_DIRECTION_AS_IS)
> that matches the default behaviour. I generally drop that case and let
> the default handle it, but that is just personal preference.
>
> Similarly in other set_XXX switches.
>
My personal preference is to be explicit and include those default
cases even if only to tell my future self what's happening here.
> > @@ -209,7 +170,7 @@ static void set_direction(struct base_config *config, int direction)
> > GPIOD_API void
> > gpiod_line_config_set_direction(struct gpiod_line_config *config, int direction)
> > {
> > - set_direction(&config->primary, direction);
> > + set_direction(&config->defaults, direction);
> > }
> >
> > GPIOD_API void
> > @@ -224,13 +185,19 @@ gpiod_line_config_set_direction_subset(struct gpiod_line_config *config,
> > int direction, unsigned int num_offsets,
> > const unsigned int *offsets)
> > {
> > - struct secondary_config *secondary;
> > + struct override_config *override;
> > + unsigned int i, offset;
> >
> > - secondary = get_secondary_config(config, num_offsets, offsets);
> > - if (!secondary)
> > - return;
> > + for (i = 0; i < num_offsets; i++) {
> > + offset = offsets[i];
> >
>
> Worth the effort given it is only used once?
>
Must have been a leftover, thanks.
[snip]
> >
> > GPIOD_API void
> > @@ -464,20 +448,26 @@ gpiod_line_config_set_active_high_subset(struct gpiod_line_config *config,
> > unsigned int num_offsets,
> > const unsigned int *offsets)
> > {
> > - struct secondary_config *secondary;
> > + struct override_config *override;
> > + unsigned int i, offset;
> >
> > - secondary = get_secondary_config(config, num_offsets, offsets);
> > - if (!secondary)
> > - return;
> > + for (i = 0; i < num_offsets; i++) {
> > + offset = offsets[i];
> > +
> > + override = get_override_config_for_writing(config, offset);
> > + if (!override)
> > + return;
> >
> > - secondary->config.active_low = false;
> > + override->base.active_low = false;
> > + override->override_flags |= OVERRIDE_FLAG_ACTIVE_LOW;
> > + }
> > }
> >
>
> gpiod_line_config_set_active_low_subset() and
> gpiod_line_config_set_active_high_subset() could call a common helper
> that accepts the active_low as a parameter?
>
Makes sense.
[snip]
> > +
> > +static bool override_config_debounce_period_is_equal(struct override_config *a,
> > + struct override_config *b)
> > +{
> > + if (base_debounce_period_is_equal(&a->base, b) &&
> > + ((a->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD) ==
> > + (b->override_flags & OVERRIDE_FLAG_DEBOUNCE_PERIOD)))
> > + return true;
> > +
> > + return false;
> > +}
> > +
>
> To improve readability, flip the order here to test the flag equivalence
> first.
> Particularly wrt the "a" doesn't have debounce overridden case.
>
Done.
[snip]
>
> Don't see anything wrong here, but I'd like to see a bunch of tests to
> cover the corner cases you mentioned, as the bulk of the module complexity
> is here.
> Not that I expect that now.
>
Yes, definitely. Trying to get the gpio-sim module upstream for this
exact reason. Please take a look at this series if you can.
[snip]
>
> Overall it looks really good to me, so no problem with applying it,
> with or without my suggestions.
I've applied most of your suggestions and will squash this into the
big patch, thanks as usual!
Bart
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-08 19:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 8:11 [libgpiod v2][PATCH] line-config: rework the internal implementation Bartosz Golaszewski
2021-10-05 4:20 ` Kent Gibson
2021-10-08 19:15 ` Bartosz Golaszewski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.