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.