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

Currently there is no logical place to put the code that prepares the
splash image data. The splash image data should be ready in memory
before bmp_display() is called, and after the environment is ready
(since lcd.c looks for the splash image in an address specified by
the environment variable "splashimage").

Our window of opportunity in board_init_r() is therefore: between
env_relocate() and bmp_display(), and from the available options
only the lcd related functions in drv_lcd_init() seem appropriate
for such lcd oriented code.

Add the option to prepare the splash image data in lcd_logo() right
before it is sent to be displayed.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 README        |    8 ++++++++
 common/lcd.c  |   15 +++++++++++++++
 include/lcd.h |    1 +
 3 files changed, 24 insertions(+)

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> Currently there is no logical place to put the code that prepares the
> splash image data. The splash image data should be ready in memory
> before bmp_display() is called, and after the environment is ready
> (since lcd.c looks for the splash image in an address specified by
> the environment variable "splashimage").
>
> Our window of opportunity in board_init_r() is therefore: between
> env_relocate() and bmp_display(), and from the available options
> only the lcd related functions in drv_lcd_init() seem appropriate
> for such lcd oriented code.
>
> Add the option to prepare the splash image data in lcd_logo() right
> before it is sent to be displayed.
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>   README        |    8 ++++++++
>   common/lcd.c  |   15 +++++++++++++++
>   include/lcd.h |    1 +
>   3 files changed, 24 insertions(+)
>
> diff --git a/README b/README
> index 78f40df..cffc016 100644
> --- a/README
> +++ b/README
> @@ -1541,6 +1541,14 @@ CBFS (Coreboot Filesystem) support
>   			=> vertically centered image
>   			   at x = dspWidth - bmpWidth - 9
>   
> +		CONFIG_SPLASH_SCREEN_PREPARE
> +
> +		If this option is set then the board_splash_screen_prepare()
> +		function, which must be defined in your code, is called as part
> +		of the splash screen display sequence. It gives the board an
> +		opportunity to prepare the splash image data before it is
> +		processed and sent to the frame buffer by U-Boot.
> +
>   - Gzip compressed BMP image support: CONFIG_VIDEO_BMP_GZIP
>   
>   		If this option is set, additionally to standard BMP
> diff --git a/common/lcd.c b/common/lcd.c
> index 66d4f94..129cf7e 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -1034,6 +1034,18 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
>   }
>   #endif
>   
> +#ifdef CONFIG_SPLASH_SCREEN_PREPARE
> +static inline int splash_screen_prepare(void)
> +{
> +	return board_splash_screen_prepare();
> +}
> +#else
> +static inline int splash_screen_prepare(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>   static void *lcd_logo(void)
>   {
>   #ifdef CONFIG_SPLASH_SCREEN
> @@ -1045,6 +1057,9 @@ static void *lcd_logo(void)
>   		int x = 0, y = 0;
>   		do_splash = 0;
>   
> +		if (splash_screen_prepare())
> +			return (void *)lcd_base;
> +
>   		addr = simple_strtoul (s, NULL, 16);
>   #ifdef CONFIG_SPLASH_SCREEN_ALIGN
>   		s = getenv("splashpos");
> 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.

Regards,
Jeroen

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.

>
> Regards,
> Jeroen

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

But anyway, can't this at least be changed to a __weak function, so the
CONFIG and ifdef stuff can be dropped?

Regards,
Jeroen

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

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

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.

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

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

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

Regards,
Jeroen

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.

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