Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

Andy Shevchenko
2020-09-14T15:55:02Z

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