[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
Igor Grinberg
grinberg at compulab.co.il
Sun Jun 9 09:35:55 CEST 2013
On 06/06/13 22:06, Robert Winkler wrote:
> Hi All,
>
> On Wed, Jun 5, 2013 at 1:31 AM, Igor Grinberg <grinberg at compulab.co.il> wrote:
>> Hi Robert,
>>
>> On 06/04/13 21:11, Robert Winkler wrote:
>>> Adding Anatolij to the CC list.
>>>
>>> On Tue, Jun 4, 2013 at 8:10 AM, Robert Winkler
>>> <robert.winkler at boundarydevices.com> wrote:
>>>> Hi Igor,
>>>>
>>>> On Mon, Jun 3, 2013 at 11:10 PM, Igor Grinberg <grinberg at compulab.co.il> wrote:
>>>>> Hi Robert,
>>>>>
>>>>> On 06/03/13 20:20, Robert Winkler wrote:
>>>>>> Also change splash_screen_prepare to a weak function.
>>>>>
>>>>> You should be able to make a commit message a bit better.
>>>>> Also, personally, I see here two functional changes and it would be better
>>>>> to separate them into two patches:
>>>>> 1) Make the splash_screen_prepare() weak (I don't like this approach,
>>>>> explanation below)
>>>>> 2) Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO
>>>>>
>>>>> We have considered making the splash_screen_prepare() function weak
>>>>> in the past, but decided to make it a config option instead.
>>>>> This way it is documented in the README and is selectable in the
>>>>> board config.
>>>>> Once you change it to be weak, there is no point in the config option
>>>>> anymore... Personally, I don't like this approach.
>>>>>
>>>> Wolfgang said as long as I was fixing the bug of SPLASH_SCREEN_PREPARE
>>>> not working
>>>> for CONFIG_VIDEO I should change it to a weak function so that's what I did.
>>>>
>>>> See the email here:
>>>> http://lists.denx.de/pipermail/u-boot/2013-May/155478.html
>>
>> Ok.
>> The major benefit of the construct (which Wolfgang wants to remove) is
>> clear code with no #ifdefs inside functions.
>> I don't have any special feelings for that construct, but I'd like to
>> keep #ifdefs out of any functions (the benefit).
>>
>>>>
>>>> I agree with you about the pointlessness of the CONFIG option and I
>>>> too like having
>>>> it documented in the README. That's the only reason I left the
>>>> #ifdefs in at all because
>>>> I didn't want to/didn't think I should remove the CONFIG altogether.
>>>>
>>>> I'm not sure what the solution is.
>>
>> ...
>> some thoughts below...
>>
>>>>
>>>>
>>>>>>
>>>>>> Signed-off-by: Robert Winkler <robert.winkler at boundarydevices.com>
>>>>>> ---
>>>>>> board/compulab/cm_t35/cm_t35.c | 2 +-
>>>>>> common/lcd.c | 10 ++++------
>>>>>> drivers/video/cfb_console.c | 14 ++++++++++++++
>>>>>> 3 files changed, 19 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/board/compulab/cm_t35/cm_t35.c b/board/compulab/cm_t35/cm_t35.c
>>>>>> index b0b80e5..95098af 100644
>>>>>> --- a/board/compulab/cm_t35/cm_t35.c
>>>>>> +++ b/board/compulab/cm_t35/cm_t35.c
>>>>>> @@ -120,7 +120,7 @@ static inline int splash_load_from_nand(void)
>>>>>> }
>>>>>> #endif /* CONFIG_CMD_NAND */
>>>>>>
>>>>>> -int board_splash_screen_prepare(void)
>>>>>> +int splash_screen_prepare(void)
>>>>>> {
>>>>>> char *env_splashimage_value;
>>>>>> u32 bmp_load_addr;
>>>>>> diff --git a/common/lcd.c b/common/lcd.c
>>>>>> index edae835..90f1143 100644
>>>>>> --- a/common/lcd.c
>>>>>> +++ b/common/lcd.c
>>>>>> @@ -1069,15 +1069,13 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
>>>>>> #endif
>>>>>>
>>>>>> #ifdef CONFIG_SPLASH_SCREEN_PREPARE
>>>>>> -static inline int splash_screen_prepare(void)
>>>>>> -{
>>>>>> - return board_splash_screen_prepare();
>>>>>> -}
>>>>>> -#else
>>>>>> -static inline int splash_screen_prepare(void)
>>>>>> +int __splash_screen_prepare(void)
>>>>>> {
>>>>>> return 0;
>>>>>> }
>>>>>> +
>>>>>> +int splash_screen_prepare(void)
>>>>>> + __attribute__ ((weak, alias("__splash_screen_prepare")));
>>>>>> #endif
>>>>>
>>>>> You remove the #else statement, apparently you break the compilation for
>>>>> !CONFIG_SPLASH_SCREEN_PREPARE, as the below lcd_logo() function references
>>>>> the splash_screen_prepare() function.
>>>>> Also, this means you force the code to have the ugly #ifdefs inside
>>>>> functions - that is not really nice.
>>>>>
>>>>>>
>>>>>> static void *lcd_logo(void)
>>>>>> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
>>>>>> index 0793f07..9180998 100644
>>>>>> --- a/drivers/video/cfb_console.c
>>>>>> +++ b/drivers/video/cfb_console.c
>>>>>> @@ -1966,6 +1966,16 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)
>>>>>> #endif
>>>>>> }
>>>>>>
>>>>>> +#ifdef CONFIG_SPLASH_SCREEN_PREPARE
>>>>>> +int __splash_screen_prepare(void)
>>>>>> +{
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +int splash_screen_prepare(void)
>>>>>> + __attribute__ ((weak, alias("__splash_screen_prepare")));
>>>>>> +#endif
>>>>>
>>>>> Why duplicate the above?
>>>>> Probably, just place it in a common location?
>>>> I asked Wolfgang about this in the original thread but haven't heard
>>>> back so I just submitted it
>>>> like this with the thought that CONFIG_VIDEO and CONFIG_LCD are never
>>>> used simultaneously and are
>>>> designed not to be (I could easily be wrong about that).
>>>>
>>>>>
>>>>>> +
>>>>>> static void *video_logo(void)
>>>>>> {
>>>>>> char info[128];
>>>>>> @@ -1996,6 +2006,10 @@ static void *video_logo(void)
>>>>>> s = getenv("splashimage");
>>>>>> if (s != NULL) {
>>>>>>
>>>>>> +#ifdef CONFIG_SPLASH_SCREEN_PREPARE
>>>>>> + splash_screen_prepare();
>>>>>> +#endif
>>>>>
>>>>> These are the ugly #ifdefs, I was talking about...
>>>> Agreed
>>>>>
>>>>> I would recommend to just move the splash_screen_prepare() function definition
>>>>> to a common location and add the above function call (with no #ifdef around).
>>>>
>>>> And keep the meaningless CONFIG?
>>
>> While I was writing the above, I meant to keep the #ifdef ... #else ... #endif
>> construct.
>>
>> So currently, after reading the link you've provided,
>> I can see two reasonable solutions:
>>
>> 1) Keep the #ifdef construct as it was, but move it to a common location,
>> so it can be called from both the lcd.c and cfb_console.c with no code
>> duplication. This also keeps the CONFIG option still meaningful.
>>
>> 2) Remove the construct and make the splash_screen_prepare() function weak.
>> Move the weak definition to a common location, so it can be called from both
>> the lcd.c and cfb_console.c with no code duplication.
>> Remove the CONFIG option as it turns meaningless, but move its documentation
>> (with needed adjustments) to some splash screen doc place. This will keep the
>> functions clear of the #ifdefs.
>>
>> I would go for 1) and the patch for it is shorter.
>> For 2) it still should be two patches:
>> * Making the splash_screen_prepare() function weak and removing CONFIG option.
>> * Fixing the bug with cfb_console.c by moving the weak definition
>> to a common location and adding a call to it in cfb_console.c.
>>
>
> What common location is there? In either case (making it weak or not)
> I'm not sure where to put it and it looks like
> common.h, video_font.h and video_fond_data.h are the only headers they
> share so common.h is probably where I'd put the prototype if I didn't
> create a new header.
Well, this is a splash screen specific code, may it is time for splash.h?
This might be a question for Anatolij?
>
> Either way it sees stupid to make it's own C file unless we were also
> taking the time to pull out a lot more of the duplication between
> lcd.c and cfb_console.c at the same time.
Well, its own C file might be a good approach.
I don't think you have to take the time right now to pull out
all the duplications, but you can start this and then we encourage people
to continue your work.
> There is a lot of duplication and very similar code. There have been
> some similar extractions the past:
> c270730f580e85ddab82e981abf8a518f78ae803
> d3983ee85325d2be730830ebcf82585ee7cd2ecb
>
> One other unrelated question for Nikita from his splash prepare patch.
> Why did you use gd->fb_base when the global lcd_base is used
> elsewhere
> in the function and lcd.c in general. It seems inconsistent.
>
> 1094 if (splash_screen_prepare())
> 1095 return (void *)gd->fb_base;
>
>> --
>> Regards,
>> Igor.
>
>
> Please advise,
>
> Robert
>
--
Regards,
Igor.
More information about the U-Boot
mailing list