[PATCH] amba-clcd: add RGB444 support

[PATCH] amba-clcd: add RGB444 support

[Luotao Fu]

This one adds RGB444 (bpp=12) support to amba clcd drivers. Tested on a lpc3250
based platform.

Signed-off-by: Luotao Fu <l.fu@pengutronix.de>
---
 drivers/video/amba-clcd.c |   11 ++++++++---
 include/linux/amba/clcd.h |    7 ++++++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
index a21efcd..5111a12 100644
--- a/drivers/video/amba-clcd.c
+++ b/drivers/video/amba-clcd.c
@@ -134,6 +134,11 @@ clcdfb_set_bitfields(struct clcd_fb *fb, struct fb_var_screeninfo *var)
 		var->blue.length	= var->bits_per_pixel;
 		var->blue.offset	= 0;
 		break;
+	case 12:
+		var->red.length		= 4;
+		var->green.length	= 4;
+		var->blue.length	= 4;
+		break;
 	case 16:
 		var->red.length = 5;
 		var->blue.length = 5;
@@ -161,7 +166,7 @@ clcdfb_set_bitfields(struct clcd_fb *fb, struct fb_var_screeninfo *var)
 	 * encoded in the pixel data.  Calculate their position from
 	 * the bitfield length defined above.
 	 */
