| Message ID | 20191115144355.975-12-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for bias flags and SET_CONFIG | expand |
pt., 15 lis 2019 o 15:45 Kent Gibson <warthog618@gmail.com> napisał(a): > > 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 <warthog618@gmail.com> > --- > include/gpiod.h | 115 ++++++++++++++++++++++++++++ > lib/core.c | 196 +++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 300 insertions(+), 11 deletions(-) > > diff --git a/include/gpiod.h b/include/gpiod.h > index 159d745..4053fd2 100644 > --- a/include/gpiod.h > +++ b/include/gpiod.h > @@ -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". > + */ > +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. > + * > + * 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. > + > +/** > + * @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; > + > /** > * @} > * > diff --git a/lib/core.c b/lib/core.c > index 9b7d88f..c42cda5 100644 > --- a/lib/core.c > +++ b/lib/core.c > @@ -36,10 +36,13 @@ struct gpiod_line { > unsigned int offset; > int direction; > int active_state; > - __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. > > 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]; > + 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. Bart > + 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 >
On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski wrote: > pt., 15 lis 2019 o 15:45 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > 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 <warthog618@gmail.com> > > --- > > include/gpiod.h | 115 ++++++++++++++++++++++++++++ > > lib/core.c | 196 +++++++++++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 300 insertions(+), 11 deletions(-) > > > > diff --git a/include/gpiod.h b/include/gpiod.h > > index 159d745..4053fd2 100644 > > --- a/include/gpiod.h > > +++ b/include/gpiod.h > > @@ -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". > > > + */ > > +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. > > + * > > + * 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. > That would be clearer. Not sure why I went with those names - maybe taking the lead from gpiod_line_request_bulk_falling_edge_events_flags? Maybe I originally had a core set_direction function, and these were to be wrappers around that? Anyway - will change it. > > + > > +/** > > + * @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; > > + > > /** > > * @} > > * > > diff --git a/lib/core.c b/lib/core.c > > index 9b7d88f..c42cda5 100644 > > --- a/lib/core.c > > +++ b/lib/core.c > > @@ -36,10 +36,13 @@ struct gpiod_line { > > unsigned int offset; > > int direction; > > int active_state; > > - __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. > Yeah - not the best - lflags are line_flags (those returned by LINEINFO_IOCTL) and cflags is config_flags (those set in the initial line request). So maybe rename lflags to line_flags or info_flags, and rename cflags to handle_flags or req_flags? And add some documentation as well. > > > > 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]; > > + 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. Happy to entertain alternatives... Cheers, Kent.
On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > + > > > +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. > > > > 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 <warthog618@gmail.com> napisał(a): > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > + > > > > +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. > > > > > > > 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. > Yes, this is what I was thinking. Just need to make sure the value from the kernel is up-to-date. Bart
On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > + > > > > > +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. > > > > > > > > > > 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. > > > > 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 <warthog618@gmail.com> napisał(a): > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > + > > > > > > +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. > > > > > > > > > > > > > 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. > > > > > > > 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. Bart
On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > + > > > > > > > +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. > > > > > > > > > > > > > > > > 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. > > > > > > > > > > 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. > So gpiod_line_update if needs_update, and fail if that fails? Kent.
śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > + > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > 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. > > > > 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. Bart
On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > + > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > 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. > > > > > > > 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. > 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 <warthog618@gmail.com> napisał(a): > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > + > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > 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. Bart > Kent.
On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > + > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > 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. > > > > > > > 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. > 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 <warthog618@gmail.com> napisał(a): > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > + > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > 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 > 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.
On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > + > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > 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. > > > > > > > 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 didn't realise it was possible for a requested line's flags to be changed by other processes. Quite the opposite - I thought that was one of the reasons for GPIOD was to allow the userspace to prevent other from processes messing with requested lines. Kent. > I then thought that nobody is requesting this yet and so it may be overkill. > > Bart > > > 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 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > 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. > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > changed by other processes. Quite the opposite - I thought that was one > of the reasons for GPIOD was to allow the userspace to prevent other from > processes messing with requested lines. > Ugh, sorry, was writing it before coffee. I was thinking about a non-requested line. Something like lineinfo ioctl() but returning an fd notifying about changes. Maybe we could even consider having lineinfo2 ioctl() which would be extended with this functionality - not only would it fill the relevant structure but also pass a new fd for notification about changes. Bart
On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > changed by other processes. Quite the opposite - I thought that was one > > of the reasons for GPIOD was to allow the userspace to prevent other from > > processes messing with requested lines. > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > non-requested line. Something like lineinfo ioctl() but returning an > fd notifying about changes. Maybe we could even consider having > lineinfo2 ioctl() which would be extended with this functionality - > not only would it fill the relevant structure but also pass a new fd > for notification about changes. > Whew - that makes more sense. Had me worried there. Not sure how useful an async info ioctl would be. Couldn't you build something equivalent in userspace with the existing API - as long as you don't mind the daemon holding the line, and so having to control the line via the daemon. You want to be able to monitor without requesting the line? I'm still puzzled as to when the existing info ioctl could fail on a requested line - which is when needs_update gets set in line_maybe_update(). Hardware being unplugged? Cheers, Kent.
czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > changed by other processes. Quite the opposite - I thought that was one > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > processes messing with requested lines. > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > non-requested line. Something like lineinfo ioctl() but returning an > > fd notifying about changes. Maybe we could even consider having > > lineinfo2 ioctl() which would be extended with this functionality - > > not only would it fill the relevant structure but also pass a new fd > > for notification about changes. > > > > Whew - that makes more sense. Had me worried there. > > Not sure how useful an async info ioctl would be. Couldn't you build > something equivalent in userspace with the existing API - as long as you > don't mind the daemon holding the line, and so having to control the > line via the daemon. You want to be able to monitor without requesting > the line? > I'm not sure if I was expressing myself clearly enough: a hypothetical daemon calls LINEINFO ioctl(). Now a different program or kernel driver requests this line. The daemon is not up-to-date on its state unless it polls the line all the time. If a user now asks the daemon about this line's state - it will be given outdated info. Listening on this fd would allow us to be informed about such changes immediately. > I'm still puzzled as to when the existing info ioctl could fail on a > requested line - which is when needs_update gets set in > line_maybe_update(). Hardware being unplugged? > If the ioctl() can fail, then we're obligated to check the return value. As you say: unplugging the device is a good example - it may be a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can be easily disconnected from USB. Bart > Cheers, > Kent.
On Thu, Nov 21, 2019 at 11:03:32AM +0100, Bartosz Golaszewski wrote: > czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > > changed by other processes. Quite the opposite - I thought that was one > > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > > processes messing with requested lines. > > > > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > > non-requested line. Something like lineinfo ioctl() but returning an > > > fd notifying about changes. Maybe we could even consider having > > > lineinfo2 ioctl() which would be extended with this functionality - > > > not only would it fill the relevant structure but also pass a new fd > > > for notification about changes. > > > > > > > Whew - that makes more sense. Had me worried there. > > > > Not sure how useful an async info ioctl would be. Couldn't you build > > something equivalent in userspace with the existing API - as long as you > > don't mind the daemon holding the line, and so having to control the > > line via the daemon. You want to be able to monitor without requesting > > the line? > > > > I'm not sure if I was expressing myself clearly enough: a hypothetical > daemon calls LINEINFO ioctl(). Now a different program or kernel > driver requests this line. The daemon is not up-to-date on its state > unless it polls the line all the time. If a user now asks the daemon > about this line's state - it will be given outdated info. Listening on > this fd would allow us to be informed about such changes immediately. > I think I understand you - but you might not be getting my meaning... I was thinking the daemon would request the lines it wanted to monitor - which is why you would then have to control the line via the daemon. The daemon then always knows the state of the line. That obviously isn't the case if you want to monitor a line without requesting it, hence the "You want to be able to monitor without requesting the line?" question. > > I'm still puzzled as to when the existing info ioctl could fail on a > > requested line - which is when needs_update gets set in > > line_maybe_update(). Hardware being unplugged? > > > > If the ioctl() can fail, then we're obligated to check the return > value. As you say: unplugging the device is a good example - it may be > a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can > be easily disconnected from USB. > Fair enough. But for failures of that scale shouldn't the line request fail - rather than just setting needs_update? Or are there less catastrohpic failure modes? Kent.
czw., 21 lis 2019 o 11:18 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Thu, Nov 21, 2019 at 11:03:32AM +0100, Bartosz Golaszewski wrote: > > czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > > > changed by other processes. Quite the opposite - I thought that was one > > > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > > > processes messing with requested lines. > > > > > > > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > > > non-requested line. Something like lineinfo ioctl() but returning an > > > > fd notifying about changes. Maybe we could even consider having > > > > lineinfo2 ioctl() which would be extended with this functionality - > > > > not only would it fill the relevant structure but also pass a new fd > > > > for notification about changes. > > > > > > > > > > Whew - that makes more sense. Had me worried there. > > > > > > Not sure how useful an async info ioctl would be. Couldn't you build > > > something equivalent in userspace with the existing API - as long as you > > > don't mind the daemon holding the line, and so having to control the > > > line via the daemon. You want to be able to monitor without requesting > > > the line? > > > > > > > I'm not sure if I was expressing myself clearly enough: a hypothetical > > daemon calls LINEINFO ioctl(). Now a different program or kernel > > driver requests this line. The daemon is not up-to-date on its state > > unless it polls the line all the time. If a user now asks the daemon > > about this line's state - it will be given outdated info. Listening on > > this fd would allow us to be informed about such changes immediately. > > > > I think I understand you - but you might not be getting my meaning... > I was thinking the daemon would request the lines it wanted to monitor > - which is why you would then have to control the line via the daemon. No, I don't think requesting the line should be obligatory. In my WiP dbus daemon, I expose line info for all lines in the system by reading LINEINFO for each one. Then - for unrequested lines - every time the client asks for any line info again - I call gpiod_line_update() before responding. This could be optimized by this lineinfo fd feature. I don't want to force the user-space to choose between using a single central daemon or dealing with lines separately. > The daemon then always knows the state of the line. > That obviously isn't the case if you want to monitor a line without > requesting it, hence the "You want to be able to monitor without requesting > the line?" question. > In other words: yes. > > > > I'm still puzzled as to when the existing info ioctl could fail on a > > > requested line - which is when needs_update gets set in > > > line_maybe_update(). Hardware being unplugged? > > > > > > > If the ioctl() can fail, then we're obligated to check the return > > value. As you say: unplugging the device is a good example - it may be > > a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can > > be easily disconnected from USB. > > > > Fair enough. But for failures of that scale shouldn't the line request > fail - rather than just setting needs_update? Or are there less > catastrohpic failure modes? > What if the disconnect happens after the request but before the update? It's super unlikely, but again: the lineinfo ioctl() can fail, so we need to check the return value. We also can't update line info before requesting the line as it's racy - someone can change the state between the update and the request. (I hope I'm getting this right :)) Bart > Kent.
czw., 21 lis 2019 o 11:27 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): > > czw., 21 lis 2019 o 11:18 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Thu, Nov 21, 2019 at 11:03:32AM +0100, Bartosz Golaszewski wrote: > > > czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > > > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > > > > changed by other processes. Quite the opposite - I thought that was one > > > > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > > > > processes messing with requested lines. > > > > > > > > > > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > > > > non-requested line. Something like lineinfo ioctl() but returning an > > > > > fd notifying about changes. Maybe we could even consider having > > > > > lineinfo2 ioctl() which would be extended with this functionality - > > > > > not only would it fill the relevant structure but also pass a new fd > > > > > for notification about changes. > > > > > > > > > > > > > Whew - that makes more sense. Had me worried there. > > > > > > > > Not sure how useful an async info ioctl would be. Couldn't you build > > > > something equivalent in userspace with the existing API - as long as you > > > > don't mind the daemon holding the line, and so having to control the > > > > line via the daemon. You want to be able to monitor without requesting > > > > the line? > > > > > > > > > > I'm not sure if I was expressing myself clearly enough: a hypothetical > > > daemon calls LINEINFO ioctl(). Now a different program or kernel > > > driver requests this line. The daemon is not up-to-date on its state > > > unless it polls the line all the time. If a user now asks the daemon > > > about this line's state - it will be given outdated info. Listening on > > > this fd would allow us to be informed about such changes immediately. > > > > > > > I think I understand you - but you might not be getting my meaning... > > I was thinking the daemon would request the lines it wanted to monitor > > - which is why you would then have to control the line via the daemon. > > No, I don't think requesting the line should be obligatory. In my WiP > dbus daemon, I expose line info for all lines in the system by reading > LINEINFO for each one. Then - for unrequested lines - every time the > client asks for any line info again - I call gpiod_line_update() > before responding. This could be optimized by this lineinfo fd > feature. > > I don't want to force the user-space to choose between using a single > central daemon or dealing with lines separately. > > > The daemon then always knows the state of the line. > > That obviously isn't the case if you want to monitor a line without > > requesting it, hence the "You want to be able to monitor without requesting > > the line?" question. > > > > In other words: yes. > Just to be clear: I mean line info - not values or events. By monitoring I mean: be notified about changes to the line properties without requesting it. As for implementation: I imagine an ioctl() called LINEINFO_FD that would return an open file descriptor on which read events would arrive when the line properties change and then we could call regular line info ioctl() to actually re-read it. Does that make sense? I can try to prepare an example implementation. Bart > > > > > > I'm still puzzled as to when the existing info ioctl could fail on a > > > > requested line - which is when needs_update gets set in > > > > line_maybe_update(). Hardware being unplugged? > > > > > > > > > > If the ioctl() can fail, then we're obligated to check the return > > > value. As you say: unplugging the device is a good example - it may be > > > a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can > > > be easily disconnected from USB. > > > > > > > Fair enough. But for failures of that scale shouldn't the line request > > fail - rather than just setting needs_update? Or are there less > > catastrohpic failure modes? > > > > What if the disconnect happens after the request but before the > update? It's super unlikely, but again: the lineinfo ioctl() can fail, > so we need to check the return value. We also can't update line info > before requesting the line as it's racy - someone can change the state > between the update and the request. > > (I hope I'm getting this right :)) > > Bart > > > Kent.
On Thu, Nov 21, 2019 at 11:27:10AM +0100, Bartosz Golaszewski wrote: > czw., 21 lis 2019 o 11:18 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Thu, Nov 21, 2019 at 11:03:32AM +0100, Bartosz Golaszewski wrote: > > > czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > > > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > > > > changed by other processes. Quite the opposite - I thought that was one > > > > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > > > > processes messing with requested lines. > > > > > > > > > > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > > > > non-requested line. Something like lineinfo ioctl() but returning an > > > > > fd notifying about changes. Maybe we could even consider having > > > > > lineinfo2 ioctl() which would be extended with this functionality - > > > > > not only would it fill the relevant structure but also pass a new fd > > > > > for notification about changes. > > > > > > > > > > > > > Whew - that makes more sense. Had me worried there. > > > > > > > > Not sure how useful an async info ioctl would be. Couldn't you build > > > > something equivalent in userspace with the existing API - as long as you > > > > don't mind the daemon holding the line, and so having to control the > > > > line via the daemon. You want to be able to monitor without requesting > > > > the line? > > > > > > > > > > I'm not sure if I was expressing myself clearly enough: a hypothetical > > > daemon calls LINEINFO ioctl(). Now a different program or kernel > > > driver requests this line. The daemon is not up-to-date on its state > > > unless it polls the line all the time. If a user now asks the daemon > > > about this line's state - it will be given outdated info. Listening on > > > this fd would allow us to be informed about such changes immediately. > > > > > > > I think I understand you - but you might not be getting my meaning... > > I was thinking the daemon would request the lines it wanted to monitor > > - which is why you would then have to control the line via the daemon. > > No, I don't think requesting the line should be obligatory. In my WiP > dbus daemon, I expose line info for all lines in the system by reading > LINEINFO for each one. Then - for unrequested lines - every time the > client asks for any line info again - I call gpiod_line_update() > before responding. This could be optimized by this lineinfo fd > feature. > > I don't want to force the user-space to choose between using a single > central daemon or dealing with lines separately. > > > The daemon then always knows the state of the line. > > That obviously isn't the case if you want to monitor a line without > > requesting it, hence the "You want to be able to monitor without requesting > > the line?" question. > > > > In other words: yes. > > > > > > > I'm still puzzled as to when the existing info ioctl could fail on a > > > > requested line - which is when needs_update gets set in > > > > line_maybe_update(). Hardware being unplugged? > > > > > > > > > > If the ioctl() can fail, then we're obligated to check the return > > > value. As you say: unplugging the device is a good example - it may be > > > a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can > > > be easily disconnected from USB. > > > > > > > Fair enough. But for failures of that scale shouldn't the line request > > fail - rather than just setting needs_update? Or are there less > > catastrohpic failure modes? > > > > What if the disconnect happens after the request but before the > update? It's super unlikely, but again: the lineinfo ioctl() can fail, > so we need to check the return value. We also can't update line info > before requesting the line as it's racy - someone can change the state > between the update and the request. > > (I hope I'm getting this right :)) > I understand that the disconnect can occur between the request ioctl and the info ioctl, but both of those are called within line_request_values(), which implements the core of gpiod_line_request_bulk(), so the opportunity exists to propagate the info failure back as part of the request, but instead the error is absorbed and needs_update is set. This puts the onus on the caller to always check gpiod_line_needs_update() between requesting a line and calling any of the state accessors - else they may be returning garbage. Similarly the event case in line_request_event_single(). I was wondering what the reasoning was for this approach? Kent.
On Thu, Nov 21, 2019 at 11:31:52AM +0100, Bartosz Golaszewski wrote: > czw., 21 lis 2019 o 11:27 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): > > > > czw., 21 lis 2019 o 11:18 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Thu, Nov 21, 2019 at 11:03:32AM +0100, Bartosz Golaszewski wrote: > > > > czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > > > > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > > > > > changed by other processes. Quite the opposite - I thought that was one > > > > > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > > > > > processes messing with requested lines. > > > > > > > > > > > > > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > > > > > non-requested line. Something like lineinfo ioctl() but returning an > > > > > > fd notifying about changes. Maybe we could even consider having > > > > > > lineinfo2 ioctl() which would be extended with this functionality - > > > > > > not only would it fill the relevant structure but also pass a new fd > > > > > > for notification about changes. > > > > > > > > > > > > > > > > Whew - that makes more sense. Had me worried there. > > > > > > > > > > Not sure how useful an async info ioctl would be. Couldn't you build > > > > > something equivalent in userspace with the existing API - as long as you > > > > > don't mind the daemon holding the line, and so having to control the > > > > > line via the daemon. You want to be able to monitor without requesting > > > > > the line? > > > > > > > > > > > > > I'm not sure if I was expressing myself clearly enough: a hypothetical > > > > daemon calls LINEINFO ioctl(). Now a different program or kernel > > > > driver requests this line. The daemon is not up-to-date on its state > > > > unless it polls the line all the time. If a user now asks the daemon > > > > about this line's state - it will be given outdated info. Listening on > > > > this fd would allow us to be informed about such changes immediately. > > > > > > > > > > I think I understand you - but you might not be getting my meaning... > > > I was thinking the daemon would request the lines it wanted to monitor > > > - which is why you would then have to control the line via the daemon. > > > > No, I don't think requesting the line should be obligatory. In my WiP > > dbus daemon, I expose line info for all lines in the system by reading > > LINEINFO for each one. Then - for unrequested lines - every time the > > client asks for any line info again - I call gpiod_line_update() > > before responding. This could be optimized by this lineinfo fd > > feature. > > > > I don't want to force the user-space to choose between using a single > > central daemon or dealing with lines separately. > > > > > The daemon then always knows the state of the line. > > > That obviously isn't the case if you want to monitor a line without > > > requesting it, hence the "You want to be able to monitor without requesting > > > the line?" question. > > > > > > > In other words: yes. > > > > Just to be clear: I mean line info - not values or events. By > monitoring I mean: be notified about changes to the line properties > without requesting it. > > As for implementation: I imagine an ioctl() called LINEINFO_FD that > would return an open file descriptor on which read events would arrive > when the line properties change and then we could call regular line > info ioctl() to actually re-read it. Does that make sense? I can try > to prepare an example implementation. > Ah, ok. So you might want to send a dbus message when someone requests a line, or changes direction or whatever, but not have the monitoring daemon involved in the line control. And not have the daemon polling the LINEINFO ioctl. Sounds reasonable. Now, how did we get here?? ;-). Kent. > Bart > > > > > > > > > I'm still puzzled as to when the existing info ioctl could fail on a > > > > > requested line - which is when needs_update gets set in > > > > > line_maybe_update(). Hardware being unplugged? > > > > > > > > > > > > > If the ioctl() can fail, then we're obligated to check the return > > > > value. As you say: unplugging the device is a good example - it may be > > > > a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can > > > > be easily disconnected from USB. > > > > > > > > > > Fair enough. But for failures of that scale shouldn't the line request > > > fail - rather than just setting needs_update? Or are there less > > > catastrohpic failure modes? > > > > > > > What if the disconnect happens after the request but before the > > update? It's super unlikely, but again: the lineinfo ioctl() can fail, > > so we need to check the return value. We also can't update line info > > before requesting the line as it's racy - someone can change the state > > between the update and the request. > > > > (I hope I'm getting this right :)) > > > > Bart > > > > > Kent.
czw., 21 lis 2019 o 11:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Thu, Nov 21, 2019 at 11:27:10AM +0100, Bartosz Golaszewski wrote: > > czw., 21 lis 2019 o 11:18 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Thu, Nov 21, 2019 at 11:03:32AM +0100, Bartosz Golaszewski wrote: > > > > czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > > > > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > > > > > changed by other processes. Quite the opposite - I thought that was one > > > > > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > > > > > processes messing with requested lines. > > > > > > > > > > > > > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > > > > > non-requested line. Something like lineinfo ioctl() but returning an > > > > > > fd notifying about changes. Maybe we could even consider having > > > > > > lineinfo2 ioctl() which would be extended with this functionality - > > > > > > not only would it fill the relevant structure but also pass a new fd > > > > > > for notification about changes. > > > > > > > > > > > > > > > > Whew - that makes more sense. Had me worried there. > > > > > > > > > > Not sure how useful an async info ioctl would be. Couldn't you build > > > > > something equivalent in userspace with the existing API - as long as you > > > > > don't mind the daemon holding the line, and so having to control the > > > > > line via the daemon. You want to be able to monitor without requesting > > > > > the line? > > > > > > > > > > > > > I'm not sure if I was expressing myself clearly enough: a hypothetical > > > > daemon calls LINEINFO ioctl(). Now a different program or kernel > > > > driver requests this line. The daemon is not up-to-date on its state > > > > unless it polls the line all the time. If a user now asks the daemon > > > > about this line's state - it will be given outdated info. Listening on > > > > this fd would allow us to be informed about such changes immediately. > > > > > > > > > > I think I understand you - but you might not be getting my meaning... > > > I was thinking the daemon would request the lines it wanted to monitor > > > - which is why you would then have to control the line via the daemon. > > > > No, I don't think requesting the line should be obligatory. In my WiP > > dbus daemon, I expose line info for all lines in the system by reading > > LINEINFO for each one. Then - for unrequested lines - every time the > > client asks for any line info again - I call gpiod_line_update() > > before responding. This could be optimized by this lineinfo fd > > feature. > > > > I don't want to force the user-space to choose between using a single > > central daemon or dealing with lines separately. > > > > > The daemon then always knows the state of the line. > > > That obviously isn't the case if you want to monitor a line without > > > requesting it, hence the "You want to be able to monitor without requesting > > > the line?" question. > > > > > > > In other words: yes. > > > > > > > > > > I'm still puzzled as to when the existing info ioctl could fail on a > > > > > requested line - which is when needs_update gets set in > > > > > line_maybe_update(). Hardware being unplugged? > > > > > > > > > > > > > If the ioctl() can fail, then we're obligated to check the return > > > > value. As you say: unplugging the device is a good example - it may be > > > > a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can > > > > be easily disconnected from USB. > > > > > > > > > > Fair enough. But for failures of that scale shouldn't the line request > > > fail - rather than just setting needs_update? Or are there less > > > catastrohpic failure modes? > > > > > > > What if the disconnect happens after the request but before the > > update? It's super unlikely, but again: the lineinfo ioctl() can fail, > > so we need to check the return value. We also can't update line info > > before requesting the line as it's racy - someone can change the state > > between the update and the request. > > > > (I hope I'm getting this right :)) > > > > I understand that the disconnect can occur between the request ioctl and > the info ioctl, but both of those are called within > line_request_values(), which implements the core of > gpiod_line_request_bulk(), so the opportunity exists to propagate the info > failure back as part of the request, but instead the error is absorbed > and needs_update is set. This puts the onus on the caller to always > check gpiod_line_needs_update() between requesting a line and calling > any of the state accessors - else they may be returning garbage. > > Similarly the event case in line_request_event_single(). > > I was wondering what the reasoning was for this approach? > So I've been thinking about why I decided to go with this - and while I can't remember any more (I didn't comment on it...) I think there must have been *some* reason to do this. Now it actually seems to me as if we could make the request function release all lines and return an error if updating fails (we'd also need to make gpiod_line_needs_update() deprecated and always return false for compatibility). I need to think about it some more... Bart > Kent.
czw., 21 lis 2019 o 12:07 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Thu, Nov 21, 2019 at 11:31:52AM +0100, Bartosz Golaszewski wrote: > > czw., 21 lis 2019 o 11:27 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): > > > > > > czw., 21 lis 2019 o 11:18 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Thu, Nov 21, 2019 at 11:03:32AM +0100, Bartosz Golaszewski wrote: > > > > > czw., 21 lis 2019 o 10:30 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Thu, Nov 21, 2019 at 09:46:07AM +0100, Bartosz Golaszewski wrote: > > > > > > > czw., 21 lis 2019 o 08:46 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > On Thu, Nov 21, 2019 at 08:13:42AM +0100, Bartosz Golaszewski wrote: > > > > > > > > > czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote: > > > > > > > > > > > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 didn't realise it was possible for a requested line's flags to be > > > > > > > > changed by other processes. Quite the opposite - I thought that was one > > > > > > > > of the reasons for GPIOD was to allow the userspace to prevent other from > > > > > > > > processes messing with requested lines. > > > > > > > > > > > > > > > > > > > > > > Ugh, sorry, was writing it before coffee. I was thinking about a > > > > > > > non-requested line. Something like lineinfo ioctl() but returning an > > > > > > > fd notifying about changes. Maybe we could even consider having > > > > > > > lineinfo2 ioctl() which would be extended with this functionality - > > > > > > > not only would it fill the relevant structure but also pass a new fd > > > > > > > for notification about changes. > > > > > > > > > > > > > > > > > > > Whew - that makes more sense. Had me worried there. > > > > > > > > > > > > Not sure how useful an async info ioctl would be. Couldn't you build > > > > > > something equivalent in userspace with the existing API - as long as you > > > > > > don't mind the daemon holding the line, and so having to control the > > > > > > line via the daemon. You want to be able to monitor without requesting > > > > > > the line? > > > > > > > > > > > > > > > > I'm not sure if I was expressing myself clearly enough: a hypothetical > > > > > daemon calls LINEINFO ioctl(). Now a different program or kernel > > > > > driver requests this line. The daemon is not up-to-date on its state > > > > > unless it polls the line all the time. If a user now asks the daemon > > > > > about this line's state - it will be given outdated info. Listening on > > > > > this fd would allow us to be informed about such changes immediately. > > > > > > > > > > > > > I think I understand you - but you might not be getting my meaning... > > > > I was thinking the daemon would request the lines it wanted to monitor > > > > - which is why you would then have to control the line via the daemon. > > > > > > No, I don't think requesting the line should be obligatory. In my WiP > > > dbus daemon, I expose line info for all lines in the system by reading > > > LINEINFO for each one. Then - for unrequested lines - every time the > > > client asks for any line info again - I call gpiod_line_update() > > > before responding. This could be optimized by this lineinfo fd > > > feature. > > > > > > I don't want to force the user-space to choose between using a single > > > central daemon or dealing with lines separately. > > > > > > > The daemon then always knows the state of the line. > > > > That obviously isn't the case if you want to monitor a line without > > > > requesting it, hence the "You want to be able to monitor without requesting > > > > the line?" question. > > > > > > > > > > In other words: yes. > > > > > > > Just to be clear: I mean line info - not values or events. By > > monitoring I mean: be notified about changes to the line properties > > without requesting it. > > > > As for implementation: I imagine an ioctl() called LINEINFO_FD that > > would return an open file descriptor on which read events would arrive > > when the line properties change and then we could call regular line > > info ioctl() to actually re-read it. Does that make sense? I can try > > to prepare an example implementation. > > > > Ah, ok. So you might want to send a dbus message when someone requests > a line, or changes direction or whatever, but not have the monitoring > daemon involved in the line control. And not have the daemon polling > the LINEINFO ioctl. > > Sounds reasonable. > That would be great, yes. > Now, how did we get here?? ;-). > I'm actually happy you're doing this. You're making me think about issues I never noticed. It's always great to have someone go through your code. Thanks! Bart > Kent. > > > Bart > > > > > > > > > > > > I'm still puzzled as to when the existing info ioctl could fail on a > > > > > > requested line - which is when needs_update gets set in > > > > > > line_maybe_update(). Hardware being unplugged? > > > > > > > > > > > > > > > > If the ioctl() can fail, then we're obligated to check the return > > > > > value. As you say: unplugging the device is a good example - it may be > > > > > a GPIO expander on an HID device (e.g. Silicon Labs CP2112) that can > > > > > be easily disconnected from USB. > > > > > > > > > > > > > Fair enough. But for failures of that scale shouldn't the line request > > > > fail - rather than just setting needs_update? Or are there less > > > > catastrohpic failure modes? > > > > > > > > > > What if the disconnect happens after the request but before the > > > update? It's super unlikely, but again: the lineinfo ioctl() can fail, > > > so we need to check the return value. We also can't update line info > > > before requesting the line as it's racy - someone can change the state > > > between the update and the request. > > > > > > (I hope I'm getting this right :)) > > > > > > Bart > > > > > > > Kent.
diff --git a/include/gpiod.h b/include/gpiod.h index 159d745..4053fd2 100644 --- a/include/gpiod.h +++ b/include/gpiod.h @@ -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. + */ +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. + * + * 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; + +/** + * @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; + /** * @} * diff --git a/lib/core.c b/lib/core.c index 9b7d88f..c42cda5 100644 --- a/lib/core.c +++ b/lib/core.c @@ -36,10 +36,13 @@ struct gpiod_line { unsigned int offset; int direction; int active_state; - __u32 flags; + int output_value; + __u32 lflags; + __u32 cflags; 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]; + unsigned int i; + int direction; + + line = gpiod_line_bulk_get_line(bulk, 0); + if (line->as_is) { + 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) {
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 <warthog618@gmail.com> --- include/gpiod.h | 115 ++++++++++++++++++++++++++++ lib/core.c | 196 +++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 300 insertions(+), 11 deletions(-)