All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] input: goodix: Poll the 'buffer status' bit before reading data
@ 2017-03-30 13:33 ` Paul Cercueil
  0 siblings, 0 replies; 5+ messages in thread
From: Paul Cercueil @ 2017-03-30 13:33 UTC (permalink / raw)
  To: Bastien Nocera, Dmitry Torokhov; +Cc: linux-input, linux-kernel, Paul Cercueil

The Goodix panel triggers an interrupt on touch events. However, its
registers will contain the valid values a short time after the
interrupt, and not when it's raised. At that moment, the 'buffer status'
bit is set.

Previously, if the 'buffer status' bit was not set when the registers
were read, the data was discarded and no input event was emitted,
causing "finger down" or "finger up" events to be missed sometimes.

This went unnoticed until v4.9, as the DesignWare I2C driver commonly
used with this driver had enough latency for that bug to never trigger.

Now, in the IRQ handler we will poll (with a timeout) the 'buffer status'
bit and process the data of the panel as soon as this bit gets set.

Note that the Goodix panel will send a few spurious interrupts after the
'finger up' event, in which the 'buffer status' bit will never be set.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/input/touchscreen/goodix.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f3ee97..7a8b89b87c2f 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -195,18 +195,34 @@ static int goodix_get_cfg_len(u16 id)
 
 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 {
+	unsigned int timeout = 100;
 	int touch_num;
 	int error;
 
-	error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
+	/* The 'buffer status' bit, which indicates that the data is valid, is
+	 * not set as soon as the interrupt is raised, but slightly after.
+	 */
+	do {
+		error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
 				GOODIX_CONTACT_SIZE + 1);
-	if (error) {
-		dev_err(&ts->client->dev, "I2C transfer error: %d\n", error);
-		return error;
-	}
+		if (error) {
+			dev_err(&ts->client->dev, "I2C transfer error: %d\n",
+					error);
+			return error;
+		}
 
-	if (!(data[0] & 0x80))
-		return -EAGAIN;
+		if (data[0] & 0x80)
+			break;
+
+		usleep_range(100, 200);
+	} while (--timeout);
+
+	if (!timeout) {
+		/* The Goodix panel will send spurious interrupts after a
+		 * 'finger up' event, which will always cause a timeout.
+		 */
+		return 0;
+	}
 
 	touch_num = data[0] & 0x0f;
 	if (touch_num > ts->max_touch_num)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH] input: goodix: Poll the 'buffer status' bit before reading data
@ 2017-03-30 13:33 ` Paul Cercueil
  0 siblings, 0 replies; 5+ messages in thread
From: Paul Cercueil @ 2017-03-30 13:33 UTC (permalink / raw)
  To: Bastien Nocera, Dmitry Torokhov; +Cc: linux-input, linux-kernel, Paul Cercueil

The Goodix panel triggers an interrupt on touch events. However, its
registers will contain the valid values a short time after the
interrupt, and not when it's raised. At that moment, the 'buffer status'
bit is set.

Previously, if the 'buffer status' bit was not set when the registers
were read, the data was discarded and no input event was emitted,
causing "finger down" or "finger up" events to be missed sometimes.

This went unnoticed until v4.9, as the DesignWare I2C driver commonly
used with this driver had enough latency for that bug to never trigger.

Now, in the IRQ handler we will poll (with a timeout) the 'buffer status'
bit and process the data of the panel as soon as this bit gets set.

