From: Jeroen Hofstee <jeroen@myspectrum.nl>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation
Date: Wed, 23 Jan 2013 23:13:51 +0100 [thread overview]
Message-ID: <5100609F.5000303@myspectrum.nl> (raw)
In-Reply-To: <50FF9FFB.4090806@compulab.co.il>
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
next prev parent reply other threads:[~2013-01-23 22:13 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-23 7:03 [U-Boot] [PATCH 0/5] Add splash screen for CM-T35 Nikita Kiryanov
2012-12-23 7:03 ` [U-Boot] [PATCH 1/5] omap3: add useful dss defines Nikita Kiryanov
2013-01-20 21:42 ` Jeroen Hofstee
2013-01-21 7:53 ` Nikita Kiryanov
2013-01-21 18:38 ` Jeroen Hofstee
2013-01-23 8:23 ` Nikita Kiryanov
2012-12-23 7:03 ` [U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation Nikita Kiryanov
2013-01-20 20:34 ` Jeroen Hofstee
2013-01-21 7:51 ` Nikita Kiryanov
2013-01-21 19:14 ` Jeroen Hofstee
2013-01-23 8:31 ` Nikita Kiryanov
2013-01-23 22:13 ` Jeroen Hofstee [this message]
2013-01-24 8:35 ` Igor Grinberg
2013-01-24 22:34 ` Jeroen Hofstee
2013-01-25 6:45 ` Igor Grinberg
2013-01-26 13:33 ` Jeroen Hofstee
2012-12-23 7:03 ` [U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays Nikita Kiryanov
2013-01-20 20:59 ` Jeroen Hofstee
2013-01-21 8:12 ` Nikita Kiryanov
2013-01-23 21:39 ` Jeroen Hofstee
2013-01-24 9:02 ` Igor Grinberg
2012-12-23 7:03 ` [U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters Nikita Kiryanov
2013-01-20 21:08 ` Jeroen Hofstee
2013-01-21 8:25 ` Nikita Kiryanov
2013-01-23 22:36 ` Jeroen Hofstee
2013-01-24 9:12 ` Igor Grinberg
2012-12-23 7:03 ` [U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND Nikita Kiryanov
2012-12-24 8:55 ` Jeroen Hofstee
2012-12-25 8:56 ` Nikita Kiryanov
2012-12-26 14:27 ` Jeroen Hofstee
2012-12-30 14:39 ` Nikita Kiryanov
2013-01-22 7:37 ` Albert ARIBAUD
2013-01-23 10:47 ` Nikita Kiryanov
2013-01-23 11:07 ` Nikita Kiryanov
2013-03-26 14:51 ` [U-Boot] [U-Boot, " Tom Rini
2013-01-20 12:25 ` [U-Boot] [PATCH 0/5] Add splash screen for CM-T35 Nikita Kiryanov
2013-01-20 20:31 ` Jeroen Hofstee
2013-01-21 14:10 ` Tom Rini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5100609F.5000303@myspectrum.nl \
--to=jeroen@myspectrum.nl \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox