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

[Igor Grinberg]

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.

> 
> 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?

> +
>  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...

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).

> +
>  		addr = simple_strtoul(s, NULL, 16);
>  
>  
> 

-- 
Regards,
Igor.