LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 00/18] gpio: cdev: add uAPI V2
@ 2020-07-25  4:19 Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 01/18] gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes Kent Gibson
                   ` (17 more replies)
  0 siblings, 18 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

This patchset defines and implements adds a new version of the
GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add
support for debounce, event sequence numbers, and allowing for requested
lines with different configurations.
It provides some future proofing by adding optional configuration fields
and padding reserved for future use.

The series can be partitioned into two sets; the first eleven
contain the V2 uAPI implementation, and the final seven port
the GPIO tools to the V2 uAPI and extend them to use new uAPI features.

The more complicated patches include their own commentary where
appropriate.

Cheers,
Kent.

Changes since v1:
 - split out cleanup patches into a separate series.
 - split implementation patch into a patch for each ioctl or major feature.
 - split tool port patch into a patch per tool.
 - rework uAPI to allow requested lines with different configurations.

Kent Gibson (18):
  gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes
  gpio: uapi: define uAPI v2
  gpiolib: make cdev a build option
  gpiolib: add build option for CDEV v1 ABI
  gpiolib: cdev: support GPIO_GET_LINE_IOCTL and
    GPIOLINE_GET_VALUES_IOCTL
  gpiolib: cdev: support GPIO_GET_LINEINFO_V2_IOCTL and
    GPIO_GET_LINEINFO_WATCH_V2_IOCTL
  gpiolib: cdev: support edge detection for uAPI v2
  gpiolib: cdev: support GPIOLINE_SET_CONFIG_IOCTL
  gpiolib: cdev: support GPIOLINE_SET_VALUES_IOCTL
  gpiolib: cdev: support setting debounce
  gpio: uapi: document uAPI v1 as deprecated
  tools: gpio: port lsgpio to v2 uAPI
  tools: gpio: port gpio-watch to v2 uAPI
  tools: gpio: rename nlines to num_lines
  tools: gpio: port gpio-hammer to v2 uAPI
  tools: gpio: port gpio-event-mon to v2 uAPI
  tools: gpio: add debounce support to gpio-event-mon
  tools: gpio: add multi-line monitoring to gpio-event-mon

 drivers/gpio/Kconfig        |   28 +-
 drivers/gpio/Makefile       |    2 +-
 drivers/gpio/gpiolib-cdev.c | 1296 ++++++++++++++++++++++++++++++++++-
 drivers/gpio/gpiolib-cdev.h |   15 +
 drivers/gpio/gpiolib.c      |    2 +
 drivers/gpio/gpiolib.h      |    6 +
 include/uapi/linux/gpio.h   |  327 ++++++++-
 tools/gpio/gpio-event-mon.c |  137 ++--
 tools/gpio/gpio-hammer.c    |   27 +-
 tools/gpio/gpio-utils.c     |  117 ++--
 tools/gpio/gpio-utils.h     |   48 +-
 tools/gpio/gpio-watch.c     |   10 +-
 tools/gpio/lsgpio.c         |  102 ++-
 13 files changed, 1882 insertions(+), 235 deletions(-)


base-commit: 8fc3ed3a474d76cd76dd0a154ea904373e9a5530
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 01/18] gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 02/18] gpio: uapi: define uAPI v2 Kent Gibson
                   ` (16 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Replace constant array sizes with a macro constant to clarify the source
of array sizes, provide a place to document any constraints on the size,
and to simplify array sizing in userspace if constructing structs
from their composite fields.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

This change is not terribly important for V1, but in adding V2 more
documentation for the usage of this value is appropriate.
As it is also used with V1 it warrants a separate patch.

 include/uapi/linux/gpio.h | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 9c27cecf406f..285cc10355b2 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -14,6 +14,11 @@
 #include <linux/ioctl.h>
 #include <linux/types.h>
 
+/*
+ * The maximum size of name and label arrays.
+ */
+#define GPIO_MAX_NAME_SIZE 32
+
 /**
  * struct gpiochip_info - Information about a certain GPIO chip
  * @name: the Linux kernel name of this GPIO chip
@@ -22,8 +27,8 @@
  * @lines: number of GPIO lines on this chip
  */
 struct gpiochip_info {
-	char name[32];
-	char label[32];
+	char name[GPIO_MAX_NAME_SIZE];
+	char label[GPIO_MAX_NAME_SIZE];
 	__u32 lines;
 };
 
@@ -52,8 +57,8 @@ struct gpiochip_info {
 struct gpioline_info {
 	__u32 line_offset;
 	__u32 flags;
-	char name[32];
-	char consumer[32];
+	char name[GPIO_MAX_NAME_SIZE];
+	char consumer[GPIO_MAX_NAME_SIZE];
 };
 
 /* Maximum number of requested handles */
@@ -123,7 +128,7 @@ struct gpiohandle_request {
 	__u32 lineoffsets[GPIOHANDLES_MAX];
 	__u32 flags;
 	__u8 default_values[GPIOHANDLES_MAX];
-	char consumer_label[32];
+	char consumer_label[GPIO_MAX_NAME_SIZE];
 	__u32 lines;
 	int fd;
 };
@@ -182,7 +187,7 @@ struct gpioevent_request {
 	__u32 lineoffset;
 	__u32 handleflags;
 	__u32 eventflags;
-	char consumer_label[32];
+	char consumer_label[GPIO_MAX_NAME_SIZE];
 	int fd;
 };
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 02/18] gpio: uapi: define uAPI v2
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 01/18] gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-08-04 17:42   ` Bartosz Golaszewski
  2020-07-25  4:19 ` [PATCH v2 03/18] gpiolib: make cdev a build option Kent Gibson
                   ` (15 subsequent siblings)
  17 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add a new version of the uAPI to address existing 32/64-bit 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 32-bit and 64-bit platforms. That creates problems for 32-bit apps
running on 64-bit kernels.  The patch addresses that particular issue, and
the problem more generally, by adding pad fields that explicitly pad
structs out to 64-bit 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.

I've also merged the handle and event requests into a single request, the
line request, as the two requests were 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.

Changes since v1:
 - lower case V1 and V2, except in capitalized names
 - hyphenate 32/64-bit
 - rename bitmap field to bits
 - drop PAD_SIZE consts in favour of hard coded numbers
 - sort includes
 - change config flags to __u64
 - increase padding of gpioline_event
 - relocate GPIOLINE_CHANGED enum into v2 section (is common with v1)
 - rework config to collapse direction, drive, bias and edge enums back
   into flags and add optional attributes that can be associated with a
   subset of the requested lines.

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
 - rename debounce to debounce_period
 - rename lines to num_lines
 
 include/uapi/linux/gpio.h | 284 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 270 insertions(+), 14 deletions(-)

diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 285cc10355b2..3f6db33014f0 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -12,10 +12,13 @@
 #define _UAPI_GPIO_H_
 
 #include <linux/ioctl.h>
+#include <linux/kernel.h>
 #include <linux/types.h>
 
 /*
  * The maximum size of name and label arrays.
+ *
+ * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
  */
 #define GPIO_MAX_NAME_SIZE 32
 
@@ -32,6 +35,251 @@ struct gpiochip_info {
 	__u32 lines;
 };
 
+/*
+ * Maximum number of requested lines.
+ *
+ * Must be a multiple of 8 to ensure 32/64-bit 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)
+
+/*
+ * The maximum number of configuration attributes associated with a line
+ * request.
+ */
+#define GPIOLINE_NUM_ATTRS_MAX 10
+
+/**
+ * enum gpioline_flag_v2 - &struct gpioline_attribute.flags values
+ */
+enum gpioline_flag_v2 {
+	GPIOLINE_FLAG_V2_USED			= 1UL << 0, /* line is not available for request */
+	GPIOLINE_FLAG_V2_ACTIVE_LOW		= 1UL << 1,
+	GPIOLINE_FLAG_V2_INPUT			= 1UL << 2,
+	GPIOLINE_FLAG_V2_OUTPUT			= 1UL << 3,
+	GPIOLINE_FLAG_V2_EDGE_RISING		= 1UL << 4,
+	GPIOLINE_FLAG_V2_EDGE_FALLING		= 1UL << 5,
+	GPIOLINE_FLAG_V2_OPEN_DRAIN		= 1UL << 6,
+	GPIOLINE_FLAG_V2_OPEN_SOURCE		= 1UL << 7,
+	GPIOLINE_FLAG_V2_BIAS_PULL_UP		= 1UL << 8,
+	GPIOLINE_FLAG_V2_BIAS_PULL_DOWN		= 1UL << 9,
+	GPIOLINE_FLAG_V2_BIAS_DISABLED		= 1UL << 10,
+};
+
+/**
+ * struct gpioline_values - Values of GPIO lines
+ * @bits: 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 the line is configured as active
+ * low.
+ */
+struct gpioline_values {
+	__u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+
+/**
+ * struct gpioline_set_values - Values to set a group of GPIO lines
+ * @mask: a bitmap identifying the lines to set.
+ * @bits: 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 the line is configured as active
+ * low.
+ */
+struct gpioline_set_values {
+	__u64 mask[GPIOLINES_BITMAP_SIZE];
+	__u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+
+/**
+ * enum gpioline_attr_id - &struct gpioline_attribute.id values
+ */
+enum gpioline_attr_id {
+	GPIOLINE_ATTR_ID_FLAGS			= 1,
+	GPIOLINE_ATTR_ID_OUTPUT_VALUES		= 2,
+	GPIOLINE_ATTR_ID_DEBOUNCE		= 3,
+};
+
+/**
+ * struct gpioline_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpioline_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIOLINE_ATTR_ID_FLAGS, the flags for the GPIO line,
+ * with values from enum gpioline_flag_v2, such as
+ * GPIOLINE_FLAG_V2_ACTIVE_LOW, GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed
+ * together.  This overrides the default flags contained in the &struct
+ * gpioline_config for the associated line.
+ * @values: if id is GPIOLINE_ATTR_ID_OUTPUT_VALUES, the values to which
+ * the lines will be set
+ * @debounce_period: if id is GPIOLINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds
+ */
+struct gpioline_attribute {
+	__u32 id;
+	__u32 padding;
+	union {
+		__u64 flags;
+		struct gpioline_values values;
+		__u32 debounce_period;
+	};
+};
+
+/**
+ * struct gpioline_config_attribute - a configuration attribute associated
+ * with one or more of the requested lines.
+ * @mask: a bitmap identifying the lines to which the attribute applies
+ * @attr: the configurable attribute
+ */
+struct gpioline_config_attribute {
+	__u64 mask[GPIOLINES_BITMAP_SIZE];
+	struct gpioline_attribute attr;
+};
+
+/**
+ * struct gpioline_config - Configuration for GPIO lines
+ * @flags: flags for the GPIO lines, with values from enum
+ * gpioline_flag_v2, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
+ * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together.  This is the default for
+ * all requested lines but may be overridden for particular lines using
+ * attrs.
+ * @num_attrs: the number of attributes in attrs
+ * @padding: reserved for future use and must be zero filled
+ * @attrs: the configuration attributes associated with the requested
+ * lines.
+ */
+struct gpioline_config {
+	__u64 flags;
+	__u32 num_attrs;
+	/*
+	 * Pad to fill implicit padding and provide space for future use.
+	 */
+	__u32 padding[5];
+	struct gpioline_config_attribute attrs[GPIOLINE_NUM_ATTRS_MAX];
+};
+
+/**
+ * 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 lines.
+ * @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 in the configuration. 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;
+	/*
+	 * Pad struct to 64-bit boundary and provide space for future use.
+	 */
+	__u32 padding[5];
+	__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
+ * @flags: flags for the GPIO line, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
+ * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together
+ * @offset: the local offset on this GPIO device, fill this in when
+ * requesting the line information from the kernel
+ * @num_attrs: the number of attributes in attrs
+ * @attrs: the configuration attributes associated with the line.
+ * @padding: reserved for future use
+ */
+struct gpioline_info_v2 {
+	char name[GPIO_MAX_NAME_SIZE];
+	char consumer[GPIO_MAX_NAME_SIZE];
+	__u32 offset;
+	__u32 num_attrs;
+	__u64 flags;
+	struct gpioline_attribute attrs[GPIOLINE_NUM_ATTRS_MAX];
+	/*
+	 * Pad struct to 64-bit boundary and provide space for future use.
+	 */
+	__u32 padding[4];
+};
+
+enum gpioline_changed_type {
+	GPIOLINE_CHANGED_REQUESTED	= 1,
+	GPIOLINE_CHANGED_RELEASED	= 2,
+	GPIOLINE_CHANGED_CONFIG		= 3,
+};
+
+/**
+ * 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
+ * @event_type: the type of change with a value from enum gpioline_changed_type
+ * @padding: reserved for future use
+ */
+struct gpioline_info_changed_v2 {
+	struct gpioline_info_v2 info;
+	__u64 timestamp;
+	__u32 event_type;
+	/*
+	 * Pad struct to 64-bit boundary and provide space for future use.
+	 */
+	__u32 padding[5];
+};
+
+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;
+	/*
+	 * Pad struct to 64-bit boundary and provide space for future use.
+	 */
+	__u32 padding[6];
+};
+
+/*
+ *  ABI v1
+ */
+
 /* Informational flags */
 #define GPIOLINE_FLAG_KERNEL		(1UL << 0) /* Line used by the kernel */
 #define GPIOLINE_FLAG_IS_OUT		(1UL << 1)
@@ -64,13 +312,6 @@ struct gpioline_info {
 /* Maximum number of requested handles */
 #define GPIOHANDLES_MAX 64
 
-/* Possible line status change events */
-enum {
-	GPIOLINE_CHANGED_REQUESTED = 1,
-	GPIOLINE_CHANGED_RELEASED,
-	GPIOLINE_CHANGED_CONFIG,
-};
-
 /**
  * struct gpioline_info_changed - Information about a change in status
  * of a GPIO line
@@ -149,8 +390,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 +400,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 +443,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_set_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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 03/18] gpiolib: make cdev a build option
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 01/18] gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 02/18] gpio: uapi: define uAPI v2 Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25 20:29   ` Andy Shevchenko
  2020-07-26 22:25   ` Linus Walleij
  2020-07-25  4:19 ` [PATCH v2 04/18] gpiolib: add build option for CDEV v1 ABI Kent Gibson
                   ` (14 subsequent siblings)
  17 siblings, 2 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Make the gpiolib-cdev module a build option.  This allows the CDEV
interface to be removed from the kernel to reduce kernel size in
applications where is it not required, and provides the parent for
other other CDEV interface specific build options to follow.

Suggested-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/Kconfig        | 16 ++++++++++++++--
 drivers/gpio/Makefile       |  2 +-
 drivers/gpio/gpiolib-cdev.h | 15 +++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8030fd91a3cc..b5bb9efc1092 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -66,8 +66,20 @@ config GPIO_SYSFS
 
 	  This ABI is deprecated. If you want to use GPIO from userspace,
 	  use the character device /dev/gpiochipN with the appropriate
-	  ioctl() operations instead. The character device is always
-	  available.
+	  ioctl() operations instead.
+
+config GPIO_CDEV
+	bool "/dev/gpiochipN (character device interface)"
+	default y
+	help
+	  Say Y here to add the character device /dev/gpiochipN interface
+	  for GPIOs. The character device allows userspace to control GPIOs
+	  using ioctl() operations.
+
+	  Only say N is you are sure that the GPIO character device is not
+	  required.
+
+	  If unsure, say Y.
 
 config GPIO_GENERIC
 	depends on HAS_IOMEM # Only for IOMEM drivers
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4f9abff4f2dc..7c24c8d77068 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -7,8 +7,8 @@ obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-devres.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-legacy.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-devprop.o
-obj-$(CONFIG_GPIOLIB)		+= gpiolib-cdev.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
+obj-$(CONFIG_GPIO_CDEV)		+= gpiolib-cdev.o
 obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
 obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
 
diff --git a/drivers/gpio/gpiolib-cdev.h b/drivers/gpio/gpiolib-cdev.h
index 973578e7ad10..19a4e3d57120 100644
--- a/drivers/gpio/gpiolib-cdev.h
+++ b/drivers/gpio/gpiolib-cdev.h
@@ -5,7 +5,22 @@
 
 #include <linux/device.h>
 
+#ifdef CONFIG_GPIO_CDEV
+
 int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt);
 void gpiolib_cdev_unregister(struct gpio_device *gdev);
 
+#else
+
+static inline int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
+{
+	return 0;
+}
+
+static inline void gpiolib_cdev_unregister(struct gpio_device *gdev)
+{
+}
+
+#endif /* CONFIG_GPIO_CDEV */
+
 #endif /* GPIOLIB_CDEV_H */
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 04/18] gpiolib: add build option for CDEV v1 ABI
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (2 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 03/18] gpiolib: make cdev a build option Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL Kent Gibson
                   ` (13 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add a build option to allow the removal of the CDEV v1 ABI.

Suggested-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

This patch is before the V2 implementation, and is non-functional until
that patch, as some parts of that patch would be written slightly
differently if removing V1 was not considered.
Adding this patch after that would necessitate revisiting the V2 changes,
so this ordering results in two simpler patches.

 drivers/gpio/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b5bb9efc1092..d9775998a997 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -81,6 +81,18 @@ config GPIO_CDEV
 
 	  If unsure, say Y.
 
+config GPIO_CDEV_V1
+	bool "Support GPIO ABI Version 1"
+	default y
+	depends on GPIO_CDEV
+	help
+	  Say Y here to support version 1 of the GPIO CDEV ABI.
+
+	  This ABI version is deprecated and will be removed in the future.
+	  Please use the latest ABI for new developments.
+
+	  If unsure, say Y.
+
 config GPIO_GENERIC
 	depends on HAS_IOMEM # Only for IOMEM drivers
 	tristate
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (3 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 04/18] gpiolib: add build option for CDEV v1 ABI Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25 20:51   ` Andy Shevchenko
  2020-07-25  4:19 ` [PATCH v2 06/18] gpiolib: cdev: support GPIO_GET_LINEINFO_V2_IOCTL and GPIO_GET_LINEINFO_WATCH_V2_IOCTL Kent Gibson
                   ` (12 subsequent siblings)
  17 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and
returning their current values using GPIOLINE_GET_VALUES_IOCTL.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

The struct line implementation is based on the V1 struct linehandle
implementation.

The line_ioctl() is a simple wrapper around line_get_values() here, but
will be extended with other ioctls in subsequent patches.

 drivers/gpio/gpiolib-cdev.c | 389 ++++++++++++++++++++++++++++++++++++
 1 file changed, 389 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e6c9b78adfc2..0908ae117b7d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/anon_inodes.h>
+#include <linux/atomic.h>
 #include <linux/bitmap.h>
+#include <linux/build_bug.h>
 #include <linux/cdev.h>
 #include <linux/compat.h>
 #include <linux/device.h>
@@ -34,6 +36,7 @@
  * GPIO line handle management
  */
 
+#ifdef CONFIG_GPIO_CDEV_V1
 /**
  * struct linehandle_state - contains the state of a userspace handle
  * @gdev: the GPIO device the handle pertains to
@@ -376,6 +379,366 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	linehandle_free(lh);
 	return ret;
 }
+#endif /* CONFIG_GPIO_CDEV_V1 */
+
+/**
+ * struct line - contains the state of a userspace line request
+ * @gdev: the GPIO device the line request pertains to
+ * @label: consumer label used to tag descriptors
+ * @num_descs: the number of descriptors held in the descs array
+ * @descs: the GPIO descriptors held by this line request, with @num_descs
+ * elements.
+ */
+struct line {
+	struct gpio_device *gdev;
+	const char *label;
+	u32 num_descs;
+	/* descs must be last so it can be dynamically sized */
+	struct gpio_desc *descs[];
+};
+
+static bool padding_not_zeroed(__u32 *padding, int pad_size)
+{
+	int i, sum = 0;
+
+	for (i = 0; i < pad_size; i++)
+		sum |= padding[i];
+
+	return sum;
+}
+
+#define GPIOLINE_BIAS_FLAGS \
+	(GPIOLINE_FLAG_V2_BIAS_PULL_UP | \
+	 GPIOLINE_FLAG_V2_BIAS_PULL_DOWN | \
+	 GPIOLINE_FLAG_V2_BIAS_DISABLED)
+
+#define GPIOLINE_DIRECTION_FLAGS \
+	(GPIOLINE_FLAG_V2_INPUT | \
+	 GPIOLINE_FLAG_V2_OUTPUT)
+
+#define GPIOLINE_DRIVE_FLAGS \
+	(GPIOLINE_FLAG_V2_OPEN_DRAIN | \
+	 GPIOLINE_FLAG_V2_OPEN_SOURCE)
+
+#define GPIOLINE_VALID_FLAGS \
+	(GPIOLINE_FLAG_V2_ACTIVE_LOW | \
+	 GPIOLINE_DIRECTION_FLAGS | \
+	 GPIOLINE_DRIVE_FLAGS | \
+	 GPIOLINE_BIAS_FLAGS)
+
+static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
+{
+	int i;
+
+	for (i = lc->num_attrs - 1; i >= 0; i--) {
+		if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) &&
+		    test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))
+			return lc->attrs[i].attr.flags;
+	}
+	return lc->flags;
+}
+
+static int gpioline_config_output_value(struct gpioline_config *lc,
+					int line_idx)
+{
+	int i;
+
+	for (i = lc->num_attrs - 1; i >= 0; i--) {
+		if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_OUTPUT_VALUES) &&
+		    test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))
+			return test_bit(line_idx,
+				(unsigned long *)lc->attrs[i].attr.values.bits);
+	}
+	return 0;
+}
+
+static int gpioline_flags_validate(u64 flags)
+{
+	/* Return an error if an unknown flag is set */
+	if (flags & ~GPIOLINE_VALID_FLAGS)
+		return -EINVAL;
+
+	/*
+	 * Do not allow both INPUT & OUTPUT flags to be set as they are
+	 * contradictory.
+	 */
+	if ((flags & GPIOLINE_FLAG_V2_INPUT) &&
+	    (flags & GPIOLINE_FLAG_V2_OUTPUT))
+		return -EINVAL;
+
+	/*
+	 * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
+	 * the hardware actually supports enabling both at the same time the
+	 * electrical result would be disastrous.
+	 */
+	if ((flags & GPIOLINE_FLAG_V2_OPEN_DRAIN) &&
+	    (flags & GPIOLINE_FLAG_V2_OPEN_SOURCE))
+		return -EINVAL;
+
+	/* Drive requires explicit output direction. */
+	if ((flags & GPIOLINE_DRIVE_FLAGS) &&
+	    !(flags & GPIOLINE_FLAG_V2_OUTPUT))
+		return -EINVAL;
+
+	/* Bias requies explicit direction. */
+	if ((flags & GPIOLINE_BIAS_FLAGS) &&
+	    !(flags & GPIOLINE_DIRECTION_FLAGS))
+		return -EINVAL;
+
+	/* Only one bias flag can be set. */
+	if (((flags & GPIOLINE_FLAG_V2_BIAS_DISABLED) &&
+	     (flags & (GPIOLINE_FLAG_V2_BIAS_PULL_DOWN |
+		       GPIOLINE_FLAG_V2_BIAS_PULL_UP))) ||
+	    ((flags & GPIOLINE_FLAG_V2_BIAS_PULL_DOWN) &&
+	     (flags & GPIOLINE_FLAG_V2_BIAS_PULL_UP)))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int gpioline_config_validate(struct gpioline_config *lc, int num_lines)
+{
+	int i, ret;
+	u64 flags;
+
+	if (lc->num_attrs > GPIOLINE_NUM_ATTRS_MAX)
+		return -EINVAL;
+
+	if (padding_not_zeroed(lc->padding, ARRAY_SIZE(lc->padding)))
+		return -EINVAL;
+
+	for (i = 0; i < num_lines; i++) {
+		flags = gpioline_config_flags(lc, i);
+		ret = gpioline_flags_validate(flags);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static void gpioline_config_flags_to_desc_flags(u64 flags,
+						unsigned long *flagsp)
+{
+	assign_bit(FLAG_ACTIVE_LOW, flagsp,
+		   flags & GPIOLINE_FLAG_V2_ACTIVE_LOW);
+	if (flags & GPIOLINE_FLAG_V2_OUTPUT)
+		set_bit(FLAG_IS_OUT, flagsp);
+	else if (flags & GPIOLINE_FLAG_V2_INPUT)
+		clear_bit(FLAG_IS_OUT, flagsp);
+	assign_bit(FLAG_OPEN_DRAIN, flagsp,
+		   flags & GPIOLINE_FLAG_V2_OPEN_DRAIN);
+	assign_bit(FLAG_OPEN_SOURCE, flagsp,
+		   flags & GPIOLINE_FLAG_V2_OPEN_SOURCE);
+	assign_bit(FLAG_PULL_UP, flagsp,
+		   flags & GPIOLINE_FLAG_V2_BIAS_PULL_UP);
+	assign_bit(FLAG_PULL_DOWN, flagsp,
+		   flags & GPIOLINE_FLAG_V2_BIAS_PULL_DOWN);
+	assign_bit(FLAG_BIAS_DISABLE, flagsp,
+		   flags & GPIOLINE_FLAG_V2_BIAS_DISABLED);
+}
+
+static long line_get_values(struct line *line, void __user *ip)
+{
+	struct gpioline_values lv;
+	unsigned long *vals = (unsigned long *)lv.bits;
+	int ret;
+
+	/* NOTE: It's ok to read values of output lines. */
+	memset(&lv, 0, sizeof(lv));
+	ret = gpiod_get_array_value_complex(false,
+					    true,
+					    line->num_descs,
+					    line->descs,
+					    NULL,
+					    vals);
+	if (ret)
+		return ret;
+
+	if (copy_to_user(ip, &lv, sizeof(lv)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static long line_ioctl(struct file *file, unsigned int cmd,
+		       unsigned long arg)
+{
+	struct line *line = file->private_data;
+	void __user *ip = (void __user *)arg;
+
+	if (cmd == GPIOLINE_GET_VALUES_IOCTL)
+		return line_get_values(line, ip);
+
+	return -EINVAL;
+}
+
+#ifdef CONFIG_COMPAT
+static long line_ioctl_compat(struct file *file, unsigned int cmd,
+			      unsigned long arg)
+{
+	return line_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+static void line_free(struct line *line)
+{
+	int i;
+
+	for (i = 0; i < line->num_descs; i++) {
+		if (line->descs[i])
+			gpiod_free(line->descs[i]);
+	}
+	kfree(line->label);
+	put_device(&line->gdev->dev);
+	kfree(line);
+}
+
+static int line_release(struct inode *inode, struct file *file)
+{
+	struct line *line = file->private_data;
+
+	line_free(line);
+	return 0;
+}
+
+static const struct file_operations line_fileops = {
+	.release = line_release,
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.unlocked_ioctl = line_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = line_ioctl_compat,
+#endif
+};
+
+static int line_create(struct gpio_device *gdev, void __user *ip)
+{
+	struct gpioline_request linereq;
+	struct line *line;
+	struct file *file;
+	int fd, i, ret;
+	struct gpioline_config *lc;
+	unsigned long flags;
+
+	if (copy_from_user(&linereq, ip, sizeof(linereq)))
+		return -EFAULT;
+	if ((linereq.num_lines == 0) || (linereq.num_lines > GPIOLINES_MAX))
+		return -EINVAL;
+
+	if (padding_not_zeroed(linereq.padding, ARRAY_SIZE(linereq.padding)))
+		return -EINVAL;
+
+	lc = &linereq.config;
+	ret = gpioline_config_validate(lc, linereq.num_lines);
+	if (ret)
+		return ret;
+
+	line = kzalloc(struct_size(line, descs, linereq.num_lines),
+		       GFP_KERNEL);
+	if (!line)
+		return -ENOMEM;
+
+	line->gdev = gdev;
+	get_device(&gdev->dev);
+
+	/* Make sure this is terminated */
+	linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
+	if (strlen(linereq.consumer)) {
+		line->label = kstrdup(linereq.consumer, GFP_KERNEL);
+		if (!line->label) {
+			ret = -ENOMEM;
+			goto out_free_line;
+		}
+	}
+
+	line->num_descs = linereq.num_lines;
+
+	/* Request each GPIO */
+	for (i = 0; i < linereq.num_lines; i++) {
+		u32 offset = linereq.offsets[i];
+		struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
+
+		if (IS_ERR(desc)) {
+			ret = PTR_ERR(desc);
+			goto out_free_line;
+		}
+
+		ret = gpiod_request(desc, line->label);
+		if (ret)
+			goto out_free_line;
+
+		line->descs[i] = desc;
+		flags = gpioline_config_flags(lc, i);
+		gpioline_config_flags_to_desc_flags(flags, &desc->flags);
+
+		ret = gpiod_set_transitory(desc, false);
+		if (ret < 0)
+			goto out_free_line;
+
+		/*
+		 * Lines have to be requested explicitly for input
+		 * or output, else the line will be treated "as is".
+		 */
+		if (flags & GPIOLINE_FLAG_V2_OUTPUT) {
+			int val = gpioline_config_output_value(lc, i);
+
+			ret = gpiod_direction_output(desc, val);
+			if (ret)
+				goto out_free_line;
+		} else if (flags & GPIOLINE_FLAG_V2_INPUT) {
+			ret = gpiod_direction_input(desc);
+			if (ret)
+				goto out_free_line;
+		}
+
+		blocking_notifier_call_chain(&desc->gdev->notifier,
+					     GPIOLINE_CHANGED_REQUESTED, desc);
+
+		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+			offset);
+	}
+
+	fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+	if (fd < 0) {
+		ret = fd;
+		goto out_free_line;
+	}
+
+	file = anon_inode_getfile("gpio-line",
+				  &line_fileops,
+				  line,
+				  O_RDONLY | O_CLOEXEC);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto out_put_unused_fd;
+	}
+
+	linereq.fd = fd;
+	if (copy_to_user(ip, &linereq, sizeof(linereq))) {
+		/*
+		 * fput() will trigger the release() callback, so do not go onto
+		 * the regular error cleanup path here.
+		 */
+		fput(file);
+		put_unused_fd(fd);
+		return -EFAULT;
+	}
+
+	fd_install(fd, file);
+
+	dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
+		line->num_descs);
+
+	return 0;
+
+out_put_unused_fd:
+	put_unused_fd(fd);
+out_free_line:
+	line_free(line);
+	return ret;
+}
+
+#ifdef CONFIG_GPIO_CDEV_V1
 
 /*
  * GPIO line event management
@@ -745,6 +1108,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	return ret;
 }
 
+#endif /* CONFIG_GPIO_CDEV_V1 */
+
 static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 				  struct gpioline_info *info)
 {
@@ -850,6 +1215,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
 			return -EFAULT;
 		return 0;
+#ifdef CONFIG_GPIO_CDEV_V1
 	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
 		struct gpioline_info lineinfo;
 
@@ -892,6 +1258,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 
 		return 0;
+#endif /* CONFIG_GPIO_CDEV_V1 */
+	} else if (cmd == GPIO_GET_LINE_IOCTL) {
+		return line_create(gdev, ip);
 	} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
 		if (copy_from_user(&offset, ip, sizeof(offset)))
 			return -EFAULT;
@@ -1118,4 +1487,24 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 void gpiolib_cdev_unregister(struct gpio_device *gdev)
 {
 	cdev_device_del(&gdev->chrdev, &gdev->dev);
+
+	/*
+	 * array sizes must be a multiple of 8 to ensure 64-bit alignment and
+	 * to not create holes in the struct packing.
+	 */
+	BUILD_BUG_ON(GPIOLINES_MAX % 8);
+	BUILD_BUG_ON(GPIO_MAX_NAME_SIZE % 8);
+
+	/*
+	 * check that uAPI structs are 64-bit aligned for 32/64-bit
+	 * compatibility
+	 */
+	BUILD_BUG_ON(sizeof(struct gpioline_attribute) % 8);
+	BUILD_BUG_ON(sizeof(struct gpioline_config_attribute) % 8);
+	BUILD_BUG_ON(sizeof(struct gpioline_config) % 8);
+	BUILD_BUG_ON(sizeof(struct gpioline_request) % 8);
+	BUILD_BUG_ON(sizeof(struct gpioline_info_v2) % 8);
+	BUILD_BUG_ON(sizeof(struct gpioline_info_changed_v2) % 8);
+	BUILD_BUG_ON(sizeof(struct gpioline_event) % 8);
+	BUILD_BUG_ON(sizeof(struct gpioline_values) % 8);
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 06/18] gpiolib: cdev: support GPIO_GET_LINEINFO_V2_IOCTL and GPIO_GET_LINEINFO_WATCH_V2_IOCTL
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (4 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 07/18] gpiolib: cdev: support edge detection for uAPI v2 Kent Gibson
                   ` (11 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add support for GPIO_GET_LINEINFO_V2_IOCTL and
GPIO_GET_LINEINFO_WATCH_V2_IOCTL.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

The core of this change is the event kfifo switching to contain
struct gpioline_info_changed_v2, instead of V1 as V2 is richer.

The two uAPI versions are mostly independent - other than where they both
provide line info changes via reads on the chip fd.  As the info change
structs differ between V1 and V2, the infowatch implementation tracks which
version of the infowatch ioctl, either GPIO_GET_LINEINFO_WATCH_IOCTL or
GPIO_GET_LINEINFO_WATCH_V2_IOCTL, initiates the initial watch and returns
the corresponding info change struct to the read.  The version supported
on that fd locks to that version on the first watch request, so subsequent
watches from that process must use the same uAPI version.

drivers/gpio/gpiolib-cdev.c | 187 +++++++++++++++++++++++++++++++-----
 1 file changed, 164 insertions(+), 23 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0908ae117b7d..1f282207fb70 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1108,10 +1108,52 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	return ret;
 }
 
+static void gpioline_info_v2_to_v1(struct gpioline_info_v2 *info_v2,
+				   struct gpioline_info *info_v1)
+{
+	int flagsv2 = info_v2->flags;
+
+	strncpy(info_v1->name, info_v2->name, sizeof(info_v1->name));
+	strncpy(info_v1->consumer, info_v2->consumer,
+		sizeof(info_v1->consumer));
+	info_v1->line_offset = info_v2->offset;
+	info_v1->flags = 0;
+
+	if (flagsv2 & GPIOLINE_FLAG_V2_USED)
+		info_v1->flags |= GPIOLINE_FLAG_KERNEL;
+
+	if (flagsv2 & GPIOLINE_FLAG_V2_OUTPUT)
+		info_v1->flags |= GPIOLINE_FLAG_IS_OUT;
+
+	if (flagsv2 & GPIOLINE_FLAG_V2_ACTIVE_LOW)
+		info_v1->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+
+	if (flagsv2 & GPIOLINE_FLAG_V2_OPEN_DRAIN)
+		info_v1->flags |= GPIOLINE_FLAG_OPEN_DRAIN;
+	if (flagsv2 & GPIOLINE_FLAG_V2_OPEN_SOURCE)
+		info_v1->flags |= GPIOLINE_FLAG_OPEN_SOURCE;
+
+	if (flagsv2 & GPIOLINE_FLAG_V2_BIAS_PULL_UP)
+		info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+	if (flagsv2 & GPIOLINE_FLAG_V2_BIAS_PULL_DOWN)
+		info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+	if (flagsv2 & GPIOLINE_FLAG_V2_BIAS_DISABLED)
+		info_v1->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+}
+
+static void gpioline_info_changed_v2_to_v1(
+		struct gpioline_info_changed_v2 *lic_v2,
+		struct gpioline_info_changed *lic_v1)
+{
+	gpioline_info_v2_to_v1(&lic_v2->info, &lic_v1->info);
+	lic_v1->timestamp = lic_v2->timestamp;
+	lic_v1->event_type = lic_v2->event_type;
+}
+
 #endif /* CONFIG_GPIO_CDEV_V1 */
 
 static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
-				  struct gpioline_info *info)
+				  struct gpioline_info_v2 *info)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
 	bool ok_for_pinctrl;
