Message-ID: <dcee1139-c53f-5ea0-f387-a3aa5a9bf39f@ysoft.com>
Date:   Fri, 26 Jul 2019 13:31:31 +0200
From:   Michal Vokáč <michal.vokac@...ft.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Pengutronix Kernel Team <kernel@...gutronix.de>
Subject: Re: [RFC PATCH v2 0/4] Input: mpr121-polled: Add polled driver for
 MPR121

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@...il.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);
--

Powered by blists - more mailing lists