[PATCH 1/3] Input: zforce - use irq handler instead of gpio polling

From: Oleksij Rempel <redacted>

Remove the IRQ GPIO polling and request. Existing DTS should not be affected
since the IRQ registration was and is based on "interrupts" descriptor of DTS.

Signed-off-by: Oleksij Rempel <redacted>
Signed-off-by: Dirk Behme <redacted>
---

Note: All 3 patches in this series are against zforce in input next
      https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/log/?h=next
      to fit on top of the previous zforce gpiod change.

 drivers/input/touchscreen/zforce_ts.c | 135 ++++++++++++++++------------------
 1 file changed, 62 insertions(+), 73 deletions(-)

diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index 32749db..19dc297 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c

@@ -510,73 +510,71 @@ static irqreturn_t zforce_irq_thread(int irq, void *dev_id)
 	if (!ts->suspending && device_may_wakeup(&client->dev))
 		pm_stay_awake(&client->dev);
 
-	while (!gpiod_get_value_cansleep(ts->gpio_int)) {
-		ret = zforce_read_packet(ts, payload_buffer);
-		if (ret < 0) {
-			dev_err(&client->dev,
-				"could not read packet, ret: %d\n", ret);
-			break;
-		}
+	ret = zforce_read_packet(ts, payload_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"could not read packet, ret: %d\n", ret);
+		return IRQ_HANDLED;
+	}
+
+	payload =  &payload_buffer[PAYLOAD_BODY];
 
