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>
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
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>
 <CAHp75VdTv-uCQue3VU=czZJd4iTG+XBVe2kFtnP+fZ1XQuFbzA@mail.gmail.com>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <137be969-f99a-66e0-ebe4-b86f4be2b5d3@gmail.com>
Date:   Mon, 14 Sep 2020 18:26:25 +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: <CAHp75VdTv-uCQue3VU=czZJd4iTG+XBVe2kFtnP+fZ1XQuFbzA@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

14.09.2020 16:49, Andy Shevchenko пишет:
> On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@gmail.com> 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.

I'm reading this as imperative "make it positive", and thus, assumed
that you asked to do so because the "retry" implies a positive
condition, while "retried" implies the negative.

If you've added "Could you please explain why", then I'd read it as a
question.

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

This should be up to a particular maintainer to decide. Usually nobody
requires patches to be overly pedantic, this may turn away contributors
because it feels like an unnecessary bikeshedding. It's more preferred
to accept patch as-is if it does right thing and then maintainer could
modify the patch, making cosmetic changes.

> And since it's a fix it should have a Fixes tag.

It shouldn't be a fix, but a new feature because apparently the 1386
controller wasn't ever intended to be properly supported before this patch.