[PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries

[PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries

[Jiada Wang]

From: Nick Dyer <nick.dyer@itdev.co.uk>

Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
when they are in a sleep state. It must be retried after a delay for the
chip to wake up.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Acked-by: Yufeng Shen <miletus@chromium.org>
(cherry picked from ndyer/linux/for-upstream commit 63fd7a2cd03c3a572a5db39c52f4856819e1835d)
[gdavis: Forward port and fix conflicts.]
Signed-off-by: George G. Davis <george_davis@mentor.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++++++++++++++++--------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a2189739e30f..e93eda1f3d59 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -196,6 +196,7 @@ enum t100_type {
 #define MXT_CRC_TIMEOUT		1000	/* msec */
 #define MXT_FW_RESET_TIME	3000	/* msec */
 #define MXT_FW_CHG_TIMEOUT	300	/* msec */
+#define MXT_WAKEUP_TIME		25	/* msec */
 
 /* Command to unlock bootloader */
 #define MXT_UNLOCK_CMD_MSB	0xaa
@@ -626,6 +627,7 @@ static int __mxt_read_reg(struct i2c_client *client,
 	struct i2c_msg xfer[2];
 	u8 buf[2];
 	int ret;
+	bool retry = false;
 
 	buf[0] = reg & 0xff;
 	buf[1] = (reg >> 8) & 0xff;
@@ -642,17 +644,22 @@ static int __mxt_read_reg(struct i2c_client *client,
 	xfer[1].len = len;
 	xfer[1].buf = val;
 
-	ret = i2c_transfer(client->adapter, xfer, 2);
-	if (ret == 2) {
-		ret = 0;
-	} else {
-		if (ret >= 0)
-			ret = -EIO;
-		dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
-			__func__, ret);
+retry_read:
+	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
+	if (ret != ARRAY_SIZE(xfer)) {
+		if (!retry) {
+			dev_dbg(&client->dev, "%s: i2c retry\n", __func__);
+			msleep(MXT_WAKEUP_TIME);
+			retry = true;
+			goto retry_read;
+		} else {
+			dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
+				__func__, ret);
+			return -EIO;
+		}
 	}
 
-	return ret;
+	return 0;
 }
 
 static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
@@ -661,6 +668,7 @@ static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
 	u8 *buf;
 	size_t count;
 	int ret;
+	bool retry = false;
 
 	count = len + 2;
 	buf = kmalloc(count, GFP_KERNEL);
@@ -671,14 +679,21 @@ static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
 	buf[1] = (reg >> 8) & 0xff;
 	memcpy(&buf[2], val, len);
 
+retry_write:
 	ret = i2c_master_send(client, buf, count);
-	if (ret == count) {
-		ret = 0;
-	} else {
-		if (ret >= 0)
+	if (ret != count) {
+		if (!retry) {
+			dev_dbg(&client->dev, "%s: i2c retry\n", __func__);
+			msleep(MXT_WAKEUP_TIME);
+			retry = true;
+			goto retry_write;
+		} else {
+			dev_err(&client->dev, "%s: i2c send failed (%d)\n",
+				__func__, ret);
 			ret = -EIO;
-		dev_err(&client->dev, "%s: i2c send failed (%d)\n",
-			__func__, ret);
+		}
+	} else {
+		ret = 0;
 	}
 
 	kfree(buf);
-- 
2.17.1

Re: [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

21.08.2020 11:22, Jiada Wang пишет:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> when they are in a sleep state. It must be retried after a delay for the
> chip to wake up.
> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Yufeng Shen <miletus@chromium.org>
> (cherry picked from ndyer/linux/for-upstream commit 63fd7a2cd03c3a572a5db39c52f4856819e1835d)
> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++++++++++++++++--------
>  1 file changed, 30 insertions(+), 15 deletions(-)

Hello, Jiada!

I tested this patch on Acer A500 that has mXT1386 controller which
requires the I2C retrying and everything working good, no problems
spotted! Touchscreen doesn't work without this patch!

Tested-by: Dmitry Osipenko <digetx@gmail.com>

I have one minor comment, please see it below!

> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a2189739e30f..e93eda1f3d59 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -196,6 +196,7 @@ enum t100_type {
>  #define MXT_CRC_TIMEOUT		1000	/* msec */
>  #define MXT_FW_RESET_TIME	3000	/* msec */
>  #define MXT_FW_CHG_TIMEOUT	300	/* msec */
> +#define MXT_WAKEUP_TIME		25	/* msec */
>  
>  /* Command to unlock bootloader */
>  #define MXT_UNLOCK_CMD_MSB	0xaa
> @@ -626,6 +627,7 @@ static int __mxt_read_reg(struct i2c_client *client,
>  	struct i2c_msg xfer[2];
>  	u8 buf[2];
>  	int ret;
> +	bool retry = false;
>  
>  	buf[0] = reg & 0xff;
>  	buf[1] = (reg >> 8) & 0xff;
> @@ -642,17 +644,22 @@ static int __mxt_read_reg(struct i2c_client *client,
>  	xfer[1].len = len;
>  	xfer[1].buf = val;
>  
> -	ret = i2c_transfer(client->adapter, xfer, 2);
> -	if (ret == 2) {
> -		ret = 0;
> -	} else {
> -		if (ret >= 0)
> -			ret = -EIO;
> -		dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
> -			__func__, ret);
> +retry_read:
> +	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> +	if (ret != ARRAY_SIZE(xfer)) {

Is it really possible to get a positive ret != 2 from i2c_transfer()?

Maybe it's better to keep the old code behaviour by returning the "ret"
value directly if it's not equal to ARRAY_SIZE(xfer)?

Re: [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries

[Wang, Jiada]

Hi Dmitry

On 2020/08/28 17:19, Dmitry Osipenko wrote:
> 21.08.2020 11:22, Jiada Wang пишет:
>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>
>> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
>> when they are in a sleep state. It must be retried after a delay for the
>> chip to wake up.
>>
>> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
>> Acked-by: Yufeng Shen <miletus@chromium.org>
>> (cherry picked from ndyer/linux/for-upstream commit 63fd7a2cd03c3a572a5db39c52f4856819e1835d)
>> [gdavis: Forward port and fix conflicts.]
>> Signed-off-by: George G. Davis <george_davis@mentor.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>>   drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++++++++++++++++--------
>>   1 file changed, 30 insertions(+), 15 deletions(-)
> 
> Hello, Jiada!
> 
> I tested this patch on Acer A500 that has mXT1386 controller which
> requires the I2C retrying and everything working good, no problems
> spotted! Touchscreen doesn't work without this patch!
> 
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> 
> I have one minor comment, please see it below!
> 
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index a2189739e30f..e93eda1f3d59 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -196,6 +196,7 @@ enum t100_type {
>>   #define MXT_CRC_TIMEOUT		1000	/* msec */
>>   #define MXT_FW_RESET_TIME	3000	/* msec */
>>   #define MXT_FW_CHG_TIMEOUT	300	/* msec */
>> +#define MXT_WAKEUP_TIME		25	/* msec */
>>   
>>   /* Command to unlock bootloader */
>>   #define MXT_UNLOCK_CMD_MSB	0xaa
>> @@ -626,6 +627,7 @@ static int __mxt_read_reg(struct i2c_client *client,
>>   	struct i2c_msg xfer[2];
>>   	u8 buf[2];
>>   	int ret;
>> +	bool retry = false;
>>   
>>   	buf[0] = reg & 0xff;
>>   	buf[1] = (reg >> 8) & 0xff;
>> @@ -642,17 +644,22 @@ static int __mxt_read_reg(struct i2c_client *client,
>>   	xfer[1].len = len;
>>   	xfer[1].buf = val;
>>   
>> -	ret = i2c_transfer(client->adapter, xfer, 2);
>> -	if (ret == 2) {
>> -		ret = 0;
>> -	} else {
>> -		if (ret >= 0)
>> -			ret = -EIO;
>> -		dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
>> -			__func__, ret);
>> +retry_read:
>> +	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
>> +	if (ret != ARRAY_SIZE(xfer)) {
> 
> Is it really possible to get a positive ret != 2 from i2c_transfer()?
> 
> Maybe it's better to keep the old code behaviour by returning the "ret"
> value directly if it's not equal to ARRAY_SIZE(xfer)?
> 
I think, theoretically i2c_transfer() may return positive value but != 
number to transfer,
original behavior is,
when ret >= 0, it returns -EIO, when ret < 0, it just returns ret,

current behavior is, when ret != 2, it returns -EIO, after retry.

I am OK to change the behavior exactly as same original one.

Thanks,
Jiada

Re: [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

03.09.2020 17:44, Wang, Jiada пишет:
> Hi Dmitry
> 
...
>> Is it really possible to get a positive ret != 2 from i2c_transfer()?
>>
>> Maybe it's better to keep the old code behaviour by returning the "ret"
>> value directly if it's not equal to ARRAY_SIZE(xfer)?
>>
> I think, theoretically i2c_transfer() may return positive value but !=
> number to transfer,
> original behavior is,
> when ret >= 0, it returns -EIO, when ret < 0, it just returns ret,
> 
> current behavior is, when ret != 2, it returns -EIO, after retry.
> 
> I am OK to change the behavior exactly as same original one.

The comment to i2c_transfer() says that it either returns a error code
or number of executed messages. But it's not clear to me what I2C driver
could return 0 or 1 here and why.

I think it indeed should be better to keep the original behaviour by
propagating the actual error code whenever possible. It's probably not
critical for this particular case, but in general it's always better to
preserve the original error code.