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

Nikita Kiryanov
2013-01-23T08:31:55Z

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.

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

>
> Regards,
> Jeroen
>
>

--
Regards,
Nikita.