@@ -1125,7 +1167,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	 * lock common to both frameworks?
 	 */
 	ok_for_pinctrl =
-		pinctrl_gpio_can_use_line(gc->base + info->line_offset);
+		pinctrl_gpio_can_use_line(gc->base + info->offset);
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
@@ -1154,23 +1196,27 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	    test_bit(FLAG_EXPORT, &desc->flags) ||
 	    test_bit(FLAG_SYSFS, &desc->flags) ||
 	    !ok_for_pinctrl)
-		info->flags |= GPIOLINE_FLAG_KERNEL;
+		info->flags |= GPIOLINE_FLAG_V2_USED;
+
 	if (test_bit(FLAG_IS_OUT, &desc->flags))
-		info->flags |= GPIOLINE_FLAG_IS_OUT;
+		info->flags |= GPIOLINE_FLAG_V2_OUTPUT;
+	else
+		info->flags |= GPIOLINE_FLAG_V2_INPUT;
+
 	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
-		info->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+		info->flags |= GPIOLINE_FLAG_V2_ACTIVE_LOW;
+
 	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
-		info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
-				GPIOLINE_FLAG_IS_OUT);
+		info->flags |= GPIOLINE_FLAG_V2_OPEN_DRAIN;
 	if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
-		info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
-				GPIOLINE_FLAG_IS_OUT);
+		info->flags |= GPIOLINE_FLAG_V2_OPEN_SOURCE;
+
 	if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
-		info->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+		info->flags |= GPIOLINE_FLAG_V2_BIAS_DISABLED;
 	if (test_bit(FLAG_PULL_DOWN, &desc->flags))
-		info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+		info->flags |= GPIOLINE_FLAG_V2_BIAS_PULL_DOWN;
 	if (test_bit(FLAG_PULL_UP, &desc->flags))
-		info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+		info->flags |= GPIOLINE_FLAG_V2_BIAS_PULL_UP;
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 }
@@ -1178,11 +1224,65 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 struct gpio_chardev_data {
 	struct gpio_device *gdev;
 	wait_queue_head_t wait;
-	DECLARE_KFIFO(events, struct gpioline_info_changed, 32);
+	DECLARE_KFIFO(events, struct gpioline_info_changed_v2, 32);
 	struct notifier_block lineinfo_changed_nb;
 	unsigned long *watched_lines;
+#ifdef CONFIG_GPIO_CDEV_V1
+	atomic_t watch_abi_version;
+#endif
 };
 
+#ifdef CONFIG_GPIO_CDEV_V1
+static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata,
+				       unsigned int version)
+{
+	int abiv = atomic_read(&cdata->watch_abi_version);
+
+	if (abiv == 0) {
+		atomic_cmpxchg(&cdata->watch_abi_version, 0, version);
+		abiv = atomic_read(&cdata->watch_abi_version);
+	}
+	if (abiv != version)
+		return -EPERM;
+	return 0;
+}
+#endif
+
+static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
+			unsigned int cmd)
+{
+	struct gpio_desc *desc;
+	struct gpioline_info_v2 lineinfo;
+
+	if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+		return -EFAULT;
+
+	if (padding_not_zeroed(lineinfo.padding, ARRAY_SIZE(lineinfo.padding)))
+		return -EINVAL;
+
+	desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	if (cmd == GPIO_GET_LINEINFO_WATCH_V2_IOCTL) {
+#ifdef CONFIG_GPIO_CDEV_V1
+		if (lineinfo_ensure_abi_version(cdev, 2))
+			return -EPERM;
+#endif
+		if (test_and_set_bit(lineinfo.offset, cdev->watched_lines))
+			return -EBUSY;
+	}
+	gpio_desc_to_lineinfo(desc, &lineinfo);
+
+	if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
+		if (cmd == GPIO_GET_LINEINFO_WATCH_V2_IOCTL)
+			clear_bit(lineinfo.offset, cdev->watched_lines);
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
 /*
  * gpio_ioctl() - ioctl handler for the GPIO chardev
  */
@@ -1192,7 +1292,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct gpio_device *gdev = cdev->gdev;
 	struct gpio_chip *gc = gdev->chip;
 	void __user *ip = (void __user *)arg;
-	struct gpio_desc *desc;
 	__u32 offset;
 
 	/* We fail any subsequent ioctl():s when the chip is gone */
@@ -1217,7 +1316,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		return 0;
 #ifdef CONFIG_GPIO_CDEV_V1
 	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+		struct gpio_desc *desc;
 		struct gpioline_info lineinfo;
+		struct gpioline_info_v2 lineinfo_v2;
 
 		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
 			return -EFAULT;
@@ -1227,7 +1328,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (IS_ERR(desc))
 			return PTR_ERR(desc);
 
-		gpio_desc_to_lineinfo(desc, &lineinfo);
+		gpio_desc_to_lineinfo(desc, &lineinfo_v2);
+		lineinfo_v2.offset = lineinfo.line_offset;
+		gpioline_info_v2_to_v1(&lineinfo_v2, &lineinfo);
 
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
@@ -1237,7 +1340,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	} else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
 		return lineevent_create(gdev, ip);
 	} else if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
+		struct gpio_desc *desc;
 		struct gpioline_info lineinfo;
+		struct gpioline_info_v2 lineinfo_v2;
 
 		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
 			return -EFAULT;
@@ -1247,10 +1352,15 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (IS_ERR(desc))
 			return PTR_ERR(desc);
 
+		if (lineinfo_ensure_abi_version(cdev, 1))
+			return -EPERM;
+
 		if (test_and_set_bit(lineinfo.line_offset, cdev->watched_lines))
 			return -EBUSY;
 
-		gpio_desc_to_lineinfo(desc, &lineinfo);
+		gpio_desc_to_lineinfo(desc, &lineinfo_v2);
+		lineinfo_v2.offset = lineinfo.line_offset;
+		gpioline_info_v2_to_v1(&lineinfo_v2, &lineinfo);
 
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
 			clear_bit(lineinfo.line_offset, cdev->watched_lines);
@@ -1259,6 +1369,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		return 0;
 #endif /* CONFIG_GPIO_CDEV_V1 */
+	} else if (cmd == GPIO_GET_LINEINFO_V2_IOCTL ||
+		   cmd == GPIO_GET_LINEINFO_WATCH_V2_IOCTL) {
+		return lineinfo_get(cdev, ip, cmd);
 	} else if (cmd == GPIO_GET_LINE_IOCTL) {
 		return line_create(gdev, ip);
 	} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
@@ -1294,7 +1407,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 				   unsigned long action, void *data)
 {
 	struct gpio_chardev_data *cdev = to_gpio_chardev_data(nb);
-	struct gpioline_info_changed chg;
+	struct gpioline_info_changed_v2 chg;
 	struct gpio_desc *desc = data;
 	int ret;
 
@@ -1302,7 +1415,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 		return NOTIFY_DONE;
 
 	memset(&chg, 0, sizeof(chg));
-	chg.info.line_offset = gpio_chip_hwgpio(desc);
+	chg.info.offset = gpio_chip_hwgpio(desc);
 	chg.event_type = action;
 	chg.timestamp = ktime_get_ns();
 	gpio_desc_to_lineinfo(desc, &chg.info);
@@ -1335,12 +1448,16 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 				   size_t count, loff_t *off)
 {
 	struct gpio_chardev_data *cdev = file->private_data;
-	struct gpioline_info_changed event;
+	struct gpioline_info_changed_v2 event;
 	ssize_t bytes_read = 0;
 	int ret;
+	size_t event_size;
 
-	if (count < sizeof(event))
+#ifndef CONFIG_GPIO_CDEV_V1
+	event_size = sizeof(struct gpioline_info_changed_v2);
+	if (count < event_size)
 		return -EINVAL;
+#endif
 
 	do {
 		spin_lock(&cdev->wait.lock);
@@ -1362,7 +1479,17 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 				return ret;
 			}
 		}
-
+#ifdef CONFIG_GPIO_CDEV_V1
+		/* must be after kfifo check so watch_abi_version is set */
+		if (atomic_read(&cdev->watch_abi_version) == 2)
+			event_size = sizeof(struct gpioline_info_changed_v2);
+		else
+			event_size = sizeof(struct gpioline_info_changed);
+		if (count < event_size) {
+			spin_unlock(&cdev->wait.lock);
+			return -EINVAL;
+		}
+#endif
 		ret = kfifo_out(&cdev->events, &event, 1);
 		spin_unlock(&cdev->wait.lock);
 		if (ret != 1) {
@@ -1371,9 +1498,23 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 			/* We should never get here. See lineevent_read(). */
 		}
 
-		if (copy_to_user(buf + bytes_read, &event, sizeof(event)))
+#ifdef CONFIG_GPIO_CDEV_V1
+		if (event_size == sizeof(struct gpioline_info_changed_v2)) {
+			if (copy_to_user(buf + bytes_read, &event, event_size))
+				return -EFAULT;
+		} else {
+			struct gpioline_info_changed event_v1;
+
+			gpioline_info_changed_v2_to_v1(&event, &event_v1);
+			if (copy_to_user(buf + bytes_read, &event_v1,
+					 event_size))
+				return -EFAULT;
+		}
+#else
+		if (copy_to_user(buf + bytes_read, &event, event_size))
 			return -EFAULT;
-		bytes_read += sizeof(event);
+#endif
+		bytes_read += event_size;
 	} while (count >= bytes_read + sizeof(event));
 
 	return bytes_read;
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 07/18] gpiolib: cdev: support edge detection for uAPI v2
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (5 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 06/18] gpiolib: cdev: support GPIO_GET_LINEINFO_V2_IOCTL and GPIO_GET_LINEINFO_WATCH_V2_IOCTL Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-08-04 19:28   ` Bartosz Golaszewski
  2020-07-25  4:19 ` [PATCH v2 08/18] gpiolib: cdev: support GPIOLINE_SET_CONFIG_IOCTL Kent Gibson
                   ` (10 subsequent siblings)
  17 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add support for edge detection to lines requested using
GPIO_GET_LINE_IOCTL.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

The edge_detector implementation is based on the V1 lineevent implementation.

 drivers/gpio/gpiolib-cdev.c | 314 +++++++++++++++++++++++++++++++++++-
 drivers/gpio/gpiolib.c      |   2 +
 drivers/gpio/gpiolib.h      |   2 +
 3 files changed, 317 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 1f282207fb70..8caebb460557 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -381,11 +381,43 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 }
 #endif /* CONFIG_GPIO_CDEV_V1 */
 
+/**
+ * struct edge_detector - contains the state of a line edge detector
+ * @line: the corresponding line request
+ * @irq: the interrupt triggered in response to events on this GPIO
+ * @flags: the flags, GPIOLINE_FLAG_V2_EDGE_RISING and/or
+ * GPIOLINE_FLAG_V2_EDGE_FALLING, indicating the edge detection applied
+ * @timestamp: cache for the timestamp storing it between hardirq and IRQ
+ * thread, used to bring the timestamp close to the actual event
+ * @seqno: the seqno for the current edge event in the sequence of events
+ * for the corresponding line request. Ths is drawn from the @line.
+ * @line_seqno: the seqno for the current edge event in the sequence of
+ * events for this line.
+ */
+struct edge_detector {
+	struct line *line;
+	unsigned int irq;
+	u64 flags;
+	/*
+	 * timestamp and seqno are shared by edge_irq_handler() and
+	 * edge_irq_thread() which are themselves mutually exclusive.
+	 */
+	u64 timestamp;
+	u32 seqno;
+	u32 line_seqno;
+};
+
 /**
  * struct line - contains the state of a userspace line request
  * @gdev: the GPIO device the line request pertains to
  * @label: consumer label used to tag descriptors
  * @num_descs: the number of descriptors held in the descs array
+ * @wait: wait queue that handles blocking reads of events
+ * @events: KFIFO for the GPIO events
+ * @seqno: the sequence number for edge events generated on all lines in
+ * this line request.  Note that this is not used when @num_descs is 1, as
+ * the line_seqno is then the same and is cheaper to calculate.
+ * @edets: an array of edge detectors, of size @num_descs
  * @descs: the GPIO descriptors held by this line request, with @num_descs
  * elements.
  */
@@ -393,10 +425,147 @@ struct line {
 	struct gpio_device *gdev;
 	const char *label;
 	u32 num_descs;
+	wait_queue_head_t wait;
+	DECLARE_KFIFO_PTR(events, struct gpioline_event);
+	atomic_t seqno;
+	struct edge_detector *edets;
 	/* descs must be last so it can be dynamically sized */
 	struct gpio_desc *descs[];
 };
 
+static inline struct gpio_desc *edge_detector_desc(
+		const struct edge_detector *edet)
+{
+	return edet->line->descs[edet - &edet->line->edets[0]];
+}
+
+static irqreturn_t edge_irq_thread(int irq, void *p)
+{
+	struct edge_detector *edet = p;
+	struct line *line = edet->line;
+	struct gpio_desc *desc = edge_detector_desc(edet);
+	struct gpioline_event le;
+	int ret;
+
+	/* Do not leak kernel stack to userspace */
+	memset(&le, 0, sizeof(le));
+
+	/*
+	 * We may be running from a nested threaded interrupt in which case
+	 * we didn't get the timestamp from edge_irq_handler().
+	 */
+	if (!edet->timestamp) {
+		le.timestamp = ktime_get_ns();
+		if (line->num_descs != 1)
+			edet->seqno = atomic_inc_return(&line->seqno);
+	} else {
+		le.timestamp = edet->timestamp;
+	}
+	edet->timestamp = 0;
+
+	if (edet->flags == (GPIOLINE_FLAG_V2_EDGE_RISING |
+			    GPIOLINE_FLAG_V2_EDGE_FALLING)) {
+		int level = gpiod_get_value_cansleep(desc);
+
+		if (level)
+			/* Emit low-to-high event */
+			le.id = GPIOLINE_EVENT_RISING_EDGE;
+		else
+			/* Emit high-to-low event */
+			le.id = GPIOLINE_EVENT_FALLING_EDGE;
+	} else if (edet->flags == GPIOLINE_FLAG_V2_EDGE_RISING) {
+		/* Emit low-to-high event */
+		le.id = GPIOLINE_EVENT_RISING_EDGE;
+	} else if (edet->flags == GPIOLINE_FLAG_V2_EDGE_FALLING) {
+		/* Emit high-to-low event */
+		le.id = GPIOLINE_EVENT_FALLING_EDGE;
+	} else {
+		return IRQ_NONE;
+	}
+	edet->line_seqno++;
+	le.line_seqno = edet->line_seqno;
+	le.seqno = (line->num_descs == 1) ? le.line_seqno : edet->seqno;
+	le.offset = gpio_chip_hwgpio(desc);
+
+	ret = kfifo_in_spinlocked_noirqsave(&line->events, &le,
+					    1, &line->wait.lock);
+	if (ret)
+		wake_up_poll(&line->wait, EPOLLIN);
+	else
+		pr_debug_ratelimited("event FIFO is full - event dropped\n");
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t edge_irq_handler(int irq, void *p)
+{
+	struct edge_detector *edet = p;
+	struct line *line = edet->line;
+
+	/*
+	 * Just store the timestamp in hardirq context so we get it as
+	 * close in time as possible to the actual event.
+	 */
+	edet->timestamp = ktime_get_ns();
+
+	if (line->num_descs != 1)
+		edet->seqno = atomic_inc_return(&line->seqno);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static int edge_detector_start(struct edge_detector *edet)
+{
+	int ret, irq, irqflags = 0;
+	struct gpio_desc *desc;
+
+	desc = edge_detector_desc(edet);
+	irq = gpiod_to_irq(desc);
+
+	if (irq <= 0)
+		return -ENODEV;
+
+	edet->seqno = 0;
+	edet->line_seqno = 0;
+
+	if (edet->flags & GPIOLINE_FLAG_V2_EDGE_RISING)
+		irqflags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
+			IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
+	if (edet->flags & GPIOLINE_FLAG_V2_EDGE_FALLING)
+		irqflags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
+			IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
+	irqflags |= IRQF_ONESHOT;
+
+	/* Request a thread to read the events */
+	ret = request_threaded_irq(irq,
+				   edge_irq_handler,
+				   edge_irq_thread,
+				   irqflags,
+				   edet->line->label,
+				   edet);
+	if (ret)
+		return ret;
+
+	edet->irq = irq;
+	return 0;
+}
+
+static void edge_detector_stop(struct edge_detector *edet)
+{
+	if (edet->irq) {
+		free_irq(edet->irq, edet);
+		edet->irq = 0;
+	}
+}
+
+static int edge_detector_setup(struct edge_detector *edet,
+			       struct gpioline_config *lc)
+{
+	if (edet->flags)
+		return edge_detector_start(edet);
+	return 0;
+}
+
 static bool padding_not_zeroed(__u32 *padding, int pad_size)
 {
 	int i, sum = 0;
@@ -420,10 +589,15 @@ static bool padding_not_zeroed(__u32 *padding, int pad_size)
 	(GPIOLINE_FLAG_V2_OPEN_DRAIN | \
 	 GPIOLINE_FLAG_V2_OPEN_SOURCE)
 
+#define GPIOLINE_EDGE_FLAGS \
+	(GPIOLINE_FLAG_V2_EDGE_RISING | \
+	 GPIOLINE_FLAG_V2_EDGE_FALLING)
+
 #define GPIOLINE_VALID_FLAGS \
 	(GPIOLINE_FLAG_V2_ACTIVE_LOW | \
 	 GPIOLINE_DIRECTION_FLAGS | \
 	 GPIOLINE_DRIVE_FLAGS | \
+	 GPIOLINE_EDGE_FLAGS | \
 	 GPIOLINE_BIAS_FLAGS)
 
 static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
@@ -438,6 +612,21 @@ static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
 	return lc->flags;
 }
 
+static int gpioline_config_has_edge_detection(struct gpioline_config *lc)
+{
+	int i;
+
+	if (lc->flags & GPIOLINE_EDGE_FLAGS)
+		return 1;
+
+	for (i = 0; i < lc->num_attrs; i++) {
+		if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) &&
+		    (lc->attrs[i].attr.flags & GPIOLINE_EDGE_FLAGS))
+			return 1;
+	}
+	return 0;
+}
+
 static int gpioline_config_output_value(struct gpioline_config *lc,
 					int line_idx)
 {
@@ -466,6 +655,10 @@ static int gpioline_flags_validate(u64 flags)
 	    (flags & GPIOLINE_FLAG_V2_OUTPUT))
 		return -EINVAL;
 
+	/* Edge detection requires explicit input. */
+	if ((flags & GPIOLINE_EDGE_FLAGS) && !(flags & GPIOLINE_FLAG_V2_INPUT))
+		return -EINVAL;
+
 	/*
 	 * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
 	 * the hardware actually supports enabling both at the same time the
@@ -525,6 +718,10 @@ static void gpioline_config_flags_to_desc_flags(u64 flags,
 		set_bit(FLAG_IS_OUT, flagsp);
 	else if (flags & GPIOLINE_FLAG_V2_INPUT)
 		clear_bit(FLAG_IS_OUT, flagsp);
+	assign_bit(FLAG_EDGE_RISING, flagsp,
+		   flags & GPIOLINE_FLAG_V2_EDGE_RISING);
+	assign_bit(FLAG_EDGE_FALLING, flagsp,
+		   flags & GPIOLINE_FLAG_V2_EDGE_FALLING);
 	assign_bit(FLAG_OPEN_DRAIN, flagsp,
 		   flags & GPIOLINE_FLAG_V2_OPEN_DRAIN);
 	assign_bit(FLAG_OPEN_SOURCE, flagsp,
@@ -580,14 +777,86 @@ static long line_ioctl_compat(struct file *file, unsigned int cmd,
 }
 #endif
 
+static __poll_t line_poll(struct file *file,
+			  struct poll_table_struct *wait)
+{
+	struct line *line = file->private_data;
+	__poll_t events = 0;
+
+	poll_wait(file, &line->wait, wait);
+
+	if (!kfifo_is_empty_spinlocked_noirqsave(&line->events, &line->wait.lock))
+		events = EPOLLIN | EPOLLRDNORM;
+
+	return events;
+}
+
+static ssize_t line_read(struct file *file,
+			 char __user *buf,
+			 size_t count,
+			 loff_t *f_ps)
+{
+	struct line *line = file->private_data;
+	struct gpioline_event le;
+	ssize_t bytes_read = 0;
+	int ret;
+
+	if (count < sizeof(le))
+		return -EINVAL;
+
+	do {
+		spin_lock(&line->wait.lock);
+		if (kfifo_is_empty(&line->events)) {
+			if (bytes_read) {
+				spin_unlock(&line->wait.lock);
+				return bytes_read;
+			}
+
+			if (file->f_flags & O_NONBLOCK) {
+				spin_unlock(&line->wait.lock);
+				return -EAGAIN;
+			}
+
+			ret = wait_event_interruptible_locked(line->wait,
+					!kfifo_is_empty(&line->events));
+			if (ret) {
+				spin_unlock(&line->wait.lock);
+				return ret;
+			}
+		}
+
+		ret = kfifo_out(&line->events, &le, 1);
+		spin_unlock(&line->wait.lock);
+		if (ret != 1) {
+			/*
+			 * This should never happen - we were holding the lock
+			 * from the moment we learned the fifo is no longer
+			 * empty until now.
+			 */
+			ret = -EIO;
+			break;
+		}
+
+		if (copy_to_user(buf + bytes_read, &le, sizeof(le)))
+			return -EFAULT;
+		bytes_read += sizeof(le);
+	} while (count >= bytes_read + sizeof(le));
+
+	return bytes_read;
+}
+
 static void line_free(struct line *line)
 {
 	int i;
 
 	for (i = 0; i < line->num_descs; i++) {
+		if (line->edets)
+			edge_detector_stop(&line->edets[i]);
 		if (line->descs[i])
 			gpiod_free(line->descs[i]);
 	}
+	kfifo_free(&line->events);
+	kfree(line->edets);
 	kfree(line->label);
 	put_device(&line->gdev->dev);
 	kfree(line);
@@ -603,6 +872,8 @@ static int line_release(struct inode *inode, struct file *file)
 
 static const struct file_operations line_fileops = {
 	.release = line_release,
+	.read = line_read,
+	.poll = line_poll,
 	.owner = THIS_MODULE,
 	.llseek = noop_llseek,
 	.unlocked_ioctl = line_ioctl,
@@ -616,7 +887,7 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 	struct gpioline_request linereq;
 	struct line *line;
 	struct file *file;
-	int fd, i, ret;
+	int fd, i, ret, size, has_edge_detection;
 	struct gpioline_config *lc;
 	unsigned long flags;
 
@@ -633,6 +904,11 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 	if (ret)
 		return ret;
 
+	/* event_buffer_size only valid with edge detection */
+	has_edge_detection = gpioline_config_has_edge_detection(lc);
+	if (linereq.event_buffer_size && !has_edge_detection)
+		return -EINVAL;
+
 	line = kzalloc(struct_size(line, descs, linereq.num_lines),
 		       GFP_KERNEL);
 	if (!line)
@@ -641,6 +917,16 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 	line->gdev = gdev;
 	get_device(&gdev->dev);
 
+	line->edets = kcalloc(linereq.num_lines, sizeof(*line->edets),
+			      GFP_KERNEL);
+	if (!line->edets) {
+		ret = -ENOMEM;
+		goto out_free_line;
+	}
+
+	for (i = 0; i < linereq.num_lines; i++)
+		line->edets[i].line = line;
+
 	/* Make sure this is terminated */
 	linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
 	if (strlen(linereq.consumer)) {
@@ -651,6 +937,21 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 		}
 	}
 
+	init_waitqueue_head(&line->wait);
+	if (has_edge_detection) {
+		size = linereq.event_buffer_size;
+
+		if (size > GPIOLINES_MAX*16)
+			size = GPIOLINES_MAX*16;
+		else if (size == 0)
+			size = linereq.num_lines*16;
+
+		ret = kfifo_alloc(&line->events, size, GFP_KERNEL);
+		if (ret)
+			goto out_free_line;
+	}
+
+	atomic_set(&line->seqno, 0);
 	line->num_descs = linereq.num_lines;
 
 	/* Request each GPIO */
@@ -689,6 +990,10 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 			ret = gpiod_direction_input(desc);
 			if (ret)
 				goto out_free_line;
+			line->edets[i].flags = flags & GPIOLINE_EDGE_FLAGS;
+			ret = edge_detector_setup(&line->edets[i], lc);
+			if (ret)
+				goto out_free_line;
 		}
 
 		blocking_notifier_call_chain(&desc->gdev->notifier,
@@ -1218,6 +1523,13 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	if (test_bit(FLAG_PULL_UP, &desc->flags))
 		info->flags |= GPIOLINE_FLAG_V2_BIAS_PULL_UP;
 
+	if (test_bit(FLAG_EDGE_RISING, &desc->flags))
+		info->flags |= GPIOLINE_FLAG_V2_EDGE_RISING;
+	if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
+		info->flags |= GPIOLINE_FLAG_V2_EDGE_FALLING;
+
+	info->num_attrs = 0;
+
 	spin_unlock_irqrestore(&gpio_lock, flags);
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 80137c1b3cdc..e4c81dca7f8b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2041,6 +2041,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_PULL_UP, &desc->flags);
 		clear_bit(FLAG_PULL_DOWN, &desc->flags);
 		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
+		clear_bit(FLAG_EDGE_RISING, &desc->flags);
+		clear_bit(FLAG_EDGE_FALLING, &desc->flags);
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 #ifdef CONFIG_OF_DYNAMIC
 		desc->hog = NULL;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 6709f79c02dd..39b356160937 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -114,6 +114,8 @@ struct gpio_desc {
 #define FLAG_PULL_UP    13	/* GPIO has pull up enabled */
 #define FLAG_PULL_DOWN  14	/* GPIO has pull down enabled */
 #define FLAG_BIAS_DISABLE    15	/* GPIO has pull disabled */
+#define FLAG_EDGE_RISING     16	/* GPIO CDEV detects rising edge events */
+#define FLAG_EDGE_FALLING    17	/* GPIO CDEV detects falling edge events */
 
 	/* Connection label */
 	const char		*label;
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 08/18] gpiolib: cdev: support GPIOLINE_SET_CONFIG_IOCTL
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (6 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 07/18] gpiolib: cdev: support edge detection for uAPI v2 Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 09/18] gpiolib: cdev: support GPIOLINE_SET_VALUES_IOCTL Kent Gibson
                   ` (9 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add support for GPIOLINE_SET_CONFIG_IOCTL, the uAPI v2
line set config ioctl.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 91 +++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 8caebb460557..9d944ca0ff59 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/kfifo.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/poll.h>
 #include <linux/spinlock.h>
@@ -417,6 +418,8 @@ struct edge_detector {
  * @seqno: the sequence number for edge events generated on all lines in
  * this line request.  Note that this is not used when @num_descs is 1, as
  * the line_seqno is then the same and is cheaper to calculate.
+ * @config_mutex: mutex for serializing ioctl() calls to ensure consistency
+ * of configuration, partiuclarly multi-step accesses to desc flags.
  * @edets: an array of edge detectors, of size @num_descs
  * @descs: the GPIO descriptors held by this line request, with @num_descs
  * elements.
@@ -428,6 +431,7 @@ struct line {
 	wait_queue_head_t wait;
 	DECLARE_KFIFO_PTR(events, struct gpioline_event);
 	atomic_t seqno;
+	struct mutex config_mutex;
 	struct edge_detector *edets;
 	/* descs must be last so it can be dynamically sized */
 	struct gpio_desc *descs[];
@@ -709,6 +713,30 @@ static int gpioline_config_validate(struct gpioline_config *lc, int num_lines)
 	return 0;
 }
 
+static int gpioline_config_change_validate(struct line *line,
+					   struct gpioline_config *lc)
+{
+	int i;
+	u64 flags;
+	struct gpio_desc *desc;
+
+	for (i = 0; i < line->num_descs; i++) {
+		desc = line->descs[i];
+		flags = gpioline_config_flags(lc, i);
+		/* disallow edge detection changes */
+		if (line->edets[i].flags != (flags & GPIOLINE_EDGE_FLAGS))
+			return -EINVAL;
+
+		if (line->edets[i].flags) {
+			/* disallow polarity changes */
+			if (test_bit(FLAG_ACTIVE_LOW, &desc->flags) !=
+			    ((flags & GPIOLINE_FLAG_V2_ACTIVE_LOW) != 0))
+				return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 static void gpioline_config_flags_to_desc_flags(u64 flags,
 						unsigned long *flagsp)
 {
@@ -757,6 +785,66 @@ static long line_get_values(struct line *line, void __user *ip)
 	return 0;
 }
 
+static long line_set_config_locked(struct line *line,
+				   struct gpioline_config *lc)
+{
+	struct gpio_desc *desc;
+	int i, ret;
+	u64 flags;
+
+	ret = gpioline_config_change_validate(line, lc);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < line->num_descs; i++) {
+		desc = line->descs[i];
+		flags = gpioline_config_flags(lc, i);
+
+		gpioline_config_flags_to_desc_flags(flags, &desc->flags);
+		/*
+		 * Lines have to be requested explicitly for input
+		 * or output, else the line will be treated "as is".
+		 */
+		if (flags & GPIOLINE_FLAG_V2_OUTPUT) {
+			int val = gpioline_config_output_value(lc, i);
+
+			edge_detector_stop(&line->edets[i]);
+			ret = gpiod_direction_output(desc, val);
+			if (ret)
+				return ret;
+		} else if (flags & GPIOLINE_FLAG_V2_INPUT) {
+			ret = gpiod_direction_input(desc);
+			if (ret)
+				return ret;
+		}
+
+		blocking_notifier_call_chain(&desc->gdev->notifier,
+					     GPIOLINE_CHANGED_CONFIG, desc);
+	}
+	return 0;
+}
+
+static long line_set_config(struct line *line, void __user *ip)
+{
+	struct gpioline_config lc;
+	int ret;
+
+	if (copy_from_user(&lc, ip, sizeof(lc)))
+		return -EFAULT;
+
+	ret = gpioline_config_validate(&lc, line->num_descs);
+	if (ret)
+		return ret;
+
+	mutex_lock(&line->config_mutex);
+
+	ret = line_set_config_locked(line, &lc);
+
+	mutex_unlock(&line->config_mutex);
+
+	return ret;
+}
+
 static long line_ioctl(struct file *file, unsigned int cmd,
 		       unsigned long arg)
 {
@@ -765,6 +853,8 @@ static long line_ioctl(struct file *file, unsigned int cmd,
 
 	if (cmd == GPIOLINE_GET_VALUES_IOCTL)
 		return line_get_values(line, ip);
+	else if (cmd == GPIOLINE_SET_CONFIG_IOCTL)
+		return line_set_config(line, ip);
 
 	return -EINVAL;
 }
@@ -937,6 +1027,7 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 		}
 	}
 
+	mutex_init(&line->config_mutex);
 	init_waitqueue_head(&line->wait);
 	if (has_edge_detection) {
 		size = linereq.event_buffer_size;
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 09/18] gpiolib: cdev: support GPIOLINE_SET_VALUES_IOCTL
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (7 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 08/18] gpiolib: cdev: support GPIOLINE_SET_CONFIG_IOCTL Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 10/18] gpiolib: cdev: support setting debounce Kent Gibson
                   ` (8 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add support for the GPIOLINE_SET_VALUES_IOCTL.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 66 +++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 4822cb10aa40..0482a16388a0 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -785,6 +785,70 @@ static long line_get_values(struct line *line, void __user *ip)
 	return 0;
 }
 
+static long line_set_values_locked(struct line *line,
+				   struct gpioline_set_values *lsv)
+{
+	unsigned long *vals = (unsigned long *)lsv->bits;
+	unsigned long *mask = (unsigned long *)lsv->mask;
+	struct gpio_desc **descs;
+	int ret, i, didx, num_set = 0;
+
+	for (i = 0; i < line->num_descs; i++) {
+		if (test_bit(i, mask)) {
+			if (!test_bit(FLAG_IS_OUT, &line->descs[i]->flags))
+				return -EPERM;
+			num_set++;
+		}
+	}
+	if (num_set == 0)
+		return -EINVAL;
+
+	if (num_set == line->num_descs)
+		/* Reuse the array setting function */
+		return gpiod_set_array_value_complex(false,
+						     true,
+						     line->num_descs,
+						     line->descs,
+						     NULL,
+						     vals);
+
+	/* build compacted desc array and values */
+	descs = kmalloc_array(num_set, sizeof(*descs), GFP_KERNEL);
+	for (didx = 0, i = 0; i < line->num_descs; i++) {
+		if (test_bit(i, mask)) {
+			descs[didx] = line->descs[i];
+			assign_bit(didx, vals, test_bit(i, vals));
+			didx++;
+		}
+	}
+	ret = gpiod_set_array_value_complex(false,
+					    true,
+					    num_set,
+					    descs,
+					    NULL,
+					    vals);
+
+	kfree(descs);
+	return ret;
+}
+
+static long line_set_values(struct line *line, void __user *ip)
+{
+	struct gpioline_set_values lsv;
+	int ret;
+
+	if (copy_from_user(&lsv, ip, sizeof(lsv)))
+		return -EFAULT;
+
+	mutex_lock(&line->config_mutex);
+
+	ret = line_set_values_locked(line, &lsv);
+
+	mutex_unlock(&line->config_mutex);
+
+	return ret;
+}
+
 static long line_set_config_locked(struct line *line,
 				   struct gpioline_config *lc)
 {
@@ -853,6 +917,8 @@ static long line_ioctl(struct file *file, unsigned int cmd,
 
 	if (cmd == GPIOLINE_GET_VALUES_IOCTL)
 		return line_get_values(line, ip);
+	else if (cmd == GPIOLINE_SET_VALUES_IOCTL)
+		return line_set_values(line, ip);
 	else if (cmd == GPIOLINE_SET_CONFIG_IOCTL)
 		return line_set_config(line, ip);
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 10/18] gpiolib: cdev: support setting debounce
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (8 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 09/18] gpiolib: cdev: support GPIOLINE_SET_VALUES_IOCTL Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 11/18] gpio: uapi: document uAPI v1 as deprecated Kent Gibson
                   ` (7 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add support for setting debounce on a line via the GPIO uAPI.
Where debounce is not supported by hardware, a software debounce is
provided.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

The implementation of the software debouncer waits for the line to be
stable for the debounce period before determining if a level change,
and a corresponding edge event, has occurred.  This provides maximum
protection against glitches, but also introduces a debounce_period
latency to edge events.

The software debouncer is integrated with the edge detection as it
utilises the line interrupt, and integration is simpler than getting
the two to interwork.  Where software debounce AND edge detection is
required, the debouncer provides both.

Due to the tight integration between the debouncer and edge detection,
and to avoid particular corner cases, it is not allowed to alter the
debounce value if edge detection is enabled.  Changing the debounce with
edge detection enabled is a very unlikely use case, so it is preferable
to disallow it rather than complicate the code to allow it.
Should the user wish to alter the debounce value in such cases they will
need to release and re-request the line.

Changes since v1:
 - improve documentation on fields shared by threads.
 - use READ_ONCE/WRITE_ONCE for shared fields rather than atomic_t
   which was overkill.


 drivers/gpio/gpiolib-cdev.c | 261 +++++++++++++++++++++++++++++++++++-
 drivers/gpio/gpiolib.h      |   4 +
 2 files changed, 260 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 9f2f3336c62a..b6761d267a7d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -6,6 +6,7 @@
 #include <linux/build_bug.h>
 #include <linux/cdev.h>
 #include <linux/compat.h>
+#include <linux/compiler.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/file.h>
@@ -22,6 +23,7 @@
 #include <linux/spinlock.h>
 #include <linux/timekeeping.h>
 #include <linux/uaccess.h>
+#include <linux/workqueue.h>
 #include <uapi/linux/gpio.h>
 
 #include "gpiolib.h"
@@ -394,6 +396,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * for the corresponding line request. Ths is drawn from the @line.
  * @line_seqno: the seqno for the current edge event in the sequence of
  * events for this line.
+ * @work: the worker that implements software debouncing
+ * @sw_debounced: flag indicating if the software debouncer is active
+ * @level: the current debounced physical level of the line
  */
 struct edge_detector {
 	struct line *line;
@@ -405,7 +410,27 @@ struct edge_detector {
 	 */
 	u64 timestamp;
 	u32 seqno;
+	/*
+	 * line_seqno is used by either edge_irq_thread() or
+	 * debounce_work_func() which are themselves mutually exclusive.
+	 */
 	u32 line_seqno;
+	/*
+	 * -- debouncer specific fields --
+	 */
+	struct delayed_work work;
+	/*
+	 * sw_debounce is shared by line_set_config(), which is the only
+	 * setter, and line_ioctl(), which can live with a slightly stale
+	 * value.
+	 */
+	unsigned int sw_debounced;
+	/*
+	 * level is shared by debounce_work_func(), which is the only
+	 * setter, and line_ioctl() which can live with a slightly stale
+	 * value.
+	 */
+	unsigned int level;
 };
 
 /**
@@ -523,6 +548,10 @@ static int edge_detector_start(struct edge_detector *edet)
 	int ret, irq, irqflags = 0;
 	struct gpio_desc *desc;
 
+	if (READ_ONCE(edet->sw_debounced))
+		/* debouncer is setup and will provide edge detection */
+		return 0;
+
 	desc = edge_detector_desc(edet);
 	irq = gpiod_to_irq(desc);
 
@@ -554,17 +583,212 @@ static int edge_detector_start(struct edge_detector *edet)
 	return 0;
 }
 
+/*
+ * returns the current debounced logical value.
+ */
+static int debounced_value(struct edge_detector *edet)
+{
+	int value;
+
+	/*
+	 * minor race - debouncer may be stopped here, so edge_detector_stop
+	 * must leave the value unchanged so the following will read the level
+	 * from when the debouncer was last running.
+	 */
+	value = READ_ONCE(edet->level);
+
+	if (test_bit(FLAG_ACTIVE_LOW, &edge_detector_desc(edet)->flags))
+		value = !value;
+
+	return value;
+}
+
+static irqreturn_t debounce_irq_handler(int irq, void *p)
+{
+	struct edge_detector *edet = p;
+	struct gpio_desc *desc = edge_detector_desc(edet);
+
+	mod_delayed_work(system_wq,
+			 &edet->work,
+			 usecs_to_jiffies(READ_ONCE(desc->debounce_period)));
+
+	return IRQ_HANDLED;
+}
+
+static void debounce_work_func(struct work_struct *work)
+{
+	struct gpioline_event le;
+	int ret, level;
+	struct edge_detector *edet =
+		container_of(work, struct edge_detector, work.work);
+	struct gpio_desc *desc = edge_detector_desc(edet);
+	struct line *line;
+
+	level = gpiod_get_raw_value_cansleep(desc);
+	if (level < 0) {
+		pr_debug_ratelimited("debouncer failed to read line value\n");
+		return;
+	}
+
+	if (READ_ONCE(edet->level) == level)
+		return;
+
+	WRITE_ONCE(edet->level, level);
+
+	/* -- edge detection -- */
+	if (!edet->flags)
+		return;
+
+	/* switch from physical level to logical - if they differ */
+	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+		level = !level;
+
+	/* ignore edges that are not being monitored */
+	if (((edet->flags == GPIOLINE_FLAG_V2_EDGE_RISING) && !level) ||
+	    ((edet->flags == GPIOLINE_FLAG_V2_EDGE_FALLING) && level))
+		return;
+
+	/* Do not leak kernel stack to userspace */
+	memset(&le, 0, sizeof(le));
+
+	line = edet->line;
+	le.timestamp = ktime_get_ns();
+	le.offset = gpio_chip_hwgpio(desc);
+	edet->line_seqno++;
+	le.line_seqno = edet->line_seqno;
+	le.seqno = (line->num_descs == 1) ?
+		le.line_seqno : atomic_inc_return(&line->seqno);
+
+	if (level)
+		/* Emit low-to-high event */
+		le.id = GPIOLINE_EVENT_RISING_EDGE;
+	else
+		/* Emit high-to-low event */
+		le.id = GPIOLINE_EVENT_FALLING_EDGE;
+
+	ret = kfifo_in_spinlocked_noirqsave(&line->events, &le,
+					    1, &line->wait.lock);
+	if (ret)
+		wake_up_poll(&line->wait, EPOLLIN);
+	else
+		pr_debug_ratelimited("event FIFO is full - event dropped\n");
+}
+
+static int debounce_setup(struct edge_detector *edet,
+			  unsigned int debounce_period)
+{
+	int ret, level, irq, irqflags;
+	struct gpio_desc *desc = edge_detector_desc(edet);
+
+	/* try hardware */
+	ret = gpiod_set_debounce(desc, debounce_period);
+	if (!ret) {
+		WRITE_ONCE(desc->debounce_period, debounce_period);
+		return ret;
+	}
+	if (ret != -ENOTSUPP)
+		return ret;
+
+	if (debounce_period) {
+		/* setup software debounce */
+		level = gpiod_get_raw_value_cansleep(desc);
+		if (level < 0)
+			return level;
+
+		irq = gpiod_to_irq(desc);
+		if (irq <= 0)
+			return -ENODEV;
+
+		WRITE_ONCE(edet->level, level);
+		edet->line_seqno = 0;
+		irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
+		ret = request_irq(irq,
+				  debounce_irq_handler,
+				  irqflags,
+				  edet->line->label,
+				  edet);
+		if (ret)
+			return ret;
+
+		WRITE_ONCE(edet->sw_debounced, 1);
+		edet->irq = irq;
+	}
+	WRITE_ONCE(desc->debounce_period, debounce_period);
+	return 0;
+}
+
 static void edge_detector_stop(struct edge_detector *edet)
 {
+	struct gpio_desc *desc = edge_detector_desc(edet);
+
 	if (edet->irq) {
 		free_irq(edet->irq, edet);
 		edet->irq = 0;
 	}
+
+	cancel_delayed_work_sync(&edet->work);
+	WRITE_ONCE(edet->sw_debounced, 0);
+	/* do not change edet->level - see comment in debounced_value */
+
+	if (desc)
+		WRITE_ONCE(desc->debounce_period, 0);
+}
+
+static int debounce_update(struct edge_detector *edet,
+			   unsigned int debounce_period)
+{
+	struct gpio_desc *desc = edge_detector_desc(edet);
+
+	if (READ_ONCE(desc->debounce_period) == debounce_period)
+		return 0;
+
+	if (!READ_ONCE(edet->sw_debounced))
+		return debounce_setup(edet, debounce_period);
+
+	if (!debounce_period)
+		edge_detector_stop(edet);
+	else
+		WRITE_ONCE(desc->debounce_period, debounce_period);
+	return 0;
+}
+
+static int gpioline_config_debounced(struct gpioline_config *lc, int line_idx)
+{
+	int i;
+
+	for (i = lc->num_attrs - 1; i >= 0; i--) {
+		if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_DEBOUNCE) &&
+		    test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))
+			return 1;
+	}
+	return 0;
+}
+
+static u32 gpioline_config_debounce_period(struct gpioline_config *lc,
+					   int line_idx)
+{
+	int i;
+
+	for (i = lc->num_attrs - 1; i >= 0; i--) {
+		if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_DEBOUNCE) &&
+		    test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))
+			return lc->attrs[i].attr.debounce_period;
+	}
+	return 0;
 }
 
 static int edge_detector_setup(struct edge_detector *edet,
-			       struct gpioline_config *lc)
+			       struct gpioline_config *lc,
+			       int line_idx)
 {
+	int ret;
+
+	if (gpioline_config_debounced(lc, line_idx)) {
+		ret = debounce_setup(edet,
+			gpioline_config_debounce_period(lc, line_idx));
+		if (ret)
+			return ret;
+	}
 	if (edet->flags)
 		return edge_detector_start(edet);
 	return 0;
@@ -709,6 +933,11 @@ static int gpioline_config_validate(struct gpioline_config *lc, int num_lines)
 		ret = gpioline_flags_validate(flags);
 		if (ret)
 			return ret;
+
+		/* debounce requires explicit input */
+		if (gpioline_config_debounced(lc, i) &&
+		    !(flags & GPIOLINE_FLAG_V2_INPUT))
+			return -EINVAL;
 	}
 	return 0;
 }
@@ -766,7 +995,7 @@ static long line_get_values(struct line *line, void __user *ip)
 {
 	struct gpioline_values lv;
 	unsigned long *vals = (unsigned long *)lv.bits;
-	int ret;
+	int ret, i;
 
 	/* NOTE: It's ok to read values of output lines. */
 	memset(&lv, 0, sizeof(lv));
@@ -779,6 +1008,11 @@ static long line_get_values(struct line *line, void __user *ip)
 	if (ret)
 		return ret;
 
+	for (i = 0; i < line->num_descs; i++) {
+		if (line->edets[i].sw_debounced)
+			assign_bit(i, vals, debounced_value(&line->edets[i]));
+	}
+
 	if (copy_to_user(ip, &lv, sizeof(lv)))
 		return -EFAULT;
 
@@ -880,6 +1114,12 @@ static long line_set_config_locked(struct line *line,
 			ret = gpiod_direction_input(desc);
 			if (ret)
 				return ret;
+			if (gpioline_config_debounced(lc, i)) {
+				ret = debounce_update(&line->edets[i],
+					gpioline_config_debounce_period(lc, i));
+				if (ret)
+					return ret;
+			}
 		}
 
 		blocking_notifier_call_chain(&desc->gdev->notifier,
@@ -1080,8 +1320,11 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 		goto out_free_line;
 	}
 
-	for (i = 0; i < linereq.num_lines; i++)
+	for (i = 0; i < linereq.num_lines; i++) {
 		line->edets[i].line = line;
+		WRITE_ONCE(line->edets[i].sw_debounced, 0);
+		INIT_DELAYED_WORK(&line->edets[i].work, debounce_work_func);
+	}
 
 	/* Make sure this is terminated */
 	linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
@@ -1148,7 +1391,7 @@ static int line_create(struct gpio_device *gdev, void __user *ip)
 			if (ret)
 				goto out_free_line;
 			line->edets[i].flags = flags & GPIOLINE_EDGE_FLAGS;
-			ret = edge_detector_setup(&line->edets[i], lc);
+			ret = edge_detector_setup(&line->edets[i], lc, i);
 			if (ret)
 				goto out_free_line;
 		}
@@ -1620,6 +1863,8 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	struct gpio_chip *gc = desc->gdev->chip;
 	bool ok_for_pinctrl;
 	unsigned long flags;