-	if (ret == 0 && var->bits_per_pixel >= 16) {
+	if (ret == 0 && var->bits_per_pixel >= 12) {
 		if (fb->panel->cntl & CNTL_BGR) {
 			var->blue.offset = 0;
 			var->green.offset = var->blue.offset + var->blue.length;
@@ -200,6 +205,8 @@ static int clcdfb_set_par(struct fb_info *info)
 	struct clcd_fb *fb = to_clcd(info);
 	struct clcd_regs regs;
 
+	fb->board->decode(fb, &regs);
+
 	fb->fb.fix.line_length = fb->fb.var.xres_virtual *
 				 fb->fb.var.bits_per_pixel / 8;
 
@@ -208,8 +215,6 @@ static int clcdfb_set_par(struct fb_info *info)
 	else
 		fb->fb.fix.visual = FB_VISUAL_TRUECOLOR;
 
-	fb->board->decode(fb, &regs);
-
 	clcdfb_disable(fb);
 
 	writel(regs.tim0, fb->regs + CLCD_TIM0);
diff --git a/include/linux/amba/clcd.h b/include/linux/amba/clcd.h
index 8d0d491..7e0bcc7 100644
--- a/include/linux/amba/clcd.h
+++ b/include/linux/amba/clcd.h
@@ -53,6 +53,7 @@
 #define CNTL_LCDBPP2		(1 << 1)
 #define CNTL_LCDBPP4		(2 << 1)
 #define CNTL_LCDBPP8		(3 << 1)
+#define CNTL_LCDBPP12		(7 << 1)
 #define CNTL_LCDBPP16		(4 << 1)
 #define CNTL_LCDBPP16_565	(6 << 1)
 #define CNTL_LCDBPP24		(5 << 1)
@@ -209,6 +210,10 @@ static inline void clcdfb_decode(struct clcd_fb *fb, struct clcd_regs *regs)
 	case 8:
 		val |= CNTL_LCDBPP8;
 		break;
+	case 12:
+		fb->fb.var.bits_per_pixel = 16;
+		val |= CNTL_LCDBPP12;
+		break;
 	case 16:
 		/*
 		 * PL110 cannot choose between 5551 and 565 modes in
@@ -225,7 +230,7 @@ static inline void clcdfb_decode(struct clcd_fb *fb, struct clcd_regs *regs)
 		val |= CNTL_LCDBPP24;
 		break;
 	}
-
+	printk("**** %s: cntl val 0x%08x\n", __func__, val);
 	regs->cntl = val;
 	regs->pixclock = fb->fb.var.pixclock;
 }
-- 
1.7.0

Re: [PATCH] amba-clcd: add RGB444 support

[Russell King - ARM Linux]

On Mon, Mar 15, 2010 at 02:30:09PM +0100, Luotao Fu wrote:
> This one adds RGB444 (bpp=12) support to amba clcd drivers. Tested on a lpc3250
> based platform.

No - this is wrong.  You have a 16bpp display.  bpp is the number of bits
in the framebuffer between two successive pixels.  It is not the number of
bits used for the pixel information.

The only case where you have a 12bpp display is if you have two pixels
packed into three bytes.

What you actually have is this arrangement:

111111
5432109876543210
....RRRRGGGGBBBB

where '....' are unused.

So, bpp = 16.  In order to work out whether you want RGB565, RGB555 or
RGB444, you have to look at the pixel format being requested in the
bitfield information.  I'd suggest looking at var->green.length:

                /*
                 * Green length can be 4, 5 or 6 depending whether
                 * we're operating in RGB444, RGB555 or RGB565 mode.
                 */
		if (var->green.length == 4) {
			var->blue.length = 4;
			var->red.length = 4;
		} else {
			var->blue.length = 5;
			var->red.length = 5;
			if (var->green.length != 5 && var->green.length != 6)
				var->green.length = 6;
		}

> 
> Signed-off-by: Luotao Fu <l.fu@pengutronix.de>
> ---
>  drivers/video/amba-clcd.c |   11 ++++++++---
>  include/linux/amba/clcd.h |    7 ++++++-
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
> index a21efcd..5111a12 100644
> --- a/drivers/video/amba-clcd.c
> +++ b/drivers/video/amba-clcd.c
> @@ -134,6 +134,11 @@ clcdfb_set_bitfields(struct clcd_fb *fb, struct fb_var_screeninfo *var)
>  		var->blue.length	= var->bits_per_pixel;
>  		var->blue.offset	= 0;
>  		break;
> +	case 12:
> +		var->red.length		= 4;
> +		var->green.length	= 4;
> +		var->blue.length	= 4;
> +		break;
>  	case 16:
>  		var->red.length = 5;
>  		var->blue.length = 5;
> @@ -161,7 +166,7 @@ clcdfb_set_bitfields(struct clcd_fb *fb, struct fb_var_screeninfo *var)
>  	 * encoded in the pixel data.  Calculate their position from
>  	 * the bitfield length defined above.
>  	 */
> -	if (ret == 0 && var->bits_per_pixel >= 16) {
> +	if (ret == 0 && var->bits_per_pixel >= 12) {
>  		if (fb->panel->cntl & CNTL_BGR) {
>  			var->blue.offset = 0;
>  			var->green.offset = var->blue.offset + var->blue.length;
> @@ -200,6 +205,8 @@ static int clcdfb_set_par(struct fb_info *info)
>  	struct clcd_fb *fb = to_clcd(info);
>  	struct clcd_regs regs;
>  
> +	fb->board->decode(fb, &regs);
> +
>  	fb->fb.fix.line_length = fb->fb.var.xres_virtual *
>  				 fb->fb.var.bits_per_pixel / 8;
>  
> @@ -208,8 +215,6 @@ static int clcdfb_set_par(struct fb_info *info)
>  	else
>  		fb->fb.fix.visual = FB_VISUAL_TRUECOLOR;
>  
> -	fb->board->decode(fb, &regs);
> -
>  	clcdfb_disable(fb);
>  
>  	writel(regs.tim0, fb->regs + CLCD_TIM0);
> diff --git a/include/linux/amba/clcd.h b/include/linux/amba/clcd.h
> index 8d0d491..7e0bcc7 100644
> --- a/include/linux/amba/clcd.h
> +++ b/include/linux/amba/clcd.h
> @@ -53,6 +53,7 @@
>  #define CNTL_LCDBPP2		(1 << 1)
>  #define CNTL_LCDBPP4		(2 << 1)
>  #define CNTL_LCDBPP8		(3 << 1)
> +#define CNTL_LCDBPP12		(7 << 1)
>  #define CNTL_LCDBPP16		(4 << 1)
>  #define CNTL_LCDBPP16_565	(6 << 1)
>  #define CNTL_LCDBPP24		(5 << 1)
> @@ -209,6 +210,10 @@ static inline void clcdfb_decode(struct clcd_fb *fb, struct clcd_regs *regs)
>  	case 8:
>  		val |= CNTL_LCDBPP8;
>  		break;
> +	case 12:
> +		fb->fb.var.bits_per_pixel = 16;
> +		val |= CNTL_LCDBPP12;
> +		break;
>  	case 16:
>  		/*
>  		 * PL110 cannot choose between 5551 and 565 modes in
> @@ -225,7 +230,7 @@ static inline void clcdfb_decode(struct clcd_fb *fb, struct clcd_regs *regs)
>  		val |= CNTL_LCDBPP24;
>  		break;
>  	}
> -
> +	printk("**** %s: cntl val 0x%08x\n", __func__, val);
>  	regs->cntl = val;
>  	regs->pixclock = fb->fb.var.pixclock;
>  }
> -- 
> 1.7.0
> 

Re: [PATCH] amba-clcd: add RGB444 support

[Luotao Fu]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

Hi Russell,

On Mon, Mar 15, 2010 at 01:38:10PM +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 15, 2010 at 02:30:09PM +0100, Luotao Fu wrote:
> > This one adds RGB444 (bpp=12) support to amba clcd drivers. Tested on a lpc3250
> > based platform.
> 
> No - this is wrong.  You have a 16bpp display.  bpp is the number of bits
> in the framebuffer between two successive pixels.  It is not the number of
> bits used for the pixel information.
> 

> The only case where you have a 12bpp display is if you have two pixels
> packed into three bytes.
>
For 12bpp actually 16bits/pixel is used. I'm aware of that. That's the
reason why the fb.var.bits_per_pixel is set to 16 in clcdfb_decode if
it is originally set to 12 from the platform file to make usage of 16bpp
display. omapfb driver does quite the same.

On my framebuffer controller (here on a lpc3250) there is hardware RGB444
support. Switching to this mode will spare some pins. What I intend to
do with this patch is to make use of this feature. Similar RGB444
Support can be found in omapfb.

cheers
Luotao Fu
-- 
Pengutronix e.K.                           | Dipl.-Ing. Luotao Fu        |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] amba-clcd: add RGB444 support

[Russell King - ARM Linux]

On Mon, Mar 15, 2010 at 02:59:45PM +0100, Luotao Fu wrote:
> Hi Russell,
> 
> On Mon, Mar 15, 2010 at 01:38:10PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Mar 15, 2010 at 02:30:09PM +0100, Luotao Fu wrote:
> > > This one adds RGB444 (bpp=12) support to amba clcd drivers. Tested on a lpc3250
> > > based platform.
> > 
> > No - this is wrong.  You have a 16bpp display.  bpp is the number of bits
> > in the framebuffer between two successive pixels.  It is not the number of
> > bits used for the pixel information.
> > 
> 
> > The only case where you have a 12bpp display is if you have two pixels
> > packed into three bytes.
> >
> For 12bpp actually 16bits/pixel is used. I'm aware of that. That's the
> reason why the fb.var.bits_per_pixel is set to 16 in clcdfb_decode if
> it is originally set to 12 from the platform file to make usage of 16bpp
> display. omapfb driver does quite the same.

Then omapfb is also buggy and needs fixing in a similar manner.

The de-facto way of determining the bitfield structure is as I highlighted,
which is used by all the x86 drivers supporting RGB555 and RGB565.

We don't use bits_per_pixel=15 for RGB555, and using bits_per_pixel=12 for
RGB444 is wrong for all the same reasons.