From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeroen Hofstee Date: Sat, 26 Jan 2013 14:33:58 +0100 Subject: [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation In-Reply-To: <510229F3.9060902@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> <5101B711.1020503@myspectrum.nl> <510229F3.9060902@compulab.co.il> Message-ID: <5103DB46.2000802@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/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