Re: [PATCH 1/2] Add support for Samsung S3C24x0 framebuffer.

[Juergen Beisert <jbe@pengutronix.de>]

Sascha Hauer wrote:
> Hi Alex,
>
> On Tue, May 17, 2011 at 10:44:00PM +0600, Alex Galakhov wrote:
> > Heavily based on original Juergen Beisert's code.
> >
> > Signed-off-by: Alex Galakhov <agalakhov@gmail.com>
> > ---
> >  arch/arm/mach-s3c24xx/include/mach/fb.h |   59 ++++
> >  drivers/video/Kconfig                   |   13 +
> >  drivers/video/Makefile                  |    1 +
> >  drivers/video/s3c.c                     |  451
> > +++++++++++++++++++++++++++++++ 4 files changed, 524 insertions(+), 0
> > deletions(-)
> >  create mode 100644 arch/arm/mach-s3c24xx/include/mach/fb.h
> >  create mode 100644 drivers/video/s3c.c
>
> The patch looks mostly fine, only small nitpicks follow.
>
> > diff --git a/arch/arm/mach-s3c24xx/include/mach/fb.h
> > b/arch/arm/mach-s3c24xx/include/mach/fb.h new file mode 100644
> > index 0000000..05e013a
> > --- /dev/null
> > +++ b/arch/arm/mach-s3c24xx/include/mach/fb.h
> > +
> > +/**
> > + * @param fb_info Framebuffer information
> > + */
> > +static void s3cfb_enable_controller(struct fb_info *fb_info)
> > +{
> > +	struct s3cfb_info *fbi = fb_info->priv;
> > +	uint32_t con1;
> > +
> > +	con1 = readl(fbi->base + LCDCON1);
> > +
> > +	con1 |= ENVID;
> > +
> > +	writel(con1, fbi->base + LCDCON1);
> > +
> > +	if (fbi->enable)
> > +		(fbi->enable)(1);
>
> unneeded brackets.

Seem my old code.

> > +}
> > +
> > +/**
> > + * @param fb_info Framebuffer information
> > + */
> > +static void s3cfb_disable_controller(struct fb_info *fb_info)
> > +{
> > +	struct s3cfb_info *fbi = fb_info->priv;
> > +	uint32_t con1;
> > +
> > +	if (fbi->enable)
> > +		(fbi->enable)(0);
>
> ditto
>
> > +
> > +	con1 = readl(fbi->base + LCDCON1);
> > +
> > +	con1 &= ~ENVID;
> > +
> > +	writel(con1, fbi->base + LCDCON1);
> > +}
> > +
> > +/**
> > + * Prepare the video hardware for a specified video mode
> > + * @param fb_info Framebuffer information
> > + * @param mode The video mode description to initialize
> > + * @return 0 on success
> > + */
> > +static int s3cfb_activate_var(struct fb_info *fb_info)
> > +{
> > +	struct s3cfb_info *fbi = fb_info->priv;
> > +	struct fb_videomode *mode = fb_info->mode;
> > +	unsigned size, hclk, div;
> > +	uint32_t con1, con2, con3, con4, con5 = 0;
> > +
> > +	if (fbi->passive_display != 0) {
> > +		pr_err("Passive displays are currently not supported\n");
>
> dev_err please (some more follow).
>
> > +
> > +	switch (fb_info->bits_per_pixel) {
> > +#if 0
> > +	/* TODO add CLUT based modes */
> > +	case 1:
> > +		con1 |= BPPMODE(8);
> > +		break;
> > +	case 2:
> > +		con1 |= BPPMODE(9);
> > +		break;
> > +	case 4:
> > +		con1 |= BPPMODE(10);
> > +		break;
> > +	case 8:
> > +		con1 |= BPPMODE(11);
> > +		break;
> > +#endif
>
> I think we can remove this dead code. I see no reason to add clut
> support. If someone needs it, I think this missing snippet is the least
> of his problems.
>
> > +
> > +/**
> > + * The S3C244x LCD controller supports passive (CSTN/STN) and active
> > (TFT) LC displays + *
> > + * The driver itself currently supports only active TFT LC displays in
> > the follwing manner: + *
> > + *  * Palletized colours
> > + *    - 1 bpp
> > + *    - 2 bpp
> > + *    - 4 bpp
> > + *    - 8 bpp
>
> This doesn't seem to be true, right?
>
> > + *  * True colours
> > + *    - 16 bpp
> > + *    - 24 bpp
> > + */

Its unfinished out of date. Alex should update or remove it.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-5121-206917-5128 |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox