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

[PATCH v3 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>
[jiada: return exact errno when i2c_transfer & i2c_master_send fails,
	add "_MS" suffix MXT_WAKEUP_TIME]
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..145780f78122 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_MS	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_MS);
+			retry = true;
+			goto retry_read;
+		} else {
+			dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
+				__func__, ret);
+			return ret < 0 ? ret : -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;
+	if (ret != count) {
+		if (!retry) {
+			dev_dbg(&client->dev, "%s: i2c retry\n", __func__);
+			msleep(MXT_WAKEUP_TIME_MS);
+			retry = true;
+			goto retry_write;
+		} else {
+			dev_err(&client->dev, "%s: i2c send failed (%d)\n",
+				__func__, ret);
+			ret = ret < 0 ? ret : -EIO;
+		}
 	} else {
-		if (ret >= 0)
-			ret = -EIO;
-		dev_err(&client->dev, "%s: i2c send failed (%d)\n",
-			__func__, ret);
+		ret = 0;
 	}
 
 	kfree(buf);
-- 
2.17.1

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

[Dmitry Osipenko]

08.09.2020 18:16, 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)

Hello, Jiada!

In the comment to v2 Andy Shevchenko suggested that it should be better
to remove the "cherry picked from ..." info from the commit message
because that commit can't be found in the kernel.org git repo, and thus,
it's not a very useful information.

Also, please note that the original ACK from Yufeng Shen probably isn't
valid anymore since you modified the patch. Hence it should be better to
remove it.


You may also add my r-b and t-b tags to the commit message that I gave
to the v2.

> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> [jiada: return exact errno when i2c_transfer & i2c_master_send fails,
> 	add "_MS" suffix MXT_WAKEUP_TIME]
> 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..145780f78122 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_MS	25	/* msec */

I think Andy Shevchenko meant here that the "/* msec */" comment won't
be needed and should be removed if _MS postfix is added to the name.

But I think you should keep this hunk as it was in v2 and then there
could be a new separate patch which renames all those timeouts at once.
This will keep code consistent in regards to the chosen naming convention.

>  /* 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_MS);
> +			retry = true;
> +			goto retry_read;
> +		} else {
> +			dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
> +				__func__, ret);
> +			return ret < 0 ? ret : -EIO;
> +		}

Andy also suggested that it could be a cleaner to write such code like this:

if (ret != ARRAY_SIZE(xfer)) {
	if (!retry) {
		...
		goto retry_read;
	}

	dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
		__func__, ret);

	return ret < 0 ? ret : -EIO;
}

>  	}
>  
> -	return ret;
> +	return 0;
>  }
...

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

[Dmitry Osipenko]

11.09.2020 17:50, Dmitry Osipenko пишет:
...
>> @@ -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;

Andy suggested to write this hunk like this:

	struct i2c_msg xfer[2];
	bool retry = false;
	u8 buf[2];
	int ret;

This is not a mandatory request at all, but it will make this particular
piece of code to look a bit nicer.

There is also an opportunity to improve formatting of all variables by
sorting them by-length across the whole driver, this will improve
readability of the code. But of course it should be a separate patch.
Please note that I'm *not* saying that you should create this separate
patch!

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

[Dmitry Osipenko]

11.09.2020 18:05, Dmitry Osipenko пишет:
> 11.09.2020 17:50, Dmitry Osipenko пишет:
> ...
>>> @@ -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;
> 
> Andy suggested to write this hunk like this:
> 
> 	struct i2c_msg xfer[2];
> 	bool retry = false;
> 	u8 buf[2];
> 	int ret;
> 
> This is not a mandatory request at all, but it will make this particular
> piece of code to look a bit nicer.
> 
> There is also an opportunity to improve formatting of all variables by
> sorting them by-length across the whole driver, this will improve
> readability of the code. But of course it should be a separate patch.
> Please note that I'm *not* saying that you should create this separate
> patch!
> 

I'd also recommend to rename the "retry" variable to "retried", which
will be a more logical name (more proper English).

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

[Andy Shevchenko]

On Tue, Sep 8, 2020 at 11:11 PM Jiada Wang <jiada_wang@mentor.com> wrote:
>
> 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.
>

Same comments as per v2.


-- 
With Best Regards,
Andy Shevchenko