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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
