From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeroen Hofstee Date: Wed, 23 Jan 2013 23:13:51 +0100 Subject: [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation In-Reply-To: <50FF9FFB.4090806@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> Message-ID: <5100609F.5000303@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 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). >> >> 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... I don't see the improvement of this patch.. Regards, Jeroen