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

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

[Dirk Behme]

From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>

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 <external.Oleksij.Rempel@de.bosch.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---

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

[PATCH 2/3] Input: zforce - enable irq only if we are ready to process it

[Dirk Behme]

From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>

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 <external.Oleksij.Rempel@de.bosch.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 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

[PATCH 3/3] Input: zforce - remove zforce_irq

[Dirk Behme]

From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>

zforce_irq will not be executed, since we use IRQF_ONESHOT.

Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 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

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

[Dmitry Torokhov]

On Mon, Jul 13, 2015 at 02:49:37PM +0200, Dirk Behme wrote:
> From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> 
> 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.

> 
> Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
> 
> 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
> 

-- 
Dmitry

Re: [PATCH 2/3] Input: zforce - enable irq only if we are ready to process it

[Dmitry Torokhov]

On Mon, Jul 13, 2015 at 02:49:38PM +0200, Dirk Behme wrote:
> From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> 
> 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 <external.Oleksij.Rempel@de.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  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);

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

> +	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
> 

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] Input: zforce - remove zforce_irq

[Dmitry Torokhov]

On Mon, Jul 13, 2015 at 02:49:39PM +0200, Dirk Behme wrote:
> From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> 
> 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.

> 
> Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  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
> 

-- 
Dmitry

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

[Dirk Behme]

Hi Dmitry,

On 13.07.2015 19:04, Dmitry Torokhov wrote:
> On Mon, Jul 13, 2015 at 02:49:37PM +0200, Dirk Behme wrote:
>> From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
>>
>> 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
>>
>> Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>>
>> 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
>>

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

[Dmitry Torokhov]

On Tue, Jul 14, 2015 at 09:42:00AM +0200, Dirk Behme wrote:
> Hi Dmitry,
> 
> On 13.07.2015 19:04, Dmitry Torokhov wrote:
> >On Mon, Jul 13, 2015 at 02:49:37PM +0200, Dirk Behme wrote:
> >>From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> >>
> >>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

Re: [PATCH 2/3] Input: zforce - enable irq only if we are ready to process it

[Dmitry Torokhov]

Hi Oleksij,

On Tue, Jul 14, 2015 at 08:40:47AM +0200, external.Oleksij.Rempel wrote:
> Hi Dmitry,
> 
> On 13.07.2015 19:07, Dmitry Torokhov wrote:
> >On Mon, Jul 13, 2015 at 02:49:38PM +0200, Dirk Behme wrote:
> >>From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> >>
> >>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 <external.Oleksij.Rempel@de.bosch.com>
> >>Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> >>---
> >>  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);
> >Instead of playing with the flags should we simply request irq after we
> >release the reset pin?
> 
> Because we should be able to process the interrupt. If i put
> request_irq after reset, some times the interrupt can be missed.

OK, I see that enabling after reset is racy. But I am still confused why
having IRQ enabled while reste pin is asserted results in lost
touchscreen. Can you walk me through the sequence of events?

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: zforce - enable irq only if we are ready to process it

[Dmitry Torokhov]

On Mon, Jul 20, 2015 at 07:19:21AM +0200, external.Oleksij.Rempel wrote:
> Hi Dmitry,
> 
> 
> On 17.07.2015 23:58, Dmitry Torokhov wrote:
> >Hi Oleksij,
> >
> >On Tue, Jul 14, 2015 at 08:40:47AM +0200, external.Oleksij.Rempel wrote:
> >>Hi Dmitry,
> >>
> >>On 13.07.2015 19:07, Dmitry Torokhov wrote:
> >>>On Mon, Jul 13, 2015 at 02:49:38PM +0200, Dirk Behme wrote:
> >>>>From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> >>>>
> >>>>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 <external.Oleksij.Rempel@de.bosch.com>
> >>>>Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> >>>>---
> >>>>  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);
> >>>Instead of playing with the flags should we simply request irq after we
> >>>release the reset pin?
> >>Because we should be able to process the interrupt. If i put
> >>request_irq after reset, some times the interrupt can be missed.
> >OK, I see that enabling after reset is racy. But I am still confused why
> >having IRQ enabled while reste pin is asserted results in lost
> >touchscreen. Can you walk me through the sequence of events?
> >
> >Thanks.
> Mostly it will fail if zforce attached over SerDes
> (Serializer/Deserializer). If we register IRQ after release of reset
> pin, SerDes will get irq just before it has irq handler. In this
> case irq will be lost, which means, not BOOT event will be passed to
> the zforce.

Yes, I agreed that requesting interrupt after releasing reset pin is not
a good idea. But what is the issue with the original code that requests
interrupt first (and enables it) and then releases the reset pin?

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: zforce - enable irq only if we are ready to process it

[Dmitry Torokhov]

On Mon, Jul 20, 2015 at 08:59:35AM +0200, external.Oleksij.Rempel wrote:
> On 20.07.2015 08:35, Dmitry Torokhov wrote:
> >On Mon, Jul 20, 2015 at 07:19:21AM +0200, external.Oleksij.Rempel wrote:
> >>Hi Dmitry,
> >>
> >>
> >>On 17.07.2015 23:58, Dmitry Torokhov wrote:
> >>>Hi Oleksij,
> >>>
> >>>On Tue, Jul 14, 2015 at 08:40:47AM +0200, external.Oleksij.Rempel wrote:
> >>>>Hi Dmitry,
> >>>>
> >>>>On 13.07.2015 19:07, Dmitry Torokhov wrote:
> >>>>>On Mon, Jul 13, 2015 at 02:49:38PM +0200, Dirk Behme wrote:
> >>>>>>From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> >>>>>>
> >>>>>>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 <external.Oleksij.Rempel@de.bosch.com>
> >>>>>>Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> >>>>>>---
> >>>>>>  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);
> >>>>>Instead of playing with the flags should we simply request irq after we
> >>>>>release the reset pin?
> >>>>Because we should be able to process the interrupt. If i put
> >>>>request_irq after reset, some times the interrupt can be missed.
> >>>OK, I see that enabling after reset is racy. But I am still confused why
> >>>having IRQ enabled while reste pin is asserted results in lost
> >>>touchscreen. Can you walk me through the sequence of events?
> >>>
> >>>Thanks.
> >>Mostly it will fail if zforce attached over SerDes
> >>(Serializer/Deserializer). If we register IRQ after release of reset
> >>pin, SerDes will get irq just before it has irq handler. In this
> >>case irq will be lost, which means, not BOOT event will be passed to
> >>the zforce.
> >Yes, I agreed that requesting interrupt after releasing reset pin is not
> >a good idea. But what is the issue with the original code that requests
> >interrupt first (and enables it) and then releases the reset pin?
> 
> If i remember it correctly SerDes will catch spurious interrupt from
> the line and trigger it before client data was set. This means,
> interrupt handler will need extra sanity checks.

Client data is only referenced in suspend/resume routines though and
they are not called until registration is completed.

Thanks.

-- 
Dmitry