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

[Robert Winkler]

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