Note that the Goodix panel will send a few spurious interrupts after the
'finger up' event, in which the 'buffer status' bit will never be set.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/input/touchscreen/goodix.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f3ee97..7a8b89b87c2f 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -195,18 +195,34 @@ static int goodix_get_cfg_len(u16 id)
 
 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 {
+	unsigned int timeout = 100;
 	int touch_num;
 	int error;
 
-	error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
+	/* The 'buffer status' bit, which indicates that the data is valid, is
+	 * not set as soon as the interrupt is raised, but slightly after.
+	 */
+	do {
+		error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
 				GOODIX_CONTACT_SIZE + 1);
-	if (error) {
-		dev_err(&ts->client->dev, "I2C transfer error: %d\n", error);
-		return error;
-	}
+		if (error) {
+			dev_err(&ts->client->dev, "I2C transfer error: %d\n",
+					error);
+			return error;
+		}
 
-	if (!(data[0] & 0x80))
-		return -EAGAIN;
+		if (data[0] & 0x80)
+			break;
+
+		usleep_range(100, 200);
+	} while (--timeout);
+
+	if (!timeout) {
+		/* The Goodix panel will send spurious interrupts after a
+		 * 'finger up' event, which will always cause a timeout.
+		 */
+		return 0;
+	}
 
 	touch_num = data[0] & 0x0f;
 	if (touch_num > ts->max_touch_num)
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] input: goodix: Poll the 'buffer status' bit before reading data
  2017-03-30 13:33 ` Paul Cercueil
  (?)
@ 2017-03-30 22:48 ` Dmitry Torokhov
  2017-03-31 10:18   ` Paul Cercueil
  -1 siblings, 1 reply; 5+ messages in thread
From: Dmitry Torokhov @ 2017-03-30 22:48 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: Bastien Nocera, linux-input, linux-kernel

On Thu, Mar 30, 2017 at 03:33:49PM +0200, Paul Cercueil wrote:
> The Goodix panel triggers an interrupt on touch events. However, its
> registers will contain the valid values a short time after the
> interrupt, and not when it's raised. At that moment, the 'buffer status'
> bit is set.

Yay for awesome hardware.

> 
> Previously, if the 'buffer status' bit was not set when the registers
> were read, the data was discarded and no input event was emitted,
> causing "finger down" or "finger up" events to be missed sometimes.
> 
> This went unnoticed until v4.9, as the DesignWare I2C driver commonly
> used with this driver had enough latency for that bug to never trigger.
> 
> Now, in the IRQ handler we will poll (with a timeout) the 'buffer status'
> bit and process the data of the panel as soon as this bit gets set.
> 
> Note that the Goodix panel will send a few spurious interrupts after the
> 'finger up' event, in which the 'buffer status' bit will never be set.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/input/touchscreen/goodix.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 240b16f3ee97..7a8b89b87c2f 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -195,18 +195,34 @@ static int goodix_get_cfg_len(u16 id)
>  
>  static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
>  {
> +	unsigned int timeout = 100;
>  	int touch_num;
>  	int error;
>  
> -	error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
> +	/* The 'buffer status' bit, which indicates that the data is valid, is
> +	 * not set as soon as the interrupt is raised, but slightly after.
> +	 */
> +	do {
> +		error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
>  				GOODIX_CONTACT_SIZE + 1);
> -	if (error) {
> -		dev_err(&ts->client->dev, "I2C transfer error: %d\n", error);
> -		return error;
> -	}
> +		if (error) {
> +			dev_err(&ts->client->dev, "I2C transfer error: %d\n",
> +					error);
> +			return error;
> +		}
>  
> -	if (!(data[0] & 0x80))
> -		return -EAGAIN;
> +		if (data[0] & 0x80)
> +			break;
> +
> +		usleep_range(100, 200);

What is the typical delay between IRQ and data being ready? 100 repeats
suggest that poll interval is too small.

> +	} while (--timeout);
> +
> +	if (!timeout) {
> +		/* The Goodix panel will send spurious interrupts after a
> +		 * 'finger up' event, which will always cause a timeout.
> +		 */
> +		return 0;
> +	}
>  
>  	touch_num = data[0] & 0x0f;
>  	if (touch_num > ts->max_touch_num)
> -- 
> 2.11.0
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] input: goodix: Poll the 'buffer status' bit before reading data
  2017-03-30 22:48 ` Dmitry Torokhov
