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

[Robert Winkler]

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
>
> 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.
>
>
>>>
>>> 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?
>
>>
>>> +
>>>               addr = simple_strtoul(s, NULL, 16);
>>>
>>>
>>>
>>
>> --
>> Regards,
>> Igor.
>
> Robert Winkler