On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko [off-list ref] wrote:

13.09.2020 11:43, Andy Shevchenko пишет:

quoted

...

quoted

+       bool retried = false;

quoted

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.

quoted

quoted

        u8 buf[2];
        int ret;

quoted

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

quoted

+       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.

quoted

quoted

+               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