[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Jeroen Hofstee]

Hello Igor,

On 01/24/2013 09:35 AM, Igor Grinberg wrote:
> On 01/24/13 00:13, Jeroen Hofstee wrote:
>> Hello Nikita,
>>
>> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote:
>>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:
>>>>
>>>> mmm, I am not so sure I agree that loading a bitmap in lcd_enable is
>>>> a _problem_, while loading it in show logo and requiring a CONFIG_* is
>>>> _natural_.
>>> Well, it is a problem. If we don't respect the abstractions we create
>>> then things like function names become meaningless. A function called
>>> "lcd_enable" should do just that- enable lcd. Not load stuff from
>>> storage to memory or manipulate BMPs.
>>>
>> my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it,
>> it seems you're make a side effect of a function only called once a side effect
>> of another function (which could be called multiple times). So you make things
>> even worse (loading an bitmap while the function is just named to display it).
> So what's your point? Do you think we should add a splash screen specific
> callback inside the board.c U-Boot boot flow?
no.
> Please, be more specific, as both approaches are not suitable according
> to what was said above...

lets see, drv_lcd_init calls lcd_init. which does

lcd_ctrl_init(lcdbase);
lcd_is_enabled = 1;
lcd_clear();
lcd_enable();

After this patch, lcd_clear calls lcd_logo which calls
board_splash_screen_prepare in its turn. In my mind this
still leaves allot of side effects. If you want to prepare
for displaying and not have it as a side effect of a function
which is named to do something else, it should be in
between lcd_ctrl_init and lcd_clear in my mind.

>
>>>> But anyway, can't this at least be changed to a __weak function, so the
>>>> CONFIG and ifdef stuff can be dropped?
>>> The motivation behind the CONFIG was to make it a documentable user setting,
>>> rather than an undocumented feature buried in the code.
>>>
>> then document the callback...
> Sorry, may be I've missed something, but I can't see any callback being
> documented in the README file...
>
>> I don't see the improvement of this patch..
> What does that suppose to mean? Either be constructive or don't bother...
This means, as I hopefully explained a bit more clearly now, that
the patch makes the loading of a bitmap a side effect of lcd_clear,
while the intention was to make it a more natural call sequence.
(which can simply be done by putting it somewhere else as
mentioned above)

btw, I think, loading the image in lcd_enable() won't even work
since lcd_enable is actually before lcd_clear. Scanning some
boards which load in lcd_enable, they seem to call bmp_display
manually. So that makes this patch no longer optional, but is
actually required and is an improvement....
> I'd like to hear Anatolij's opinion on this.
>
yes, me too. I like the __weak far more than requiring a CONFIG_to
enable a callback. I cannot think of a reason why these __weak
functions cannot be documented. So that's up to Anatolij.

Regards,
Jeroen