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

[Jeroen Hofstee]

Hello Igor,

On 01/25/2013 07:45 AM, Igor Grinberg wrote:
> 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.
>

Given that I now know that lcd_enable is not an alternative to this
patch, but adding a callback is the only method to load a bitmap
and have it shown, I understand now that this patch is not just a
formal / cleanup change, but adds an actually missing feature.
So I am fine with having it inside lcd_logo.

>> 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().
>
yes.
>>> 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,
Jeroen