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
