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;
Subject: Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries
To:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        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>,
        Eugeniu Rosca <erosca@de.adit-jv.com>,
        Andrew_Gabbasov@mentor.com
References: <20200912005521.26319-1-jiada_wang@mentor.com>
 <CAHp75Vc5YCb-6oRRfVOE5bL_Dmzy0LwDpetxqD-G+E9M=EwA=w@mail.gmail.com>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <bd668b99-5b14-f54d-101d-7d56e0c8c4c0@gmail.com>
Date:   Sun, 13 Sep 2020 15:57:54 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAHp75Vc5YCb-6oRRfVOE5bL_Dmzy0LwDpetxqD-G+E9M=EwA=w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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, hence I
thought it's YOU who wants it to be retried.

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.

>> +               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;
>>  }