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

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

[Michal Vokáč]

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 will appreciate any comments. Thank you in advance,
Michal


Michal Vokáč (4):
  dt-bindings: input: Add support for the MPR121 without interrupt line
  Input: mpr121-polled: Add polling variant of the MPR121 touchkey
    driver
  Input: mpr121-polled: Add write-through cache to detect corrupted
    registers
  ARM: dts: imx6dl-yapp4: Enable MPR121 touch keypad on Hydra

 .../bindings/input/mpr121-touchkey-polled.txt      |  26 ++
 arch/arm/boot/dts/imx6dl-yapp4-common.dtsi         |  12 +
 arch/arm/boot/dts/imx6dl-yapp4-hydra.dts           |   4 +
 drivers/input/keyboard/Kconfig                     |  13 +
 drivers/input/keyboard/Makefile                    |   1 +
 drivers/input/keyboard/mpr121_touchkey_polled.c    | 493 +++++++++++++++++++++
 6 files changed, 549 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/mpr121-touchkey-polled.txt
 create mode 100644 drivers/input/keyboard/mpr121_touchkey_polled.c

-- 
2.1.4

[RFC PATCH v2 1/4] dt-bindings: input: Add support for the MPR121 without interrupt line

[Michal Vokáč]

Normally, the MPR121 controller uses separate interrupt line to notify
the I2C host that a key was touched/released. To support platforms that
can not use the interrupt line, polling of the MPR121 registers can be
used.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes since v1:
- Document the polled binding in the original file, do not create a new one.
  (Rob)

 Documentation/devicetree/bindings/input/mpr121-touchkey.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
index b7c61ee5841b..97f55273d473 100644
--- a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
+++ b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
@@ -1,9 +1,14 @@
-* Freescale MPR121 Controllor
+* Freescale MPR121 Controller
 
 Required Properties:
-- compatible:		Should be "fsl,mpr121-touchkey"
+- compatible:		Should be one of:
+			- "fsl,mpr121-touchkey" - MPR121 with interrupt line
+			- "fsl,mpr121-touchkey-polled" - MPR121 with polling
 - reg:			The I2C slave address of the device.
 - interrupts:		The interrupt number to the cpu.
+			In case of "fsl,mpr121-touchkey-polled" the interrupt
+			line is not used and hence the interrupts property is
+			not required.
 - vdd-supply:		Phandle to the Vdd power supply.
 - linux,keycodes:	Specifies an array of numeric keycode values to
 			be used for reporting button presses. The array can
-- 
2.1.4

Re: [RFC PATCH v2 1/4] dt-bindings: input: Add support for the MPR121 without interrupt line

[Rob Herring]

On Fri, May 17, 2019 at 03:12:50PM +0200, Michal Vokáč wrote:
> Normally, the MPR121 controller uses separate interrupt line to notify
> the I2C host that a key was touched/released. To support platforms that
> can not use the interrupt line, polling of the MPR121 registers can be
> used.

'separate' from what?

> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
> Changes since v1:
> - Document the polled binding in the original file, do not create a new one.
>   (Rob)
> 
>  Documentation/devicetree/bindings/input/mpr121-touchkey.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
> index b7c61ee5841b..97f55273d473 100644
> --- a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
> +++ b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
> @@ -1,9 +1,14 @@
> -* Freescale MPR121 Controllor
> +* Freescale MPR121 Controller
>  
>  Required Properties:
> -- compatible:		Should be "fsl,mpr121-touchkey"
> +- compatible:		Should be one of:
> +			- "fsl,mpr121-touchkey" - MPR121 with interrupt line
> +			- "fsl,mpr121-touchkey-polled" - MPR121 with polling
>  - reg:			The I2C slave address of the device.
>  - interrupts:		The interrupt number to the cpu.
> +			In case of "fsl,mpr121-touchkey-polled" the interrupt
> +			line is not used and hence the interrupts property is
> +			not required.

Absence of the interrupts property is enough to determine polled mode 
and you don't need a separate compatible string.

>  - vdd-supply:		Phandle to the Vdd power supply.
>  - linux,keycodes:	Specifies an array of numeric keycode values to
>  			be used for reporting button presses. The array can
> -- 
> 2.1.4
> 

Re: [RFC PATCH v2 1/4] dt-bindings: input: Add support for the MPR121 without interrupt line

[Michal Vokáč]

On 14. 06. 19 0:39, Rob Herring wrote:
> On Fri, May 17, 2019 at 03:12:50PM +0200, Michal Vokáč wrote:
>> Normally, the MPR121 controller uses separate interrupt line to notify
>> the I2C host that a key was touched/released. To support platforms that
>> can not use the interrupt line, polling of the MPR121 registers can be
>> used.
> 
> 'separate' from what?

"Separate" here is meant like "additional to the standard set of SCL
and SDA I2C lines". Looks like inappropriately used word by
a non-native speaker that can be omitted.

>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>> ---
>> Changes since v1:
>> - Document the polled binding in the original file, do not create a new one.
>>    (Rob)
>>
>>   Documentation/devicetree/bindings/input/mpr121-touchkey.txt | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
>> index b7c61ee5841b..97f55273d473 100644
>> --- a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
>> +++ b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
>> @@ -1,9 +1,14 @@
>> -* Freescale MPR121 Controllor
>> +* Freescale MPR121 Controller
>>   
>>   Required Properties:
>> -- compatible:		Should be "fsl,mpr121-touchkey"
>> +- compatible:		Should be one of:
>> +			- "fsl,mpr121-touchkey" - MPR121 with interrupt line
>> +			- "fsl,mpr121-touchkey-polled" - MPR121 with polling
>>   - reg:			The I2C slave address of the device.
>>   - interrupts:		The interrupt number to the cpu.
>> +			In case of "fsl,mpr121-touchkey-polled" the interrupt
>> +			line is not used and hence the interrupts property is
>> +			not required.
> 
> Absence of the interrupts property is enough to determine polled mode
> and you don't need a separate compatible string.

Would not this work only if the polled mode was implemented as
part of the current driver? I raised this question in the cover letter.
I do not really know how this should be done.

So I implemented the polled mode in a new driver (taking the
gpio-keys-polled as an example). Having separate compatible string is
the only option I know of to match the right driver.

Anyway, Dmitry already commented that his addition of input_polled_dev
for creating polled input devices was not the best choice. He would
rather like to implement polling mode for all regular input devices
and that would allow to enable polling mode in existing drivers.

Since I do not know how to help with that work I am stuck with the
separate driver/compatible string solution.

Re: [RFC PATCH v2 1/4] dt-bindings: input: Add support for the MPR121 without interrupt line

[Dmitry Torokhov]

On Thu, Jun 13, 2019 at 04:39:45PM -0600, Rob Herring wrote:
> On Fri, May 17, 2019 at 03:12:50PM +0200, Michal Vokáč wrote:
> > Normally, the MPR121 controller uses separate interrupt line to notify
> > the I2C host that a key was touched/released. To support platforms that
> > can not use the interrupt line, polling of the MPR121 registers can be
> > used.
> 
> 'separate' from what?
> 
> > 
> > Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> > ---
> > Changes since v1:
> > - Document the polled binding in the original file, do not create a new one.
> >   (Rob)
> > 
> >  Documentation/devicetree/bindings/input/mpr121-touchkey.txt | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
> > index b7c61ee5841b..97f55273d473 100644
> > --- a/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
> > +++ b/Documentation/devicetree/bindings/input/mpr121-touchkey.txt
> > @@ -1,9 +1,14 @@
> > -* Freescale MPR121 Controllor
> > +* Freescale MPR121 Controller
> >  
> >  Required Properties:
> > -- compatible:		Should be "fsl,mpr121-touchkey"
> > +- compatible:		Should be one of:
> > +			- "fsl,mpr121-touchkey" - MPR121 with interrupt line
> > +			- "fsl,mpr121-touchkey-polled" - MPR121 with polling
> >  - reg:			The I2C slave address of the device.
> >  - interrupts:		The interrupt number to the cpu.
> > +			In case of "fsl,mpr121-touchkey-polled" the interrupt
> > +			line is not used and hence the interrupts property is
> > +			not required.
> 
> Absence of the interrupts property is enough to determine polled mode 
> and you don't need a separate compatible string.

I would prefer if we could distinguish between chip working in polled
mode intentionally vs DT writer simply forgetting to specify interrupt
property. Should we key the polling mode off "linux,poll-interval"
property? We probably going to need it anyway as not everyone needs the
same polling frequency.

Thanks.

-- 
Dmitry

[RFC PATCH v2 2/4] Input: mpr121-polled: Add polling variant of the MPR121 touchkey driver

[Michal Vokáč]

