This patchset defines and implements a new version of the
GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add
support for debounce, event sequence numbers, and allow 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 three blocks; the first two patches
are minor fixes that impact later patches, the next eleven contain the
v2 uAPI definition and 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 for v5:
All changes for v5 fix issues with the gpiolib-cdev.c implementation,
in patches 07-12.
The uAPI is unchanged from v4, as is the port of the tools.
- use IS_ALIGNED in BUILD_BUG_ON checks (patch 07)
- relocate BUILD_BUG_ON checks to gpiolib_cdev_register (patch 07)
- s/requies/requires/ (patch 07)
- use unsigned int for variables that are never negative
- change lineinfo_get() parameter from cmd to bool watch (patch 08)
- flagsv2 in gpio_v2_line_info_to_v1() should be u64, not int (patch 08)
- change "_locked" suffixed function names to "_unlocked" (patch 10 and
11)
- be less eager breaking long lines
- move commentary into checkin comment where appropriate - particularly
patch 12
- restructure the request/line split - rename struct line to
struct linereq, and struct edge_detector to struct line, and relocate
the desc field from linereq to line. The linereq name was selected
over line_request as function names such as linereq_set_values() are
more clearly associated with requests than line_request_set_values(),
particularly as there is also a struct line. And linereq is as
informative as linerequest, so I went with the shortened form.
Changes for v4:
- bitmap width clarification in gpiod.h (patch 04)
- fix info offset initialisation bug (patch 08 and inserting patch 01)
- replace strncpy with strscpy to remove compiler warnings
(patch 08 and inserting patch 02)
- fix mask handling in line_get_values (patch 07)
Changes for v3:
- disabling the character device from the build requires EXPERT
- uAPI revisions (see patch 02)
- replace padding_not_zeroed with calls to memchr_inv
- don't use bitops on 64-bit flags as that doesn't work on BE-32
- accept first attribute matching a line in gpio_v2_line_config.attrs
rather than the last
- rework lsgpio port to uAPI v2 as flags reverted to v1 like layout
(since patch v2)
- swapped patches 17 and 18 to apply debounce to multiple monitored
lines
Changes for v2:
- 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 (20):
gpiolib: cdev: desc_to_lineinfo should set info offset
gpiolib: cdev: replace strncpy with strscpy
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_V2_GET_LINE_IOCTL and
GPIO_V2_LINE_GET_VALUES_IOCTL
gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and
GPIO_V2_GET_LINEINFO_WATCH_IOCTL
gpiolib: cdev: support edge detection for uAPI v2
gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL
gpiolib: cdev: support GPIO_V2_LINE_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 multi-line monitoring to gpio-event-mon
tools: gpio: add debounce support to gpio-event-mon
drivers/gpio/Kconfig | 29 +-
drivers/gpio/Makefile | 2 +-
drivers/gpio/gpiolib-cdev.c | 1315 +++++++++++++++++++++++++++++++++--
drivers/gpio/gpiolib-cdev.h | 15 +
drivers/gpio/gpiolib.c | 2 +
drivers/gpio/gpiolib.h | 6 +
include/uapi/linux/gpio.h | 316 ++++++++-
tools/gpio/gpio-event-mon.c | 146 ++--
tools/gpio/gpio-hammer.c | 56 +-
tools/gpio/gpio-utils.c | 127 ++--
tools/gpio/gpio-utils.h | 50 +-
tools/gpio/gpio-watch.c | 16 +-
tools/gpio/lsgpio.c | 60 +-
13 files changed, 1910 insertions(+), 230 deletions(-)
base-commit: 22cc422070d9a9a399f8a70b89f1b852945444cb
--
2.28.0
Replace usage of strncpy with strscpy to remove -Wstringop-truncation
warnings.
The structs being populated are zeroed, to prevent stack leakage as
they are returned to userspace, so strscpy performs the equivalent
function without the warnings.
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Kent Gibson <[email protected]>
---
The memset in gpio_desc_to_lineinfo(), in conjunction with the strscpy,
is necessary as strncpy zero pads the remainder of the destination.
It also guarantees that the info cannot leak kernel stack to userspace.
This is useful here, but is even more important for the v2 info, that
this function is changed to generate in a subsequent patch, as that
struct contains padding and attribute arrays that also need to be
initialised.
drivers/gpio/gpiolib-cdev.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e95e3eab9867..8b012879fe3f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -752,6 +752,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
bool ok_for_pinctrl;
unsigned long flags;
+ memset(info, 0, sizeof(*info));
info->line_offset = gpio_chip_hwgpio(desc);
/*
* This function takes a mutex so we must check this before taking
@@ -765,19 +766,11 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
spin_lock_irqsave(&gpio_lock, flags);
- if (desc->name) {
- strncpy(info->name, desc->name, sizeof(info->name));
- info->name[sizeof(info->name) - 1] = '\0';
- } else {
- info->name[0] = '\0';
- }
+ if (desc->name)
+ strscpy(info->name, desc->name, sizeof(info->name));
- if (desc->label) {
- strncpy(info->consumer, desc->label, sizeof(info->consumer));
- info->consumer[sizeof(info->consumer) - 1] = '\0';
- } else {
- info->consumer[0] = '\0';
- }
+ if (desc->label)
+ strscpy(info->consumer, desc->label, sizeof(info->consumer));
/*
* Userspace only need to know that the kernel is using this GPIO so
@@ -841,12 +834,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
memset(&chipinfo, 0, sizeof(chipinfo));
- strncpy(chipinfo.name, dev_name(&gdev->dev),
+ strscpy(chipinfo.name, dev_name(&gdev->dev),
sizeof(chipinfo.name));
- chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
- strncpy(chipinfo.label, gdev->label,
+ strscpy(chipinfo.label, gdev->label,
sizeof(chipinfo.label));
- chipinfo.label[sizeof(chipinfo.label)-1] = '\0';
chipinfo.lines = gdev->ngpio;
if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
return -EFAULT;
--
2.28.0
Port the lsgpio tool to the latest GPIO uAPI.
Signed-off-by: Kent Gibson <[email protected]>
---
tools/gpio/lsgpio.c | 60 ++++++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 22 deletions(-)
diff --git a/tools/gpio/lsgpio.c b/tools/gpio/lsgpio.c
index b08d7a5e779b..deda38244026 100644
--- a/tools/gpio/lsgpio.c
+++ b/tools/gpio/lsgpio.c
@@ -25,57 +25,73 @@
struct gpio_flag {
char *name;
- unsigned long mask;
+ unsigned long long mask;
};
struct gpio_flag flagnames[] = {
{
- .name = "kernel",
- .mask = GPIOLINE_FLAG_KERNEL,
+ .name = "used",
+ .mask = GPIO_V2_LINE_FLAG_USED,
+ },
+ {
+ .name = "input",
+ .mask = GPIO_V2_LINE_FLAG_INPUT,
},
{
.name = "output",
- .mask = GPIOLINE_FLAG_IS_OUT,
+ .mask = GPIO_V2_LINE_FLAG_OUTPUT,
},
{
.name = "active-low",
- .mask = GPIOLINE_FLAG_ACTIVE_LOW,
+ .mask = GPIO_V2_LINE_FLAG_ACTIVE_LOW,
},
{
.name = "open-drain",
- .mask = GPIOLINE_FLAG_OPEN_DRAIN,
+ .mask = GPIO_V2_LINE_FLAG_OPEN_DRAIN,
},
{
.name = "open-source",
- .mask = GPIOLINE_FLAG_OPEN_SOURCE,
+ .mask = GPIO_V2_LINE_FLAG_OPEN_SOURCE,
},
{
.name = "pull-up",
- .mask = GPIOLINE_FLAG_BIAS_PULL_UP,
+ .mask = GPIO_V2_LINE_FLAG_BIAS_PULL_UP,
},
{
.name = "pull-down",
- .mask = GPIOLINE_FLAG_BIAS_PULL_DOWN,
+ .mask = GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN,
},
{
.name = "bias-disabled",
- .mask = GPIOLINE_FLAG_BIAS_DISABLE,
+ .mask = GPIO_V2_LINE_FLAG_BIAS_DISABLED,
},
};
-void print_flags(unsigned long flags)
+static void print_attributes(struct gpio_v2_line_info *info)
{
int i;
- int printed = 0;
+ const char *field_format = "%s";
for (i = 0; i < ARRAY_SIZE(flagnames); i++) {
- if (flags & flagnames[i].mask) {
- if (printed)
- fprintf(stdout, " ");
- fprintf(stdout, "%s", flagnames[i].name);
- printed++;
+ if (info->flags & flagnames[i].mask) {
+ fprintf(stdout, field_format, flagnames[i].name);
+ field_format = ", %s";
}
}
+
+ if ((info->flags & GPIO_V2_LINE_FLAG_EDGE_RISING) &&
+ (info->flags & GPIO_V2_LINE_FLAG_EDGE_FALLING))
+ fprintf(stdout, field_format, "both-edges");
+ else if (info->flags & GPIO_V2_LINE_FLAG_EDGE_RISING)
+ fprintf(stdout, field_format, "rising-edge");
+ else if (info->flags & GPIO_V2_LINE_FLAG_EDGE_FALLING)
+ fprintf(stdout, field_format, "falling-edge");
+
+ for (i = 0; i < info->num_attrs; i++) {
+ if (info->attrs[i].id == GPIO_V2_LINE_ATTR_ID_DEBOUNCE)
+ fprintf(stdout, ", debounce_period=%dusec",
+ info->attrs[0].debounce_period);
+ }
}
int list_device(const char *device_name)
@@ -109,18 +125,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 gpio_v2_line_info 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_V2_GET_LINEINFO_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 +147,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.28.0
Add support for setting debounce on a line via the GPIO uAPI.
Where debounce is not supported by hardware, a software debounce is
provided.
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.
Signed-off-by: Kent Gibson <[email protected]>
---
Changes for v5:
- as per cover letter
Changes for v4:
- fix handling of mask in line_get_values
Changes for v3:
- only GPIO_V2 field renaming
Changes for v2:
- 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 | 254 +++++++++++++++++++++++++++++++++++-
drivers/gpio/gpiolib.h | 4 +
2 files changed, 252 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 3d2d9eefdfa0..b93479a98637 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"
@@ -396,6 +398,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
* events for the corresponding line request. This is drawn from the @req.
* @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 line {
struct gpio_desc *desc;
@@ -411,7 +416,27 @@ struct line {
*/
u64 timestamp;
u32 req_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 linereq_set_config(), which is the only
+ * setter, and linereq_get_values(), 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 linereq_get_values() which can live with a slightly
+ * stale value.
+ */
+ unsigned int level;
};
/**
@@ -518,6 +543,10 @@ static int edge_detector_start(struct line *line)
unsigned long irqflags = 0;
int irq, ret;
+ if (READ_ONCE(line->sw_debounced))
+ /* debouncer is setup and will provide edge detection */
+ return 0;
+
irq = gpiod_to_irq(line->desc);
if (irq <= 0)
return -ENODEV;
@@ -543,17 +572,204 @@ static int edge_detector_start(struct line *line)
return 0;
}
+/*
+ * returns the current debounced logical value.
+ */
+static unsigned int debounced_value(struct line *line)
+{
+ unsigned 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(line->level);
+
+ if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
+ value = !value;
+
+ return value;
+}
+
+static irqreturn_t debounce_irq_handler(int irq, void *p)
+{
+ struct line *line = p;
+
+ mod_delayed_work(system_wq, &line->work,
+ usecs_to_jiffies(READ_ONCE(line->desc->debounce_period)));
+
+ return IRQ_HANDLED;
+}
+
+static void debounce_work_func(struct work_struct *work)
+{
+ struct gpio_v2_line_event le;
+ struct line *line = container_of(work, struct line, work.work);
+ struct linereq *lr;
+ int ret, level;
+
+ level = gpiod_get_raw_value_cansleep(line->desc);
+ if (level < 0) {
+ pr_debug_ratelimited("debouncer failed to read line value\n");
+ return;
+ }
+
+ if (READ_ONCE(line->level) == level)
+ return;
+
+ WRITE_ONCE(line->level, level);
+
+ /* -- edge detection -- */
+ if (!line->eflags)
+ return;
+
+ /* switch from physical level to logical - if they differ */
+ if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
+ level = !level;
+
+ /* ignore edges that are not being monitored */
+ if (((line->eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) && !level) ||
+ ((line->eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) && level))
+ return;
+
+ /* Do not leak kernel stack to userspace */
+ memset(&le, 0, sizeof(le));
+
+ lr = line->req;
+ le.timestamp = ktime_get_ns();
+ le.offset = gpio_chip_hwgpio(line->desc);
+ line->line_seqno++;
+ le.line_seqno = line->line_seqno;
+ le.seqno = (lr->num_lines == 1) ?
+ le.line_seqno : atomic_inc_return(&lr->seqno);
+
+ if (level)
+ /* Emit low-to-high event */
+ le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
+ else
+ /* Emit high-to-low event */
+ le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
+
+ ret = kfifo_in_spinlocked_noirqsave(&lr->events, &le,
+ 1, &lr->wait.lock);
+ if (ret)
+ wake_up_poll(&lr->wait, EPOLLIN);
+ else
+ pr_debug_ratelimited("event FIFO is full - event dropped\n");
+}
+
+static int debounce_setup(struct line *line,
+ unsigned int debounce_period)
+{
+ unsigned long irqflags;
+ int ret, level, irq;
+
+ /* try hardware */
+ ret = gpiod_set_debounce(line->desc, debounce_period);
+ if (!ret) {
+ WRITE_ONCE(line->desc->debounce_period, debounce_period);
+ return ret;
+ }
+ if (ret != -ENOTSUPP)
+ return ret;
+
+ if (debounce_period) {
+ /* setup software debounce */
+ level = gpiod_get_raw_value_cansleep(line->desc);
+ if (level < 0)
+ return level;
+
+ irq = gpiod_to_irq(line->desc);
+ if (irq <= 0)
+ return -ENODEV;
+
+ WRITE_ONCE(line->level, level);
+ line->line_seqno = 0;
+ irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
+ ret = request_irq(irq, debounce_irq_handler, irqflags,
+ line->req->label, line);
+ if (ret)
+ return ret;
+
+ WRITE_ONCE(line->sw_debounced, 1);
+ line->irq = irq;
+ }
+ WRITE_ONCE(line->desc->debounce_period, debounce_period);
+ return 0;
+}
+
static void edge_detector_stop(struct line *line)
{
if (line->irq) {
free_irq(line->irq, line);
line->irq = 0;
}
+
+ cancel_delayed_work_sync(&line->work);
+ WRITE_ONCE(line->sw_debounced, 0);
+ /* do not change line->level - see comment in debounced_value */
+
+ if (line->desc)
+ WRITE_ONCE(line->desc->debounce_period, 0);
+}
+
+static int debounce_update(struct line *line,
+ unsigned int debounce_period)
+{
+ if (READ_ONCE(line->desc->debounce_period) == debounce_period)
+ return 0;
+
+ if (!READ_ONCE(line->sw_debounced))
+ return debounce_setup(line, debounce_period);
+
+ if (!debounce_period)
+ edge_detector_stop(line);
+ else
+ WRITE_ONCE(line->desc->debounce_period, debounce_period);
+ return 0;
+}
+
+static bool gpio_v2_line_config_debounced(struct gpio_v2_line_config *lc,
+ unsigned int line_idx)
+{
+ unsigned int i;
+ u64 mask = BIT_ULL(line_idx);
+
+ for (i = 0; i < lc->num_attrs; i++) {
+ if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_DEBOUNCE) &&
+ (lc->attrs[i].mask & mask))
+ return true;
+ }
+ return false;
+}
+
+static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
+ unsigned int line_idx)
+{
+ unsigned int i;
+ u64 mask = BIT_ULL(line_idx);
+
+ for (i = 0; i < lc->num_attrs; i++) {
+ if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_DEBOUNCE) &&
+ (lc->attrs[i].mask & mask))
+ return lc->attrs[i].attr.debounce_period;
+ }
+ return 0;
}
static int edge_detector_setup(struct line *line,
- struct gpio_v2_line_config *lc)
+ struct gpio_v2_line_config *lc,
+ unsigned int line_idx)
{
+ int ret;
+
+ if (gpio_v2_line_config_debounced(lc, line_idx)) {
+ ret = debounce_setup(line,
+ gpio_v2_line_config_debounce_period(lc, line_idx));
+ if (ret)
+ return ret;
+ }
if (line->eflags)
return edge_detector_start(line);
return 0;
@@ -693,6 +909,11 @@ static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
ret = gpio_v2_line_flags_validate(flags);
if (ret)
return ret;
+
+ /* debounce requires explicit input */
+ if (gpio_v2_line_config_debounced(lc, i) &&
+ !(flags & GPIO_V2_LINE_FLAG_INPUT))
+ return -EINVAL;
}
return 0;
}
@@ -750,7 +971,7 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
struct gpio_v2_line_values lv;
DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
struct gpio_desc **descs;
- unsigned int i, didx, num_get;
+ unsigned int i, val, didx, num_get;
int ret;
/* NOTE: It's ok to read values of output lines. */
@@ -787,7 +1008,11 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
lv.bits = 0;
for (didx = 0, i = 0; i < lr->num_lines; i++) {
if (lv.mask & BIT_ULL(i)) {
- if (test_bit(didx, vals))
+ if (lr->lines[i].sw_debounced)
+ val = debounced_value(&lr->lines[i]);
+ else
+ val = test_bit(didx, vals);
+ if (val)
lv.bits |= BIT_ULL(i);
didx++;
}
@@ -888,6 +1113,12 @@ static long linereq_set_config_unlocked(struct linereq *lr,
ret = gpiod_direction_input(desc);
if (ret)
return ret;
+ if (gpio_v2_line_config_debounced(lc, i)) {
+ ret = debounce_update(&lr->lines[i],
+ gpio_v2_line_config_debounce_period(lc, i));
+ if (ret)
+ return ret;
+ }
}
blocking_notifier_call_chain(&desc->gdev->notifier,
@@ -1083,8 +1314,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
lr->gdev = gdev;
get_device(&gdev->dev);
- for (i = 0; i < ulr.num_lines; i++)
+ for (i = 0; i < ulr.num_lines; i++) {
lr->lines[i].req = lr;
+ WRITE_ONCE(lr->lines[i].sw_debounced, 0);
+ INIT_DELAYED_WORK(&lr->lines[i].work, debounce_work_func);
+ }
/* Make sure this is terminated */
ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
@@ -1151,7 +1385,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
if (ret)
goto out_free_linereq;
lr->lines[i].eflags = flags & GPIO_V2_LINE_EDGE_FLAGS;
- ret = edge_detector_setup(&lr->lines[i], lc);
+ ret = edge_detector_setup(&lr->lines[i], lc, i);
if (ret)
goto out_free_linereq;
}
@@ -1621,6 +1855,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;
+ unsigned int num_attrs = 0;
memset(info, 0, sizeof(*info));
info->offset = gpio_chip_hwgpio(desc);
@@ -1680,7 +1916,13 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING;
- info->num_attrs = 0;
+ debounce_period = READ_ONCE(desc->debounce_period);
+ if (debounce_period) {
+ info->attrs[num_attrs].id = GPIO_V2_LINE_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.28.0
Update uAPI documentation to deprecate v1 structs and ioctls.
Signed-off-by: Kent Gibson <[email protected]>
---
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 0eb1f53b47e0..4af67415d73e 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -274,6 +274,9 @@ struct gpio_v2_line_event {
/*
* ABI v1
+ *
+ * This version of the ABI is deprecated and will be removed in the future.
+ * Use the latest version of the ABI, defined above, instead.
*/
/* Informational flags */
@@ -297,6 +300,9 @@ struct gpio_v2_line_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 is part of ABI v1 and is deprecated.
+ * Use struct gpio_v2_line_info instead.
*/
struct gpioline_info {
__u32 line_offset;
@@ -328,6 +334,9 @@ enum {
* 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 is part of ABI v1 and is deprecated.
+ * Use struct gpio_v2_line_info_changed instead.
*/
struct gpioline_info_changed {
struct gpioline_info info;
@@ -367,6 +376,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 is part of ABI v1 and is deprecated.
+ * Use struct gpio_v2_line_request instead.
*/
struct gpiohandle_request {
__u32 lineoffsets[GPIOHANDLES_MAX];
@@ -386,6 +398,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 is part of ABI v1 and is deprecated.
+ * Use struct gpio_v2_line_config instead.
*/
struct gpiohandle_config {
__u32 flags;
@@ -398,6 +413,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 is part of ABI v1 and is deprecated.
+ * Use struct gpio_v2_line_values instead.
*/
struct gpiohandle_data {
__u8 values[GPIOHANDLES_MAX];
@@ -421,6 +439,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 is part of ABI v1 and is deprecated.
+ * Use struct gpio_v2_line_request instead.
*/
struct gpioevent_request {
__u32 lineoffset;
@@ -440,6 +461,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 is part of ABI v1 and is deprecated.
+ * Use struct gpio_v2_line_event instead.
*/
struct gpioevent_data {
__u64 timestamp;
@@ -464,6 +488,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.28.0
Add support for the GPIO_V2_LINE_SET_VALUES_IOCTL.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 59 +++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 9c1e3f5f01af..3d2d9eefdfa0 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -799,6 +799,63 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
return 0;
}
+static long linereq_set_values_unlocked(struct linereq *lr,
+ struct gpio_v2_line_values *lv)
+{
+ DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
+ struct gpio_desc **descs;
+ unsigned int i, didx, num_set;
+ int ret;
+
+ bitmap_zero(vals, GPIO_V2_LINES_MAX);
+ for (num_set = 0, i = 0; i < lr->num_lines; i++) {
+ if (lv->mask & BIT_ULL(i)) {
+ if (!test_bit(FLAG_IS_OUT, &lr->lines[i].desc->flags))
+ return -EPERM;
+ if (lv->bits & BIT_ULL(i))
+ __set_bit(num_set, vals);
+ num_set++;
+ descs = &lr->lines[i].desc;
+ }
+ }
+ if (num_set == 0)
+ return -EINVAL;
+
+ if (num_set != 1) {
+ /* build compacted desc array and values */
+ descs = kmalloc_array(num_set, sizeof(*descs), GFP_KERNEL);
+ for (didx = 0, i = 0; i < lr->num_lines; i++) {
+ if (lv->mask & BIT_ULL(i)) {
+ descs[didx] = lr->lines[i].desc;
+ didx++;
+ }
+ }
+ }
+ ret = gpiod_set_array_value_complex(false, true, num_set,
+ descs, NULL, vals);
+
+ if (num_set != 1)
+ kfree(descs);
+ return ret;
+}
+
+static long linereq_set_values(struct linereq *lr, void __user *ip)
+{
+ struct gpio_v2_line_values lv;
+ int ret;
+
+ if (copy_from_user(&lv, ip, sizeof(lv)))
+ return -EFAULT;
+
+ mutex_lock(&lr->config_mutex);
+
+ ret = linereq_set_values_unlocked(lr, &lv);
+
+ mutex_unlock(&lr->config_mutex);
+
+ return ret;
+}
+
static long linereq_set_config_unlocked(struct linereq *lr,
struct gpio_v2_line_config *lc)
{
@@ -869,6 +926,8 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
if (cmd == GPIO_V2_LINE_GET_VALUES_IOCTL)
return linereq_get_values(lr, ip);
+ else if (cmd == GPIO_V2_LINE_SET_VALUES_IOCTL)
+ return linereq_set_values(lr, ip);
else if (cmd == GPIO_V2_LINE_SET_CONFIG_IOCTL)
return linereq_set_config(lr, ip);
--
2.28.0
Add support for GPIO_V2_LINE_SET_CONFIG_IOCTL, the uAPI v2
line set config ioctl.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 92 +++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 05b15f20fe2c..9c1e3f5f01af 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>
@@ -423,6 +424,8 @@ struct line {
* @seqno: the sequence number for edge events generated on all lines in
* this line request. Note that this is not used when @num_lines 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, particularly multi-step accesses to desc flags.
* @lines: the lines held by this line request, with @num_lines elements.
*/
struct linereq {
@@ -432,6 +435,7 @@ struct linereq {
wait_queue_head_t wait;
DECLARE_KFIFO_PTR(events, struct gpio_v2_line_event);
atomic_t seqno;
+ struct mutex config_mutex;
struct line lines[];
};
@@ -693,6 +697,29 @@ static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
return 0;
}
+static int gpio_v2_line_config_change_validate(struct linereq *lr,
+ struct gpio_v2_line_config *lc)
+{
+ unsigned int i;
+ u64 flags;
+
+ for (i = 0; i < lr->num_lines; i++) {
+ flags = gpio_v2_line_config_flags(lc, i);
+ /* disallow edge detection changes */
+ if (lr->lines[i].eflags != (flags & GPIO_V2_LINE_EDGE_FLAGS))
+ return -EINVAL;
+
+ if (lr->lines[i].eflags & GPIO_V2_LINE_EDGE_FLAGS) {
+ /* disallow polarity changes */
+ if (test_bit(FLAG_ACTIVE_LOW,
+ &lr->lines[i].desc->flags) !=
+ ((flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW) != 0))
+ return -EINVAL;
+ }
+ }
+ return 0;
+}
+
static void gpio_v2_line_config_flags_to_desc_flags(u64 flags,
unsigned long *flagsp)
{
@@ -772,6 +799,68 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
return 0;
}
+static long linereq_set_config_unlocked(struct linereq *lr,
+ struct gpio_v2_line_config *lc)
+{
+ struct gpio_desc *desc;
+ unsigned int i;
+ u64 flags;
+ int ret;
+
+ ret = gpio_v2_line_config_change_validate(lr, lc);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < lr->num_lines; i++) {
+ desc = lr->lines[i].desc;
+ flags = gpio_v2_line_config_flags(lc, i);
+
+ gpio_v2_line_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 & GPIO_V2_LINE_FLAG_OUTPUT) {
+ int val = gpio_v2_line_config_output_value(lc, i);
+
+ edge_detector_stop(&lr->lines[i]);
+ ret = gpiod_direction_output(desc, val);
+ if (ret)
+ return ret;
+ } else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
+ ret = gpiod_direction_input(desc);
+ if (ret)
+ return ret;
+ }
+
+ blocking_notifier_call_chain(&desc->gdev->notifier,
+ GPIO_V2_LINE_CHANGED_CONFIG,
+ desc);
+ }
+ return 0;
+}
+
+static long linereq_set_config(struct linereq *lr, void __user *ip)
+{
+ struct gpio_v2_line_config lc;
+ int ret;
+
+ if (copy_from_user(&lc, ip, sizeof(lc)))
+ return -EFAULT;
+
+ ret = gpio_v2_line_config_validate(&lc, lr->num_lines);
+ if (ret)
+ return ret;
+
+ mutex_lock(&lr->config_mutex);
+
+ ret = linereq_set_config_unlocked(lr, &lc);
+
+ mutex_unlock(&lr->config_mutex);
+
+ return ret;
+}
+
static long linereq_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -780,6 +869,8 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
if (cmd == GPIO_V2_LINE_GET_VALUES_IOCTL)
return linereq_get_values(lr, ip);
+ else if (cmd == GPIO_V2_LINE_SET_CONFIG_IOCTL)
+ return linereq_set_config(lr, ip);
return -EINVAL;
}
@@ -946,6 +1037,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
}
}
+ mutex_init(&lr->config_mutex);
init_waitqueue_head(&lr->wait);
if (has_edge_detection) {
size = ulr.event_buffer_size;
--
2.28.0
Add a new version of the uAPI to address existing 32/64-bit alignment
issues, add support for debounce and event sequence numbers, allow
requested lines with different configurations, 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. uAPI v2 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 new structs have been analysed with pahole to ensure that they
are sized as expected and contain no implicit padding.
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 configurable attributes in
line config and reserved padding in all structs for future features.
Specifically, the line request, config, info, info_changed and event
structs receive updated versions and new ioctls.
As the majority of the structs and ioctls were being replaced, it is
opportune to rework some of the other aspects of the uAPI:
v1 has three different flags fields, each with their own separate
bit definitions. In v2 that is collapsed to one - gpio_v2_line_flag.
The handle and event requests are merged 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.
As a consequence, 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 GPIO_V2_LINES_MAX in v2 for
clarity, and the gpiohandle_data __u8 array becomes a bitmap in
gpio_v2_line_values.
The v2 uAPI is mostly a reorganisation and extension of v1, so userspace
code, particularly libgpiod, should readily port to it.
Signed-off-by: Kent Gibson <[email protected]>
---
Changes for v4:
- clarify bitmap width in GPIO_V2_LINES_MAX description
Changes for v3:
- relocated commentary into commit description
- hard limit max requested lines to 64 so bitmaps always fit in a single
u64.
- prefix all v2 symbols with GPIO_V2
- 64-bit flag values to ULL
- use __aligned_u64 to ensure 64-bit fields are 64-bit aligned
- support masked get values, as per set values.
Changes for v2:
- 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 for v1 (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 | 273 +++++++++++++++++++++++++++++++++++++-
1 file changed, 266 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 285cc10355b2..0eb1f53b47e0 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -16,6 +16,8 @@
/*
* 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 +34,248 @@ struct gpiochip_info {
__u32 lines;
};
+/*
+ * Maximum number of requested lines.
+ *
+ * Must be no greater than 64, as bitmaps are restricted here to 64-bits
+ * for simplicity, and a multiple of 2 to ensure 32/64-bit alignment of
+ * structs.
+ */
+#define GPIO_V2_LINES_MAX 64
+
+/*
+ * The maximum number of configuration attributes associated with a line
+ * request.
+ */
+#define GPIO_V2_LINE_NUM_ATTRS_MAX 10
+
+/**
+ * enum gpio_v2_line_flag - &struct gpio_v2_line_attribute.flags values
+ */
+enum gpio_v2_line_flag {
+ GPIO_V2_LINE_FLAG_USED = 1ULL << 0, /* line is not available for request */
+ GPIO_V2_LINE_FLAG_ACTIVE_LOW = 1ULL << 1,
+ GPIO_V2_LINE_FLAG_INPUT = 1ULL << 2,
+ GPIO_V2_LINE_FLAG_OUTPUT = 1ULL << 3,
+ GPIO_V2_LINE_FLAG_EDGE_RISING = 1ULL << 4,
+ GPIO_V2_LINE_FLAG_EDGE_FALLING = 1ULL << 5,
+ GPIO_V2_LINE_FLAG_OPEN_DRAIN = 1ULL << 6,
+ GPIO_V2_LINE_FLAG_OPEN_SOURCE = 1ULL << 7,
+ GPIO_V2_LINE_FLAG_BIAS_PULL_UP = 1ULL << 8,
+ GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = 1ULL << 9,
+ GPIO_V2_LINE_FLAG_BIAS_DISABLED = 1ULL << 10,
+};
+
+/**
+ * struct gpio_v2_line_values - Values of GPIO lines
+ * @mask: a bitmap identifying the lines to get or set, with each bit
+ * number corresponding to the index into &struct
+ * gpio_v2_line_request.offsets.
+ * @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 gpio_v2_line_values {
+ __aligned_u64 mask;
+ __aligned_u64 bits;
+};
+
+/**
+ * enum gpio_v2_line_attr_id - &struct gpio_v2_line_attribute.id values
+ */
+enum gpio_v2_line_attr_id {
+ GPIO_V2_LINE_ATTR_ID_FLAGS = 1,
+ GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES = 2,
+ GPIO_V2_LINE_ATTR_ID_DEBOUNCE = 3,
+};
+
+/**
+ * struct gpio_v2_line_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpio_v2_line_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO
+ * line, with values from enum gpio_v2_line_flag, such as
+ * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed
+ * together. This overrides the default flags contained in the &struct
+ * gpio_v2_line_config for the associated line.
+ * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap
+ * containing the values to which the lines will be set, with each bit
+ * number corresponding to the index into &struct
+ * gpio_v2_line_request.offsets.
+ * @debounce_period: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds
+ */
+struct gpio_v2_line_attribute {
+ __u32 id;
+ __u32 padding;
+ union {
+ __aligned_u64 flags;
+ __aligned_u64 values;
+ __u32 debounce_period;
+ };
+};
+
+/**
+ * struct gpio_v2_line_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,
+ * with each bit number corresponding to the index into &struct
+ * gpio_v2_line_request.offsets.
+ * @attr: the configurable attribute
+ */
+struct gpio_v2_line_config_attribute {
+ __aligned_u64 mask;
+ struct gpio_v2_line_attribute attr;
+};
+
+/**
+ * struct gpio_v2_line_config - Configuration for GPIO lines
+ * @flags: flags for the GPIO lines, with values from enum
+ * gpio_v2_line_flag, such as GPIO_V2_LINE_FLAG_ACTIVE_LOW,
+ * GPIO_V2_LINE_FLAG_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. Any attribute should only be associated with a particular line
+ * once. If an attribute is associated with a line multiple times then the
+ * first occurrence (i.e. lowest index) has precedence.
+ */
+struct gpio_v2_line_config {
+ __aligned_u64 flags;
+ __u32 num_attrs;
+ /*
+ * Pad to fill implicit padding and provide space for future use.
+ */
+ __u32 padding[5];
+ struct gpio_v2_line_config_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX];
+};
+
+/**
+ * struct gpio_v2_line_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 GPIO_V2_LINES_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 gpio_v2_line_request {
+ __u32 offsets[GPIO_V2_LINES_MAX];
+ char consumer[GPIO_MAX_NAME_SIZE];
+ struct gpio_v2_line_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 gpio_v2_line_info - 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 GPIO_V2_LINE_FLAG_ACTIVE_LOW,
+ * GPIO_V2_LINE_FLAG_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 gpio_v2_line_info {
+ char name[GPIO_MAX_NAME_SIZE];
+ char consumer[GPIO_MAX_NAME_SIZE];
+ __u32 offset;
+ __u32 num_attrs;
+ __aligned_u64 flags;
+ struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX];
+ /*
+ * Pad struct to 64-bit boundary and provide space for future use.
+ */
+ __u32 padding[4];
+};
+
+enum gpio_v2_line_changed_type {
+ GPIO_V2_LINE_CHANGED_REQUESTED = 1,
+ GPIO_V2_LINE_CHANGED_RELEASED = 2,
+ GPIO_V2_LINE_CHANGED_CONFIG = 3,
+};
+
+/**
+ * struct gpio_v2_line_info_changed - 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
+ * gpio_v2_line_changed_type
+ * @padding: reserved for future use
+ */
+struct gpio_v2_line_info_changed {
+ struct gpio_v2_line_info info;
+ __aligned_u64 timestamp;
+ __u32 event_type;
+ /*
+ * Pad struct to 64-bit boundary and provide space for future use.
+ */
+ __u32 padding[5];
+};
+
+enum gpio_v2_line_event_id {
+ GPIO_V2_LINE_EVENT_RISING_EDGE = 1,
+ GPIO_V2_LINE_EVENT_FALLING_EDGE = 2,
+};
+
+/**
+ * struct gpio_v2_line_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 gpio_v2_line_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 gpio_v2_line_event {
+ __aligned_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)
@@ -149,8 +393,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 +403,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 +446,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_V2_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x05, struct gpio_v2_line_info)
+#define GPIO_V2_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x06, struct gpio_v2_line_info)
+#define GPIO_V2_GET_LINE_IOCTL _IOWR(0xB4, 0x07, struct gpio_v2_line_request)
+#define GPIO_V2_LINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0D, struct gpio_v2_line_config)
+#define GPIO_V2_LINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x0E, struct gpio_v2_line_values)
+#define GPIO_V2_LINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x0F, struct gpio_v2_line_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.28.0
Add a build option to allow the removal of the CDEV v1 ABI.
Suggested-by: Bartosz Golaszewski <[email protected]>
Signed-off-by: Kent Gibson <[email protected]>
---
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 8e409b9c33dc..0c62e35cf3a6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -82,6 +82,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.28.0
Add support for GPIO_V2_GET_LINEINFO_IOCTL and
GPIO_V2_GET_LINEINFO_WATCH_IOCTL.
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_V2_GET_LINEINFO_WATCH_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.
Signed-off-by: Kent Gibson <[email protected]>
---
Changes for v5:
- as per cover letter
Changes for v4:
- replace strncpy with memcpy in gpio_v2_line_info_to_v1
drivers/gpio/gpiolib-cdev.c | 195 +++++++++++++++++++++++++++++++-----
1 file changed, 168 insertions(+), 27 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 1c0ed493cb83..281255e05323 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -162,7 +162,8 @@ static long linehandle_set_config(struct linehandle_state *lh,
}
blocking_notifier_call_chain(&desc->gdev->notifier,
- GPIOLINE_CHANGED_CONFIG, desc);
+ GPIO_V2_LINE_CHANGED_CONFIG,
+ desc);
}
return 0;
}
@@ -334,7 +335,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
}
blocking_notifier_call_chain(&desc->gdev->notifier,
- GPIOLINE_CHANGED_REQUESTED, desc);
+ GPIO_V2_LINE_CHANGED_REQUESTED, desc);
dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
offset);
@@ -724,7 +725,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
}
blocking_notifier_call_chain(&desc->gdev->notifier,
- GPIOLINE_CHANGED_REQUESTED, desc);
+ GPIO_V2_LINE_CHANGED_REQUESTED, desc);
dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
offset);
@@ -1071,7 +1072,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
goto out_free_le;
blocking_notifier_call_chain(&desc->gdev->notifier,
- GPIOLINE_CHANGED_REQUESTED, desc);
+ GPIO_V2_LINE_CHANGED_REQUESTED, desc);
irq = gpiod_to_irq(desc);
if (irq <= 0) {
@@ -1138,17 +1139,59 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
return ret;
}
+static void gpio_v2_line_info_to_v1(struct gpio_v2_line_info *info_v2,
+ struct gpioline_info *info_v1)
+{
+ u64 flagsv2 = info_v2->flags;
+
+ memcpy(info_v1->name, info_v2->name, sizeof(info_v1->name));
+ memcpy(info_v1->consumer, info_v2->consumer,
+ sizeof(info_v1->consumer));
+ info_v1->line_offset = info_v2->offset;
+ info_v1->flags = 0;
+
+ if (flagsv2 & GPIO_V2_LINE_FLAG_USED)
+ info_v1->flags |= GPIOLINE_FLAG_KERNEL;
+
+ if (flagsv2 & GPIO_V2_LINE_FLAG_OUTPUT)
+ info_v1->flags |= GPIOLINE_FLAG_IS_OUT;
+
+ if (flagsv2 & GPIO_V2_LINE_FLAG_ACTIVE_LOW)
+ info_v1->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+
+ if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_DRAIN)
+ info_v1->flags |= GPIOLINE_FLAG_OPEN_DRAIN;
+ if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_SOURCE)
+ info_v1->flags |= GPIOLINE_FLAG_OPEN_SOURCE;
+
+ if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)
+ info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+ if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN)
+ info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+ if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_DISABLED)
+ info_v1->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+}
+
+static void gpio_v2_line_info_changed_to_v1(
+ struct gpio_v2_line_info_changed *lic_v2,
+ struct gpioline_info_changed *lic_v1)
+{
+ gpio_v2_line_info_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 gpio_v2_line_info *info)
{
struct gpio_chip *gc = desc->gdev->chip;
bool ok_for_pinctrl;
unsigned long flags;
memset(info, 0, sizeof(*info));
- info->line_offset = gpio_chip_hwgpio(desc);
+ info->offset = gpio_chip_hwgpio(desc);
/*
* This function takes a mutex so we must check this before taking
* the spinlock.
@@ -1157,7 +1200,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);
@@ -1178,23 +1221,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 |= GPIO_V2_LINE_FLAG_USED;
+
if (test_bit(FLAG_IS_OUT, &desc->flags))
- info->flags |= GPIOLINE_FLAG_IS_OUT;
+ info->flags |= GPIO_V2_LINE_FLAG_OUTPUT;
+ else
+ info->flags |= GPIO_V2_LINE_FLAG_INPUT;
+
if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
- info->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+ info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW;
+
if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
- info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
- GPIOLINE_FLAG_IS_OUT);
+ info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN;
if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
- info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
- GPIOLINE_FLAG_IS_OUT);
+ info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE;
+
if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
- info->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+ info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED;
if (test_bit(FLAG_PULL_DOWN, &desc->flags))
- info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+ info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
if (test_bit(FLAG_PULL_UP, &desc->flags))
- info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+ info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
spin_unlock_irqrestore(&gpio_lock, flags);
}
@@ -1202,11 +1249,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 gpio_v2_line_info_changed, 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,
+ bool watch)
+{
+ struct gpio_desc *desc;
+ struct gpio_v2_line_info lineinfo;
+
+ if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+ return -EFAULT;
+
+ if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding)))
+ return -EINVAL;
+
+ desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset);
+ if (IS_ERR(desc))
+ return PTR_ERR(desc);
+
+ if (watch) {
+#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 (watch)
+ clear_bit(lineinfo.offset, cdev->watched_lines);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
/*
* gpio_ioctl() - ioctl handler for the GPIO chardev
*/
@@ -1216,7 +1317,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 */
@@ -1239,7 +1339,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 gpio_v2_line_info lineinfo_v2;
if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
@@ -1249,7 +1351,8 @@ 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);
+ gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo);
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
return -EFAULT;
@@ -1259,7 +1362,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 gpio_v2_line_info lineinfo_v2;
if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
return -EFAULT;
@@ -1269,10 +1374,14 @@ 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);
+ gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo);
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
clear_bit(lineinfo.line_offset, cdev->watched_lines);
@@ -1281,6 +1390,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
return 0;
#endif /* CONFIG_GPIO_CDEV_V1 */
+ } else if (cmd == GPIO_V2_GET_LINEINFO_IOCTL ||
+ cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL) {
+ return lineinfo_get(cdev, ip,
+ cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL);
} else if (cmd == GPIO_V2_GET_LINE_IOCTL) {
return linereq_create(gdev, ip);
} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
@@ -1316,7 +1429,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 gpio_v2_line_info_changed chg;
struct gpio_desc *desc = data;
int ret;
@@ -1356,12 +1469,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 gpio_v2_line_info_changed 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 gpio_v2_line_info_changed);
+ if (count < event_size)
return -EINVAL;
+#endif
do {
spin_lock(&cdev->wait.lock);
@@ -1383,7 +1500,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 gpio_v2_line_info_changed);
+ 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) {
@@ -1392,9 +1519,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 gpio_v2_line_info_changed)) {
+ if (copy_to_user(buf + bytes_read, &event, event_size))
+ return -EFAULT;
+ } else {
+ struct gpioline_info_changed event_v1;
+
+ gpio_v2_line_info_changed_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.28.0
Add support for edge detection to lines requested using
GPIO_V2_GET_LINE_IOCTL.
The edge_detector implementation is based on the v1 lineevent
implementation.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 290 +++++++++++++++++++++++++++++++++++-
drivers/gpio/gpiolib.c | 2 +
drivers/gpio/gpiolib.h | 2 +
3 files changed, 293 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 281255e05323..05b15f20fe2c 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -385,9 +385,32 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
/**
* struct line - contains the state of a requested line
* @desc: the GPIO descriptor for this line.
+ * @req: the corresponding line request
+ * @irq: the interrupt triggered in response to events on this GPIO
+ * @eflags: the edge flags, GPIO_V2_LINE_FLAG_EDGE_RISING and/or
+ * GPIO_V2_LINE_FLAG_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
+ * @req_seqno: the seqno for the current edge event in the sequence of
+ * events for the corresponding line request. This is drawn from the @req.
+ * @line_seqno: the seqno for the current edge event in the sequence of
+ * events for this line.
*/
struct line {
struct gpio_desc *desc;
+ /*
+ * -- edge detector specific fields --
+ */
+ struct linereq *req;
+ unsigned int irq;
+ u64 eflags;
+ /*
+ * timestamp and req_seqno are shared by edge_irq_handler() and
+ * edge_irq_thread() which are themselves mutually exclusive.
+ */
+ u64 timestamp;
+ u32 req_seqno;
+ u32 line_seqno;
};
/**
@@ -395,15 +418,143 @@ struct line {
* @gdev: the GPIO device the line request pertains to
* @label: consumer label used to tag GPIO descriptors
* @num_lines: the number of lines in the lines 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_lines is 1, as
+ * the line_seqno is then the same and is cheaper to calculate.
* @lines: the lines held by this line request, with @num_lines elements.
*/
struct linereq {
struct gpio_device *gdev;
const char *label;
u32 num_lines;
+ wait_queue_head_t wait;
+ DECLARE_KFIFO_PTR(events, struct gpio_v2_line_event);
+ atomic_t seqno;
struct line lines[];
};
+static irqreturn_t edge_irq_thread(int irq, void *p)
+{
+ struct line *line = p;
+ struct linereq *lr = line->req;
+ struct gpio_v2_line_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 (!line->timestamp) {
+ le.timestamp = ktime_get_ns();
+ if (lr->num_lines != 1)
+ line->req_seqno = atomic_inc_return(&lr->seqno);
+ } else {
+ le.timestamp = line->timestamp;
+ }
+ line->timestamp = 0;
+
+ if (line->eflags == (GPIO_V2_LINE_FLAG_EDGE_RISING |
+ GPIO_V2_LINE_FLAG_EDGE_FALLING)) {
+ int level = gpiod_get_value_cansleep(line->desc);
+
+ if (level)
+ /* Emit low-to-high event */
+ le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
+ else
+ /* Emit high-to-low event */
+ le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
+ } else if (line->eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) {
+ /* Emit low-to-high event */
+ le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
+ } else if (line->eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) {
+ /* Emit high-to-low event */
+ le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
+ } else {
+ return IRQ_NONE;
+ }
+ line->line_seqno++;
+ le.line_seqno = line->line_seqno;
+ le.seqno = (lr->num_lines == 1) ? le.line_seqno : line->req_seqno;
+ le.offset = gpio_chip_hwgpio(line->desc);
+
+ ret = kfifo_in_spinlocked_noirqsave(&lr->events, &le,
+ 1, &lr->wait.lock);
+ if (ret)
+ wake_up_poll(&lr->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 line *line = p;
+ struct linereq *lr = line->req;
+
+ /*
+ * Just store the timestamp in hardirq context so we get it as
+ * close in time as possible to the actual event.
+ */
+ line->timestamp = ktime_get_ns();
+
+ if (lr->num_lines != 1)
+ line->req_seqno = atomic_inc_return(&lr->seqno);
+
+ return IRQ_WAKE_THREAD;
+}
+
+static int edge_detector_start(struct line *line)
+{
+ unsigned long irqflags = 0;
+ int irq, ret;
+
+ irq = gpiod_to_irq(line->desc);
+ if (irq <= 0)
+ return -ENODEV;
+
+ line->req_seqno = 0;
+ line->line_seqno = 0;
+
+ if (line->eflags & GPIO_V2_LINE_FLAG_EDGE_RISING)
+ irqflags |= test_bit(FLAG_ACTIVE_LOW, &line->desc->flags) ?
+ IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
+ if (line->eflags & GPIO_V2_LINE_FLAG_EDGE_FALLING)
+ irqflags |= test_bit(FLAG_ACTIVE_LOW, &line->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, line->req->label, line);
+ if (ret)
+ return ret;
+
+ line->irq = irq;
+ return 0;
+}
+
+static void edge_detector_stop(struct line *line)
+{
+ if (line->irq) {
+ free_irq(line->irq, line);
+ line->irq = 0;
+ }
+}
+
+static int edge_detector_setup(struct line *line,
+ struct gpio_v2_line_config *lc)
+{
+ if (line->eflags)
+ return edge_detector_start(line);
+ return 0;
+}
+
#define GPIO_V2_LINE_BIAS_FLAGS \
(GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
@@ -417,10 +568,15 @@ struct linereq {
(GPIO_V2_LINE_FLAG_OPEN_DRAIN | \
GPIO_V2_LINE_FLAG_OPEN_SOURCE)
+#define GPIO_V2_LINE_EDGE_FLAGS \
+ (GPIO_V2_LINE_FLAG_EDGE_RISING | \
+ GPIO_V2_LINE_FLAG_EDGE_FALLING)
+
#define GPIO_V2_LINE_VALID_FLAGS \
(GPIO_V2_LINE_FLAG_ACTIVE_LOW | \
GPIO_V2_LINE_DIRECTION_FLAGS | \
GPIO_V2_LINE_DRIVE_FLAGS | \
+ GPIO_V2_LINE_EDGE_FLAGS | \
GPIO_V2_LINE_BIAS_FLAGS)
static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
@@ -437,6 +593,21 @@ static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
return lc->flags;
}
+static bool gpio_v2_line_config_has_edge_detection(struct gpio_v2_line_config *lc)
+{
+ unsigned int i;
+
+ if (lc->flags & GPIO_V2_LINE_EDGE_FLAGS)
+ return true;
+
+ for (i = 0; i < lc->num_attrs; i--) {
+ if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_FLAGS) &&
+ (lc->attrs[i].attr.flags & GPIO_V2_LINE_EDGE_FLAGS))
+ return true;
+ }
+ return false;
+}
+
static int gpio_v2_line_config_output_value(struct gpio_v2_line_config *lc,
unsigned int line_idx)
{
@@ -465,6 +636,11 @@ static int gpio_v2_line_flags_validate(u64 flags)
(flags & GPIO_V2_LINE_FLAG_OUTPUT))
return -EINVAL;
+ /* Edge detection requires explicit input. */
+ if ((flags & GPIO_V2_LINE_EDGE_FLAGS) &&
+ !(flags & GPIO_V2_LINE_FLAG_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
@@ -526,6 +702,10 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 flags,
set_bit(FLAG_IS_OUT, flagsp);
else if (flags & GPIO_V2_LINE_FLAG_INPUT)
clear_bit(FLAG_IS_OUT, flagsp);
+ assign_bit(FLAG_EDGE_RISING, flagsp,
+ flags & GPIO_V2_LINE_FLAG_EDGE_RISING);
+ assign_bit(FLAG_EDGE_FALLING, flagsp,
+ flags & GPIO_V2_LINE_FLAG_EDGE_FALLING);
assign_bit(FLAG_OPEN_DRAIN, flagsp,
flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
assign_bit(FLAG_OPEN_SOURCE, flagsp,
@@ -612,14 +792,85 @@ static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
}
#endif
+static __poll_t linereq_poll(struct file *file,
+ struct poll_table_struct *wait)
+{
+ struct linereq *lr = file->private_data;
+ __poll_t events = 0;
+
+ poll_wait(file, &lr->wait, wait);
+
+ if (!kfifo_is_empty_spinlocked_noirqsave(&lr->events,
+ &lr->wait.lock))
+ events = EPOLLIN | EPOLLRDNORM;
+
+ return events;
+}
+
+static ssize_t linereq_read(struct file *file,
+ char __user *buf,
+ size_t count,
+ loff_t *f_ps)
+{
+ struct linereq *lr = file->private_data;
+ struct gpio_v2_line_event le;
+ ssize_t bytes_read = 0;
+ int ret;
+
+ if (count < sizeof(le))
+ return -EINVAL;
+
+ do {
+ spin_lock(&lr->wait.lock);
+ if (kfifo_is_empty(&lr->events)) {
+ if (bytes_read) {
+ spin_unlock(&lr->wait.lock);
+ return bytes_read;
+ }
+
+ if (file->f_flags & O_NONBLOCK) {
+ spin_unlock(&lr->wait.lock);
+ return -EAGAIN;
+ }
+
+ ret = wait_event_interruptible_locked(lr->wait,
+ !kfifo_is_empty(&lr->events));
+ if (ret) {
+ spin_unlock(&lr->wait.lock);
+ return ret;
+ }
+ }
+
+ ret = kfifo_out(&lr->events, &le, 1);
+ spin_unlock(&lr->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 linereq_free(struct linereq *lr)
{
unsigned int i;
for (i = 0; i < lr->num_lines; i++) {
+ edge_detector_stop(&lr->lines[i]);
if (lr->lines[i].desc)
gpiod_free(lr->lines[i].desc);
}
+ kfifo_free(&lr->events);
kfree(lr->label);
put_device(&lr->gdev->dev);
kfree(lr);
@@ -635,6 +886,8 @@ static int linereq_release(struct inode *inode, struct file *file)
static const struct file_operations line_fileops = {
.release = linereq_release,
+ .read = linereq_read,
+ .poll = linereq_poll,
.owner = THIS_MODULE,
.llseek = noop_llseek,
.unlocked_ioctl = linereq_ioctl,
@@ -650,7 +903,8 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
struct linereq *lr;
struct file *file;
unsigned long flags;
- unsigned int i;
+ unsigned int i, size;
+ bool has_edge_detection;
int fd, ret;
if (copy_from_user(&ulr, ip, sizeof(ulr)))
@@ -667,6 +921,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
if (ret)
return ret;
+ /* event_buffer_size is only valid with edge detection */
+ has_edge_detection = gpio_v2_line_config_has_edge_detection(lc);
+ if (ulr.event_buffer_size && !has_edge_detection)
+ return -EINVAL;
+
lr = kzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
if (!lr)
return -ENOMEM;
@@ -674,6 +933,9 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
lr->gdev = gdev;
get_device(&gdev->dev);
+ for (i = 0; i < ulr.num_lines; i++)
+ lr->lines[i].req = lr;
+
/* Make sure this is terminated */
ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
if (strlen(ulr.consumer)) {
@@ -684,6 +946,21 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
}
}
+ init_waitqueue_head(&lr->wait);
+ if (has_edge_detection) {
+ size = ulr.event_buffer_size;
+
+ if (size > GPIO_V2_LINES_MAX*16)
+ size = GPIO_V2_LINES_MAX*16;
+ else if (size == 0)
+ size = ulr.num_lines*16;
+
+ ret = kfifo_alloc(&lr->events, size, GFP_KERNEL);
+ if (ret)
+ goto out_free_linereq;
+ }
+
+ atomic_set(&lr->seqno, 0);
lr->num_lines = ulr.num_lines;
/* Request each GPIO */
@@ -722,6 +999,10 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
ret = gpiod_direction_input(desc);
if (ret)
goto out_free_linereq;
+ lr->lines[i].eflags = flags & GPIO_V2_LINE_EDGE_FLAGS;
+ ret = edge_detector_setup(&lr->lines[i], lc);
+ if (ret)
+ goto out_free_linereq;
}
blocking_notifier_call_chain(&desc->gdev->notifier,
@@ -1243,6 +1524,13 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
if (test_bit(FLAG_PULL_UP, &desc->flags))
info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
+ if (test_bit(FLAG_EDGE_RISING, &desc->flags))
+ info->flags |= GPIO_V2_LINE_FLAG_EDGE_RISING;
+ if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
+ info->flags |= GPIO_V2_LINE_FLAG_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.28.0
Add support for requesting lines using the GPIO_V2_GET_LINE_IOCTL, and
returning their current values using GPIO_V2_LINE_GET_VALUES_IOCTL.
The struct linereq implementation is based on the v1 struct linehandle
implementation.
Signed-off-by: Kent Gibson <[email protected]>
---
The linereq_ioctl() is a simple wrapper around linereq_get_values() here,
but will be extended with other ioctls in subsequent patches.
Similarly, the struct line only contains the desc here, but will receive
the edge detector and debouncer fields in subsequent patches.
Changes for v5:
- as per cover letter
Changes for v4:
- fix handling of mask in line_get_values
drivers/gpio/gpiolib-cdev.c | 418 ++++++++++++++++++++++++++++++++++++
1 file changed, 418 insertions(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 8b012879fe3f..1c0ed493cb83 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,396 @@ 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 requested line
+ * @desc: the GPIO descriptor for this line.
+ */
+struct line {
+ struct gpio_desc *desc;
+};
+
+/**
+ * struct linereq - contains the state of a userspace line request
+ * @gdev: the GPIO device the line request pertains to
+ * @label: consumer label used to tag GPIO descriptors
+ * @num_lines: the number of lines in the lines array
+ * @lines: the lines held by this line request, with @num_lines elements.
+ */
+struct linereq {
+ struct gpio_device *gdev;
+ const char *label;
+ u32 num_lines;
+ struct line lines[];
+};
+
+#define GPIO_V2_LINE_BIAS_FLAGS \
+ (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
+ GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
+ GPIO_V2_LINE_FLAG_BIAS_DISABLED)
+
+#define GPIO_V2_LINE_DIRECTION_FLAGS \
+ (GPIO_V2_LINE_FLAG_INPUT | \
+ GPIO_V2_LINE_FLAG_OUTPUT)
+
+#define GPIO_V2_LINE_DRIVE_FLAGS \
+ (GPIO_V2_LINE_FLAG_OPEN_DRAIN | \
+ GPIO_V2_LINE_FLAG_OPEN_SOURCE)
+
+#define GPIO_V2_LINE_VALID_FLAGS \
+ (GPIO_V2_LINE_FLAG_ACTIVE_LOW | \
+ GPIO_V2_LINE_DIRECTION_FLAGS | \
+ GPIO_V2_LINE_DRIVE_FLAGS | \
+ GPIO_V2_LINE_BIAS_FLAGS)
+
+static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
+ unsigned int line_idx)
+{
+ unsigned int i;
+ u64 mask = BIT_ULL(line_idx);
+
+ for (i = 0; i < lc->num_attrs; i++) {
+ if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_FLAGS) &&
+ (lc->attrs[i].mask & mask))
+ return lc->attrs[i].attr.flags;
+ }
+ return lc->flags;
+}
+
+static int gpio_v2_line_config_output_value(struct gpio_v2_line_config *lc,
+ unsigned int line_idx)
+{
+ unsigned int i;
+ u64 mask = BIT_ULL(line_idx);
+
+ for (i = 0; i < lc->num_attrs; i++) {
+ if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES) &&
+ (lc->attrs[i].mask & mask))
+ return !!(lc->attrs[i].attr.values & mask);
+ }
+ return 0;
+}
+
+static int gpio_v2_line_flags_validate(u64 flags)
+{
+ /* Return an error if an unknown flag is set */
+ if (flags & ~GPIO_V2_LINE_VALID_FLAGS)
+ return -EINVAL;
+
+ /*
+ * Do not allow both INPUT & OUTPUT flags to be set as they are
+ * contradictory.
+ */
+ if ((flags & GPIO_V2_LINE_FLAG_INPUT) &&
+ (flags & GPIO_V2_LINE_FLAG_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 & GPIO_V2_LINE_FLAG_OPEN_DRAIN) &&
+ (flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE))
+ return -EINVAL;
+
+ /* Drive requires explicit output direction. */
+ if ((flags & GPIO_V2_LINE_DRIVE_FLAGS) &&
+ !(flags & GPIO_V2_LINE_FLAG_OUTPUT))
+ return -EINVAL;
+
+ /* Bias requires explicit direction. */
+ if ((flags & GPIO_V2_LINE_BIAS_FLAGS) &&
+ !(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
+ return -EINVAL;
+
+ /* Only one bias flag can be set. */
+ if (((flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED) &&
+ (flags & (GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN |
+ GPIO_V2_LINE_FLAG_BIAS_PULL_UP))) ||
+ ((flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN) &&
+ (flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
+ unsigned int num_lines)
+{
+ unsigned int i;
+ u64 flags;
+ int ret;
+
+ if (lc->num_attrs > GPIO_V2_LINE_NUM_ATTRS_MAX)
+ return -EINVAL;
+
+ if (memchr_inv(lc->padding, 0, sizeof(lc->padding)))
+ return -EINVAL;
+
+ for (i = 0; i < num_lines; i++) {
+ flags = gpio_v2_line_config_flags(lc, i);
+ ret = gpio_v2_line_flags_validate(flags);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+
+static void gpio_v2_line_config_flags_to_desc_flags(u64 flags,
+ unsigned long *flagsp)
+{
+ assign_bit(FLAG_ACTIVE_LOW, flagsp,
+ flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
+ if (flags & GPIO_V2_LINE_FLAG_OUTPUT)
+ set_bit(FLAG_IS_OUT, flagsp);
+ else if (flags & GPIO_V2_LINE_FLAG_INPUT)
+ clear_bit(FLAG_IS_OUT, flagsp);
+ assign_bit(FLAG_OPEN_DRAIN, flagsp,
+ flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
+ assign_bit(FLAG_OPEN_SOURCE, flagsp,
+ flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
+ assign_bit(FLAG_PULL_UP, flagsp,
+ flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
+ assign_bit(FLAG_PULL_DOWN, flagsp,
+ flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
+ assign_bit(FLAG_BIAS_DISABLE, flagsp,
+ flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);
+}
+
+static long linereq_get_values(struct linereq *lr, void __user *ip)
+{
+ struct gpio_v2_line_values lv;
+ DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
+ struct gpio_desc **descs;
+ unsigned int i, didx, num_get;
+ int ret;
+
+ /* NOTE: It's ok to read values of output lines. */
+ if (copy_from_user(&lv, ip, sizeof(lv)))
+ return -EFAULT;
+
+ for (num_get = 0, i = 0; i < lr->num_lines; i++) {
+ if (lv.mask & BIT_ULL(i)) {
+ num_get++;
+ descs = &lr->lines[i].desc;
+ }
+ }
+
+ if (num_get == 0)
+ return -EINVAL;
+
+ if (num_get != 1) {
+ descs = kmalloc_array(num_get, sizeof(*descs), GFP_KERNEL);
+ for (didx = 0, i = 0; i < lr->num_lines; i++) {
+ if (lv.mask & BIT_ULL(i)) {
+ descs[didx] = lr->lines[i].desc;
+ didx++;
+ }
+ }
+ }
+ ret = gpiod_get_array_value_complex(false, true, num_get,
+ descs, NULL, vals);
+
+ if (num_get != 1)
+ kfree(descs);
+ if (ret)
+ return ret;
+
+ lv.bits = 0;
+ for (didx = 0, i = 0; i < lr->num_lines; i++) {
+ if (lv.mask & BIT_ULL(i)) {
+ if (test_bit(didx, vals))
+ lv.bits |= BIT_ULL(i);
+ didx++;
+ }
+ }
+
+ if (copy_to_user(ip, &lv, sizeof(lv)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static long linereq_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct linereq *lr = file->private_data;
+ void __user *ip = (void __user *)arg;
+
+ if (cmd == GPIO_V2_LINE_GET_VALUES_IOCTL)
+ return linereq_get_values(lr, ip);
+
+ return -EINVAL;
+}
+
+#ifdef CONFIG_COMPAT
+static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return linereq_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+static void linereq_free(struct linereq *lr)
+{
+ unsigned int i;
+
+ for (i = 0; i < lr->num_lines; i++) {
+ if (lr->lines[i].desc)
+ gpiod_free(lr->lines[i].desc);
+ }
+ kfree(lr->label);
+ put_device(&lr->gdev->dev);
+ kfree(lr);
+}
+
+static int linereq_release(struct inode *inode, struct file *file)
+{
+ struct linereq *lr = file->private_data;
+
+ linereq_free(lr);
+ return 0;
+}
+
+static const struct file_operations line_fileops = {
+ .release = linereq_release,
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = linereq_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = linereq_ioctl_compat,
+#endif
+};
+
+static int linereq_create(struct gpio_device *gdev, void __user *ip)
+{
+ struct gpio_v2_line_request ulr;
+ struct gpio_v2_line_config *lc;
+ struct linereq *lr;
+ struct file *file;
+ unsigned long flags;
+ unsigned int i;
+ int fd, ret;
+
+ if (copy_from_user(&ulr, ip, sizeof(ulr)))
+ return -EFAULT;
+
+ if ((ulr.num_lines == 0) || (ulr.num_lines > GPIO_V2_LINES_MAX))
+ return -EINVAL;
+
+ if (memchr_inv(ulr.padding, 0, sizeof(ulr.padding)))
+ return -EINVAL;
+
+ lc = &ulr.config;
+ ret = gpio_v2_line_config_validate(lc, ulr.num_lines);
+ if (ret)
+ return ret;
+
+ lr = kzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
+ if (!lr)
+ return -ENOMEM;
+
+ lr->gdev = gdev;
+ get_device(&gdev->dev);
+
+ /* Make sure this is terminated */
+ ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
+ if (strlen(ulr.consumer)) {
+ lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
+ if (!lr->label) {
+ ret = -ENOMEM;
+ goto out_free_linereq;
+ }
+ }
+
+ lr->num_lines = ulr.num_lines;
+
+ /* Request each GPIO */
+ for (i = 0; i < ulr.num_lines; i++) {
+ u32 offset = ulr.offsets[i];
+ struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
+
+ if (IS_ERR(desc)) {
+ ret = PTR_ERR(desc);
+ goto out_free_linereq;
+ }
+
+ ret = gpiod_request(desc, lr->label);
+ if (ret)
+ goto out_free_linereq;
+
+ lr->lines[i].desc = desc;
+ flags = gpio_v2_line_config_flags(lc, i);
+ gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
+
+ ret = gpiod_set_transitory(desc, false);
+ if (ret < 0)
+ goto out_free_linereq;
+
+ /*
+ * Lines have to be requested explicitly for input
+ * or output, else the line will be treated "as is".
+ */
+ if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
+ int val = gpio_v2_line_config_output_value(lc, i);
+
+ ret = gpiod_direction_output(desc, val);
+ if (ret)
+ goto out_free_linereq;
+ } else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
+ ret = gpiod_direction_input(desc);
+ if (ret)
+ goto out_free_linereq;
+ }
+
+ 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_linereq;
+ }
+
+ file = anon_inode_getfile("gpio-line", &line_fileops, lr,
+ O_RDONLY | O_CLOEXEC);
+ if (IS_ERR(file)) {
+ ret = PTR_ERR(file);
+ goto out_put_unused_fd;
+ }
+
+ ulr.fd = fd;
+ if (copy_to_user(ip, &ulr, sizeof(ulr))) {
+ /*
+ * 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",
+ lr->num_lines);
+
+ return 0;
+
+out_put_unused_fd:
+ put_unused_fd(fd);
+out_free_linereq:
+ linereq_free(lr);
+ return ret;
+}
+
+#ifdef CONFIG_GPIO_CDEV_V1
/*
* GPIO line event management
@@ -745,6 +1138,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)
{
@@ -842,6 +1237,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;
@@ -884,6 +1280,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_V2_GET_LINE_IOCTL) {
+ return linereq_create(gdev, ip);
} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
if (copy_from_user(&offset, ip, sizeof(offset)))
return -EFAULT;
@@ -1104,6 +1503,25 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
MAJOR(devt), gdev->id);
return 0;
+ /*
+ * array sizes must ensure 64-bit alignment and not create holes in
+ * the struct packing.
+ */
+ BUILD_BUG_ON(IS_ALIGNED(GPIO_V2_LINES_MAX, 2));
+ BUILD_BUG_ON(IS_ALIGNED(GPIO_MAX_NAME_SIZE, 8));
+
+ /*
+ * check that uAPI structs are 64-bit aligned for 32/64-bit
+ * compatibility
+ */
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_attribute), 8));
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_config_attribute), 8));
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_config), 8));
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_request), 8));
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_info), 8));
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_info_changed), 8));
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_event), 8));
+ BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_values), 8));
}
void gpiolib_cdev_unregister(struct gpio_device *gdev)
--
2.28.0
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 <[email protected]>
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/Kconfig | 17 +++++++++++++++--
drivers/gpio/Makefile | 2 +-
drivers/gpio/gpiolib-cdev.h | 15 +++++++++++++++
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8030fd91a3cc..8e409b9c33dc 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -66,8 +66,21 @@ 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
+ prompt "Character device (/dev/gpiochipN) support" if EXPERT
+ 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 if 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.28.0
On Thu, Aug 27, 2020 at 4:00 PM Kent Gibson <[email protected]> wrote:
> This patchset defines and implements a new version of the
> GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add
> support for debounce, event sequence numbers, and allow 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 three blocks; the first two patches
> are minor fixes that impact later patches, the next eleven contain the
> v2 uAPI definition and 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.
I'm ready to queue this now. Certainly any remaining snags can be
fixed in-tree.
It kind of keeps in tradition with proper software projects "plan to
throw one away" which is what we have traditionally done several
times: the first Bluetooh framework was tossed, JFFS was tossed
for JFFS2, Video4Linux was tossed for V4L2. So let's do this.
Anyone against? I will put it on an immutable branch and then merge
that in for devel.
Yours,
Linus Walleij
On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <[email protected]> wrote:
>
> On Thu, Aug 27, 2020 at 4:00 PM Kent Gibson <[email protected]> wrote:
>
> > This patchset defines and implements a new version of the
> > GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add
> > support for debounce, event sequence numbers, and allow 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 three blocks; the first two patches
> > are minor fixes that impact later patches, the next eleven contain the
> > v2 uAPI definition and 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.
>
> I'm ready to queue this now. Certainly any remaining snags can be
> fixed in-tree.
>
> It kind of keeps in tradition with proper software projects "plan to
> throw one away" which is what we have traditionally done several
> times: the first Bluetooh framework was tossed, JFFS was tossed
> for JFFS2, Video4Linux was tossed for V4L2. So let's do this.
>
> Anyone against? I will put it on an immutable branch and then merge
> that in for devel.
>
Hi Linus,
please hold it maybe for one more week - I'd love to have some more
people take a look at the user facing header at least. Andy is usually
very thorough in his reviews so I'm Ccing him here.
I'll too skim through the series one more time.
Bart
On Thu, Aug 27, 2020 at 06:02:03PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <[email protected]> wrote:
> >
> > On Thu, Aug 27, 2020 at 4:00 PM Kent Gibson <[email protected]> wrote:
> >
> > > This patchset defines and implements a new version of the
> > > GPIO CDEV uAPI to address existing 32/64-bit alignment issues, add
> > > support for debounce, event sequence numbers, and allow 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 three blocks; the first two patches
> > > are minor fixes that impact later patches, the next eleven contain the
> > > v2 uAPI definition and 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.
> >
> > I'm ready to queue this now. Certainly any remaining snags can be
> > fixed in-tree.
> >
> > It kind of keeps in tradition with proper software projects "plan to
> > throw one away" which is what we have traditionally done several
> > times: the first Bluetooh framework was tossed, JFFS was tossed
> > for JFFS2, Video4Linux was tossed for V4L2. So let's do this.
> >
> > Anyone against? I will put it on an immutable branch and then merge
> > that in for devel.
> >
>
> Hi Linus,
>
> please hold it maybe for one more week - I'd love to have some more
> people take a look at the user facing header at least. Andy is usually
> very thorough in his reviews so I'm Ccing him here.
>
> I'll too skim through the series one more time.
>
I'd be happier with more eyeballs over it before it goes in as well.
I'm also wondering if it is worthwhile dropping the restriction on
changing edge detection configuration, that is present in
gpio_v2_line_config_change_validate() in patch 10, so long as userspace
is made aware that changing it may result in lost interrupts as we
may need to release and re-request the interrupt to effect the change.
Similarly changing debounce while edge detection is enabled, that is
disallowed in patch 12.
My policy when writing the implementation was to disallow any operation
that may have unanticiapted side-effects, and forcing userspace to
release and re-request the line(s), but that may be overly restrictive?
The particular use case I am considering is one I had been asked about -
changing a requested line from input with edge detection to output, and
vice versa. Losing interrupts isn't really an issue for this use case -
it is expected. Yet the current implementation requires a re-request.
Cheers,
Kent.
On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <[email protected]> wrote:
> The particular use case I am considering is one I had been asked about -
> changing a requested line from input with edge detection to output, and
> vice versa. Losing interrupts isn't really an issue for this use case -
> it is expected. Yet the current implementation requires a re-request.
This is possible to do for in-kernel users, but I don't know if that makes
sense for userspace. It is for one-offs and prototyping after all, there
is no need (IMO) to make it overly convenient for users to implement
all kind of weirdness in userspace unless there is a very real use case.
Yours,
Linus Walleij
On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote:
> On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <[email protected]> wrote:
>
> > The particular use case I am considering is one I had been asked about -
> > changing a requested line from input with edge detection to output, and
> > vice versa. Losing interrupts isn't really an issue for this use case -
> > it is expected. Yet the current implementation requires a re-request.
>
> This is possible to do for in-kernel users, but I don't know if that makes
> sense for userspace. It is for one-offs and prototyping after all, there
> is no need (IMO) to make it overly convenient for users to implement
> all kind of weirdness in userspace unless there is a very real use case.
>
Fair point - in fact it is the same one that made me reconsider why I
was so concerned about potentially losing an edge event in a few rare
corner cases.
Another point for this change are that it actually simplifies the kernel
code, as it takes as much code to detect and filter these cases as it
does to include them in the normal flow.
I had a play with it yesterday and the change removes two whole
functions, gpio_v2_line_config_change_validate() and
gpio_v2_line_config_has_edge_detection() at the expense of making
debounce_update() a little more complicated. I'm happy to put together a
v6 that incorporates those changes if there aren't any strenuous
objections - we can always revert to v5. Or I could mail the couple of
patches I've made and if they seem reasonable then I could merge them
into this set?
Cheers,
Kent.
On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <[email protected]> wrote:
>
> On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote:
> > On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <[email protected]> wrote:
> >
> > > The particular use case I am considering is one I had been asked about -
> > > changing a requested line from input with edge detection to output, and
> > > vice versa. Losing interrupts isn't really an issue for this use case -
> > > it is expected. Yet the current implementation requires a re-request.
> >
> > This is possible to do for in-kernel users, but I don't know if that makes
> > sense for userspace. It is for one-offs and prototyping after all, there
> > is no need (IMO) to make it overly convenient for users to implement
> > all kind of weirdness in userspace unless there is a very real use case.
> >
>
> Fair point - in fact it is the same one that made me reconsider why I
> was so concerned about potentially losing an edge event in a few rare
> corner cases.
>
> Another point for this change are that it actually simplifies the kernel
> code, as it takes as much code to detect and filter these cases as it
> does to include them in the normal flow.
>
> I had a play with it yesterday and the change removes two whole
> functions, gpio_v2_line_config_change_validate() and
> gpio_v2_line_config_has_edge_detection() at the expense of making
> debounce_update() a little more complicated. I'm happy to put together a
> v6 that incorporates those changes if there aren't any strenuous
> objections - we can always revert to v5. Or I could mail the couple of
> patches I've made and if they seem reasonable then I could merge them
> into this set?
>
> Cheers,
> Kent.
I personally like v6 more. The code is more elegant and we've also
tried limiting GPIO chardev features before and now we're doing v2 so
let's not make the same mistake twice. :)
I'll try to review v6 in detail later today.
Bartosz
On Thu, Sep 10, 2020 at 1:10 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Aug 27, 2020 at 06:02:03PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <[email protected]> wrote:
>
> > please hold it maybe for one more week - I'd love to have some more
> > people take a look at the user facing header at least. Andy is usually
> > very thorough in his reviews so I'm Ccing him here.
> >
> > I'll too skim through the series one more time.
>
> Thank you, Bart.
>
> I think you, guys know how to do that best. Unfortunately I was almost squeezed
> under pile of several tasks and didn't find time for this.
>
If we knew, we wouldn't be needing a v2. :)
> Meanwhile I have sent an updated fix for v1 as suggested by Arnd. It works for
> me.
>
Cool, I'll take a look at that.
Bart
On Tue, Sep 01, 2020 at 11:28:13AM +0200, Bartosz Golaszewski wrote:
> On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <[email protected]> wrote:
> >
> > On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote:
> > > On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <[email protected]> wrote:
> > >
> > > > The particular use case I am considering is one I had been asked about -
> > > > changing a requested line from input with edge detection to output, and
> > > > vice versa. Losing interrupts isn't really an issue for this use case -
> > > > it is expected. Yet the current implementation requires a re-request.
> > >
> > > This is possible to do for in-kernel users, but I don't know if that makes
> > > sense for userspace. It is for one-offs and prototyping after all, there
> > > is no need (IMO) to make it overly convenient for users to implement
> > > all kind of weirdness in userspace unless there is a very real use case.
> > >
> >
> > Fair point - in fact it is the same one that made me reconsider why I
> > was so concerned about potentially losing an edge event in a few rare
> > corner cases.
> >
> > Another point for this change are that it actually simplifies the kernel
> > code, as it takes as much code to detect and filter these cases as it
> > does to include them in the normal flow.
> >
> > I had a play with it yesterday and the change removes two whole
> > functions, gpio_v2_line_config_change_validate() and
> > gpio_v2_line_config_has_edge_detection() at the expense of making
> > debounce_update() a little more complicated. I'm happy to put together a
> > v6 that incorporates those changes if there aren't any strenuous
> > objections - we can always revert to v5. Or I could mail the couple of
> > patches I've made and if they seem reasonable then I could merge them
> > into this set?
> >
> > Cheers,
> > Kent.
>
> I personally like v6 more. The code is more elegant and we've also
> tried limiting GPIO chardev features before and now we're doing v2 so
> let's not make the same mistake twice. :)
>
> I'll try to review v6 in detail later today.
Let me briefly review to this. Can you remind which patch has a top level
description of what features are provided in comparison to uAPI v1?
(Btw, do we have some kind of comparison table?)
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 10, 2020 at 04:12:08PM +0200, Bartosz Golaszewski wrote:
> On Thu, Sep 10, 2020 at 4:09 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Sep 01, 2020 at 11:28:13AM +0200, Bartosz Golaszewski wrote:
> > > On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <[email protected]> wrote:
...
> > > I personally like v6 more. The code is more elegant and we've also
> > > tried limiting GPIO chardev features before and now we're doing v2 so
> > > let's not make the same mistake twice. :)
> > >
> > > I'll try to review v6 in detail later today.
> >
> > Let me briefly review to this. Can you remind which patch has a top level
> > description of what features are provided in comparison to uAPI v1?
> > (Btw, do we have some kind of comparison table?)
>
> We are now at v8 for this series. The cover letter contains a lot of
> info and patch 4/20 defining the uAPI header explains v2 even more. I
> think these are the most important parts. Any implementation details
> can be fixed later as opposed to the API itself.
Right, thanks for pointers!
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 10, 2020 at 4:09 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Sep 01, 2020 at 11:28:13AM +0200, Bartosz Golaszewski wrote:
> > On Sat, Aug 29, 2020 at 3:35 AM Kent Gibson <[email protected]> wrote:
> > >
> > > On Fri, Aug 28, 2020 at 04:37:19PM +0200, Linus Walleij wrote:
> > > > On Fri, Aug 28, 2020 at 12:47 AM Kent Gibson <[email protected]> wrote:
> > > >
> > > > > The particular use case I am considering is one I had been asked about -
> > > > > changing a requested line from input with edge detection to output, and
> > > > > vice versa. Losing interrupts isn't really an issue for this use case -
> > > > > it is expected. Yet the current implementation requires a re-request.
> > > >
> > > > This is possible to do for in-kernel users, but I don't know if that makes
> > > > sense for userspace. It is for one-offs and prototyping after all, there
> > > > is no need (IMO) to make it overly convenient for users to implement
> > > > all kind of weirdness in userspace unless there is a very real use case.
> > > >
> > >
> > > Fair point - in fact it is the same one that made me reconsider why I
> > > was so concerned about potentially losing an edge event in a few rare
> > > corner cases.
> > >
> > > Another point for this change are that it actually simplifies the kernel
> > > code, as it takes as much code to detect and filter these cases as it
> > > does to include them in the normal flow.
> > >
> > > I had a play with it yesterday and the change removes two whole
> > > functions, gpio_v2_line_config_change_validate() and
> > > gpio_v2_line_config_has_edge_detection() at the expense of making
> > > debounce_update() a little more complicated. I'm happy to put together a
> > > v6 that incorporates those changes if there aren't any strenuous
> > > objections - we can always revert to v5. Or I could mail the couple of
> > > patches I've made and if they seem reasonable then I could merge them
> > > into this set?
> > >
> > > Cheers,
> > > Kent.
> >
> > I personally like v6 more. The code is more elegant and we've also
> > tried limiting GPIO chardev features before and now we're doing v2 so
> > let's not make the same mistake twice. :)
> >
> > I'll try to review v6 in detail later today.
>
> Let me briefly review to this. Can you remind which patch has a top level
> description of what features are provided in comparison to uAPI v1?
> (Btw, do we have some kind of comparison table?)
We are now at v8 for this series. The cover letter contains a lot of
info and patch 4/20 defining the uAPI header explains v2 even more. I
think these are the most important parts. Any implementation details
can be fixed later as opposed to the API itself.
Bart
On Thu, Aug 27, 2020 at 06:02:03PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 27, 2020 at 5:53 PM Linus Walleij <[email protected]> wrote:
> please hold it maybe for one more week - I'd love to have some more
> people take a look at the user facing header at least. Andy is usually
> very thorough in his reviews so I'm Ccing him here.
>
> I'll too skim through the series one more time.
Thank you, Bart.
I think you, guys know how to do that best. Unfortunately I was almost squeezed
under pile of several tasks and didn't find time for this.
Meanwhile I have sent an updated fix for v1 as suggested by Arnd. It works for
me.
--
With Best Regards,
Andy Shevchenko