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