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

Andy Shevchenko
2020-09-13T08:44:05Z

On Sat, Sep 12, 2020 at 3:55 AM 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.
>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> [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
> rename "retry" to "retried" and keep its order in length
> set "ret" to correct errno before calling dev_err()

> remove reduntant conditional]

redundant

> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>

...

> + bool retried = false;

I thought Dmitry wants that to be retry

> u8 buf[2];
> int ret;

> - ret = i2c_transfer(client->adapter, xfer, 2);
> - if (ret == 2) {
> - ret = 0;
> - } else {
> - if (ret >= 0)
> - ret = -EIO;
> +retry_read:

> + ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> + if (ret != ARRAY_SIZE(xfer)) {

I'm wondering why you can't leave 2 as is and change it to ARRAY_SIZE
in a separate patch?
Also why switch from positive to negative conditional?

> + if (!retried) {
> + dev_dbg(&client->dev, "i2c retry\n");
> + msleep(MXT_WAKEUP_TIME);
> + retried = true;
> + goto retry_read;
> + }
> + ret = ret < 0 ? ret : -EIO;
> dev_err(&client->dev, "%s: i2c transfer failed (%d)\n",
> __func__, ret);
> + return ret;
> }
>
> - return ret;
> + return 0;
> }

..

> + bool retried = false;

Same comments here, in this function.

> +retry_write:
> ret = i2c_master_send(client, buf, count);
> - if (ret == count) {
> - ret = 0;
> - } else {
> - if (ret >= 0)
> - ret = -EIO;
> + if (ret != count) {
> + if (!retried) {
> + dev_dbg(&client->dev, "i2c retry\n");
> + msleep(MXT_WAKEUP_TIME);
> + retried = true;
> + goto retry_write;
> + }
> + ret = ret < 0 ? ret : -EIO;
> dev_err(&client->dev, "%s: i2c send failed (%d)\n",
> __func__, ret);
> + } else {
> + ret = 0;
> }

--
With Best Regards,
Andy Shevchenko