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.