On Fri, 2017-10-13 at 13:19 +0200, Hans de Goede wrote:
> From: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
>
> 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.
Link to the bug fix?
> 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.
>
> Cc: Bastien Nocera <hadess@xxxxxxxxxx>
> Cc: russianneuromancer@xxxxx
> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> [hdegoede@xxxxxxxxxx: Change poll loop to use jiffies,
> add comment about typical poll time]
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/input/touchscreen/goodix.c | 36
> ++++++++++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 32d2762448aa..a2071c132a2b 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -195,18 +195,38 @@ static int goodix_get_cfg_len(u16 id)
>
> static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8
> *data)
> {
> + unsigned long timeout;
> int touch_num;
> int error;
>
> - 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;
> - }
> + /*
> + * The 'buffer status' bit, which indicates that the data is
> valid, is
> + * not set as soon as the interrupt is raised, but slightly
> after.
> + * This takes around 10 ms to happen, so we poll for 20 ms.
> + */
> + timeout = jiffies + msecs_to_jiffies(20);
around 16msec is the time between 2 frames on a 60Hz display. Is 20msec
what we want? Does this looping block below any other parts of the
kernel, or does it run in its own thread?
> + 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 (!(data[0] & 0x80))
> - return -EAGAIN;
> + if (data[0] & 0x80)
> + break;
> +
> + usleep_range(1000, 2000); /* Poll every 1 - 2 ms */
> + } while (time_before(jiffies, timeout));
> +
> + if (!(data[0] & 0x80)) {
> + /*
> + * 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)
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
