lkml.org 
[lkml]   [2019]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@lemonage.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@lemonage.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

    \
     
     \ /
      Last update: 2019-10-17 10:08    [W:2.141 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site