+	u32 debounce_period;
+	int num_attrs = 0;
 
 	/*
 	 * This function takes a mutex so we must check this before taking
@@ -1685,7 +1930,13 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
 		info->flags |= GPIOLINE_FLAG_V2_EDGE_FALLING;
 
-	info->num_attrs = 0;
+	debounce_period = READ_ONCE(desc->debounce_period);
+	if (debounce_period) {
+		info->attrs[num_attrs].id = GPIOLINE_ATTR_ID_DEBOUNCE;
+		info->attrs[num_attrs].debounce_period = debounce_period;
+		num_attrs++;
+	}
+	info->num_attrs = num_attrs;
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 39b356160937..671805a79a15 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -124,6 +124,10 @@ struct gpio_desc {
 #ifdef CONFIG_OF_DYNAMIC
 	struct device_node	*hog;
 #endif
+#ifdef CONFIG_GPIO_CDEV
+	/* debounce period in microseconds */
+	unsigned int		debounce_period;
+#endif
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 11/18] gpio: uapi: document uAPI v1 as deprecated
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (9 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 10/18] gpiolib: cdev: support setting debounce Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-08-04 19:43   ` Bartosz Golaszewski
  2020-07-25  4:19 ` [PATCH v2 12/18] tools: gpio: port lsgpio to v2 uAPI Kent Gibson
                   ` (6 subsequent siblings)
  17 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Update uAPI documentation to deprecate v1 structs and ioctls.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 include/uapi/linux/gpio.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 3f6db33014f0..92a74c245534 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -278,6 +278,9 @@ struct gpioline_event {
 
 /*
  *  ABI v1
+ *
+ * This version of the ABI is deprecated and will be removed in the future.
+ * Use the latest version if the ABI, defined above, instead.
  */
 
 /* Informational flags */
@@ -301,6 +304,9 @@ struct gpioline_event {
  * @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
+ *
+ * This struct part of ABI v1 and is deprecated.
+ * Use struct gpioline_info_v2 instead.
  */
 struct gpioline_info {
 	__u32 line_offset;
@@ -325,6 +331,9 @@ struct gpioline_info {
  * guarantee there are no implicit holes between it and subsequent members.
  * The 20-byte padding at the end makes sure we don't add any implicit padding
  * at the end of the structure on 64-bit architectures.
+ *
+ * This struct part of ABI v1 and is deprecated.
+ * Use struct gpioline_info_changed_v2 instead.
  */
 struct gpioline_info_changed {
 	struct gpioline_info info;
@@ -364,6 +373,9 @@ struct gpioline_info_changed {
  * @fd: if successful this field will contain a valid anonymous file handle
  * after a GPIO_GET_LINEHANDLE_IOCTL operation, zero or negative value
  * means error
+ *
+ * This struct part of ABI v1 and is deprecated.
+ * Use struct gpioline_request instead.
  */
 struct gpiohandle_request {
 	__u32 lineoffsets[GPIOHANDLES_MAX];
@@ -383,6 +395,9 @@ struct gpiohandle_request {
  * this specifies the default output value, should be 0 (low) or
  * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
  * @padding: reserved for future use and should be zero filled
+ *
+ * This struct part of ABI v1 and is deprecated.
+ * Use struct gpioline_config instead.
  */
 struct gpiohandle_config {
 	__u32 flags;
@@ -395,6 +410,9 @@ struct gpiohandle_config {
  * @values: when getting the state of lines this contains the current
  * state of a line, when setting the state of lines these should contain
  * the desired target state
+ *
+ * This struct part of ABI v1 and is deprecated.
+ * Use struct gpioline_values instead.
  */
 struct gpiohandle_data {
 	__u8 values[GPIOHANDLES_MAX];
@@ -418,6 +436,9 @@ struct gpiohandle_data {
  * @fd: if successful this field will contain a valid anonymous file handle
  * after a GPIO_GET_LINEEVENT_IOCTL operation, zero or negative value
  * means error
+ *
+ * This struct part of ABI v1 and is deprecated.
+ * Use struct gpioline_request instead.
  */
 struct gpioevent_request {
 	__u32 lineoffset;
@@ -437,6 +458,9 @@ struct gpioevent_request {
  * struct gpioevent_data - The actual event being pushed to userspace
  * @timestamp: best estimate of time of event occurrence, in nanoseconds
  * @id: event identifier
+ *
+ * This struct part of ABI v1 and is deprecated.
+ * Use struct gpioline_event instead.
  */
 struct gpioevent_data {
 	__u64 timestamp;
@@ -461,6 +485,8 @@ struct gpioevent_data {
 
 /*
  * v1 ioctl()s
+ *
+ * These ioctl()s are deprecated.  Use the v2 equivalent instead.
  */
 #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
 #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 12/18] tools: gpio: port lsgpio to v2 uAPI
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (10 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 11/18] gpio: uapi: document uAPI v1 as deprecated Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 13/18] tools: gpio: port gpio-watch " Kent Gibson
                   ` (5 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Port the lsgpio tool to the latest GPIO uAPI.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/lsgpio.c | 102 ++++++++++++++++++++------------------------
 1 file changed, 47 insertions(+), 55 deletions(-)

diff --git a/tools/gpio/lsgpio.c b/tools/gpio/lsgpio.c
index b08d7a5e779b..0a993d535707 100644
--- a/tools/gpio/lsgpio.c
+++ b/tools/gpio/lsgpio.c
@@ -23,58 +23,50 @@
 
 #include "gpio-utils.h"
 
-struct gpio_flag {
-	char *name;
-	unsigned long mask;
-};
-
-struct gpio_flag flagnames[] = {
-	{
-		.name = "kernel",
-		.mask = GPIOLINE_FLAG_KERNEL,
-	},
-	{
-		.name = "output",
-		.mask = GPIOLINE_FLAG_IS_OUT,
-	},
-	{
-		.name = "active-low",
-		.mask = GPIOLINE_FLAG_ACTIVE_LOW,
-	},
-	{
-		.name = "open-drain",
-		.mask = GPIOLINE_FLAG_OPEN_DRAIN,
-	},
-	{
-		.name = "open-source",
-		.mask = GPIOLINE_FLAG_OPEN_SOURCE,
-	},
-	{
-		.name = "pull-up",
-		.mask = GPIOLINE_FLAG_BIAS_PULL_UP,
-	},
-	{
-		.name = "pull-down",
-		.mask = GPIOLINE_FLAG_BIAS_PULL_DOWN,
-	},
-	{
-		.name = "bias-disabled",
-		.mask = GPIOLINE_FLAG_BIAS_DISABLE,
-	},
-};
-
-void print_flags(unsigned long flags)
+static void print_attributes(struct gpioline_info_v2 *info)
 {
 	int i;
-	int printed = 0;
-
-	for (i = 0; i < ARRAY_SIZE(flagnames); i++) {
-		if (flags & flagnames[i].mask) {
-			if (printed)
-				fprintf(stdout, " ");
-			fprintf(stdout, "%s", flagnames[i].name);
-			printed++;
-		}
+	const char *field_format = "%s";
+
+	if (info->flags & GPIOLINE_FLAG_V2_USED) {
+		fprintf(stdout, field_format, "used");
+		field_format = ", %s";
+	}
+
+	if (info->flags & GPIOLINE_FLAG_V2_OUTPUT)
+		fprintf(stdout, field_format, "output");
+	else
+		fprintf(stdout, field_format, "input");
+
+	field_format = ", %s";
+
+	if (info->flags & GPIOLINE_FLAG_V2_ACTIVE_LOW)
+		fprintf(stdout, field_format, "active-low");
+
+	if (info->flags & GPIOLINE_FLAG_V2_OPEN_DRAIN)
+		fprintf(stdout, field_format, "open-drain");
+	if (info->flags & GPIOLINE_FLAG_V2_OPEN_SOURCE)
+		fprintf(stdout, field_format, "open-source");
+
+	if (info->flags & GPIOLINE_FLAG_V2_BIAS_DISABLED)
+		fprintf(stdout, field_format, "bias-disabled");
+	if (info->flags == GPIOLINE_FLAG_V2_BIAS_PULL_UP)
+		fprintf(stdout, field_format, "pull-up");
+	if (info->flags == GPIOLINE_FLAG_V2_BIAS_PULL_DOWN)
+		fprintf(stdout, field_format, "pull-down");
+
+	if ((info->flags & GPIOLINE_FLAG_V2_EDGE_RISING) &&
+	    (info->flags & GPIOLINE_FLAG_V2_EDGE_FALLING))
+		fprintf(stdout, field_format, "both-edges");
+	else if (info->flags & GPIOLINE_FLAG_V2_EDGE_RISING)
+		fprintf(stdout, field_format, "rising-edge");
+	else if (info->flags & GPIOLINE_FLAG_V2_EDGE_FALLING)
+		fprintf(stdout, field_format, "falling-edge");
+
+	for (i = 0; i < info->num_attrs; i++) {
+		if (info->attrs[i].id == GPIOLINE_ATTR_ID_DEBOUNCE)
+			fprintf(stdout, ", debounce_period=%dusec",
+				info->attrs[0].debounce_period);
 	}
 }
 
@@ -109,18 +101,18 @@ int list_device(const char *device_name)
 
 	/* Loop over the lines and print info */
 	for (i = 0; i < cinfo.lines; i++) {
-		struct gpioline_info linfo;
+		struct gpioline_info_v2 linfo;
 
 		memset(&linfo, 0, sizeof(linfo));
-		linfo.line_offset = i;
+		linfo.offset = i;
 
-		ret = ioctl(fd, GPIO_GET_LINEINFO_IOCTL, &linfo);
+		ret = ioctl(fd, GPIO_GET_LINEINFO_V2_IOCTL, &linfo);
 		if (ret == -1) {
 			ret = -errno;
 			perror("Failed to issue LINEINFO IOCTL\n");
 			goto exit_close_error;
 		}
-		fprintf(stdout, "\tline %2d:", linfo.line_offset);
+		fprintf(stdout, "\tline %2d:", linfo.offset);
 		if (linfo.name[0])
 			fprintf(stdout, " \"%s\"", linfo.name);
 		else
@@ -131,7 +123,7 @@ int list_device(const char *device_name)
 			fprintf(stdout, " unused");
 		if (linfo.flags) {
 			fprintf(stdout, " [");
-			print_flags(linfo.flags);
+			print_attributes(&linfo);
 			fprintf(stdout, "]");
 		}
 		fprintf(stdout, "\n");
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 13/18] tools: gpio: port gpio-watch to v2 uAPI
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (11 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 12/18] tools: gpio: port lsgpio to v2 uAPI Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 14/18] tools: gpio: rename nlines to num_lines Kent Gibson
                   ` (4 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Port the gpio-watch tool to the latest GPIO uAPI.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/gpio-watch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/gpio/gpio-watch.c b/tools/gpio/gpio-watch.c
index 5cea24fddfa7..0dd5a04ab250 100644
--- a/tools/gpio/gpio-watch.c
+++ b/tools/gpio/gpio-watch.c
@@ -21,8 +21,8 @@
 
 int main(int argc, char **argv)
 {
-	struct gpioline_info_changed chg;
-	struct gpioline_info req;
+	struct gpioline_info_changed_v2 chg;
+	struct gpioline_info_v2 req;
 	struct pollfd pfd;
 	int fd, i, j, ret;
 	char *event, *end;
@@ -40,11 +40,11 @@ int main(int argc, char **argv)
 	for (i = 0, j = 2; i < argc - 2; i++, j++) {
 		memset(&req, 0, sizeof(req));
 
-		req.line_offset = strtoul(argv[j], &end, 0);
+		req.offset = strtoul(argv[j], &end, 0);
 		if (*end != '\0')
 			goto err_usage;
 
-		ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_IOCTL, &req);
+		ret = ioctl(fd, GPIO_GET_LINEINFO_WATCH_V2_IOCTL, &req);
 		if (ret) {
 			perror("unable to set up line watch");
 			return EXIT_FAILURE;
@@ -87,7 +87,7 @@ int main(int argc, char **argv)
 			}
 
 			printf("line %u: %s at %llu\n",
-			       chg.info.line_offset, event, chg.timestamp);
+			       chg.info.offset, event, chg.timestamp);
 		}
 	}
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 14/18] tools: gpio: rename nlines to num_lines
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (12 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 13/18] tools: gpio: port gpio-watch " Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 15/18] tools: gpio: port gpio-hammer to v2 uAPI Kent Gibson
                   ` (3 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Rename nlines to num_lines to be consistent with other usage for fields
describing the number of entries in an array.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/gpio-utils.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
index 16a5d9cb9da2..d527980bcb94 100644
--- a/tools/gpio/gpio-utils.c
+++ b/tools/gpio/gpio-utils.c
@@ -38,7 +38,7 @@
  *			such as "gpiochip0"
  * @lines:		An array desired lines, specified by offset
  *			index for the associated GPIO device.
- * @nline:		The number of lines to request.
+ * @num_lines:		The number of lines to request.
  * @flag:		The new flag for requsted gpio. Reference
  *			"linux/gpio.h" for the meaning of flag.
  * @data:		Default value will be set to gpio when flag is
@@ -56,7 +56,7 @@
  *			On failure return the errno.
  */
 int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
-				 unsigned int nlines, unsigned int flag,
+				 unsigned int num_lines, unsigned int flag,
 				 struct gpiohandle_data *data,
 				 const char *consumer_label)
 {
@@ -78,12 +78,12 @@ int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
 		goto exit_free_name;
 	}
 
-	for (i = 0; i < nlines; i++)
+	for (i = 0; i < num_lines; i++)
 		req.lineoffsets[i] = lines[i];
 
 	req.flags = flag;
 	strcpy(req.consumer_label, consumer_label);
-	req.lines = nlines;
+	req.lines = num_lines;
 	if (flag & GPIOHANDLE_REQUEST_OUTPUT)
 		memcpy(req.default_values, data, sizeof(req.default_values));
 
@@ -194,20 +194,20 @@ int gpiotools_get(const char *device_name, unsigned int line)
  *			such as "gpiochip0".
  * @lines:		An array desired lines, specified by offset
  *			index for the associated GPIO device.
- * @nline:		The number of lines to request.
+ * @num_lines:		The number of lines to request.
  * @data:		The array of values get from gpiochip.
  *
  * Return:		On success return 0;
  *			On failure return the errno.
  */
 int gpiotools_gets(const char *device_name, unsigned int *lines,
-		   unsigned int nlines, struct gpiohandle_data *data)
+		   unsigned int num_lines, struct gpiohandle_data *data)
 {
 	int fd;
 	int ret;
 	int ret_close;
 
-	ret = gpiotools_request_linehandle(device_name, lines, nlines,
+	ret = gpiotools_request_linehandle(device_name, lines, num_lines,
 					   GPIOHANDLE_REQUEST_INPUT, data,
 					   CONSUMER);
 	if (ret < 0)
@@ -245,7 +245,7 @@ int gpiotools_set(const char *device_name, unsigned int line,
  *			such as "gpiochip0".
  * @lines:		An array desired lines, specified by offset
  *			index for the associated GPIO device.
- * @nline:		The number of lines to request.
+ * @num_lines:		The number of lines to request.
  * @data:		The array of values set to gpiochip, must be
  *			0(low) or 1(high).
  *
@@ -253,11 +253,11 @@ int gpiotools_set(const char *device_name, unsigned int line,
  *			On failure return the errno.
  */
 int gpiotools_sets(const char *device_name, unsigned int *lines,
-		   unsigned int nlines, struct gpiohandle_data *data)
+		   unsigned int num_lines, struct gpiohandle_data *data)
 {
 	int ret;
 
-	ret = gpiotools_request_linehandle(device_name, lines, nlines,
+	ret = gpiotools_request_linehandle(device_name, lines, num_lines,
 					   GPIOHANDLE_REQUEST_OUTPUT, data,
 					   CONSUMER);
 	if (ret < 0)
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 15/18] tools: gpio: port gpio-hammer to v2 uAPI
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (13 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 14/18] tools: gpio: rename nlines to num_lines Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 16/18] tools: gpio: port gpio-event-mon " Kent Gibson
                   ` (2 subsequent siblings)
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Port the gpio-hammer tool to the latest GPIO uAPI.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/gpio-hammer.c |  27 +++++-----
 tools/gpio/gpio-utils.c  | 109 +++++++++++++++++++++------------------
 tools/gpio/gpio-utils.h  |  48 +++++++++++++----
 3 files changed, 112 insertions(+), 72 deletions(-)

diff --git a/tools/gpio/gpio-hammer.c b/tools/gpio/gpio-hammer.c
index 9fd926e8cb52..1ab58f8fc03e 100644
--- a/tools/gpio/gpio-hammer.c
+++ b/tools/gpio/gpio-hammer.c
@@ -25,23 +25,25 @@
 int hammer_device(const char *device_name, unsigned int *lines, int nlines,
 		  unsigned int loops)
 {
-	struct gpiohandle_data data;
+	struct gpioline_values values;
+	struct gpioline_config config;
 	char swirr[] = "-\\|/";
 	int fd;
 	int ret;
 	int i, j;
 	unsigned int iteration = 0;
 
-	memset(&data.values, 0, sizeof(data.values));
-	ret = gpiotools_request_linehandle(device_name, lines, nlines,
-					   GPIOHANDLE_REQUEST_OUTPUT, &data,
-					   "gpio-hammer");
+	memset(&config, 0, sizeof(config));
+	config.flags = GPIOLINE_FLAG_V2_OUTPUT;
+
+	ret = gpiotools_request_line(device_name, lines, nlines,
+				     &config, "gpio-hammer");
 	if (ret < 0)
 		goto exit_error;
 	else
 		fd = ret;
 
-	ret = gpiotools_get_values(fd, &data);
+	ret = gpiotools_get_values(fd, &values);
 	if (ret < 0)
 		goto exit_close_error;
 
@@ -53,7 +55,7 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
 	}
 	fprintf(stdout, "] on %s, initial states: [", device_name);
 	for (i = 0; i < nlines; i++) {
-		fprintf(stdout, "%d", data.values[i]);
+		fprintf(stdout, "%d", gpiotools_test_bit(values.bits, i));
 		if (i != (nlines - 1))
 			fprintf(stdout, ", ");
 	}
@@ -64,14 +66,14 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
 	while (1) {
 		/* Invert all lines so we blink */
 		for (i = 0; i < nlines; i++)
-			data.values[i] = !data.values[i];
+			gpiotools_change_bit(values.bits, i);
 
-		ret = gpiotools_set_values(fd, &data);
+		ret = gpiotools_set_values(fd, &values);
 		if (ret < 0)
 			goto exit_close_error;
 
 		/* Re-read values to get status */
-		ret = gpiotools_get_values(fd, &data);
+		ret = gpiotools_get_values(fd, &values);
 		if (ret < 0)
 			goto exit_close_error;
 
@@ -82,7 +84,8 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
 
 		fprintf(stdout, "[");
 		for (i = 0; i < nlines; i++) {
-			fprintf(stdout, "%d: %d", lines[i], data.values[i]);
+			fprintf(stdout, "%d: %d", lines[i],
+				gpiotools_test_bit(values.bits, i));
 			if (i != (nlines - 1))
 				fprintf(stdout, ", ");
 		}
@@ -97,7 +100,7 @@ int hammer_device(const char *device_name, unsigned int *lines, int nlines,
 	ret = 0;
 
 exit_close_error:
-	gpiotools_release_linehandle(fd);
+	gpiotools_release_line(fd);
 exit_error:
 	return ret;
 }
diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
index d527980bcb94..435d4fc64644 100644
--- a/tools/gpio/gpio-utils.c
+++ b/tools/gpio/gpio-utils.c
@@ -33,34 +33,31 @@
  * release these lines.
  */
 /**
- * gpiotools_request_linehandle() - request gpio lines in a gpiochip
+ * gpiotools_request_line() - request gpio lines in a gpiochip
  * @device_name:	The name of gpiochip without prefix "/dev/",
  *			such as "gpiochip0"
  * @lines:		An array desired lines, specified by offset
  *			index for the associated GPIO device.
  * @num_lines:		The number of lines to request.
- * @flag:		The new flag for requsted gpio. Reference
- *			"linux/gpio.h" for the meaning of flag.
- * @data:		Default value will be set to gpio when flag is
- *			GPIOHANDLE_REQUEST_OUTPUT.
- * @consumer_label:	The name of consumer, such as "sysfs",
+ * @config:		The new config for requested gpio. Reference
+ *			"linux/gpio.h" for config details.
+ * @consumer:		The name of consumer, such as "sysfs",
  *			"powerkey". This is useful for other users to
  *			know who is using.
  *
  * Request gpio lines through the ioctl provided by chardev. User
  * could call gpiotools_set_values() and gpiotools_get_values() to
  * read and write respectively through the returned fd. Call
- * gpiotools_release_linehandle() to release these lines after that.
+ * gpiotools_release_line() to release these lines after that.
  *
  * Return:		On success return the fd;
  *			On failure return the errno.
  */
-int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
-				 unsigned int num_lines, unsigned int flag,
-				 struct gpiohandle_data *data,
-				 const char *consumer_label)
+int gpiotools_request_line(const char *device_name, unsigned int *lines,
+			   unsigned int num_lines, struct gpioline_config *config,
+			   const char *consumer)
 {
-	struct gpiohandle_request req;
+	struct gpioline_request req;
 	char *chrdev_name;
 	int fd;
 	int i;
@@ -78,20 +75,19 @@ int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
 		goto exit_free_name;
 	}
 
+	memset(&req, 0, sizeof(req));
 	for (i = 0; i < num_lines; i++)
-		req.lineoffsets[i] = lines[i];
+		req.offsets[i] = lines[i];
 
-	req.flags = flag;
-	strcpy(req.consumer_label, consumer_label);
-	req.lines = num_lines;
-	if (flag & GPIOHANDLE_REQUEST_OUTPUT)
-		memcpy(req.default_values, data, sizeof(req.default_values));
+	req.config = *config;
+	strcpy(req.consumer, consumer);
+	req.num_lines = num_lines;
 
-	ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
+	ret = ioctl(fd, GPIO_GET_LINE_IOCTL, &req);
 	if (ret == -1) {
 		ret = -errno;
 		fprintf(stderr, "Failed to issue %s (%d), %s\n",
-			"GPIO_GET_LINEHANDLE_IOCTL", ret, strerror(errno));
+			"GPIO_GET_LINE_IOCTL", ret, strerror(errno));
 	}
 
 	if (close(fd) == -1)
@@ -103,17 +99,17 @@ int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
 /**
  * gpiotools_set_values(): Set the value of gpio(s)
  * @fd:			The fd returned by
- *			gpiotools_request_linehandle().
- * @data:		The array of values want to set.
+ *			gpiotools_request_line().
+ * @values:		The array of values want to set.
  *
  * Return:		On success return 0;
  *			On failure return the errno.
  */
-int gpiotools_set_values(const int fd, struct gpiohandle_data *data)
+int gpiotools_set_values(const int fd, struct gpioline_values *values)
 {
 	int ret;
 
-	ret = ioctl(fd, GPIOHANDLE_SET_LINE_VALUES_IOCTL, data);
+	ret = ioctl(fd, GPIOLINE_SET_VALUES_IOCTL, values);
 	if (ret == -1) {
 		ret = -errno;
 		fprintf(stderr, "Failed to issue %s (%d), %s\n",
@@ -127,17 +123,17 @@ int gpiotools_set_values(const int fd, struct gpiohandle_data *data)
 /**
  * gpiotools_get_values(): Get the value of gpio(s)
  * @fd:			The fd returned by
- *			gpiotools_request_linehandle().
- * @data:		The array of values get from hardware.
+ *			gpiotools_request_line().
+ * @values:		The array of values get from hardware.
  *
  * Return:		On success return 0;
  *			On failure return the errno.
  */
-int gpiotools_get_values(const int fd, struct gpiohandle_data *data)
+int gpiotools_get_values(const int fd, struct gpioline_values *values)
 {
 	int ret;
 
-	ret = ioctl(fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, data);
+	ret = ioctl(fd, GPIOLINE_GET_VALUES_IOCTL, values);
 	if (ret == -1) {
 		ret = -errno;
 		fprintf(stderr, "Failed to issue %s (%d), %s\n",
@@ -149,14 +145,14 @@ int gpiotools_get_values(const int fd, struct gpiohandle_data *data)
 }
 
 /**
- * gpiotools_release_linehandle(): Release the line(s) of gpiochip
+ * gpiotools_release_line(): Release the line(s) of gpiochip
  * @fd:			The fd returned by
- *			gpiotools_request_linehandle().
+ *			gpiotools_request_line().
  *
  * Return:		On success return 0;
  *			On failure return the errno.
  */
-int gpiotools_release_linehandle(const int fd)
+int gpiotools_release_line(const int fd)
 {
 	int ret;
 
@@ -180,11 +176,11 @@ int gpiotools_release_linehandle(const int fd)
  */
 int gpiotools_get(const char *device_name, unsigned int line)
 {
-	struct gpiohandle_data data;
+	struct gpioline_values values;
 	unsigned int lines[] = {line};
 
-	gpiotools_gets(device_name, lines, 1, &data);
-	return data.values[0];
+	gpiotools_gets(device_name, lines, 1, &values);
+	return values.bits[0] & 1;
 }
 
 
@@ -195,27 +191,29 @@ int gpiotools_get(const char *device_name, unsigned int line)
  * @lines:		An array desired lines, specified by offset
  *			index for the associated GPIO device.
  * @num_lines:		The number of lines to request.
- * @data:		The array of values get from gpiochip.
+ * @values:		The array of values get from gpiochip.
  *
  * Return:		On success return 0;
  *			On failure return the errno.
  */
 int gpiotools_gets(const char *device_name, unsigned int *lines,
-		   unsigned int num_lines, struct gpiohandle_data *data)
+		   unsigned int num_lines, struct gpioline_values *values)
 {
 	int fd;
 	int ret;
 	int ret_close;
+	struct gpioline_config config;
 
-	ret = gpiotools_request_linehandle(device_name, lines, num_lines,
-					   GPIOHANDLE_REQUEST_INPUT, data,
-					   CONSUMER);
+	memset(&config, 0, sizeof(config));
+	config.flags = GPIOLINE_FLAG_V2_INPUT;
+	ret = gpiotools_request_line(device_name, lines, num_lines,
+				     &config, CONSUMER);
 	if (ret < 0)
 		return ret;
 
 	fd = ret;
-	ret = gpiotools_get_values(fd, data);
-	ret_close = gpiotools_release_linehandle(fd);
+	ret = gpiotools_get_values(fd, values);
+	ret_close = gpiotools_release_line(fd);
 	return ret < 0 ? ret : ret_close;
 }
 
@@ -232,11 +230,13 @@ int gpiotools_gets(const char *device_name, unsigned int *lines,
 int gpiotools_set(const char *device_name, unsigned int line,
 		  unsigned int value)
 {
-	struct gpiohandle_data data;
+	struct gpioline_values values;
 	unsigned int lines[] = {line};
 
-	data.values[0] = value;
-	return gpiotools_sets(device_name, lines, 1, &data);
+	memset(&values, 0, sizeof(values));
+	if (value)
+		values.bits[0] |= 1;
+	return gpiotools_sets(device_name, lines, 1, &values);
 }
 
 /**
@@ -246,22 +246,29 @@ int gpiotools_set(const char *device_name, unsigned int line,
  * @lines:		An array desired lines, specified by offset
  *			index for the associated GPIO device.
  * @num_lines:		The number of lines to request.
- * @data:		The array of values set to gpiochip, must be
+ * @value:		The array of values set to gpiochip, must be
  *			0(low) or 1(high).
  *
  * Return:		On success return 0;
  *			On failure return the errno.
  */
 int gpiotools_sets(const char *device_name, unsigned int *lines,
-		   unsigned int num_lines, struct gpiohandle_data *data)
+		   unsigned int num_lines, struct gpioline_values *values)
 {
-	int ret;
+	int ret, i;
+	struct gpioline_config config;
 
-	ret = gpiotools_request_linehandle(device_name, lines, num_lines,
-					   GPIOHANDLE_REQUEST_OUTPUT, data,
-					   CONSUMER);
+	memset(&config, 0, sizeof(config));
+	config.flags = GPIOLINE_FLAG_V2_OUTPUT;
+	config.num_attrs = 1;
+	for (i = 0; i < num_lines; i++)
+		gpiotools_set_bit(config.attrs[0].mask, i);
+	config.attrs[0].attr.id = GPIOLINE_ATTR_ID_OUTPUT_VALUES;
+	config.attrs[0].attr.values = *values;
+	ret = gpiotools_request_line(device_name, lines, num_lines,
+				     &config, CONSUMER);
 	if (ret < 0)
 		return ret;
 
-	return gpiotools_release_linehandle(ret);
+	return gpiotools_release_line(ret);
 }
diff --git a/tools/gpio/gpio-utils.h b/tools/gpio/gpio-utils.h
index cf37f13f3dcb..9357765b6b79 100644
--- a/tools/gpio/gpio-utils.h
+++ b/tools/gpio/gpio-utils.h
@@ -12,7 +12,9 @@
 #ifndef _GPIO_UTILS_H_
 #define _GPIO_UTILS_H_
 
+#include <stdbool.h>
 #include <string.h>
+#include <linux/types.h>
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 
@@ -22,20 +24,48 @@ static inline int check_prefix(const char *str, const char *prefix)
 		strncmp(str, prefix, strlen(prefix)) == 0;
 }
 
-int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
-				 unsigned int nlines, unsigned int flag,
-				 struct gpiohandle_data *data,
-				 const char *consumer_label);
-int gpiotools_set_values(const int fd, struct gpiohandle_data *data);
-int gpiotools_get_values(const int fd, struct gpiohandle_data *data);
-int gpiotools_release_linehandle(const int fd);
+int gpiotools_request_line(const char *device_name, unsigned int *lines,
+			   unsigned int nlines, struct gpioline_config *config,
+			   const char *consumer_label);
+int gpiotools_set_values(const int fd, struct gpioline_values *values);
+int gpiotools_get_values(const int fd, struct gpioline_values *values);
+int gpiotools_release_line(const int fd);
 
 int gpiotools_get(const char *device_name, unsigned int line);
 int gpiotools_gets(const char *device_name, unsigned int *lines,
-		   unsigned int nlines, struct gpiohandle_data *data);
+		   unsigned int nlines, struct gpioline_values *values);
 int gpiotools_set(const char *device_name, unsigned int line,
 		  unsigned int value);
 int gpiotools_sets(const char *device_name, unsigned int *lines,
-		   unsigned int nlines, struct gpiohandle_data *data);
+		   unsigned int nlines, struct gpioline_values *values);
+
+/* helper functions for gpioline_values bitmap bits */
+static inline void gpiotools_set_bit(__u64 b[], int n)
+{
+	b[n>>6] |= 1UL << (n%64);
+}
+
+static inline void gpiotools_change_bit(__u64 b[], int n)
+{
+	b[n>>6] ^= 1UL << (n%64);
+}
+
+static inline void gpiotools_clear_bit(__u64 b[], int n)
+{
+	b[n>>6] &= ~(1UL << (n%64));
+}
+
+static inline int gpiotools_test_bit(__u64 b[], int n)
+{
+	return !!(b[n>>6] & 1 <<  (n%64));
+}
+
+static inline void gpiotools_assign_bit(__u64 b[], int n, bool value)
+{
+	if (value)
+		gpiotools_set_bit(b, n);
+	else
+		gpiotools_clear_bit(b, n);
+}
 
 #endif /* _GPIO_UTILS_H_ */
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 16/18] tools: gpio: port gpio-event-mon to v2 uAPI
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (14 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 15/18] tools: gpio: port gpio-hammer to v2 uAPI Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 17/18] tools: gpio: add debounce support to gpio-event-mon Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 18/18] tools: gpio: add multi-line monitoring " Kent Gibson
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Port the gpio-event-mon tool to the latest GPIO uAPI.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/gpio-event-mon.c | 89 +++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

diff --git a/tools/gpio/gpio-event-mon.c b/tools/gpio/gpio-event-mon.c
index 1a303a81aeef..5da980f78881 100644
--- a/tools/gpio/gpio-event-mon.c
+++ b/tools/gpio/gpio-event-mon.c
@@ -23,17 +23,16 @@
 #include <sys/ioctl.h>
 #include <sys/types.h>
 #include <linux/gpio.h>
+#include "gpio-utils.h"
 
 int monitor_device(const char *device_name,
 		   unsigned int line,
-		   uint32_t handleflags,
-		   uint32_t eventflags,
+		   struct gpioline_config *config,
 		   unsigned int loops)
 {
-	struct gpioevent_request req;
-	struct gpiohandle_data data;
+	struct gpioline_values values;
 	char *chrdev_name;
-	int fd;
+	int cfd, lfd;
 	int ret;
 	int i = 0;
 
@@ -41,44 +40,37 @@ int monitor_device(const char *device_name,
 	if (ret < 0)
 		return -ENOMEM;
 
-	fd = open(chrdev_name, 0);
-	if (fd == -1) {
+	cfd = open(chrdev_name, 0);
+	if (cfd == -1) {
 		ret = -errno;
 		fprintf(stderr, "Failed to open %s\n", chrdev_name);
 		goto exit_free_name;
 	}
 
-	req.lineoffset = line;
-	req.handleflags = handleflags;
-	req.eventflags = eventflags;
-	strcpy(req.consumer_label, "gpio-event-mon");
-
-	ret = ioctl(fd, GPIO_GET_LINEEVENT_IOCTL, &req);
-	if (ret == -1) {
-		ret = -errno;
-		fprintf(stderr, "Failed to issue GET EVENT "
-			"IOCTL (%d)\n",
-			ret);
-		goto exit_close_error;
-	}
+	ret = gpiotools_request_line(device_name, &line, 1, config,
+				     "gpio-event-mon");
+	if (ret < 0)
+		goto exit_device_close;
+	else
+		lfd = ret;
 
 	/* Read initial states */
-	ret = ioctl(req.fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
-	if (ret == -1) {
-		ret = -errno;
-		fprintf(stderr, "Failed to issue GPIOHANDLE GET LINE "
-			"VALUES IOCTL (%d)\n",
+	ret = gpiotools_get_values(lfd, &values);
+	if (ret < 0) {
+		fprintf(stderr,
+			"Failed to issue GPIO LINE GET VALUES IOCTL (%d)\n",
 			ret);
-		goto exit_close_error;
+		goto exit_line_close;
 	}
 
 	fprintf(stdout, "Monitoring line %d on %s\n", line, device_name);
-	fprintf(stdout, "Initial line value: %d\n", data.values[0]);
+	fprintf(stdout, "Initial line value: %d\n",
+		gpiotools_test_bit(values.bits, 0));
 
 	while (1) {
-		struct gpioevent_data event;
+		struct gpioline_event event;
 
-		ret = read(req.fd, &event, sizeof(event));
+		ret = read(lfd, &event, sizeof(event));
 		if (ret == -1) {
 			if (errno == -EAGAIN) {
 				fprintf(stderr, "nothing available\n");
@@ -96,12 +88,14 @@ int monitor_device(const char *device_name,
 			ret = -EIO;
 			break;
 		}
-		fprintf(stdout, "GPIO EVENT %llu: ", event.timestamp);
+		fprintf(stdout, "GPIO EVENT at %llu on line %d (%d|%d) ",
+			event.timestamp, event.offset, event.line_seqno,
+			event.seqno);
 		switch (event.id) {
-		case GPIOEVENT_EVENT_RISING_EDGE:
+		case GPIOLINE_EVENT_RISING_EDGE:
 			fprintf(stdout, "rising edge");
 			break;
-		case GPIOEVENT_EVENT_FALLING_EDGE:
+		case GPIOLINE_EVENT_FALLING_EDGE:
 			fprintf(stdout, "falling edge");
 			break;
 		default:
@@ -114,8 +108,11 @@ int monitor_device(const char *device_name,
 			break;
 	}
 
-exit_close_error:
-	if (close(fd) == -1)
+exit_line_close:
+	if (close(lfd) == -1)
+		perror("Failed to close line file");
+exit_device_close:
+	if (close(cfd) == -1)
 		perror("Failed to close GPIO character device file");
 exit_free_name:
 	free(chrdev_name);
@@ -140,15 +137,20 @@ void print_usage(void)
 	);
 }
 
+#define EDGE_FLAGS \
+	(GPIOLINE_FLAG_V2_EDGE_RISING | \
+	 GPIOLINE_FLAG_V2_EDGE_FALLING)
+
 int main(int argc, char **argv)
 {
 	const char *device_name = NULL;
 	unsigned int line = -1;
 	unsigned int loops = 0;
-	uint32_t handleflags = GPIOHANDLE_REQUEST_INPUT;
-	uint32_t eventflags = 0;
+	struct gpioline_config config;
 	int c;
 
+	memset(&config, 0, sizeof(config));
+	config.flags = GPIOLINE_FLAG_V2_INPUT;
 	while ((c = getopt(argc, argv, "c:n:o:dsrf?")) != -1) {
 		switch (c) {
 		case 'c':
@@ -161,16 +163,16 @@ int main(int argc, char **argv)
 			line = strtoul(optarg, NULL, 10);
 			break;
 		case 'd':
-			handleflags |= GPIOHANDLE_REQUEST_OPEN_DRAIN;
+			config.flags |= GPIOLINE_FLAG_V2_OPEN_DRAIN;
 			break;
 		case 's':
-			handleflags |= GPIOHANDLE_REQUEST_OPEN_SOURCE;
+			config.flags |= GPIOLINE_FLAG_V2_OPEN_SOURCE;
 			break;
 		case 'r':
-			eventflags |= GPIOEVENT_REQUEST_RISING_EDGE;
+			config.flags |= GPIOLINE_FLAG_V2_EDGE_RISING;
 			break;
 		case 'f':
-			eventflags |= GPIOEVENT_REQUEST_FALLING_EDGE;
+			config.flags |= GPIOLINE_FLAG_V2_EDGE_FALLING;
 			break;
 		case '?':
 			print_usage();
@@ -182,11 +184,10 @@ int main(int argc, char **argv)
 		print_usage();
 		return -1;
 	}
-	if (!eventflags) {
+	if (!(config.flags & EDGE_FLAGS)) {
 		printf("No flags specified, listening on both rising and "
 		       "falling edges\n");
-		eventflags = GPIOEVENT_REQUEST_BOTH_EDGES;
+		config.flags |= EDGE_FLAGS;
 	}
-	return monitor_device(device_name, line, handleflags,
-			      eventflags, loops);
+	return monitor_device(device_name, line, &config, loops);
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 17/18] tools: gpio: add debounce support to gpio-event-mon
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (15 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 16/18] tools: gpio: port gpio-event-mon " Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  2020-07-25  4:19 ` [PATCH v2 18/18] tools: gpio: add multi-line monitoring " Kent Gibson
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Add support for debouncing monitored lines to gpio-event-mon.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/gpio-event-mon.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/gpio/gpio-event-mon.c b/tools/gpio/gpio-event-mon.c
index 5da980f78881..b64427d78942 100644
--- a/tools/gpio/gpio-event-mon.c
+++ b/tools/gpio/gpio-event-mon.c
@@ -129,11 +129,12 @@ void print_usage(void)
 		"  -s         Set line as open source\n"
 		"  -r         Listen for rising edges\n"
 		"  -f         Listen for falling edges\n"
+		"  -b <n>     Debounce the line with period n microseconds\n"
 		" [-c <n>]    Do <n> loops (optional, infinite loop if not stated)\n"
 		"  -?         This helptext\n"
 		"\n"
 		"Example:\n"
-		"gpio-event-mon -n gpiochip0 -o 4 -r -f\n"
+		"gpio-event-mon -n gpiochip0 -o 4 -r -f -b 10000\n"
 	);
 }
 
@@ -147,11 +148,11 @@ int main(int argc, char **argv)
 	unsigned int line = -1;
 	unsigned int loops = 0;
 	struct gpioline_config config;
-	int c;
+	int c, attr;
 
 	memset(&config, 0, sizeof(config));
 	config.flags = GPIOLINE_FLAG_V2_INPUT;
-	while ((c = getopt(argc, argv, "c:n:o:dsrf?")) != -1) {
+	while ((c = getopt(argc, argv, "c:n:o:b:dsrf?")) != -1) {
 		switch (c) {
 		case 'c':
 			loops = strtoul(optarg, NULL, 10);
@@ -162,6 +163,14 @@ int main(int argc, char **argv)
 		case 'o':
 			line = strtoul(optarg, NULL, 10);
 			break;
+		case 'b':
+			attr = config.num_attrs;
+			config.num_attrs++;
+			gpiotools_set_bit(config.attrs[attr].mask, 0);
+			config.attrs[attr].attr.id = GPIOLINE_ATTR_ID_DEBOUNCE;
+			config.attrs[attr].attr.debounce_period =
+				strtoul(optarg, NULL, 10);
+			break;
 		case 'd':
 			config.flags |= GPIOLINE_FLAG_V2_OPEN_DRAIN;
 			break;
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 18/18] tools: gpio: add multi-line monitoring to gpio-event-mon
  2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
                   ` (16 preceding siblings ...)
  2020-07-25  4:19 ` [PATCH v2 17/18] tools: gpio: add debounce support to gpio-event-mon Kent Gibson
@ 2020-07-25  4:19 ` Kent Gibson
  17 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-25  4:19 UTC (permalink / raw)
  To: linux-kernel, linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson

Extend gpio-event-mon to support monitoring multiple lines.
This would require multiple lineevent requests to implement using uAPI v1,
but can be performed with a single line request using uAPI v2.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/gpio-event-mon.c | 41 ++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/tools/gpio/gpio-event-mon.c b/tools/gpio/gpio-event-mon.c
index b64427d78942..e22cbf631e49 100644
--- a/tools/gpio/gpio-event-mon.c
+++ b/tools/gpio/gpio-event-mon.c
@@ -26,7 +26,8 @@
 #include "gpio-utils.h"
 
 int monitor_device(const char *device_name,
-		   unsigned int line,
+		   unsigned int *lines,
+		   unsigned int num_lines,
 		   struct gpioline_config *config,
 		   unsigned int loops)
 {
@@ -47,7 +48,7 @@ int monitor_device(const char *device_name,
 		goto exit_free_name;
 	}
 
-	ret = gpiotools_request_line(device_name, &line, 1, config,
+	ret = gpiotools_request_line(device_name, lines, num_lines, config,
 				     "gpio-event-mon");
 	if (ret < 0)
 		goto exit_device_close;
@@ -63,9 +64,23 @@ int monitor_device(const char *device_name,
 		goto exit_line_close;
 	}
 
-	fprintf(stdout, "Monitoring line %d on %s\n", line, device_name);
-	fprintf(stdout, "Initial line value: %d\n",
-		gpiotools_test_bit(values.bits, 0));
+	if (num_lines == 1) {
+		fprintf(stdout, "Monitoring line %d on %s\n", lines[0], device_name);
+		fprintf(stdout, "Initial line value: %d\n",
+			gpiotools_test_bit(values.bits, 0));
+	} else {
+		fprintf(stdout, "Monitoring lines %d", lines[0]);
+		for (i = 1; i < num_lines - 1; i++)
+			fprintf(stdout, ", %d", lines[i]);
+		fprintf(stdout, " and %d on %s\n", lines[i], device_name);
+		fprintf(stdout, "Initial line values: %d",
+			gpiotools_test_bit(values.bits, 0));
+		for (i = 1; i < num_lines - 1; i++)
+			fprintf(stdout, ", %d",
+				gpiotools_test_bit(values.bits, i));
+		fprintf(stdout, " and %d\n",
+			gpiotools_test_bit(values.bits, i));
+	}
 
 	while (1) {
 		struct gpioline_event event;
@@ -124,7 +139,7 @@ void print_usage(void)
 	fprintf(stderr, "Usage: gpio-event-mon [options]...\n"
 		"Listen to events on GPIO lines, 0->1 1->0\n"
 		"  -n <name>  Listen on GPIOs on a named device (must be stated)\n"
-		"  -o <n>     Offset to monitor\n"
+		"  -o <n>     Offset of line to monitor (may be repeated)\n"
 		"  -d         Set line as open drain\n"
 		"  -s         Set line as open source\n"
 		"  -r         Listen for rising edges\n"
@@ -145,7 +160,8 @@ void print_usage(void)
 int main(int argc, char **argv)
 {
 	const char *device_name = NULL;
-	unsigned int line = -1;
+	unsigned int lines[GPIOLINES_MAX];
+	unsigned int num_lines = 0;
 	unsigned int loops = 0;
 	struct gpioline_config config;
 	int c, attr;
@@ -161,7 +177,12 @@ int main(int argc, char **argv)
 			device_name = optarg;
 			break;
 		case 'o':
-			line = strtoul(optarg, NULL, 10);
+			if (num_lines >= GPIOLINES_MAX) {
+				print_usage();
+				return -1;
+			}
+			lines[num_lines] = strtoul(optarg, NULL, 10);
+			num_lines++;
 			break;
 		case 'b':
 			attr = config.num_attrs;
@@ -189,7 +210,7 @@ int main(int argc, char **argv)
 		}
 	}
 
-	if (!device_name || line == -1) {
+	if (!device_name || num_lines == 0) {
 		print_usage();
 		return -1;
 	}
@@ -198,5 +219,5 @@ int main(int argc, char **argv)
 		       "falling edges\n");
 		config.flags |= EDGE_FLAGS;
 	}
-	return monitor_device(device_name, line, &config, loops);
+	return monitor_device(device_name, lines, num_lines, &config, loops);
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 03/18] gpiolib: make cdev a build option
  2020-07-25  4:19 ` [PATCH v2 03/18] gpiolib: make cdev a build option Kent Gibson
@ 2020-07-25 20:29   ` Andy Shevchenko
  2020-07-26 22:25   ` Linus Walleij
  1 sibling, 0 replies; 46+ messages in thread
From: Andy Shevchenko @ 2020-07-25 20:29 UTC (permalink / raw)
  To: Kent Gibson
  Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Linus Walleij

On Sat, Jul 25, 2020 at 7:21 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Make the gpiolib-cdev module a build option.  This allows the CDEV
> interface to be removed from the kernel to reduce kernel size in
> applications where is it not required, and provides the parent for
> other other CDEV interface specific build options to follow.
>
> Suggested-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/Kconfig        | 16 ++++++++++++++--
>  drivers/gpio/Makefile       |  2 +-
>  drivers/gpio/gpiolib-cdev.h | 15 +++++++++++++++
>  3 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 8030fd91a3cc..b5bb9efc1092 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -66,8 +66,20 @@ config GPIO_SYSFS
>
>           This ABI is deprecated. If you want to use GPIO from userspace,
>           use the character device /dev/gpiochipN with the appropriate
> -         ioctl() operations instead. The character device is always
> -         available.
> +         ioctl() operations instead.
> +
> +config GPIO_CDEV
> +       bool "/dev/gpiochipN (character device interface)"
> +       default y
> +       help
> +         Say Y here to add the character device /dev/gpiochipN interface
> +         for GPIOs. The character device allows userspace to control GPIOs
> +         using ioctl() operations.
> +
> +         Only say N is you are sure that the GPIO character device is not

is -> if

> +         required.
> +
> +         If unsure, say Y.
>
>  config GPIO_GENERIC
>         depends on HAS_IOMEM # Only for IOMEM drivers
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 4f9abff4f2dc..7c24c8d77068 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -7,8 +7,8 @@ obj-$(CONFIG_GPIOLIB)           += gpiolib.o
>  obj-$(CONFIG_GPIOLIB)          += gpiolib-devres.o
>  obj-$(CONFIG_GPIOLIB)          += gpiolib-legacy.o
>  obj-$(CONFIG_GPIOLIB)          += gpiolib-devprop.o
> -obj-$(CONFIG_GPIOLIB)          += gpiolib-cdev.o
>  obj-$(CONFIG_OF_GPIO)          += gpiolib-of.o
> +obj-$(CONFIG_GPIO_CDEV)                += gpiolib-cdev.o
>  obj-$(CONFIG_GPIO_SYSFS)       += gpiolib-sysfs.o
>  obj-$(CONFIG_GPIO_ACPI)                += gpiolib-acpi.o
>
> diff --git a/drivers/gpio/gpiolib-cdev.h b/drivers/gpio/gpiolib-cdev.h
> index 973578e7ad10..19a4e3d57120 100644
> --- a/drivers/gpio/gpiolib-cdev.h
> +++ b/drivers/gpio/gpiolib-cdev.h
> @@ -5,7 +5,22 @@
>
>  #include <linux/device.h>
>
> +#ifdef CONFIG_GPIO_CDEV
> +
>  int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt);
>  void gpiolib_cdev_unregister(struct gpio_device *gdev);
>
> +#else
> +
> +static inline int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
> +{
> +       return 0;
> +}
> +
> +static inline void gpiolib_cdev_unregister(struct gpio_device *gdev)
> +{
> +}
> +
> +#endif /* CONFIG_GPIO_CDEV */
> +
>  #endif /* GPIOLIB_CDEV_H */
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-25  4:19 ` [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL Kent Gibson
@ 2020-07-25 20:51   ` Andy Shevchenko
  2020-07-26  1:12     ` Kent Gibson
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2020-07-25 20:51 UTC (permalink / raw)
  To: Kent Gibson
  Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Linus Walleij

On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and
> returning their current values using GPIOLINE_GET_VALUES_IOCTL.

...

> +struct line {
> +       struct gpio_device *gdev;
> +       const char *label;
> +       u32 num_descs;

> +       /* descs must be last so it can be dynamically sized */

I guess [] implies above comment and thus comment can be dropped.

> +       struct gpio_desc *descs[];
> +};

...

> +static bool padding_not_zeroed(__u32 *padding, int pad_size)
> +{
> +       int i, sum = 0;
> +
> +       for (i = 0; i < pad_size; i++)
> +               sum |= padding[i];
> +
> +       return sum;
> +}

Reimplementation of memchr_inv() ?

...

> +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
> +{
> +       int i;
> +
> +       for (i = lc->num_attrs - 1; i >= 0; i--) {

Much better to read is

unsigned int i = lc->num_attrs;

while (i--) {
 ...
}

> +               if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) &&

> +                   test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))

This casting is not good. What about BE 32-bit architecture?

> +                       return lc->attrs[i].attr.flags;
> +       }
> +       return lc->flags;
> +}
> +
> +static int gpioline_config_output_value(struct gpioline_config *lc,
> +                                       int line_idx)
> +{

Same comments as per above.

> +}

...

> +static long line_get_values(struct line *line, void __user *ip)
> +{
> +       struct gpioline_values lv;

> +       unsigned long *vals = (unsigned long *)lv.bits;

Casting u64 to unsigned long is not good.

> +}

...

> +static void line_free(struct line *line)
> +{
> +       int i;
> +
> +       for (i = 0; i < line->num_descs; i++) {

> +               if (line->descs[i])

Redundant?

> +                       gpiod_free(line->descs[i]);
> +       }
> +       kfree(line->label);
> +       put_device(&line->gdev->dev);
> +       kfree(line);
> +}

...

> +       /* Make sure this is terminated */
> +       linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
> +       if (strlen(linereq.consumer)) {
> +               line->label = kstrdup(linereq.consumer, GFP_KERNEL);

kstrndup() ?

> +               if (!line->label) {
> +                       ret = -ENOMEM;
> +                       goto out_free_line;
> +               }
> +       }

...

> +               struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);

I prefer to see this split, but it's minor.

> +               if (IS_ERR(desc)) {
> +                       ret = PTR_ERR(desc);
> +                       goto out_free_line;
> +               }

...

> +               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> +                       offset);

Perhaps tracepoint / event?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-25 20:51   ` Andy Shevchenko
@ 2020-07-26  1:12     ` Kent Gibson
  2020-07-26  3:24       ` Kent Gibson
                         ` (2 more replies)
  0 siblings, 3 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-26  1:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Linus Walleij

On Sat, Jul 25, 2020 at 11:51:54PM +0300, Andy Shevchenko wrote:
> On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and
> > returning their current values using GPIOLINE_GET_VALUES_IOCTL.
> 
> ...
> 
> > +struct line {
> > +       struct gpio_device *gdev;
> > +       const char *label;
> > +       u32 num_descs;
> 
> > +       /* descs must be last so it can be dynamically sized */
> 
> I guess [] implies above comment and thus comment can be dropped.
> 
> > +       struct gpio_desc *descs[];
> > +};
> 
> ...
> 
> > +static bool padding_not_zeroed(__u32 *padding, int pad_size)
> > +{
> > +       int i, sum = 0;
> > +
> > +       for (i = 0; i < pad_size; i++)
> > +               sum |= padding[i];
> > +
> > +       return sum;
> > +}
> 
> Reimplementation of memchr_inv() ?
> 

I was hoping to find an existing function, surely checking a region is
zeroed is a common thing, right?, so this was a place holder as much
as anything.  Not sure memchr_inv fits the bill, but I'll give it a
try...

> ...
> 
> > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
> > +{
> > +       int i;
> > +
> > +       for (i = lc->num_attrs - 1; i >= 0; i--) {
> 
> Much better to read is
> 
> unsigned int i = lc->num_attrs;
> 
> while (i--) {
>  ...
> }
> 

Really? I find that the post-decrement in the while makes determining the
bounds of the loop more confusing.

> > +               if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) &&
> 
> > +                   test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))
> 
> This casting is not good. What about BE 32-bit architecture?
> 

I agree the casting is hideous, but I thought the outcome was correct
as it is manipulating addresses, not data.
You think the address of a 64-bit differs based on endian??
Happy to change it - but not sure what to.

> > +                       return lc->attrs[i].attr.flags;
> > +       }
> > +       return lc->flags;
> > +}
> > +
> > +static int gpioline_config_output_value(struct gpioline_config *lc,
> > +                                       int line_idx)
> > +{
> 
> Same comments as per above.
> 
> > +}
> 
> ...
> 
> > +static long line_get_values(struct line *line, void __user *ip)
> > +{
> > +       struct gpioline_values lv;
> 
> > +       unsigned long *vals = (unsigned long *)lv.bits;
> 
> Casting u64 to unsigned long is not good.
> 

Same comments as per above.

> > +}
> 
> ...
> 
> > +static void line_free(struct line *line)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < line->num_descs; i++) {
> 
> > +               if (line->descs[i])
> 
> Redundant?
> 

Actually, no.  The line_free is also used to clean up construction
failures, so the line may be partially constructed.  num_descs is set
first, but the descs themselves may have failed to allocate.
And gpiod_free throws a warning if you pass a NULL, hence the extra check here.

> > +                       gpiod_free(line->descs[i]);
> > +       }
> > +       kfree(line->label);
> > +       put_device(&line->gdev->dev);
> > +       kfree(line);
> > +}
> 
> ...
> 
> > +       /* Make sure this is terminated */
> > +       linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
> > +       if (strlen(linereq.consumer)) {
> > +               line->label = kstrdup(linereq.consumer, GFP_KERNEL);
> 
> kstrndup() ?
> 

That was a cut-and-paste from V1...

> > +               if (!line->label) {
> > +                       ret = -ENOMEM;
> > +                       goto out_free_line;
> > +               }
> > +       }
> 

... and changing it would result in this logic behaving differently.
You couldn't distinguish between consumer not being set, and
so label not being set, and kstrndup returning NULL due to no mem.

> ...
> 
> > +               struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
> 
> I prefer to see this split, but it's minor.
> 
> > +               if (IS_ERR(desc)) {
> > +                       ret = PTR_ERR(desc);
> > +                       goto out_free_line;
> > +               }
> 
> ...
> 
> > +               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> > +                       offset);
> 
> Perhaps tracepoint / event?
> 

Again, a cut-and-paste from V1, and I have no experience with
tracepoints or events, so I have no opinion on that.

So, yeah - perhaps?

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-26  1:12     ` Kent Gibson
@ 2020-07-26  3:24       ` Kent Gibson
  2020-07-29  2:28       ` Kent Gibson
  2020-07-31 16:05       ` Bartosz Golaszewski
  2 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-26  3:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Linus Walleij

On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote:
> On Sat, Jul 25, 2020 at 11:51:54PM +0300, Andy Shevchenko wrote:
> > On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >

[ snip ]
> > 
> > > +static bool padding_not_zeroed(__u32 *padding, int pad_size)
> > > +{
> > > +       int i, sum = 0;
> > > +
> > > +       for (i = 0; i < pad_size; i++)
> > > +               sum |= padding[i];
> > > +
> > > +       return sum;
> > > +}
> > 
> > Reimplementation of memchr_inv() ?
> > 
> 
> I was hoping to find an existing function, surely checking a region is
> zeroed is a common thing, right?, so this was a place holder as much
> as anything.  Not sure memchr_inv fits the bill, but I'll give it a
> try...
> 

I gave it a try.  It is a good fit functionally, but in my build it
results in a larger object by ~104 bytes.  I assume that is because 
padding_not_zeroed is being inlined, and otherwise optimized, while
memchr_inv calls aren't.

As such I'm inclined to leave it as is - unless there are other
objections.

Cheers,
Kent.


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 03/18] gpiolib: make cdev a build option
  2020-07-25  4:19 ` [PATCH v2 03/18] gpiolib: make cdev a build option Kent Gibson
  2020-07-25 20:29   ` Andy Shevchenko
@ 2020-07-26 22:25   ` Linus Walleij
  2020-07-27  1:46     ` Kent Gibson
  1 sibling, 1 reply; 46+ messages in thread
From: Linus Walleij @ 2020-07-26 22:25 UTC (permalink / raw)
  To: Kent Gibson; +Cc: linux-kernel, open list:GPIO SUBSYSTEM, Bartosz Golaszewski

On Sat, Jul 25, 2020 at 6:21 AM Kent Gibson <warthog618@gmail.com> wrote:

> +config GPIO_CDEV
> +       bool "/dev/gpiochipN (character device interface)"
> +       default y

I don't want to make it too easy to do this, as I see it as a standard
kernel feature.

Can we add:

depends on EXPERT

as with other standard kernel features?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 03/18] gpiolib: make cdev a build option
  2020-07-26 22:25   ` Linus Walleij
@ 2020-07-27  1:46     ` Kent Gibson
  2020-07-27  5:57       ` Kent Gibson
  0 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-07-27  1:46 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-kernel, open list:GPIO SUBSYSTEM, Bartosz Golaszewski

On Mon, Jul 27, 2020 at 12:25:53AM +0200, Linus Walleij wrote:
> On Sat, Jul 25, 2020 at 6:21 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> > +config GPIO_CDEV
> > +       bool "/dev/gpiochipN (character device interface)"
> > +       default y
> 
> I don't want to make it too easy to do this, as I see it as a standard
> kernel feature.
> 
> Can we add:
> 
> depends on EXPERT
> 
> as with other standard kernel features?
> 

Fair enough.

But what of the GPIO_CDEV_V1 option to disable uAPI V1 added in patch 04,
and that depends on GPIO_CDEV?
That is equivalent to GPIO_SYSFS, which is not dependent on EXPERT,
so I'll need to restructure the dependencies so it doesn't
inherit the EXPERT dependency.
Unless you also want it to be dependent on EXPERT.

Hmmm, and maybe patch 04 should be later in the series - after V2 is
fully implemented and V1 is deprecated - around patch 11.

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 03/18] gpiolib: make cdev a build option
  2020-07-27  1:46     ` Kent Gibson
@ 2020-07-27  5:57       ` Kent Gibson
  2020-07-27  8:15         ` Linus Walleij
  0 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-07-27  5:57 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-kernel, open list:GPIO SUBSYSTEM, Bartosz Golaszewski

On Mon, Jul 27, 2020 at 09:46:01AM +0800, Kent Gibson wrote:
> On Mon, Jul 27, 2020 at 12:25:53AM +0200, Linus Walleij wrote:
> > On Sat, Jul 25, 2020 at 6:21 AM Kent Gibson <warthog618@gmail.com> wrote:
> > 
> > > +config GPIO_CDEV
> > > +       bool "/dev/gpiochipN (character device interface)"
> > > +       default y
> > 
> > I don't want to make it too easy to do this, as I see it as a standard
> > kernel feature.
> > 
> > Can we add:
> > 
> > depends on EXPERT
> > 
> > as with other standard kernel features?
> > 
> 
> Fair enough.
> 
> But what of the GPIO_CDEV_V1 option to disable uAPI V1 added in patch 04,
> and that depends on GPIO_CDEV?
> That is equivalent to GPIO_SYSFS, which is not dependent on EXPERT,
> so I'll need to restructure the dependencies so it doesn't
> inherit the EXPERT dependency.
> Unless you also want it to be dependent on EXPERT.
> 

I've gone with this:

+config GPIO_CDEV
+       bool
+       prompt "Character device (/dev/gpiochipN) support" if EXPERT
+       default y

so the entry is always present in menuconfig, and GPIO_CDEV_V1 can still
depend on it, but GPIO_CDEV can only be disabled if EXPERT is set.

> Hmmm, and maybe patch 04 should be later in the series - after V2 is
> fully implemented and V1 is deprecated - around patch 11.
> 

Just ignore me - the earlier code patches need the define else the V1
will be compiled out.

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 03/18] gpiolib: make cdev a build option
  2020-07-27  5:57       ` Kent Gibson
@ 2020-07-27  8:15         ` Linus Walleij
  0 siblings, 0 replies; 46+ messages in thread
From: Linus Walleij @ 2020-07-27  8:15 UTC (permalink / raw)
  To: Kent Gibson; +Cc: linux-kernel, open list:GPIO SUBSYSTEM, Bartosz Golaszewski

On Mon, Jul 27, 2020 at 7:57 AM Kent Gibson <warthog618@gmail.com> wrote:

> I've gone with this:
>
> +config GPIO_CDEV
> +       bool
> +       prompt "Character device (/dev/gpiochipN) support" if EXPERT
> +       default y
>
> so the entry is always present in menuconfig, and GPIO_CDEV_V1 can still
> depend on it, but GPIO_CDEV can only be disabled if EXPERT is set.

This is perfect, thanks Kent!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-26  1:12     ` Kent Gibson
  2020-07-26  3:24       ` Kent Gibson
@ 2020-07-29  2:28       ` Kent Gibson
  2020-07-29  8:05         ` Andy Shevchenko
  2020-07-31 16:05       ` Bartosz Golaszewski
  2 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-07-29  2:28 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Linus Walleij

On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote:
> On Sat, Jul 25, 2020 at 11:51:54PM +0300, Andy Shevchenko wrote:
> > On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >

[ snip ]

> > > +                   test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))
> > 
> > This casting is not good. What about BE 32-bit architecture?
> > 
> 
> I agree the casting is hideous, but I thought the outcome was correct
> as it is manipulating addresses, not data.
> You think the address of a 64-bit differs based on endian??
> Happy to change it - but not sure what to.
> 