This driver is based on the original driver with interrupts. Polling
driver may be used in cases where the MPR121 chip is connected using
only the I2C interface and the interrupt line is not available.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 drivers/input/keyboard/Kconfig                  |  13 +
 drivers/input/keyboard/Makefile                 |   1 +
 drivers/input/keyboard/mpr121_touchkey_polled.c | 417 ++++++++++++++++++++++++
 3 files changed, 431 insertions(+)
 create mode 100644 drivers/input/keyboard/mpr121_touchkey_polled.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 52d7f55fca32..b61cf6e4f1ba 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -418,6 +418,19 @@ config KEYBOARD_MPR121
 	  To compile this driver as a module, choose M here: the
 	  module will be called mpr121_touchkey.
 
+config KEYBOARD_MPR121_POLLED
+	tristate "Polled Freescale MPR121 Touchkey"
+	depends on I2C
+	help
+	  Say Y here if you have Freescale MPR121 touchkey controller
+	  chip in your system connected only using the I2C line without
+	  the interrupt line.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called mpr121_touchkey_polled.
+
 config KEYBOARD_SNVS_PWRKEY
 	tristate "IMX SNVS Power Key Driver"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 182e92985dbf..903f50842844 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
 obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
 obj-$(CONFIG_KEYBOARD_MCS)		+= mcs_touchkey.o
 obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121_touchkey.o
+obj-$(CONFIG_KEYBOARD_MPR121_POLLED)	+= mpr121_touchkey_polled.o
 obj-$(CONFIG_KEYBOARD_MTK_PMIC) 	+= mtk-pmic-keys.o
 obj-$(CONFIG_KEYBOARD_NEWTON)		+= newtonkbd.o
 obj-$(CONFIG_KEYBOARD_NOMADIK)		+= nomadik-ske-keypad.o
