Re: [PATCH v2 28/32] auxdisplay: hd44780: Remove clear_fast

[Lars Poeschel]

On Tue, Sep 22, 2020 at 11:21:21AM +0200, Willy Tarreau wrote:
> On Tue, Sep 22, 2020 at 10:49:12AM +0200, Lars Poeschel wrote:
> > > I might have got confused, but it looks to me like patches 27 and 28
> > > basically undo patch 26: in 26 you moved code to hd44780 and wrote a
> > > fallback, just to later delete that code.
> > 
> > To be honest: I also got confused by this whole clear code and why are
> > there 3 different clear variants. ;-)
> > The reason why I did it this way is to show what happened to the
> > original code. The original code has a fallback that kind of does the
> > same like my naive implementation but it uses the fact, that on hd44780
> > displays the underlying buffer can be accessed in a linear manner to
> > speed things up a little. This then also clears the not visible area of
> > the buffer (which my naive implementation does not). To become
> > independent of hd44780 code at this point I had to do something.
> > I opted to for this naive implementation as a replacement for the old
> > optimized clear loop. The fallback was already there.
> > And then I did a separate step to remove it because I found it a bit
> > suboptimal. All current users have the clear command...
> 
> I'm not contesting your naive implementation, it just looks to me that
> patch 26 adds it while moving code that patch 27 removes, and patch 28
> removes it. So I don't understand why not to remove it entirely in the
> first place. It's possible I missed something related to other users of
> that code but that wasn't clear from the patch nor the descriptions.

The reason is to tell the story, where the original code went. I wanted
kind of avoid a discussion why I just deleted this code. So now it's the
other way round. :-)
I do not have any deeper mental connection to this. I will squash this
together in the next version of this patch series unless other opinions
arise.

> > > I seem to remember that the reason for the clear_fast() implementation
> > > is that the default clear_display() is quite slow for small displays,
> > > compared to what can be achieved by just filling the display with spaces
> > > (in the order of tens of milliseconds vs hundreds of microseconds). But
> > > I could be mistaken given how old this is, so please take my comment
> > > with a grain of salt.
> > 
> > ... well, and what the hd44780 controller does when it executes the
> > clear command is also writing a space character to all character
> > locations (also to the not visible ones). Probably very much the same
> > than the original fallback implementation.
> 
> I've just checked on some old datasheets, and indeed the Display clear
> command takes up to 1.6 ms, which looks very reasonable. But the charlcd
> code currently sleeps 15 ms, which is 10 times more than needed. I've
> just found its roots inside the panel-0.8.1 version that changed the delay
> from 1ms to 15ms on 2001/11/10, and added the lcd_clear_fast() function
> as a workaround. Thus probably 1ms was too short for the 1.6 ms spec,
> but 15ms was needlessly high. So I think we can get rid of all of this
> indeed!

Ok, understood. I will also address this in the next series and make a
separate patch reducing the delay.

> > For me issuing the clear command is faster by at least one order!
> > I have one of these cheap china displays with 4x20 characters. The flow
> > is like this: i2c -> gpio -> hd44780 in 4 bit mode. And gpio is issuing
> > a i2c request for EVERY SINGLE gpio. This is slow as hell. But it works.
> > :-)
> 
> Good point for sur over i2c it would stink a little bit! Same for those
> using 9600 bps serial interfaces.
> 
> > As I said I also was a bit confused by the different clean
> > implementations, but the only real user of the clear_fast is the panel
> > driver. The hd44780 one I use did not provide a clear_fast.
> > 
> > So I would opt for the way I proposed in the patch series with the clear
> > command instead of the loop. And let the panel driver use its optimized
> > clear.
> 
> Or we could align the panel driver to get rid of it as well.

Ok, I will remove the clear_fast from panel as well and let it just use
the hd44780_common_clear_display then. This looks much cleaner. We have
just a single clear_display variant then!

Regards,
Lars