You are right - using bitops on u64 is problematic for BE-32 - the 32-bit
words will be swapped if userspace treats the flags as the u64 it is
defined as.

I'll rework that for v3.

Cheers,
Kent.


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-29  2:28       ` Kent Gibson
@ 2020-07-29  8:05         ` Andy Shevchenko
  2020-07-29 10:01           ` Kent Gibson
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2020-07-29  8:05 UTC (permalink / raw)
  To: Kent Gibson
  Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Linus Walleij

On Wed, Jul 29, 2020 at 5:28 AM Kent Gibson <warthog618@gmail.com> wrote:
> On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote:

...

> I'll rework that for v3.

Please give some more time to review v2. Especially the v2 API approach.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-29  8:05         ` Andy Shevchenko
@ 2020-07-29 10:01           ` Kent Gibson
  0 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-07-29 10:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
	Bartosz Golaszewski, Linus Walleij

On Wed, Jul 29, 2020 at 11:05:48AM +0300, Andy Shevchenko wrote:
> On Wed, Jul 29, 2020 at 5:28 AM Kent Gibson <warthog618@gmail.com> wrote:
> > On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote:
> 
> ...
> 
> > I'll rework that for v3.
> 
> Please give some more time to review v2. Especially the v2 API approach.
> 

For sure.  I'll be spending some time setting up and testing on a BE 32
target so I wont be ready to submit a v3 for a few days anyway.

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-26  1:12     ` Kent Gibson
  2020-07-26  3:24       ` Kent Gibson
  2020-07-29  2:28       ` Kent Gibson
@ 2020-07-31 16:05       ` Bartosz Golaszewski
  2020-08-02  3:31         ` Kent Gibson
  2 siblings, 1 reply; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-07-31 16:05 UTC (permalink / raw)
  To: Kent Gibson
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote:
>

[snip]

> >
> > > +static bool padding_not_zeroed(__u32 *padding, int pad_size)
> > > +{
> > > +       int i, sum = 0;
> > > +
> > > +       for (i = 0; i < pad_size; i++)
> > > +               sum |= padding[i];
> > > +
> > > +       return sum;
> > > +}
> >
> > Reimplementation of memchr_inv() ?
> >
>
> I was hoping to find an existing function, surely checking a region is
> zeroed is a common thing, right?, so this was a place holder as much
> as anything.  Not sure memchr_inv fits the bill, but I'll give it a
> try...
>

If you don't find an appropriate function: please put your new
implementation in lib/ so that others may reuse it.

> > ...
> >
> > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = lc->num_attrs - 1; i >= 0; i--) {
> >
> > Much better to read is
> >
> > unsigned int i = lc->num_attrs;
> >
> > while (i--) {
> >  ...
> > }
> >
>
> Really? I find that the post-decrement in the while makes determining the
> bounds of the loop more confusing.
>

Agreed, Andy: this is too much nit-picking. :)

[snip]

> > ...
> >
> > > +               struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
> >
> > I prefer to see this split, but it's minor.
> >
> > > +               if (IS_ERR(desc)) {
> > > +                       ret = PTR_ERR(desc);
> > > +                       goto out_free_line;
> > > +               }
> >
> > ...
> >
> > > +               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> > > +                       offset);
> >
> > Perhaps tracepoint / event?
> >
>
> Again, a cut-and-paste from V1, and I have no experience with
> tracepoints or events, so I have no opinion on that.
>
> So, yeah - perhaps?
>

I think it's a good idea to add some proper instrumentation this time
other than much less reliable logs. Can you take a look at
include/trace/events/gpio.h? Adding new GPIO trace events should be
pretty straightforward by copy-pasti... drawing inspiration from
existing ones.

Bart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-07-31 16:05       ` Bartosz Golaszewski
@ 2020-08-02  3:31         ` Kent Gibson
  2020-08-02  9:32           ` Kent Gibson
  2020-08-03 20:02           ` Bartosz Golaszewski
  0 siblings, 2 replies; 46+ messages in thread
From: Kent Gibson @ 2020-08-02  3:31 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote:
> On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> 
> [snip]
> 
> > >
> > > > +static bool padding_not_zeroed(__u32 *padding, int pad_size)
> > > > +{
> > > > +       int i, sum = 0;
> > > > +
> > > > +       for (i = 0; i < pad_size; i++)
> > > > +               sum |= padding[i];
> > > > +
> > > > +       return sum;
> > > > +}
> > >
> > > Reimplementation of memchr_inv() ?
> > >
> >
> > I was hoping to find an existing function, surely checking a region is
> > zeroed is a common thing, right?, so this was a place holder as much
> > as anything.  Not sure memchr_inv fits the bill, but I'll give it a
> > try...
> >
> 
> If you don't find an appropriate function: please put your new
> implementation in lib/ so that others may reuse it.
> 

Changed to memchr_inv.

> > > ...
> > >
> > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = lc->num_attrs - 1; i >= 0; i--) {
> > >
> > > Much better to read is
> > >
> > > unsigned int i = lc->num_attrs;
> > >
> > > while (i--) {
> > >  ...
> > > }
> > >
> >
> > Really? I find that the post-decrement in the while makes determining the
> > bounds of the loop more confusing.
> >
> 
> Agreed, Andy: this is too much nit-picking. :)
> 

I was actually hoping for some feedback on the direction of that loop,
as it relates to the handling of multiple instances of the same
attribute associated with a given line.

The reverse loop here implements a last in wins policy, but I'm now
thinking the kernel should be encouraging userspace to only associate a
given attribute with a line once, and that a first in wins would help do
that - as additional associations would be ignored.

Alternatively, the kernel should enforce that an attribute can only be
associated once, but that would require adding more request validation.

> [snip]
> 
> > > ...
> > >
> > > > +               struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
> > >
> > > I prefer to see this split, but it's minor.
> > >
> > > > +               if (IS_ERR(desc)) {
> > > > +                       ret = PTR_ERR(desc);
> > > > +                       goto out_free_line;
> > > > +               }
> > >
> > > ...
> > >
> > > > +               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> > > > +                       offset);
> > >
> > > Perhaps tracepoint / event?
> > >
> >
> > Again, a cut-and-paste from V1, and I have no experience with
> > tracepoints or events, so I have no opinion on that.
> >
> > So, yeah - perhaps?
> >
> 
> I think it's a good idea to add some proper instrumentation this time
> other than much less reliable logs. Can you take a look at
> include/trace/events/gpio.h? Adding new GPIO trace events should be
> pretty straightforward by copy-pasti... drawing inspiration from
> existing ones.
> 

You only want tracepoints to replace those dev_dbg()s, so when a line
is requested? What about the release?  Any other points?

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-08-02  3:31         ` Kent Gibson
@ 2020-08-02  9:32           ` Kent Gibson
  2020-08-03 19:59             ` Bartosz Golaszewski
  2020-08-03 20:02           ` Bartosz Golaszewski
  1 sibling, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-08-02  9:32 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Sun, Aug 02, 2020 at 11:31:58AM +0800, Kent Gibson wrote:
> On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote:
> > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > 
> > > >
> > > > > +               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> > > > > +                       offset);
> > > >
> > > > Perhaps tracepoint / event?
> > > >
> > >
> > > Again, a cut-and-paste from V1, and I have no experience with
> > > tracepoints or events, so I have no opinion on that.
> > >
> > > So, yeah - perhaps?
> > >
> > 
> > I think it's a good idea to add some proper instrumentation this time
> > other than much less reliable logs. Can you take a look at
> > include/trace/events/gpio.h? Adding new GPIO trace events should be
> > pretty straightforward by copy-pasti... drawing inspiration from
> > existing ones.
> > 
> 
> You only want tracepoints to replace those dev_dbg()s, so when a line
> is requested? What about the release?  Any other points?
> 

Had a closer look and it seems to me that the correct place to add such
tracepoints would be gpiod_request() and gpiod_free(), so they catch all
requests, not just the cdev ones.  And that moves it outside the scope
of this patch.

I personally don't have any use for the dev_dbg()s here and am happy to
remove them - they were only there to match the behaviour of
linehandle_create as closely as possible.

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-08-02  9:32           ` Kent Gibson
@ 2020-08-03 19:59             ` Bartosz Golaszewski
  0 siblings, 0 replies; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-03 19:59 UTC (permalink / raw)
  To: Kent Gibson
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Sun, Aug 2, 2020 at 11:32 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sun, Aug 02, 2020 at 11:31:58AM +0800, Kent Gibson wrote:
> > On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote:
> > > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > >
> > > > >
> > > > > > +               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> > > > > > +                       offset);
> > > > >
> > > > > Perhaps tracepoint / event?
> > > > >
> > > >
> > > > Again, a cut-and-paste from V1, and I have no experience with
> > > > tracepoints or events, so I have no opinion on that.
> > > >
> > > > So, yeah - perhaps?
> > > >
> > >
> > > I think it's a good idea to add some proper instrumentation this time
> > > other than much less reliable logs. Can you take a look at
> > > include/trace/events/gpio.h? Adding new GPIO trace events should be
> > > pretty straightforward by copy-pasti... drawing inspiration from
> > > existing ones.
> > >
> >
> > You only want tracepoints to replace those dev_dbg()s, so when a line
> > is requested? What about the release?  Any other points?
> >
>
> Had a closer look and it seems to me that the correct place to add such
> tracepoints would be gpiod_request() and gpiod_free(), so they catch all
> requests, not just the cdev ones.  And that moves it outside the scope
> of this patch.
>
> I personally don't have any use for the dev_dbg()s here and am happy to
> remove them - they were only there to match the behaviour of
> linehandle_create as closely as possible.
>

Sounds good, we can work on trace points separately.

Bartosz

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-08-02  3:31         ` Kent Gibson
  2020-08-02  9:32           ` Kent Gibson
@ 2020-08-03 20:02           ` Bartosz Golaszewski
  2020-08-03 23:01             ` Kent Gibson
  1 sibling, 1 reply; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-03 20:02 UTC (permalink / raw)
  To: Kent Gibson
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Sun, Aug 2, 2020 at 5:32 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote:
> > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote:

[snip!]

