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

[Andy Shevchenko]

On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> 13.09.2020 11:43, Andy Shevchenko пишет:
> > ...
> >
> >> +       bool retried = 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 = 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)) {
> ...> 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 = 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;
> >>  }



-- 
With Best Regards,
Andy Shevchenko