[U-Boot] [PATCH] video:lcd:cfb_console: cm_t35: Add CONFIG_SPLASH_SCREEN_PREPARE support to CONFIG_VIDEO

[Igor Grinberg]

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.


-- 
Regards,
Igor.