-		payload =  &payload_buffer[PAYLOAD_BODY];
-
-		switch (payload[RESPONSE_ID]) {
-		case NOTIFICATION_TOUCH:
-			/*
-			 * Always report touch-events received while
-			 * suspending, when being a wakeup source
-			 */
-			if (ts->suspending && device_may_wakeup(&client->dev))
-				pm_wakeup_event(&client->dev, 500);
-			zforce_touch_event(ts, &payload[RESPONSE_DATA]);
-			break;
-
-		case NOTIFICATION_BOOTCOMPLETE:
-			ts->boot_complete = payload[RESPONSE_DATA];
-			zforce_complete(ts, payload[RESPONSE_ID], 0);
-			break;
-
-		case RESPONSE_INITIALIZE:
-		case RESPONSE_DEACTIVATE:
-		case RESPONSE_SETCONFIG:
-		case RESPONSE_RESOLUTION:
-		case RESPONSE_SCANFREQ:
-			zforce_complete(ts, payload[RESPONSE_ID],
-					payload[RESPONSE_DATA]);
-			break;
-
-		case RESPONSE_STATUS:
-			/*
-			 * Version Payload Results
-			 * [2:major] [2:minor] [2:build] [2:rev]
-			 */
-			ts->version_major = (payload[RESPONSE_DATA + 1] << 8) |
-						payload[RESPONSE_DATA];
-			ts->version_minor = (payload[RESPONSE_DATA + 3] << 8) |
-						payload[RESPONSE_DATA + 2];
-			ts->version_build = (payload[RESPONSE_DATA + 5] << 8) |
-						payload[RESPONSE_DATA + 4];
-			ts->version_rev   = (payload[RESPONSE_DATA + 7] << 8) |
-						payload[RESPONSE_DATA + 6];
-			dev_dbg(&ts->client->dev,
-				"Firmware Version %04x:%04x %04x:%04x\n",
-				ts->version_major, ts->version_minor,
-				ts->version_build, ts->version_rev);
-
-			zforce_complete(ts, payload[RESPONSE_ID], 0);
-			break;
-
-		case NOTIFICATION_INVALID_COMMAND:
-			dev_err(&ts->client->dev, "invalid command: 0x%x\n",
+	switch (payload[RESPONSE_ID]) {
+	case NOTIFICATION_TOUCH:
+		/*
+		 * Always report touch-events received while
+		 * suspending, when being a wakeup source
+		 */
+		if (ts->suspending && device_may_wakeup(&client->dev))
+			pm_wakeup_event(&client->dev, 500);
+		zforce_touch_event(ts, &payload[RESPONSE_DATA]);
+		break;
+
+	case NOTIFICATION_BOOTCOMPLETE:
+		ts->boot_complete = payload[RESPONSE_DATA];
+		zforce_complete(ts, payload[RESPONSE_ID], 0);
+		break;
+
+	case RESPONSE_INITIALIZE:
+	case RESPONSE_DEACTIVATE:
+	case RESPONSE_SETCONFIG:
+	case RESPONSE_RESOLUTION:
+	case RESPONSE_SCANFREQ:
+		zforce_complete(ts, payload[RESPONSE_ID],
 				payload[RESPONSE_DATA]);
-			break;
+		break;
 
-		default:
-			dev_err(&ts->client->dev,
-				"unrecognized response id: 0x%x\n",
-				payload[RESPONSE_ID]);
-			break;
-		}
+	case RESPONSE_STATUS:
+		/*
+		 * Version Payload Results
+		 * [2:major] [2:minor] [2:build] [2:rev]
+		 */
+		ts->version_major = (payload[RESPONSE_DATA + 1] << 8) |
+					payload[RESPONSE_DATA];
+		ts->version_minor = (payload[RESPONSE_DATA + 3] << 8) |
+					payload[RESPONSE_DATA + 2];
+		ts->version_build = (payload[RESPONSE_DATA + 5] << 8) |
+					payload[RESPONSE_DATA + 4];
+		ts->version_rev   = (payload[RESPONSE_DATA + 7] << 8) |
+					payload[RESPONSE_DATA + 6];
+		dev_dbg(&ts->client->dev,
+			"Firmware Version %04x:%04x %04x:%04x\n",
+			ts->version_major, ts->version_minor,
+			ts->version_build, ts->version_rev);
+
+		zforce_complete(ts, payload[RESPONSE_ID], 0);
+		break;
+
+	case NOTIFICATION_INVALID_COMMAND:
+		dev_err(&ts->client->dev, "invalid command: 0x%x\n",
+			payload[RESPONSE_DATA]);
+		break;
+
+	default:
+		dev_err(&ts->client->dev,
+			"unrecognized response id: 0x%x\n",
+			payload[RESPONSE_ID]);
+		break;
 	}
 
 	if (!ts->suspending && device_may_wakeup(&client->dev))

@@ -754,15 +752,6 @@ static int zforce_probe(struct i2c_client *client,
 	if (!ts)
 		return -ENOMEM;
 
-	/* INT GPIO */
-	ts->gpio_int = devm_gpiod_get_index(&client->dev, NULL, 0, GPIOD_IN);
-	if (IS_ERR(ts->gpio_int)) {
-		ret = PTR_ERR(ts->gpio_int);
-		dev_err(&client->dev,
-			"failed to request interrupt GPIO: %d\n", ret);
-		return ret;
-	}
-
 	/* RST GPIO */
 	ts->gpio_rst = devm_gpiod_get_index(&client->dev, NULL, 1,
 					    GPIOD_OUT_HIGH);

-- 
2.3.4

From: Oleksij Rempel <redacted>

If zforce is not ready to process the interrupt, the touchscreen will
be lost forever. Make sure we enable the interrupt only if processing
is ready.

Signed-off-by: Oleksij Rempel <redacted>
Signed-off-by: Dirk Behme <redacted>
---
 drivers/input/touchscreen/zforce_ts.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index 19dc297..a1889e5 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c

@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>

@@ -741,6 +742,7 @@ static int zforce_probe(struct i2c_client *client,
 	struct zforce_ts *ts;
 	struct input_dev *input_dev;
 	int ret;
+	unsigned int irq;
 
 	if (!pdata) {
 		pdata = zforce_parse_dt(&client->dev);

@@ -835,6 +837,7 @@ static int zforce_probe(struct i2c_client *client,
 
 	init_completion(&ts->command_done);
 
+	irq = client->irq;
 	/*
 	 * The zforce pulls the interrupt low when it has data ready.
 	 * After it is triggered the isr thread runs until all the available

@@ -842,7 +845,8 @@ static int zforce_probe(struct i2c_client *client,
 	 * Therefore we can trigger the interrupt anytime it is low and do
 	 * not need to limit it to the interrupt edge.
 	 */
-	ret = devm_request_threaded_irq(&client->dev, client->irq,
+	irq_set_status_flags(irq, IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(&client->dev, irq,
 					zforce_irq, zforce_irq_thread,
 					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 					input_dev->name, ts);

@@ -855,6 +859,7 @@ static int zforce_probe(struct i2c_client *client,
 
 	/* let the controller boot */
 	zforce_reset_deassert(ts);
+	enable_irq(irq);
 
 	ts->command_waiting = NOTIFICATION_BOOTCOMPLETE;
 	if (wait_for_completion_timeout(&ts->command_done, WAIT_TIMEOUT) == 0)

-- 
2.3.4

From: Oleksij Rempel <redacted>

zforce_irq will not be executed, since we use IRQF_ONESHOT.

Signed-off-by: Oleksij Rempel <redacted>
Signed-off-by: Dirk Behme <redacted>
---
 drivers/input/touchscreen/zforce_ts.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index a1889e5..96bec6a 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c

@@ -477,17 +477,6 @@ static void zforce_complete(struct zforce_ts *ts, int cmd, int result)
 	}
 }
 
-static irqreturn_t zforce_irq(int irq, void *dev_id)
-{
-	struct zforce_ts *ts = dev_id;
-	struct i2c_client *client = ts->client;
-
-	if (ts->suspended && device_may_wakeup(&client->dev))
-		pm_wakeup_event(&client->dev, 500);
-
-	return IRQ_WAKE_THREAD;
-}
-
 static irqreturn_t zforce_irq_thread(int irq, void *dev_id)
 {
 	struct zforce_ts *ts = dev_id;

@@ -847,7 +836,7 @@ static int zforce_probe(struct i2c_client *client,
 	 */
 	irq_set_status_flags(irq, IRQ_NOAUTOEN);
 	ret = devm_request_threaded_irq(&client->dev, irq,
-					zforce_irq, zforce_irq_thread,
+					NULL, zforce_irq_thread,
 					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 					input_dev->name, ts);
 	if (ret) {

-- 
2.3.4

On Mon, Jul 13, 2015 at 02:49:37PM +0200, Dirk Behme wrote:

From: Oleksij Rempel <redacted>

Remove the IRQ GPIO polling and request. Existing DTS should not be affected
since the IRQ registration was and is based on "interrupts" descriptor of DTS.

But this means that consecutive touchscreen readings will be delayed by
the time it takes to schedule the thread.

What is the motivation for this change?

Thanks.

-- 
Dmitry

On Mon, Jul 13, 2015 at 02:49:38PM +0200, Dirk Behme wrote:

Instead of playing with the flags should we simply request irq after we
release the reset pin?

Thanks.

-- 
Dmitry

On Mon, Jul 13, 2015 at 02:49:39PM +0200, Dirk Behme wrote:

From: Oleksij Rempel <redacted>

zforce_irq will not be executed, since we use IRQF_ONESHOT.

? This does not make sense. Even with IRQF_ONESHOT the hard interrupt
handles does get called. See handle_irq_event_percpu().

Thanks.

-- 
Dmitry

Hi Dmitry,

On 13.07.2015 19:04, Dmitry Torokhov wrote:

On Mon, Jul 13, 2015 at 02:49:37PM +0200, Dirk Behme wrote:

quoted

From: Oleksij Rempel <redacted>

Remove the IRQ GPIO polling and request. Existing DTS should not be affected
since the IRQ registration was and is based on "interrupts" descriptor of DTS.

But this means that consecutive touchscreen readings will be delayed by
the time it takes to schedule the thread.

What is the motivation for this change?

This is the generic part we've done for a special hardware 
configuration: There is some hardware which uses an I2C Serializer / 
Deserializer (SerDes) to communicate with the zFroce touch driver. In 
this case the SerDes will be configured as an interrupt controller and 
the zForce driver will have no access to poll the GPIO line.

Best regards

Di

quoted

Signed-off-by: Oleksij Rempel <redacted>
Signed-off-by: Dirk Behme <redacted>
---

Note: All 3 patches in this series are against zforce in input next
       https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/log/?h=next
       to fit on top of the previous zforce gpiod change.

  drivers/input/touchscreen/zforce_ts.c | 135 ++++++++++++++++------------------
  1 file changed, 62 insertions(+), 73 deletions(-)

diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index 32749db..19dc297 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c

@@ -510,73 +510,71 @@ static irqreturn_t zforce_irq_thread(int irq, void *dev_id)
  	if (!ts->suspending && device_may_wakeup(&client->dev))
  		pm_stay_awake(&client->dev);

-	while (!gpiod_get_value_cansleep(ts->gpio_int)) {
-		ret = zforce_read_packet(ts, payload_buffer);
-		if (ret < 0) {
-			dev_err(&client->dev,
-				"could not read packet, ret: %d\n", ret);
-			break;
-		}
+	ret = zforce_read_packet(ts, payload_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"could not read packet, ret: %d\n", ret);
+		return IRQ_HANDLED;
+	}
+
+	payload =  &payload_buffer[PAYLOAD_BODY];

-		payload =  &payload_buffer[PAYLOAD_BODY];
-
-		switch (payload[RESPONSE_ID]) {
-		case NOTIFICATION_TOUCH:
-			/*
-			 * Always report touch-events received while
-			 * suspending, when being a wakeup source
-			 */
-			if (ts->suspending && device_may_wakeup(&client->dev))
-				pm_wakeup_event(&client->dev, 500);
-			zforce_touch_event(ts, &payload[RESPONSE_DATA]);
-			break;
-
-		case NOTIFICATION_BOOTCOMPLETE:
-			ts->boot_complete = payload[RESPONSE_DATA];
-			zforce_complete(ts, payload[RESPONSE_ID], 0);
-			break;
-
-		case RESPONSE_INITIALIZE:
-		case RESPONSE_DEACTIVATE:
-		case RESPONSE_SETCONFIG:
-		case RESPONSE_RESOLUTION:
-		case RESPONSE_SCANFREQ:
-			zforce_complete(ts, payload[RESPONSE_ID],
-					payload[RESPONSE_DATA]);
-			break;
-
-		case RESPONSE_STATUS:
-			/*
-			 * Version Payload Results
-			 * [2:major] [2:minor] [2:build] [2:rev]
-			 */
-			ts->version_major = (payload[RESPONSE_DATA + 1] << 8) |
-						payload[RESPONSE_DATA];
-			ts->version_minor = (payload[RESPONSE_DATA + 3] << 8) |
-						payload[RESPONSE_DATA + 2];
-			ts->version_build = (payload[RESPONSE_DATA + 5] << 8) |
-						payload[RESPONSE_DATA + 4];
-			ts->version_rev   = (payload[RESPONSE_DATA + 7] << 8) |
-						payload[RESPONSE_DATA + 6];
-			dev_dbg(&ts->client->dev,
-				"Firmware Version %04x:%04x %04x:%04x\n",
-				ts->version_major, ts->version_minor,
-				ts->version_build, ts->version_rev);
-
-			zforce_complete(ts, payload[RESPONSE_ID], 0);
-			break;
-
-		case NOTIFICATION_INVALID_COMMAND:
-			dev_err(&ts->client->dev, "invalid command: 0x%x\n",
+	switch (payload[RESPONSE_ID]) {
+	case NOTIFICATION_TOUCH:
+		/*
+		 * Always report touch-events received while
+		 * suspending, when being a wakeup source
+		 */
+		if (ts->suspending && device_may_wakeup(&client->dev))
+			pm_wakeup_event(&client->dev, 500);
+		zforce_touch_event(ts, &payload[RESPONSE_DATA]);
+		break;
+
+	case NOTIFICATION_BOOTCOMPLETE:
+		ts->boot_complete = payload[RESPONSE_DATA];
+		zforce_complete(ts, payload[RESPONSE_ID], 0);
+		break;
+
+	case RESPONSE_INITIALIZE:
+	case RESPONSE_DEACTIVATE:
+	case RESPONSE_SETCONFIG:
+	case RESPONSE_RESOLUTION:
+	case RESPONSE_SCANFREQ:
+		zforce_complete(ts, payload[RESPONSE_ID],
  				payload[RESPONSE_DATA]);
-			break;
+		break;

-		default:
-			dev_err(&ts->client->dev,
-				"unrecognized response id: 0x%x\n",
-				payload[RESPONSE_ID]);
-			break;
-		}
+	case RESPONSE_STATUS:
+		/*
+		 * Version Payload Results
+		 * [2:major] [2:minor] [2:build] [2:rev]
+		 */
+		ts->version_major = (payload[RESPONSE_DATA + 1] << 8) |
+					payload[RESPONSE_DATA];
+		ts->version_minor = (payload[RESPONSE_DATA + 3] << 8) |
+					payload[RESPONSE_DATA + 2];
+		ts->version_build = (payload[RESPONSE_DATA + 5] << 8) |
+					payload[RESPONSE_DATA + 4];
+		ts->version_rev   = (payload[RESPONSE_DATA + 7] << 8) |
+					payload[RESPONSE_DATA + 6];
+		dev_dbg(&ts->client->dev,
+			"Firmware Version %04x:%04x %04x:%04x\n",
+			ts->version_major, ts->version_minor,
+			ts->version_build, ts->version_rev);
+
+		zforce_complete(ts, payload[RESPONSE_ID], 0);
+		break;
+
+	case NOTIFICATION_INVALID_COMMAND:
+		dev_err(&ts->client->dev, "invalid command: 0x%x\n",
+			payload[RESPONSE_DATA]);
+		break;
+
+	default:
+		dev_err(&ts->client->dev,
+			"unrecognized response id: 0x%x\n",
+			payload[RESPONSE_ID]);
+		break;
  	}

  	if (!ts->suspending && device_may_wakeup(&client->dev))

@@ -754,15 +752,6 @@ static int zforce_probe(struct i2c_client *client,
  	if (!ts)
  		return -ENOMEM;

-	/* INT GPIO */
-	ts->gpio_int = devm_gpiod_get_index(&client->dev, NULL, 0, GPIOD_IN);
-	if (IS_ERR(ts->gpio_int)) {
-		ret = PTR_ERR(ts->gpio_int);
-		dev_err(&client->dev,
-			"failed to request interrupt GPIO: %d\n", ret);
-		return ret;
-	}
-
  	/* RST GPIO */
  	ts->gpio_rst = devm_gpiod_get_index(&client->dev, NULL, 1,
  					    GPIOD_OUT_HIGH);
--

2.3.4

On Tue, Jul 14, 2015 at 09:42:00AM +0200, Dirk Behme wrote:

Hi Dmitry,

On 13.07.2015 19:04, Dmitry Torokhov wrote:

quoted

On Mon, Jul 13, 2015 at 02:49:37PM +0200, Dirk Behme wrote:

quoted

From: Oleksij Rempel <redacted>

Remove the IRQ GPIO polling and request. Existing DTS should not be affected
since the IRQ registration was and is based on "interrupts" descriptor of DTS.

But this means that consecutive touchscreen readings will be delayed by
the time it takes to schedule the thread.

What is the motivation for this change?

This is the generic part we've done for a special hardware
configuration: There is some hardware which uses an I2C Serializer /
Deserializer (SerDes) to communicate with the zFroce touch driver.
In this case the SerDes will be configured as an interrupt
controller and the zForce driver will have no access to poll the
GPIO line.

In this case can we make gpio optional and use it if it is specified
falling back on one read per ISR invocation in its absence?

Thanks.

-- 
Dmitry