Re: [RFC PATCH v2 0/4] Input: mpr121-polled: Add polled driver for MPR121

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 :(

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.

 - set the poll interval + min/max interval
 - register the input device as usual (input_register_device)
 - What about the device_init_wakeup? It does not make sense for
   polling I think.

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.

Thanks, Michal


Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
---
  drivers/input/Makefile       |    2
  drivers/input/input-poller.c |  207 ++++++++++++++++++++++++++++++++++++++++++
  drivers/input/input.c        |   36 ++++++-
  include/linux/input.h        |   10 ++
  4 files changed, 247 insertions(+), 8 deletions(-)
  create mode 100644 drivers/input/input-poller.c

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.c b/drivers/input/input.c
index 7f3c5fcb9ed6..6b87c07d5981 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);
@@ -2135,6 +2154,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 510e78558c10..956f32be87cc 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)
@@ -147,6 +149,8 @@ struct input_dev {
struct ff_device *ff; + struct input_dev_poller *poller;
+
  	unsigned int repeat_key;
  	struct timer_list timer;
@@ -361,6 +365,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 *);




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux