| Message ID | 20200623040107.22270-14-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: cdev: add uAPI V2 | expand |
On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <warthog618@gmail.com> wrote: > > Add a new version of the uAPI to address existing 32/64bit alignment I think using - would be nice, like 32/64-bit (or at least space like 32/64 bit) as a common practice. > issues, add support for debounce and event sequence numbers, and provide > some future proofing by adding padding reserved for future use. > > The alignment issue relates to the gpioevent_data, which packs to different > sizes on 32bit and 64bit platforms. That creates problems for 32bit apps > running on 64bit kernels. Dashes, please. $ git grep -n -w '32 bit' | wc 1904 $ git grep -n -w '64 bit' | wc 1155 ~3k $ git grep -n -w '32bit' | wc 1405 $ git grep -n -w '64bit' | wc 870 ~2.2k $ git grep -n -w '64-bit' | wc 3196 $ git grep -n -w '32-bit' | wc 4369 ~7.5k > The patch addresses that particular issue, and > the problem more generally, by adding pad fields that explicitly pad > structs out to 64bit boundaries, so they will pack to the same size now, > and even if some of the reserved padding is used for __u64 fields in the > future. > > The lack of future proofing in V1 makes it impossible to, for example, > add the debounce feature that is included in v2. > The future proofing is addressed by providing reserved padding in all > structs for future features. Specifically, the line request, > config, info, info_changed and event structs receive updated versions, > and the first three new ioctls. ... > Firstly, I've reworked the flags field throughout. V1 has three different > flags fields, each with their own separate bit definitions. In V2 that is > collapsed to one. Further, the bits of the V2 flags field are used > as feature enable flags, with any other necessary configuration fields encoded > separately. This is simpler and clearer, while also providing a foundation > for adding features in the future. > > I've also merged the handle and event requests into a single request, the > line request, as the two requests where mostly the same, other than the where-> were ? (yes I noticed it's not a part of commit message) > edge detection provided by event requests. As a byproduct, the V2 uAPI > allows for multiple lines producing edge events on the same line handle. > This is a new capability as V1 only supports a single line in an event > request. > > This means there are now only two types of file handle to be concerned with, > the chip and the line, and it is clearer which ioctls apply to which type > of handle. > > There is also some minor renaming of fields for consistency compared to their > V1 counterparts, e.g. offset rather than lineoffset or line_offset, and > consumer rather than consumer_label. > > Additionally, V1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in V2 for clarity, > and the gpiohandle_data __u8 array becomes a bitmap gpioline_values. > > The V2 uAPI is mostly just a reorganisation of V1, so userspace code, > particularly libgpiod, should easily port to it. > > The padding sizes have been carried over from the RFC version, although the > seqnos added to the gpioline_event alone would've used the all of the padding > for that struct, had they not been added here. So I'm moderatly concerned > that those values are too small due to a lack of imagination on may part and > should be increased to decrease the probability of running out of space in > the padding and requiring creative solutions or even a V3. ... > +#include <linux/kernel.h> Perhaps keep it in order? ... > + * Must be a multiple of 8 to ensure 32/64bit alignment of structs. Dash. And same for all cases like this. ... > +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */ the -> The ? > +#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64) Not sure we need this definition. The problem is that definitions in the uAPI header are also part of uAPI. Here is just a calculus which can be done manually since if anybody changes MAX, they will anyway have to check if everything else is consistent. And yes, I'm not in favour of including kernel.h here and there. ... > +/* > + * Struct padding sizes. > + * > + * These are sized to pad structs to 64bit boundaries. > + * To maintain 32/64bit alignment, any arbitrary change must be even, as > + * the pad elements are __u32. > + */ > +#define GPIOLINE_CONFIG_PAD_SIZE 7 > +#define GPIOLINE_REQUEST_PAD_SIZE 5 > +#define GPIOLINE_INFO_V2_PAD_SIZE 5 > +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5 > +#define GPIOLINE_EVENT_PAD_SIZE 2 I'm not sure they (definitions) should be part of UAPI. Can't you simple hard code numbers per case? ... > +/** > + * struct gpioline_config - Configuration for GPIO lines > + */ > +struct gpioline_config { > + struct gpioline_values values; > + __u32 flags; > + /* Note that the following four fields are equivalent to a single u32. */ > + __u8 direction; > + __u8 drive; > + __u8 bias; > + __u8 edge_detection; > + __u32 debounce_period; > + __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */ I would just put minimum here. If you need to extend you have to use sizeof(struct) as a version of ABI. > +}; I'm wondering how many lines (in average) the user usually changes at once? One? Two? Perhaps we need to be better with this, something like single line / multiple lines? So, having a struct for single line change being embedded multiple times would allow to configure atomically several lines with different requirements. For example you can turn directions of the two lines for some kind of half-duplex bit banging protocol. I'm not sure about the rest, but to me it seems reasonable to have single vs. multiple separation in some of the structures. ... > +/* > + * ABI V1 V1 -> v1 And below V2 -> v2. > + */
wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > Add a new version of the uAPI to address existing 32/64bit alignment > issues, add support for debounce and event sequence numbers, and provide > some future proofing by adding padding reserved for future use. > > The alignment issue relates to the gpioevent_data, which packs to different > sizes on 32bit and 64bit platforms. That creates problems for 32bit apps > running on 64bit kernels. The patch addresses that particular issue, and > the problem more generally, by adding pad fields that explicitly pad > structs out to 64bit boundaries, so they will pack to the same size now, > and even if some of the reserved padding is used for __u64 fields in the > future. > > The lack of future proofing in V1 makes it impossible to, for example, > add the debounce feature that is included in v2. > The future proofing is addressed by providing reserved padding in all > structs for future features. Specifically, the line request, > config, info, info_changed and event structs receive updated versions, > and the first three new ioctls. > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > --- > > I haven't added any padding to gpiochip_info, as I haven't seen any calls > for new features for the corresponding ioctl, but I'm open to updating that > as well. > > As the majority of the structs and ioctls were being replaced, it seemed > opportune to rework some of the other aspects of the uAPI. > > Firstly, I've reworked the flags field throughout. V1 has three different > flags fields, each with their own separate bit definitions. In V2 that is > collapsed to one. Further, the bits of the V2 flags field are used > as feature enable flags, with any other necessary configuration fields encoded > separately. This is simpler and clearer, while also providing a foundation > for adding features in the future. > > I've also merged the handle and event requests into a single request, the > line request, as the two requests where mostly the same, other than the > edge detection provided by event requests. As a byproduct, the V2 uAPI > allows for multiple lines producing edge events on the same line handle. > This is a new capability as V1 only supports a single line in an event > request. > > This means there are now only two types of file handle to be concerned with, > the chip and the line, and it is clearer which ioctls apply to which type > of handle. > > There is also some minor renaming of fields for consistency compared to their > V1 counterparts, e.g. offset rather than lineoffset or line_offset, and > consumer rather than consumer_label. > > Additionally, V1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in V2 for clarity, > and the gpiohandle_data __u8 array becomes a bitmap gpioline_values. > > The V2 uAPI is mostly just a reorganisation of V1, so userspace code, > particularly libgpiod, should easily port to it. > > The padding sizes have been carried over from the RFC version, although the > seqnos added to the gpioline_event alone would've used the all of the padding > for that struct, had they not been added here. So I'm moderatly concerned > that those values are too small due to a lack of imagination on may part and > should be increased to decrease the probability of running out of space in > the padding and requiring creative solutions or even a V3. > > Changes since the RFC: > - document the constraints on array sizes to maintain 32/64 alignment > - add sequence numbers to gpioline_event > - use bitmap for values instead of array of __u8 > - gpioline_info_v2 contains gpioline_config instead of its composite fields > - provide constants for all array sizes, especially padding > - renamed "GPIOLINE_FLAG_V2_KERNEL" to "GPIOLINE_FLAG_V2_USED" > - renamed "default_values" to "values" > - made gpioline_direction zero based > - document clock used in gpioline_event timestamp > - add event_buffer_size to gpioline_request > > > include/uapi/linux/gpio.h | 237 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 230 insertions(+), 7 deletions(-) > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > index 4e1139ab25bc..e4ed6f79e332 100644 > --- a/include/uapi/linux/gpio.h > +++ b/include/uapi/linux/gpio.h > @@ -11,11 +11,14 @@ > #ifndef _UAPI_GPIO_H_ > #define _UAPI_GPIO_H_ > > +#include <linux/kernel.h> > #include <linux/ioctl.h> > #include <linux/types.h> > > /* > * The maximum size of name and label arrays. > + * > + * Must be a multiple of 8 to ensure 32/64bit alignment of structs. > */ > #define GPIO_MAX_NAME_SIZE 32 > > @@ -32,6 +35,211 @@ struct gpiochip_info { > __u32 lines; > }; > > +/* > + * Maximum number of requested lines. > + * > + * Must be a multiple of 8 to ensure 32/64bit alignment of structs. > + */ > +#define GPIOLINES_MAX 64 > + > +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */ > +#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64) > + > +enum gpioline_direction { > + GPIOLINE_DIRECTION_INPUT = 0, > + GPIOLINE_DIRECTION_OUTPUT = 1, > +}; > + > +enum gpioline_drive { > + GPIOLINE_DRIVE_PUSH_PULL = 0, > + GPIOLINE_DRIVE_OPEN_DRAIN = 1, > + GPIOLINE_DRIVE_OPEN_SOURCE = 2, > +}; > + > +enum gpioline_bias { > + GPIOLINE_BIAS_DISABLED = 0, > + GPIOLINE_BIAS_PULL_UP = 1, > + GPIOLINE_BIAS_PULL_DOWN = 2, > +}; > + > +enum gpioline_edge { > + GPIOLINE_EDGE_NONE = 0, > + GPIOLINE_EDGE_RISING = 1, > + GPIOLINE_EDGE_FALLING = 2, > + GPIOLINE_EDGE_BOTH = 3, > +}; Nit: I'd just add a comment before these enums saying that values of 0 represent sensible default settings. > + > +/* Line flags - V2 */ > +#define GPIOLINE_FLAG_V2_USED (1UL << 0) /* line is not available for request */ > +#define GPIOLINE_FLAG_V2_ACTIVE_LOW (1UL << 1) > +#define GPIOLINE_FLAG_V2_DIRECTION (1UL << 2) > +#define GPIOLINE_FLAG_V2_DRIVE (1UL << 3) > +#define GPIOLINE_FLAG_V2_BIAS (1UL << 4) > +#define GPIOLINE_FLAG_V2_EDGE_DETECTION (1UL << 5) > +#define GPIOLINE_FLAG_V2_DEBOUNCE (1UL << 6) > + > +/* > + * Struct padding sizes. > + * > + * These are sized to pad structs to 64bit boundaries. > + * To maintain 32/64bit alignment, any arbitrary change must be even, as > + * the pad elements are __u32. > + */ > +#define GPIOLINE_CONFIG_PAD_SIZE 7 > +#define GPIOLINE_REQUEST_PAD_SIZE 5 > +#define GPIOLINE_INFO_V2_PAD_SIZE 5 > +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5 > +#define GPIOLINE_EVENT_PAD_SIZE 2 > + Did someone request such definitions? IMO it's one of those rare instances where I'd prefer to see magic numbers used in the structs. I'm not sure if we need this indirection. > +/** > + * struct gpioline_values - Values of GPIO lines > + * @bitmap: a bitmap containing the value of the lines, set to 1 for active > + * and 0 for inactive. Note that this is the logical value, which will be > + * the opposite of the physical value if GPIOLINE_FLAG_V2_ACTIVE_LOW is > + * set in flags. > + */ > +struct gpioline_values { > + __u64 bitmap[GPIOLINES_BITMAP_SIZE]; > +}; Ok so now when embedding this structure we get something like "config.values.bitmap". This looks fine with the exception that "bits" would be even shorter and the information about this field being a bitmap is not necessary. I hope this isn't too much bikeshedding. :) > + > +/** > + * struct gpioline_config - Configuration for GPIO lines > + * @values: if the direction is GPIOLINE_DIRECTION_OUTPUT, the values that > + * the lines will be set to. This field is write-only and is zeroed when > + * returned within struct gpioline_info. > + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW, > + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together > + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired > + * direction for the requested lines, with a value from enum > + * gpioline_direction > + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for > + * the requested lines, with a value from enum gpioline_drive > + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for > + * the requested lines, with a value from enum gpioline_bias > + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the > + * desired edge_detection for the requested lines, with a value from enum > + * gpioline_edge > + * @debounce_period: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the > + * desired debounce period for the requested lines, in microseconds > + * @padding: reserved for future use and must be zero filled > + */ > +struct gpioline_config { > + struct gpioline_values values; > + __u32 flags; > + /* Note that the following four fields are equivalent to a single u32. */ > + __u8 direction; > + __u8 drive; > + __u8 bias; > + __u8 edge_detection; Limiting the size of these enum fields doesn't improve performance in any measurable way. We already have 4 possible values for events. I'm afraid that at some point in the future we'll add support for level events or something else etc. and we'll run into a problem because we only have 8 bits available. If there are no objections, I'd make it 32 bits wide. For the same reason I was thinking that making flags 64 bits wouldn't hurt either. > + __u32 debounce_period; > + __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */ > +}; > + > +/** > + * struct gpioline_request - Information about a request for GPIO lines > + * @offsets: an array of desired lines, specified by offset index for the > + * associated GPIO device > + * @consumer: a desired consumer label for the selected GPIO lines such as > + * "my-bitbanged-relay" > + * @config: requested configuration for the requested lines. Note that even > + * if multiple lines are requested, the same configuration must be > + * applicable to all of them. If you want lines with individual > + * configuration, request them one by one. It is possible to select a batch > + * of input or output lines, but they must all have the same configuration, > + * i.e. all inputs or all outputs, all active low etc > + * @num_lines: number of lines requested in this request, i.e. the number > + * of valid fields in the GPIOLINES_MAX sized arrays, set to 1 to request a > + * single line > + * @event_buffer_size: a suggested minimum number of line events that the > + * kernel should buffer. This is only relevant if edge_detection is > + * enabled. Note that this is only a suggested value and the kernel may > + * allocate a larger buffer or cap the size of the buffer. If this field is > + * zero then the buffer size defaults to a minimum of num_lines*16. > + * @padding: reserved for future use and must be zero filled > + * @fd: if successful this field will contain a valid anonymous file handle > + * after a GPIO_GET_LINE_IOCTL operation, zero or negative value means > + * error > + */ > +struct gpioline_request { > + __u32 offsets[GPIOLINES_MAX]; > + char consumer[GPIO_MAX_NAME_SIZE]; > + struct gpioline_config config; > + __u32 num_lines; > + __u32 event_buffer_size; > + __u32 padding[GPIOLINE_REQUEST_PAD_SIZE]; /* for future use */ > + __s32 fd; > +}; > + > +/** > + * struct gpioline_info_v2 - Information about a certain GPIO line > + * @name: the name of this GPIO line, such as the output pin of the line on > + * the chip, a rail or a pin header name on a board, as specified by the > + * gpio chip, may be empty > + * @consumer: a functional name for the consumer of this GPIO line as set > + * by whatever is using it, will be empty if there is no current user but > + * may also be empty if the consumer doesn't set this up > + * @config: the configuration of the line. Note that the values field is > + * always zeroed - the line must be requested to read the values. > + * @offset: the local offset on this GPIO device, fill this in when > + * requesting the line information from the kernel > + * @padding: reserved for future use > + */ > +struct gpioline_info_v2 { > + char name[GPIO_MAX_NAME_SIZE]; > + char consumer[GPIO_MAX_NAME_SIZE]; > + struct gpioline_config config; > + __u32 offset; > + __u32 padding[GPIOLINE_INFO_V2_PAD_SIZE]; /* for future use */ > +}; > + > +/** > + * struct gpioline_info_changed_v2 - Information about a change in status > + * of a GPIO line > + * @info: updated line information > + * @timestamp: estimate of time of status change occurrence, in nanoseconds > + * and GPIOLINE_CHANGED_CONFIG > + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED > + * @padding: reserved for future use > + */ > +struct gpioline_info_changed_v2 { > + struct gpioline_info_v2 info; > + __u64 timestamp; > + __u32 event_type; > + __u32 padding[GPIOLINE_INFO_CHANGED_V2_PAD_SIZE]; /* for future use */ > +}; > + > +enum gpioline_event_id { > + GPIOLINE_EVENT_RISING_EDGE = 1, > + GPIOLINE_EVENT_FALLING_EDGE = 2, > +}; > + > +/** > + * struct gpioline_event - The actual event being pushed to userspace > + * @timestamp: best estimate of time of event occurrence, in nanoseconds. > + * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the > + * accurate measurement of the time between events. It does not provide > + * the wall-clock time. > + * @id: event identifier with value from enum gpioline_event_id > + * @offset: the offset of the line that triggered the event > + * @seqno: the sequence number for this event in the sequence of events for > + * all the lines in this line request > + * @line_seqno: the sequence number for this event in the sequence of > + * events on this particular line > + * @padding: reserved for future use > + */ > +struct gpioline_event { > + __u64 timestamp; > + __u32 id; > + __u32 offset; > + __u32 seqno; > + __u32 line_seqno; > + __u32 padding[GPIOLINE_EVENT_PAD_SIZE]; /* for future use */ Any reason for only having 64 bits of padding? 128 wouldn't change much, right? > +}; > + > +/* > + * ABI V1 > + */ > + > /* Informational flags */ > #define GPIOLINE_FLAG_KERNEL (1UL << 0) /* Line used by the kernel */ > #define GPIOLINE_FLAG_IS_OUT (1UL << 1) > @@ -149,8 +357,6 @@ struct gpiohandle_config { > __u32 padding[4]; /* padding for future use */ > }; > > -#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config) > - > /** > * struct gpiohandle_data - Information of values on a GPIO handle > * @values: when getting the state of lines this contains the current > @@ -161,9 +367,6 @@ struct gpiohandle_data { > __u8 values[GPIOHANDLES_MAX]; > }; > > -#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data) > -#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data) > - > /* Eventrequest flags */ > #define GPIOEVENT_REQUEST_RISING_EDGE (1UL << 0) > #define GPIOEVENT_REQUEST_FALLING_EDGE (1UL << 1) > @@ -207,11 +410,31 @@ struct gpioevent_data { > __u32 id; > }; > > +/* > + * V1 and V2 ioctl()s > + */ > #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info) > +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32) > + > +/* > + * V2 ioctl()s > + */ > +#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x05, struct gpioline_info_v2) > +#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x06, struct gpioline_info_v2) > +#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x07, struct gpioline_request) > +#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_config) > +#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_values) > +#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_values) > + > +/* > + * V1 ioctl()s > + */ > #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info) > -#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info) > -#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32) > #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request) > #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request) > +#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data) > +#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data) > +#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0A, struct gpiohandle_config) > +#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0B, struct gpioline_info) > > #endif /* _UAPI_GPIO_H_ */ > -- > 2.27.0 > Looks nice! Thanks again Kent! Bart
On Wed, Jun 24, 2020 at 05:33:26PM +0300, Andy Shevchenko wrote: > On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > Add a new version of the uAPI to address existing 32/64bit alignment > > I think using - would be nice, like 32/64-bit (or at least space like > 32/64 bit) as a common practice. > Fair enough. And you don't need to point out every single case - a catch all is sufficient. [ snip ] > > > +#include <linux/kernel.h> > > Perhaps keep it in order? > Haha, I didn't notice that one. > ... > > > + * Must be a multiple of 8 to ensure 32/64bit alignment of structs. > > Dash. And same for all cases like this. > And you are repeating yourself. Again ;-). > ... > > > +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */ > > the -> The ? > > > +#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64) > > Not sure we need this definition. The problem is that definitions in > the uAPI header are also part of uAPI. Here is just a calculus which > can be done manually since if anybody changes MAX, they will anyway > have to check if everything else is consistent. And yes, I'm not in > favour of including kernel.h here and there. > We are talking about include/uapi/linux/kernel.h, right? I thought headers in uapi/linux were fair game for other uapi headers. That is what they are there for right? Similarly, I thought those macros were there to avoid having the recalc manually. > ... > > > +/* > > + * Struct padding sizes. > > + * > > + * These are sized to pad structs to 64bit boundaries. > > + * To maintain 32/64bit alignment, any arbitrary change must be even, as > > + * the pad elements are __u32. > > + */ > > +#define GPIOLINE_CONFIG_PAD_SIZE 7 > > +#define GPIOLINE_REQUEST_PAD_SIZE 5 > > +#define GPIOLINE_INFO_V2_PAD_SIZE 5 > > +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5 > > +#define GPIOLINE_EVENT_PAD_SIZE 2 > > I'm not sure they (definitions) should be part of UAPI. Can't you > simple hard code numbers per case? > Ironically they are there as you wanted the padding sizes, and their impact on alignment, documented and it made sense to me to group them like this. > ... > > > +/** > > + * struct gpioline_config - Configuration for GPIO lines > > + */ > > +struct gpioline_config { > > + struct gpioline_values values; > > + __u32 flags; > > + /* Note that the following four fields are equivalent to a single u32. */ > > + __u8 direction; > > + __u8 drive; > > + __u8 bias; > > + __u8 edge_detection; > > + __u32 debounce_period; > > > + __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */ > > I would just put minimum here. If you need to extend you have to use > sizeof(struct) as a version of ABI. > That is doable, but gets complicated when you have embedded structs, as this one is in gpioline_request and gpioline_info_v2. To keep decoding simple on the kernel side, we would have to explode all the struct definitions so new fields are always added to the end. Or we would end up with rather complex decoding on the kernel side. We can always use that as a fallback if we run out of padding. > > +}; > > I'm wondering how many lines (in average) the user usually changes at > once? One? Two? > > Perhaps we need to be better with this, something like single line / > multiple lines? > > So, having a struct for single line change being embedded multiple > times would allow to configure atomically several lines with different > requirements. > For example you can turn directions of the two lines for some kind of > half-duplex bit banging protocol. > > I'm not sure about the rest, but to me it seems reasonable to have > single vs. multiple separation in some of the structures. > I agree in general, but not sure where to draw the line (pun not indended). Or how to provide a coherent API at a higher layer for that matter. Do you have a concrete use case you need a solution for? > ... > > > +/* > > + * ABI V1 > > V1 -> v1 > > And below V2 -> v2. > Fair enough. Cheers, Kent.
On Wed, Jun 24, 2020 at 04:36:22PM +0200, Bartosz Golaszewski wrote: > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > [ snip ] > > + > > +/* > > + * Struct padding sizes. > > + * > > + * These are sized to pad structs to 64bit boundaries. > > + * To maintain 32/64bit alignment, any arbitrary change must be even, as > > + * the pad elements are __u32. > > + */ > > +#define GPIOLINE_CONFIG_PAD_SIZE 7 > > +#define GPIOLINE_REQUEST_PAD_SIZE 5 > > +#define GPIOLINE_INFO_V2_PAD_SIZE 5 > > +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5 > > +#define GPIOLINE_EVENT_PAD_SIZE 2 > > + > > Did someone request such definitions? IMO it's one of those rare > instances where I'd prefer to see magic numbers used in the structs. > I'm not sure if we need this indirection. > Not explicitly. Andy requested documentation on the pad sizes, which seemed reasonable, and it also seemed reasonable to me to group them in the one place rather than repeat the doco for each struct. But having the size of the pad visible in the struct definition is preferable, as it is only really relevant to that struct. And everyone wants to keep the uAPI definitions as short possible, so will drop these in v2. > > +/** > > + * struct gpioline_values - Values of GPIO lines > > + * @bitmap: a bitmap containing the value of the lines, set to 1 for active > > + * and 0 for inactive. Note that this is the logical value, which will be > > + * the opposite of the physical value if GPIOLINE_FLAG_V2_ACTIVE_LOW is > > + * set in flags. > > + */ > > +struct gpioline_values { > > + __u64 bitmap[GPIOLINES_BITMAP_SIZE]; > > +}; > > Ok so now when embedding this structure we get something like > "config.values.bitmap". This looks fine with the exception that "bits" > would be even shorter and the information about this field being a > bitmap is not necessary. I hope this isn't too much bikeshedding. :) > Fair enough - bits it is. > > + > > +/** > > + * struct gpioline_config - Configuration for GPIO lines > > + * @values: if the direction is GPIOLINE_DIRECTION_OUTPUT, the values that > > + * the lines will be set to. This field is write-only and is zeroed when > > + * returned within struct gpioline_info. > > + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW, > > + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together > > + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired > > + * direction for the requested lines, with a value from enum > > + * gpioline_direction > > + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for > > + * the requested lines, with a value from enum gpioline_drive > > + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for > > + * the requested lines, with a value from enum gpioline_bias > > + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the > > + * desired edge_detection for the requested lines, with a value from enum > > + * gpioline_edge > > + * @debounce_period: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the > > + * desired debounce period for the requested lines, in microseconds > > + * @padding: reserved for future use and must be zero filled > > + */ > > +struct gpioline_config { > > + struct gpioline_values values; > > + __u32 flags; > > + /* Note that the following four fields are equivalent to a single u32. */ > > + __u8 direction; > > + __u8 drive; > > + __u8 bias; > > + __u8 edge_detection; > > Limiting the size of these enum fields doesn't improve performance in > any measurable way. We already have 4 possible values for events. I'm > afraid that at some point in the future we'll add support for level > events or something else etc. and we'll run into a problem because we > only have 8 bits available. If there are no objections, I'd make it 32 > bits wide. > Agreed - the reduction in size isn't worth the effort. > For the same reason I was thinking that making flags 64 bits wouldn't > hurt either. > OK [ snip ] > > + * struct gpioline_event - The actual event being pushed to userspace > > + * @timestamp: best estimate of time of event occurrence, in nanoseconds. > > + * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the > > + * accurate measurement of the time between events. It does not provide > > + * the wall-clock time. > > + * @id: event identifier with value from enum gpioline_event_id > > + * @offset: the offset of the line that triggered the event > > + * @seqno: the sequence number for this event in the sequence of events for > > + * all the lines in this line request > > + * @line_seqno: the sequence number for this event in the sequence of > > + * events on this particular line > > + * @padding: reserved for future use > > + */ > > +struct gpioline_event { > > + __u64 timestamp; > > + __u32 id; > > + __u32 offset; > > + __u32 seqno; > > + __u32 line_seqno; > > + __u32 padding[GPIOLINE_EVENT_PAD_SIZE]; /* for future use */ > > Any reason for only having 64 bits of padding? 128 wouldn't change much, right? > No, as I mentioned in the commentary, I'm concerned that I've gone too small with the padding, so happy to bump that. And I'm open to suggestions on the other pads as well. Cheers, Kent.
On Wed, Jun 24, 2020 at 11:40:57PM +0800, Kent Gibson wrote: > On Wed, Jun 24, 2020 at 05:33:26PM +0300, Andy Shevchenko wrote: > > On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > Add a new version of the uAPI to address existing 32/64bit alignment > > [ snip ] > > > > I'm wondering how many lines (in average) the user usually changes at > > once? One? Two? > > > > Perhaps we need to be better with this, something like single line / > > multiple lines? > > > > So, having a struct for single line change being embedded multiple > > times would allow to configure atomically several lines with different > > requirements. > > For example you can turn directions of the two lines for some kind of > > half-duplex bit banging protocol. > > > > I'm not sure about the rest, but to me it seems reasonable to have > > single vs. multiple separation in some of the structures. > > > I think you are right about this - we should be taking the opportunity to remove the homogeneous config restriction, so we can have a mixture of lines with different configs in the one request. e.g. I've had a request to have lines with different active low settings. Your example requires both inputs and outputs. And for a recent rotary encoder example I would've liked to be able to have edge detection on one line, but return a snapshot with the state of several lines in the edge event. So what I'm thinking is to replace the config that I had proposed with something like this: struct attrib { /* * the set of lines from request.offsets that this attrib DOESN'T * apply to. A zero value means it applies to all lines. * A set bit means the line request.offsets[bit] does NOT get this * attribute. */ __u64 mask[GPIOLINES_BITMAP_SIZE]; /* * an attribute identifier that dictates the which of the union * fields is valid and how it is interpreted. */ __u32 id; union { __u64 flags; /* similar to v1 handleflags + eventflags */ __u32 debounce_period; __u64 values[GPIOLINES_BITMAP_SIZE]; /* for id == output */ /* ... */ /* future config values go here */ /* * padding to ensure 32/64-bit alignment of attrib * * This must be the largest sized value. */ __u32 padding[3]; }; }; /* config is a stack of attribs associated with requested lines. */ struct config { /* the number of populated attribs */ __u32 num_attribs; /* for 32/64-bit alignment */ __u32 padding; struct attrib attribs[MAX_CONFIG_ATTRIBS]; }; The idea is a stack of attributes, each of which can be applied to a subset of the requested lines, as determined by the attrib.mask. The config for a given attribute on a given line is determined by finding the first match while walking down the stack, or falling back to the sensible default if no match is found. To reduce the number of attributes required, a number of boolean or boolean-ish fields can be combined into flags, similar to v1. I'm guessing a handful of attribs would suffice for the vast majority of cases, so MAX_CONFIG_ATTRIBS would be in the 5-10 range. (Are we concerned about struct sizes?) Adding new config attribs in the future would involve adding a new id and a corresponding value in the union. So we can potentially add as many new config attribs as we like - though the user would still be limited to MAX_CONFIG_ATTRIBS and may have to trade-off attribs in their particular application. Does that make any sense? Cheers, Kent.
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h index 4e1139ab25bc..e4ed6f79e332 100644 --- a/include/uapi/linux/gpio.h +++ b/include/uapi/linux/gpio.h @@ -11,11 +11,14 @@ #ifndef _UAPI_GPIO_H_ #define _UAPI_GPIO_H_ +#include <linux/kernel.h> #include <linux/ioctl.h> #include <linux/types.h> /* * The maximum size of name and label arrays. + * + * Must be a multiple of 8 to ensure 32/64bit alignment of structs. */ #define GPIO_MAX_NAME_SIZE 32 @@ -32,6 +35,211 @@ struct gpiochip_info { __u32 lines; }; +/* + * Maximum number of requested lines. + * + * Must be a multiple of 8 to ensure 32/64bit alignment of structs. + */ +#define GPIOLINES_MAX 64 + +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */ +#define GPIOLINES_BITMAP_SIZE __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64) + +enum gpioline_direction { + GPIOLINE_DIRECTION_INPUT = 0, + GPIOLINE_DIRECTION_OUTPUT = 1, +}; + +enum gpioline_drive { + GPIOLINE_DRIVE_PUSH_PULL = 0, + GPIOLINE_DRIVE_OPEN_DRAIN = 1, + GPIOLINE_DRIVE_OPEN_SOURCE = 2, +}; + +enum gpioline_bias { + GPIOLINE_BIAS_DISABLED = 0, + GPIOLINE_BIAS_PULL_UP = 1, + GPIOLINE_BIAS_PULL_DOWN = 2, +}; + +enum gpioline_edge { + GPIOLINE_EDGE_NONE = 0, + GPIOLINE_EDGE_RISING = 1, + GPIOLINE_EDGE_FALLING = 2, + GPIOLINE_EDGE_BOTH = 3, +}; + +/* Line flags - V2 */ +#define GPIOLINE_FLAG_V2_USED (1UL << 0) /* line is not available for request */ +#define GPIOLINE_FLAG_V2_ACTIVE_LOW (1UL << 1) +#define GPIOLINE_FLAG_V2_DIRECTION (1UL << 2) +#define GPIOLINE_FLAG_V2_DRIVE (1UL << 3) +#define GPIOLINE_FLAG_V2_BIAS (1UL << 4) +#define GPIOLINE_FLAG_V2_EDGE_DETECTION (1UL << 5) +#define GPIOLINE_FLAG_V2_DEBOUNCE (1UL << 6) + +/* + * Struct padding sizes. + * + * These are sized to pad structs to 64bit boundaries. + * To maintain 32/64bit alignment, any arbitrary change must be even, as + * the pad elements are __u32. + */ +#define GPIOLINE_CONFIG_PAD_SIZE 7 +#define GPIOLINE_REQUEST_PAD_SIZE 5 +#define GPIOLINE_INFO_V2_PAD_SIZE 5 +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE 5 +#define GPIOLINE_EVENT_PAD_SIZE 2 + +/** + * struct gpioline_values - Values of GPIO lines + * @bitmap: a bitmap containing the value of the lines, set to 1 for active + * and 0 for inactive. Note that this is the logical value, which will be + * the opposite of the physical value if GPIOLINE_FLAG_V2_ACTIVE_LOW is + * set in flags. + */ +struct gpioline_values { + __u64 bitmap[GPIOLINES_BITMAP_SIZE]; +}; + +/** + * struct gpioline_config - Configuration for GPIO lines + * @values: if the direction is GPIOLINE_DIRECTION_OUTPUT, the values that + * the lines will be set to. This field is write-only and is zeroed when + * returned within struct gpioline_info. + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW, + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired + * direction for the requested lines, with a value from enum + * gpioline_direction + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for + * the requested lines, with a value from enum gpioline_drive + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for + * the requested lines, with a value from enum gpioline_bias + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the + * desired edge_detection for the requested lines, with a value from enum + * gpioline_edge + * @debounce_period: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the + * desired debounce period for the requested lines, in microseconds + * @padding: reserved for future use and must be zero filled + */ +struct gpioline_config { + struct gpioline_values values; + __u32 flags; + /* Note that the following four fields are equivalent to a single u32. */ + __u8 direction; + __u8 drive; + __u8 bias; + __u8 edge_detection; + __u32 debounce_period; + __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */ +}; + +/** + * struct gpioline_request - Information about a request for GPIO lines + * @offsets: an array of desired lines, specified by offset index for the + * associated GPIO device + * @consumer: a desired consumer label for the selected GPIO lines such as + * "my-bitbanged-relay" + * @config: requested configuration for the requested lines. Note that even + * if multiple lines are requested, the same configuration must be + * applicable to all of them. If you want lines with individual + * configuration, request them one by one. It is possible to select a batch + * of input or output lines, but they must all have the same configuration, + * i.e. all inputs or all outputs, all active low etc + * @num_lines: number of lines requested in this request, i.e. the number + * of valid fields in the GPIOLINES_MAX sized arrays, set to 1 to request a + * single line + * @event_buffer_size: a suggested minimum number of line events that the + * kernel should buffer. This is only relevant if edge_detection is + * enabled. Note that this is only a suggested value and the kernel may + * allocate a larger buffer or cap the size of the buffer. If this field is + * zero then the buffer size defaults to a minimum of num_lines*16. + * @padding: reserved for future use and must be zero filled + * @fd: if successful this field will contain a valid anonymous file handle + * after a GPIO_GET_LINE_IOCTL operation, zero or negative value means + * error + */ +struct gpioline_request { + __u32 offsets[GPIOLINES_MAX]; + char consumer[GPIO_MAX_NAME_SIZE]; + struct gpioline_config config; + __u32 num_lines; + __u32 event_buffer_size; + __u32 padding[GPIOLINE_REQUEST_PAD_SIZE]; /* for future use */ + __s32 fd; +}; + +/** + * struct gpioline_info_v2 - Information about a certain GPIO line + * @name: the name of this GPIO line, such as the output pin of the line on + * the chip, a rail or a pin header name on a board, as specified by the + * gpio chip, may be empty + * @consumer: a functional name for the consumer of this GPIO line as set + * by whatever is using it, will be empty if there is no current user but + * may also be empty if the consumer doesn't set this up + * @config: the configuration of the line. Note that the values field is + * always zeroed - the line must be requested to read the values. + * @offset: the local offset on this GPIO device, fill this in when + * requesting the line information from the kernel + * @padding: reserved for future use + */ +struct gpioline_info_v2 { + char name[GPIO_MAX_NAME_SIZE]; + char consumer[GPIO_MAX_NAME_SIZE]; + struct gpioline_config config; + __u32 offset; + __u32 padding[GPIOLINE_INFO_V2_PAD_SIZE]; /* for future use */ +}; + +/** + * struct gpioline_info_changed_v2 - Information about a change in status + * of a GPIO line + * @info: updated line information + * @timestamp: estimate of time of status change occurrence, in nanoseconds + * and GPIOLINE_CHANGED_CONFIG + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED + * @padding: reserved for future use + */ +struct gpioline_info_changed_v2 { + struct gpioline_info_v2 info; + __u64 timestamp; + __u32 event_type; + __u32 padding[GPIOLINE_INFO_CHANGED_V2_PAD_SIZE]; /* for future use */ +}; + +enum gpioline_event_id { + GPIOLINE_EVENT_RISING_EDGE = 1, + GPIOLINE_EVENT_FALLING_EDGE = 2, +}; + +/** + * struct gpioline_event - The actual event being pushed to userspace + * @timestamp: best estimate of time of event occurrence, in nanoseconds. + * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the + * accurate measurement of the time between events. It does not provide + * the wall-clock time. + * @id: event identifier with value from enum gpioline_event_id + * @offset: the offset of the line that triggered the event + * @seqno: the sequence number for this event in the sequence of events for + * all the lines in this line request + * @line_seqno: the sequence number for this event in the sequence of + * events on this particular line + * @padding: reserved for future use + */ +struct gpioline_event { + __u64 timestamp; + __u32 id; + __u32 offset; + __u32 seqno; + __u32 line_seqno; + __u32 padding[GPIOLINE_EVENT_PAD_SIZE]; /* for future use */ +}; + +/* + * ABI V1 + */ + /* Informational flags */ #define GPIOLINE_FLAG_KERNEL (1UL << 0) /* Line used by the kernel */ #define GPIOLINE_FLAG_IS_OUT (1UL << 1) @@ -149,8 +357,6 @@ struct gpiohandle_config { __u32 padding[4]; /* padding for future use */ }; -#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config) - /** * struct gpiohandle_data - Information of values on a GPIO handle * @values: when getting the state of lines this contains the current @@ -161,9 +367,6 @@ struct gpiohandle_data { __u8 values[GPIOHANDLES_MAX]; }; -#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data) -#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data) - /* Eventrequest flags */ #define GPIOEVENT_REQUEST_RISING_EDGE (1UL << 0) #define GPIOEVENT_REQUEST_FALLING_EDGE (1UL << 1) @@ -207,11 +410,31 @@ struct gpioevent_data { __u32 id; }; +/* + * V1 and V2 ioctl()s + */ #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info) +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32) + +/* + * V2 ioctl()s + */ +#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x05, struct gpioline_info_v2) +#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x06, struct gpioline_info_v2) +#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x07, struct gpioline_request) +#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_config) +#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_values) +#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_values) + +/* + * V1 ioctl()s + */ #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info) -#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info) -#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32) #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request) #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request) +#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data) +#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data) +#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0A, struct gpiohandle_config) +#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0B, struct gpioline_info) #endif /* _UAPI_GPIO_H_ */
Add a new version of the uAPI to address existing 32/64bit alignment issues, add support for debounce and event sequence numbers, and provide some future proofing by adding padding reserved for future use. The alignment issue relates to the gpioevent_data, which packs to different sizes on 32bit and 64bit platforms. That creates problems for 32bit apps running on 64bit kernels. The patch addresses that particular issue, and the problem more generally, by adding pad fields that explicitly pad structs out to 64bit boundaries, so they will pack to the same size now, and even if some of the reserved padding is used for __u64 fields in the future. The lack of future proofing in V1 makes it impossible to, for example, add the debounce feature that is included in v2. The future proofing is addressed by providing reserved padding in all structs for future features. Specifically, the line request, config, info, info_changed and event structs receive updated versions, and the first three new ioctls. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- I haven't added any padding to gpiochip_info, as I haven't seen any calls for new features for the corresponding ioctl, but I'm open to updating that as well. As the majority of the structs and ioctls were being replaced, it seemed opportune to rework some of the other aspects of the uAPI. Firstly, I've reworked the flags field throughout. V1 has three different flags fields, each with their own separate bit definitions. In V2 that is collapsed to one. Further, the bits of the V2 flags field are used as feature enable flags, with any other necessary configuration fields encoded separately. This is simpler and clearer, while also providing a foundation for adding features in the future. I've also merged the handle and event requests into a single request, the line request, as the two requests where mostly the same, other than the edge detection provided by event requests. As a byproduct, the V2 uAPI allows for multiple lines producing edge events on the same line handle. This is a new capability as V1 only supports a single line in an event request. This means there are now only two types of file handle to be concerned with, the chip and the line, and it is clearer which ioctls apply to which type of handle. There is also some minor renaming of fields for consistency compared to their V1 counterparts, e.g. offset rather than lineoffset or line_offset, and consumer rather than consumer_label. Additionally, V1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in V2 for clarity, and the gpiohandle_data __u8 array becomes a bitmap gpioline_values. The V2 uAPI is mostly just a reorganisation of V1, so userspace code, particularly libgpiod, should easily port to it. The padding sizes have been carried over from the RFC version, although the seqnos added to the gpioline_event alone would've used the all of the padding for that struct, had they not been added here. So I'm moderatly concerned that those values are too small due to a lack of imagination on may part and should be increased to decrease the probability of running out of space in the padding and requiring creative solutions or even a V3. Changes since the RFC: - document the constraints on array sizes to maintain 32/64 alignment - add sequence numbers to gpioline_event - use bitmap for values instead of array of __u8 - gpioline_info_v2 contains gpioline_config instead of its composite fields - provide constants for all array sizes, especially padding - renamed "GPIOLINE_FLAG_V2_KERNEL" to "GPIOLINE_FLAG_V2_USED" - renamed "default_values" to "values" - made gpioline_direction zero based - document clock used in gpioline_event timestamp - add event_buffer_size to gpioline_request include/uapi/linux/gpio.h | 237 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 230 insertions(+), 7 deletions(-)