Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191016082430.5955-1-poeschel@lemonage.de> <CANiq72=uXWpEWHixM+wwyxZfzQ41WYvQsoV8B3+JLRharDjC0w@mail.gmail.com>
 <20191017080741.GA17556@lem-wkst-02.lemonage>
In-Reply-To: <20191017080741.GA17556@lem-wkst-02.lemonage>
From:   Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date:   Fri, 18 Oct 2019 17:08:50 +0200
Message-ID: <CANiq72m0V5CBxp97Q4h70Gup1DCoZj4ZFa6VWQLk0jyurxeztQ@mail.gmail.com>
Subject: Re: [PATCH 1/3] auxdisplay: Make charlcd.[ch] more general
To:     Lars Poeschel <poeschel@lemonage.de>
Cc:     Willy Tarreau <willy@haproxy.com>,
        Ksenija Stanojevic <ksenija.stanojevic@gmail.com>,
        open list <linux-kernel@vger.kernel.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Joe Perches <joe@perches.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Oct 17, 2019 at 10:07 AM Lars Poeschel <poeschel@lemonage.de> wrote:
>
> That panel.c doesn't compile is of course a no-go. Sorry. I missed
> something and I will fix this in a next version of the patch. But before
> submitting a next version, I will wait some time, if there is more
> feedback.

I meant I wasn't sure if the overall patch as that well tested ;-)

> The idea with changing the return types: It seems a bit, that with this
> patch charlcd is becoming more of an universal interface and maybe more
> display backends get added - maybe with displays, that can report
> failure of operations. And I thought, it will be better to have this
> earlier and have the "interface" stable and more uniform. But you are
> the maintainer. If you don't like the changed return types I happily
> revert back to the original ones in the next version of the patch.

I see the value of the idea, but given kernel APIs are not stable
anyway, I prefer not to have things that we do not use yet. If someone
requires returning an error code in a future driver, they can change
it later on. I don't have a strong opinion on this anyway.

> >   * Some things (e.g. the addition of enums like charlcd_onoff) seem
> > like could have been done other patches (since they are not really
> > related to the reorganization).
>
> I can split this out into separate patches. It'd be good know what else
> you mean by "some things" so I can do this as well. Do you want each
> enum as a separate patch or one big enum patch ?

As Andy said, ideally patches/commits implement logical changes, i.e.
broken down as much as reasonably possible. For instance, changing the
interface from int/bool to an enum is probably a good idea, but it is
not related to the overall reorganization. In fact, it is orthogonal,
since it could be done either before or after the reorganization.

In general, it is much more easy to understand and review changes if
you split them. For instance, I noticed the enum change by chance, I
may have not noticed it; but if it was in a different patch, it would
have been clear for everyone. It also allows you to explain each
change individually in the commit message. Another of those "things"
is what Andy suggested. Of course, after you split things, it will
become easier to spot other things too, so we might have to do another
round etc.

A single patch for all the enums is fine. We could split those too,
but given the change has the same rationale, and is small enough, we
can put them together and share the commit message + is easier for you
and us to handle too. That is the "as much as reasonably possible"
part of the guideline ;-)

> Strange. I did indeed checkpatch.pl the patches before submitting and I
> got no complaints about whitespace or line endings. There was "WARNING:
> added, moved or deleted file(s), does MAINTAINERS need updating?" and
> patch 1 also has "WARNING: please write a paragraph that describes the
> config symbol fully". I submitted the patches with git send-email so it
> is very unlikely, that the mailer messed up the patches. Strange...

Hm... Try sending the series to yourself and see if you can reproduce it.

> Oh by the way: Do you know what I can do to make checkpatch happy with
> its describing of the config symbol ? I tried writing a help paragraph
> for the config symbols in Kconfig, but that did not help.

CC'ing Joe.

> > I am not capable of testing this, so extra testing by anyone who has
> > the different hardware affected around is very welcome.
>
> Are you able to test the panel driver ?

Even worse: at the moment I cannot test any of the drivers (and more
have been added over time). The 3 of them I could test are very
old/obsolete and I will be dropping them, so my role here is basically
"abstract" :-)

If there is anyone that can actually test most of the drivers in
auxdisplay and wants to help maintaining, it would be great, in fact.

> Thank you for your prompt feedback!

You're welcome!

Cheers,
Miguel