[U-Boot] [PATCH] Added support for splash screen positioning adding by adding

Anatolij Gustschin agust at denx.de
Wed Jul 1 11:18:24 CEST 2009


Dear Matthias,

Thanks for this patch, some comments however. Please use short
patch subject, e.g. "[PATCH] Add support for splash screen positioning"
is enough. And than please add more descriptive patch commit message
above your "Signed-off-by" line. In this commit message you can shortly
explain how it is done (e.g. what you mean with "adding by adding").

This is a new feature and also we should document it in the README
where the original "splashimage" environment variable is documented.
But if this feature will be documented, we also have to support it
in other places, it is used in "common/lcd.c".
Also please see other comments below.

Matthias Weisser wrote:
> Signed-off-by: Matthias Weisser <matthias.weisser at graf-syteco.de>
> ---
>  drivers/video/cfb_console.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 drivers/video/cfb_console.c
> 
> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> old mode 100644
> new mode 100755
> index bcafb27..15b99cb
> --- a/drivers/video/cfb_console.c
> +++ b/drivers/video/cfb_console.c
> @@ -314,7 +314,7 @@ void	console_cursor (int state);
>  #else
>  #define SWAP16(x)	 (x)
>  #define SWAP32(x)	 (x)
> -#if defined(VIDEO_FB_16BPP_PIXEL_SWAP)
> +#if defined(VIDEO_FB_16BPP_PIXEL_SWAP) || defined (CONFIG_VIDEO_JADEGDC)

Here you added CONFIG_VIDEO_JADEGDC case. Please do this in your
other patch where you add support for JADE GDC. This patch addresses
extension of "splashimage" and should do only this logical change.

>  #define SHORTSWAP32(x)	 ( ((x) >> 16) | ((x) << 16) )
>  #else
>  #define SHORTSWAP32(x)	 (x)
> @@ -1188,9 +1188,17 @@ static void *video_logo (void)
>  	ulong addr;
>  
>  	if ((s = getenv ("splashimage")) != NULL) {
> +		int x = 0, y = 0;
> +		
----^^^^^^^^^^^^
please remove tabs in the empty line above.

>  		addr = simple_strtoul (s, NULL, 16);
>  
> -		if (video_display_bitmap (addr, 0, 0) == 0) {
> +		if ((s = strchr (s, ' ')) != NULL) {
> +				x = simple_strtoul (s + 1, NULL, 0);
> +			if ((s = strchr (s + 1, ' ')) != NULL) 
------------------------------------------------------------^^^^
remove trailing white space here, please.

> +				y = simple_strtoul (s + 1, NULL, 0);
> +		}
> +		
-----^^^^^^^^^^^
please remove tabs here, too.

> +		if (video_display_bitmap (addr, x, y) == 0) {
>  			return ((void *) (video_fb_address));
>  		}
>  	}

Please fix and resubmit, thanks!
Anatolij


More information about the U-Boot mailing list