Add support for bias flags and SET_CONFIG

Extend the libgpiod API to support the bias flags recently added to the kernel GPIO uAPI. The core change is the addition of GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE, GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP and GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN flags to be passed into line_request functions, and the addition of gpiod_line_bias to return the bias state of lines.

Extended variants of the ctxless functions that accept an active_low flag are added to also accept other flags. The variant names add a "_ext" suffix to the name of the original function.

Based on initial work by Drew Fustini <[email protected]>.

Signed-off-by Kent Gibson <[email protected]>

---
 include/gpiod.h | 184 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/core.c      |  32 ++++++---
 lib/ctxless.c   | 115 +++++++++++++++++++++++++++---
 3 files changed, 313 insertions(+), 18 deletions(-)

include/gpiod.h +184 -0

index 9860ea8..0f01cab 100644
@@ -84,6 +84,22 @@ struct gpiod_line_bulk;
  * the need to use the gpiod_* structures or to keep track of resources.
  */
 
+/**
+ * @brief Miscellaneous GPIO flags.
+ */
+enum {
+       GPIOD_CTXLESS_FLAG_OPEN_DRAIN           = GPIOD_BIT(0),
+       /**< The line is an open-drain port. */
+       GPIOD_CTXLESS_FLAG_OPEN_SOURCE          = GPIOD_BIT(1),
+       /**< The line is an open-source port. */
+       GPIOD_CTXLESS_FLAG_BIAS_DISABLE         = GPIOD_BIT(2),
+       /**< The line has neither either pull-up nor pull-down resistor */
+       GPIOD_CTXLESS_FLAG_BIAS_PULL_DOWN       = GPIOD_BIT(3),
+       /**< The line has pull-down resistor enabled */
+       GPIOD_CTXLESS_FLAG_BIAS_PULL_UP         = GPIOD_BIT(4),
+       /**< The line has pull-up resistor enabled */
+};
+
 /**
  * @brief Read current value from a single GPIO line.
  * @param device Name, path, number or label of the gpiochip.

@@ -95,6 +111,19 @@ struct gpiod_line_bulk;
 int gpiod_ctxless_get_value(const char *device, unsigned int offset,
                            bool active_low, const char *consumer) GPIOD_API;
 
+/**
+ * @brief Read current value from a single GPIO line.
+ * @param device Name, path, number or label of the gpiochip.
+ * @param offset Offset of the GPIO line.
+ * @param active_low The active state of this line - true if low.
+ * @param flags The flags for the line.
+ * @param consumer Name of the consumer.
+ * @return 0 or 1 (GPIO value) if the operation succeeds, -1 on error.
+ */
+int gpiod_ctxless_get_value_ext(const char *device, unsigned int offset,
+                               bool active_low, int flags,
+                               const char *consumer) GPIOD_API;
+
 /**
  * @brief Read current values from a set of GPIO lines.
  * @param device Name, path, number or label of the gpiochip.

@@ -110,6 +139,24 @@ int gpiod_ctxless_get_value_multiple(const char *device,
                                     unsigned int num_lines, bool active_low,
                                     const char *consumer) GPIOD_API;
 
+/**
+ * @brief Read current values from a set of GPIO lines.
+ * @param device Name, path, number or label of the gpiochip.
+ * @param offsets Array of offsets of lines whose values should be read.
+ * @param values Buffer in which the values will be stored.
+ * @param num_lines Number of lines, must be > 0.
+ * @param active_low The active state of this line - true if low.
+ * @param flags The flags for the lines.
+ * @param consumer Name of the consumer.
+ * @return 0 if the operation succeeds, -1 on error.
+ */
+int gpiod_ctxless_get_value_multiple_ext(const char *device,
+                                        const unsigned int *offsets,
+                                        int *values,
+                                        unsigned int num_lines,
+                                        bool active_low, int flags,
+                                        const char *consumer) GPIOD_API;
+
 /**
  * @brief Simple set value callback signature.
  */

@@ -133,6 +180,26 @@ int gpiod_ctxless_set_value(const char *device, unsigned int offset, int value,
                            gpiod_ctxless_set_value_cb cb,
                            void *data) GPIOD_API;
 
+/**
+ * @brief Set value of a single GPIO line.
+ * @param device Name, path, number or label of the gpiochip.
+ * @param offset The offset of the GPIO line.
+ * @param value New value (0 or 1).
+ * @param active_low The active state of this line - true if low.
+ * @param flags The flags for the line.
+ * @param consumer Name of the consumer.
+ * @param cb Optional callback function that will be called right after setting
+ *           the value. Users can use this, for example, to pause the execution
+ *           after toggling a GPIO.
+ * @param data Optional user data that will be passed to the callback function.
+ * @return 0 if the operation succeeds, -1 on error.
+ */
+int gpiod_ctxless_set_value_ext(const char *device, unsigned int offset,
+                               int value, bool active_low, int flags,
+                               const char *consumer,
+                               gpiod_ctxless_set_value_cb cb,
+                               void *data) GPIOD_API;
+
 /**
  * @brief Set values of multiple GPIO lines.
  * @param device Name, path, number or label of the gpiochip.

@@ -153,6 +220,29 @@ int gpiod_ctxless_set_value_multiple(const char *device,
                                     gpiod_ctxless_set_value_cb cb,
                                     void *data) GPIOD_API;
 
+/**
+ * @brief Set values of multiple GPIO lines.
+ * @param device Name, path, number or label of the gpiochip.
+ * @param offsets Array of offsets of lines the values of which should be set.
+ * @param values Array of integers containing new values.
+ * @param num_lines Number of lines, must be > 0.
+ * @param active_low The active state of this line - true if low.
+ * @param flags The flags for the lines.
+ * @param consumer Name of the consumer.
+ * @param cb Optional callback function that will be called right after setting
+ *           all values. Works the same as in ::gpiod_ctxless_set_value.
+ * @param data Optional user data that will be passed to the callback function.
+ * @return 0 if the operation succeeds, -1 on error.
+ */
+int gpiod_ctxless_set_value_multiple_ext(const char *device,
+                                        const unsigned int *offsets,
+                                        const int *values,
+                                        unsigned int num_lines,
+                                        bool active_low, int flags,
+                                        const char *consumer,
+                                        gpiod_ctxless_set_value_cb cb,
+                                        void *data) GPIOD_API;
+
 /**
  * @brief Event types that the ctxless event monitor can wait for.
  */

@@ -327,6 +417,31 @@ int gpiod_ctxless_event_monitor(const char *device, int event_type,
                                gpiod_ctxless_event_handle_cb event_cb,
                                void *data) GPIOD_API;
 
+/**
+ * @brief Wait for events on a single GPIO line.
+ * @param device Name, path, number or label of the gpiochip.
+ * @param event_type Type of events to listen for.
+ * @param offset GPIO line offset to monitor.
+ * @param active_low The active state of this line - true if low.
+ * @param flags The flags for the line.
+ * @param consumer Name of the consumer.
+ * @param timeout Maximum wait time for each iteration.
+ * @param poll_cb Callback function to call when waiting for events.
+ * @param event_cb Callback function to call for each line event.
+ * @param data User data passed to the callback.
+ * @return 0 if no errors were encountered, -1 if an error occurred.
+ * @note The way the ctxless event loop works is described in detail in
+ *       ::gpiod_ctxless_event_monitor_multiple - this is just a wrapper aound
+ *       this routine which calls it for a single GPIO line.
+ */
+int gpiod_ctxless_event_monitor_ext(const char *device, int event_type,
+                                   unsigned int offset, bool active_low,
+                                   int flags, const char *consumer,
+                                   const struct timespec *timeout,
+                                   gpiod_ctxless_event_poll_cb poll_cb,
+                                   gpiod_ctxless_event_handle_cb event_cb,
+                                   void *data) GPIOD_API;
+
 /**
  * @brief Wait for events on multiple GPIO lines.
  * @param device Name, path, number or label of the gpiochip.

@@ -366,6 +481,47 @@ int gpiod_ctxless_event_monitor_multiple(
                        gpiod_ctxless_event_handle_cb event_cb,
                        void *data) GPIOD_API;
 
+/**
+ * @brief Wait for events on multiple GPIO lines.
+ * @param device Name, path, number or label of the gpiochip.
+ * @param event_type Type of events to listen for.
+ * @param offsets Array of GPIO line offsets to monitor.
+ * @param num_lines Number of lines to monitor.
+ * @param active_low The active state of this line - true if low.
+ * @param flags The flags for the lines.
+ * @param consumer Name of the consumer.
+ * @param timeout Maximum wait time for each iteration.
+ * @param poll_cb Callback function to call when waiting for events. Can
+ *                be NULL.
+ * @param event_cb Callback function to call on event occurrence.
+ * @param data User data passed to the callback.
+ * @return 0 no errors were encountered, -1 if an error occurred.
+ * @note The poll callback can be NULL in which case the routine will fall
+ *       back to a basic, ppoll() based callback.
+ *
+ * Internally this routine opens the GPIO chip, requests the set of lines for
+ * the type of events specified in the event_type parameter and calls the
+ * polling callback in a loop. The role of the polling callback is to detect
+ * input events on a set of file descriptors and notify the caller about the
+ * fds ready for reading.
+ *
+ * The ctxless event loop then reads each queued event from marked descriptors
+ * and calls the event callback. Both callbacks can stop the loop at any
+ * point.
+ *
+ * The poll_cb argument can be NULL in which case the function falls back to
+ * a default, ppoll() based callback.
+ */
+int gpiod_ctxless_event_monitor_multiple_ext(
+                       const char *device, int event_type,
+                       const unsigned int *offsets,
+                       unsigned int num_lines, bool active_low, int flags,
+                       const char *consumer, const struct timespec *timeout,
+                       gpiod_ctxless_event_poll_cb poll_cb,
+                       gpiod_ctxless_event_handle_cb event_cb,
+                       void *data) GPIOD_API;
+
+
 /**
  * @brief Determine the chip name and line offset of a line with given name.
  * @param name The name of the GPIO line to lookup.

@@ -658,6 +814,20 @@ enum {
        /**< The active state of a GPIO is active-low. */
 };
 
