From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeroen Hofstee Date: Thu, 24 Jan 2013 23:34:57 +0100 Subject: [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation In-Reply-To: <5100F26A.7000404@compulab.co.il> 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> Message-ID: <5101B711.1020503@myspectrum.nl> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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