@ 2017-03-31 10:18   ` Paul Cercueil
  0 siblings, 0 replies; 5+ messages in thread
From: Paul Cercueil @ 2017-03-31 10:18 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Bastien Nocera, linux-input, linux-kernel

Hi,

Le 2017-03-31 00:48, Dmitry Torokhov a écrit :
> On Thu, Mar 30, 2017 at 03:33:49PM +0200, Paul Cercueil wrote:
>> The Goodix panel triggers an interrupt on touch events. However, its
>> registers will contain the valid values a short time after the
>> interrupt, and not when it's raised. At that moment, the 'buffer 
>> status'
>> bit is set.
> 
> Yay for awesome hardware.
> 
>> 
>> Previously, if the 'buffer status' bit was not set when the registers
>> were read, the data was discarded and no input event was emitted,
>> causing "finger down" or "finger up" events to be missed sometimes.
>> 
>> This went unnoticed until v4.9, as the DesignWare I2C driver commonly
>> used with this driver had enough latency for that bug to never 
>> trigger.
>> 
>> Now, in the IRQ handler we will poll (with a timeout) the 'buffer 
>> status'
>> bit and process the data of the panel as soon as this bit gets set.
>> 
>> Note that the Goodix panel will send a few spurious interrupts after 
>> the
>> 'finger up' event, in which the 'buffer status' bit will never be set.
>> 
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>>  drivers/input/touchscreen/goodix.c | 30 
>> +++++++++++++++++++++++-------
>>  1 file changed, 23 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/input/touchscreen/goodix.c 
>> b/drivers/input/touchscreen/goodix.c
>> index 240b16f3ee97..7a8b89b87c2f 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -195,18 +195,34 @@ static int goodix_get_cfg_len(u16 id)
>> 
>>  static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 
>> *data)
>>  {
>> +	unsigned int timeout = 100;
>>  	int touch_num;
>>  	int error;
>> 
>> -	error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
>> +	/* The 'buffer status' bit, which indicates that the data is valid, 
>> is
>> +	 * not set as soon as the interrupt is raised, but slightly after.
>> +	 */
>> +	do {
>> +		error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
>>  				GOODIX_CONTACT_SIZE + 1);
>> -	if (error) {
>> -		dev_err(&ts->client->dev, "I2C transfer error: %d\n", error);
>> -		return error;
>> -	}
>> +		if (error) {
>> +			dev_err(&ts->client->dev, "I2C transfer error: %d\n",
>> +					error);
>> +			return error;
>> +		}
>> 
>> -	if (!(data[0] & 0x80))
>> -		return -EAGAIN;
>> +		if (data[0] & 0x80)
>> +			break;
>> +
>> +		usleep_range(100, 200);
> 
> What is the typical delay between IRQ and data being ready? 100 repeats
> suggest that poll interval is too small.
> 

On my panel it seems to be about 10ms - which sounds like a *huge* lot 
of time...

>> +	} while (--timeout);
>> +
>> +	if (!timeout) {
>> +		/* The Goodix panel will send spurious interrupts after a
>> +		 * 'finger up' event, which will always cause a timeout.
>> +		 */
>> +		return 0;
>> +	}
>> 
>>  	touch_num = data[0] & 0x0f;
>>  	if (touch_num > ts->max_touch_num)
>> --
>> 2.11.0
>> 
> 
> Thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] input: goodix: Poll the 'buffer status' bit before reading data
  2017-03-30 13:33 ` Paul Cercueil
  (?)
  (?)
@ 2017-06-19 22:41 ` Bastien Nocera
  -1 siblings, 0 replies; 5+ messages in thread
From: Bastien Nocera @ 2017-06-19 22:41 UTC (permalink / raw)
  To: Paul Cercueil, Dmitry Torokhov; +Cc: linux-input, linux-kernel, Hans de Goede

Hey,

Sorry I took this long to look into this.

On Thu, 2017-03-30 at 15:33 +0200, Paul Cercueil wrote:
> The Goodix panel triggers an interrupt on touch events. However, its
> registers will contain the valid values a short time after the
> interrupt, and not when it's raised. At that moment, the 'buffer
> status'
> bit is set.
> 
> Previously, if the 'buffer status' bit was not set when the registers
> were read, the data was discarded and no input event was emitted,
> causing "finger down" or "finger up" events to be missed sometimes.
> 
> This went unnoticed until v4.9, as the DesignWare I2C driver commonly
> used with this driver had enough latency for that bug to never
> trigger.
> 
> Now, in the IRQ handler we will poll (with a timeout)

I don't like the fact that the timeout isn't actually a timeout, but a
loop counter...

>  the 'buffer status'
> bit and process the data of the panel as soon as this bit gets set.

OK.

> Note that the Goodix panel will send a few spurious interrupts after
> the
> 'finger up' event, in which the 'buffer status' bit will never be
> set.

Can you please re-send the patch with diff option that makes it easier
to comment on whole patch? There's various block levels being mixed
because they have a linefeed in common...

I'm also CC:ing Hans to test it out as my Goodix tablet is still out of
commission.

Cheers

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-06-19 22:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 13:33 [PATCH] input: goodix: Poll the 'buffer status' bit before reading data Paul Cercueil
2017-03-30 13:33 ` Paul Cercueil
2017-03-30 22:48 ` Dmitry Torokhov
2017-03-31 10:18   ` Paul Cercueil
2017-06-19 22:41 ` Bastien Nocera

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.