linux-kernel - Re: [PATCH 1/3] auxdisplay: Make charlcd.[ch] more general

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]

Message-ID: <20191017080741.GA17556@lem-wkst-02.lemonage>
Date:   Thu, 17 Oct 2019 10:07:41 +0200
From:   Lars Poeschel <poeschel@...onage.de>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Willy Tarreau <willy@...roxy.com>,
        Ksenija Stanojevic <ksenija.stanojevic@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH 1/3] auxdisplay: Make charlcd.[ch] more general

On Wed, Oct 16, 2019 at 06:53:20PM +0200, Miguel Ojeda wrote:
> On Wed, Oct 16, 2019 at 10:24 AM Lars Poeschel <poeschel@...onage.de> wrote:
> >
> > charlcd.c contains lots of hd44780 hardware specific stuff. It is nearly
> > impossible to reuse the interface for other character based displays.
> > The current users of charlcd are the hd44780 and the panel drivers.
> > This does factor out the hd44780 specific stuff out of charlcd into a
> > new module called hd44780_common.
> > charlcd gets rid of the hd44780 specfics and more generally useable.
> > The hd44780 and panel drivers are modified to use the new
> > hd44780_common.
> > This is tested on a hd44780 connected through the gpios of a pcf8574.
> >
> > Signed-off-by: Lars Poeschel <poeschel@...onage.de>
> > ---
> >  drivers/auxdisplay/Kconfig          |  16 +
> >  drivers/auxdisplay/Makefile         |   1 +
> >  drivers/auxdisplay/charlcd.c        | 591 ++++++++--------------------
> >  drivers/auxdisplay/charlcd.h        | 109 ++++-
> >  drivers/auxdisplay/hd44780.c        | 121 ++++--
> >  drivers/auxdisplay/hd44780_common.c | 370 +++++++++++++++++
> >  drivers/auxdisplay/hd44780_common.h |  32 ++
> >  drivers/auxdisplay/panel.c          | 178 ++++-----
> >  8 files changed, 851 insertions(+), 567 deletions(-)
> 
> Thanks Lars, CC'ing Geert since he wrote a large portion of this, as
> well as Andy.
> 
> From a cursory look (sorry, not doing it inline since it is a pain to
> edit in this UI given the size...):

I am okay with this. I know, what you are talking about, since I know
the code very well. But maybe it is a bit harder to follow for others.

>   * panel.c doesn't compile since lcd_backlight's return type did not
> get updated, which makes me uneasy. I am not sure why you changed the
> return type anyway, since callers ignore it and callees always return
> 0.

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

>   * Declared and then immediately defined hd44780_common in the header...?

This is not intended. I'll change it.

>   * 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 ?

>   * From checkpatch.pl: DOS line endings and trailing whitespace

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

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

Thank you for your prompt feedback!

Lars