+/**
+ * @brief Possible internal bias settings.
+ */
+enum {
+       GPIOD_LINE_BIAS_AS_IS = 1,
+       /**< The internal bias state is unknown. */
+       GPIOD_LINE_BIAS_DISABLE,
+       /**< The internal bias is disabled. */
+       GPIOD_LINE_BIAS_PULL_UP,
+       /**< The internal pull-up bias is enabled. */
+       GPIOD_LINE_BIAS_PULL_DOWN,
+       /**< The internal pull-down bias is enabled. */
+};
+
 /**
  * @brief Read the GPIO line offset.
  * @param line GPIO line object.

@@ -697,6 +867,14 @@ int gpiod_line_direction(struct gpiod_line *line) GPIOD_API;
  */
 int gpiod_line_active_state(struct gpiod_line *line) GPIOD_API;
 
+/**
+ * @brief Read the GPIO line bias setting.
+ * @param line GPIO line object.
+ * @return Returns GPIOD_LINE_BIAS_PULL_UP, GPIOD_LINE_BIAS_PULL_DOWN,
+ *         GPIOD_LINE_BIAS_DISABLE or GPIOD_LINE_BIAS_AS_IS.
+ */
+int gpiod_line_bias(struct gpiod_line *line) GPIOD_API;
+
 /**
  * @brief Check if the line is currently in use.
  * @param line GPIO line object.

@@ -792,6 +970,12 @@ enum {
        /**< The line is an open-source port. */
        GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW      = GPIOD_BIT(2),
        /**< The active state of the line is low (high is the default). */
