input: touchscreen: stmpe: check fifo_empty flag before reading x/y values
diff mbox

Message ID 1491817946-13926-1-git-send-email-pfink@christ-es.de
State Under Review
Headers show

Commit Message

Peter Fink April 10, 2017, 9:52 a.m. UTC
STMPE driver with STMPE811 reported x=0/y=0 when clicking in succession at
higher speeds. In some configurations the consuming X11 input stack is able
to ignore this input, but e.g. with inverted axes this is prone to fail.

In these cases it might happen that even applications are closed when the
close-X is located in the same corner as touch 0/0 is mapped to. This
happens when touch coordinates are read from an empty fifo although the
threshold interrupt indicates that new data is available.

To avoid wrong touch data read and evaluate the fifo_empty bit before
accessing the fifo.

Signed-off-by: Peter Fink <pfink@christ-es.de>
---
 drivers/input/touchscreen/stmpe-ts.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comments

Dmitry Torokhov April 11, 2017, 3:33 a.m. UTC | #1
Hi Peter,

On Mon, Apr 10, 2017 at 11:52:26AM +0200, Peter Fink wrote:
> STMPE driver with STMPE811 reported x=0/y=0 when clicking in succession at
> higher speeds. In some configurations the consuming X11 input stack is able
> to ignore this input, but e.g. with inverted axes this is prone to fail.
> 
> In these cases it might happen that even applications are closed when the
> close-X is located in the same corner as touch 0/0 is mapped to. This
> happens when touch coordinates are read from an empty fifo although the
> threshold interrupt indicates that new data is available.
> 
> To avoid wrong touch data read and evaluate the fifo_empty bit before
> accessing the fifo.
> 
> Signed-off-by: Peter Fink <pfink@christ-es.de>
> ---
>  drivers/input/touchscreen/stmpe-ts.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
> index 2a78e27..dcc9d48 100644
> --- a/drivers/input/touchscreen/stmpe-ts.c
> +++ b/drivers/input/touchscreen/stmpe-ts.c
> @@ -44,6 +44,7 @@
>  #define OP_MOD_XYZ			0
>  
>  #define STMPE_TSC_CTRL_TSC_EN		(1<<0)
> +#define STMPE_FIFO_EMPTY		(1<<5)

This belongs with other FIFO STA register definitions, please move
there.  Also, all these could be converted to BIT().

