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

[Jeroen Hofstee]

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

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

I don't see the improvement of this patch..

Regards,
Jeroen