Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200912005521.26319-1-jiada_wang@mentor.com>
In-Reply-To: <20200912005521.26319-1-jiada_wang@mentor.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sun, 13 Sep 2020 11:43:41 +0300
Message-ID: <CAHp75Vc5YCb-6oRRfVOE5bL_Dmzy0LwDpetxqD-G+E9M=EwA=w@mail.gmail.com>
Subject: Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries
To:     Jiada Wang <jiada_wang@mentor.com>
Cc:     nick@shmanahar.org, Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        linux-input <linux-input@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Dmitry Osipenko <digetx@gmail.com>,
        Eugeniu Rosca <erosca@de.adit-jv.com>,
        Andrew_Gabbasov@mentor.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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