[RFC 0/4] iio-input-bridge so that accelerometers which only have an iio driver can still present evdev input events
Some user spaces (e.g. some Android) use /dev/input/event* for handling the 3D position of the device with respect to the center of gravity (earth). This can be used for gaming input, rotation of screens etc.
This should be the standard because this interface is an abstraction of how this data is acquired from sensor chips. Sensor chips may be connected through different interfaces and in different positions. They may also have different parameters. And, if a chip is replaced by a different one, the values reported by the device position interface should remain the same.
But nowadays, new accelerometer chips usually just get iio drivers and rarely an evdev input driver.
Therefore we need something like a protocol stack: input device vs. raw data. It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or keyboard input events vs. raw gpio or raw USB access.
This patch set bridges the gap between raw iio data and the input device abstraction so that accelerometer measurements can also be presented as X/Y/Z accelerometer channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
H. Nikolaus Schaller (4): iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input iio: input-bridge: add iio-input-bridge to Makefile iio: input-bridge: add IIO_INPUT_BRIDGE kernel config option iio: input-bridge: make the iio-input-bridge driver called by iio-core
drivers/iio/Kconfig | 7 + drivers/iio/Makefile | 1 + drivers/iio/industrialio-core.c | 12 + drivers/iio/industrialio-inputbridge.c | 420 +++++++++++++++++++++++++ drivers/iio/industrialio-inputbridge.h | 28 ++ 5 files changed, 468 insertions(+) create mode 100644 drivers/iio/industrialio-inputbridge.c create mode 100644 drivers/iio/industrialio-inputbridge.h
Some user spaces (e.g. some Android devices) use /dev/input/event* for handling the 3D position of the device with respect to the center of gravity (earth). This can be used for gaming input, auto-rotation of screens etc.
This interface should be the standard because it is an abstraction of how orientation data is acquired from sensor chips. Sensor chips may be connected through different interfaces and in different positions. They may also have different parameters. And, if a chip is replaced by a different one, the values reported by the device position interface should remain the same, provided the device tree reflects the changed chip.
But nowadays, new accelerometer chips mostly get iio drivers and rarely input drivers. Therefore we need something like a protocol stack: input device vs. raw data. It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or keyboard input events vs. raw gpio or raw USB access.
This patch bridges the gap between raw iio data and the input device abstraction so that accelerometer measurements can also be presented as X/Y/Z accelerometer channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
There are no special requirements or changes needed for an iio driver.
This driver simply collects the first 3 accelerometer channels as X, Y and Z. If only 1 or 2 channels are available, they are used for X and Y only. Additional channels are ignored.
Scaling is done automatically so that 1g is represented by value 256 and range is assumed to be -511 .. +511.
If a mount-matrix is provided by the iio driver, it is also taken into account so that the input event automatically gets the correct orientation with respect to the device.
If this extension is not configured into the kernel it takes no resources (except source code).
If it is configured, but there is no accelerometer, there is only a tiny penalty for scanning for accelerometer channels once during probe of each iio device.
If it runs, the driver polls the device(s) once every 100 ms. A mode where the iio device defines the update rate is not implemented and for further study.
The driver is capable of supporting up to 5 iio accelerometers and they are presented by unique /dev/input/event* files. The iio chip name is used to define the input device name so that it can be identified (e.g. by udev rules or evtest).
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/iio/industrialio-inputbridge.c | 420 +++++++++++++++++++++++++ drivers/iio/industrialio-inputbridge.h | 28 ++ 2 files changed, 448 insertions(+) create mode 100644 drivers/iio/industrialio-inputbridge.c create mode 100644 drivers/iio/industrialio-inputbridge.h
diff --git a/drivers/iio/industrialio-inputbridge.c b/drivers/iio/industrialio-inputbridge.c new file mode 100644 index 000000000000..0288ba2ef69f --- /dev/null +++ b/drivers/iio/industrialio-inputbridge.c @@ -0,0 +1,420 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * The Industrial I/O core, bridge to input devices + * + * Copyright (c) 2016-2019 Golden Delicious Computers GmbH&Co. KG + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include <linux/iio/consumer.h> +#include <linux/iio/iio.h> +#include <linux/iio/types.h> +#include <linux/input.h> +#include <linux/module.h> +#include <linux/slab.h> + +#include "industrialio-inputbridge.h" + +/* currently, only polling is implemented */ +#define POLLING_MSEC 100 + +/* handle up to this number of input devices */ +#define MAX_INPUT_DEVICES 5 + +#define FIXED_POINT_UNIT 1000 + +static struct iio_input_map { + struct iio_dev *indio_dev; /* the iio device */ + struct input_dev *input; /* the input device */ + struct iio_channel channels[3]; /* x, y, z channels */ + int values[3]; /* values while processing */ + struct matrix { + int mxx, myx, mzx; /* translated and scaled mount-matrix */ + int mxy, myy, mzy; + int mxz, myz, mzz; + } matrix; +} devices[MAX_INPUT_DEVICES]; + +static inline struct iio_input_map *to_iio_input_map( + struct iio_channel *channel) +{ + return (struct iio_input_map *) channel->data; +} + +/* we must protect against races in channel allocation */ + +static DEFINE_MUTEX(inputbridge_device_mutex); + +static struct delayed_work input_work; + +/* we can start/stop the worker by open("/dev/input/event") */ +static int open_count; + +/* we must protect the open counter */ +static DEFINE_MUTEX(inputbridge_open_mutex); + +/* minimum and maximum range we want to report */ +#define ABSMAX_ACC_VAL (512 - 1) +#define ABSMIN_ACC_VAL -(ABSMAX_ACC_VAL) + +/* scale processed iio values so that 1g maps to ABSMAX_ACC_VAL / 2 */ +#define SCALE ((100 * ABSMAX_ACC_VAL) / (2 * 981)) + +/* + * convert float string to scaled fixed point format, e.g. + * 1.23 -> 1230 + * 0.1234 -> 123 + * .01234 -> 12 + */ + +static int32_t atofix(const char *str) +{ + int32_t mant = 0; + bool sign = false; + bool decimal = false; + int32_t divisor = 1; + + if (*str == '-') + sign = true, str++; + while (*str && divisor < FIXED_POINT_UNIT) { + if (*str >= '0' && *str <= '9') { + mant = 10 * mant + (*str - '0'); + if (decimal) + divisor *= 10; + } else if (*str == '.') + decimal = true; + else + return 0; /* error */ + str++; + } + + mant = (FIXED_POINT_UNIT * mant) / divisor; + if (sign) + mant = -mant; + + return mant; +} + +static void iio_apply_matrix(struct matrix *m, int *in, int *out) +{ + /* apply mount matrix */ + out[0] = (m->mxx * in[0] + m->myx * in[1] + m->mzx * in[2]) + / FIXED_POINT_UNIT; + out[1] = (m->mxy * in[0] + m->myy * in[1] + m->mzy * in[2]) + / FIXED_POINT_UNIT; + out[2] = (m->mxz * in[0] + m->myz * in[1] + m->mzz * in[2]) + / FIXED_POINT_UNIT; +} + +static void iio_accel_report_channels(void) +{ + int dindex; + + for (dindex = 0; dindex < ARRAY_SIZE(devices); dindex++) { + struct iio_input_map *map = &devices[dindex]; + + /* device might become closed while we are still processing */ + mutex_lock(&inputbridge_device_mutex); + + if (map->input) { + int aligned_values[3]; + int cindex = 0; + + while (cindex < ARRAY_SIZE(map->channels)) { + struct iio_channel *channel = + &map->channels[cindex]; + int val; + int ret; + + if (!channel->indio_dev) + continue; + + ret = iio_read_channel_raw(channel, &val); + + if (ret < 0) { + pr_err("%s(): channel read error %d\n", + __func__, cindex); + return; + } + + ret = iio_convert_raw_to_processed(channel, val, + &map->values[cindex], SCALE); + + if (ret < 0) { + pr_err("%s(): channel processing error\n", + __func__); + return; + } + + cindex++; + } + + iio_apply_matrix(&map->matrix, map->values, + aligned_values); + + input_report_abs(map->input, ABS_X, aligned_values[0]); + input_report_abs(map->input, ABS_Y, aligned_values[1]); + input_report_abs(map->input, ABS_Z, aligned_values[2]); + input_sync(map->input); + } + + mutex_unlock(&inputbridge_device_mutex); + } +} + +static void iio_inputbridge_work(struct work_struct *work) +{ + struct delayed_work *delayed_work; + + delayed_work = to_delayed_work(work); + + mutex_lock(&inputbridge_open_mutex); + + iio_accel_report_channels(); + + schedule_delayed_work(delayed_work, + msecs_to_jiffies(POLLING_MSEC)); + + mutex_unlock(&inputbridge_open_mutex); +} + +static int iio_accel_open(struct input_dev *input) +{ + struct iio_dev *iiodev = input_get_drvdata(input); + + mutex_lock(&inputbridge_open_mutex); + + /* start on first open */ + if (open_count++ == 0) + schedule_delayed_work(&input_work, + msecs_to_jiffies(0)); + + mutex_unlock(&inputbridge_open_mutex); + + return 0; +} + +static void iio_accel_close(struct input_dev *input) +{ + struct iio_dev *iiodev = input_get_drvdata(input); + + mutex_lock(&inputbridge_open_mutex); + + /* stop after last close */ + if (--open_count == 0) + cancel_delayed_work(&input_work); + + mutex_unlock(&inputbridge_open_mutex); +} + +static int iio_input_register_accel_channel(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ /* we found some accelerometer channel */ + int ret; + + int dindex, cindex; + struct iio_input_map *map; + + mutex_lock(&inputbridge_device_mutex); + + /* look for existing input device for this iio device */ + + for (dindex = 0; dindex < ARRAY_SIZE(devices); dindex++) { + if (devices[dindex].indio_dev == indio_dev) + break; + } + + if (dindex == ARRAY_SIZE(devices)) { + struct input_dev *input; + const struct iio_chan_spec_ext_info *ext_info; + + /* not found, look for a free slot for a new input device */ + + for (dindex = 0; dindex < ARRAY_SIZE(devices); dindex++) { + if (!devices[dindex].input) + break; + } + + if (dindex == ARRAY_SIZE(devices)) { + mutex_unlock(&inputbridge_device_mutex); + return -ENOMEM; + } + + input = input_allocate_device(); + + if (!input) { + mutex_unlock(&inputbridge_device_mutex); + return -ENOMEM; + } + + input->name = kasprintf(GFP_KERNEL, "iio-bridge: %s", + indio_dev->name); + input->phys = kasprintf(GFP_KERNEL, "accel/input%d", + dindex); + +// input->id.bustype = BUS_I2C; +// input->dev.parent = &indio_dev->client->dev; + + set_bit(INPUT_PROP_ACCELEROMETER, input->propbit); + + input->evbit[0] = BIT_MASK(EV_ABS); + input->open = iio_accel_open; + input->close = iio_accel_close; + +// FIXME: what happens if we unregister the first device? +// and then register another one? + + if (dindex == 0) // first input + INIT_DELAYED_WORK(&input_work, iio_inputbridge_work); + + input_set_drvdata(input, indio_dev); + + input_alloc_absinfo(input); + ret = input_register_device(input); + + if (ret < 0) { + kfree(input->name); + kfree(input->phys); + input_free_device(input); + mutex_unlock(&inputbridge_device_mutex); + return ret; + } + + map = &devices[dindex]; + + map->input = input; + map->indio_dev = indio_dev; + + /* assume all channels of a device share the same matrix */ + + ext_info = chan->ext_info; + for (; ext_info && ext_info->name; ext_info++) { + if (strcmp(ext_info->name, "mount_matrix") == 0) + break; + } + + if (ext_info && ext_info->name) { + /* matrix found */ + uintptr_t priv = ext_info->private; + const struct iio_mount_matrix *mtx; + + mtx = ((iio_get_mount_matrix_t *) priv)(indio_dev, + chan); + + map->matrix.mxx = atofix(mtx->rotation[0]); + map->matrix.myx = atofix(mtx->rotation[1]); + map->matrix.mzx = atofix(mtx->rotation[2]); + map->matrix.mxy = atofix(mtx->rotation[3]); + map->matrix.myy = atofix(mtx->rotation[4]); + map->matrix.mzy = atofix(mtx->rotation[5]); + map->matrix.mxz = atofix(mtx->rotation[6]); + map->matrix.myz = atofix(mtx->rotation[7]); + map->matrix.mzz = atofix(mtx->rotation[8]); + } else { + map->matrix.mxx = FIXED_POINT_UNIT; + map->matrix.myx = 0; + map->matrix.mzx = 0; + map->matrix.mxy = 0; + map->matrix.myy = FIXED_POINT_UNIT; + map->matrix.mzy = 0; + map->matrix.mxz = 0; + map->matrix.myz = 0; + map->matrix.mzz = FIXED_POINT_UNIT; + } + } + + /* find free channel within this device block */ + map = &devices[dindex]; + + for (cindex = 0; cindex < ARRAY_SIZE(map->channels); cindex++) { + if (!map->channels[cindex].indio_dev) + break; + } + + /* check if we already have collected enough channels */ + if (cindex == ARRAY_SIZE(map->channels)) { + mutex_unlock(&inputbridge_device_mutex); + return 0; /* silently ignore */ + } + + map->channels[cindex].indio_dev = indio_dev; + map->channels[cindex].channel = chan; + map->channels[cindex].data = (void *) &devices[dindex]; + + switch (cindex) { + case 0: + input_set_abs_params(map->input, ABS_X, ABSMIN_ACC_VAL, + ABSMAX_ACC_VAL, 0, 0); + break; + case 1: + input_set_abs_params(map->input, ABS_Y, ABSMIN_ACC_VAL, + ABSMAX_ACC_VAL, 0, 0); + break; + case 2: + input_set_abs_params(map->input, ABS_Z, ABSMIN_ACC_VAL, + ABSMAX_ACC_VAL, 0, 0); + break; + } + mutex_unlock(&inputbridge_device_mutex); + + return 0; +} + +int iio_device_register_inputbridge(struct iio_dev *indio_dev) +{ + int i; + + for (i = 0; i < indio_dev->num_channels; i++) { + const struct iio_chan_spec *chan = + &indio_dev->channels[i]; + + if (chan->type == IIO_ACCEL) { + int r = iio_input_register_accel_channel(indio_dev, + chan); + + if (r < 0) + return r; + } + } + + return 0; +} + +void iio_device_unregister_inputbridge(struct iio_dev *indio_dev) +{ + struct input_dev *input = NULL; + + int dindex = 0; + + mutex_lock(&inputbridge_device_mutex); + + for (; dindex < ARRAY_SIZE(devices); dindex++) { + int cindex = 0; + + while (cindex < ARRAY_SIZE(devices[dindex].channels)) { + struct iio_channel *channel = + &devices[dindex].channels[cindex]; + + /* mark slot as empty */ + if (channel->indio_dev == indio_dev) + channel->indio_dev = NULL; + cindex++; + } + input_unregister_device(input); + kfree(input->name); + kfree(input->phys); + input_free_device(input); + + devices[dindex].indio_dev = NULL; + devices[dindex].input = NULL; + } + + mutex_unlock(&inputbridge_device_mutex); +} + +MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("Bridge to present Industrial I/O accelerometers as properly oriented Input devices"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/iio/industrialio-inputbridge.h b/drivers/iio/industrialio-inputbridge.h new file mode 100644 index 000000000000..1363b10ab3f7 --- /dev/null +++ b/drivers/iio/industrialio-inputbridge.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * The Industrial I/O core, bridge to input devices + * + * Copyright (c) 2016-2019 Golden Delicious Computers GmbH&Co. KG + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#if defined(CONFIG_IIO_INPUT_BRIDGE) + +extern int iio_device_register_inputbridge(struct iio_dev *indio_dev); +extern void iio_device_unregister_inputbridge(struct iio_dev *indio_dev); + +#else + +static inline int iio_device_register_inputbridge(struct iio_dev *indio_dev) +{ + return 0; +} + +static inline void iio_device_unregister_inputbridge(struct iio_dev *indio_dev) +{ +} + +#endif
Add the iio-input-bridge to the Makefile.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/iio/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile index cb5993251381..d695e5a27da5 100644 --- a/drivers/iio/Makefile +++ b/drivers/iio/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_IIO) += industrialio.o industrialio-y := industrialio-core.o industrialio-event.o inkern.o industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o +industrialio-$(CONFIG_IIO_INPUT_BRIDGE) += industrialio-inputbridge.o
obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o obj-$(CONFIG_IIO_SW_DEVICE) += industrialio-sw-device.o
Add the iio-input-bridge to the Kconfig.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/iio/Kconfig | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig index d08aeb41cd07..d85afe002613 100644 --- a/drivers/iio/Kconfig +++ b/drivers/iio/Kconfig @@ -68,6 +68,13 @@ config IIO_TRIGGERED_EVENT help Provides helper functions for setting up triggered events.
+config IIO_INPUT_BRIDGE + bool "Enable accelerometer bridge to input driver" + help + Provides a /dev/input/event* device for accelerometers + to use as a 3D input device, e.g. for gaming or auto-rotation + of screen contents. + source "drivers/iio/accel/Kconfig" source "drivers/iio/adc/Kconfig" source "drivers/iio/afe/Kconfig"
This adds the iio-input-bridge to iio device (un)registering so that accelerometers are located and made present an input event device.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/iio/industrialio-core.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 4700fd5d8c90..81f412b41a78 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -29,6 +29,7 @@ #include <linux/iio/iio.h> #include "iio_core.h" #include "iio_core_trigger.h" +#include "industrialio-inputbridge.h" #include <linux/iio/sysfs.h> #include <linux/iio/events.h> #include <linux/iio/buffer.h> @@ -1723,6 +1724,15 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) if (ret < 0) goto error_unreg_eventset;
+ ret = iio_device_register_inputbridge(indio_dev); + if (ret) { + dev_err(indio_dev->dev.parent, + "Failed to register as input driver\n"); + device_del(&indio_dev->dev); + + return ret; + } + return 0;
error_unreg_eventset: @@ -1745,6 +1755,8 @@ void iio_device_unregister(struct iio_dev *indio_dev) { mutex_lock(&indio_dev->info_exist_lock);
+ iio_device_unregister_inputbridge(indio_dev); + cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
iio_device_unregister_debugfs(indio_dev);
On Mon, 18 Mar 2019 21:39:30 +0100 "H. Nikolaus Schaller" hns@goldelico.com wrote:
Some user spaces (e.g. some Android) use /dev/input/event* for handling the 3D position of the device with respect to the center of gravity (earth). This can be used for gaming input, rotation of screens etc.
This should be the standard because this interface is an abstraction of how this data is acquired from sensor chips. Sensor chips may be connected through different interfaces and in different positions. They may also have different parameters. And, if a chip is replaced by a different one, the values reported by the device position interface should remain the same.
But nowadays, new accelerometer chips usually just get iio drivers and rarely an evdev input driver.
Therefore we need something like a protocol stack: input device vs. raw data. It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or keyboard input events vs. raw gpio or raw USB access.
This patch set bridges the gap between raw iio data and the input device abstraction so that accelerometer measurements can also be presented as X/Y/Z accelerometer channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
Hi,
I've kind of run out of time today and want to give this a detailed look.
In the meantime a few initial comments.
1. Resend the whole series cc'ing the linux-input list and maintainer. 2. In the vast majority of devices the interrupt pin is connected for an accelerometer. and they support data ready signals. My gut feeling is that should be the preferred mode. It was for that use case that we originally put all the demux and multiple buffer code in IIO. The original series for that actually had an input bridge. https://lore.kernel.org/linux-iio/1351679431-7963-5-git-send-email-jic23@ker...
3. It's going to be a hard sell to have it 'always' map an accelerometer in IIO to an input device when configured. There are lots of other accelerometer use cases where this would be crazy. In the repost, give some comentary perhaps on the following options: a) Explicit binding - like the iio-hwmon bridge. b) A userspace bridge (I even wrote one of those years ago using uevent) c) Some sort of userspace triggered way of creating these on demand.
4. Input has polled modes. Don't reinvent them.
5. The patch break up is very very random. Just have one patch :)
Anyhow, I'll take a look in detail but may be a little while.
Thanks,
Jonathan
H. Nikolaus Schaller (4): iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input iio: input-bridge: add iio-input-bridge to Makefile iio: input-bridge: add IIO_INPUT_BRIDGE kernel config option iio: input-bridge: make the iio-input-bridge driver called by iio-core
drivers/iio/Kconfig | 7 + drivers/iio/Makefile | 1 + drivers/iio/industrialio-core.c | 12 + drivers/iio/industrialio-inputbridge.c | 420 +++++++++++++++++++++++++ drivers/iio/industrialio-inputbridge.h | 28 ++ 5 files changed, 468 insertions(+) create mode 100644 drivers/iio/industrialio-inputbridge.c create mode 100644 drivers/iio/industrialio-inputbridge.h
Hi Jonathan,
Am 24.03.2019 um 19:29 schrieb Jonathan Cameron jic23@kernel.org:
On Mon, 18 Mar 2019 21:39:30 +0100 "H. Nikolaus Schaller" hns@goldelico.com wrote:
Some user spaces (e.g. some Android) use /dev/input/event* for handling the 3D position of the device with respect to the center of gravity (earth). This can be used for gaming input, rotation of screens etc.
This should be the standard because this interface is an abstraction of how this data is acquired from sensor chips. Sensor chips may be connected through different interfaces and in different positions. They may also have different parameters. And, if a chip is replaced by a different one, the values reported by the device position interface should remain the same.
But nowadays, new accelerometer chips usually just get iio drivers and rarely an evdev input driver.
Therefore we need something like a protocol stack: input device vs. raw data. It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or keyboard input events vs. raw gpio or raw USB access.
This patch set bridges the gap between raw iio data and the input device abstraction so that accelerometer measurements can also be presented as X/Y/Z accelerometer channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
Hi,
I've kind of run out of time today and want to give this a detailed look.
No problem, please take the time it needs.
In the meantime a few initial comments.
- Resend the whole series cc'ing the linux-input list and maintainer.
Ok!
- In the vast majority of devices the interrupt pin is connected for an
accelerometer. and they support data ready signals. My gut feeling is that should be the preferred mode.
Mine too, and the commit message 1/4 describes this by
"If it runs, the driver polls the device(s) once every 100 ms. A mode where the iio device defines the update rate is not implemented and for further study."
But I didn't manage to get this correctly set up for in-kernel iio on demand (start/stop when a process does an open/close - it was simpler to start/stop polling).
Here I am lacking some understanding of details of the subsystems and their capabilities.
Therefore I focussed on the polling case to leave the interrupt driven case for later improvements. Also, since 100ms doesn't seem to be a big resource burden.
I am also not sure if we really want to process every fidget detected by the raw sensor to the input subsystem. This could require more resources than polling with 100ms distance.
It was for that use case that we originally put all the demux and multiple buffer code in IIO. The original series for that actually had an input bridge. https://lore.kernel.org/linux-iio/1351679431-7963-5-git-send-email-jic23@ker...
Oh, nice! I wasn't aware of that. If I had known, we might have based our code on it.
It seems to be a good blueprint to understand how to set up the callbacks.
What I do not understand is where the "iio map" comes from in your approach.
From device tree? That would IMHO be against the rule that DTS should describe hardware but bridging data to a different user-space interface is not related to hardware description.
So our approach does not need a specific mapping.
- It's going to be a hard sell to have it 'always' map an accelerometer in IIO
to an input device when configured. There are lots of other accelerometer use cases where this would be crazy. In the repost, give some comentary perhaps on the following options: a) Explicit binding - like the iio-hwmon bridge. b) A userspace bridge (I even wrote one of those years ago using uevent) c) Some sort of userspace triggered way of creating these on demand.
Basically, if you have one of these many other use cases, one can just keep this bridge unconfigured. Then, it does not eat up memory or code space or processor cycles.
Its only use case is to map iio accelerometers to input devices which are really used as input devices, i.e. gaming, device orientation etc. Other use cases (e.g. industrial sensor grid data aggregator) should not consider this as an alternative interface and use e.g. libiio of course.
Those (handheld) devices in mind usually have only a single accelerometer chip, but there are exceptions like the GTA04 and the Pyra which can have two of them (with different precision, mounting orientation and functions). Maybe there are some devices with more than two. So these iio devices should be reported separately but the data (X,Y,Z) should mainly agree on input device level.
In the two-sensor case, our proposed driver simply creates two distinct /dev/input/event files which can be given fixed file names by udev instead of inventing a new trigger to create these on demand. IMHO it should not be handled differently from plugging in multiple mice or joysticks or other gaming devices to USB.
And, this driver converts "raw" accelerometer data into a higher abstraction level input event in a way that user-space becomes independent of the specific chip set and its orientation. This is like hiding different flash hardware interfaces by MTD or file systems. All the hardware details are hidden and shielded. Or it is like mapping gpios or alternatively an i2c scanner chio into standardized key codes.
Finally, there is no polling (in this polling setup) if no process opens the input device. So the only waste of 'always' mapping 'all' accelerometers but not using them are some data structure allocations in the kernel and duration of probe().
- Input has polled modes. Don't reinvent them.
Haven't heard of, but that is what a RFC is good for.
Anyways, if we use iio interrupts we don't need it, because the information flow should go from the chip to the iio driver to the input bridge to the input subsystem and only then notify user space (wake up the process running a select on the event device file).
Now, there is something I have learned from studying your comment and struct input_dev.
Although I did not find polled modes, I can simplify our approach significantly since we do not have to count open/close ourselves and define a mutex for that.
This is already done by the input_dev. So I should improve this part of our patches before resending to input list. Thanks for inpsiring this.
- The patch break up is very very random. Just have one patch :)
Well, it tries to have small pieces in a specific order that you can bisect and do not run into compile problems (e.g. adding Makefile before code).
But I can squash it. Seems to make sense since it is not really useful to know which of these pieces breaks other things. It can only be the driver and bisect will still pin-point it.
Anyhow, I'll take a look in detail but may be a little while.
No reason to hurry. We can wait.
Thanks,
Jonathan
Thanks and BR, Nikolaus
H. Nikolaus Schaller (4): iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input iio: input-bridge: add iio-input-bridge to Makefile iio: input-bridge: add IIO_INPUT_BRIDGE kernel config option iio: input-bridge: make the iio-input-bridge driver called by iio-core
drivers/iio/Kconfig | 7 + drivers/iio/Makefile | 1 + drivers/iio/industrialio-core.c | 12 + drivers/iio/industrialio-inputbridge.c | 420 +++++++++++++++++++++++++ drivers/iio/industrialio-inputbridge.h | 28 ++ 5 files changed, 468 insertions(+) create mode 100644 drivers/iio/industrialio-inputbridge.c create mode 100644 drivers/iio/industrialio-inputbridge.h
Hi all, I'll send a single v2 patch in some minutes.
Changes compared to this v1: * add linux-input list and maintainers for discussion * use input-polldev * allocate one mapping record for each iio device instead of trying to manage that in some array
Am 28.03.2019 um 18:42 schrieb H. Nikolaus Schaller hns@goldelico.com:
Hi Jonathan,
Am 24.03.2019 um 19:29 schrieb Jonathan Cameron jic23@kernel.org:
On Mon, 18 Mar 2019 21:39:30 +0100 "H. Nikolaus Schaller" hns@goldelico.com wrote:
Some user spaces (e.g. some Android) use /dev/input/event* for handling the 3D position of the device with respect to the center of gravity (earth). This can be used for gaming input, rotation of screens etc.
This should be the standard because this interface is an abstraction of how this data is acquired from sensor chips. Sensor chips may be connected through different interfaces and in different positions. They may also have different parameters. And, if a chip is replaced by a different one, the values reported by the device position interface should remain the same.
But nowadays, new accelerometer chips usually just get iio drivers and rarely an evdev input driver.
Therefore we need something like a protocol stack: input device vs. raw data. It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or keyboard input events vs. raw gpio or raw USB access.
This patch set bridges the gap between raw iio data and the input device abstraction so that accelerometer measurements can also be presented as X/Y/Z accelerometer channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
Hi,
I've kind of run out of time today and want to give this a detailed look.
No problem, please take the time it needs.
In the meantime a few initial comments.
- Resend the whole series cc'ing the linux-input list and maintainer.
Ok!
Done.
- In the vast majority of devices the interrupt pin is connected for an
accelerometer. and they support data ready signals. My gut feeling is that should be the preferred mode.
Mine too, and the commit message 1/4 describes this by
"If it runs, the driver polls the device(s) once every 100 ms. A mode where the iio device defines the update rate is not implemented and for further study."
But I didn't manage to get this correctly set up for in-kernel iio on demand (start/stop when a process does an open/close - it was simpler to start/stop polling).
Here I am lacking some understanding of details of the subsystems and their capabilities.
Therefore I focussed on the polling case to leave the interrupt driven case for later improvements. Also, since 100ms doesn't seem to be a big resource burden.
I am also not sure if we really want to process every fidget detected by the raw sensor to the input subsystem. This could require more resources than polling with 100ms distance.
Because this is a quite fundamental design decision (do we call input_register_device or input_register_polled_device?) and the above expressed concern about noise triggering more input events than user-space may want to see, I have stayed with low-speed polling for the time being.
It was for that use case that we originally put all the demux and multiple buffer code in IIO. The original series for that actually had an input bridge. https://lore.kernel.org/linux-iio/1351679431-7963-5-git-send-email-jic23@ker...
Oh, nice! I wasn't aware of that. If I had known, we might have based our code on it.
It seems to be a good blueprint to understand how to set up the callbacks.
What I do not understand is where the "iio map" comes from in your approach.
From device tree? That would IMHO be against the rule that DTS should describe hardware but bridging data to a different user-space interface is not related to hardware description.
So our approach does not need a specific mapping.
- It's going to be a hard sell to have it 'always' map an accelerometer in IIO
to an input device when configured. There are lots of other accelerometer use cases where this would be crazy. In the repost, give some comentary perhaps on the following options: a) Explicit binding - like the iio-hwmon bridge. b) A userspace bridge (I even wrote one of those years ago using uevent) c) Some sort of userspace triggered way of creating these on demand.
Basically, if you have one of these many other use cases, one can just keep this bridge unconfigured. Then, it does not eat up memory or code space or processor cycles.
Its only use case is to map iio accelerometers to input devices which are really used as input devices, i.e. gaming, device orientation etc. Other use cases (e.g. industrial sensor grid data aggregator) should not consider this as an alternative interface and use e.g. libiio of course.
Those (handheld) devices in mind usually have only a single accelerometer chip, but there are exceptions like the GTA04 and the Pyra which can have two of them (with different precision, mounting orientation and functions). Maybe there are some devices with more than two. So these iio devices should be reported separately but the data (X,Y,Z) should mainly agree on input device level.
In the two-sensor case, our proposed driver simply creates two distinct /dev/input/event files which can be given fixed file names by udev instead of inventing a new trigger to create these on demand. IMHO it should not be handled differently from plugging in multiple mice or joysticks or other gaming devices to USB.
And, this driver converts "raw" accelerometer data into a higher abstraction level input event in a way that user-space becomes independent of the specific chip set and its orientation. This is like hiding different flash hardware interfaces by MTD or file systems. All the hardware details are hidden and shielded. Or it is like mapping gpios or alternatively an i2c scanner chio into standardized key codes.
Finally, there is no polling (in this polling setup) if no process opens the input device. So the only waste of 'always' mapping 'all' accelerometers but not using them are some data structure allocations in the kernel and duration of probe().
- Input has polled modes. Don't reinvent them.
Haven't heard of, but that is what a RFC is good for.
Anyways, if we use iio interrupts we don't need it, because the information flow should go from the chip to the iio driver to the input bridge to the input subsystem and only then notify user space (wake up the process running a select on the event device file).
Now, there is something I have learned from studying your comment and struct input_dev.
Although I did not find polled modes, I can simplify our approach significantly since we do not have to count open/close ourselves and define a mutex for that.
This is already done by the input_dev. So I should improve this part of our patches before resending to input list. Thanks for inpsiring this.
I finally found it. It is an input-polldev wrapper.
I have reworked the driver in polling mode to use that now. It has simplified the code a lot.
Thanks again!
Nikolaus
- The patch break up is very very random. Just have one patch :)
Well, it tries to have small pieces in a specific order that you can bisect and do not run into compile problems (e.g. adding Makefile before code).
But I can squash it. Seems to make sense since it is not really useful to know which of these pieces breaks other things. It can only be the driver and bisect will still pin-point it.
Anyhow, I'll take a look in detail but may be a little while.
No reason to hurry. We can wait.
Thanks,
Jonathan
Thanks and BR, Nikolaus
H. Nikolaus Schaller (4): iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input iio: input-bridge: add iio-input-bridge to Makefile iio: input-bridge: add IIO_INPUT_BRIDGE kernel config option iio: input-bridge: make the iio-input-bridge driver called by iio-core
drivers/iio/Kconfig | 7 + drivers/iio/Makefile | 1 + drivers/iio/industrialio-core.c | 12 + drivers/iio/industrialio-inputbridge.c | 420 +++++++++++++++++++++++++ drivers/iio/industrialio-inputbridge.h | 28 ++ 5 files changed, 468 insertions(+) create mode 100644 drivers/iio/industrialio-inputbridge.c create mode 100644 drivers/iio/industrialio-inputbridge.h
On Thu, 28 Mar 2019 18:42:44 +0100 "H. Nikolaus Schaller" hns@goldelico.com wrote:
Hi Jonathan,
Am 24.03.2019 um 19:29 schrieb Jonathan Cameron jic23@kernel.org:
On Mon, 18 Mar 2019 21:39:30 +0100 "H. Nikolaus Schaller" hns@goldelico.com wrote:
Some user spaces (e.g. some Android) use /dev/input/event* for handling the 3D position of the device with respect to the center of gravity (earth). This can be used for gaming input, rotation of screens etc.
This should be the standard because this interface is an abstraction of how this data is acquired from sensor chips. Sensor chips may be connected through different interfaces and in different positions. They may also have different parameters. And, if a chip is replaced by a different one, the values reported by the device position interface should remain the same.
But nowadays, new accelerometer chips usually just get iio drivers and rarely an evdev input driver.
Therefore we need something like a protocol stack: input device vs. raw data. It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or keyboard input events vs. raw gpio or raw USB access.
This patch set bridges the gap between raw iio data and the input device abstraction so that accelerometer measurements can also be presented as X/Y/Z accelerometer channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
Hi,
I've kind of run out of time today and want to give this a detailed look.
No problem, please take the time it needs.
In the meantime a few initial comments.
- Resend the whole series cc'ing the linux-input list and maintainer.
Ok!
- In the vast majority of devices the interrupt pin is connected for an
accelerometer. and they support data ready signals. My gut feeling is that should be the preferred mode.
Mine too, and the commit message 1/4 describes this by
"If it runs, the driver polls the device(s) once every 100 ms. A mode where the iio device defines the update rate is not implemented and for further study."
Absolutely, I saw that, just wanted to encourage this avenue ;) It's particularly good if we have a controllable frequency device and can have it self clock at a rather that makes sense for human input. Also lets the gamers run at 2kHz if they really want to ;)
But I didn't manage to get this correctly set up for in-kernel iio on demand (start/stop when a process does an open/close - it was simpler to start/stop polling).
Here I am lacking some understanding of details of the subsystems and their capabilities.
Therefore I focussed on the polling case to leave the interrupt driven case for later improvements. Also, since 100ms doesn't seem to be a big resource burden.
One thing the IIO consumer interfaces have always lacked is nice fallback between the buffered / interrupt mode and polled read_raw modes. In theory if should be possible to elegantly assume that we have a self clocking sensor but if it doesn't actually support that, fall back to polling and still have it come out of the buffered interface. The snag has always been that we currently end up adding, non-trivial, code to the IIO drivers to support buffered modes and we don't want to put that burden on all drivers.
I am also not sure if we really want to process every fidget detected by the raw sensor to the input subsystem. This could require more resources than polling with 100ms distance.
Agreed. Thankfully most such sensors also have frequency controls so we can normally get to a sensible level. Input gets to do all the filtering etc beyond that.
It was for that use case that we originally put all the demux and multiple buffer code in IIO. The original series for that actually had an input bridge. https://lore.kernel.org/linux-iio/1351679431-7963-5-git-send-email-jic23@ker...
Oh, nice! I wasn't aware of that. If I had known, we might have based our code on it.
You probably noticed in the comments, it was never finished! The problem is that all my boards are headless so I never cared ;) Good to have someone looking at this problem who does.
It seems to be a good blueprint to understand how to set up the callbacks.
What I do not understand is where the "iio map" comes from in your approach.
From device tree? That would IMHO be against the rule that DTS should describe hardware but bridging data to a different user-space interface is not related to hardware description.
This has long been an 'argument' on what exactly makes up hardware description. The problem is that usecases really are hardware description. There has to be some means fo knowing what the thing is for, to dictate what software support we supply. Whether that is in userspace or kernel or as you have done is assumed "if plausible" is an open question.
Back when this was originally proposed, there weren't clear rules on this in DT. It's hard to argue this is any different from hwmon, though Mark in particular really dislikes the iio-hwmon interface. It has the problem that no one has ever come up with a better one. Only potential option is to create abstract devices to represent the fact a given ADC channel is used for measuring a 'board voltage' or something like that. They provide no useful meaning that is actually different from putting them in the iio-hwmon bindings.
The same is kind of true here with the reality being we need to know what the 'driver' of the acceleration is.
Could have a binding for 'human' vs 'bridge' vs 'car' :)
So our approach does not need a specific mapping.
- It's going to be a hard sell to have it 'always' map an accelerometer in IIO
to an input device when configured. There are lots of other accelerometer use cases where this would be crazy. In the repost, give some comentary perhaps on the following options: a) Explicit binding - like the iio-hwmon bridge. b) A userspace bridge (I even wrote one of those years ago using uevent) c) Some sort of userspace triggered way of creating these on demand.
Basically, if you have one of these many other use cases, one can just keep this bridge unconfigured. Then, it does not eat up memory or code space or processor cycles.
That's a non starter. Lots of people aren't going to tune their kernel to 'unconfigure' some support because they want to turn a feature off. Take a raspberry pi for instance. If it has an accelerometer, the question of whether anyone wants to use it is down to whether there is a screen attached or not.
Its only use case is to map iio accelerometers to input devices which are really used as input devices, i.e. gaming, device orientation etc. Other use cases (e.g. industrial sensor grid data aggregator) should not consider this as an alternative interface and use e.g. libiio of course.
Those (handheld) devices in mind usually have only a single accelerometer chip, but there are exceptions like the GTA04 and the Pyra which can have two of them (with different precision, mounting orientation and functions). Maybe there are some devices with more than two. So these iio devices should be reported separately but the data (X,Y,Z) should mainly agree on input device level.
There are lots of two sensor cases where the sensors don't agree, mainly because they are connected by a hinge (screen and keyboard on laptops for example). Doesn't matter really though.
In the two-sensor case, our proposed driver simply creates two distinct /dev/input/event files which can be given fixed file names by udev instead of inventing a new trigger to create these on demand. IMHO it should not be handled differently from plugging in multiple mice or joysticks or other gaming devices to USB.
Alternative UDEV can pick up on a new IIO device and poke an interface to register an input device from it. We've talked about allowing iio_maps to be instantiated via configfs and it may be that is the modern choice for how to do this.
And, this driver converts "raw" accelerometer data into a higher abstraction level input event in a way that user-space becomes independent of the specific chip set and its orientation. This is like hiding different flash hardware interfaces by MTD or file systems. All the hardware details are hidden and shielded. Or it is like mapping gpios or alternatively an i2c scanner chio into standardized key codes.
Finally, there is no polling (in this polling setup) if no process opens the input device. So the only waste of 'always' mapping 'all' accelerometers but not using them are some data structure allocations in the kernel and duration of probe().
Sure, it's not a heavy burden - but it is conceptually dubious to register an interface because 'it might' be wanted. I'd like Dmitry and the input lists view on this (plus others in IIO!)
- Input has polled modes. Don't reinvent them.
Haven't heard of, but that is what a RFC is good for.
Anyways, if we use iio interrupts we don't need it, because the information flow should go from the chip to the iio driver to the input bridge to the input subsystem and only then notify user space (wake up the process running a select on the event device file).
Now, there is something I have learned from studying your comment and struct input_dev.
Although I did not find polled modes, I can simplify our approach significantly since we do not have to count open/close ourselves and define a mutex for that.
This is already done by the input_dev. So I should improve this part of our patches before resending to input list. Thanks for inpsiring this.
I see from the next mail you found the polled code. Dmitry would have kicked back on that if you hadn't (we have had this a few times before ;)
- The patch break up is very very random. Just have one patch :)
Well, it tries to have small pieces in a specific order that you can bisect and do not run into compile problems (e.g. adding Makefile before code).
But I can squash it. Seems to make sense since it is not really useful to know which of these pieces breaks other things. It can only be the driver and bisect will still pin-point it.
Quite. It's really one new function so keeping it in one patch is fine.
Patch break up of large new changes is always an interesting one. Sometimes it's much easier to review a 2000 line patch than 5 smaller ones, but conversely a 2000 line patch doesn't always fit into lunch time ;)
Anyhow, I'll take a look in detail but may be a little while.
No reason to hurry. We can wait.
Thanks,
Jonathan
Thanks and BR, Nikolaus
Great to have this 'hole' filled at last. I've been moaning about it for years but never actually got interested enough to do it myself!
Jonathan
H. Nikolaus Schaller (4): iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input iio: input-bridge: add iio-input-bridge to Makefile iio: input-bridge: add IIO_INPUT_BRIDGE kernel config option iio: input-bridge: make the iio-input-bridge driver called by iio-core
drivers/iio/Kconfig | 7 + drivers/iio/Makefile | 1 + drivers/iio/industrialio-core.c | 12 + drivers/iio/industrialio-inputbridge.c | 420 +++++++++++++++++++++++++ drivers/iio/industrialio-inputbridge.h | 28 ++ 5 files changed, 468 insertions(+) create mode 100644 drivers/iio/industrialio-inputbridge.c create mode 100644 drivers/iio/industrialio-inputbridge.h
-
H. Nikolaus Schaller -
Jonathan Cameron