From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mike Frysinger <vapier@gentoo.org>
Date: Thu, 20 Oct 2011 09:25:23 -0400
Subject: [U-Boot] [PATCH V4 3/3] api: export LCD device to external apps
In-Reply-To: <1319089126-7358-4-git-send-email-clchiou@chromium.org>
References: <1319089126-7358-1-git-send-email-clchiou@chromium.org>
	<1319089126-7358-4-git-send-email-clchiou@chromium.org>
Message-ID: <201110200925.24614.vapier@gentoo.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Thursday 20 October 2011 01:38:46 Che-Liang Chiou wrote:
> --- a/api/api.c
> +++ b/api/api.c
> 
> +/*
> + * pseudo signature:
> + *
> + * int API_display_get_info(int type, struct display_info *di)
> + */
> +static int API_display_get_info(va_list ap)
> +{
> +	int type;
> +	struct display_info *di;
> +
> +	type = (int)va_arg(ap, u_int32_t);
> +	di = (struct display_info *)va_arg(ap, u_int32_t);

i know you simply copied these warts from the rest of the code, but this 
va_arg() usage looks wrong to me.  why is u_int32_t always used as the 
argument to va_arg() and then the return value is cast ?  seems to me this 
code should actually read:
	type = va_arg(ap, int);
	di = va_arg(ap, struct display_info *);

could you see if that change still works for you ?

> +static int API_display_draw_bitmap(va_list ap)
> +{
> +	ulong bitmap;
> +	int x, y;
> +
> +	bitmap = (ulong)va_arg(ap, ulong);
> +	if (!bitmap)
> +		return API_EINVAL;

seems to me that this NULL pointer check belongs in the lcd core

> --- /dev/null
> +++ b/api/api_display.c
>
> +int display_draw_bitmap(ulong bitmap, int x, int y)
> +{
> +	int err = 0;
> +#ifdef CONFIG_LCD
> +	err |= lcd_display_bitmap(bitmap, x, y);
> +#endif
> +	return err;
> +}

i think this should read:
int display_draw_bitmap(ulong bitmap, int x, int y)
{
#ifdef CONFIG_LCD
	return lcd_display_bitmap(bitmap, x, y);
#else
	return API_ENODEV;
#endif
}

> --- a/include/video_font.h
> +++ b/include/video_font.h
> 
> +/*
> + * Adding unused attribute here so that compiler will not complain
> + * video_fontdata is not used in source files that only use
> + * VIDEO_FONT_* (e.g., api/api_display.c).
> + */
> +__attribute__ ((unused))
>  static unsigned char video_fontdata[VIDEO_FONT_SIZE] = {

i don't think this is right.  we should split the data from the rest of the 
defines/structs.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111020/53c09ffb/attachment.pgp