diff --git a/drivers/input/keyboard/mpr121_touchkey_polled.c b/drivers/input/keyboard/mpr121_touchkey_polled.c
new file mode 100644
index 000000000000..e5e80530c9d8
--- /dev/null
+++ b/drivers/input/keyboard/mpr121_touchkey_polled.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Touchkey driver for Freescale MPR121 Controllor
+//
+// Copyright (C) 2011 Freescale Semiconductor, Inc.
+// Author: Zhang Jiejing <jiejing.zhang@freescale.com>
+//
+// Based on mcs_touchkey.c
+//
+// Copyright (C) 2019 Y Soft Corporation, a.s.
+// Author: Pavel Staněk <pavel.stanek@ysoft.com>
+// Author: Michal Vokáč <michal.vokac@ysoft.com>
+//
+// Reworked into polled driver based on mpr121_touchkey.c
+
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input-polldev.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+/* Register definitions */
+#define ELE_TOUCH_STATUS_0_ADDR	0x0
+#define ELE_TOUCH_STATUS_1_ADDR	0X1
+#define MHD_RISING_ADDR		0x2b
+#define NHD_RISING_ADDR		0x2c
+#define NCL_RISING_ADDR		0x2d
+#define FDL_RISING_ADDR		0x2e
+#define MHD_FALLING_ADDR	0x2f
+#define NHD_FALLING_ADDR	0x30
+#define NCL_FALLING_ADDR	0x31
+#define FDL_FALLING_ADDR	0x32
+#define ELE0_TOUCH_THRESHOLD_ADDR	0x41
+#define ELE0_RELEASE_THRESHOLD_ADDR	0x42
+#define AFE_CONF_ADDR			0x5c
+#define FILTER_CONF_ADDR		0x5d
+
+/*
+ * ELECTRODE_CONF_ADDR: This register configures the number of
+ * enabled capacitance sensing inputs and its run/suspend mode.
+ */
+#define ELECTRODE_CONF_ADDR		0x5e
+#define ELECTRODE_CONF_QUICK_CHARGE	0x80
+#define AUTO_CONFIG_CTRL_ADDR		0x7b
+#define AUTO_CONFIG_USL_ADDR		0x7d
+#define AUTO_CONFIG_LSL_ADDR		0x7e
+#define AUTO_CONFIG_TL_ADDR		0x7f
+
+/* Threshold of touch/release trigger */
+#define TOUCH_THRESHOLD			0x08
+#define RELEASE_THRESHOLD		0x05
+/* Masks for touch and release triggers */
+#define TOUCH_STATUS_MASK		0xfff
+/* MPR121 has 12 keys */
+#define MPR121_MAX_KEY_COUNT		12
+
+#define MPR121_POLL_INTERVAL		50
+#define MPR121_POLL_INTERVAL_MIN	10
+#define MPR121_POLL_INTERVAL_MAX	200
+#define MPR121_POLL_INTERVAL_REINIT	500
+#define MPR121_POLL_RETRY_MAX		4
+
+struct mpr121_polled {
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	struct input_polled_dev	*poll_dev;
+	unsigned int statusbits;
+	unsigned int keycount;
+	u32 keycodes[MPR121_MAX_KEY_COUNT];
+	u8 read_errors;
+	int vdd_uv;
+};
+
+struct mpr121_polled_init_register {
+	int addr;
+	u8 val;
+};
+
+static const struct mpr121_polled_init_register init_reg_table[] = {
+	{ MHD_RISING_ADDR,	0x1 },
+	{ NHD_RISING_ADDR,	0x1 },
+	{ MHD_FALLING_ADDR,	0x1 },
+	{ NHD_FALLING_ADDR,	0x1 },
+	{ NCL_FALLING_ADDR,	0xff },
+	{ FDL_FALLING_ADDR,	0x02 },
+	{ FILTER_CONF_ADDR,	0x04 },
+	{ AFE_CONF_ADDR,	0x0b },
+	{ AUTO_CONFIG_CTRL_ADDR, 0x0b },
+};
+
+static void mpr121_polled_vdd_supply_disable(void *data)
+{
+	struct regulator *vdd_supply = data;
+
+	regulator_disable(vdd_supply);
+}
+
+static struct regulator *mpr121_polled_vdd_supply_init(struct device *dev)
+{
+	struct regulator *vdd_supply;
+	int err;
+
+	vdd_supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(vdd_supply)) {
+		dev_err(dev, "failed to get vdd regulator: %ld\n",
+			PTR_ERR(vdd_supply));
+		return vdd_supply;
+	}
+
+	err = regulator_enable(vdd_supply);
+	if (err) {
+		dev_err(dev, "failed to enable vdd regulator: %d\n", err);
+		return ERR_PTR(err);
+	}
+
+	err = devm_add_action(dev, mpr121_polled_vdd_supply_disable,
+			      vdd_supply);
+	if (err) {
+		regulator_disable(vdd_supply);
+		dev_err(dev, "failed to add disable regulator action: %d\n",
+			err);
+		return ERR_PTR(err);
+	}
+
+	return vdd_supply;
+}
+
+static int mpr121_polled_phys_init(struct mpr121_polled *mpr121,
+				   struct i2c_client *client)
+{
+	const struct mpr121_polled_init_register *reg;
+	unsigned char usl, lsl, tl, eleconf;
+	int i, t, vdd, ret;
+
+	/* Set stop mode prior to writing any register */
+	ret = i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
+	if (ret < 0)
+		goto err_i2c_write;
+
+	/* Set up touch/release threshold for ele0-ele11 */
+	for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
+		t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
+		ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
+		if (ret < 0)
+			goto err_i2c_write;
+		ret = i2c_smbus_write_byte_data(client, t + 1,
+						RELEASE_THRESHOLD);
+		if (ret < 0)
+			goto err_i2c_write;
+	}
+
+	/* Set up init register */
+	for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
+		reg = &init_reg_table[i];
+		ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
+		if (ret < 0)
+			goto err_i2c_write;
+	}
+
+	/*
+	 * Capacitance on sensing input varies and needs to be compensated.
+	 * The internal MPR121-auto-configuration can do this if it's
+	 * registers are set properly (based on vdd_uv).
+	 */
+	vdd = mpr121->vdd_uv / 1000;
+	usl = ((vdd - 700) * 256) / vdd;
+	lsl = (usl * 65) / 100;
+	tl = (usl * 90) / 100;
+	ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
+	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
+	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
+
+	/*
+	 * Quick charge bit will let the capacitive charge to ready
+	 * state quickly, or the buttons may not function after system
+	 * boot.
+	 */
+	eleconf = mpr121->keycount | ELECTRODE_CONF_QUICK_CHARGE;
+	ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
+					 eleconf);
+	if (ret != 0)
+		goto err_i2c_write;
+
+	dev_dbg(&client->dev, "set up with %x keys.\n", mpr121->keycount);
+
+	return 0;
+
+err_i2c_write:
+	dev_err(&client->dev, "i2c write error: %d\n", ret);
+	return ret;
+}
+
+static void mpr121_polled_release_keys(struct mpr121_polled *mpr121)
+{
+	struct input_dev *input = mpr121->input_dev;
+	struct i2c_client *client = mpr121->client;
+	unsigned long statusbits;
+	unsigned int key_num;
+
+	if (!mpr121->statusbits)
+		return;
+
+	statusbits = mpr121->statusbits;
+	mpr121->statusbits = 0;
+	for_each_set_bit(key_num, &statusbits, mpr121->keycount) {
+		unsigned int key_val;
+
+		key_val = mpr121->keycodes[key_num];
+
+		input_event(input, EV_MSC, MSC_SCAN, key_num);
+		input_report_key(input, key_val, 0);
+
+		dev_dbg(&client->dev, "key %d %d %s\n", key_num, key_val,
+			"released");
+	}
+	input_sync(input);
+}
+
+static int mpr121_polled_process_keys(struct mpr121_polled *mpr121)
+{
+	struct input_dev *input = mpr121->input_dev;
+	struct i2c_client *client = mpr121->client;
+	unsigned long bit_changed;
+	unsigned int key_num;
+	int reg;
+
+	reg = i2c_smbus_read_word_data(client, ELE_TOUCH_STATUS_0_ADDR);
+	if (reg < 0) {
+		dev_err(&client->dev, "i2c read error: %d\n", reg);
+		return reg;
+	}
+
+	reg &= TOUCH_STATUS_MASK;
+	bit_changed = reg ^ mpr121->statusbits;
+	mpr121->statusbits = reg;
+	for_each_set_bit(key_num, &bit_changed, mpr121->keycount) {
+		unsigned int key_val, pressed;
+
+		pressed = reg & BIT(key_num);
+		key_val = mpr121->keycodes[key_num];
+
+		input_event(input, EV_MSC, MSC_SCAN, key_num);
+		input_report_key(input, key_val, pressed);
+
+		dev_dbg(&client->dev, "key %d %d %s\n", key_num, key_val,
+			pressed ? "pressed" : "released");
+	}
+	input_sync(input);
+
+	return 0;
+}
+static void mpr121_poll(struct input_polled_dev *dev)
+{
+	struct mpr121_polled *mpr121 = dev->private;
+	struct i2c_client *client = mpr121->client;
+	int ret;
+
+	if (mpr121->read_errors > MPR121_POLL_RETRY_MAX) {
+		dev_warn(&client->dev,
+			 "device does not respond, re-initializing\n");
+		mpr121_polled_release_keys(mpr121);
+		ret = mpr121_polled_phys_init(mpr121, client);
+		if (ret >= 0) {
+			mpr121->read_errors = 0;
+			dev->poll_interval = MPR121_POLL_INTERVAL;
+		} else {
+			dev->poll_interval = MPR121_POLL_INTERVAL_REINIT;
+		}
+	}
+
+	ret = mpr121_polled_process_keys(mpr121);
+	if (ret < 0) {
+		mpr121->read_errors++;
+		return;
+	}
+
+	mpr121->read_errors = 0;
+}
+
+static int mpr121_polled_probe(struct i2c_client *client,
+			       const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct regulator *vdd_supply;
+	struct mpr121_polled *mpr121;
+	struct input_dev *input_dev;
+	struct input_polled_dev *poll_dev;
+	int error;
+	int i;
+
+	vdd_supply = mpr121_polled_vdd_supply_init(dev);
+	if (IS_ERR(vdd_supply))
+		return PTR_ERR(vdd_supply);
+
+	mpr121 = devm_kzalloc(dev, sizeof(*mpr121), GFP_KERNEL);
+	if (!mpr121)
+		return -ENOMEM;
+
+	poll_dev = devm_input_allocate_polled_device(dev);
+	if (!poll_dev)
+		return -ENOMEM;
+
+	mpr121->vdd_uv = regulator_get_voltage(vdd_supply);
+	mpr121->client = client;
+	mpr121->input_dev = poll_dev->input;
+	mpr121->poll_dev = poll_dev;
+	mpr121->keycount = device_property_read_u32_array(dev, "linux,keycodes",
+							  NULL, 0);
+	if (mpr121->keycount > MPR121_MAX_KEY_COUNT) {
+		dev_err(dev, "too many keys defined (%d)\n", mpr121->keycount);
+		return -EINVAL;
+	}
+
+	error = device_property_read_u32_array(dev, "linux,keycodes",
+					       mpr121->keycodes,
+					       mpr121->keycount);
+	if (error) {
+		dev_err(dev,
+			"failed to read linux,keycode property: %d\n", error);
+		return error;
+	}
+
+	poll_dev->private = mpr121;
+	poll_dev->poll = mpr121_poll;
+	poll_dev->poll_interval = MPR121_POLL_INTERVAL;
+	poll_dev->poll_interval_max = MPR121_POLL_INTERVAL_MAX;
+	poll_dev->poll_interval_min = MPR121_POLL_INTERVAL_MIN;
+
+	input_dev = poll_dev->input;
+	input_dev->name = "Freescale MPR121 Polled Touchkey";
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->dev.parent = dev;
+	if (device_property_read_bool(dev, "autorepeat"))
+		__set_bit(EV_REP, input_dev->evbit);
+	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+
+	input_dev->keycode = mpr121->keycodes;
+	input_dev->keycodesize = sizeof(mpr121->keycodes[0]);
+	input_dev->keycodemax = mpr121->keycount;
+
+	for (i = 0; i < mpr121->keycount; i++)
+		input_set_capability(input_dev, EV_KEY, mpr121->keycodes[i]);
+
+	error = mpr121_polled_phys_init(mpr121, client);
+	if (error) {
+		dev_err(dev, "Failed to init register\n");
+		return error;
+	}
+
+	error = input_register_polled_device(poll_dev);
+	if (error)
+		return error;
+
+	i2c_set_clientdata(client, mpr121);
+
+	return 0;
+}
+
+static int __maybe_unused mpr121_polled_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
+
+	return 0;
+}
+
+static int __maybe_unused mpr121_polled_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct mpr121_polled *mpr121 = i2c_get_clientdata(client);
+
+	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
+				  mpr121->keycount);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(mpr121_polled_pm_ops, mpr121_polled_suspend,
+			 mpr121_polled_resume);
+
+static const struct i2c_device_id mpr121_polled_id[] = {
+	{ "mpr121_polled", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mpr121_polled_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id mpr121_polled_dt_match_table[] = {
+	{ .compatible = "fsl,mpr121-touchkey-polled" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mpr121_polled_dt_match_table);
+#endif
+
+static struct i2c_driver mpr121_polled_driver = {
+	.driver = {
+		.name	= "mpr121-polled",
+		.pm	= &mpr121_polled_pm_ops,
+		.of_match_table = of_match_ptr(mpr121_polled_dt_match_table),
+	},
+	.id_table	= mpr121_polled_id,
+	.probe		= mpr121_polled_probe,
+};
+
+module_i2c_driver(mpr121_polled_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Michal Vokáč <michal.vokac@ysoft.com>");
+MODULE_DESCRIPTION("Polled driver for Freescale MPR121 chip");
-- 
2.1.4

[RFC PATCH v2 3/4] Input: mpr121-polled: Add write-through cache to detect corrupted registers

[Michal Vokáč]

The MPR121 chip (and I2C bus in general) is quite sensitive to ESD.
An electrostatic discharge can easily cause a reset of the MPR121 chip.
Even though the chip then recovers and respond to read/write commands,
it is not properly initialized.

This state can be detected using a write-through cache of the internal
registers. Each time a register is written to, its value is stored in
the cache and marked as valid. Once per MPR121_REG_CACHE_CHECK_LIMIT
polls one valid cache value is compared with its corresponding register
value. In case of difference an error counter is increased. If the error
counter limit is exceeded, the chip is re-initialized.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 drivers/input/keyboard/mpr121_touchkey_polled.c | 100 +++++++++++++++++++++---
 1 file changed, 88 insertions(+), 12 deletions(-)

diff --git a/drivers/input/keyboard/mpr121_touchkey_polled.c b/drivers/input/keyboard/mpr121_touchkey_polled.c
index e5e80530c9d8..6536d9b2eeb8 100644
--- a/drivers/input/keyboard/mpr121_touchkey_polled.c
+++ b/drivers/input/keyboard/mpr121_touchkey_polled.c
@@ -67,6 +67,19 @@
 #define MPR121_POLL_INTERVAL_REINIT	500
 #define MPR121_POLL_RETRY_MAX		4
 
+#define MPR121_REG_CACHE_MIN_ADDR	0x2b
+#define MPR121_REG_CACHE_MAX_ADDR	0x7f
+#define MPR121_REG_CACHE_SIZE		\
+		(MPR121_REG_CACHE_MAX_ADDR - MPR121_REG_CACHE_MIN_ADDR + 1)
+#define MPR121_REG_CACHE_CHECK_LIMIT	8
+#define mpr121_addr_to_cache_idx(addr)	(addr - MPR121_REG_CACHE_MIN_ADDR)
+#define mpr121_cache_idx_to_addr(idx)	(idx + MPR121_REG_CACHE_MIN_ADDR)
+
+struct mpr121_polled_reg_cache {
+	bool valid;
+	u8 value;
+};
+
 struct mpr121_polled {
 	struct i2c_client *client;
 	struct input_dev *input_dev;
@@ -76,6 +89,9 @@ struct mpr121_polled {
 	u32 keycodes[MPR121_MAX_KEY_COUNT];
 	u8 read_errors;
 	int vdd_uv;
+	struct mpr121_polled_reg_cache reg_cache[MPR121_REG_CACHE_SIZE];
+	u8 reg_cache_check_count;
+	u8 reg_cache_next_check_item;
 };
 
 struct mpr121_polled_init_register {
@@ -95,6 +111,29 @@ static const struct mpr121_polled_init_register init_reg_table[] = {
 	{ AUTO_CONFIG_CTRL_ADDR, 0x0b },
 };
 
+static int mpr121_polled_write_reg(struct mpr121_polled *mpr121, u8 addr,
+				   u8 value)
+{
+	struct i2c_client *client = mpr121->client;
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, addr, value);
+	if (ret < 0) {
+		dev_err(&client->dev, "i2c write error: %d\n", ret);
+		return ret;
+	}
+
+	if (addr >= MPR121_REG_CACHE_MIN_ADDR &&
+	    addr <= MPR121_REG_CACHE_MAX_ADDR) {
+		u8 i = mpr121_addr_to_cache_idx(addr);
+
+		mpr121->reg_cache[i].valid = 1;
+		mpr121->reg_cache[i].value = value;
+	}
+
+	return 0;
+}
+
 static void mpr121_polled_vdd_supply_disable(void *data)
 {
 	struct regulator *vdd_supply = data;
@@ -140,18 +179,18 @@ static int mpr121_polled_phys_init(struct mpr121_polled *mpr121,
 	int i, t, vdd, ret;
 
 	/* Set stop mode prior to writing any register */
-	ret = i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
+	ret = mpr121_polled_write_reg(mpr121, ELECTRODE_CONF_ADDR, 0x00);
 	if (ret < 0)
 		goto err_i2c_write;
 
 	/* Set up touch/release threshold for ele0-ele11 */
 	for (i = 0; i <= MPR121_MAX_KEY_COUNT; i++) {
 		t = ELE0_TOUCH_THRESHOLD_ADDR + (i * 2);
-		ret = i2c_smbus_write_byte_data(client, t, TOUCH_THRESHOLD);
+		ret = mpr121_polled_write_reg(mpr121, t, TOUCH_THRESHOLD);
 		if (ret < 0)
 			goto err_i2c_write;
-		ret = i2c_smbus_write_byte_data(client, t + 1,
-						RELEASE_THRESHOLD);
+		ret = mpr121_polled_write_reg(mpr121, t + 1,
+					      RELEASE_THRESHOLD);
 		if (ret < 0)
 			goto err_i2c_write;
 	}
@@ -159,7 +198,7 @@ static int mpr121_polled_phys_init(struct mpr121_polled *mpr121,
 	/* Set up init register */
 	for (i = 0; i < ARRAY_SIZE(init_reg_table); i++) {
 		reg = &init_reg_table[i];
-		ret = i2c_smbus_write_byte_data(client, reg->addr, reg->val);
+		ret = mpr121_polled_write_reg(mpr121, reg->addr, reg->val);
 		if (ret < 0)
 			goto err_i2c_write;
 	}
@@ -173,9 +212,9 @@ static int mpr121_polled_phys_init(struct mpr121_polled *mpr121,
 	usl = ((vdd - 700) * 256) / vdd;
 	lsl = (usl * 65) / 100;
 	tl = (usl * 90) / 100;
-	ret = i2c_smbus_write_byte_data(client, AUTO_CONFIG_USL_ADDR, usl);
-	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_LSL_ADDR, lsl);
-	ret |= i2c_smbus_write_byte_data(client, AUTO_CONFIG_TL_ADDR, tl);
+	ret = mpr121_polled_write_reg(mpr121, AUTO_CONFIG_USL_ADDR, usl);
+	ret |= mpr121_polled_write_reg(mpr121, AUTO_CONFIG_LSL_ADDR, lsl);
+	ret |= mpr121_polled_write_reg(mpr121, AUTO_CONFIG_TL_ADDR, tl);
 
 	/*
 	 * Quick charge bit will let the capacitive charge to ready
@@ -183,7 +222,7 @@ static int mpr121_polled_phys_init(struct mpr121_polled *mpr121,
 	 * boot.
 	 */
 	eleconf = mpr121->keycount | ELECTRODE_CONF_QUICK_CHARGE;
-	ret |= i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
+	ret |= mpr121_polled_write_reg(mpr121, ELECTRODE_CONF_ADDR,
 					 eleconf);
 	if (ret != 0)
 		goto err_i2c_write;
@@ -256,6 +295,36 @@ static int mpr121_polled_process_keys(struct mpr121_polled *mpr121)
 
 	return 0;
 }
+
+static int mpr121_polled_check_regs(struct mpr121_polled *mpr121)
+{
+	struct i2c_client *client = mpr121->client;
+	int i, reg;
+
+	/* Skip registers that were never written to (have invalid cache) */
+	i = mpr121->reg_cache_next_check_item;
+	for (; i < MPR121_REG_CACHE_SIZE; i++)
+		if (mpr121->reg_cache[i].valid)
+			break;
+
+	if (i == MPR121_REG_CACHE_SIZE) {
+		mpr121->reg_cache_next_check_item = 0;
+		return 0;
+	}
+
+	reg = i2c_smbus_read_byte_data(client, mpr121_cache_idx_to_addr(i));
+	if (reg < 0) {
+		dev_err(&client->dev, "i2c read error: %d\n", reg);
+		return -1;
+	}
+
+	if (reg != mpr121->reg_cache[i].value)
+		return -1;
+
+	mpr121->reg_cache_next_check_item = i + 1;
+	return 0;
+}
+
 static void mpr121_poll(struct input_polled_dev *dev)
 {
 	struct mpr121_polled *mpr121 = dev->private;
@@ -282,6 +351,13 @@ static void mpr121_poll(struct input_polled_dev *dev)
 	}
 
 	mpr121->read_errors = 0;
+	mpr121->reg_cache_check_count++;
+	if (mpr121->reg_cache_check_count > MPR121_REG_CACHE_CHECK_LIMIT) {
+		mpr121->reg_cache_check_count = 0;
+		ret = mpr121_polled_check_regs(mpr121);
+		if (ret < 0)
+			mpr121->read_errors++;
+	}
 }
 
 static int mpr121_polled_probe(struct i2c_client *client,
@@ -366,8 +442,9 @@ static int mpr121_polled_probe(struct i2c_client *client,
 static int __maybe_unused mpr121_polled_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct mpr121_polled *mpr121 = i2c_get_clientdata(client);
 
-	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR, 0x00);
+	mpr121_polled_write_reg(mpr121, ELECTRODE_CONF_ADDR, 0x00);
 
 	return 0;
 }
@@ -377,8 +454,7 @@ static int __maybe_unused mpr121_polled_resume(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct mpr121_polled *mpr121 = i2c_get_clientdata(client);
 
-	i2c_smbus_write_byte_data(client, ELECTRODE_CONF_ADDR,
-				  mpr121->keycount);
+	mpr121_polled_write_reg(mpr121, ELECTRODE_CONF_ADDR, mpr121->keycount);
 
 	return 0;
 }
-- 
2.1.4

[RFC PATCH v2 4/4] ARM: dts: imx6dl-yapp4: Enable MPR121 touch keypad on Hydra

[Michal Vokáč]

Enable the I2C connected touch keypad on Hydra board.
Use the polled binding as the interrupt line is not available.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 arch/arm/boot/dts/imx6dl-yapp4-common.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6dl-yapp4-hydra.dts   |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
index e8d800fec637..65a670e5bd4f 100644
--- a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
+++ b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
@@ -4,6 +4,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/input/input.h>
 #include <dt-bindings/pwm/pwm.h>
 
 / {
@@ -330,6 +331,17 @@
 		vcc-supply = <&sw2_reg>;
 		status = "disabled";
 	};
+
+	touchkeys: keys@5a {
+		compatible = "fsl,mpr121-touchkey-polled";
+		reg = <0x5a>;
+		vdd-supply = <&sw2_reg>;
+		autorepeat;
+		linux,keycodes = <KEY_1>, <KEY_2>, <KEY_3>, <KEY_4>, <KEY_5>,
+				<KEY_6>, <KEY_7>, <KEY_8>, <KEY_9>,
+				<KEY_BACKSPACE>, <KEY_0>, <KEY_ENTER>;
+		status = "disabled";
+	};
 };
 
 &iomuxc {
diff --git a/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts b/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts
index f97927064750..84c275bfdd38 100644
--- a/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts
+++ b/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts
@@ -45,6 +45,10 @@
 	status = "okay";
 };
 
+&touchkeys {
+	status = "okay";
+};
+
 &usdhc3 {
 	status = "okay";
 };
-- 
2.1.4

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

[Dmitry Torokhov]

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.

As far as gpio-keys vs gpio-key-polled, I feel that the capabilities of
polling driver is sufficiently different from interrupt-driven one, so
we will likely keep them separate.

Thanks.

-- 
Dmitry

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

[Michal Vokáč]

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.

I am still quite a novice in all kernel areas as I literally jump from
one subsystem to another to fix issues related to our platform. Anyway,
do you see any opportunity to help with that work?

> As far as gpio-keys vs gpio-key-polled, I feel that the capabilities of
> polling driver is sufficiently different from interrupt-driven one, so
> we will likely keep them separate.

OK, understand.

Thank you,
Michal

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

[Dmitry Torokhov]

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 ;)

Thanks!

Input: add support for polling to input devices

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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@gmail.com>
---
 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@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);
@@ -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 *);
 
-- 
Dmitry

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

[Michal Vokáč]

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@gmail.com>
> 
> 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@gmail.com>
> ---
>   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@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);
> @@ -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 *);
>   
> 

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

