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