On Thu, Jul 25, 2019 at 02:58:02PM +0200, Michal Vokáč wrote:
> On 25. 07. 19 10:57, Dmitry Torokhov wrote:
> > Hi Michal,
> >
> > On Tue, May 21, 2019 at 08:51:17AM +0200, Michal Vokáč wrote:
> > > On 21. 05. 19 7:37, Dmitry Torokhov wrote:
> > > > Hi Michal,
> > > >
> > > > On Fri, May 17, 2019 at 03:12:49PM +0200, Michal Vokáč wrote:
> > > > > Hi,
> > > > >
> > > > > I have to deal with a situation where we have a custom i.MX6 based
> > > > > platform in production that uses the MPR121 touchkey controller.
> > > > > Unfortunately the chip is connected using only the I2C interface.
> > > > > The interrupt line is not used. Back in 2015 (Linux v3.14), my
> > > > > colleague modded the existing mpr121_touchkey.c driver to use polling
> > > > > instead of interrupt.
> > > > >
> > > > > For quite some time yet I am in a process of updating the product from
> > > > > the ancient Freescale v3.14 kernel to the latest mainline and pushing
> > > > > any needed changes upstream. The DT files for our imx6dl-yapp4 platform
> > > > > already made it into v5.1-rc.
> > > > >
> > > > > I rebased and updated our mpr121 patch to the latest mainline.
> > > > > It is created as a separate driver, similarly to gpio_keys_polled.
> > > > >
> > > > > The I2C device is quite susceptible to ESD. An ESD test quite often
> > > > > causes reset of the chip or some register randomly changes its value.
> > > > > The [PATCH 3/4] adds a write-through register cache. With the cache
> > > > > this state can be detected and the device can be re-initialied.
> > > > >
> > > > > The main question is: Is there any chance that such a polled driver
> > > > > could be accepted? Is it correct to implement it as a separate driver
> > > > > or should it be done as an option in the existing driver? I can not
> > > > > really imagine how I would do that though..
> > > > >
> > > > > There are also certain worries that the MPR121 chip may no longer be
> > > > > available in nonspecifically distant future. In case of EOL I will need
> > > > > to add a polled driver for an other touchkey chip. May it be already
> > > > > in mainline or a completely new one.
> > > >
> > > > I think that my addition of input_polled_dev was ultimately a wrong
> > > > thing to do. I am looking into enabling polling mode for regular input
> > > > devices as we then can enable polling mode in existing drivers.
> > >
> > > OK, that sounds good. Especially when one needs to switch from one chip
> > > to another that is already in tree, the need for a whole new polling
> > > driver is eliminated.
> >
> > Could you please try the patch below and see if it works for your use
> > case? Note that I have not tried running it, but it compiles so it must
> > be good ;)
>
> Hi Dmitry,
> Thank you very much for the patch!
> I gave it a shot and it seems you forgot to add the input-poller.h file
> to the patch.. it does not compile on my side :(
Oops ;) Please see the updated patch below.
>
> > Input: add support for polling to input devices
> >
> > From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> >
> > 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.
>
> After reading this I am not really sure what else needs to be done
> to test/use the poller. I suspect I need to modify the input device
> driver (mpr121_touchkey.c in my case) like this:
>
> If the interrupt gpio is not provided in DT, the device driver probe
> function should:
> - not request the threaded interrupt
> - call input_setup_polling and provide it with poll_fn
> Can the mpr_touchkey_interrupt function be used as is for this
> purpose? The only problem I see is it returns IRQ_HANDLED.
I'd factor out code suitable for polling from mpr_touchkey_interrupt()
and then do
static irqreturn_t mpr_touchkey_interrupt(...)
{
mpr_touchkey_report(...);
return IRQ_HANDLED;
}
>
> - set the poll interval + min/max interval
> - register the input device as usual (input_register_device)
Yes.
> - What about the device_init_wakeup? It does not make sense for
> polling I think.
We can either trust DT not to have the property for polled case, or add
more checks. I think we should simply trust DT. Even if it is wrong it
will not cause any issues.
>
> Just nitpicking - I also ran checkpatch.pl on the patch and it spits
> out some warnings. I am not sure whether some of those should not
> be fixed.
I think in this particular case checkpatch is full of it.
Thanks.
Input: add support for polling to input devices
From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
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.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
---
drivers/input/Makefile | 2
drivers/input/input-poller.c | 207 ++++++++++++++++++++++++++++++++++++++++++
drivers/input/input-poller.h | 18 ++++
drivers/input/input.c | 36 ++++++-
include/linux/input.h | 10 ++
5 files changed, 265 insertions(+), 8 deletions(-)
create mode 100644 drivers/input/input-poller.c
create mode 100644 drivers/input/input-poller.h
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..008d6f362d60
--- /dev/null
+++ b/drivers/input/input-poller.c
@@ -0,0 +1,207 @@
+// 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->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@xxxxxxx>");
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..10346211ff15 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)
@@ -156,6 +158,8 @@ struct input_dev {
struct ff_device *ff;
+ struct input_dev_poller *poller;
+
unsigned int repeat_key;
struct timer_list timer;
@@ -372,6 +376,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 *);
--
Dmitry
