linux-kernel - Re: [PATCH v4 1/1] Input: atmel_mxt_ts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Sep 2020 18:50:38 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jiada Wang <jiada_wang@...tor.com>, nick@...anahar.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input <linux-input@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Eugeniu Rosca <erosca@...adit-jv.com>,
        Andrew_Gabbasov@...tor.com
Subject: Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

On Mon, Sep 14, 2020 at 6:26 PM Dmitry Osipenko <digetx@...il.com> wrote:
> 14.09.2020 16:49, Andy Shevchenko пишет:
> > On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@...il.com> wrote:

...

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

Let's see what Wolfram thinks about this.

> It's more preferred
> to accept patch as-is if it does right thing and then maintainer could
> modify the patch, making cosmetic changes.

It depends on the maintainer's workflow (which may be different from
maintainer to maintainer).

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

Thanks for clarification. Indeed in this case no tag is needed.

-- 
With Best Regards,
Andy Shevchenko