>  
>  #define STMPE_FIFO_STA_RESET		(1<<0)
>  
> @@ -175,17 +176,22 @@ static irqreturn_t stmpe_ts_handler(int irq, void *data)
>  	stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
>  				STMPE_TSC_CTRL_TSC_EN, 0);
>  
> -	stmpe_block_read(ts->stmpe, STMPE_REG_TSC_DATA_XYZ, 4, data_set);
> +	if (!(stmpe_reg_read(ts->stmpe, STMPE_REG_FIFO_STA)
> +					& STMPE_FIFO_EMPTY)) {

We probably want to check for errors. So:

	fifo_sta = stmpe_reg_read(ts->stmpe, STMPE_REG_FIFO_STA);
	if (fifo_sta >= 0 && !(fifo_sta & STMPE_FIFO_EMPTY)) {
		...
	}

I am also looking at stmpe_work() and wonder if we should not check FIFO
status when we see touch detect and if we see it there we probably want
to bail?

Thanks.
Peter Fink April 11, 2017, 10:07 a.m. UTC | #2
Hi Dmitry,
thank you for your response.

On Mon, Apr 10, 2017 at 20:33:47 -0700, Dmitry Torokhov wrote:
> Hi Peter,
>
> On Mon, Apr 10, 2017 at 11:52:26AM +0200, Peter Fink wrote:
>> STMPE driver with STMPE811 reported x=0/y=0 when clicking in
>> succession at higher speeds. In some configurations the consuming X11
>> input stack is able to ignore this input, but e.g. with inverted axes this is prone to fail.
>>
>> In these cases it might happen that even applications are closed when
>> the close-X is located in the same corner as touch 0/0 is mapped to.
>> This happens when touch coordinates are read from an empty fifo
>> although the threshold interrupt indicates that new data is available.
>>
>> To avoid wrong touch data read and evaluate the fifo_empty bit before
>> accessing the fifo.
>>
>> Signed-off-by: Peter Fink <pfink@christ-es.de>
>> ---
>>   drivers/input/touchscreen/stmpe-ts.c | 24 +++++++++++++++---------
>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/stmpe-ts.c
>> b/drivers/input/touchscreen/stmpe-ts.c
>> index 2a78e27..dcc9d48 100644
>> --- a/drivers/input/touchscreen/stmpe-ts.c
>> +++ b/drivers/input/touchscreen/stmpe-ts.c
>> @@ -44,6 +44,7 @@
>>   #define OP_MOD_XYZ                   0
>>
>>   #define STMPE_TSC_CTRL_TSC_EN                (1<<0)
>> +#define STMPE_FIFO_EMPTY             (1<<5)
> This belongs with other FIFO STA register definitions, please move there.  Also, all these could be converted to BIT().

Sorry, this define was intended to go unter FIFO_STA_RESET. I copied the 
define to the wrong line. Should I include conversion to BIT in the same 
patch or prepare a second patch?

>>   #define STMPE_FIFO_STA_RESET         (1<<0)
>>
>> @@ -175,17 +176,22 @@ static irqreturn_t stmpe_ts_handler(int irq, void *data)
>>        stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
>>                                STMPE_TSC_CTRL_TSC_EN, 0);
>>
>> -     stmpe_block_read(ts->stmpe, STMPE_REG_TSC_DATA_XYZ, 4, data_set);
>> +     if (!(stmpe_reg_read(ts->stmpe, STMPE_REG_FIFO_STA)
>> +                                     & STMPE_FIFO_EMPTY)) {
> We probably want to check for errors. So:
>
>          fifo_sta = stmpe_reg_read(ts->stmpe, STMPE_REG_FIFO_STA);
>          if (fifo_sta >= 0 && !(fifo_sta & STMPE_FIFO_EMPTY)) {
>                  ...
>          }

fifo_sta could only indicate fifo_overflow, fifo_full and
fifo_treshold_triggered and does not contain any other error flags.
Would that be useful in this case?
We flush the fifo anyway after the event is (not) reported:

        /* flush the FIFO after we have read out our values. */
         __stmpe_reset_fifo(ts->stmpe);
>
> I am also looking at stmpe_work() and wonder if we should not check FIFO status when we see touch detect and if we see it there we probably want to bail?

Good point. I think the best way would be not to schedule stmpe_work() 
at all in the case of the fifo not containing any samples.

         /* start polling for touch_det to detect release */
         schedule_delayed_work(&ts->work, msecs_to_jiffies(50));

What do you think?
>
> Thanks.
>
> --
> Dmitry

--
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
index 2a78e27..dcc9d48 100644
--- a/drivers/input/touchscreen/stmpe-ts.c
+++ b/drivers/input/touchscreen/stmpe-ts.c
@@ -44,6 +44,7 @@ 
 #define OP_MOD_XYZ			0
 
 #define STMPE_TSC_CTRL_TSC_EN		(1<<0)
+#define STMPE_FIFO_EMPTY		(1<<5)
 
 #define STMPE_FIFO_STA_RESET		(1<<0)
 
@@ -175,17 +176,22 @@  static irqreturn_t stmpe_ts_handler(int irq, void *data)
 	stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
 				STMPE_TSC_CTRL_TSC_EN, 0);
 
-	stmpe_block_read(ts->stmpe, STMPE_REG_TSC_DATA_XYZ, 4, data_set);
+	if (!(stmpe_reg_read(ts->stmpe, STMPE_REG_FIFO_STA)
+					& STMPE_FIFO_EMPTY)) {
 
-	x = (data_set[0] << 4) | (data_set[1] >> 4);
-	y = ((data_set[1] & 0xf) << 8) | data_set[2];
-	z = data_set[3];
+		stmpe_block_read(ts->stmpe, STMPE_REG_TSC_DATA_XYZ,
+							4, data_set);
 
-	input_report_abs(ts->idev, ABS_X, x);
-	input_report_abs(ts->idev, ABS_Y, y);
-	input_report_abs(ts->idev, ABS_PRESSURE, z);
-	input_report_key(ts->idev, BTN_TOUCH, 1);
-	input_sync(ts->idev);
+		x = (data_set[0] << 4) | (data_set[1] >> 4);
+		y = ((data_set[1] & 0xf) << 8) | data_set[2];
+		z = data_set[3];
+
+		input_report_abs(ts->idev, ABS_X, x);
+		input_report_abs(ts->idev, ABS_Y, y);
+		input_report_abs(ts->idev, ABS_PRESSURE, z);
+		input_report_key(ts->idev, BTN_TOUCH, 1);
+		input_sync(ts->idev);
+	}
 
        /* flush the FIFO after we have read out our values. */
 	__stmpe_reset_fifo(ts->stmpe);