[Dmitry Torokhov]

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@gmail.com>
> > 
> > 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@gmail.com>

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@gmail.com>
---
 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@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..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

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

[Michal Vokáč]

On 25. 07. 19 16:40, Dmitry Torokhov wrote:
> 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.

Thank you, now it is (almost) good as you said :D

>>
>>> Input: add support for polling to input devices
>>>
>>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>
>>> 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;
> }
> 

Probably a trivial problem for experienced kernel hacker but I can not
wrap my head around this - the interrupt handler takes the mpr121
device id as an argument while the poller poll_fn takes struct input_dev.

I fail to figure out how to get the device id from the input device.

Here is what I have:

diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
index e9ceaa16b46a..1124f77ee10a 100644
--- a/drivers/input/keyboard/mpr121_touchkey.c
+++ b/drivers/input/keyboard/mpr121_touchkey.c
@@ -54,6 +54,10 @@
  /* MPR121 has 12 keys */
  #define MPR121_MAX_KEY_COUNT		12
  
+#define MPR121_POLL_INTERVAL		50
+#define MPR121_MIN_POLL_INTERVAL	10
+#define MPR121_MAX_POLL_INTERVAL	200
+
  struct mpr121_touchkey {
  	struct i2c_client	*client;
  	struct input_dev	*input_dev;
@@ -115,9 +119,12 @@ static struct regulator *mpr121_vdd_supply_init(struct device *dev)
  	return vdd_supply;
  }
  
