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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