+       GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE    = GPIOD_BIT(3),
+       /**< The line has neither either pull-up nor pull-down resistor */
+       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN  = GPIOD_BIT(4),
+       /**< The line has pull-down resistor enabled */
+       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP    = GPIOD_BIT(5),
+       /**< The line has pull-up resistor enabled */
 };
 
 /**

lib/core.c +23 -9

index f05e595..9b7d88f 100644
@@ -36,9 +36,7 @@ struct gpiod_line {
        unsigned int offset;
        int direction;
        int active_state;
-       bool used;
-       bool open_source;
-       bool open_drain;
+       __u32 flags;
 
        int state;
        bool needs_update;

@@ -359,19 +357,31 @@ int gpiod_line_active_state(struct gpiod_line *line)
        return line->active_state;
 }
 
+int gpiod_line_bias(struct gpiod_line *line)
+{
+       if (line->flags & GPIOLINE_FLAG_BIAS_DISABLE)
+               return GPIOD_LINE_BIAS_DISABLE;
+       if (line->flags & GPIOLINE_FLAG_BIAS_PULL_UP)
+               return GPIOD_LINE_BIAS_PULL_UP;
+       if (line->flags & GPIOLINE_FLAG_BIAS_PULL_DOWN)
+               return GPIOD_LINE_BIAS_PULL_DOWN;
+
+       return GPIOD_LINE_BIAS_AS_IS;
+}
+
 bool gpiod_line_is_used(struct gpiod_line *line)
 {
-       return line->used;
+       return line->flags & GPIOLINE_FLAG_KERNEL;
 }
 
 bool gpiod_line_is_open_drain(struct gpiod_line *line)
 {
-       return line->open_drain;
+       return line->flags & GPIOLINE_FLAG_OPEN_DRAIN;
 }
 
 bool gpiod_line_is_open_source(struct gpiod_line *line)
 {
-       return line->open_source;
+       return line->flags & GPIOLINE_FLAG_OPEN_SOURCE;
 }
 
 bool gpiod_line_needs_update(struct gpiod_line *line)

@@ -398,9 +408,7 @@ int gpiod_line_update(struct gpiod_line *line)
                                                ? GPIOD_LINE_ACTIVE_STATE_LOW
                                                : GPIOD_LINE_ACTIVE_STATE_HIGH;
 
-       line->used = info.flags & GPIOLINE_FLAG_KERNEL;
-       line->open_drain = info.flags & GPIOLINE_FLAG_OPEN_DRAIN;
-       line->open_source = info.flags & GPIOLINE_FLAG_OPEN_SOURCE;
+       line->flags = info.flags;
 
        strncpy(line->name, info.name, sizeof(line->name));
        strncpy(line->consumer, info.consumer, sizeof(line->consumer));

@@ -473,6 +481,12 @@ static __u32 line_request_flag_to_gpio_handleflag(int flags)
                hflags |= GPIOHANDLE_REQUEST_OPEN_SOURCE;
        if (flags & GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW)
                hflags |= GPIOHANDLE_REQUEST_ACTIVE_LOW;
+       if (flags & GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE)
+               hflags |= GPIOHANDLE_REQUEST_BIAS_DISABLE;
+       if (flags & GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN)
+               hflags |= GPIOHANDLE_REQUEST_BIAS_PULL_DOWN;
+       if (flags & GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP)
+               hflags |= GPIOHANDLE_REQUEST_BIAS_PULL_UP;
 
        return hflags;
 }

lib/ctxless.c +106 -10

index ba85018..44f1872 100644
@@ -14,6 +14,26 @@
 #include <stdio.h>
 #include <string.h>
 
+static int ctxless_flags_to_line_request_flags(bool active_low, int flags)
+{
+       int lflags = 0;
+
+       if (active_low)
+               lflags |= GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW;
+       if (flags & GPIOD_CTXLESS_FLAG_OPEN_DRAIN)
+               lflags |= GPIOD_LINE_REQUEST_FLAG_OPEN_DRAIN;
+       if (flags & GPIOD_CTXLESS_FLAG_OPEN_SOURCE)
+               lflags |= GPIOD_LINE_REQUEST_FLAG_OPEN_SOURCE;
+       if (flags & GPIOD_CTXLESS_FLAG_BIAS_DISABLE)
+               lflags |= GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE;
+       if (flags & GPIOD_CTXLESS_FLAG_BIAS_PULL_UP)
+               lflags |= GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP;
+       if (flags & GPIOD_CTXLESS_FLAG_BIAS_PULL_DOWN)
+               lflags |= GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN;
+
+       return lflags;
+}
+
 int gpiod_ctxless_get_value(const char *device, unsigned int offset,
                            bool active_low, const char *consumer)
 {

@@ -27,16 +47,45 @@ int gpiod_ctxless_get_value(const char *device, unsigned int offset,
        return value;
 }
 
+int gpiod_ctxless_get_value_ext(const char *device, unsigned int offset,
+                               bool active_low, int flags,
+                               const char *consumer)
+{
+       int value, rv;
+
+       rv = gpiod_ctxless_get_value_multiple_ext(device, &offset, &value, 1,
+                                                 active_low, flags, consumer);
+       if (rv < 0)
+               return rv;
+
+       return value;
+}
+
 int gpiod_ctxless_get_value_multiple(const char *device,
                                     const unsigned int *offsets, int *values,
                                     unsigned int num_lines, bool active_low,
                                     const char *consumer)
+{
+       int rv;
+
+       rv = gpiod_ctxless_get_value_multiple_ext(device, offsets, values,
+                                                 num_lines, active_low, 0,
+                                                 consumer);
+       return rv;
+}
+
+int gpiod_ctxless_get_value_multiple_ext(const char *device,
+                                        const unsigned int *offsets,
+                                        int *values,
+                                        unsigned int num_lines,
+                                        bool active_low, int flags,
+                                        const char *consumer)
 {
        struct gpiod_line_bulk bulk;
        struct gpiod_chip *chip;
        struct gpiod_line *line;
        unsigned int i;
-       int rv, flags;
+       int rv, lflags;
 
        if (!num_lines || num_lines > GPIOD_LINE_BULK_MAX_LINES) {
                errno = EINVAL;

@@ -59,9 +108,8 @@ int gpiod_ctxless_get_value_multiple(const char *device,
                gpiod_line_bulk_add(&bulk, line);
        }
 
-       flags = active_low ? GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW : 0;
-
-       rv = gpiod_line_request_bulk_input_flags(&bulk, consumer, flags);
+       lflags = ctxless_flags_to_line_request_flags(active_low, flags);
+       rv = gpiod_line_request_bulk_input_flags(&bulk, consumer, lflags);
        if (rv < 0) {
                gpiod_chip_close(chip);
                return -1;

@@ -83,17 +131,39 @@ int gpiod_ctxless_set_value(const char *device, unsigned int offset, int value,
                                                active_low, consumer, cb, data);
 }
 
+int gpiod_ctxless_set_value_ext(const char *device, unsigned int offset,
+                               int value, bool active_low, int flags,
+                               const char *consumer,
+                               gpiod_ctxless_set_value_cb cb, void *data)
+{
+       return gpiod_ctxless_set_value_multiple_ext(device, &offset, &value,
+                                                   1, active_low, flags,
+                                                   consumer, cb, data);
+}
+
 int gpiod_ctxless_set_value_multiple(const char *device,
                                     const unsigned int *offsets,
                                     const int *values, unsigned int num_lines,
                                     bool active_low, const char *consumer,
                                     gpiod_ctxless_set_value_cb cb, void *data)
+{
+       return gpiod_ctxless_set_value_multiple_ext(device, offsets, values,
+                                                   num_lines, active_low,
+                                                   0, consumer, cb, data);
+}
+
+int gpiod_ctxless_set_value_multiple_ext(
+                       const char *device,
+                       const unsigned int *offsets,
+                       const int *values, unsigned int num_lines,
+                       bool active_low, int flags, const char *consumer,
+                       gpiod_ctxless_set_value_cb cb, void *data)
 {
        struct gpiod_line_bulk bulk;
        struct gpiod_chip *chip;
        struct gpiod_line *line;
        unsigned int i;
-       int rv, flags;
+       int rv, lflags;
 
        if (!num_lines || num_lines > GPIOD_LINE_BULK_MAX_LINES) {
                errno = EINVAL;

@@ -116,10 +186,9 @@ int gpiod_ctxless_set_value_multiple(const char *device,
                gpiod_line_bulk_add(&bulk, line);
        }
 
-       flags = active_low ? GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW : 0;
-
+       lflags = ctxless_flags_to_line_request_flags(active_low, flags);
        rv = gpiod_line_request_bulk_output_flags(&bulk, consumer,
-                                                 flags, values);
+                                                 lflags, values);
        if (rv < 0) {
                gpiod_chip_close(chip);
                return -1;

@@ -216,6 +285,19 @@ int gpiod_ctxless_event_monitor(const char *device, int event_type,
                                                    poll_cb, event_cb, data);
 }
 
+int gpiod_ctxless_event_monitor_ext(const char *device, int event_type,
+                                   unsigned int offset, bool active_low,
+                                   int flags, const char *consumer,
+                                   const struct timespec *timeout,
+                                   gpiod_ctxless_event_poll_cb poll_cb,
+                                   gpiod_ctxless_event_handle_cb event_cb,
+                                   void *data)
+{
+       return gpiod_ctxless_event_monitor_multiple_ext(
+                       device, event_type, &offset, 1, active_low,
+                       flags, consumer, timeout, poll_cb, event_cb, data);
+}
+
 int gpiod_ctxless_event_monitor_multiple(
                        const char *device, int event_type,
                        const unsigned int *offsets,

@@ -225,6 +307,21 @@ int gpiod_ctxless_event_monitor_multiple(
                        gpiod_ctxless_event_poll_cb poll_cb,
                        gpiod_ctxless_event_handle_cb event_cb,
                        void *data)
+{
+       return gpiod_ctxless_event_monitor_multiple_ext(
+                       device, event_type, offsets,
+                       num_lines, active_low, 0, consumer, timeout,
+                       poll_cb, event_cb, data);
+}
+
+int gpiod_ctxless_event_monitor_multiple_ext(
+                       const char *device, int event_type,
+                       const unsigned int *offsets,
+                       unsigned int num_lines, bool active_low, int flags,
+                       const char *consumer, const struct timespec *timeout,
+                       gpiod_ctxless_event_poll_cb poll_cb,
+                       gpiod_ctxless_event_handle_cb event_cb,
+                       void *data)
 {
        struct gpiod_ctxless_event_poll_fd fds[GPIOD_LINE_BULK_MAX_LINES];
        struct gpiod_line_request_config conf;

@@ -259,7 +356,7 @@ int gpiod_ctxless_event_monitor_multiple(
                gpiod_line_bulk_add(&bulk, line);
        }
 
-       conf.flags = active_low ? GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW : 0;
+       conf.flags = ctxless_flags_to_line_request_flags(active_low, flags);
        conf.consumer = consumer;
 
        if (event_type == GPIOD_CTXLESS_EVENT_RISING_EDGE) {
--

2.24.0

pt., 15 lis 2019 o 15:44 Kent Gibson <[email protected]> napisał(a):

Quoted 8 lines

Extend the libgpiod API to support the bias flags recently added to the kernel GPIO uAPI. The core change is the addition of GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE, GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP and GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN flags to be passed into line_request functions, and the addition of gpiod_line_bias to return the bias state of lines.

Quoted 7 lines

Extended variants of the ctxless functions that accept an active_low flag are added to also accept other flags. The variant names add a "_ext" suffix to the name of the original function.

Based on initial work by Drew Fustini <[email protected]>.

Signed-off-by Kent Gibson <[email protected]>

Hi Kent,

this looks good, just a couple nits. I'd change the subject line to "core: add support for bias flags". Also: how about moving the flags argument to the last position in all the ctxless extended variants, so that they keep the same signature *except* for the appended new argument?

Extend test coverage over the bias flags, gpiod_line_bias and the extended ctxless functions.

Also update existing tests to check bias flags where line state is checked.

Signed-off-by Kent Gibson <[email protected]>

---
 tests/tests-ctxless.c |  64 +++++++++++++++++++++-
 tests/tests-event.c   | 120 ++++++++++++++++++++++++++++++++++++++++++
 tests/tests-line.c    |  98 ++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+), 2 deletions(-)

tests/tests-ctxless.c +36 -2

index c1e1ca6..da70587 100644
@@ -26,11 +26,41 @@ GPIOD_TEST_CASE(get_value, 0, { 8 })
        g_assert_cmpint(ret, ==, 1);
 }
 
-static void set_value_check(gpointer data G_GNUC_UNUSED)
+GPIOD_TEST_CASE(get_value_ext, 0, { 8 })
+{
+       gint ret;
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               false, GPIOD_CTXLESS_FLAG_BIAS_PULL_DOWN,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 0);
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               false, GPIOD_CTXLESS_FLAG_BIAS_PULL_UP,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               true, GPIOD_CTXLESS_FLAG_BIAS_PULL_DOWN,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               true, GPIOD_CTXLESS_FLAG_BIAS_PULL_UP,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 0);
+}
+
+static void set_value_check_hi(gpointer data G_GNUC_UNUSED)
 {
        g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 1);
 }
 
+static void set_value_check_lo(gpointer data G_GNUC_UNUSED)
+{
+       g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 0);
+}
+
 GPIOD_TEST_CASE(set_value, 0, { 8 })
 {
        gint ret;

@@ -39,13 +69,43 @@ GPIOD_TEST_CASE(set_value, 0, { 8 })
 
        ret = gpiod_ctxless_set_value(gpiod_test_chip_name(0), 3, 1,
                                      false, GPIOD_TEST_CONSUMER,
-                                     set_value_check, NULL);
+                                     set_value_check_hi, NULL);
        gpiod_test_return_if_failed();
        g_assert_cmpint(ret, ==, 0);
 
        g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 0);
 }
 
+GPIOD_TEST_CASE(set_value_ext, 0, { 8 })
+{
+       gint ret;
+

tests/tests-ctxless.c +16 -0

+       ret = gpiod_ctxless_set_value_ext(gpiod_test_chip_name(0), 3, 1,
+                       false, GPIOD_CTXLESS_FLAG_OPEN_DRAIN,
+                       GPIOD_TEST_CONSUMER, set_value_check_lo, NULL);
+       gpiod_test_return_if_failed();
+       g_assert_cmpint(ret, ==, 0);
+       g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 0);
+
+       gpiod_test_chip_set_pull(0, 3, 1);
+       ret = gpiod_ctxless_set_value_ext(gpiod_test_chip_name(0), 3, 0,
+                       false, GPIOD_CTXLESS_FLAG_OPEN_SOURCE,
+                       GPIOD_TEST_CONSUMER, set_value_check_hi, NULL);
+       gpiod_test_return_if_failed();
+       g_assert_cmpint(ret, ==, 0);
+       g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 1);
+}
+
 static const guint get_value_multiple_offsets[] = {
        1, 3, 4, 5, 6, 7, 8, 9, 13, 14
 };

tests/tests-event.c +120 -0

index 28b77ec..d425d1a 100644
@@ -196,6 +196,126 @@ GPIOD_TEST_CASE(both_edges_active_low, 0, { 8 })
        g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
 }
 
+GPIOD_TEST_CASE(both_edges_bias_disable, 0, { 8 })
+{
+       g_autoptr(GpiodTestEventThread) ev_thread = NULL;
+       g_autoptr(gpiod_chip_struct) chip = NULL;
+       struct timespec ts = { 1, 0 };
+       struct gpiod_line_event ev;
+       struct gpiod_line *line;
+       gint ret;
+
+       chip = gpiod_chip_open(gpiod_test_chip_path(0));
+       g_assert_nonnull(chip);
+       gpiod_test_return_if_failed();
+
+       line = gpiod_chip_get_line(chip, 7);
+       g_assert_nonnull(line);
+       gpiod_test_return_if_failed();
+
+       ret = gpiod_line_request_both_edges_events_flags(line,
+               GPIOD_TEST_CONSUMER, GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE);
+       g_assert_cmpint(ret, ==, 0);
+
+       ev_thread = gpiod_test_start_event_thread(0, 7, 100);
+
+       ret = gpiod_line_event_wait(line, &ts);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_line_event_read(line, &ev);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
+
+       ret = gpiod_line_event_wait(line, &ts);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_line_event_read(line, &ev);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_FALLING_EDGE);
+}
+
+GPIOD_TEST_CASE(both_edges_bias_pull_down, 0, { 8 })
+{
+       g_autoptr(GpiodTestEventThread) ev_thread = NULL;
+       g_autoptr(gpiod_chip_struct) chip = NULL;
+       struct timespec ts = { 1, 0 };
+       struct gpiod_line_event ev;
+       struct gpiod_line *line;
+       gint ret;
+
+       chip = gpiod_chip_open(gpiod_test_chip_path(0));
+       g_assert_nonnull(chip);
+       gpiod_test_return_if_failed();
+
+       line = gpiod_chip_get_line(chip, 7);
+       g_assert_nonnull(line);
+       gpiod_test_return_if_failed();
+
+       ret = gpiod_line_request_both_edges_events_flags(line,
+               GPIOD_TEST_CONSUMER, GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN);
+       g_assert_cmpint(ret, ==, 0);
+
+       ev_thread = gpiod_test_start_event_thread(0, 7, 100);
+
+       ret = gpiod_line_event_wait(line, &ts);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_line_event_read(line, &ev);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
+
+       ret = gpiod_line_event_wait(line, &ts);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_line_event_read(line, &ev);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_FALLING_EDGE);
+}
+
+GPIOD_TEST_CASE(both_edges_bias_pull_up, 0, { 8 })
+{
+       g_autoptr(GpiodTestEventThread) ev_thread = NULL;
+       g_autoptr(gpiod_chip_struct) chip = NULL;
+       struct timespec ts = { 1, 0 };
+       struct gpiod_line_event ev;
+       struct gpiod_line *line;
+       gint ret;
+
+       chip = gpiod_chip_open(gpiod_test_chip_path(0));
+       g_assert_nonnull(chip);
+       gpiod_test_return_if_failed();
+
+       line = gpiod_chip_get_line(chip, 7);
+       g_assert_nonnull(line);
+       gpiod_test_return_if_failed();
+
+       ret = gpiod_line_request_both_edges_events_flags(line,
+               GPIOD_TEST_CONSUMER, GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP);
+       g_assert_cmpint(ret, ==, 0);
+
+       ev_thread = gpiod_test_start_event_thread(0, 7, 100);
+
+       ret = gpiod_line_event_wait(line, &ts);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_line_event_read(line, &ev);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_FALLING_EDGE);
+
+       ret = gpiod_line_event_wait(line, &ts);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_line_event_read(line, &ev);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
+}
+
 GPIOD_TEST_CASE(falling_edge_active_low, 0, { 8 })
 {
        g_autoptr(GpiodTestEventThread) ev_thread = NULL;

tests/tests-line.c +98 -1

index 4792211..b4bef1a 100644
@@ -451,6 +451,7 @@ GPIOD_TEST_CASE(misc_flags, 0, { 8 })
        g_assert_false(gpiod_line_is_used(line));
        g_assert_false(gpiod_line_is_open_drain(line));
        g_assert_false(gpiod_line_is_open_source(line));
+       g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
 
        config.request_type = GPIOD_LINE_REQUEST_DIRECTION_OUTPUT;
        config.consumer = GPIOD_TEST_CONSUMER;

@@ -462,6 +463,7 @@ GPIOD_TEST_CASE(misc_flags, 0, { 8 })
        g_assert_true(gpiod_line_is_used(line));
        g_assert_true(gpiod_line_is_open_drain(line));
        g_assert_false(gpiod_line_is_open_source(line));
+       g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
        g_assert_cmpint(gpiod_line_direction(line), ==,
                        GPIOD_LINE_DIRECTION_OUTPUT);
 

@@ -475,8 +477,11 @@ GPIOD_TEST_CASE(misc_flags, 0, { 8 })
        g_assert_true(gpiod_line_is_used(line));
        g_assert_false(gpiod_line_is_open_drain(line));
        g_assert_true(gpiod_line_is_open_source(line));
+       g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
        g_assert_cmpint(gpiod_line_direction(line), ==,
                        GPIOD_LINE_DIRECTION_OUTPUT);
+
+       gpiod_line_release(line);
 }
 
 GPIOD_TEST_CASE(misc_flags_work_together, 0, { 8 })

@@ -510,6 +515,7 @@ GPIOD_TEST_CASE(misc_flags_work_together, 0, { 8 })
        g_assert_true(gpiod_line_is_used(line));
        g_assert_true(gpiod_line_is_open_drain(line));
        g_assert_false(gpiod_line_is_open_source(line));
+       g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
        g_assert_cmpint(gpiod_line_active_state(line), ==,
                        GPIOD_LINE_ACTIVE_STATE_LOW);
        g_assert_cmpint(gpiod_line_direction(line), ==,

@@ -526,8 +532,59 @@ GPIOD_TEST_CASE(misc_flags_work_together, 0, { 8 })
        g_assert_true(gpiod_line_is_used(line));
        g_assert_false(gpiod_line_is_open_drain(line));
        g_assert_true(gpiod_line_is_open_source(line));
+       g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
        g_assert_cmpint(gpiod_line_active_state(line), ==,
                        GPIOD_LINE_ACTIVE_STATE_LOW);
+
+       gpiod_line_release(line);
+
+       /*
+        * Verify that pull-up/down flags work together
+        * with active_low.
+        */
+
+       config.request_type = GPIOD_LINE_REQUEST_DIRECTION_INPUT;
+       config.flags = GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN |
+                      GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW;
+
+       ret = gpiod_line_request(line, &config, 0);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_true(gpiod_line_is_used(line));
+       g_assert_false(gpiod_line_is_open_drain(line));
+       g_assert_false(gpiod_line_is_open_source(line));
+       g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_PULL_DOWN);
+       g_assert_cmpint(gpiod_line_active_state(line), ==,
+                       GPIOD_LINE_ACTIVE_STATE_LOW);
+       g_assert_cmpint(gpiod_line_direction(line), ==,
+                       GPIOD_LINE_DIRECTION_INPUT);
+
+       ret = gpiod_line_get_value(line);
+       g_assert_cmpint(ret, ==, 1);
+       g_assert_cmpint(errno, ==, 0);
+
+       gpiod_line_release(line);
+
+       config.flags = GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP |
+                      GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW;
+
+       ret = gpiod_line_request(line, &config, 0);
+       g_assert_cmpint(ret, ==, 0);
+
+       g_assert_true(gpiod_line_is_used(line));
+       g_assert_false(gpiod_line_is_open_drain(line));
+       g_assert_false(gpiod_line_is_open_source(line));
+       g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_PULL_UP);
+       g_assert_cmpint(gpiod_line_active_state(line), ==,
+                       GPIOD_LINE_ACTIVE_STATE_LOW);
+       g_assert_cmpint(gpiod_line_direction(line), ==,
+                       GPIOD_LINE_DIRECTION_INPUT);
+
+       ret = gpiod_line_get_value(line);
+       g_assert_cmpint(ret, ==, 0);
+       g_assert_cmpint(errno, ==, 0);
+
+       gpiod_line_release(line);
 }
 
 GPIOD_TEST_CASE(open_source_open_drain_input_mode, 0, { 8 })