-static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
+static void mpr_touchkey_report(struct input_dev *dev)
  {
-	struct mpr121_touchkey *mpr121 = dev_id;
+	/*
+	 * TODO the input_dev dev needs to be converted to mpr121 device id.
+	 */
+	struct mpr121_touchkey *mpr121 = /* TODO */;
  	struct i2c_client *client = mpr121->client;
  	struct input_dev *input = mpr121->input_dev;
  	unsigned long bit_changed;
@@ -127,14 +134,14 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
  	reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
  	if (reg < 0) {
  		dev_err(&client->dev, "i2c read error [%d]\n", reg);
-		goto out;
+		return;
  	}
  
  	reg <<= 8;
  	reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
  	if (reg < 0) {
  		dev_err(&client->dev, "i2c read error [%d]\n", reg);
-		goto out;
+		return;
  	}
  
  	reg &= TOUCH_STATUS_MASK;
@@ -155,8 +162,17 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
  
  	}
  	input_sync(input);
+}
+
+static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
+{
+	/*
+	 * TODO
+	 * mpr_touchkey_report takes struct input_dev as an argument,
+	 * not the device id.
+	 */
+	mpr_touchkey_report(/* TODO */);
  
-out:
  	return IRQ_HANDLED;
  }
  
@@ -232,11 +248,6 @@ static int mpr_touchkey_probe(struct i2c_client *client,
  	int error;
  	int i;
  
-	if (!client->irq) {
-		dev_err(dev, "irq number should not be zero\n");
-		return -EINVAL;
-	}
-
  	vdd_supply = mpr121_vdd_supply_init(dev);
  	if (IS_ERR(vdd_supply))
  		return PTR_ERR(vdd_supply);
@@ -289,13 +300,27 @@ static int mpr_touchkey_probe(struct i2c_client *client,
  		return error;
  	}
  
-	error = devm_request_threaded_irq(dev, client->irq, NULL,
-					  mpr_touchkey_interrupt,
-					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					  dev->driver->name, mpr121);
-	if (error) {
-		dev_err(dev, "Failed to register interrupt\n");
-		return error;
+	if (client->irq) {
+		error = devm_request_threaded_irq(dev, client->irq, NULL,
+						  mpr_touchkey_interrupt,
+						  IRQF_TRIGGER_FALLING |
+						  IRQF_ONESHOT,
+						  dev->driver->name, mpr121);
+		if (error) {
+			dev_err(dev, "Failed to register interrupt\n");
+			return error;
+		}
+	} else {
+		dev_dbg(dev, "invalid IRQ number, using polling mode\n");
+		error = input_setup_polling(input_dev, mpr_touchkey_report);
+		if (error)
+			return error;
+
+		input_set_poll_interval(input_dev, MPR121_POLL_INTERVAL);
+		input_set_min_poll_interval(input_dev,
+					    MPR121_MIN_POLL_INTERVAL);
+		input_set_max_poll_interval(input_dev,
+					    MPR121_MAX_POLL_INTERVAL);
  	}
  
  	error = input_register_device(input_dev);
--

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

[Dmitry Torokhov]

On Fri, Jul 26, 2019 at 01:31:31PM +0200, Michal Vokáč wrote:
> On 25. 07. 19 16:40, Dmitry Torokhov wrote:
> > 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.
> 
> Thank you, now it is (almost) good as you said :D
> 
> > > 
> > > > Input: add support for polling to input devices
> > > > 
> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > 
> > > > 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;
> > }
> > 
> 
> Probably a trivial problem for experienced kernel hacker but I can not
> wrap my head around this - the interrupt handler takes the mpr121
> device id as an argument while the poller poll_fn takes struct input_dev.
> 
> I fail to figure out how to get the device id from the input device.
> 
> Here is what I have:
> 
> diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
> index e9ceaa16b46a..1124f77ee10a 100644
> --- a/drivers/input/keyboard/mpr121_touchkey.c
> +++ b/drivers/input/keyboard/mpr121_touchkey.c
> @@ -54,6 +54,10 @@
>  /* MPR121 has 12 keys */
>  #define MPR121_MAX_KEY_COUNT		12
> +#define MPR121_POLL_INTERVAL		50
> +#define MPR121_MIN_POLL_INTERVAL	10
> +#define MPR121_MAX_POLL_INTERVAL	200
> +
>  struct mpr121_touchkey {
>  	struct i2c_client	*client;
>  	struct input_dev	*input_dev;
> @@ -115,9 +119,12 @@ static struct regulator *mpr121_vdd_supply_init(struct device *dev)
>  	return vdd_supply;
>  }
> -static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
> +static void mpr_touchkey_report(struct input_dev *dev)
>  {
> -	struct mpr121_touchkey *mpr121 = dev_id;
> +	/*
> +	 * TODO the input_dev dev needs to be converted to mpr121 device id.
> +	 */
> +	struct mpr121_touchkey *mpr121 = /* TODO */;

Use input_set_drvdata() in probe() and you can do

	struct mpr121_touchkey *mpr121 = input_get_drvdata(dev);

here.

>  	struct i2c_client *client = mpr121->client;
>  	struct input_dev *input = mpr121->input_dev;
>  	unsigned long bit_changed;
> @@ -127,14 +134,14 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
>  	reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
>  	if (reg < 0) {
>  		dev_err(&client->dev, "i2c read error [%d]\n", reg);
> -		goto out;
> +		return;
>  	}
>  	reg <<= 8;
>  	reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
>  	if (reg < 0) {
>  		dev_err(&client->dev, "i2c read error [%d]\n", reg);
> -		goto out;
> +		return;
>  	}
>  	reg &= TOUCH_STATUS_MASK;
> @@ -155,8 +162,17 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
>  	}
>  	input_sync(input);
> +}
> +
> +static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
> +{
> +	/*
> +	 * TODO
> +	 * mpr_touchkey_report takes struct input_dev as an argument,
> +	 * not the device id.
> +	 */

	struct mpr121_touchkey *mpr121 = dev_id;

	mpr_touchkey_report(mpr121->input);

> +	mpr_touchkey_report(/* TODO */);
> -out:
>  	return IRQ_HANDLED;
>  }
> @@ -232,11 +248,6 @@ static int mpr_touchkey_probe(struct i2c_client *client,
>  	int error;
>  	int i;
> -	if (!client->irq) {
> -		dev_err(dev, "irq number should not be zero\n");
> -		return -EINVAL;
> -	}
> -
>  	vdd_supply = mpr121_vdd_supply_init(dev);
>  	if (IS_ERR(vdd_supply))
>  		return PTR_ERR(vdd_supply);
> @@ -289,13 +300,27 @@ static int mpr_touchkey_probe(struct i2c_client *client,
>  		return error;
>  	}
> -	error = devm_request_threaded_irq(dev, client->irq, NULL,
> -					  mpr_touchkey_interrupt,
> -					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -					  dev->driver->name, mpr121);
> -	if (error) {
> -		dev_err(dev, "Failed to register interrupt\n");
> -		return error;
> +	if (client->irq) {
> +		error = devm_request_threaded_irq(dev, client->irq, NULL,
> +						  mpr_touchkey_interrupt,
> +						  IRQF_TRIGGER_FALLING |
> +						  IRQF_ONESHOT,
> +						  dev->driver->name, mpr121);
> +		if (error) {
> +			dev_err(dev, "Failed to register interrupt\n");
> +			return error;
> +		}
> +	} else {
> +		dev_dbg(dev, "invalid IRQ number, using polling mode\n");

I think it would be better if we checked if poll interval device
property is present before setting polling mode, so that polling mode is
not activated simply because someone forgot to add interrupt
specification.

> +		error = input_setup_polling(input_dev, mpr_touchkey_report);
> +		if (error)
> +			return error;
> +
> +		input_set_poll_interval(input_dev, MPR121_POLL_INTERVAL);
> +		input_set_min_poll_interval(input_dev,
> +					    MPR121_MIN_POLL_INTERVAL);
> +		input_set_max_poll_interval(input_dev,
> +					    MPR121_MAX_POLL_INTERVAL);
>  	}
>  	error = input_register_device(input_dev);
> --

Thanks.

-- 
Dmitry

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

[Michal Vokáč]

On 27. 07. 19 9:31, Dmitry Torokhov wrote:
> On Fri, Jul 26, 2019 at 01:31:31PM +0200, Michal Vokáč wrote:
>> On 25. 07. 19 16:40, Dmitry Torokhov wrote:
>>> 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.
>>
>> Thank you, now it is (almost) good as you said :D
>>
>>>>
>>>>> Input: add support for polling to input devices
>>>>>
>>>>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>>
>>>>> 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;
>>> }
>>>
>>
>> Probably a trivial problem for experienced kernel hacker but I can not
>> wrap my head around this - the interrupt handler takes the mpr121
>> device id as an argument while the poller poll_fn takes struct input_dev.
>>
>> I fail to figure out how to get the device id from the input device.
>>
Thanks for the hints Dmitry. I am trying my best but still have some
issues with the input_set/get_drvdata.

The kernel Oopses on NULL pointer dereference in mpr_touchkey_report.
Here is the backtrace:

[    2.916960] 8<--- cut here ---
[    2.920022] Unable to handle kernel NULL pointer dereference at virtual address 000001d0
[    2.928138] pgd = (ptrval)
[    2.930851] [000001d0] *pgd=00000000
[    2.934439] Internal error: Oops: 5 [#1] SMP ARM
[    2.939065] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.3.0-rc1-00001-g7278b7c3986c-dirty #2
[    2.947503] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    2.954044] PC is at mpr_touchkey_report+0x18/0x1bc
[    2.958932] LR is at input_dev_poller_start+0x30/0x3c
[    2.963987] pc : [<80728c50>]    lr : [<8071f444>]    psr: 20000013
[    2.970255] sp : e8131c10  ip : e8131c68  fp : e8131c64
[    2.975480] r10: 000000c9  r9 : 8108339c  r8 : 81083340
[    2.980707] r7 : 00000000  r6 : e86cf574  r5 : e86b8480  r4 : e86b8040
[    2.987236] r3 : 80728c38  r2 : e8128000  r1 : 00000000  r0 : 00000000
[    2.993767] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    3.000906] Control: 10c5387d  Table: 1000404a  DAC: 00000051
[    3.006656] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[    3.012667] Stack: (0xe8131c10 to 0xe8132000)
[    3.017033] 1c00:                                     60000013 8017f290 e8131c64 e8131c28
[    3.025219] 1c20: 8017f290 8017f098 80e7eb04 e8131cd4 0000013d 00000000 80bc6ba4 e86b8040
[    3.033403] 1c40: e86b8480 e86cf574 00000000 81083340 8108339c 000000c9 e8131c7c e8131c68
[    3.041587] 1c60: 8071f444 80728c44 e86cf400 e86b8480 e8131c9c e8131c80 8071d230 8071f420
[    3.049769] 1c80: e86b8480 00000000 e86cf400 8106baa4 e8131cbc e8131ca0 80591b00 8071d1a0
[    3.057951] 1ca0: 80c567c4 e86cf400 8106baa4 8106baa4 e8131cdc e8131cc0 8071d75c 80591a98
[    3.066136] 1cc0: e86cf400 00000000 e86ba1c0 8106baa4 e8131d04 e8131ce0 8071df50 8071d6cc
[    3.074320] 1ce0: 00000000 e834a400 e86cf400 e86ba0c0 e834a420 51eb851f e8131d54 e8131d08
[    3.082502] 1d00: 807291c4 8071dbc0 00000000 00000000 00000000 0d3abafe 00325aa0 81006548
[    3.090686] 1d20: 000003e8 0d3abafe 8063907c e834a420 80728e34 81083950 e834a400 00000000
[    3.098867] 1d40: 00000000 00000000 e8131d7c e8131d58 80747410 80728e40 81123a00 e834a420
[    3.107051] 1d60: 81123b0c 00000000 81083950 00000000 e8131dac e8131d80 8061f684 807471c4
[    3.115233] 1d80: 00000000 e834a420 81083950 81083950 81006548 00000000 80f8c83c 80f008ac
[    3.123415] 1da0: e8131de4 e8131db0 8061fca8 8061f590 e8131dcc e8131dc0 8080e234 8061ed70
[    3.131599] 1dc0: e834a420 00000000 81083950 81006548 00000000 80f8c83c e8131e04 e8131de8
[    3.139784] 1de0: 8061ffcc 8061fc44 81083950 e834a420 8061ffd4 81006548 e8131e24 e8131e08
[    3.147969] 1e00: 80620040 8061ff70 00000000 81083950 8061ffd4 81006548 e8131e54 e8131e28
[    3.156151] 1e20: 8061d974 8061ffe0 e8131e60 e821b758 e83413b4 0d3abafe 81083950 e8697f00
[    3.164333] 1e40: 81084b4c 00000000 e8131e64 e8131e58 806200e0 8061d8fc e8131e8c e8131e68
[    3.172517] 1e60: 8061e314 806200c0 80e7f9a0 e8131e78 81083950 81006548 80f4a898 ffffe000
[    3.180699] 1e80: e8131ea4 e8131e90 80620ce4 8061e1b8 81083934 81006548 e8131ebc e8131ea8
[    3.188882] 1ea0: 80748e18 80620c64 810c0660 81006548 e8131ecc e8131ec0 80f4a8bc 80748dd8
[    3.197065] 1ec0: e8131f44 e8131ed0 80f01330 80f4a8a4 00000000 80e12924 80e12904 80e12900
[    3.205250] 1ee0: 80e24610 81006548 00000000 80e128dc 00000006 00000006 00000000 80f008ac
[    3.213433] 1f00: 80e7eac0 80ee9234 8016f4d4 ebfffb37 ebfffb3f 0d3abafe e8131f44 0d3abafe
[    3.220284] g_ether gadget: high-speed config #1: CDC Ethernet (ECM)
[    3.221616] 1f20: 810c0660 00000007 810c0660 80fc21f0 810c5980 810c5980 e8131f94 e8131f48
[    3.236147] 1f40: 80f0174c 80f01274 00000006 00000006 00000000 80f008ac 801346cc 80133d54
[    3.244330] 1f60: 80ee9234 00000154 e8131f8c 00000000 80bdb670 00000000 00000000 00000000
[    3.252512] 1f80: 00000000 00000000 e8131fac e8131f98 80bdb688 80f0146c 00000000 80bdb670
[    3.260695] 1fa0: 00000000 e8131fb0 801010e8 80bdb67c 00000000 00000000 00000000 00000000
[    3.268878] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    3.277061] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    3.285237] Backtrace:
[    3.287699] [<80728c38>] (mpr_touchkey_report) from [<8071f444>] (input_dev_poller_start+0x30/0x3c)
[    3.296753]  r10:000000c9 r9:8108339c r8:81083340 r7:00000000 r6:e86cf574 r5:e86b8480
[    3.304584]  r4:e86b8040
[    3.307128] [<8071f414>] (input_dev_poller_start) from [<8071d230>] (input_open_device+0x9c/0xc4)
[    3.316002]  r5:e86b8480 r4:e86cf400
[    3.319590] [<8071d194>] (input_open_device) from [<80591b00>] (kbd_connect+0x74/0x90)
[    3.327510]  r7:8106baa4 r6:e86cf400 r5:00000000 r4:e86b8480
[    3.333180] [<80591a8c>] (kbd_connect) from [<8071d75c>] (input_attach_handler+0x9c/0xd0)
[    3.341361]  r7:8106baa4 r6:8106baa4 r5:e86cf400 r4:80c567c4
[    3.347029] [<8071d6c0>] (input_attach_handler) from [<8071df50>] (input_register_device+0x39c/0x40c)
[    3.356251]  r7:8106baa4 r6:e86ba1c0 r5:00000000 r4:e86cf400
[    3.361923] [<8071dbb4>] (input_register_device) from [<807291c4>] (mpr_touchkey_probe+0x390/0x4d4)
[    3.370974]  r9:51eb851f r8:e834a420 r7:e86ba0c0 r6:e86cf400 r5:e834a400 r4:00000000
[    3.378727] [<80728e34>] (mpr_touchkey_probe) from [<80747410>] (i2c_device_probe+0x258/0x27c)
[    3.387344]  r10:00000000 r9:00000000 r8:00000000 r7:e834a400 r6:81083950 r5:80728e34
[    3.395174]  r4:e834a420
[    3.397715] [<807471b8>] (i2c_device_probe) from [<8061f684>] (really_probe+0x100/0x2d8)
[    3.405810]  r9:00000000 r8:81083950 r7:00000000 r6:81123b0c r5:e834a420 r4:81123a00
[    3.413562] [<8061f584>] (really_probe) from [<8061fca8>] (driver_probe_device+0x70/0x180)
[    3.421832]  r10:80f008ac r9:80f8c83c r8:00000000 r7:81006548 r6:81083950 r5:81083950
[    3.429662]  r4:e834a420 r3:00000000
[    3.433245] [<8061fc38>] (driver_probe_device) from [<8061ffcc>] (device_driver_attach+0x68/0x70)
[    3.442121]  r9:80f8c83c r8:00000000 r7:81006548 r6:81083950 r5:00000000 r4:e834a420
[    3.449872] [<8061ff64>] (device_driver_attach) from [<80620040>] (__driver_attach+0x6c/0xe0)
[    3.458399]  r7:81006548 r6:8061ffd4 r5:e834a420 r4:81083950
[    3.464071] [<8061ffd4>] (__driver_attach) from [<8061d974>] (bus_for_each_dev+0x84/0xc4)
[    3.472251]  r7:81006548 r6:8061ffd4 r5:81083950 r4:00000000
[    3.477920] [<8061d8f0>] (bus_for_each_dev) from [<806200e0>] (driver_attach+0x2c/0x30)
[    3.485926]  r7:00000000 r6:81084b4c r5:e8697f00 r4:81083950
[    3.491594] [<806200b4>] (driver_attach) from [<8061e314>] (bus_add_driver+0x168/0x1ec)
[    3.499604] [<8061e1ac>] (bus_add_driver) from [<80620ce4>] (driver_register+0x8c/0x124)
[    3.507697]  r7:ffffe000 r6:80f4a898 r5:81006548 r4:81083950
[    3.513366] [<80620c58>] (driver_register) from [<80748e18>] (i2c_register_driver+0x4c/0x8c)
[    3.521804]  r5:81006548 r4:81083934
[    3.525394] [<80748dcc>] (i2c_register_driver) from [<80f4a8bc>] (mpr_touchkey_driver_init+0x24/0x28)
[    3.534616]  r5:81006548 r4:810c0660
[    3.538203] [<80f4a898>] (mpr_touchkey_driver_init) from [<80f01330>] (do_one_initcall+0xc8/0x1f8)
[    3.547169] [<80f01268>] (do_one_initcall) from [<80f0174c>] (kernel_init_freeable+0x2ec/0x380)
[    3.555872]  r8:810c5980 r7:810c5980 r6:80fc21f0 r5:810c0660 r4:00000007
[    3.562587] [<80f01460>] (kernel_init_freeable) from [<80bdb688>] (kernel_init+0x18/0x120)
[    3.570856]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:80bdb670
[    3.578685]  r4:00000000
[    3.581230] [<80bdb670>] (kernel_init) from [<801010e8>] (ret_from_fork+0x14/0x2c)
[    3.588802] Exception stack(0xe8131fb0 to 0xe8131ff8)
[    3.593859] 1fa0:                                     00000000 00000000 00000000 00000000
[    3.602042] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    3.610222] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    3.616839]  r5:80bdb670 r4:00000000
[    3.620422] Code: e24cb004 e24dd02c e52de004 e8bd4000 (e59051d0)
[    3.626572] ---[ end trace eb840c8cb957e159 ]---

I can confirm the poller mechanism works fine if I leave the
mpr_touchkey_report function empty and just return.

I can also confirm the interrupt mechanism works as fine if I bodge
a wire from some available GPIO (commented lines in dtsi).

Here is my code:

diff --git a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
index e8d800fec637..7516da441915 100644
--- a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
+++ b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
@@ -4,6 +4,7 @@
  
  #include <dt-bindings/gpio/gpio.h>
  #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/input/input.h>
  #include <dt-bindings/pwm/pwm.h>
  
  / {
@@ -330,6 +331,21 @@
  		vcc-supply = <&sw2_reg>;
  		status = "disabled";
  	};
+
+	touchkeys: keys@5a {
+		compatible = "fsl,mpr121-touchkey";
+		//pinctrl-0 = <&pinctrl_key_irq>;
+		reg = <0x5a>;
+		vdd-supply = <&sw2_reg>;
+		autorepeat;
+		linux,keycodes = <KEY_1>, <KEY_2>, <KEY_3>, <KEY_4>, <KEY_5>,
+				<KEY_6>, <KEY_7>, <KEY_8>, <KEY_9>,
+				<KEY_BACKSPACE>, <KEY_0>, <KEY_ENTER>;
+		linux,poll-interval = <50>;
+		//interrupt-parent = <&gpio1>;
+		//interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
+		status = "disabled";
+	};
  };
  
  &iomuxc {
@@ -433,6 +449,12 @@
  		>;
  	};
  
+	pinctrl_key_irq: keyirq {
+		fsl,pins = <
+			MX6QDL_PAD_SD1_CMD__GPIO1_IO18	0x1b098
+		>;
+	};
+
  	pinctrl_touch: touchgrp {
  		fsl,pins = <
  			MX6QDL_PAD_GPIO_19__GPIO4_IO05	0x1b098
diff --git a/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts b/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts
index f97927064750..84c275bfdd38 100644
--- a/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts
+++ b/arch/arm/boot/dts/imx6dl-yapp4-hydra.dts
@@ -45,6 +45,10 @@
  	status = "okay";
  };
  
+&touchkeys {
+	status = "okay";
+};
+
  &usdhc3 {
  	status = "okay";
  };
diff --git a/drivers/input/keyboard/mpr121_touchkey.c b/drivers/input/keyboard/mpr121_touchkey.c
index e9ceaa16b46a..d6b9f6acddca 100644
--- a/drivers/input/keyboard/mpr121_touchkey.c
+++ b/drivers/input/keyboard/mpr121_touchkey.c
@@ -7,7 +7,7 @@
   *
   * Based on mcs_touchkey.c
   */
-
+#define DEBUG
  #include <linux/bitops.h>
  #include <linux/delay.h>
  #include <linux/i2c.h>
@@ -54,6 +54,9 @@
  /* MPR121 has 12 keys */
  #define MPR121_MAX_KEY_COUNT		12
  
+#define MPR121_MIN_POLL_INTERVAL	10
+#define MPR121_MAX_POLL_INTERVAL	2000
+
  struct mpr121_touchkey {
  	struct i2c_client	*client;
  	struct input_dev	*input_dev;
@@ -115,11 +118,11 @@ static struct regulator *mpr121_vdd_supply_init(struct device *dev)
  	return vdd_supply;
  }
  
-static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
+static void mpr_touchkey_report(struct input_dev *dev)
  {
-	struct mpr121_touchkey *mpr121 = dev_id;
-	struct i2c_client *client = mpr121->client;
+	struct mpr121_touchkey *mpr121 = input_get_drvdata(dev);
  	struct input_dev *input = mpr121->input_dev;
+	struct i2c_client *client = mpr121->client;
  	unsigned long bit_changed;
  	unsigned int key_num;
  	int reg;
@@ -127,14 +130,14 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
  	reg = i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_1_ADDR);
  	if (reg < 0) {
  		dev_err(&client->dev, "i2c read error [%d]\n", reg);
-		goto out;
+		return;
  	}
  
  	reg <<= 8;
  	reg |= i2c_smbus_read_byte_data(client, ELE_TOUCH_STATUS_0_ADDR);
  	if (reg < 0) {
  		dev_err(&client->dev, "i2c read error [%d]\n", reg);
-		goto out;
+		return;
  	}
  
  	reg &= TOUCH_STATUS_MASK;
@@ -155,8 +158,13 @@ static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
  
  	}
  	input_sync(input);
+}
+
+static irqreturn_t mpr_touchkey_interrupt(int irq, void *dev_id)
+{
+	struct mpr121_touchkey *mpr121 = dev_id;
+	mpr_touchkey_report(mpr121->input_dev);
  
-out:
  	return IRQ_HANDLED;
  }
  
@@ -229,13 +237,10 @@ static int mpr_touchkey_probe(struct i2c_client *client,
  	int vdd_uv;
  	struct mpr121_touchkey *mpr121;
  	struct input_dev *input_dev;
+	u32 poll_interval = 0;
  	int error;
  	int i;
  
-	if (!client->irq) {
-		dev_err(dev, "irq number should not be zero\n");
-		return -EINVAL;
-	}
  
  	vdd_supply = mpr121_vdd_supply_init(dev);
  	if (IS_ERR(vdd_supply))
@@ -275,11 +280,13 @@ static int mpr_touchkey_probe(struct i2c_client *client,
  	if (device_property_read_bool(dev, "autorepeat"))
  		__set_bit(EV_REP, input_dev->evbit);
  	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+	input_set_drvdata(input_dev, mpr121);
  
  	input_dev->keycode = mpr121->keycodes;
  	input_dev->keycodesize = sizeof(mpr121->keycodes[0]);
  	input_dev->keycodemax = mpr121->keycount;
  
+
  	for (i = 0; i < mpr121->keycount; i++)
  		input_set_capability(input_dev, EV_KEY, mpr121->keycodes[i]);
  
@@ -289,13 +296,34 @@ static int mpr_touchkey_probe(struct i2c_client *client,
  		return error;
  	}
  
-	error = devm_request_threaded_irq(dev, client->irq, NULL,
-					  mpr_touchkey_interrupt,
-					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					  dev->driver->name, mpr121);
-	if (error) {
-		dev_err(dev, "Failed to register interrupt\n");
-		return error;
+	device_property_read_u32(dev, "linux,poll-interval", &poll_interval);
+
+	if (client->irq) {
+		error = devm_request_threaded_irq(dev, client->irq, NULL,
+						  mpr_touchkey_interrupt,
+						  IRQF_TRIGGER_FALLING |
+						  IRQF_ONESHOT,
+						  dev->driver->name, mpr121);
+		if (error) {
+			dev_err(dev, "Failed to register interrupt\n");
+			return error;
+		}
+	} else if (poll_interval > 0) {
+		error = input_setup_polling(input_dev, mpr_touchkey_report);
+		if (error) {
+			dev_err(dev, "Failed to setup polling\n");
+			return error;
+		}
+
+		input_set_poll_interval(input_dev, poll_interval);
+		input_set_min_poll_interval(input_dev,
+					    MPR121_MIN_POLL_INTERVAL);
+		input_set_max_poll_interval(input_dev,
+					    MPR121_MAX_POLL_INTERVAL);
+	} else {
+		dev_err(dev,
+			"invalid IRQ number and polling not configured\n");
+		return -EINVAL;
  	}
  
  	error = input_register_device(input_dev);
--

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

[Dmitry Torokhov]

On Tue, Jul 30, 2019 at 11:25:49AM +0200, Michal Vokáč wrote:
> On 27. 07. 19 9:31, Dmitry Torokhov wrote:
> > On Fri, Jul 26, 2019 at 01:31:31PM +0200, Michal Vokáč wrote:
> > > On 25. 07. 19 16:40, Dmitry Torokhov wrote:
> > > > 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.
> > > 
> > > Thank you, now it is (almost) good as you said :D
> > > 
> > > > > 
> > > > > > Input: add support for polling to input devices
> > > > > > 
> > > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > > 
> > > > > > 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;
> > > > }
> > > > 
> > > 
> > > Probably a trivial problem for experienced kernel hacker but I can not
> > > wrap my head around this - the interrupt handler takes the mpr121
> > > device id as an argument while the poller poll_fn takes struct input_dev.
> > > 
> > > I fail to figure out how to get the device id from the input device.
> > > 
> Thanks for the hints Dmitry. I am trying my best but still have some
> issues with the input_set/get_drvdata.
> 
> The kernel Oopses on NULL pointer dereference in mpr_touchkey_report.
> Here is the backtrace:
> 
> [    2.916960] 8<--- cut here ---
> [    2.920022] Unable to handle kernel NULL pointer dereference at virtual address 000001d0
> [    2.928138] pgd = (ptrval)

Ah, that's my fault I believe. Can you please try sticking

	poller->input = dev;

into input_setup_polling()?

Thanks.

-- 
Dmitry

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

[Michal Vokáč]

On 02. 08. 19 1:49, Dmitry Torokhov wrote:
> On Tue, Jul 30, 2019 at 11:25:49AM +0200, Michal Vokáč wrote:
>> On 27. 07. 19 9:31, Dmitry Torokhov wrote:
>>> On Fri, Jul 26, 2019 at 01:31:31PM +0200, Michal Vokáč wrote:
>>>> On 25. 07. 19 16:40, Dmitry Torokhov wrote:
>>>>> 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.
>>>>
>>>> Thank you, now it is (almost) good as you said :D
>>>>
>>>>>>
>>>>>>> Input: add support for polling to input devices
>>>>>>>
>>>>>>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>>>>
>>>>>>> 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;
>>>>> }
>>>>>
>>>>
>>>> Probably a trivial problem for experienced kernel hacker but I can not
>>>> wrap my head around this - the interrupt handler takes the mpr121
>>>> device id as an argument while the poller poll_fn takes struct input_dev.
>>>>
>>>> I fail to figure out how to get the device id from the input device.
>>>>
>> Thanks for the hints Dmitry. I am trying my best but still have some
>> issues with the input_set/get_drvdata.
>>
>> The kernel Oopses on NULL pointer dereference in mpr_touchkey_report.
>> Here is the backtrace:
>>
>> [    2.916960] 8<--- cut here ---
>> [    2.920022] Unable to handle kernel NULL pointer dereference at virtual address 000001d0
>> [    2.928138] pgd = (ptrval)
> 
> Ah, that's my fault I believe. Can you please try sticking
> 
> 	poller->input = dev;
> 
> into input_setup_polling()?
> 
Nice, that solved the problem and I confirm the poller mechanism works
as expected. The sysfs poll/min/max interface also works just fine.

Please Cc me when you submit your patch. I think you can already add
my "Tested-by: Michal Vokáč <michal.vokac@ysoft.com>".

I will send mine series when the poller is in your tree. I will include
the proposed DT binding change, adding the "linux,poll-interrupt"
property, though Rob did not respond to this yet.

What about the min/max poll interval limits? Was your idea those should
also be configurable from DT? Currently I defined some limits that are
reasonable for our use case but may not be suitable for someone else.

In the meantime I also need to improve reliability of the reading.
Sometimes the keys get stuck or an electrostatic discharge causes
reset of the chip. I will extract changes that deal with these problems
from the RFC series and from some downstream patches and submit those
later.

Thank you!
Michal