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

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

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:
>>> Hello Nikita,
>>>
>>> On 01/21/2013 08:51 AM, Nikita Kiryanov wrote:
>>>> Hi Jeroen,
>>>>
>>>> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
>>>> [...]
>>>>>> diff --git a/include/lcd.h b/include/lcd.h
>>>>>> index c24164a..4ac4ddd 100644
>>>>>> --- a/include/lcd.h
>>>>>> +++ b/include/lcd.h
>>>>>> @@ -47,6 +47,7 @@ extern struct vidinfo panel_info;
>>>>>>   extern void lcd_ctrl_init (void *lcdbase);
>>>>>>   extern void lcd_enable (void);
>>>>>> +extern int board_splash_screen_prepare(void);
>>>>>>   /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */
>>>>>>   extern void lcd_setcolreg (ushort regno,
>>>>> Other boards seem to do this in lcd_enable. Perhaps that is also an
>>>>> option.
>>>>
>>>> The problem with doing it in lcd_enable is that it's akin to a side
>>>> effect, given what the function's name advertises. Preparing the splash
>>>> image can, however, be a natural part of the process that displays it.
>>>>
>>> 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?
Please, be more specific, as both approaches are not suitable according
to what was said above...

> 
>>>
>>> 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...
I'd like to hear Anatolij's opinion on this.


-- 
Regards,
Igor.