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> <CAHp75Vc5YCb-6oRRfVOE5bL_Dmzy0LwDpetxqD-G+E9M=EwA=w@mail.gmail.com>
 <bd668b99-5b14-f54d-101d-7d56e0c8c4c0@gmail.com>
In-Reply-To: <bd668b99-5b14-f54d-101d-7d56e0c8c4c0@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 14 Sep 2020 16:49:08 +0300
Message-ID: <CAHp75VdTv-uCQue3VU=czZJd4iTG+XBVe2kFtnP+fZ1XQuFbzA@mail.gmail.com>
Subject: Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries
To:     Dmitry Osipenko <digetx@gmail.com>
Cc:     Jiada Wang <jiada_wang@mentor.com>, 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>,
        Eugeniu Rosca <erosca@de.adit-jv.com>,
        Andrew_Gabbasov@mentor.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 13.09.2020 11:43, Andy Shevchenko =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
> > ...
> >
> >> +       bool retried =3D false;
>
> > I thought Dmitry wants that to be retry
>
> In the comment to v2 you suggested to negate the condition,

Where? I just checked a few messages before and I found that I asked
the same question: why is negative conditional used instead of
positive.

> hence I
> thought it's YOU who wants it to be retried.

I see. Let's see how it goes with positive conditionals first.


> The "retried" is a very common form among kernel drivers, so it's good
> to me.
>
> >>         u8 buf[2];
> >>         int ret;
> >
> >> -       ret =3D i2c_transfer(client->adapter, xfer, 2);
> >> -       if (ret =3D=3D 2) {
> >> -               ret =3D 0;
> >> -       } else {
> >> -               if (ret >=3D 0)
> >> -                       ret =3D -EIO;
> >> +retry_read:
> >
> >> +       ret =3D i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> >> +       if (ret !=3D ARRAY_SIZE(xfer)) {
> ...> Also why switch from positive to negative conditional?
>
> This will make code less readable because of the goto, and thus, there
> will be two branches for handling of the returned value instead of one +
> goto. Hence this part is good to me as-is.

But it's not the purpose of this patch, right?
Style changes should be really separated from the fix.
And since it's a fix it should have a Fixes tag.

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


--=20
With Best Regards,
Andy Shevchenko