> > > >
> > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
> > > > > +{
> > > > > +       int i;
> > > > > +
> > > > > +       for (i = lc->num_attrs - 1; i >= 0; i--) {
> > > >
> > > > Much better to read is
> > > >
> > > > unsigned int i = lc->num_attrs;
> > > >
> > > > while (i--) {
> > > >  ...
> > > > }
> > > >
> > >
> > > Really? I find that the post-decrement in the while makes determining the
> > > bounds of the loop more confusing.
> > >
> >
> > Agreed, Andy: this is too much nit-picking. :)
> >
>
> I was actually hoping for some feedback on the direction of that loop,
> as it relates to the handling of multiple instances of the same
> attribute associated with a given line.
>
> The reverse loop here implements a last in wins policy, but I'm now
> thinking the kernel should be encouraging userspace to only associate a
> given attribute with a line once, and that a first in wins would help do
> that - as additional associations would be ignored.
>
> Alternatively, the kernel should enforce that an attribute can only be
> associated once, but that would require adding more request validation.
>

I guess this would result in a lot of churn to do validation which is
largely unnecessary? To me the first in wins sounds more consistent.

Also: I just started going through the patches - nice idea with the
GPIO attributes, I really like it. Although I need to give it a longer
thought tomorrow - I'm wondering if we can maybe unify them and the
flags.

[snip]

Bartosz

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-08-03 20:02           ` Bartosz Golaszewski
@ 2020-08-03 23:01             ` Kent Gibson
  2020-08-04 17:47               ` Bartosz Golaszewski
  0 siblings, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-08-03 23:01 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Mon, Aug 03, 2020 at 10:02:50PM +0200, Bartosz Golaszewski wrote:
> On Sun, Aug 2, 2020 at 5:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote:
> > > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> [snip!]
> 
> > > > >
> > > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
> > > > > > +{
> > > > > > +       int i;
> > > > > > +
> > > > > > +       for (i = lc->num_attrs - 1; i >= 0; i--) {
> > > > >
> > > > > Much better to read is
> > > > >
> > > > > unsigned int i = lc->num_attrs;
> > > > >
> > > > > while (i--) {
> > > > >  ...
> > > > > }
> > > > >
> > > >
> > > > Really? I find that the post-decrement in the while makes determining the
> > > > bounds of the loop more confusing.
> > > >
> > >
> > > Agreed, Andy: this is too much nit-picking. :)
> > >
> >
> > I was actually hoping for some feedback on the direction of that loop,
> > as it relates to the handling of multiple instances of the same
> > attribute associated with a given line.
> >
> > The reverse loop here implements a last in wins policy, but I'm now
> > thinking the kernel should be encouraging userspace to only associate a
> > given attribute with a line once, and that a first in wins would help do
> > that - as additional associations would be ignored.
> >
> > Alternatively, the kernel should enforce that an attribute can only be
> > associated once, but that would require adding more request validation.
> >
> 
> I guess this would result in a lot of churn to do validation which is
> largely unnecessary? To me the first in wins sounds more consistent.
> 

Fully validating the attrs would involve a lot of tedious looping, which
would be pointless 99.99% of the time, so I was hoping to avoid it.
OTOH we're interacting with hardware so I don't want to be doing
anything that userspace hasn't explicitly requested.

But I would be satisfied with clearly documenting the behaviour - and
in most cases libgpiod will be taking care of it anyway...

> Also: I just started going through the patches - nice idea with the
> GPIO attributes, I really like it. Although I need to give it a longer
> thought tomorrow - I'm wondering if we can maybe unify them and the
> flags.
> 

I had an earlier draft that did just that - and that is partially why
the loop is last in wins - I was using slot 0 as the default flags.
But the default flags cover a lot of use cases, including all of v1, and
it was simple and cheap to provide a default - and it simplified the
initial port of libgpiod to v2...

Cheers,
Kent.


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 02/18] gpio: uapi: define uAPI v2
  2020-07-25  4:19 ` [PATCH v2 02/18] gpio: uapi: define uAPI v2 Kent Gibson
@ 2020-08-04 17:42   ` Bartosz Golaszewski
  2020-08-05  5:18     ` Kent Gibson
  0 siblings, 1 reply; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-04 17:42 UTC (permalink / raw)
  To: Kent Gibson; +Cc: LKML, linux-gpio, Linus Walleij

On Sat, Jul 25, 2020 at 6:20 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Add a new version of the uAPI to address existing 32/64-bit 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 32-bit and 64-bit platforms. That creates problems for 32-bit apps
> running on 64-bit kernels.  The patch addresses that particular issue, and
> the problem more generally, by adding pad fields that explicitly pad
> structs out to 64-bit 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>
> ---

Hi Kent,

Thanks a lot for your work on this. Please see comments below.

One thing I'd change globally for better readability is to have all
new symbols marked as v2 - even if they have no counterparts in v1. I
know libgpiod will wrap it all anyway but I think it's still a good
way to make our work in user-space easier.

>
> 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.
>
> I've also merged the handle and event requests into a single request, the
> line request, as the two requests were 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.
>

I think the info above is worth putting into the commit message.
Especially the part about merging the two event types.

> Changes since v1:
>  - lower case V1 and V2, except in capitalized names
>  - hyphenate 32/64-bit
>  - rename bitmap field to bits
>  - drop PAD_SIZE consts in favour of hard coded numbers
>  - sort includes
>  - change config flags to __u64
>  - increase padding of gpioline_event
>  - relocate GPIOLINE_CHANGED enum into v2 section (is common with v1)
>  - rework config to collapse direction, drive, bias and edge enums back
>    into flags and add optional attributes that can be associated with a
>    subset of the requested lines.
>
> 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
>  - rename debounce to debounce_period
>  - rename lines to num_lines
>
>  include/uapi/linux/gpio.h | 284 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 270 insertions(+), 14 deletions(-)
>
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 285cc10355b2..3f6db33014f0 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -12,10 +12,13 @@
>  #define _UAPI_GPIO_H_
>
>  #include <linux/ioctl.h>
> +#include <linux/kernel.h>
>  #include <linux/types.h>
>
>  /*
>   * The maximum size of name and label arrays.
> + *
> + * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
>   */
>  #define GPIO_MAX_NAME_SIZE 32
>
> @@ -32,6 +35,251 @@ struct gpiochip_info {
>         __u32 lines;
>  };
>
> +/*
> + * Maximum number of requested lines.
> + *
> + * Must be a multiple of 8 to ensure 32/64-bit 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)
> +

In what circumstances can this be different than 1? It's worth
documenting here I suppose.

> +/*
> + * The maximum number of configuration attributes associated with a line
> + * request.
> + */
> +#define GPIOLINE_NUM_ATTRS_MAX 10
> +

How did you choose this number? I mean: it's reasonable - just asking
for clarification.

> +/**
> + * enum gpioline_flag_v2 - &struct gpioline_attribute.flags values
> + */
> +enum gpioline_flag_v2 {
> +       GPIOLINE_FLAG_V2_USED                   = 1UL << 0, /* line is not available for request */
> +       GPIOLINE_FLAG_V2_ACTIVE_LOW             = 1UL << 1,
> +       GPIOLINE_FLAG_V2_INPUT                  = 1UL << 2,
> +       GPIOLINE_FLAG_V2_OUTPUT                 = 1UL << 3,
> +       GPIOLINE_FLAG_V2_EDGE_RISING            = 1UL << 4,
> +       GPIOLINE_FLAG_V2_EDGE_FALLING           = 1UL << 5,
> +       GPIOLINE_FLAG_V2_OPEN_DRAIN             = 1UL << 6,
> +       GPIOLINE_FLAG_V2_OPEN_SOURCE            = 1UL << 7,
> +       GPIOLINE_FLAG_V2_BIAS_PULL_UP           = 1UL << 8,
> +       GPIOLINE_FLAG_V2_BIAS_PULL_DOWN         = 1UL << 9,
> +       GPIOLINE_FLAG_V2_BIAS_DISABLED          = 1UL << 10,
> +};
> +
> +/**
> + * struct gpioline_values - Values of GPIO lines
> + * @bits: 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 the line is configured as active
> + * low.
> + */
> +struct gpioline_values {
> +       __u64 bits[GPIOLINES_BITMAP_SIZE];
> +};
> +

We can set values only for a subset of requested lines but AFAICT we
can't read values of only a subset of lines. Would it be difficult to
remove this limitation? While reading values always succeeds - even if
the line is in input mode and has edge detected - I think that someone
may want to request the max number of lines without reading all their
values each time. Maybe consider merging this with struct
gpioline_set_values?

> +/**
> + * struct gpioline_set_values - Values to set a group of GPIO lines
> + * @mask: a bitmap identifying the lines to set.
> + * @bits: 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 the line is configured as active
> + * low.
> + */
> +struct gpioline_set_values {
> +       __u64 mask[GPIOLINES_BITMAP_SIZE];
> +       __u64 bits[GPIOLINES_BITMAP_SIZE];
> +};
> +
> +/**
> + * enum gpioline_attr_id - &struct gpioline_attribute.id values
> + */
> +enum gpioline_attr_id {
> +       GPIOLINE_ATTR_ID_FLAGS                  = 1,
> +       GPIOLINE_ATTR_ID_OUTPUT_VALUES          = 2,
> +       GPIOLINE_ATTR_ID_DEBOUNCE               = 3,
> +};
> +
> +/**
> + * struct gpioline_attribute - a configurable attribute of a line
> + * @id: attribute identifier with value from &enum gpioline_attr_id
> + * @padding: reserved for future use and must be zero filled
> + * @flags: if id is GPIOLINE_ATTR_ID_FLAGS, the flags for the GPIO line,
> + * with values from enum gpioline_flag_v2, such as
> + * GPIOLINE_FLAG_V2_ACTIVE_LOW, GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed
> + * together.  This overrides the default flags contained in the &struct
> + * gpioline_config for the associated line.
> + * @values: if id is GPIOLINE_ATTR_ID_OUTPUT_VALUES, the values to which
> + * the lines will be set
> + * @debounce_period: if id is GPIOLINE_ATTR_ID_DEBOUNCE, the desired
> + * debounce period, in microseconds
> + */
> +struct gpioline_attribute {
> +       __u32 id;
> +       __u32 padding;
> +       union {
> +               __u64 flags;
> +               struct gpioline_values values;
> +               __u32 debounce_period;
> +       };
> +};

I'm afraid that if we don't have enough padding here (at the end),
we'll end up wanting to add a new attribute at some point whose
argument won't fit. Maybe have a specific field in the union that's
even larger than __u64?

> +
> +/**
> + * struct gpioline_config_attribute - a configuration attribute associated
> + * with one or more of the requested lines.
> + * @mask: a bitmap identifying the lines to which the attribute applies
> + * @attr: the configurable attribute
> + */
> +struct gpioline_config_attribute {
> +       __u64 mask[GPIOLINES_BITMAP_SIZE];
> +       struct gpioline_attribute attr;
> +};
> +
> +/**
> + * struct gpioline_config - Configuration for GPIO lines
> + * @flags: flags for the GPIO lines, with values from enum
> + * gpioline_flag_v2, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> + * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together.  This is the default for
> + * all requested lines but may be overridden for particular lines using
> + * attrs.

So I'm having a hard time with this. I understand that the thinking
behind it was: use the flags field to set all lines to INPUT by
default and only set certain lines to OUTPUT with attrs. This would
make life easier for user-space but it complicates the kernel code and
I also believe that any such simplification should be handled by
user-space libraries, not be exposed by kernel uAPI. My personal
preference would be to drop the flags field and only handle attributes
(maybe even define a special macro to set all bits in mask -
GPIOLINE_CONFIG_ALL_LINES or something) on a first-in-wins basis. I'm
open to other suggestions though.

> + * @num_attrs: the number of attributes in attrs
> + * @padding: reserved for future use and must be zero filled
> + * @attrs: the configuration attributes associated with the requested
> + * lines.
> + */
> +struct gpioline_config {
> +       __u64 flags;
> +       __u32 num_attrs;
> +       /*
> +        * Pad to fill implicit padding and provide space for future use.
> +        */
> +       __u32 padding[5];
> +       struct gpioline_config_attribute attrs[GPIOLINE_NUM_ATTRS_MAX];
> +};
> +
> +/**
> + * 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 lines.
> + * @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 in the configuration. 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;
> +       /*
> +        * Pad struct to 64-bit boundary and provide space for future use.
> +        */
> +       __u32 padding[5];
> +       __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
> + * @flags: flags for the GPIO line, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> + * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together
> + * @offset: the local offset on this GPIO device, fill this in when
> + * requesting the line information from the kernel
> + * @num_attrs: the number of attributes in attrs
> + * @attrs: the configuration attributes associated with the line.
> + * @padding: reserved for future use
> + */
> +struct gpioline_info_v2 {
> +       char name[GPIO_MAX_NAME_SIZE];
> +       char consumer[GPIO_MAX_NAME_SIZE];
> +       __u32 offset;
> +       __u32 num_attrs;
> +       __u64 flags;
> +       struct gpioline_attribute attrs[GPIOLINE_NUM_ATTRS_MAX];
> +       /*
> +        * Pad struct to 64-bit boundary and provide space for future use.
> +        */
> +       __u32 padding[4];
> +};
> +
> +enum gpioline_changed_type {
> +       GPIOLINE_CHANGED_REQUESTED      = 1,
> +       GPIOLINE_CHANGED_RELEASED       = 2,
> +       GPIOLINE_CHANGED_CONFIG         = 3,
> +};
> +
> +/**
> + * 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
> + * @event_type: the type of change with a value from enum gpioline_changed_type
> + * @padding: reserved for future use
> + */
> +struct gpioline_info_changed_v2 {
> +       struct gpioline_info_v2 info;
> +       __u64 timestamp;
> +       __u32 event_type;
> +       /*
> +        * Pad struct to 64-bit boundary and provide space for future use.
> +        */
> +       __u32 padding[5];
> +};
> +
> +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;
> +       /*
> +        * Pad struct to 64-bit boundary and provide space for future use.
> +        */
> +       __u32 padding[6];
> +};
> +
> +/*
> + *  ABI v1
> + */
> +
>  /* Informational flags */
>  #define GPIOLINE_FLAG_KERNEL           (1UL << 0) /* Line used by the kernel */
>  #define GPIOLINE_FLAG_IS_OUT           (1UL << 1)
> @@ -64,13 +312,6 @@ struct gpioline_info {
>  /* Maximum number of requested handles */
>  #define GPIOHANDLES_MAX 64
>
> -/* Possible line status change events */
> -enum {
> -       GPIOLINE_CHANGED_REQUESTED = 1,
> -       GPIOLINE_CHANGED_RELEASED,
> -       GPIOLINE_CHANGED_CONFIG,
> -};
> -
>  /**
>   * struct gpioline_info_changed - Information about a change in status
>   * of a GPIO line
> @@ -149,8 +390,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 +400,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 +443,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_set_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
>

Everything else looks good to me. Much cleaner and more elegant than
v1. Great job!

I may come up with something else while browsing other patches I suppose.

Bartosz

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-08-03 23:01             ` Kent Gibson
@ 2020-08-04 17:47               ` Bartosz Golaszewski
  2020-08-05  3:06                 ` Kent Gibson
  0 siblings, 1 reply; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-04 17:47 UTC (permalink / raw)
  To: Kent Gibson
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Tue, Aug 4, 2020 at 1:01 AM Kent Gibson <warthog618@gmail.com> wrote:
>

[snip]

>
> > Also: I just started going through the patches - nice idea with the
> > GPIO attributes, I really like it. Although I need to give it a longer
> > thought tomorrow - I'm wondering if we can maybe unify them and the
> > flags.
> >
>
> I had an earlier draft that did just that - and that is partially why
> the loop is last in wins - I was using slot 0 as the default flags.
> But the default flags cover a lot of use cases, including all of v1, and
> it was simple and cheap to provide a default - and it simplified the
> initial port of libgpiod to v2...
>

If porting libgpiod to v2 is the only concern then I wouldn't stress
about it. At the same time I'm wondering - is there any use-case where
we wouldn't need the flags attribute for at least some lines? Because
if it's always required than maybe having a default isn't that bad.

Bartosz

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 07/18] gpiolib: cdev: support edge detection for uAPI v2
  2020-07-25  4:19 ` [PATCH v2 07/18] gpiolib: cdev: support edge detection for uAPI v2 Kent Gibson
@ 2020-08-04 19:28   ` Bartosz Golaszewski
  0 siblings, 0 replies; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-04 19:28 UTC (permalink / raw)
  To: Kent Gibson; +Cc: LKML, linux-gpio, Linus Walleij

On Sat, Jul 25, 2020 at 6:21 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Add support for edge detection to lines requested using
> GPIO_GET_LINE_IOCTL.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---

[snip!]

> +
> +static irqreturn_t edge_irq_thread(int irq, void *p)
> +{
> +       struct edge_detector *edet = p;
> +       struct line *line = edet->line;
> +       struct gpio_desc *desc = edge_detector_desc(edet);
> +       struct gpioline_event le;
> +       int ret;
> +
> +       /* Do not leak kernel stack to userspace */
> +       memset(&le, 0, sizeof(le));
> +
> +       /*
> +        * We may be running from a nested threaded interrupt in which case
> +        * we didn't get the timestamp from edge_irq_handler().
> +        */
> +       if (!edet->timestamp) {
> +               le.timestamp = ktime_get_ns();

IIRC Marc suggested using smp_rmw() here before reading the timestamp.
Do we still need it or something changed?

Bartosz

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 11/18] gpio: uapi: document uAPI v1 as deprecated
  2020-07-25  4:19 ` [PATCH v2 11/18] gpio: uapi: document uAPI v1 as deprecated Kent Gibson
@ 2020-08-04 19:43   ` Bartosz Golaszewski
  0 siblings, 0 replies; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-04 19:43 UTC (permalink / raw)
  To: Kent Gibson; +Cc: LKML, linux-gpio, Linus Walleij

On Sat, Jul 25, 2020 at 6:22 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Update uAPI documentation to deprecate v1 structs and ioctls.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  include/uapi/linux/gpio.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 3f6db33014f0..92a74c245534 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -278,6 +278,9 @@ struct gpioline_event {
>
>  /*
>   *  ABI v1
> + *
> + * This version of the ABI is deprecated and will be removed in the future.
> + * Use the latest version if the ABI, defined above, instead.

typo: of the ABI

>   */
>
>  /* Informational flags */
> @@ -301,6 +304,9 @@ struct gpioline_event {
>   * @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
> + *
> + * This struct part of ABI v1 and is deprecated.

"This struct is part of ABI (...)"

> + * Use struct gpioline_info_v2 instead.
>   */
>  struct gpioline_info {
>         __u32 line_offset;
> @@ -325,6 +331,9 @@ struct gpioline_info {
>   * guarantee there are no implicit holes between it and subsequent members.
>   * The 20-byte padding at the end makes sure we don't add any implicit padding
>   * at the end of the structure on 64-bit architectures.
> + *
> + * This struct part of ABI v1 and is deprecated.

Same as above and same below.

Bart

> + * Use struct gpioline_info_changed_v2 instead.
>   */
>  struct gpioline_info_changed {
>         struct gpioline_info info;
> @@ -364,6 +373,9 @@ struct gpioline_info_changed {
>   * @fd: if successful this field will contain a valid anonymous file handle
>   * after a GPIO_GET_LINEHANDLE_IOCTL operation, zero or negative value
>   * means error
> + *
> + * This struct part of ABI v1 and is deprecated.
> + * Use struct gpioline_request instead.
>   */
>  struct gpiohandle_request {
>         __u32 lineoffsets[GPIOHANDLES_MAX];
> @@ -383,6 +395,9 @@ struct gpiohandle_request {
>   * this specifies the default output value, should be 0 (low) or
>   * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
>   * @padding: reserved for future use and should be zero filled
> + *
> + * This struct part of ABI v1 and is deprecated.
> + * Use struct gpioline_config instead.
>   */
>  struct gpiohandle_config {
>         __u32 flags;
> @@ -395,6 +410,9 @@ struct gpiohandle_config {
>   * @values: when getting the state of lines this contains the current
>   * state of a line, when setting the state of lines these should contain
>   * the desired target state
> + *
> + * This struct part of ABI v1 and is deprecated.
> + * Use struct gpioline_values instead.
>   */
>  struct gpiohandle_data {
>         __u8 values[GPIOHANDLES_MAX];
> @@ -418,6 +436,9 @@ struct gpiohandle_data {
>   * @fd: if successful this field will contain a valid anonymous file handle
>   * after a GPIO_GET_LINEEVENT_IOCTL operation, zero or negative value
>   * means error
> + *
> + * This struct part of ABI v1 and is deprecated.
> + * Use struct gpioline_request instead.
>   */
>  struct gpioevent_request {
>         __u32 lineoffset;
> @@ -437,6 +458,9 @@ struct gpioevent_request {
>   * struct gpioevent_data - The actual event being pushed to userspace
>   * @timestamp: best estimate of time of event occurrence, in nanoseconds
>   * @id: event identifier
> + *
> + * This struct part of ABI v1 and is deprecated.
> + * Use struct gpioline_event instead.
>   */
>  struct gpioevent_data {
>         __u64 timestamp;
> @@ -461,6 +485,8 @@ struct gpioevent_data {
>
>  /*
>   * v1 ioctl()s
> + *
> + * These ioctl()s are deprecated.  Use the v2 equivalent instead.
>   */
>  #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
>  #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL
  2020-08-04 17:47               ` Bartosz Golaszewski
@ 2020-08-05  3:06                 ` Kent Gibson
  0 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-08-05  3:06 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Andy Shevchenko, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, Linus Walleij

On Tue, Aug 04, 2020 at 07:47:43PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 4, 2020 at 1:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> 
> [snip]
> 
> >
> > > Also: I just started going through the patches - nice idea with the
> > > GPIO attributes, I really like it. Although I need to give it a longer
> > > thought tomorrow - I'm wondering if we can maybe unify them and the
> > > flags.
> > >
> >
> > I had an earlier draft that did just that - and that is partially why
> > the loop is last in wins - I was using slot 0 as the default flags.
> > But the default flags cover a lot of use cases, including all of v1, and
> > it was simple and cheap to provide a default - and it simplified the
> > initial port of libgpiod to v2...
> >
> 
> If porting libgpiod to v2 is the only concern then I wouldn't stress
> about it. At the same time I'm wondering - is there any use-case where
> we wouldn't need the flags attribute for at least some lines? Because
> if it's always required than maybe having a default isn't that bad.
> 

The only case where flags are not required is an AS-IS request. I
have no idea what that use case is useful for, but it is in v1 and
therefore supported by v2 for backward compatibility.

So there is almost always a flags attribute, and I didn't want to
waste an attribute slot on it.

Supporting the default in the kernel is trivial - it is literally just
the default return in gpioline_config_flags:

+	}
+	return lc->flags;
+}

which would otherwise be 0.

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 02/18] gpio: uapi: define uAPI v2
  2020-08-04 17:42   ` Bartosz Golaszewski
@ 2020-08-05  5:18     ` Kent Gibson
  2020-08-05 17:47       ` Bartosz Golaszewski
  2020-08-06  1:15       ` Kent Gibson
  0 siblings, 2 replies; 46+ messages in thread
From: Kent Gibson @ 2020-08-05  5:18 UTC (permalink / raw)
  To: Bartosz Golaszewski; +Cc: LKML, linux-gpio, Linus Walleij

On Tue, Aug 04, 2020 at 07:42:34PM +0200, Bartosz Golaszewski wrote:
> On Sat, Jul 25, 2020 at 6:20 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Add a new version of the uAPI to address existing 32/64-bit 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 32-bit and 64-bit platforms. That creates problems for 32-bit apps
> > running on 64-bit kernels.  The patch addresses that particular issue, and
> > the problem more generally, by adding pad fields that explicitly pad
> > structs out to 64-bit 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>
> > ---
> 
> Hi Kent,
> 
> Thanks a lot for your work on this. Please see comments below.
> 
> One thing I'd change globally for better readability is to have all
> new symbols marked as v2 - even if they have no counterparts in v1. I
> know libgpiod will wrap it all anyway but I think it's still a good
> way to make our work in user-space easier.
> 

Fair enough.  Oh joy.

> >
> > 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.
> >
> > I've also merged the handle and event requests into a single request, the
> > line request, as the two requests were 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.
> >
> 
> I think the info above is worth putting into the commit message.
> Especially the part about merging the two event types.
> 

OK, but I'll rework it a bit to make it more suitable for a commit
message.

> > Changes since v1:
> >  - lower case V1 and V2, except in capitalized names
> >  - hyphenate 32/64-bit
> >  - rename bitmap field to bits
> >  - drop PAD_SIZE consts in favour of hard coded numbers
> >  - sort includes
> >  - change config flags to __u64
> >  - increase padding of gpioline_event
> >  - relocate GPIOLINE_CHANGED enum into v2 section (is common with v1)
> >  - rework config to collapse direction, drive, bias and edge enums back
> >    into flags and add optional attributes that can be associated with a
> >    subset of the requested lines.
> >
> > 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
> >  - rename debounce to debounce_period
> >  - rename lines to num_lines
> >
> >  include/uapi/linux/gpio.h | 284 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 270 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> > index 285cc10355b2..3f6db33014f0 100644
> > --- a/include/uapi/linux/gpio.h
> > +++ b/include/uapi/linux/gpio.h
> > @@ -12,10 +12,13 @@
> >  #define _UAPI_GPIO_H_
> >
> >  #include <linux/ioctl.h>
> > +#include <linux/kernel.h>
> >  #include <linux/types.h>
> >
> >  /*
> >   * The maximum size of name and label arrays.
> > + *
> > + * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
> >   */
> >  #define GPIO_MAX_NAME_SIZE 32
> >
> > @@ -32,6 +35,251 @@ struct gpiochip_info {
> >         __u32 lines;
> >  };
> >
> > +/*
> > + * Maximum number of requested lines.
> > + *
> > + * Must be a multiple of 8 to ensure 32/64-bit 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)
> > +
> 
> In what circumstances can this be different than 1? It's worth
> documenting here I suppose.
> 

In terms of the API definition, GPIOLINES_MAX can be anything you want
and the definitions are still valid.  In practice in the mainline kernel
it would always be 1 for ABI compatibility.

Chiselling GPIOLINES_MAX <= 64 into stone could simplify things a bit,
as all the bitmaps reduce to a single __u64.  Would you prefer that?

> > +/*
> > + * The maximum number of configuration attributes associated with a line
> > + * request.
> > + */
> > +#define GPIOLINE_NUM_ATTRS_MAX 10
> > +
> 
> How did you choose this number? I mean: it's reasonable - just asking
> for clarification.
> 

I didn't want to constrain the possible configurations by making it too
small, particularly allowing for future attributes, but wanted to keep the
request size down so it can still comfortably fit on the stack.
The gpioline_request stands at 592 bytes, which is already substantially
larger than the 364 bytes of the v1 request, and each additional config
attribute slot adds another 24 bytes.

So 10 seemed like a happy medium.

> > +/**
> > + * enum gpioline_flag_v2 - &struct gpioline_attribute.flags values
> > + */
> > +enum gpioline_flag_v2 {
> > +       GPIOLINE_FLAG_V2_USED                   = 1UL << 0, /* line is not available for request */
> > +       GPIOLINE_FLAG_V2_ACTIVE_LOW             = 1UL << 1,
> > +       GPIOLINE_FLAG_V2_INPUT                  = 1UL << 2,
> > +       GPIOLINE_FLAG_V2_OUTPUT                 = 1UL << 3,
> > +       GPIOLINE_FLAG_V2_EDGE_RISING            = 1UL << 4,
> > +       GPIOLINE_FLAG_V2_EDGE_FALLING           = 1UL << 5,
> > +       GPIOLINE_FLAG_V2_OPEN_DRAIN             = 1UL << 6,
> > +       GPIOLINE_FLAG_V2_OPEN_SOURCE            = 1UL << 7,
> > +       GPIOLINE_FLAG_V2_BIAS_PULL_UP           = 1UL << 8,
> > +       GPIOLINE_FLAG_V2_BIAS_PULL_DOWN         = 1UL << 9,
> > +       GPIOLINE_FLAG_V2_BIAS_DISABLED          = 1UL << 10,
> > +};
> > +
> > +/**
> > + * struct gpioline_values - Values of GPIO lines
> > + * @bits: 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 the line is configured as active
> > + * low.
> > + */
> > +struct gpioline_values {
> > +       __u64 bits[GPIOLINES_BITMAP_SIZE];
> > +};
> > +
> 
> We can set values only for a subset of requested lines but AFAICT we
> can't read values of only a subset of lines. Would it be difficult to
> remove this limitation? While reading values always succeeds - even if
> the line is in input mode and has edge detected - I think that someone
> may want to request the max number of lines without reading all their
> values each time. Maybe consider merging this with struct
> gpioline_set_values?
> 

That is correct.

I considered that corner case to be unlikely, as a major point of
requesting lines together is to be able to perform collective operations
on them as atomically as possible.  If you only want subsets then
request them as separate subsets.

Do you have a case in mind where you would have overlapping subsets?

Not difficult to remove the limitation - I just didn't see sufficient
benefit. 

> > +/**
> > + * struct gpioline_set_values - Values to set a group of GPIO lines
> > + * @mask: a bitmap identifying the lines to set.
> > + * @bits: 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 the line is configured as active
> > + * low.
> > + */
> > +struct gpioline_set_values {
> > +       __u64 mask[GPIOLINES_BITMAP_SIZE];
> > +       __u64 bits[GPIOLINES_BITMAP_SIZE];
> > +};
> > +
> > +/**
> > + * enum gpioline_attr_id - &struct gpioline_attribute.id values
> > + */
> > +enum gpioline_attr_id {
> > +       GPIOLINE_ATTR_ID_FLAGS                  = 1,
> > +       GPIOLINE_ATTR_ID_OUTPUT_VALUES          = 2,
> > +       GPIOLINE_ATTR_ID_DEBOUNCE               = 3,
> > +};
> > +
> > +/**
> > + * struct gpioline_attribute - a configurable attribute of a line
> > + * @id: attribute identifier with value from &enum gpioline_attr_id
> > + * @padding: reserved for future use and must be zero filled
> > + * @flags: if id is GPIOLINE_ATTR_ID_FLAGS, the flags for the GPIO line,
> > + * with values from enum gpioline_flag_v2, such as
> > + * GPIOLINE_FLAG_V2_ACTIVE_LOW, GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed
> > + * together.  This overrides the default flags contained in the &struct
> > + * gpioline_config for the associated line.
> > + * @values: if id is GPIOLINE_ATTR_ID_OUTPUT_VALUES, the values to which
> > + * the lines will be set
> > + * @debounce_period: if id is GPIOLINE_ATTR_ID_DEBOUNCE, the desired
> > + * debounce period, in microseconds
> > + */
> > +struct gpioline_attribute {
> > +       __u32 id;
> > +       __u32 padding;
> > +       union {
> > +               __u64 flags;
> > +               struct gpioline_values values;
> > +               __u32 debounce_period;
> > +       };
> > +};
> 
> I'm afraid that if we don't have enough padding here (at the end),
> we'll end up wanting to add a new attribute at some point whose
> argument won't fit. Maybe have a specific field in the union that's
> even larger than __u64?
> 

I'm satisfied with the 64-bit value restriction.

I don't want to go adding another 8 bytes of pad per attribute on the
off chance that we ever find such an attribute, and that we couldn't
find some other solution like using the __u32 padding, or user the
gpioline_config padding, or split it over two attributes....

> > +
> > +/**
> > + * struct gpioline_config_attribute - a configuration attribute associated
> > + * with one or more of the requested lines.
> > + * @mask: a bitmap identifying the lines to which the attribute applies
> > + * @attr: the configurable attribute
> > + */
> > +struct gpioline_config_attribute {
> > +       __u64 mask[GPIOLINES_BITMAP_SIZE];
> > +       struct gpioline_attribute attr;
> > +};
> > +
> > +/**
> > + * struct gpioline_config - Configuration for GPIO lines
> > + * @flags: flags for the GPIO lines, with values from enum
> > + * gpioline_flag_v2, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> > + * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together.  This is the default for
> > + * all requested lines but may be overridden for particular lines using
> > + * attrs.
> 
> So I'm having a hard time with this. I understand that the thinking
> behind it was: use the flags field to set all lines to INPUT by
> default and only set certain lines to OUTPUT with attrs. This would
> make life easier for user-space but it complicates the kernel code and
> I also believe that any such simplification should be handled by
> user-space libraries, not be exposed by kernel uAPI. My personal
> preference would be to drop the flags field and only handle attributes
> (maybe even define a special macro to set all bits in mask -
> GPIOLINE_CONFIG_ALL_LINES or something) on a first-in-wins basis. I'm
> open to other suggestions though.
> 

I think I've addressed this elsewhere, and still think it is worthwhile
and very low cost.  I thought it was an easy win when I added it, and
still do.

Happy to change the attrs to first-in-wins though - the validation of
the attrs is still my biggest bugbear with this version.

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 02/18] gpio: uapi: define uAPI v2
  2020-08-05  5:18     ` Kent Gibson
@ 2020-08-05 17:47       ` Bartosz Golaszewski
  2020-08-06  1:03         ` Kent Gibson
  2020-08-06  1:15       ` Kent Gibson
  1 sibling, 1 reply; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-05 17:47 UTC (permalink / raw)
  To: Kent Gibson; +Cc: LKML, linux-gpio, Linus Walleij

On Wed, Aug 5, 2020 at 7:19 AM Kent Gibson <warthog618@gmail.com> wrote:
>

[snip]

> > >
> > > +/*
> > > + * Maximum number of requested lines.
> > > + *
> > > + * Must be a multiple of 8 to ensure 32/64-bit 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)
> > > +
> >
> > In what circumstances can this be different than 1? It's worth
> > documenting here I suppose.
> >
>
> In terms of the API definition, GPIOLINES_MAX can be anything you want
> and the definitions are still valid.  In practice in the mainline kernel
> it would always be 1 for ABI compatibility.
>
> Chiselling GPIOLINES_MAX <= 64 into stone could simplify things a bit,
> as all the bitmaps reduce to a single __u64.  Would you prefer that?
>

I'm not sure I follow. We need to chisel some max value in stone. Up
to that point it's been 64. We can make it more and the bitmap API
would handle it alright but if we don't, then this
__KERNEL_DIV_ROUND_UP() is unnecessary. Limiting it to 64 makes things
very simple thanks to fitting into a __u64 though. I've personally
never needed to request even half that so I guess this value's fine?

> > > +/*
> > > + * The maximum number of configuration attributes associated with a line
> > > + * request.
> > > + */
> > > +#define GPIOLINE_NUM_ATTRS_MAX 10
> > > +
> >
> > How did you choose this number? I mean: it's reasonable - just asking
> > for clarification.
> >
>
> I didn't want to constrain the possible configurations by making it too
> small, particularly allowing for future attributes, but wanted to keep the
> request size down so it can still comfortably fit on the stack.
> The gpioline_request stands at 592 bytes, which is already substantially
> larger than the 364 bytes of the v1 request, and each additional config
> attribute slot adds another 24 bytes.
>
> So 10 seemed like a happy medium.
>

Makes sense.

> > > +/**
> > > + * enum gpioline_flag_v2 - &struct gpioline_attribute.flags values
> > > + */
> > > +enum gpioline_flag_v2 {
> > > +       GPIOLINE_FLAG_V2_USED                   = 1UL << 0, /* line is not available for request */
> > > +       GPIOLINE_FLAG_V2_ACTIVE_LOW             = 1UL << 1,
> > > +       GPIOLINE_FLAG_V2_INPUT                  = 1UL << 2,
> > > +       GPIOLINE_FLAG_V2_OUTPUT                 = 1UL << 3,
> > > +       GPIOLINE_FLAG_V2_EDGE_RISING            = 1UL << 4,
> > > +       GPIOLINE_FLAG_V2_EDGE_FALLING           = 1UL << 5,
> > > +       GPIOLINE_FLAG_V2_OPEN_DRAIN             = 1UL << 6,
> > > +       GPIOLINE_FLAG_V2_OPEN_SOURCE            = 1UL << 7,
> > > +       GPIOLINE_FLAG_V2_BIAS_PULL_UP           = 1UL << 8,
> > > +       GPIOLINE_FLAG_V2_BIAS_PULL_DOWN         = 1UL << 9,
> > > +       GPIOLINE_FLAG_V2_BIAS_DISABLED          = 1UL << 10,
> > > +};
> > > +
> > > +/**
> > > + * struct gpioline_values - Values of GPIO lines
> > > + * @bits: 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 the line is configured as active
> > > + * low.
> > > + */
> > > +struct gpioline_values {
> > > +       __u64 bits[GPIOLINES_BITMAP_SIZE];
> > > +};
> > > +
> >
> > We can set values only for a subset of requested lines but AFAICT we
> > can't read values of only a subset of lines. Would it be difficult to
> > remove this limitation? While reading values always succeeds - even if
> > the line is in input mode and has edge detected - I think that someone
> > may want to request the max number of lines without reading all their
> > values each time. Maybe consider merging this with struct
> > gpioline_set_values?
> >
>
> That is correct.
>
> I considered that corner case to be unlikely, as a major point of
> requesting lines together is to be able to perform collective operations
> on them as atomically as possible.  If you only want subsets then
> request them as separate subsets.
>

And yet this version implements heterogeneous config and setting edge
detection and values of subsets of requested lines. :)

> Do you have a case in mind where you would have overlapping subsets?
>

No, not really but then I also don't have a use-case for setting only
a certain subset of lines.

> Not difficult to remove the limitation - I just didn't see sufficient
> benefit.
>

Using the same structure for setting and getting values is a benefit
IMO. If it's not a difficult task, then I think it's worth adding it.

> > > +/**
> > > + * struct gpioline_set_values - Values to set a group of GPIO lines
> > > + * @mask: a bitmap identifying the lines to set.
> > > + * @bits: 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 the line is configured as active
> > > + * low.
> > > + */
> > > +struct gpioline_set_values {
> > > +       __u64 mask[GPIOLINES_BITMAP_SIZE];
> > > +       __u64 bits[GPIOLINES_BITMAP_SIZE];
> > > +};
> > > +
> > > +/**
> > > + * enum gpioline_attr_id - &struct gpioline_attribute.id values
> > > + */
> > > +enum gpioline_attr_id {
> > > +       GPIOLINE_ATTR_ID_FLAGS                  = 1,
> > > +       GPIOLINE_ATTR_ID_OUTPUT_VALUES          = 2,
> > > +       GPIOLINE_ATTR_ID_DEBOUNCE               = 3,
> > > +};
> > > +
> > > +/**
> > > + * struct gpioline_attribute - a configurable attribute of a line
> > > + * @id: attribute identifier with value from &enum gpioline_attr_id
> > > + * @padding: reserved for future use and must be zero filled
> > > + * @flags: if id is GPIOLINE_ATTR_ID_FLAGS, the flags for the GPIO line,
> > > + * with values from enum gpioline_flag_v2, such as
> > > + * GPIOLINE_FLAG_V2_ACTIVE_LOW, GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed
> > > + * together.  This overrides the default flags contained in the &struct
> > > + * gpioline_config for the associated line.
> > > + * @values: if id is GPIOLINE_ATTR_ID_OUTPUT_VALUES, the values to which
> > > + * the lines will be set
> > > + * @debounce_period: if id is GPIOLINE_ATTR_ID_DEBOUNCE, the desired
> > > + * debounce period, in microseconds
> > > + */
> > > +struct gpioline_attribute {
> > > +       __u32 id;
> > > +       __u32 padding;
> > > +       union {
> > > +               __u64 flags;
> > > +               struct gpioline_values values;
> > > +               __u32 debounce_period;
> > > +       };
> > > +};
> >
> > I'm afraid that if we don't have enough padding here (at the end),
> > we'll end up wanting to add a new attribute at some point whose
> > argument won't fit. Maybe have a specific field in the union that's
> > even larger than __u64?
> >
>
> I'm satisfied with the 64-bit value restriction.
>
> I don't want to go adding another 8 bytes of pad per attribute on the
> off chance that we ever find such an attribute, and that we couldn't
> find some other solution like using the __u32 padding, or user the
> gpioline_config padding, or split it over two attributes....
>

Fair enough.

> > > +
> > > +/**
> > > + * struct gpioline_config_attribute - a configuration attribute associated
> > > + * with one or more of the requested lines.
> > > + * @mask: a bitmap identifying the lines to which the attribute applies
> > > + * @attr: the configurable attribute
> > > + */
> > > +struct gpioline_config_attribute {
> > > +       __u64 mask[GPIOLINES_BITMAP_SIZE];
> > > +       struct gpioline_attribute attr;
> > > +};
> > > +
> > > +/**
> > > + * struct gpioline_config - Configuration for GPIO lines
> > > + * @flags: flags for the GPIO lines, with values from enum
> > > + * gpioline_flag_v2, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> > > + * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together.  This is the default for
> > > + * all requested lines but may be overridden for particular lines using
> > > + * attrs.
> >
> > So I'm having a hard time with this. I understand that the thinking
> > behind it was: use the flags field to set all lines to INPUT by
> > default and only set certain lines to OUTPUT with attrs. This would
> > make life easier for user-space but it complicates the kernel code and
> > I also believe that any such simplification should be handled by
> > user-space libraries, not be exposed by kernel uAPI. My personal
> > preference would be to drop the flags field and only handle attributes
> > (maybe even define a special macro to set all bits in mask -
> > GPIOLINE_CONFIG_ALL_LINES or something) on a first-in-wins basis. I'm
> > open to other suggestions though.
> >
>
> I think I've addressed this elsewhere, and still think it is worthwhile
> and very low cost.  I thought it was an easy win when I added it, and
> still do.
>
> Happy to change the attrs to first-in-wins though - the validation of
> the attrs is still my biggest bugbear with this version.

Yes, I read your other reply. Ok, makes sense to have default flags
with an attribute for overrides. This just needs very explicit
documentation.

Bartosz

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 02/18] gpio: uapi: define uAPI v2
  2020-08-05 17:47       ` Bartosz Golaszewski
@ 2020-08-06  1:03         ` Kent Gibson
  0 siblings, 0 replies; 46+ messages in thread
From: Kent Gibson @ 2020-08-06  1:03 UTC (permalink / raw)
  To: Bartosz Golaszewski; +Cc: LKML, linux-gpio, Linus Walleij

On Wed, Aug 05, 2020 at 07:47:57PM +0200, Bartosz Golaszewski wrote:
> On Wed, Aug 5, 2020 at 7:19 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> 
> [snip]
> 
> > > >
> > > > +/*
> > > > + * Maximum number of requested lines.
> > > > + *
> > > > + * Must be a multiple of 8 to ensure 32/64-bit 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)
> > > > +
> > >
> > > In what circumstances can this be different than 1? It's worth
> > > documenting here I suppose.
> > >
> >
> > In terms of the API definition, GPIOLINES_MAX can be anything you want
> > and the definitions are still valid.  In practice in the mainline kernel
> > it would always be 1 for ABI compatibility.
> >
> > Chiselling GPIOLINES_MAX <= 64 into stone could simplify things a bit,
> > as all the bitmaps reduce to a single __u64.  Would you prefer that?
> >
> 
> I'm not sure I follow. We need to chisel some max value in stone. Up
> to that point it's been 64. We can make it more and the bitmap API
> would handle it alright but if we don't, then this
> __KERNEL_DIV_ROUND_UP() is unnecessary. Limiting it to 64 makes things
> very simple thanks to fitting into a __u64 though. I've personally
> never needed to request even half that so I guess this value's fine?
> 

By "chiselling in stone" I mean not supporting > 64 lines - even in
custom kernel builds.  The uAPI and definition and implementation would
lock that in.  As it stands a custom build could use > 64 and it should
all still work as the bitmaps would be resized.

I satisfied that 64 is more than enough for what this API is intended for,
so I'll change the bitmaps to a single __u64, and remove
GPIOLINES_BITMAP_SIZE.

[ snip]
> > > > +       __u64 bits[GPIOLINES_BITMAP_SIZE];
> > > > +};
> > > > +
> > >
> > > We can set values only for a subset of requested lines but AFAICT we
> > > can't read values of only a subset of lines. Would it be difficult to
> > > remove this limitation? While reading values always succeeds - even if
> > > the line is in input mode and has edge detected - I think that someone
> > > may want to request the max number of lines without reading all their
> > > values each time. Maybe consider merging this with struct
> > > gpioline_set_values?
> > >
> >
> > That is correct.
> >
> > I considered that corner case to be unlikely, as a major point of
> > requesting lines together is to be able to perform collective operations
> > on them as atomically as possible.  If you only want subsets then
> > request them as separate subsets.
> >
> 
> And yet this version implements heterogeneous config and setting edge
> detection and values of subsets of requested lines. :)
> 

The corner case I was referring to was only wanting to get a subset of
lines and caring that there may be a slight performance gain if the
kernel filters out the lines you aren't interested in :(.

> > Do you have a case in mind where you would have overlapping subsets?
> >
> 
> No, not really but then I also don't have a use-case for setting only
> a certain subset of lines.
> 
> > Not difficult to remove the limitation - I just didn't see sufficient
> > benefit.
> >
> 
> Using the same structure for setting and getting values is a benefit
> IMO. If it's not a difficult task, then I think it's worth adding it.
> 

OK, will add it in.

[ snip]
> > > (maybe even define a special macro to set all bits in mask -
> > > GPIOLINE_CONFIG_ALL_LINES or something) on a first-in-wins basis. I'm
> > > open to other suggestions though.
> > >
> >
> > I think I've addressed this elsewhere, and still think it is worthwhile
> > and very low cost.  I thought it was an easy win when I added it, and
> > still do.
> >
> > Happy to change the attrs to first-in-wins though - the validation of
> > the attrs is still my biggest bugbear with this version.
> 
> Yes, I read your other reply. Ok, makes sense to have default flags
> with an attribute for overrides. This just needs very explicit
> documentation.
> 

I'll add documentation that the attrs associations are on a
first-in-wins basis, and that subsequent associations are ignored.

Cheers,
Kent.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 02/18] gpio: uapi: define uAPI v2
  2020-08-05  5:18     ` Kent Gibson
  2020-08-05 17:47       ` Bartosz Golaszewski
@ 2020-08-06  1:15       ` Kent Gibson
  2020-08-06 15:53         ` Bartosz Golaszewski
  1 sibling, 1 reply; 46+ messages in thread
From: Kent Gibson @ 2020-08-06  1:15 UTC (permalink / raw)
  To: Bartosz Golaszewski; +Cc: LKML, linux-gpio, Linus Walleij

On Wed, Aug 05, 2020 at 01:18:53PM +0800, Kent Gibson wrote:
> On Tue, Aug 04, 2020 at 07:42:34PM +0200, Bartosz Golaszewski wrote:
> > On Sat, Jul 25, 2020 at 6:20 AM Kent Gibson <warthog618@gmail.com> wrote:

[snip]

> > > config, info, info_changed and event structs receive updated versions,
> > > and the first three new ioctls.
> > >
> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > ---
> > 
> > Hi Kent,
> > 
> > Thanks a lot for your work on this. Please see comments below.
> > 
> > One thing I'd change globally for better readability is to have all
> > new symbols marked as v2 - even if they have no counterparts in v1. I
> > know libgpiod will wrap it all anyway but I think it's still a good
> > way to make our work in user-space easier.
> > 
> 
> Fair enough.  Oh joy.
> 

Given that the intent is to highlight that the symbols are related to the
v2 of the GPIO uAPI, and not the second version of a particular type, it
makes more sense to me that the v2 is placed adjacent to the GPIO in the
name.  e.g. gpioline_flag_v2 would become gpiov2line_flag.

Does that work for you?

Cheers,
Kent.


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 02/18] gpio: uapi: define uAPI v2
  2020-08-06  1:15       ` Kent Gibson
@ 2020-08-06 15:53         ` Bartosz Golaszewski
  0 siblings, 0 replies; 46+ messages in thread
From: Bartosz Golaszewski @ 2020-08-06 15:53 UTC (permalink / raw)
  To: Kent Gibson; +Cc: LKML, linux-gpio, Linus Walleij

On Thu, Aug 6, 2020 at 3:15 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, Aug 05, 2020 at 01:18:53PM +0800, Kent Gibson wrote:
> > On Tue, Aug 04, 2020 at 07:42:34PM +0200, Bartosz Golaszewski wrote:
> > > On Sat, Jul 25, 2020 at 6:20 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> [snip]
>
> > > > config, info, info_changed and event structs receive updated versions,
> > > > and the first three new ioctls.
> > > >
> > > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > > ---
> > >
> > > Hi Kent,
> > >
> > > Thanks a lot for your work on this. Please see comments below.
> > >
> > > One thing I'd change globally for better readability is to have all
> > > new symbols marked as v2 - even if they have no counterparts in v1. I
> > > know libgpiod will wrap it all anyway but I think it's still a good
> > > way to make our work in user-space easier.
> > >
> >
> > Fair enough.  Oh joy.
> >
>
> Given that the intent is to highlight that the symbols are related to the
> v2 of the GPIO uAPI, and not the second version of a particular type, it
> makes more sense to me that the v2 is placed adjacent to the GPIO in the
> name.  e.g. gpioline_flag_v2 would become gpiov2line_flag.
>
> Does that work for you?
>

Yes, except that gpiov2line is a terrible prefix. Perhaps we should
make that into gpiov2_line_flag and same for all others? Maybe even
gpio_v2_line_flag if that doesn't make the symbols too long/too hard
to read.

Bartosz

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, back to index

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-25  4:19 [PATCH v2 00/18] gpio: cdev: add uAPI V2 Kent Gibson
2020-07-25  4:19 ` [PATCH v2 01/18] gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes Kent Gibson
2020-07-25  4:19 ` [PATCH v2 02/18] gpio: uapi: define uAPI v2 Kent Gibson
2020-08-04 17:42   ` Bartosz Golaszewski
2020-08-05  5:18     ` Kent Gibson
2020-08-05 17:47       ` Bartosz Golaszewski
2020-08-06  1:03         ` Kent Gibson
2020-08-06  1:15       ` Kent Gibson
2020-08-06 15:53         ` Bartosz Golaszewski
2020-07-25  4:19 ` [PATCH v2 03/18] gpiolib: make cdev a build option Kent Gibson
2020-07-25 20:29   ` Andy Shevchenko
2020-07-26 22:25   ` Linus Walleij
2020-07-27  1:46     ` Kent Gibson
2020-07-27  5:57       ` Kent Gibson
2020-07-27  8:15         ` Linus Walleij
2020-07-25  4:19 ` [PATCH v2 04/18] gpiolib: add build option for CDEV v1 ABI Kent Gibson
2020-07-25  4:19 ` [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL Kent Gibson
2020-07-25 20:51   ` Andy Shevchenko
2020-07-26  1:12     ` Kent Gibson
2020-07-26  3:24       ` Kent Gibson
2020-07-29  2:28       ` Kent Gibson
2020-07-29  8:05         ` Andy Shevchenko
2020-07-29 10:01           ` Kent Gibson
2020-07-31 16:05       ` Bartosz Golaszewski
2020-08-02  3:31         ` Kent Gibson
2020-08-02  9:32           ` Kent Gibson
2020-08-03 19:59             ` Bartosz Golaszewski
2020-08-03 20:02           ` Bartosz Golaszewski
2020-08-03 23:01             ` Kent Gibson
2020-08-04 17:47               ` Bartosz Golaszewski
2020-08-05  3:06                 ` Kent Gibson
2020-07-25  4:19 ` [PATCH v2 06/18] gpiolib: cdev: support GPIO_GET_LINEINFO_V2_IOCTL and GPIO_GET_LINEINFO_WATCH_V2_IOCTL Kent Gibson
2020-07-25  4:19 ` [PATCH v2 07/18] gpiolib: cdev: support edge detection for uAPI v2 Kent Gibson
2020-08-04 19:28   ` Bartosz Golaszewski
2020-07-25  4:19 ` [PATCH v2 08/18] gpiolib: cdev: support GPIOLINE_SET_CONFIG_IOCTL Kent Gibson
2020-07-25  4:19 ` [PATCH v2 09/18] gpiolib: cdev: support GPIOLINE_SET_VALUES_IOCTL Kent Gibson
2020-07-25  4:19 ` [PATCH v2 10/18] gpiolib: cdev: support setting debounce Kent Gibson
2020-07-25  4:19 ` [PATCH v2 11/18] gpio: uapi: document uAPI v1 as deprecated Kent Gibson
2020-08-04 19:43   ` Bartosz Golaszewski
2020-07-25  4:19 ` [PATCH v2 12/18] tools: gpio: port lsgpio to v2 uAPI Kent Gibson
2020-07-25  4:19 ` [PATCH v2 13/18] tools: gpio: port gpio-watch " Kent Gibson
2020-07-25  4:19 ` [PATCH v2 14/18] tools: gpio: rename nlines to num_lines Kent Gibson
2020-07-25  4:19 ` [PATCH v2 15/18] tools: gpio: port gpio-hammer to v2 uAPI Kent Gibson
2020-07-25  4:19 ` [PATCH v2 16/18] tools: gpio: port gpio-event-mon " Kent Gibson
2020-07-25  4:19 ` [PATCH v2 17/18] tools: gpio: add debounce support to gpio-event-mon Kent Gibson
2020-07-25  4:19 ` [PATCH v2 18/18] tools: gpio: add multi-line monitoring " Kent Gibson

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git