| Message ID | 20200903155904.17454-1-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] Input: atmel_mxt_ts - implement I2C retries | expand |
03.09.2020 18:59, 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] > 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! Everything works well on Acer A500 tablet device that uses mXT1386 for the touchscreen controller! Thank you very much! Reviewed-by: Dmitry Osipenko <digetx@gmail.com> Tested-by: Dmitry Osipenko <digetx@gmail.com>
05.09.2020 21:02, Andy Shevchenko пишет: ... > #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 */ > > > Can we simple add _MS unit suffix to the definition? I'd expect this > /* 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; > > > Keep this ordered by length. and this to be separate patches that are cleaning whole driver code, otherwise there are no much benefits.
Hi Andy Thanks for your comment On 2020/09/06 3:02, Andy Shevchenko wrote: > > > On Thursday, September 3, 2020, Jiada Wang <jiada_wang@mentor.com > <mailto:jiada_wang@mentor.com>> wrote: > > From: Nick Dyer <nick.dyer@itdev.co.uk <mailto: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 > <mailto:nick.dyer@itdev.co.uk>> > Acked-by: Yufeng Shen <miletus@chromium.org > <mailto:miletus@chromium.org>> > > > (cherry picked from ndyer/linux/for-upstream commit > 63fd7a2cd03c3a572a5db39c52f4856819e1835d) > > > It’s a noise for upstream. > sure, I will remove it > [gdavis: Forward port and fix conflicts.] > Signed-off-by: George G. Davis <george_davis@mentor.com > <mailto:george_davis@mentor.com>> > [jiada: return exact errno when i2c_transfer & i2c_master_send fails] > Signed-off-by: Jiada Wang <jiada_wang@mentor.com > <mailto: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..5d4cb15d21dc 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 */ > > > Can we simple add _MS unit suffix to the definition? As Dmitry commented, I'd like to keep it as-is, probably a separate patch to update all these together. > > > /* 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; > > > Keep this ordered by length. I will move "retry" upper. > > > 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) { > > > Why not positive conditional? to me, it's not much different either positive or negative conditional, can you elaborate more about this? > > + dev_dbg(&client->dev, "%s: i2c retry\n", > __func__); > > > __func__ is redundant for dev_dbg(). > > + msleep(MXT_WAKEUP_TIME); > + retry = true; > + goto retry_read; > > + } else { > > > Redundant in either case of conditional. Allows to drop indentation level. I will remove the redundant conditional Thanks, Jiada > > + dev_err(&client->dev, "%s: i2c transfer > failed (%d)\n", > + __func__, ret); > + return ret < 0 ? ret : -EIO; > + } > } > > - return ret; > + return 0; > } > > > Same comments about below. > > 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); > + 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 > > > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index a2189739e30f..5d4cb15d21dc 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 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); + 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);