From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 23 Jul 2014 05:00:23 +0200
Subject: [U-Boot] [PATCH 01/12] video: Add S3C24xx framebuffer support
In-Reply-To: <20140722092509.685D53822C9@gemini.denx.de>
References: <1405989293-6629-1-git-send-email-marex@denx.de>
	<20140722092509.685D53822C9@gemini.denx.de>
Message-ID: <201407230500.23464.marex@denx.de>
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 Tuesday, July 22, 2014 at 11:25:09 AM, Wolfgang Denk wrote:
[...]
> > diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> > index 945f35d..7441783 100644
> > --- a/drivers/video/Makefile
> > +++ b/drivers/video/Makefile
> > @@ -33,6 +33,7 @@ obj-$(CONFIG_VIDEO_MB86R0xGDC) += mb86r0xgdc.o
> > videomodes.o
> > 
> >  obj-$(CONFIG_VIDEO_MX3) += mx3fb.o videomodes.o
> >  obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o
> >  obj-$(CONFIG_VIDEO_MXS) += mxsfb.o videomodes.o
> > 
> > +obj-$(CONFIG_VIDEO_S3C) += s3c-fb.o videomodes.o
> > 
> >  obj-$(CONFIG_VIDEO_OMAP3) += omap3_dss.o
> >  obj-$(CONFIG_VIDEO_SANDBOX_SDL) += sandbox_sdl.o
> >  obj-$(CONFIG_VIDEO_SED13806) += sed13806.o
> 
> can you please fix the sort oder of this ist?  Thanks.

Will do in V2, thanks.

> ...
> 
> > +	/* Suck display configuration from "videomode" variable */
> > +	penv = getenv("videomode");
> > +	if (!penv) {
> > +		puts("S3CFB: 'videomode' variable not set!\n");
> > +		return NULL;
> > +	}
> > +
> > +	bpp = video_get_params(&mode, penv);
> 
> Should there not be some error handling in case we pass invalid data?

There is one, about 10 lines further there is a 'switch (bpp) {}' statement. The 
default branch will trigger a fail.

> > +	/* Allocate framebuffer */
> > +	fb = memalign(S3CFB_ALIGN, roundup(panel.memSize, S3CFB_ALIGN));
> > +	if (!fb) {
> > +		printf("S3CFB: Error allocating framebuffer!\n");
> > +		return NULL;
> > +	}
> 
> Should we not use the gd->fb_base frame buffer allocation as provided
> in lib/board.c ?

I use CONFIG_VIDEO , not CONFIG_LCD here. My understanding is that CONFIG_VIDEO 
is what should be used and CONFIG_LCD is legacy. Please correct me if I'm wrong. 
Only the CONFIG_LCD will reserve the memory, CONFIG_VIDEO will not. It might be 
a good idea to explicitly set gd->fb_addr in the driver though. What do you 
think ?

Best regards,
Marek Vasut