* [PATCH] Input: add support for polling to input devices
@ 2019-08-09 17:35 Dmitry Torokhov
2019-08-12 16:50 ` Benjamin Tissoires
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2019-08-09 17:35 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel, benjamin.tissoires
Separating "normal" and "polled" input devices was a mistake, as often we
want to allow the very same device work on both interrupt-driven and
polled mode, depending on the board on which the device is used.
This introduces new APIs:
- input_setup_polling
- input_set_poll_interval
- input_set_min_poll_interval
- input_set_max_poll_interval
These new APIs allow switching an input device into polled mode with sysfs
attributes matching drivers using input_polled_dev APIs that will be
eventually removed.
Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/Makefile | 2 +-
drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
drivers/input/input-poller.h | 18 +++
drivers/input/input.c | 36 ++++--
include/linux/input.h | 12 ++
5 files changed, 268 insertions(+), 8 deletions(-)
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 40de6a7be641..e35650930371 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -6,7 +6,7 @@
# Each configuration option enables a list of files.
obj-$(CONFIG_INPUT) += input-core.o
-input-core-y := input.o input-compat.o input-mt.o ff-core.o
+input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
new file mode 100644
index 000000000000..e041adb04f5a
--- /dev/null
+++ b/drivers/input/input-poller.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Support for polling mode for input devices.
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/jiffies.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+#include "input-poller.h"
+
+struct input_dev_poller {
+ void (*poll)(struct input_dev *dev);
+
+ unsigned int poll_interval; /* msec */
+ unsigned int poll_interval_max; /* msec */
+ unsigned int poll_interval_min; /* msec */
+
+ struct input_dev *input;
+ struct delayed_work work;
+};
+
+static void input_dev_poller_queue_work(struct input_dev_poller *poller)
+{
+ unsigned long delay;
+
+ delay = msecs_to_jiffies(poller->poll_interval);
+ if (delay >= HZ)
+ delay = round_jiffies_relative(delay);
+
+ queue_delayed_work(system_freezable_wq, &poller->work, delay);
+}
+
+static void input_dev_poller_work(struct work_struct *work)
+{
+ struct input_dev_poller *poller =
+ container_of(work, struct input_dev_poller, work.work);
+
+ poller->poll(poller->input);
+ input_dev_poller_queue_work(poller);
+}
+
+void input_dev_poller_finalize(struct input_dev_poller *poller)
+{
+ if (!poller->poll_interval)
+ poller->poll_interval = 500;
+ if (!poller->poll_interval_max)
+ poller->poll_interval_max = poller->poll_interval;
+}
+
+void input_dev_poller_start(struct input_dev_poller *poller)
+{
+ /* Only start polling if polling is enabled */
+ if (poller->poll_interval > 0) {
+ poller->poll(poller->input);
+ input_dev_poller_queue_work(poller);
+ }
+}
+
+void input_dev_poller_stop(struct input_dev_poller *poller)
+{
+ cancel_delayed_work_sync(&poller->work);
+}
+
+int input_setup_polling(struct input_dev *dev,
+ void (*poll_fn)(struct input_dev *dev))
+{
+ struct input_dev_poller *poller;
+
+ poller = kzalloc(sizeof(*poller), GFP_KERNEL);
+ if (!poller) {
+ dev_err(dev->dev.parent ?: &dev->dev,
+ "%s: unable to allocate poller structure\n", __func__);
+ return -ENOMEM;
+ }
+
+ INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
+ poller->input = dev;
+ poller->poll = poll_fn;
+
+ dev->poller = poller;
+ return 0;
+}
+EXPORT_SYMBOL(input_setup_polling);
+
+static bool input_dev_ensure_poller(struct input_dev *dev)
+{
+ if (!dev->poller) {
+ dev_err(dev->dev.parent ?: &dev->dev,
+ "poller structure has not been set up\n");
+ return false;
+ }
+
+ return true;
+}
+
+void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
+{
+ if (input_dev_ensure_poller(dev))
+ dev->poller->poll_interval = interval;
+}
+EXPORT_SYMBOL(input_set_poll_interval);
+
+void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
+{
+ if (input_dev_ensure_poller(dev))
+ dev->poller->poll_interval_min = interval;
+}
+EXPORT_SYMBOL(input_set_min_poll_interval);
+
+void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
+{
+ if (input_dev_ensure_poller(dev))
+ dev->poller->poll_interval_max = interval;
+}
+EXPORT_SYMBOL(input_set_max_poll_interval);
+
+/* SYSFS interface */
+
+static ssize_t input_dev_get_poll_interval(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct input_dev *input = to_input_dev(dev);
+
+ return sprintf(buf, "%d\n", input->poller->poll_interval);
+}
+
+static ssize_t input_dev_set_poll_interval(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct input_dev *input = to_input_dev(dev);
+ struct input_dev_poller *poller = input->poller;
+ unsigned int interval;
+ int err;
+
+ err = kstrtouint(buf, 0, &interval);
+ if (err)
+ return err;
+
+ if (interval < poller->poll_interval_min)
+ return -EINVAL;
+
+ if (interval > poller->poll_interval_max)
+ return -EINVAL;
+
+ mutex_lock(&input->mutex);
+
+ poller->poll_interval = interval;
+
+ if (input->users) {
+ cancel_delayed_work_sync(&poller->work);
+ if (poller->poll_interval > 0)
+ input_dev_poller_queue_work(poller);
+ }
+
+ mutex_unlock(&input->mutex);
+
+ return count;
+}
+
+static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
+ input_dev_get_poll_interval, input_dev_set_poll_interval);
+
+static ssize_t input_dev_get_poll_max(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct input_dev *input = to_input_dev(dev);
+
+ return sprintf(buf, "%d\n", input->poller->poll_interval_max);
+}
+
+static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
+
+static ssize_t input_dev_get_poll_min(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct input_dev *input = to_input_dev(dev);
+
+ return sprintf(buf, "%d\n", input->poller->poll_interval_min);
+}
+
+static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
+
+static umode_t input_poller_attrs_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct input_dev *input = to_input_dev(dev);
+
+ return input->poller ? attr->mode : 0;
+}
+
+static struct attribute *input_poller_attrs[] = {
+ &dev_attr_poll.attr,
+ &dev_attr_max.attr,
+ &dev_attr_min.attr,
+ NULL
+};
+
+struct attribute_group input_poller_attribute_group = {
+ .is_visible = input_poller_attrs_visible,
+ .attrs = input_poller_attrs,
+};
diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
new file mode 100644
index 000000000000..e3fca0be1d32
--- /dev/null
+++ b/drivers/input/input-poller.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _INPUT_POLLER_H
+#define _INPUT_POLLER_H
+
+/*
+ * Support for polling mode for input devices.
+ */
+#include <linux/sysfs.h>
+
+struct input_dev_poller;
+
+void input_dev_poller_finalize(struct input_dev_poller *poller);
+void input_dev_poller_start(struct input_dev_poller *poller);
+void input_dev_poller_stop(struct input_dev_poller *poller);
+
+extern struct attribute_group input_poller_attribute_group;
+
+#endif /* _INPUT_POLLER_H */
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 7494a0dede79..c08aa3596144 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -24,6 +24,7 @@
#include <linux/mutex.h>
#include <linux/rcupdate.h>
#include "input-compat.h"
+#include "input-poller.h"
MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
MODULE_DESCRIPTION("Input core");
@@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)
handle->open++;
- if (!dev->users++ && dev->open)
- retval = dev->open(dev);
+ if (dev->users++) {
+ /*
+ * Device is already opened, so we can exit immediately and
+ * report success.
+ */
+ goto out;
+ }
- if (retval) {
- dev->users--;
- if (!--handle->open) {
+ if (dev->open) {
+ retval = dev->open(dev);
+ if (retval) {
+ dev->users--;
+ handle->open--;
/*
* Make sure we are not delivering any more events
* through this handle
*/
synchronize_rcu();
+ goto out;
}
}
+ if (dev->poller)
+ input_dev_poller_start(dev->poller);
+
out:
mutex_unlock(&dev->mutex);
return retval;
@@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)
__input_release_device(handle);
- if (!--dev->users && dev->close)
- dev->close(dev);
+ if (!--dev->users) {
+ if (dev->poller)
+ input_dev_poller_stop(dev->poller);
+
+ if (dev->close)
+ dev->close(dev);
+ }
if (!--handle->open) {
/*
@@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
&input_dev_attr_group,
&input_dev_id_attr_group,
&input_dev_caps_attr_group,
+ &input_poller_attribute_group,
NULL
};
@@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)
input_ff_destroy(dev);
input_mt_destroy_slots(dev);
+ kfree(dev->poller);
kfree(dev->absinfo);
kfree(dev->vals);
kfree(dev);
@@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
if (!dev->setkeycode)
dev->setkeycode = input_default_setkeycode;
+ if (dev->poller)
+ input_dev_poller_finalize(dev->poller);
+
error = device_add(&dev->dev);
if (error)
goto err_free_vals;
diff --git a/include/linux/input.h b/include/linux/input.h
index e95a439d8bd5..94f277cd806a 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -21,6 +21,8 @@
#include <linux/timer.h>
#include <linux/mod_devicetable.h>
+struct input_dev_poller;
+
/**
* struct input_value - input value representation
* @type: type of value (EV_KEY, EV_ABS, etc)
@@ -71,6 +73,8 @@ enum input_clock_type {
* not sleep
* @ff: force feedback structure associated with the device if device
* supports force feedback effects
+ * @poller: poller structure associated with the device if device is
+ * set up to use polling mode
* @repeat_key: stores key code of the last key pressed; used to implement
* software autorepeat
* @timer: timer for software autorepeat
@@ -156,6 +160,8 @@ struct input_dev {
struct ff_device *ff;
+ struct input_dev_poller *poller;
+
unsigned int repeat_key;
struct timer_list timer;
@@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);
void input_reset_device(struct input_dev *);
+int input_setup_polling(struct input_dev *dev,
+ void (*poll_fn)(struct input_dev *dev));
+void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
+void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
+void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
+
int __must_check input_register_handler(struct input_handler *);
void input_unregister_handler(struct input_handler *);
--
2.23.0.rc1.153.gdeed80330f-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: add support for polling to input devices
2019-08-09 17:35 [PATCH] Input: add support for polling to input devices Dmitry Torokhov
@ 2019-08-12 16:50 ` Benjamin Tissoires
2019-08-12 17:11 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2019-08-12 16:50 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, lkml
Hi Dmitry,
On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Separating "normal" and "polled" input devices was a mistake, as often we
> want to allow the very same device work on both interrupt-driven and
> polled mode, depending on the board on which the device is used.
>
> This introduces new APIs:
>
> - input_setup_polling
> - input_set_poll_interval
> - input_set_min_poll_interval
> - input_set_max_poll_interval
>
> These new APIs allow switching an input device into polled mode with sysfs
> attributes matching drivers using input_polled_dev APIs that will be
> eventually removed.
Are you sure that using sysfs is the correct way here?
I would think using generic properties that can be overwritten by the
DSDT or by the device tree would make things easier from a driver
point of view.
Also, checkpatch complains about a few octal permissions that are
preferred over symbolic ones, and there is a possible 'out of memory'
nessage at drivers/input/input-poller.c:75.
Cheers,
Benjamin
>
> Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/Makefile | 2 +-
> drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
> drivers/input/input-poller.h | 18 +++
> drivers/input/input.c | 36 ++++--
> include/linux/input.h | 12 ++
> 5 files changed, 268 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index 40de6a7be641..e35650930371 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -6,7 +6,7 @@
> # Each configuration option enables a list of files.
>
> obj-$(CONFIG_INPUT) += input-core.o
> -input-core-y := input.o input-compat.o input-mt.o ff-core.o
> +input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
>
> obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
> diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
> new file mode 100644
> index 000000000000..e041adb04f5a
> --- /dev/null
> +++ b/drivers/input/input-poller.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Support for polling mode for input devices.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +#include "input-poller.h"
> +
> +struct input_dev_poller {
> + void (*poll)(struct input_dev *dev);
> +
> + unsigned int poll_interval; /* msec */
> + unsigned int poll_interval_max; /* msec */
> + unsigned int poll_interval_min; /* msec */
> +
> + struct input_dev *input;
> + struct delayed_work work;
> +};
> +
> +static void input_dev_poller_queue_work(struct input_dev_poller *poller)
> +{
> + unsigned long delay;
> +
> + delay = msecs_to_jiffies(poller->poll_interval);
> + if (delay >= HZ)
> + delay = round_jiffies_relative(delay);
> +
> + queue_delayed_work(system_freezable_wq, &poller->work, delay);
> +}
> +
> +static void input_dev_poller_work(struct work_struct *work)
> +{
> + struct input_dev_poller *poller =
> + container_of(work, struct input_dev_poller, work.work);
> +
> + poller->poll(poller->input);
> + input_dev_poller_queue_work(poller);
> +}
> +
> +void input_dev_poller_finalize(struct input_dev_poller *poller)
> +{
> + if (!poller->poll_interval)
> + poller->poll_interval = 500;
> + if (!poller->poll_interval_max)
> + poller->poll_interval_max = poller->poll_interval;
> +}
> +
> +void input_dev_poller_start(struct input_dev_poller *poller)
> +{
> + /* Only start polling if polling is enabled */
> + if (poller->poll_interval > 0) {
> + poller->poll(poller->input);
> + input_dev_poller_queue_work(poller);
> + }
> +}
> +
> +void input_dev_poller_stop(struct input_dev_poller *poller)
> +{
> + cancel_delayed_work_sync(&poller->work);
> +}
> +
> +int input_setup_polling(struct input_dev *dev,
> + void (*poll_fn)(struct input_dev *dev))
> +{
> + struct input_dev_poller *poller;
> +
> + poller = kzalloc(sizeof(*poller), GFP_KERNEL);
> + if (!poller) {
> + dev_err(dev->dev.parent ?: &dev->dev,
> + "%s: unable to allocate poller structure\n", __func__);
> + return -ENOMEM;
> + }
> +
> + INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
> + poller->input = dev;
> + poller->poll = poll_fn;
> +
> + dev->poller = poller;
> + return 0;
> +}
> +EXPORT_SYMBOL(input_setup_polling);
> +
> +static bool input_dev_ensure_poller(struct input_dev *dev)
> +{
> + if (!dev->poller) {
> + dev_err(dev->dev.parent ?: &dev->dev,
> + "poller structure has not been set up\n");
> + return false;
> + }
> +
> + return true;
> +}
> +
> +void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
> +{
> + if (input_dev_ensure_poller(dev))
> + dev->poller->poll_interval = interval;
> +}
> +EXPORT_SYMBOL(input_set_poll_interval);
> +
> +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
> +{
> + if (input_dev_ensure_poller(dev))
> + dev->poller->poll_interval_min = interval;
> +}
> +EXPORT_SYMBOL(input_set_min_poll_interval);
> +
> +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
> +{
> + if (input_dev_ensure_poller(dev))
> + dev->poller->poll_interval_max = interval;
> +}
> +EXPORT_SYMBOL(input_set_max_poll_interval);
> +
> +/* SYSFS interface */
> +
> +static ssize_t input_dev_get_poll_interval(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct input_dev *input = to_input_dev(dev);
> +
> + return sprintf(buf, "%d\n", input->poller->poll_interval);
> +}
> +
> +static ssize_t input_dev_set_poll_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct input_dev *input = to_input_dev(dev);
> + struct input_dev_poller *poller = input->poller;
> + unsigned int interval;
> + int err;
> +
> + err = kstrtouint(buf, 0, &interval);
> + if (err)
> + return err;
> +
> + if (interval < poller->poll_interval_min)
> + return -EINVAL;
> +
> + if (interval > poller->poll_interval_max)
> + return -EINVAL;
> +
> + mutex_lock(&input->mutex);
> +
> + poller->poll_interval = interval;
> +
> + if (input->users) {
> + cancel_delayed_work_sync(&poller->work);
> + if (poller->poll_interval > 0)
> + input_dev_poller_queue_work(poller);
> + }
> +
> + mutex_unlock(&input->mutex);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
> + input_dev_get_poll_interval, input_dev_set_poll_interval);
> +
> +static ssize_t input_dev_get_poll_max(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct input_dev *input = to_input_dev(dev);
> +
> + return sprintf(buf, "%d\n", input->poller->poll_interval_max);
> +}
> +
> +static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
> +
> +static ssize_t input_dev_get_poll_min(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct input_dev *input = to_input_dev(dev);
> +
> + return sprintf(buf, "%d\n", input->poller->poll_interval_min);
> +}
> +
> +static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
> +
> +static umode_t input_poller_attrs_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct input_dev *input = to_input_dev(dev);
> +
> + return input->poller ? attr->mode : 0;
> +}
> +
> +static struct attribute *input_poller_attrs[] = {
> + &dev_attr_poll.attr,
> + &dev_attr_max.attr,
> + &dev_attr_min.attr,
> + NULL
> +};
> +
> +struct attribute_group input_poller_attribute_group = {
> + .is_visible = input_poller_attrs_visible,
> + .attrs = input_poller_attrs,
> +};
> diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
> new file mode 100644
> index 000000000000..e3fca0be1d32
> --- /dev/null
> +++ b/drivers/input/input-poller.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _INPUT_POLLER_H
> +#define _INPUT_POLLER_H
> +
> +/*
> + * Support for polling mode for input devices.
> + */
> +#include <linux/sysfs.h>
> +
> +struct input_dev_poller;
> +
> +void input_dev_poller_finalize(struct input_dev_poller *poller);
> +void input_dev_poller_start(struct input_dev_poller *poller);
> +void input_dev_poller_stop(struct input_dev_poller *poller);
> +
> +extern struct attribute_group input_poller_attribute_group;
> +
> +#endif /* _INPUT_POLLER_H */
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 7494a0dede79..c08aa3596144 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -24,6 +24,7 @@
> #include <linux/mutex.h>
> #include <linux/rcupdate.h>
> #include "input-compat.h"
> +#include "input-poller.h"
>
> MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> MODULE_DESCRIPTION("Input core");
> @@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)
>
> handle->open++;
>
> - if (!dev->users++ && dev->open)
> - retval = dev->open(dev);
> + if (dev->users++) {
> + /*
> + * Device is already opened, so we can exit immediately and
> + * report success.
> + */
> + goto out;
> + }
>
> - if (retval) {
> - dev->users--;
> - if (!--handle->open) {
> + if (dev->open) {
> + retval = dev->open(dev);
> + if (retval) {
> + dev->users--;
> + handle->open--;
> /*
> * Make sure we are not delivering any more events
> * through this handle
> */
> synchronize_rcu();
> + goto out;
> }
> }
>
> + if (dev->poller)
> + input_dev_poller_start(dev->poller);
> +
> out:
> mutex_unlock(&dev->mutex);
> return retval;
> @@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)
>
> __input_release_device(handle);
>
> - if (!--dev->users && dev->close)
> - dev->close(dev);
> + if (!--dev->users) {
> + if (dev->poller)
> + input_dev_poller_stop(dev->poller);
> +
> + if (dev->close)
> + dev->close(dev);
> + }
>
> if (!--handle->open) {
> /*
> @@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
> &input_dev_attr_group,
> &input_dev_id_attr_group,
> &input_dev_caps_attr_group,
> + &input_poller_attribute_group,
> NULL
> };
>
> @@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)
>
> input_ff_destroy(dev);
> input_mt_destroy_slots(dev);
> + kfree(dev->poller);
> kfree(dev->absinfo);
> kfree(dev->vals);
> kfree(dev);
> @@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
> if (!dev->setkeycode)
> dev->setkeycode = input_default_setkeycode;
>
> + if (dev->poller)
> + input_dev_poller_finalize(dev->poller);
> +
> error = device_add(&dev->dev);
> if (error)
> goto err_free_vals;
> diff --git a/include/linux/input.h b/include/linux/input.h
> index e95a439d8bd5..94f277cd806a 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -21,6 +21,8 @@
> #include <linux/timer.h>
> #include <linux/mod_devicetable.h>
>
> +struct input_dev_poller;
> +
> /**
> * struct input_value - input value representation
> * @type: type of value (EV_KEY, EV_ABS, etc)
> @@ -71,6 +73,8 @@ enum input_clock_type {
> * not sleep
> * @ff: force feedback structure associated with the device if device
> * supports force feedback effects
> + * @poller: poller structure associated with the device if device is
> + * set up to use polling mode
> * @repeat_key: stores key code of the last key pressed; used to implement
> * software autorepeat
> * @timer: timer for software autorepeat
> @@ -156,6 +160,8 @@ struct input_dev {
>
> struct ff_device *ff;
>
> + struct input_dev_poller *poller;
> +
> unsigned int repeat_key;
> struct timer_list timer;
>
> @@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);
>
> void input_reset_device(struct input_dev *);
>
> +int input_setup_polling(struct input_dev *dev,
> + void (*poll_fn)(struct input_dev *dev));
> +void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
> +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
> +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
> +
> int __must_check input_register_handler(struct input_handler *);
> void input_unregister_handler(struct input_handler *);
>
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: add support for polling to input devices
2019-08-12 16:50 ` Benjamin Tissoires
@ 2019-08-12 17:11 ` Dmitry Torokhov
2019-08-13 14:04 ` Benjamin Tissoires
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2019-08-12 17:11 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: open list:HID CORE LAYER, lkml, Rob Herring
Hi Benjamin,
On Mon, Aug 12, 2019 at 06:50:38PM +0200, Benjamin Tissoires wrote:
> Hi Dmitry,
>
> On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Separating "normal" and "polled" input devices was a mistake, as often we
> > want to allow the very same device work on both interrupt-driven and
> > polled mode, depending on the board on which the device is used.
> >
> > This introduces new APIs:
> >
> > - input_setup_polling
> > - input_set_poll_interval
> > - input_set_min_poll_interval
> > - input_set_max_poll_interval
> >
> > These new APIs allow switching an input device into polled mode with sysfs
> > attributes matching drivers using input_polled_dev APIs that will be
> > eventually removed.
>
> Are you sure that using sysfs is the correct way here?
> I would think using generic properties that can be overwritten by the
> DSDT or by the device tree would make things easier from a driver
> point of view.
Couple of points: I wanted it to be compatible with input-polldev.c so
the sysfs attributes must match (so that we can convert existing drivers
and zap input-polldev). I also am not sure if polling parameters are
property of board, or it is either fundamental hardware limitation or
simply desired system behavior. If Rob is OK with adding device
properties I'd be fine adding them as a followup, otherwise we can have
udev adjust the behavior as needed for given box shortly after boot.
>
> Also, checkpatch complains about a few octal permissions that are
> preferred over symbolic ones, and there is a possible 'out of memory'
> nessage at drivers/input/input-poller.c:75.
Yes, there is. It is there so we would know what device we were trying
to set up when OOM happened. You can probable decipher the driver from
the stack trace, but figuring particular device instance is harder.
Thanks.
>
> Cheers,
> Benjamin
>
> >
> > Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/Makefile | 2 +-
> > drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
> > drivers/input/input-poller.h | 18 +++
> > drivers/input/input.c | 36 ++++--
> > include/linux/input.h | 12 ++
> > 5 files changed, 268 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> > index 40de6a7be641..e35650930371 100644
> > --- a/drivers/input/Makefile
> > +++ b/drivers/input/Makefile
> > @@ -6,7 +6,7 @@
> > # Each configuration option enables a list of files.
> >
> > obj-$(CONFIG_INPUT) += input-core.o
> > -input-core-y := input.o input-compat.o input-mt.o ff-core.o
> > +input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
> >
> > obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> > obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
> > diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
> > new file mode 100644
> > index 000000000000..e041adb04f5a
> > --- /dev/null
> > +++ b/drivers/input/input-poller.c
> > @@ -0,0 +1,208 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Support for polling mode for input devices.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/workqueue.h>
> > +#include "input-poller.h"
> > +
> > +struct input_dev_poller {
> > + void (*poll)(struct input_dev *dev);
> > +
> > + unsigned int poll_interval; /* msec */
> > + unsigned int poll_interval_max; /* msec */
> > + unsigned int poll_interval_min; /* msec */
> > +
> > + struct input_dev *input;
> > + struct delayed_work work;
> > +};
> > +
> > +static void input_dev_poller_queue_work(struct input_dev_poller *poller)
> > +{
> > + unsigned long delay;
> > +
> > + delay = msecs_to_jiffies(poller->poll_interval);
> > + if (delay >= HZ)
> > + delay = round_jiffies_relative(delay);
> > +
> > + queue_delayed_work(system_freezable_wq, &poller->work, delay);
> > +}
> > +
> > +static void input_dev_poller_work(struct work_struct *work)
> > +{
> > + struct input_dev_poller *poller =
> > + container_of(work, struct input_dev_poller, work.work);
> > +
> > + poller->poll(poller->input);
> > + input_dev_poller_queue_work(poller);
> > +}
> > +
> > +void input_dev_poller_finalize(struct input_dev_poller *poller)
> > +{
> > + if (!poller->poll_interval)
> > + poller->poll_interval = 500;
> > + if (!poller->poll_interval_max)
> > + poller->poll_interval_max = poller->poll_interval;
> > +}
> > +
> > +void input_dev_poller_start(struct input_dev_poller *poller)
> > +{
> > + /* Only start polling if polling is enabled */
> > + if (poller->poll_interval > 0) {
> > + poller->poll(poller->input);
> > + input_dev_poller_queue_work(poller);
> > + }
> > +}
> > +
> > +void input_dev_poller_stop(struct input_dev_poller *poller)
> > +{
> > + cancel_delayed_work_sync(&poller->work);
> > +}
> > +
> > +int input_setup_polling(struct input_dev *dev,
> > + void (*poll_fn)(struct input_dev *dev))
> > +{
> > + struct input_dev_poller *poller;
> > +
> > + poller = kzalloc(sizeof(*poller), GFP_KERNEL);
> > + if (!poller) {
> > + dev_err(dev->dev.parent ?: &dev->dev,
> > + "%s: unable to allocate poller structure\n", __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
> > + poller->input = dev;
> > + poller->poll = poll_fn;
> > +
> > + dev->poller = poller;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(input_setup_polling);
> > +
> > +static bool input_dev_ensure_poller(struct input_dev *dev)
> > +{
> > + if (!dev->poller) {
> > + dev_err(dev->dev.parent ?: &dev->dev,
> > + "poller structure has not been set up\n");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
> > +{
> > + if (input_dev_ensure_poller(dev))
> > + dev->poller->poll_interval = interval;
> > +}
> > +EXPORT_SYMBOL(input_set_poll_interval);
> > +
> > +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
> > +{
> > + if (input_dev_ensure_poller(dev))
> > + dev->poller->poll_interval_min = interval;
> > +}
> > +EXPORT_SYMBOL(input_set_min_poll_interval);
> > +
> > +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
> > +{
> > + if (input_dev_ensure_poller(dev))
> > + dev->poller->poll_interval_max = interval;
> > +}
> > +EXPORT_SYMBOL(input_set_max_poll_interval);
> > +
> > +/* SYSFS interface */
> > +
> > +static ssize_t input_dev_get_poll_interval(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return sprintf(buf, "%d\n", input->poller->poll_interval);
> > +}
> > +
> > +static ssize_t input_dev_set_poll_interval(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > + struct input_dev_poller *poller = input->poller;
> > + unsigned int interval;
> > + int err;
> > +
> > + err = kstrtouint(buf, 0, &interval);
> > + if (err)
> > + return err;
> > +
> > + if (interval < poller->poll_interval_min)
> > + return -EINVAL;
> > +
> > + if (interval > poller->poll_interval_max)
> > + return -EINVAL;
> > +
> > + mutex_lock(&input->mutex);
> > +
> > + poller->poll_interval = interval;
> > +
> > + if (input->users) {
> > + cancel_delayed_work_sync(&poller->work);
> > + if (poller->poll_interval > 0)
> > + input_dev_poller_queue_work(poller);
> > + }
> > +
> > + mutex_unlock(&input->mutex);
> > +
> > + return count;
> > +}
> > +
> > +static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
> > + input_dev_get_poll_interval, input_dev_set_poll_interval);
> > +
> > +static ssize_t input_dev_get_poll_max(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return sprintf(buf, "%d\n", input->poller->poll_interval_max);
> > +}
> > +
> > +static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
> > +
> > +static ssize_t input_dev_get_poll_min(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return sprintf(buf, "%d\n", input->poller->poll_interval_min);
> > +}
> > +
> > +static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
> > +
> > +static umode_t input_poller_attrs_visible(struct kobject *kobj,
> > + struct attribute *attr, int n)
> > +{
> > + struct device *dev = kobj_to_dev(kobj);
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return input->poller ? attr->mode : 0;
> > +}
> > +
> > +static struct attribute *input_poller_attrs[] = {
> > + &dev_attr_poll.attr,
> > + &dev_attr_max.attr,
> > + &dev_attr_min.attr,
> > + NULL
> > +};
> > +
> > +struct attribute_group input_poller_attribute_group = {
> > + .is_visible = input_poller_attrs_visible,
> > + .attrs = input_poller_attrs,
> > +};
> > diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
> > new file mode 100644
> > index 000000000000..e3fca0be1d32
> > --- /dev/null
> > +++ b/drivers/input/input-poller.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef _INPUT_POLLER_H
> > +#define _INPUT_POLLER_H
> > +
> > +/*
> > + * Support for polling mode for input devices.
> > + */
> > +#include <linux/sysfs.h>
> > +
> > +struct input_dev_poller;
> > +
> > +void input_dev_poller_finalize(struct input_dev_poller *poller);
> > +void input_dev_poller_start(struct input_dev_poller *poller);
> > +void input_dev_poller_stop(struct input_dev_poller *poller);
> > +
> > +extern struct attribute_group input_poller_attribute_group;
> > +
> > +#endif /* _INPUT_POLLER_H */
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index 7494a0dede79..c08aa3596144 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -24,6 +24,7 @@
> > #include <linux/mutex.h>
> > #include <linux/rcupdate.h>
> > #include "input-compat.h"
> > +#include "input-poller.h"
> >
> > MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> > MODULE_DESCRIPTION("Input core");
> > @@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)
> >
> > handle->open++;
> >
> > - if (!dev->users++ && dev->open)
> > - retval = dev->open(dev);
> > + if (dev->users++) {
> > + /*
> > + * Device is already opened, so we can exit immediately and
> > + * report success.
> > + */
> > + goto out;
> > + }
> >
> > - if (retval) {
> > - dev->users--;
> > - if (!--handle->open) {
> > + if (dev->open) {
> > + retval = dev->open(dev);
> > + if (retval) {
> > + dev->users--;
> > + handle->open--;
> > /*
> > * Make sure we are not delivering any more events
> > * through this handle
> > */
> > synchronize_rcu();
> > + goto out;
> > }
> > }
> >
> > + if (dev->poller)
> > + input_dev_poller_start(dev->poller);
> > +
> > out:
> > mutex_unlock(&dev->mutex);
> > return retval;
> > @@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)
> >
> > __input_release_device(handle);
> >
> > - if (!--dev->users && dev->close)
> > - dev->close(dev);
> > + if (!--dev->users) {
> > + if (dev->poller)
> > + input_dev_poller_stop(dev->poller);
> > +
> > + if (dev->close)
> > + dev->close(dev);
> > + }
> >
> > if (!--handle->open) {
> > /*
> > @@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
> > &input_dev_attr_group,
> > &input_dev_id_attr_group,
> > &input_dev_caps_attr_group,
> > + &input_poller_attribute_group,
> > NULL
> > };
> >
> > @@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)
> >
> > input_ff_destroy(dev);
> > input_mt_destroy_slots(dev);
> > + kfree(dev->poller);
> > kfree(dev->absinfo);
> > kfree(dev->vals);
> > kfree(dev);
> > @@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
> > if (!dev->setkeycode)
> > dev->setkeycode = input_default_setkeycode;
> >
> > + if (dev->poller)
> > + input_dev_poller_finalize(dev->poller);
> > +
> > error = device_add(&dev->dev);
> > if (error)
> > goto err_free_vals;
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index e95a439d8bd5..94f277cd806a 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -21,6 +21,8 @@
> > #include <linux/timer.h>
> > #include <linux/mod_devicetable.h>
> >
> > +struct input_dev_poller;
> > +
> > /**
> > * struct input_value - input value representation
> > * @type: type of value (EV_KEY, EV_ABS, etc)
> > @@ -71,6 +73,8 @@ enum input_clock_type {
> > * not sleep
> > * @ff: force feedback structure associated with the device if device
> > * supports force feedback effects
> > + * @poller: poller structure associated with the device if device is
> > + * set up to use polling mode
> > * @repeat_key: stores key code of the last key pressed; used to implement
> > * software autorepeat
> > * @timer: timer for software autorepeat
> > @@ -156,6 +160,8 @@ struct input_dev {
> >
> > struct ff_device *ff;
> >
> > + struct input_dev_poller *poller;
> > +
> > unsigned int repeat_key;
> > struct timer_list timer;
> >
> > @@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);
> >
> > void input_reset_device(struct input_dev *);
> >
> > +int input_setup_polling(struct input_dev *dev,
> > + void (*poll_fn)(struct input_dev *dev));
> > +void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
> > +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
> > +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
> > +
> > int __must_check input_register_handler(struct input_handler *);
> > void input_unregister_handler(struct input_handler *);
> >
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
> >
> > --
> > Dmitry
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: add support for polling to input devices
2019-08-12 17:11 ` Dmitry Torokhov
@ 2019-08-13 14:04 ` Benjamin Tissoires
2019-08-20 11:56 ` Michal Vokáč
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2019-08-13 14:04 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, lkml, Rob Herring
On Mon, Aug 12, 2019 at 7:11 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Benjamin,
>
> On Mon, Aug 12, 2019 at 06:50:38PM +0200, Benjamin Tissoires wrote:
> > Hi Dmitry,
> >
> > On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Separating "normal" and "polled" input devices was a mistake, as often we
> > > want to allow the very same device work on both interrupt-driven and
> > > polled mode, depending on the board on which the device is used.
> > >
> > > This introduces new APIs:
> > >
> > > - input_setup_polling
> > > - input_set_poll_interval
> > > - input_set_min_poll_interval
> > > - input_set_max_poll_interval
> > >
> > > These new APIs allow switching an input device into polled mode with sysfs
> > > attributes matching drivers using input_polled_dev APIs that will be
> > > eventually removed.
> >
> > Are you sure that using sysfs is the correct way here?
> > I would think using generic properties that can be overwritten by the
> > DSDT or by the device tree would make things easier from a driver
> > point of view.
>
> Couple of points: I wanted it to be compatible with input-polldev.c so
> the sysfs attributes must match (so that we can convert existing drivers
> and zap input-polldev).
Oh, I missed that. Good point.
> I also am not sure if polling parameters are
> property of board, or it is either fundamental hardware limitation or
> simply desired system behavior.
I think it's a combination of everything: sometimes the board misses
the capability to not do IRQs for that device, and using properties
would be better here: you can define them where you need (board,
platform or device level), and have a working platfrom from the kernel
description entirely.
However, it doesn't solve the issue of input-polldev, so maybe
properties should be added on top of this sysfs.
> If Rob is OK with adding device
> properties I'd be fine adding them as a followup, otherwise we can have
> udev adjust the behavior as needed for given box shortly after boot.
Fair enough.
>
> >
> > Also, checkpatch complains about a few octal permissions that are
> > preferred over symbolic ones, and there is a possible 'out of memory'
> > nessage at drivers/input/input-poller.c:75.
>
> Yes, there is. It is there so we would know what device we were trying
> to set up when OOM happened. You can probable decipher the driver from
> the stack trace, but figuring particular device instance is harder.
Could you add a comment there explaining this choice? I have a feeling
you'll have to refuse a few patches of people running coccinelle
scripts and be too happy to send a kernel patch.
Other than that:
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
>
> Thanks.
>
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > > drivers/input/Makefile | 2 +-
> > > drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
> > > drivers/input/input-poller.h | 18 +++
> > > drivers/input/input.c | 36 ++++--
> > > include/linux/input.h | 12 ++
> > > 5 files changed, 268 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> > > index 40de6a7be641..e35650930371 100644
> > > --- a/drivers/input/Makefile
> > > +++ b/drivers/input/Makefile
> > > @@ -6,7 +6,7 @@
> > > # Each configuration option enables a list of files.
> > >
> > > obj-$(CONFIG_INPUT) += input-core.o
> > > -input-core-y := input.o input-compat.o input-mt.o ff-core.o
> > > +input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
> > >
> > > obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> > > obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
> > > diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
> > > new file mode 100644
> > > index 000000000000..e041adb04f5a
> > > --- /dev/null
> > > +++ b/drivers/input/input-poller.c
> > > @@ -0,0 +1,208 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Support for polling mode for input devices.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/input.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/types.h>
> > > +#include <linux/workqueue.h>
> > > +#include "input-poller.h"
> > > +
> > > +struct input_dev_poller {
> > > + void (*poll)(struct input_dev *dev);
> > > +
> > > + unsigned int poll_interval; /* msec */
> > > + unsigned int poll_interval_max; /* msec */
> > > + unsigned int poll_interval_min; /* msec */
> > > +
> > > + struct input_dev *input;
> > > + struct delayed_work work;
> > > +};
> > > +
> > > +static void input_dev_poller_queue_work(struct input_dev_poller *poller)
> > > +{
> > > + unsigned long delay;
> > > +
> > > + delay = msecs_to_jiffies(poller->poll_interval);
> > > + if (delay >= HZ)
> > > + delay = round_jiffies_relative(delay);
> > > +
> > > + queue_delayed_work(system_freezable_wq, &poller->work, delay);
> > > +}
> > > +
> > > +static void input_dev_poller_work(struct work_struct *work)
> > > +{
> > > + struct input_dev_poller *poller =
> > > + container_of(work, struct input_dev_poller, work.work);
> > > +
> > > + poller->poll(poller->input);
> > > + input_dev_poller_queue_work(poller);
> > > +}
> > > +
> > > +void input_dev_poller_finalize(struct input_dev_poller *poller)
> > > +{
> > > + if (!poller->poll_interval)
> > > + poller->poll_interval = 500;
> > > + if (!poller->poll_interval_max)
> > > + poller->poll_interval_max = poller->poll_interval;
> > > +}
> > > +
> > > +void input_dev_poller_start(struct input_dev_poller *poller)
> > > +{
> > > + /* Only start polling if polling is enabled */
> > > + if (poller->poll_interval > 0) {
> > > + poller->poll(poller->input);
> > > + input_dev_poller_queue_work(poller);
> > > + }
> > > +}
> > > +
> > > +void input_dev_poller_stop(struct input_dev_poller *poller)
> > > +{
> > > + cancel_delayed_work_sync(&poller->work);
> > > +}
> > > +
> > > +int input_setup_polling(struct input_dev *dev,
> > > + void (*poll_fn)(struct input_dev *dev))
> > > +{
> > > + struct input_dev_poller *poller;
> > > +
> > > + poller = kzalloc(sizeof(*poller), GFP_KERNEL);
> > > + if (!poller) {
> > > + dev_err(dev->dev.parent ?: &dev->dev,
> > > + "%s: unable to allocate poller structure\n", __func__);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
> > > + poller->input = dev;
> > > + poller->poll = poll_fn;
> > > +
> > > + dev->poller = poller;
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(input_setup_polling);
> > > +
> > > +static bool input_dev_ensure_poller(struct input_dev *dev)
> > > +{
> > > + if (!dev->poller) {
> > > + dev_err(dev->dev.parent ?: &dev->dev,
> > > + "poller structure has not been set up\n");
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
> > > +{
> > > + if (input_dev_ensure_poller(dev))
> > > + dev->poller->poll_interval = interval;
> > > +}
> > > +EXPORT_SYMBOL(input_set_poll_interval);
> > > +
> > > +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
> > > +{
> > > + if (input_dev_ensure_poller(dev))
> > > + dev->poller->poll_interval_min = interval;
> > > +}
> > > +EXPORT_SYMBOL(input_set_min_poll_interval);
> > > +
> > > +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
> > > +{
> > > + if (input_dev_ensure_poller(dev))
> > > + dev->poller->poll_interval_max = interval;
> > > +}
> > > +EXPORT_SYMBOL(input_set_max_poll_interval);
> > > +
> > > +/* SYSFS interface */
> > > +
> > > +static ssize_t input_dev_get_poll_interval(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct input_dev *input = to_input_dev(dev);
> > > +
> > > + return sprintf(buf, "%d\n", input->poller->poll_interval);
> > > +}
> > > +
> > > +static ssize_t input_dev_set_poll_interval(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct input_dev *input = to_input_dev(dev);
> > > + struct input_dev_poller *poller = input->poller;
> > > + unsigned int interval;
> > > + int err;
> > > +
> > > + err = kstrtouint(buf, 0, &interval);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (interval < poller->poll_interval_min)
> > > + return -EINVAL;
> > > +
> > > + if (interval > poller->poll_interval_max)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&input->mutex);
> > > +
> > > + poller->poll_interval = interval;
> > > +
> > > + if (input->users) {
> > > + cancel_delayed_work_sync(&poller->work);
> > > + if (poller->poll_interval > 0)
> > > + input_dev_poller_queue_work(poller);
> > > + }
> > > +
> > > + mutex_unlock(&input->mutex);
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
> > > + input_dev_get_poll_interval, input_dev_set_poll_interval);
> > > +
> > > +static ssize_t input_dev_get_poll_max(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct input_dev *input = to_input_dev(dev);
> > > +
> > > + return sprintf(buf, "%d\n", input->poller->poll_interval_max);
> > > +}
> > > +
> > > +static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
> > > +
> > > +static ssize_t input_dev_get_poll_min(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct input_dev *input = to_input_dev(dev);
> > > +
> > > + return sprintf(buf, "%d\n", input->poller->poll_interval_min);
> > > +}
> > > +
> > > +static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
> > > +
> > > +static umode_t input_poller_attrs_visible(struct kobject *kobj,
> > > + struct attribute *attr, int n)
> > > +{
> > > + struct device *dev = kobj_to_dev(kobj);
> > > + struct input_dev *input = to_input_dev(dev);
> > > +
> > > + return input->poller ? attr->mode : 0;
> > > +}
> > > +
> > > +static struct attribute *input_poller_attrs[] = {
> > > + &dev_attr_poll.attr,
> > > + &dev_attr_max.attr,
> > > + &dev_attr_min.attr,
> > > + NULL
> > > +};
> > > +
> > > +struct attribute_group input_poller_attribute_group = {
> > > + .is_visible = input_poller_attrs_visible,
> > > + .attrs = input_poller_attrs,
> > > +};
> > > diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
> > > new file mode 100644
> > > index 000000000000..e3fca0be1d32
> > > --- /dev/null
> > > +++ b/drivers/input/input-poller.h
> > > @@ -0,0 +1,18 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +#ifndef _INPUT_POLLER_H
> > > +#define _INPUT_POLLER_H
> > > +
> > > +/*
> > > + * Support for polling mode for input devices.
> > > + */
> > > +#include <linux/sysfs.h>
> > > +
> > > +struct input_dev_poller;
> > > +
> > > +void input_dev_poller_finalize(struct input_dev_poller *poller);
> > > +void input_dev_poller_start(struct input_dev_poller *poller);
> > > +void input_dev_poller_stop(struct input_dev_poller *poller);
> > > +
> > > +extern struct attribute_group input_poller_attribute_group;
> > > +
> > > +#endif /* _INPUT_POLLER_H */
> > > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > > index 7494a0dede79..c08aa3596144 100644
> > > --- a/drivers/input/input.c
> > > +++ b/drivers/input/input.c
> > > @@ -24,6 +24,7 @@
> > > #include <linux/mutex.h>
> > > #include <linux/rcupdate.h>
> > > #include "input-compat.h"
> > > +#include "input-poller.h"
> > >
> > > MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> > > MODULE_DESCRIPTION("Input core");
> > > @@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)
> > >
> > > handle->open++;
> > >
> > > - if (!dev->users++ && dev->open)
> > > - retval = dev->open(dev);
> > > + if (dev->users++) {
> > > + /*
> > > + * Device is already opened, so we can exit immediately and
> > > + * report success.
> > > + */
> > > + goto out;
> > > + }
> > >
> > > - if (retval) {
> > > - dev->users--;
> > > - if (!--handle->open) {
> > > + if (dev->open) {
> > > + retval = dev->open(dev);
> > > + if (retval) {
> > > + dev->users--;
> > > + handle->open--;
> > > /*
> > > * Make sure we are not delivering any more events
> > > * through this handle
> > > */
> > > synchronize_rcu();
> > > + goto out;
> > > }
> > > }
> > >
> > > + if (dev->poller)
> > > + input_dev_poller_start(dev->poller);
> > > +
> > > out:
> > > mutex_unlock(&dev->mutex);
> > > return retval;
> > > @@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)
> > >
> > > __input_release_device(handle);
> > >
> > > - if (!--dev->users && dev->close)
> > > - dev->close(dev);
> > > + if (!--dev->users) {
> > > + if (dev->poller)
> > > + input_dev_poller_stop(dev->poller);
> > > +
> > > + if (dev->close)
> > > + dev->close(dev);
> > > + }
> > >
> > > if (!--handle->open) {
> > > /*
> > > @@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
> > > &input_dev_attr_group,
> > > &input_dev_id_attr_group,
> > > &input_dev_caps_attr_group,
> > > + &input_poller_attribute_group,
> > > NULL
> > > };
> > >
> > > @@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)
> > >
> > > input_ff_destroy(dev);
> > > input_mt_destroy_slots(dev);
> > > + kfree(dev->poller);
> > > kfree(dev->absinfo);
> > > kfree(dev->vals);
> > > kfree(dev);
> > > @@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
> > > if (!dev->setkeycode)
> > > dev->setkeycode = input_default_setkeycode;
> > >
> > > + if (dev->poller)
> > > + input_dev_poller_finalize(dev->poller);
> > > +
> > > error = device_add(&dev->dev);
> > > if (error)
> > > goto err_free_vals;
> > > diff --git a/include/linux/input.h b/include/linux/input.h
> > > index e95a439d8bd5..94f277cd806a 100644
> > > --- a/include/linux/input.h
> > > +++ b/include/linux/input.h
> > > @@ -21,6 +21,8 @@
> > > #include <linux/timer.h>
> > > #include <linux/mod_devicetable.h>
> > >
> > > +struct input_dev_poller;
> > > +
> > > /**
> > > * struct input_value - input value representation
> > > * @type: type of value (EV_KEY, EV_ABS, etc)
> > > @@ -71,6 +73,8 @@ enum input_clock_type {
> > > * not sleep
> > > * @ff: force feedback structure associated with the device if device
> > > * supports force feedback effects
> > > + * @poller: poller structure associated with the device if device is
> > > + * set up to use polling mode
> > > * @repeat_key: stores key code of the last key pressed; used to implement
> > > * software autorepeat
> > > * @timer: timer for software autorepeat
> > > @@ -156,6 +160,8 @@ struct input_dev {
> > >
> > > struct ff_device *ff;
> > >
> > > + struct input_dev_poller *poller;
> > > +
> > > unsigned int repeat_key;
> > > struct timer_list timer;
> > >
> > > @@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);
> > >
> > > void input_reset_device(struct input_dev *);
> > >
> > > +int input_setup_polling(struct input_dev *dev,
> > > + void (*poll_fn)(struct input_dev *dev));
> > > +void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
> > > +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
> > > +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
> > > +
> > > int __must_check input_register_handler(struct input_handler *);
> > > void input_unregister_handler(struct input_handler *);
> > >
> > > --
> > > 2.23.0.rc1.153.gdeed80330f-goog
> > >
> > >
> > > --
> > > Dmitry
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: add support for polling to input devices
2019-08-13 14:04 ` Benjamin Tissoires
@ 2019-08-20 11:56 ` Michal Vokáč
2019-08-20 19:05 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Michal Vokáč @ 2019-08-20 11:56 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, open list:HID CORE LAYER, lkml, Rob Herring
On 13. 08. 19 16:04, Benjamin Tissoires wrote:
> On Mon, Aug 12, 2019 at 7:11 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> Hi Benjamin,
>>
>> On Mon, Aug 12, 2019 at 06:50:38PM +0200, Benjamin Tissoires wrote:
>>> Hi Dmitry,
>>>
>>> On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> Separating "normal" and "polled" input devices was a mistake, as often we
>>>> want to allow the very same device work on both interrupt-driven and
>>>> polled mode, depending on the board on which the device is used.
>>>>
>>>> This introduces new APIs:
>>>>
>>>> - input_setup_polling
>>>> - input_set_poll_interval
>>>> - input_set_min_poll_interval
>>>> - input_set_max_poll_interval
>>>>
>>>> These new APIs allow switching an input device into polled mode with sysfs
>>>> attributes matching drivers using input_polled_dev APIs that will be
>>>> eventually removed.
>>>
>>> Are you sure that using sysfs is the correct way here?
>>> I would think using generic properties that can be overwritten by the
>>> DSDT or by the device tree would make things easier from a driver
>>> point of view.
>>
>> Couple of points: I wanted it to be compatible with input-polldev.c so
>> the sysfs attributes must match (so that we can convert existing drivers
>> and zap input-polldev).
>
> Oh, I missed that. Good point.
>
>> I also am not sure if polling parameters are
>> property of board, or it is either fundamental hardware limitation or
>> simply desired system behavior.
>
> I think it's a combination of everything: sometimes the board misses
> the capability to not do IRQs for that device, and using properties
> would be better here: you can define them where you need (board,
> platform or device level), and have a working platfrom from the kernel
> description entirely.
> However, it doesn't solve the issue of input-polldev, so maybe
> properties should be added on top of this sysfs.
>
>> If Rob is OK with adding device
>> properties I'd be fine adding them as a followup, otherwise we can have
>> udev adjust the behavior as needed for given box shortly after boot.
>
> Fair enough.
>
>>
>>>
>>> Also, checkpatch complains about a few octal permissions that are
>>> preferred over symbolic ones, and there is a possible 'out of memory'
>>> nessage at drivers/input/input-poller.c:75.
>>
>> Yes, there is. It is there so we would know what device we were trying
>> to set up when OOM happened. You can probable decipher the driver from
>> the stack trace, but figuring particular device instance is harder.
>
> Could you add a comment there explaining this choice? I have a feeling
> you'll have to refuse a few patches of people running coccinelle
> scripts and be too happy to send a kernel patch.
>
> Other than that:
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
Hi Dmitry,
what is the status of this patch? Are we still waiting for Rob to
comment on the device properties or is this ready to land?
Little bit OT question: what tree/branch do you use to apply patches?
According to the mailing list you recently applied some patches but
I can not find them here [1].
Thank you,
Michal
[1] https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/
>>>>
>>>> Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>> ---
>>>> drivers/input/Makefile | 2 +-
>>>> drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
>>>> drivers/input/input-poller.h | 18 +++
>>>> drivers/input/input.c | 36 ++++--
>>>> include/linux/input.h | 12 ++
>>>> 5 files changed, 268 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
>>>> index 40de6a7be641..e35650930371 100644
>>>> --- a/drivers/input/Makefile
>>>> +++ b/drivers/input/Makefile
>>>> @@ -6,7 +6,7 @@
>>>> # Each configuration option enables a list of files.
>>>>
>>>> obj-$(CONFIG_INPUT) += input-core.o
>>>> -input-core-y := input.o input-compat.o input-mt.o ff-core.o
>>>> +input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
>>>>
>>>> obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
>>>> obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
>>>> diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
>>>> new file mode 100644
>>>> index 000000000000..e041adb04f5a
>>>> --- /dev/null
>>>> +++ b/drivers/input/input-poller.c
>>>> @@ -0,0 +1,208 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Support for polling mode for input devices.
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/input.h>
>>>> +#include <linux/jiffies.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/workqueue.h>
>>>> +#include "input-poller.h"
>>>> +
>>>> +struct input_dev_poller {
>>>> + void (*poll)(struct input_dev *dev);
>>>> +
>>>> + unsigned int poll_interval; /* msec */
>>>> + unsigned int poll_interval_max; /* msec */
>>>> + unsigned int poll_interval_min; /* msec */
>>>> +
>>>> + struct input_dev *input;
>>>> + struct delayed_work work;
>>>> +};
>>>> +
>>>> +static void input_dev_poller_queue_work(struct input_dev_poller *poller)
>>>> +{
>>>> + unsigned long delay;
>>>> +
>>>> + delay = msecs_to_jiffies(poller->poll_interval);
>>>> + if (delay >= HZ)
>>>> + delay = round_jiffies_relative(delay);
>>>> +
>>>> + queue_delayed_work(system_freezable_wq, &poller->work, delay);
>>>> +}
>>>> +
>>>> +static void input_dev_poller_work(struct work_struct *work)
>>>> +{
>>>> + struct input_dev_poller *poller =
>>>> + container_of(work, struct input_dev_poller, work.work);
>>>> +
>>>> + poller->poll(poller->input);
>>>> + input_dev_poller_queue_work(poller);
>>>> +}
>>>> +
>>>> +void input_dev_poller_finalize(struct input_dev_poller *poller)
>>>> +{
>>>> + if (!poller->poll_interval)
>>>> + poller->poll_interval = 500;
>>>> + if (!poller->poll_interval_max)
>>>> + poller->poll_interval_max = poller->poll_interval;
>>>> +}
>>>> +
>>>> +void input_dev_poller_start(struct input_dev_poller *poller)
>>>> +{
>>>> + /* Only start polling if polling is enabled */
>>>> + if (poller->poll_interval > 0) {
>>>> + poller->poll(poller->input);
>>>> + input_dev_poller_queue_work(poller);
>>>> + }
>>>> +}
>>>> +
>>>> +void input_dev_poller_stop(struct input_dev_poller *poller)
>>>> +{
>>>> + cancel_delayed_work_sync(&poller->work);
>>>> +}
>>>> +
>>>> +int input_setup_polling(struct input_dev *dev,
>>>> + void (*poll_fn)(struct input_dev *dev))
>>>> +{
>>>> + struct input_dev_poller *poller;
>>>> +
>>>> + poller = kzalloc(sizeof(*poller), GFP_KERNEL);
>>>> + if (!poller) {
>>>> + dev_err(dev->dev.parent ?: &dev->dev,
>>>> + "%s: unable to allocate poller structure\n", __func__);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
>>>> + poller->input = dev;
>>>> + poller->poll = poll_fn;
>>>> +
>>>> + dev->poller = poller;
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(input_setup_polling);
>>>> +
>>>> +static bool input_dev_ensure_poller(struct input_dev *dev)
>>>> +{
>>>> + if (!dev->poller) {
>>>> + dev_err(dev->dev.parent ?: &dev->dev,
>>>> + "poller structure has not been set up\n");
>>>> + return false;
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
>>>> +{
>>>> + if (input_dev_ensure_poller(dev))
>>>> + dev->poller->poll_interval = interval;
>>>> +}
>>>> +EXPORT_SYMBOL(input_set_poll_interval);
>>>> +
>>>> +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
>>>> +{
>>>> + if (input_dev_ensure_poller(dev))
>>>> + dev->poller->poll_interval_min = interval;
>>>> +}
>>>> +EXPORT_SYMBOL(input_set_min_poll_interval);
>>>> +
>>>> +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
>>>> +{
>>>> + if (input_dev_ensure_poller(dev))
>>>> + dev->poller->poll_interval_max = interval;
>>>> +}
>>>> +EXPORT_SYMBOL(input_set_max_poll_interval);
>>>> +
>>>> +/* SYSFS interface */
>>>> +
>>>> +static ssize_t input_dev_get_poll_interval(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct input_dev *input = to_input_dev(dev);
>>>> +
>>>> + return sprintf(buf, "%d\n", input->poller->poll_interval);
>>>> +}
>>>> +
>>>> +static ssize_t input_dev_set_poll_interval(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + const char *buf, size_t count)
>>>> +{
>>>> + struct input_dev *input = to_input_dev(dev);
>>>> + struct input_dev_poller *poller = input->poller;
>>>> + unsigned int interval;
>>>> + int err;
>>>> +
>>>> + err = kstrtouint(buf, 0, &interval);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + if (interval < poller->poll_interval_min)
>>>> + return -EINVAL;
>>>> +
>>>> + if (interval > poller->poll_interval_max)
>>>> + return -EINVAL;
>>>> +
>>>> + mutex_lock(&input->mutex);
>>>> +
>>>> + poller->poll_interval = interval;
>>>> +
>>>> + if (input->users) {
>>>> + cancel_delayed_work_sync(&poller->work);
>>>> + if (poller->poll_interval > 0)
>>>> + input_dev_poller_queue_work(poller);
>>>> + }
>>>> +
>>>> + mutex_unlock(&input->mutex);
>>>> +
>>>> + return count;
>>>> +}
>>>> +
>>>> +static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
>>>> + input_dev_get_poll_interval, input_dev_set_poll_interval);
>>>> +
>>>> +static ssize_t input_dev_get_poll_max(struct device *dev,
>>>> + struct device_attribute *attr, char *buf)
>>>> +{
>>>> + struct input_dev *input = to_input_dev(dev);
>>>> +
>>>> + return sprintf(buf, "%d\n", input->poller->poll_interval_max);
>>>> +}
>>>> +
>>>> +static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
>>>> +
>>>> +static ssize_t input_dev_get_poll_min(struct device *dev,
>>>> + struct device_attribute *attr, char *buf)
>>>> +{
>>>> + struct input_dev *input = to_input_dev(dev);
>>>> +
>>>> + return sprintf(buf, "%d\n", input->poller->poll_interval_min);
>>>> +}
>>>> +
>>>> +static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
>>>> +
>>>> +static umode_t input_poller_attrs_visible(struct kobject *kobj,
>>>> + struct attribute *attr, int n)
>>>> +{
>>>> + struct device *dev = kobj_to_dev(kobj);
>>>> + struct input_dev *input = to_input_dev(dev);
>>>> +
>>>> + return input->poller ? attr->mode : 0;
>>>> +}
>>>> +
>>>> +static struct attribute *input_poller_attrs[] = {
>>>> + &dev_attr_poll.attr,
>>>> + &dev_attr_max.attr,
>>>> + &dev_attr_min.attr,
>>>> + NULL
>>>> +};
>>>> +
>>>> +struct attribute_group input_poller_attribute_group = {
>>>> + .is_visible = input_poller_attrs_visible,
>>>> + .attrs = input_poller_attrs,
>>>> +};
>>>> diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
>>>> new file mode 100644
>>>> index 000000000000..e3fca0be1d32
>>>> --- /dev/null
>>>> +++ b/drivers/input/input-poller.h
>>>> @@ -0,0 +1,18 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +#ifndef _INPUT_POLLER_H
>>>> +#define _INPUT_POLLER_H
>>>> +
>>>> +/*
>>>> + * Support for polling mode for input devices.
>>>> + */
>>>> +#include <linux/sysfs.h>
>>>> +
>>>> +struct input_dev_poller;
>>>> +
>>>> +void input_dev_poller_finalize(struct input_dev_poller *poller);
>>>> +void input_dev_poller_start(struct input_dev_poller *poller);
>>>> +void input_dev_poller_stop(struct input_dev_poller *poller);
>>>> +
>>>> +extern struct attribute_group input_poller_attribute_group;
>>>> +
>>>> +#endif /* _INPUT_POLLER_H */
>>>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>>>> index 7494a0dede79..c08aa3596144 100644
>>>> --- a/drivers/input/input.c
>>>> +++ b/drivers/input/input.c
>>>> @@ -24,6 +24,7 @@
>>>> #include <linux/mutex.h>
>>>> #include <linux/rcupdate.h>
>>>> #include "input-compat.h"
>>>> +#include "input-poller.h"
>>>>
>>>> MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
>>>> MODULE_DESCRIPTION("Input core");
>>>> @@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)
>>>>
>>>> handle->open++;
>>>>
>>>> - if (!dev->users++ && dev->open)
>>>> - retval = dev->open(dev);
>>>> + if (dev->users++) {
>>>> + /*
>>>> + * Device is already opened, so we can exit immediately and
>>>> + * report success.
>>>> + */
>>>> + goto out;
>>>> + }
>>>>
>>>> - if (retval) {
>>>> - dev->users--;
>>>> - if (!--handle->open) {
>>>> + if (dev->open) {
>>>> + retval = dev->open(dev);
>>>> + if (retval) {
>>>> + dev->users--;
>>>> + handle->open--;
>>>> /*
>>>> * Make sure we are not delivering any more events
>>>> * through this handle
>>>> */
>>>> synchronize_rcu();
>>>> + goto out;
>>>> }
>>>> }
>>>>
>>>> + if (dev->poller)
>>>> + input_dev_poller_start(dev->poller);
>>>> +
>>>> out:
>>>> mutex_unlock(&dev->mutex);
>>>> return retval;
>>>> @@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)
>>>>
>>>> __input_release_device(handle);
>>>>
>>>> - if (!--dev->users && dev->close)
>>>> - dev->close(dev);
>>>> + if (!--dev->users) {
>>>> + if (dev->poller)
>>>> + input_dev_poller_stop(dev->poller);
>>>> +
>>>> + if (dev->close)
>>>> + dev->close(dev);
>>>> + }
>>>>
>>>> if (!--handle->open) {
>>>> /*
>>>> @@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
>>>> &input_dev_attr_group,
>>>> &input_dev_id_attr_group,
>>>> &input_dev_caps_attr_group,
>>>> + &input_poller_attribute_group,
>>>> NULL
>>>> };
>>>>
>>>> @@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)
>>>>
>>>> input_ff_destroy(dev);
>>>> input_mt_destroy_slots(dev);
>>>> + kfree(dev->poller);
>>>> kfree(dev->absinfo);
>>>> kfree(dev->vals);
>>>> kfree(dev);
>>>> @@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
>>>> if (!dev->setkeycode)
>>>> dev->setkeycode = input_default_setkeycode;
>>>>
>>>> + if (dev->poller)
>>>> + input_dev_poller_finalize(dev->poller);
>>>> +
>>>> error = device_add(&dev->dev);
>>>> if (error)
>>>> goto err_free_vals;
>>>> diff --git a/include/linux/input.h b/include/linux/input.h
>>>> index e95a439d8bd5..94f277cd806a 100644
>>>> --- a/include/linux/input.h
>>>> +++ b/include/linux/input.h
>>>> @@ -21,6 +21,8 @@
>>>> #include <linux/timer.h>
>>>> #include <linux/mod_devicetable.h>
>>>>
>>>> +struct input_dev_poller;
>>>> +
>>>> /**
>>>> * struct input_value - input value representation
>>>> * @type: type of value (EV_KEY, EV_ABS, etc)
>>>> @@ -71,6 +73,8 @@ enum input_clock_type {
>>>> * not sleep
>>>> * @ff: force feedback structure associated with the device if device
>>>> * supports force feedback effects
>>>> + * @poller: poller structure associated with the device if device is
>>>> + * set up to use polling mode
>>>> * @repeat_key: stores key code of the last key pressed; used to implement
>>>> * software autorepeat
>>>> * @timer: timer for software autorepeat
>>>> @@ -156,6 +160,8 @@ struct input_dev {
>>>>
>>>> struct ff_device *ff;
>>>>
>>>> + struct input_dev_poller *poller;
>>>> +
>>>> unsigned int repeat_key;
>>>> struct timer_list timer;
>>>>
>>>> @@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);
>>>>
>>>> void input_reset_device(struct input_dev *);
>>>>
>>>> +int input_setup_polling(struct input_dev *dev,
>>>> + void (*poll_fn)(struct input_dev *dev));
>>>> +void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
>>>> +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
>>>> +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
>>>> +
>>>> int __must_check input_register_handler(struct input_handler *);
>>>> void input_unregister_handler(struct input_handler *);
>>>>
>>>> --
>>>> 2.23.0.rc1.153.gdeed80330f-goog
>>>>
>>>>
>>>> --
>>>> Dmitry
>>
>> --
>> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: add support for polling to input devices
2019-08-20 11:56 ` Michal Vokáč
@ 2019-08-20 19:05 ` Dmitry Torokhov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2019-08-20 19:05 UTC (permalink / raw)
To: Michal Vokáč
Cc: Benjamin Tissoires, open list:HID CORE LAYER, lkml, Rob Herring
On Tue, Aug 20, 2019 at 01:56:53PM +0200, Michal Vokáč wrote:
> On 13. 08. 19 16:04, Benjamin Tissoires wrote:
> > On Mon, Aug 12, 2019 at 7:11 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Benjamin,
> > >
> > > On Mon, Aug 12, 2019 at 06:50:38PM +0200, Benjamin Tissoires wrote:
> > > > Hi Dmitry,
> > > >
> > > > On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > Separating "normal" and "polled" input devices was a mistake, as often we
> > > > > want to allow the very same device work on both interrupt-driven and
> > > > > polled mode, depending on the board on which the device is used.
> > > > >
> > > > > This introduces new APIs:
> > > > >
> > > > > - input_setup_polling
> > > > > - input_set_poll_interval
> > > > > - input_set_min_poll_interval
> > > > > - input_set_max_poll_interval
> > > > >
> > > > > These new APIs allow switching an input device into polled mode with sysfs
> > > > > attributes matching drivers using input_polled_dev APIs that will be
> > > > > eventually removed.
> > > >
> > > > Are you sure that using sysfs is the correct way here?
> > > > I would think using generic properties that can be overwritten by the
> > > > DSDT or by the device tree would make things easier from a driver
> > > > point of view.
> > >
> > > Couple of points: I wanted it to be compatible with input-polldev.c so
> > > the sysfs attributes must match (so that we can convert existing drivers
> > > and zap input-polldev).
> >
> > Oh, I missed that. Good point.
> >
> > > I also am not sure if polling parameters are
> > > property of board, or it is either fundamental hardware limitation or
> > > simply desired system behavior.
> >
> > I think it's a combination of everything: sometimes the board misses
> > the capability to not do IRQs for that device, and using properties
> > would be better here: you can define them where you need (board,
> > platform or device level), and have a working platfrom from the kernel
> > description entirely.
> > However, it doesn't solve the issue of input-polldev, so maybe
> > properties should be added on top of this sysfs.
> >
> > > If Rob is OK with adding device
> > > properties I'd be fine adding them as a followup, otherwise we can have
> > > udev adjust the behavior as needed for given box shortly after boot.
> >
> > Fair enough.
> >
> > >
> > > >
> > > > Also, checkpatch complains about a few octal permissions that are
> > > > preferred over symbolic ones, and there is a possible 'out of memory'
> > > > nessage at drivers/input/input-poller.c:75.
> > >
> > > Yes, there is. It is there so we would know what device we were trying
> > > to set up when OOM happened. You can probable decipher the driver from
> > > the stack trace, but figuring particular device instance is harder.
> >
> > Could you add a comment there explaining this choice? I have a feeling
> > you'll have to refuse a few patches of people running coccinelle
> > scripts and be too happy to send a kernel patch.
Done.
> >
> > Other than that:
> > Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
>
> Hi Dmitry,
>
> what is the status of this patch? Are we still waiting for Rob to
> comment on the device properties or is this ready to land?
I applied it just now.
>
> Little bit OT question: what tree/branch do you use to apply patches?
> According to the mailing list you recently applied some patches but
> I can not find them here [1].
Patches that go into current cycle will be in 'for-linus' branch.
Patches that will be merged in the next merge window (like this one)
will end up in 'next' branch.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-20 19:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 17:35 [PATCH] Input: add support for polling to input devices Dmitry Torokhov
2019-08-12 16:50 ` Benjamin Tissoires
2019-08-12 17:11 ` Dmitry Torokhov
2019-08-13 14:04 ` Benjamin Tissoires
2019-08-20 11:56 ` Michal Vokáč
2019-08-20 19:05 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).