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

[Wang, Jiada]

Hi Andy

Thanks for your comment

On 2020/09/06 3:02, Andy Shevchenko wrote:
> On Thursday, September 3, 2020, Jiada Wang <jiada_wang@xxxxxxxxxx 
> <mailto:jiada_wang@xxxxxxxxxx>> wrote:
>
>     From: Nick Dyer <nick.dyer@xxxxxxxxxxx <mailto:nick.dyer@xxxxxxxxxxx>>
>
>     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@xxxxxxxxxxx
>     <mailto:nick.dyer@xxxxxxxxxxx>>
>     Acked-by: Yufeng Shen <miletus@xxxxxxxxxxxx
>     <mailto:miletus@xxxxxxxxxxxx>>
>
>
>     (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@xxxxxxxxxx
>     <mailto:george_davis@xxxxxxxxxx>>
>     [jiada: return exact errno when i2c_transfer & i2c_master_send fails]
>     Signed-off-by: Jiada Wang <jiada_wang@xxxxxxxxxx
>     <mailto:jiada_wang@xxxxxxxxxx>>
>     ---
>       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