@@ -576,6 +633,47 @@ GPIOD_TEST_CASE(open_source_open_drain_simultaneously, 0, { 8 })
        g_assert_cmpint(errno, ==, EINVAL);
 }
 
+GPIOD_TEST_CASE(multiple_bias_flags, 0, { 8 })
+{
+       g_autoptr(gpiod_chip_struct) chip = NULL;
+       struct gpiod_line *line;
+       gint ret;
+
+       chip = gpiod_chip_open(gpiod_test_chip_path(0));
+       g_assert_nonnull(chip);
+       gpiod_test_return_if_failed();
+
+       line = gpiod_chip_get_line(chip, 2);
+       g_assert_nonnull(line);
+       gpiod_test_return_if_failed();
+
+       ret = gpiod_line_request_input_flags(line, GPIOD_TEST_CONSUMER,
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE |
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN);
+       g_assert_cmpint(ret, ==, -1);
+       g_assert_cmpint(errno, ==, EINVAL);
+
+       ret = gpiod_line_request_input_flags(line, GPIOD_TEST_CONSUMER,
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE |
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP);
+       g_assert_cmpint(ret, ==, -1);
+       g_assert_cmpint(errno, ==, EINVAL);
+
+       ret = gpiod_line_request_input_flags(line, GPIOD_TEST_CONSUMER,
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN |
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP);
+       g_assert_cmpint(ret, ==, -1);
+       g_assert_cmpint(errno, ==, EINVAL);
+
+       ret = gpiod_line_request_input_flags(line, GPIOD_TEST_CONSUMER,
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE |
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN |
+                                       GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP);
+       g_assert_cmpint(ret, ==, -1);
+       g_assert_cmpint(errno, ==, EINVAL);
+}
+
+
 /* Verify that the reference counting of the line fd handle works correctly. */
 GPIOD_TEST_CASE(release_one_use_another, 0, { 8 })
 {
--

2.24.0

pt., 15 lis 2019 o 15:44 Kent Gibson <[email protected]> napisał(a):

Quoted 72 lines

Extend test coverage over the bias flags, gpiod_line_bias and the extended ctxless functions.

Also update existing tests to check bias flags where line state is checked.

Signed-off-by Kent Gibson <[email protected]>

---
 tests/tests-ctxless.c |  64 +++++++++++++++++++++-
 tests/tests-event.c   | 120 ++++++++++++++++++++++++++++++++++++++++++
 tests/tests-line.c    |  98 ++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+), 2 deletions(-)

tests/tests-ctxless.c +32 -2

index c1e1ca6..da70587 100644
@@ -26,11 +26,41 @@ GPIOD_TEST_CASE(get_value, 0, { 8 })
        g_assert_cmpint(ret, ==, 1);
 }
 
-static void set_value_check(gpointer data G_GNUC_UNUSED)
+GPIOD_TEST_CASE(get_value_ext, 0, { 8 })
+{
+       gint ret;
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               false, GPIOD_CTXLESS_FLAG_BIAS_PULL_DOWN,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 0);
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               false, GPIOD_CTXLESS_FLAG_BIAS_PULL_UP,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               true, GPIOD_CTXLESS_FLAG_BIAS_PULL_DOWN,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 1);
+
+       ret = gpiod_ctxless_get_value_ext(gpiod_test_chip_name(0), 3,
+                               true, GPIOD_CTXLESS_FLAG_BIAS_PULL_UP,
+                               GPIOD_TEST_CONSUMER);
+       g_assert_cmpint(ret, ==, 0);
+}
+
+static void set_value_check_hi(gpointer data G_GNUC_UNUSED)
 {
        g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 1);
 }
 
+static void set_value_check_lo(gpointer data G_GNUC_UNUSED)
+{
+       g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 0);
+}
+
 GPIOD_TEST_CASE(set_value, 0, { 8 })
 {
        gint ret;

@@ -39,13 +69,43 @@ GPIOD_TEST_CASE(set_value, 0, { 8 })
 
        ret = gpiod_ctxless_set_value(gpiod_test_chip_name(0), 3, 1,
                                      false, GPIOD_TEST_CONSUMER,
-                                     set_value_check, NULL);
+                                     set_value_check_hi, NULL);
        gpiod_test_return_if_failed();
        g_assert_cmpint(ret, ==, 0);
 
        g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 0);
 }
 

Quoted 4 lines

tests/tests-ctxless.c +4 -0

