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

[Igor Grinberg <grinberg@compulab.co.il>]

On 01/25/13 00:34, Jeroen Hofstee wrote:
> 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.

That said, lcd_clear() calls lcd_logo()...
This is the real source of confusion here - the side effect,
and not the fact that lcd_logo() calls the board_splash_screen_prepare()...
Being that a problem not directly related to Nikita's patch set, I don't
think we should delay it any further.

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

I think not, lcd_logo() fits perfectly for loading the splash screen.
The fact that lcd_logo() is called from lcd_clear(), IMO,
would be the one that should be dealt with if you want to remove the
confusion ("the side effect"). But that is not related to Nikita's
patch set.

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

As explained above, the patch only uses lcd_logo(), which is meant to
display the splash screen, and add the loading functionality.
The fact that the lcd_logo() is called from lcd_clear() is the one
you should blame. And as already said, not related to this patch.

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

Ok. So we agree that this can't be done in lcd_enable().

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

Usually, I also like the __weak approach, but this time the intention was
to document the feature and hopefully stop the lcd_*() API abuse.


-- 
Regards,
Igor.