[U-Boot] [PATCH 3/6] video: cfb_console: logo can be positioned via the splashpos variable

[Anatolij Gustschin]

Hi Bastian,

there is a number of issues with this patch, please see comments
below.

On Fri, 10 Aug 2012 09:26:43 +0200
Bastian Ruppert <Bastian.Ruppert at Sewerin.de> wrote:

> Signed-off-by: Bastian Ruppert <Bastian.Ruppert at Sewerin.de>
> CC: Anatolij Gustschin <agust at denx.de>
> CC: Tom Rini <trini at ti.com>
> CC: Stefano Babic <sbabic at denx.de>
> ---
>  drivers/video/cfb_console.c |   61 ++++++++++++++++++++++++++++---------------
>  1 files changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> index 19d061f..21b52bd 100644
> --- a/drivers/video/cfb_console.c
> +++ b/drivers/video/cfb_console.c
> @@ -66,7 +66,11 @@
>   * CONFIG_CONSOLE_TIME	      - display time/date in upper right
>   *				corner, needs CONFIG_CMD_DATE and
>   *				CONFIG_CONSOLE_CURSOR
> - * CONFIG_VIDEO_LOGO	      - display Linux Logo in upper left corner
> + * CONFIG_VIDEO_LOGO	      - display Linux Logo in upper left corner.
> + *				Use CONFIG_SPLASH_SCREEN_ALIGN with
> + *				environment variable "splashpos" to place
> + *				the logo on other position. In this case
> + *				no CONSOLE_EXTRA_INFO is possible.
>   * CONFIG_VIDEO_BMP_LOGO      - use bmp_logo instead of linux_logo
>   * CONFIG_CONSOLE_EXTRA_INFO  - display additional board information
>   *				strings that normaly goes to serial
> @@ -369,6 +373,8 @@ static void *video_fb_address;	/* frame buffer address */
>  static void *video_console_address;	/* console buffer start address */
>  
>  static int video_logo_height = VIDEO_LOGO_HEIGHT;
> +static int video_logo_xpos;
> +static int video_logo_ypos;
>  
>  static int __maybe_unused cursor_state;
>  static int __maybe_unused old_col;
> @@ -1594,40 +1600,53 @@ static void *video_logo(void)
>  	__maybe_unused int y_off = 0;
>  
>  #ifdef CONFIG_SPLASH_SCREEN
> -	char *s;
>  	ulong addr;
> -
> -	s = getenv("splashimage");
> +#endif
> +#if defined(CONFIG_SPLASH_SCREEN) || defined(CONFIG_SPLASH_SCREEN_ALIGN)
> +	char *s;
> +#endif

these ifdefs should be better reduced, I think we can use __maybe_unused
here, like for y_off above.

> +#ifdef CONFIG_SPLASH_SCREEN_ALIGN
> +	s = getenv("splashpos");
>  	if (s != NULL) {
> -		int x = 0, y = 0;
> +		if (s[0] == 'm')
> +			video_logo_xpos = BMP_ALIGN_CENTER;

The 'm' case will work with splashscreen, but not with the video logo.
There is no proper offset calculation in logo_plot() if xpos or ypos
are set to BMP_ALIGN_CENTER. As a result the logo offset will be wrong
and an access to wrong offset can even brick the board (on boards with
small frame buffers).

...
> +
> +		if (video_display_bitmap(addr,			\
> +					video_logo_xpos,	\

no need to use "\" here.

...
> +	logo_plot(video_fb_address,		\
> +		VIDEO_COLS,			\
> +		video_logo_xpos,		\

ditto.

...
> +#ifdef CONFIG_SPLASH_SCREEN_ALIGN
> +	/* when using splashpos for video_logo, no console output */
> +	return (video_fb_address + video_logo_height * VIDEO_LINE_LEN);

The returned address is used as text console offset, so if the logo is
moved down, the video_logo_height should be increased by video_logo_ypos.
Otherwise the text and cursor positions will be wrong in the video
console.

I've fixed these issues and submitted a patch v2 3/6. Please test.

Thanks,

Anatolij