+GPIOD_TEST_CASE(set_value_ext, 0, { 8 })
+{
+       gint ret;
+

tests/tests-ctxless.c GPIOD_TEST_CASE(set_value, 0, { 8 }) · line 92

Quoted 10 lines

tests/tests-ctxless.c +10 -0

+       gpiod_test_chip_set_pull(0, 3, 0);
+
+       ret = gpiod_ctxless_set_value_ext(gpiod_test_chip_name(0), 3, 1,
+                       false, 0, GPIOD_TEST_CONSUMER,
+                       set_value_check_hi, NULL);
+       gpiod_test_return_if_failed();
+       g_assert_cmpint(ret, ==, 0);
+       g_assert_cmpint(gpiod_test_chip_get_value(0, 3), ==, 0);
+
+       // test drive flags by checking that sets are caught by emulation

Nit: don't use C++ comments in libgpiod.

Initialising bitsets with string is inefficient and confusing. Initialise them with the corresponding int instead.

Signed-off-by Kent Gibson <[email protected]>

---
 bindings/cxx/line_bulk.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

bindings/cxx/line_bulk.cpp +3 -4

index 8369930..c708c8b 100644
@@ -11,9 +11,9 @@
 
 namespace gpiod {
 
-const ::std::bitset<32> line_request::FLAG_ACTIVE_LOW("001");
-const ::std::bitset<32> line_request::FLAG_OPEN_SOURCE("010");
-const ::std::bitset<32> line_request::FLAG_OPEN_DRAIN("100");
+const ::std::bitset<32> line_request::FLAG_ACTIVE_LOW(GPIOD_BIT(0));
+const ::std::bitset<32> line_request::FLAG_OPEN_SOURCE(GPIOD_BIT(1));
+const ::std::bitset<32> line_request::FLAG_OPEN_DRAIN(GPIOD_BIT(2));
 
 namespace {
 
--

2.24.0

pt., 15 lis 2019 o 15:44 Kent Gibson <[email protected]> napisał(a):

Quoted 4 lines

Initialising bitsets with string is inefficient and confusing. Initialise them with the corresponding int instead.

I'm not a native English speaker, of course, so correcting you on spelling may be risky but I generally try to stick to American spelling so "initialise" looks somehow wrong to me. :) Anyway, this is just the commit message, so never mind that.

Extend the libgpiod API to support the setting line configuration using the GPIO GPIOHANDLE_SET_CONFIG_IOCTL uAPI ioctl.

The core change is the addition of gpiod_line_set_config, which provides a low level wrapper around the ioctl.

Additionally, higher level helper functions, gpiod_line_set_flags, gpiod_line_set_direction_input, and gpiod_line_set_direction_output provide slightly simplified APIs for common use cases.

Bulk forms of all functions are also provided.

Signed-off-by Kent Gibson <[email protected]>

---
 include/gpiod.h | 115 ++++++++++++++++++++++++++++
 lib/core.c      | 196 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 300 insertions(+), 11 deletions(-)

include/gpiod.h +0 -0

index 159d745..4053fd2 100644

include/gpiod.h +76 -0

+ */
+bool gpiod_line_is_requested_values(struct gpiod_line *line) GPIOD_API;
+
 /**
  * @brief Check if the calling user has neither requested ownership of this
  *        line nor configured any event notifications.

@@ -1311,6 +1319,113 @@ int gpiod_line_set_value(struct gpiod_line *line, int value) GPIOD_API;
 int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk,
                              const int *values) GPIOD_API;
 
+/**
+ * @}
+ *
+ * @defgroup __line_config__ Setting line configuration
+ * @{
+ */
+
+/**
+ * @brief Update the configuration of a single GPIO line.
+ * @param line GPIO line object.
+ * @param direction Updated direction which may be one of
+ * GPIOD_LINE_REQUEST_DIRECTION_AS_IS, GPIOD_LINE_REQUEST_DIRECTION_INPUT,
+ * or GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @param flags Replacement flags.
+ * @param value The new output value for the line when direction is
+ * GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_config(struct gpiod_line *line, int direction,
+                         int flags, int value) GPIOD_API;
+
+/**
+ * @brief Update the configuration of a set of GPIO lines.
+ * @param bulk Set of GPIO lines.
+ * @param direction Updated direction which may be one of
+ * GPIOD_LINE_REQUEST_DIRECTION_AS_IS, GPIOD_LINE_REQUEST_DIRECTION_INPUT,
+ * or GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @param flags Replacement flags.
+ * @param values An array holding line_bulk->num_lines new logical values
+ * for lines when direction is GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ *
+ * If the lines were not previously requested together, the behavior is
+ * undefined.
+ */
+int gpiod_line_set_config_bulk(struct gpiod_line_bulk *bulk,
+                              int direction, int flags,
+                              const int *values) GPIOD_API;
+
+
+/**
+ * @brief Update the configuration flags of a single GPIO line.
+ * @param line GPIO line object.
+ * @param flags Replacement flags.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_flags(struct gpiod_line *line, int flags) GPIOD_API;
+
+/**
+ * @brief Update the configuration flags of a set of GPIO lines.
+ * @param bulk Set of GPIO lines.
+ * @param flags Replacement flags.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ *
+ * If the lines were not previously requested together, the behavior is
+ * undefined.
+ */
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk,
+                             int flags) GPIOD_API;
+
+/**
+ * @brief Set the direction of a single GPIO line to input.
+ * @param line GPIO line object.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_direction_input(struct gpiod_line *line) GPIOD_API;
+
+/**

include/gpiod.h +24 -0

+
+/**
+ * @brief Set the direction of a single GPIO line to output.
+ * @param line GPIO line object.
+ * @param value The logical value output on the line.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_direction_output(struct gpiod_line *line,
+                                   int value) GPIOD_API;
+
+/**
+ * @brief Set the direction of a set of GPIO lines to output.
+ * @param bulk Set of GPIO lines.
+ * @values The logical value to output for each line.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ *
+ * If the lines were not previously requested together, the behavior is
+ * undefined.
+ */
+int gpiod_line_set_direction_bulk_output(struct gpiod_line_bulk *bulk,
+                                        const int *values) GPIOD_API;
+
 /**
  * @}
  *

lib/core.c +0 -0

index 9b7d88f..c42cda5 100644

lib/core.c +124 -10

        int state;
        bool needs_update;
+       bool as_is;
 
        struct gpiod_chip *chip;
        struct line_fd_handle *fd_handle;

@@ -359,11 +362,11 @@ int gpiod_line_active_state(struct gpiod_line *line)
 
 int gpiod_line_bias(struct gpiod_line *line)
 {
-       if (line->flags & GPIOLINE_FLAG_BIAS_DISABLE)
+       if (line->lflags & GPIOLINE_FLAG_BIAS_DISABLE)
                return GPIOD_LINE_BIAS_DISABLE;
-       if (line->flags & GPIOLINE_FLAG_BIAS_PULL_UP)
+       if (line->lflags & GPIOLINE_FLAG_BIAS_PULL_UP)
                return GPIOD_LINE_BIAS_PULL_UP;
-       if (line->flags & GPIOLINE_FLAG_BIAS_PULL_DOWN)
+       if (line->lflags & GPIOLINE_FLAG_BIAS_PULL_DOWN)
                return GPIOD_LINE_BIAS_PULL_DOWN;
 
        return GPIOD_LINE_BIAS_AS_IS;

@@ -371,17 +374,17 @@ int gpiod_line_bias(struct gpiod_line *line)
 
 bool gpiod_line_is_used(struct gpiod_line *line)
 {
-       return line->flags & GPIOLINE_FLAG_KERNEL;
+       return line->lflags & GPIOLINE_FLAG_KERNEL;
 }
 
 bool gpiod_line_is_open_drain(struct gpiod_line *line)
 {
-       return line->flags & GPIOLINE_FLAG_OPEN_DRAIN;
+       return line->lflags & GPIOLINE_FLAG_OPEN_DRAIN;
 }
 
 bool gpiod_line_is_open_source(struct gpiod_line *line)
 {
-       return line->flags & GPIOLINE_FLAG_OPEN_SOURCE;
+       return line->lflags & GPIOLINE_FLAG_OPEN_SOURCE;
 }
 
 bool gpiod_line_needs_update(struct gpiod_line *line)

@@ -408,7 +411,7 @@ int gpiod_line_update(struct gpiod_line *line)
                                                ? GPIOD_LINE_ACTIVE_STATE_LOW
                                                : GPIOD_LINE_ACTIVE_STATE_HIGH;
 
-       line->flags = info.flags;
+       line->lflags = info.flags;
 
        strncpy(line->name, info.name, sizeof(line->name));
        strncpy(line->consumer, info.consumer, sizeof(line->consumer));

@@ -457,6 +460,20 @@ static bool line_bulk_all_requested(struct gpiod_line_bulk *bulk)
        return true;
 }
 
+static bool line_bulk_all_requested_values(struct gpiod_line_bulk *bulk)
+{
+       struct gpiod_line *line, **lineptr;
+
+       gpiod_line_bulk_foreach_line(bulk, line, lineptr) {
+               if (!gpiod_line_is_requested_values(line)) {
+                       errno = EPERM;
+                       return false;
+               }
+       }
+
+       return true;
+}
+
 static bool line_bulk_all_free(struct gpiod_line_bulk *bulk)
 {
        struct gpiod_line *line, **lineptr;

@@ -471,6 +488,27 @@ static bool line_bulk_all_free(struct gpiod_line_bulk *bulk)
        return true;
 }
 
+static bool line_request_direction_is_valid(int direction)
+{
+       if ((direction == GPIOD_LINE_REQUEST_DIRECTION_AS_IS) ||
+           (direction == GPIOD_LINE_REQUEST_DIRECTION_INPUT) ||
+           (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT))
+               return true;
+
+       errno = EINVAL;
+       return false;
+}
+
+static __u32 line_request_direction_to_gpio_handleflag(int direction)
+{
+       if (direction == GPIOD_LINE_REQUEST_DIRECTION_INPUT)
+               return GPIOHANDLE_REQUEST_INPUT;
+       if (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
+               return GPIOHANDLE_REQUEST_OUTPUT;
+
+       return 0;
+}
+
 static __u32 line_request_flag_to_gpio_handleflag(int flags)
 {
        int hflags = 0;

@@ -495,7 +533,7 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
                               const struct gpiod_line_request_config *config,
                               const int *default_vals)
 {
-       struct gpiod_line *line, **lineptr;
+       struct gpiod_line *line;
        struct line_fd_handle *line_fd;
        struct gpiohandle_request req;
        unsigned int i;

@@ -524,7 +562,6 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
        else if (config->request_type == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
                req.flags |= GPIOHANDLE_REQUEST_OUTPUT;
 
-
        gpiod_line_bulk_foreach_line_off(bulk, line, i) {
                req.lineoffsets[i] = gpiod_line_offset(line);
                if (config->request_type ==

@@ -548,8 +585,14 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
        if (!line_fd)
                return -1;
 
-       gpiod_line_bulk_foreach_line(bulk, line, lineptr) {
+       gpiod_line_bulk_foreach_line_off(bulk, line, i) {
                line->state = LINE_REQUESTED_VALUES;
+               line->cflags = config->flags;
+               if (config->request_type ==
+                       GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
+                       line->output_value = req.default_values[i];
+               if (config->request_type == GPIOD_LINE_REQUEST_DIRECTION_AS_IS)
+                       line->as_is = true;
                line_set_fd(line, line_fd);
                line_maybe_update(line);
        }

@@ -590,6 +633,7 @@ static int line_request_event_single(struct gpiod_line *line,
                return -1;
 
        line->state = LINE_REQUESTED_EVENTS;
+       line->cflags = config->flags;
        line_set_fd(line, line_fd);
        line_maybe_update(line);
 

@@ -688,6 +732,11 @@ bool gpiod_line_is_requested(struct gpiod_line *line)
                line->state == LINE_REQUESTED_EVENTS);
 }
 
+bool gpiod_line_is_requested_values(struct gpiod_line *line)
+{
+       return (line->state == LINE_REQUESTED_VALUES);
+}
+
 bool gpiod_line_is_free(struct gpiod_line *line)
 {
        return line->state == LINE_FREE;

@@ -766,9 +815,134 @@ int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values)
        if (rv < 0)
                return -1;
 
+       gpiod_line_bulk_foreach_line_off(bulk, line, i)
+               line->output_value = data.values[i];
+
+       return 0;
+}
+
+int gpiod_line_set_config(struct gpiod_line *line, int direction,
+                         int flags, int value)
+{
+       struct gpiod_line_bulk bulk;
+
+       gpiod_line_bulk_init(&bulk);
+       gpiod_line_bulk_add(&bulk, line);
+
+       return gpiod_line_set_config_bulk(&bulk, direction, flags, &value);
+}
+
+int gpiod_line_set_config_bulk(struct gpiod_line_bulk *bulk,
+                              int direction, int flags,
+                              const int *values)
+{
+       struct gpiohandle_config hcfg;
+       struct gpiod_line *line;
+       unsigned int i;
+       int rv, fd;
+       bool as_is;
+
+       if (!line_bulk_same_chip(bulk) ||
+           !line_bulk_all_requested_values(bulk))
+               return -1;
+
+       if (!line_request_direction_is_valid(direction))
+               return -1;
+
+       memset(&hcfg, 0, sizeof(hcfg));
+
+       hcfg.flags = line_request_flag_to_gpio_handleflag(flags);
+       hcfg.flags |= line_request_direction_to_gpio_handleflag(direction);
+       if (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT && values) {
+               for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
+                       hcfg.default_values[i] = (uint8_t)!!values[i];
+       }
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       fd = line_get_fd(line);
+
+       rv = ioctl(fd, GPIOHANDLE_SET_CONFIG_IOCTL, &hcfg);
+       if (rv < 0)
+               return -1;
+
+       as_is = line->as_is && direction == GPIOD_LINE_REQUEST_DIRECTION_AS_IS;
+       gpiod_line_bulk_foreach_line_off(bulk, line, i) {
+               line->cflags = flags;
+               if (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
+                       line->output_value = hcfg.default_values[i];
+               line->as_is = as_is;
+               line_maybe_update(line);
+       }
        return 0;
 }
 
+int gpiod_line_set_flags(struct gpiod_line *line, int flags)
+{
+       struct gpiod_line_bulk bulk;
+
+       gpiod_line_bulk_init(&bulk);
+       gpiod_line_bulk_add(&bulk, line);
+
+       return gpiod_line_set_flags_bulk(&bulk, flags);
+}

lib/core.c +48 -1

+               errno = EPERM;
+               return -1;
+       }
+       if (line->direction == GPIOD_LINE_DIRECTION_OUTPUT) {
+               gpiod_line_bulk_foreach_line_off(bulk, line, i) {
+                       values[i] = line->output_value;
+               }
+               direction = GPIOD_LINE_REQUEST_DIRECTION_OUTPUT;
+       } else
+               direction = GPIOD_LINE_REQUEST_DIRECTION_INPUT;
+
+       return gpiod_line_set_config_bulk(bulk, direction,
+                                         flags, values);
+}
+
+int gpiod_line_set_direction_input(struct gpiod_line *line)
+{
+       return gpiod_line_set_config(line, GPIOD_LINE_REQUEST_DIRECTION_INPUT,
+                                    line->cflags, 0);
+}
+
+int gpiod_line_set_direction_bulk_input(struct gpiod_line_bulk *bulk)
+{
+       struct gpiod_line *line;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       return gpiod_line_set_config_bulk(bulk,
+                                         GPIOD_LINE_REQUEST_DIRECTION_INPUT,
+                                         line->cflags, NULL);
+}
+
+int gpiod_line_set_direction_output(struct gpiod_line *line, int value)
+{
+       return gpiod_line_set_config(line, GPIOD_LINE_REQUEST_DIRECTION_OUTPUT,
+                                    line->cflags, value);
+}
+
+int gpiod_line_set_direction_bulk_output(struct gpiod_line_bulk *bulk,
+                                       const int *values)
+{
+       struct gpiod_line *line;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       return gpiod_line_set_config_bulk(bulk,
+                                         GPIOD_LINE_REQUEST_DIRECTION_OUTPUT,
+                                         line->cflags, values);
+}
+
 int gpiod_line_event_wait(struct gpiod_line *line,
                          const struct timespec *timeout)
 {
--

2.24.0

pt., 15 lis 2019 o 15:45 Kent Gibson <[email protected]> napisał(a):

Quoted 24 lines

Extend the libgpiod API to support the setting line configuration using the GPIO GPIOHANDLE_SET_CONFIG_IOCTL uAPI ioctl.

The core change is the addition of gpiod_line_set_config, which provides a low level wrapper around the ioctl.

Additionally, higher level helper functions, gpiod_line_set_flags, gpiod_line_set_direction_input, and gpiod_line_set_direction_output provide slightly simplified APIs for common use cases.

Bulk forms of all functions are also provided.

Signed-off-by Kent Gibson <[email protected]>

---
 include/gpiod.h | 115 ++++++++++++++++++++++++++++
 lib/core.c      | 196 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 300 insertions(+), 11 deletions(-)

include/gpiod.h +0 -0

index 159d745..4053fd2 100644

include/gpiod.h void gpiod_line_release_bulk(struct gpiod_line_bulk *bulk) GPIOD_API; · line 1259

Quoted 8 lines

include/gpiod.h +5 -0

@@ -1252,6 +1252,14 @@ void gpiod_line_release_bulk(struct gpiod_line_bulk *bulk) GPIOD_API;
  */
 bool gpiod_line_is_requested(struct gpiod_line *line) GPIOD_API;
 
+/**
+ * @brief Check if the calling user has ownership of this line for values,
+ * not events.
+ * @param line GPIO line object.
+ * @return True if given line was requested, false otherwise.

I'd clarify: "requested for reading/setting values".

Quoted 88 lines

include/gpiod.h +81 -0

+ */
+bool gpiod_line_is_requested_values(struct gpiod_line *line) GPIOD_API;
+
 /**
  * @brief Check if the calling user has neither requested ownership of this
  *        line nor configured any event notifications.

@@ -1311,6 +1319,113 @@ int gpiod_line_set_value(struct gpiod_line *line, int value) GPIOD_API;
 int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk,
                              const int *values) GPIOD_API;
 
+/**
+ * @}
+ *
+ * @defgroup __line_config__ Setting line configuration
+ * @{
+ */
+
+/**
+ * @brief Update the configuration of a single GPIO line.
+ * @param line GPIO line object.
+ * @param direction Updated direction which may be one of
+ * GPIOD_LINE_REQUEST_DIRECTION_AS_IS, GPIOD_LINE_REQUEST_DIRECTION_INPUT,
+ * or GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @param flags Replacement flags.
+ * @param value The new output value for the line when direction is
+ * GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_config(struct gpiod_line *line, int direction,
+                         int flags, int value) GPIOD_API;
+
+/**
+ * @brief Update the configuration of a set of GPIO lines.
+ * @param bulk Set of GPIO lines.
+ * @param direction Updated direction which may be one of
+ * GPIOD_LINE_REQUEST_DIRECTION_AS_IS, GPIOD_LINE_REQUEST_DIRECTION_INPUT,
+ * or GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @param flags Replacement flags.
+ * @param values An array holding line_bulk->num_lines new logical values
+ * for lines when direction is GPIOD_LINE_REQUEST_DIRECTION_OUTPUT.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ *
+ * If the lines were not previously requested together, the behavior is
+ * undefined.
+ */
+int gpiod_line_set_config_bulk(struct gpiod_line_bulk *bulk,
+                              int direction, int flags,
+                              const int *values) GPIOD_API;
+
+
+/**
+ * @brief Update the configuration flags of a single GPIO line.
+ * @param line GPIO line object.
+ * @param flags Replacement flags.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_flags(struct gpiod_line *line, int flags) GPIOD_API;
+
+/**
+ * @brief Update the configuration flags of a set of GPIO lines.
+ * @param bulk Set of GPIO lines.
+ * @param flags Replacement flags.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ *
+ * If the lines were not previously requested together, the behavior is
+ * undefined.
+ */
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk,
+                             int flags) GPIOD_API;
+
+/**
+ * @brief Set the direction of a single GPIO line to input.
+ * @param line GPIO line object.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_direction_input(struct gpiod_line *line) GPIOD_API;
+
+/**
+ * @brief Set the direction of a set of GPIO lines to input.
+ * @param bulk Set of GPIO lines.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ *

include/gpiod.h int gpiod_line_set_value(struct gpiod_line *line, int value) GPIOD_API; · line 1404

Quoted 5 lines

include/gpiod.h +5 -0

+ * If the lines were not previously requested together, the behavior is
+ * undefined.
+ */
+int gpiod_line_set_direction_bulk_input(struct gpiod_line_bulk *bulk
+                                       ) GPIOD_API;

Please stick to the current convention of having "_bulk" at the end of the function name.

Quoted 35 lines

include/gpiod.h +24 -0

+
+/**
+ * @brief Set the direction of a single GPIO line to output.
+ * @param line GPIO line object.
+ * @param value The logical value output on the line.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ */
+int gpiod_line_set_direction_output(struct gpiod_line *line,
+                                   int value) GPIOD_API;
+
+/**
+ * @brief Set the direction of a set of GPIO lines to output.
+ * @param bulk Set of GPIO lines.
+ * @values The logical value to output for each line.
+ * @return 0 is the operation succeeds. In case of an error this routine
+ *         returns -1 and sets the last error number.
+ *
+ * If the lines were not previously requested together, the behavior is
+ * undefined.
+ */
+int gpiod_line_set_direction_bulk_output(struct gpiod_line_bulk *bulk,
+                                        const int *values) GPIOD_API;
+
 /**
  * @}
  *

lib/core.c +0 -0

index 9b7d88f..c42cda5 100644
@@ -36,10 +36,13 @@ struct gpiod_line {
        unsigned int offset;
        int direction;
        int active_state;

lib/core.c struct gpiod_line { · line 41

Quoted 4 lines

lib/core.c +3 -1

-       __u32 flags;
+       int output_value;
+       __u32 lflags;
+       __u32 cflags;

Ugh, these aren't very fortunate names - at first glance I have no idea what they mean. Either document those or change the names to something more obvious.

Quoted 233 lines

lib/core.c +129 -10

        int state;
        bool needs_update;
+       bool as_is;
 
        struct gpiod_chip *chip;
        struct line_fd_handle *fd_handle;

@@ -359,11 +362,11 @@ int gpiod_line_active_state(struct gpiod_line *line)
 
 int gpiod_line_bias(struct gpiod_line *line)
 {
-       if (line->flags & GPIOLINE_FLAG_BIAS_DISABLE)
+       if (line->lflags & GPIOLINE_FLAG_BIAS_DISABLE)
                return GPIOD_LINE_BIAS_DISABLE;
-       if (line->flags & GPIOLINE_FLAG_BIAS_PULL_UP)
+       if (line->lflags & GPIOLINE_FLAG_BIAS_PULL_UP)
                return GPIOD_LINE_BIAS_PULL_UP;
-       if (line->flags & GPIOLINE_FLAG_BIAS_PULL_DOWN)
+       if (line->lflags & GPIOLINE_FLAG_BIAS_PULL_DOWN)
                return GPIOD_LINE_BIAS_PULL_DOWN;
 
        return GPIOD_LINE_BIAS_AS_IS;

@@ -371,17 +374,17 @@ int gpiod_line_bias(struct gpiod_line *line)
 
 bool gpiod_line_is_used(struct gpiod_line *line)
 {
-       return line->flags & GPIOLINE_FLAG_KERNEL;
+       return line->lflags & GPIOLINE_FLAG_KERNEL;
 }
 
 bool gpiod_line_is_open_drain(struct gpiod_line *line)
 {
-       return line->flags & GPIOLINE_FLAG_OPEN_DRAIN;
+       return line->lflags & GPIOLINE_FLAG_OPEN_DRAIN;
 }
 
 bool gpiod_line_is_open_source(struct gpiod_line *line)
 {
-       return line->flags & GPIOLINE_FLAG_OPEN_SOURCE;
+       return line->lflags & GPIOLINE_FLAG_OPEN_SOURCE;
 }
 
 bool gpiod_line_needs_update(struct gpiod_line *line)

@@ -408,7 +411,7 @@ int gpiod_line_update(struct gpiod_line *line)
                                                ? GPIOD_LINE_ACTIVE_STATE_LOW
                                                : GPIOD_LINE_ACTIVE_STATE_HIGH;
 
-       line->flags = info.flags;
+       line->lflags = info.flags;
 
        strncpy(line->name, info.name, sizeof(line->name));
        strncpy(line->consumer, info.consumer, sizeof(line->consumer));

@@ -457,6 +460,20 @@ static bool line_bulk_all_requested(struct gpiod_line_bulk *bulk)
        return true;
 }
 
+static bool line_bulk_all_requested_values(struct gpiod_line_bulk *bulk)
+{
+       struct gpiod_line *line, **lineptr;
+
+       gpiod_line_bulk_foreach_line(bulk, line, lineptr) {
+               if (!gpiod_line_is_requested_values(line)) {
+                       errno = EPERM;
+                       return false;
+               }
+       }
+
+       return true;
+}
+
 static bool line_bulk_all_free(struct gpiod_line_bulk *bulk)
 {
        struct gpiod_line *line, **lineptr;

@@ -471,6 +488,27 @@ static bool line_bulk_all_free(struct gpiod_line_bulk *bulk)
        return true;
 }
 
+static bool line_request_direction_is_valid(int direction)
+{
+       if ((direction == GPIOD_LINE_REQUEST_DIRECTION_AS_IS) ||
+           (direction == GPIOD_LINE_REQUEST_DIRECTION_INPUT) ||
+           (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT))
+               return true;
+
+       errno = EINVAL;
+       return false;
+}
+
+static __u32 line_request_direction_to_gpio_handleflag(int direction)
+{
+       if (direction == GPIOD_LINE_REQUEST_DIRECTION_INPUT)
+               return GPIOHANDLE_REQUEST_INPUT;
+       if (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
+               return GPIOHANDLE_REQUEST_OUTPUT;
+
+       return 0;
+}
+
 static __u32 line_request_flag_to_gpio_handleflag(int flags)
 {
        int hflags = 0;

@@ -495,7 +533,7 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
                               const struct gpiod_line_request_config *config,
                               const int *default_vals)
 {
-       struct gpiod_line *line, **lineptr;
+       struct gpiod_line *line;
        struct line_fd_handle *line_fd;
        struct gpiohandle_request req;
        unsigned int i;

@@ -524,7 +562,6 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
        else if (config->request_type == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
                req.flags |= GPIOHANDLE_REQUEST_OUTPUT;
 
-
        gpiod_line_bulk_foreach_line_off(bulk, line, i) {
                req.lineoffsets[i] = gpiod_line_offset(line);
                if (config->request_type ==

@@ -548,8 +585,14 @@ static int line_request_values(struct gpiod_line_bulk *bulk,
        if (!line_fd)
                return -1;
 
-       gpiod_line_bulk_foreach_line(bulk, line, lineptr) {
+       gpiod_line_bulk_foreach_line_off(bulk, line, i) {
                line->state = LINE_REQUESTED_VALUES;
+               line->cflags = config->flags;
+               if (config->request_type ==
+                       GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
+                       line->output_value = req.default_values[i];
+               if (config->request_type == GPIOD_LINE_REQUEST_DIRECTION_AS_IS)
+                       line->as_is = true;
                line_set_fd(line, line_fd);
                line_maybe_update(line);
        }

@@ -590,6 +633,7 @@ static int line_request_event_single(struct gpiod_line *line,
                return -1;
 
        line->state = LINE_REQUESTED_EVENTS;
+       line->cflags = config->flags;
        line_set_fd(line, line_fd);
        line_maybe_update(line);
 

@@ -688,6 +732,11 @@ bool gpiod_line_is_requested(struct gpiod_line *line)
                line->state == LINE_REQUESTED_EVENTS);
 }
 
+bool gpiod_line_is_requested_values(struct gpiod_line *line)
+{
+       return (line->state == LINE_REQUESTED_VALUES);
+}
+
 bool gpiod_line_is_free(struct gpiod_line *line)
 {
        return line->state == LINE_FREE;

@@ -766,9 +815,134 @@ int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values)
        if (rv < 0)
                return -1;
 
+       gpiod_line_bulk_foreach_line_off(bulk, line, i)
+               line->output_value = data.values[i];
+
+       return 0;
+}
+
+int gpiod_line_set_config(struct gpiod_line *line, int direction,
+                         int flags, int value)
+{
+       struct gpiod_line_bulk bulk;
+
+       gpiod_line_bulk_init(&bulk);
+       gpiod_line_bulk_add(&bulk, line);
+
+       return gpiod_line_set_config_bulk(&bulk, direction, flags, &value);
+}
+
+int gpiod_line_set_config_bulk(struct gpiod_line_bulk *bulk,
+                              int direction, int flags,
+                              const int *values)
+{
+       struct gpiohandle_config hcfg;
+       struct gpiod_line *line;
+       unsigned int i;
+       int rv, fd;
+       bool as_is;
+
+       if (!line_bulk_same_chip(bulk) ||
+           !line_bulk_all_requested_values(bulk))
+               return -1;
+
+       if (!line_request_direction_is_valid(direction))
+               return -1;
+
+       memset(&hcfg, 0, sizeof(hcfg));
+
+       hcfg.flags = line_request_flag_to_gpio_handleflag(flags);
+       hcfg.flags |= line_request_direction_to_gpio_handleflag(direction);
+       if (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT && values) {
+               for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
+                       hcfg.default_values[i] = (uint8_t)!!values[i];
+       }
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       fd = line_get_fd(line);
+
+       rv = ioctl(fd, GPIOHANDLE_SET_CONFIG_IOCTL, &hcfg);
+       if (rv < 0)
+               return -1;
+
+       as_is = line->as_is && direction == GPIOD_LINE_REQUEST_DIRECTION_AS_IS;
+       gpiod_line_bulk_foreach_line_off(bulk, line, i) {
+               line->cflags = flags;
+               if (direction == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT)
+                       line->output_value = hcfg.default_values[i];
+               line->as_is = as_is;
+               line_maybe_update(line);
+       }
        return 0;
 }
 
+int gpiod_line_set_flags(struct gpiod_line *line, int flags)
+{
+       struct gpiod_line_bulk bulk;
+
+       gpiod_line_bulk_init(&bulk);
+       gpiod_line_bulk_add(&bulk, line);
+
+       return gpiod_line_set_flags_bulk(&bulk, flags);
+}
+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

lib/core.c int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values) · line 897

Quoted 5 lines

lib/core.c +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

lib/core.c +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

lib/core.c int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values) · line 897

Quoted 5 lines

lib/core.c +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

lib/core.c +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

lib/core.c int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values) · line 897

Quoted 5 lines

lib/core.c +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 31 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

lib/core.c +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

lib/core.c int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values) · line 897

Quoted 5 lines

lib/core.c +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

Quoted 4 lines

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

Quoted 37 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 35 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

lib/core.c +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

lib/core.c int gpiod_line_set_value_bulk(struct gpiod_line_bulk *bulk, const int *values) · line 897

Quoted 5 lines

lib/core.c +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Quoted 3 lines

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

So fail if needs_update?

Kent.

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski

Quoted 40 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 27 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Quoted 8 lines

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

Quoted 3 lines

So fail if needs_update?

Kent.

I'd say: do an implicit update before setting config.

Quoted 46 lines

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 45 lines · Bartosz Golaszewski

Quoted 32 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 31 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

Quoted 8 lines

Quoted 4 lines

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

So fail if needs_update?

Quoted 4 lines

Quoted 1 line

Kent.

I'd say: do an implicit update before setting config.

So gpiod_line_update if needs_update, and fail if that fails?

Kent.

śr., 20 lis 2019 o 15:13 Kent Gibson <[email protected]> napisał(a):

Quoted 53 lines · Bartosz Golaszewski

Quoted 50 lines

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 39 lines · Bartosz Golaszewski

Quoted 37 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 35 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Quoted 7 lines

Quoted 3 lines

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

So fail if needs_update?

Kent.

I'd say: do an implicit update before setting config.

Quoted 3 lines

So gpiod_line_update if needs_update, and fail if that fails?

Kent.

Without the if - needs_update is only set if an implicit update fails in line_maybe_update(). But in this case we need to be sure, so do it unconditionally.

Quoted 58 lines

śr., 20 lis 2019 o 15:13 Kent Gibson <[email protected]> napisał(a):

Quoted 56 lines · Bartosz Golaszewski

Quoted 44 lines

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski

Quoted 40 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 27 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Quoted 8 lines

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

Quoted 6 lines

Quoted 3 lines

So fail if needs_update?

Kent.

I'd say: do an implicit update before setting config.

So gpiod_line_update if needs_update, and fail if that fails?

Kent.

Quoted 4 lines

Without the if - needs_update is only set if an implicit update fails in line_maybe_update(). But in this case we need to be sure, so do it unconditionally.

Given that line_maybe_update is called at the end of request creation, and whenever set_config is called, how can line->direction be inconsistent with the kernel state - as long as needs_update is false?

Kent.

śr., 20 lis 2019 o 15:36 Kent Gibson <[email protected]> napisał(a):

Quoted 60 lines · Bartosz Golaszewski

Quoted 58 lines

śr., 20 lis 2019 o 15:13 Kent Gibson <[email protected]> napisał(a):

Quoted 56 lines · Bartosz Golaszewski

Quoted 44 lines

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski

Quoted 40 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 27 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Quoted 8 lines

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

Quoted 6 lines

Quoted 3 lines

So fail if needs_update?

Kent.

I'd say: do an implicit update before setting config.

So gpiod_line_update if needs_update, and fail if that fails?

Kent.

Quoted 9 lines

Quoted 4 lines

Without the if - needs_update is only set if an implicit update fails in line_maybe_update(). But in this case we need to be sure, so do it unconditionally.

Given that line_maybe_update is called at the end of request creation, and whenever set_config is called, how can line->direction be inconsistent with the kernel state - as long as needs_update is false?

I don't think we should call line_maybe_update() on set_config() - in this case we should call gpiod_line_update() and fail in set_config() if it fails.

I hope that's clearer.

Quoted 75 lines

śr., 20 lis 2019 o 15:36 Kent Gibson <[email protected]> napisał(a):

Quoted 60 lines · Bartosz Golaszewski

Quoted 58 lines

śr., 20 lis 2019 o 15:13 Kent Gibson <[email protected]> napisał(a):

Quoted 56 lines · Bartosz Golaszewski

Quoted 44 lines

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski

Quoted 40 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 27 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+       struct gpiod_line *line;
+       int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+       unsigned int i;
+       int direction;
+
+       line = gpiod_line_bulk_get_line(bulk, 0);
+       if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Quoted 8 lines

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

Quoted 6 lines

Quoted 3 lines

So fail if needs_update?

Kent.

I'd say: do an implicit update before setting config.

So gpiod_line_update if needs_update, and fail if that fails?

Kent.

Quoted 9 lines

Quoted 4 lines

Without the if - needs_update is only set if an implicit update fails in line_maybe_update(). But in this case we need to be sure, so do it unconditionally.

Given that line_maybe_update is called at the end of request creation, and whenever set_config is called, how can line->direction be inconsistent with the kernel state - as long as needs_update is false?

I don't think we should call line_maybe_update() on set_config() - in this case we should call gpiod_line_update() and fail in set_config() if it fails.

Quoted 2 lines

I hope that's clearer.

Not really. I was already shaky on the needs_update and I'm getting more confused about the overall needs_update handling policy by the minute.

Perhaps it will be more productive to go to the code. I'll send out what I have as v2 and we can go from there.

Cheers,
Kent.

czw., 21 lis 2019 o 01:34 Kent Gibson <[email protected]> napisał(a):

Quoted 77 lines · Bartosz Golaszewski

Quoted 75 lines

śr., 20 lis 2019 o 15:36 Kent Gibson <[email protected]> napisał(a):

Quoted 60 lines · Bartosz Golaszewski

Quoted 58 lines

śr., 20 lis 2019 o 15:13 Kent Gibson <[email protected]> napisał(a):

Quoted 56 lines · Bartosz Golaszewski

Quoted 44 lines

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski

Quoted 40 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 27 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 5 lines

Quoted 5 lines

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+ struct gpiod_line *line;
+ int values[GPIOD_LINE_BULK_MAX_LINES];

Quoted 9 lines

Quoted 5 lines

+ unsigned int i;
+ int direction;
+
+ line = gpiod_line_bulk_get_line(bulk, 0);
+ if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Quoted 8 lines

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

Quoted 6 lines

Quoted 3 lines

So fail if needs_update?

Kent.

I'd say: do an implicit update before setting config.

So gpiod_line_update if needs_update, and fail if that fails?

Kent.

Quoted 9 lines

Quoted 4 lines

Without the if - needs_update is only set if an implicit update fails in line_maybe_update(). But in this case we need to be sure, so do it unconditionally.

Given that line_maybe_update is called at the end of request creation, and whenever set_config is called, how can line->direction be inconsistent with the kernel state - as long as needs_update is false?

I don't think we should call line_maybe_update() on set_config() - in this case we should call gpiod_line_update() and fail in set_config() if it fails.

Quoted 6 lines

Quoted 2 lines

I hope that's clearer.

Not really. I was already shaky on the needs_update and I'm getting more confused about the overall needs_update handling policy by the minute.

Yeah it's not optimal. If you have better ideas on how to handle the fact that the kernel can't really notify us about the changes in line's flags introduced by other processes - I'll be more than glad to give them a try. At some point I was thinking about another ioctl() that - for a requested line - would return a file descriptor which would emit events when a line changes - for instance, it's requested by someone else or its direction is changed etc.

I then thought that nobody is requesting this yet and so it may be overkill.

Bart

Quoted 5 lines

Perhaps it will be more productive to go to the code. I'll send out what I have as v2 and we can go from there.

Cheers,
Kent.

Quoted 85 lines

czw., 21 lis 2019 o 01:34 Kent Gibson <[email protected]> napisał(a):

Quoted 77 lines · Bartosz Golaszewski

Quoted 75 lines

śr., 20 lis 2019 o 15:36 Kent Gibson <[email protected]> napisał(a):

Quoted 60 lines · Bartosz Golaszewski

Quoted 58 lines

śr., 20 lis 2019 o 15:13 Kent Gibson <[email protected]> napisał(a):

Quoted 56 lines · Bartosz Golaszewski

Quoted 44 lines

śr., 20 lis 2019 o 14:59 Kent Gibson <[email protected]> napisał(a):

Quoted 43 lines · Bartosz Golaszewski

Quoted 40 lines

wt., 19 lis 2019 o 16:53 Kent Gibson <[email protected]> napisał(a):

Quoted 27 lines · Kent Gibson

Quoted 16 lines

On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski

Quoted 14 lines

+
+int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
+{
+ struct gpiod_line *line;
+ int values[GPIOD_LINE_BULK_MAX_LINES];
+ unsigned int i;
+ int direction;
+
+ line = gpiod_line_bulk_get_line(bulk, 0);
+ if (line->as_is) {

Can you explain the purpose of this as_is field? I'm not sure this is
really needed.

Quoted 8 lines

It is there for gpiod_set_flags, which has to populate the direction flags in the SET_CONFIG ioctl. The existing line->direction is either input or output. It is drawn from GPIOLINE_FLAG_IS_OUT, so as-is is gets mapped to input.
I didn't want to change the existing line->direction, and adding the as-is seemed clearer than adding another flavour of direction that contained all three.

Quoted 8 lines

Hmmm, I think I see what you were getting at - the line->direction is the direction from the kernel, so it doesn't hurt to use that value to set the corresponding request flags - even if the original request was as-is??

If that is the case then the line->as_is can be dropped throughout.

Kent.

Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date.

Quoted 6 lines

Quoted 3 lines

So fail if needs_update?

Kent.

I'd say: do an implicit update before setting config.

So gpiod_line_update if needs_update, and fail if that fails?

Kent.

Quoted 9 lines

Quoted 4 lines

Without the if - needs_update is only set if an implicit update fails in line_maybe_update(). But in this case we need to be sure, so do it unconditionally.

Given that line_maybe_update is called at the end of request creation, and whenever set_config is called, how can line->direction be inconsistent with the kernel state - as long as needs_update is false?

I don't think we should call line_maybe_update() on set_config() - in this case we should call gpiod_line_update() and fail in set_config() if it fails.

Quoted 6 lines

Quoted 2 lines

I hope that's clearer.

Not really. I was already shaky on the needs_update and I'm getting more confused about the overall needs_update handling policy by the minute.

Quoted 8 lines

Yeah it's not optimal. If you have better ideas on how to handle the fact that the kernel can't really notify us about the changes in line's flags introduced by other processes - I'll be more than glad to give them a try. At some point I was thinking about another ioctl() that - for a requested line - would return a file descriptor which would emit events when a line changes - for instance, it's requested by someone else or its direction is changed etc.