From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Grinberg Date: Thu, 24 Jan 2013 10:35:54 +0200 Subject: [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation In-Reply-To: <5100609F.5000303@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> Message-ID: <5100F26A.7000404@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/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: >>> Hello Nikita, >>> >>> On 01/21/2013 08:51 AM, Nikita Kiryanov wrote: >>>> Hi Jeroen, >>>> >>>> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: >>>> [...] >>>>>> diff --git a/include/lcd.h b/include/lcd.h >>>>>> index c24164a..4ac4ddd 100644 >>>>>> --- a/include/lcd.h >>>>>> +++ b/include/lcd.h >>>>>> @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; >>>>>> extern void lcd_ctrl_init (void *lcdbase); >>>>>> extern void lcd_enable (void); >>>>>> +extern int board_splash_screen_prepare(void); >>>>>> /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ >>>>>> extern void lcd_setcolreg (ushort regno, >>>>> Other boards seem to do this in lcd_enable. Perhaps that is also an >>>>> option. >>>> >>>> The problem with doing it in lcd_enable is that it's akin to a side >>>> effect, given what the function's name advertises. Preparing the splash >>>> image can, however, be a natural part of the process that displays it. >>>> >>> 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? Please, be more specific, as both approaches are not suitable according to what was said above... > >>> >>> 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... I'd like to hear Anatolij's opinion on this. -- Regards, Igor.