* [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries
@ 2020-08-21 8:22 Jiada Wang
2020-08-28 8:19 ` Dmitry Osipenko
0 siblings, 1 reply; 4+ messages in thread
From: Jiada Wang @ 2020-08-21 8:22 UTC (permalink / raw)
To: nick, dmitry.torokhov
Cc: linux-input, linux-kernel, erosca, Andrew_Gabbasov, digetx, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries
2020-08-21 8:22 [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries Jiada Wang
@ 2020-08-28 8:19 ` Dmitry Osipenko
2020-09-03 14:44 ` Wang, Jiada
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Osipenko @ 2020-08-28 8:19 UTC (permalink / raw)
To: Jiada Wang, nick, dmitry.torokhov
Cc: linux-input, linux-kernel, erosca, Andrew_Gabbasov
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)?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries
2020-08-28 8:19 ` Dmitry Osipenko
@ 2020-09-03 14:44 ` Wang, Jiada
2020-09-03 15:06 ` Dmitry Osipenko
0 siblings, 1 reply; 4+ messages in thread
From: Wang, Jiada @ 2020-09-03 14:44 UTC (permalink / raw)
To: Dmitry Osipenko, nick, dmitry.torokhov
Cc: linux-input, linux-kernel, erosca, Andrew_Gabbasov
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries
2020-09-03 14:44 ` Wang, Jiada
@ 2020-09-03 15:06 ` Dmitry Osipenko
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Osipenko @ 2020-09-03 15:06 UTC (permalink / raw)
To: Wang, Jiada, nick, dmitry.torokhov
Cc: linux-input, linux-kernel, erosca, Andrew_Gabbasov
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-03 15:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 8:22 [PATCH 1/1] Input: atmel_mxt_ts - implement I2C retries Jiada Wang
2020-08-28 8:19 ` Dmitry Osipenko
2020-09-03 14:44 ` Wang, Jiada
2020-09-03 15:06 ` Dmitry Osipenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).