From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Grinberg Date: Fri, 25 Jan 2013 08:45:07 +0200 Subject: [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation In-Reply-To: <5101B711.1020503@myspectrum.nl> References: <1356246228-26732-1-git-send-email-nikita@compulab.co.il> <1356246228-26732-3-git-send-email-nikita@compulab.co.il> <50FC54BC.3070101@myspectrum.nl> <50FCF38D.7070901@compulab.co.il> <50FD9398.1010603@myspectrum.nl> <50FF9FFB.4090806@compulab.co.il> <5100609F.5000303@myspectrum.nl> <5100F26A.7000404@compulab.co.il> <5101B711.1020503@myspectrum.nl> Message-ID: <510229F3.9060902@compulab.co.il> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. -- Regards, Igor.