[PATCH v3] video: da8xx-fb: add 24bpp LCD configuration support

[PATCH v3] video: da8xx-fb: add 24bpp LCD configuration support

[Manjunathappa, Prakash]

LCD controller on am335x supports 24bpp raster configuration in addition
to ones on da850. LCDC also supports 24bpp in unpacked format having
ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
component of the data.

Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
Cc: Anatolij Gustschin <agust@denx.de>
---
Since v2:
Fixed additional configurations for 24bpp support.
Since v1:
Simplified calculation of pseudopalette for FB_VISUAL_TRUECOLOR type.

 drivers/video/da8xx-fb.c |  127 ++++++++++++++++++++++++++++++++++------------
 1 files changed, 94 insertions(+), 33 deletions(-)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 47118c7..3d2d0d1 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -83,6 +83,8 @@
 #define LCD_V2_LIDD_CLK_EN		BIT(1)
 #define LCD_V2_CORE_CLK_EN		BIT(0)
 #define LCD_V2_LPP_B10			26
+#define LCD_V2_TFT_24BPP_MODE		BIT(25)
+#define LCD_V2_TFT_24BPP_UNPACK		BIT(26)
 
 /* LCD Raster Timing 2 Register */
 #define LCD_AC_BIAS_TRANSITIONS_PER_INT(x)	((x) << 16)
@@ -153,7 +155,7 @@ struct da8xx_fb_par {
 	unsigned int		dma_end;
 	struct clk *lcdc_clk;
 	int irq;
-	unsigned short pseudo_palette[16];
+	unsigned long pseudo_palette[16];
 	unsigned int palette_sz;
 	unsigned int pxl_clk;
 	int blank;
@@ -482,6 +484,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
 {
 	u32 reg;
 
+	if ((bpp > 16) && (lcd_revision = LCD_VERSION_1))
+		return -EINVAL;
+
 	/* Set the Panel Width */
 	/* Pixels per line = (PPL + 1)*16 */
 	if (lcd_revision = LCD_VERSION_1) {
@@ -525,6 +530,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
 	reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8);
 	if (raster_order)
 		reg |= LCD_RASTER_ORDER;
+
+	if (bpp = 24)
+		reg |= LCD_V2_TFT_24BPP_MODE;
+	else if (bpp = 32)
+		reg |= (LCD_V2_TFT_24BPP_MODE | LCD_V2_TFT_24BPP_UNPACK);
+
 	lcdc_write(reg, LCD_RASTER_CTRL_REG);
 
 	switch (bpp) {
@@ -532,6 +543,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
 	case 2:
 	case 4:
 	case 16:
+	case 24:
+	case 32:
 		par->palette_sz = 16 * 2;
 		break;
 
@@ -546,6 +559,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
 	return 0;
 }
 
+
+#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)
 static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
 			      unsigned blue, unsigned transp,
 			      struct fb_info *info)
@@ -561,13 +576,36 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
 	if (info->fix.visual = FB_VISUAL_DIRECTCOLOR)
 		return 1;
 
-	if (info->var.bits_per_pixel = 4) {
-		if (regno > 15)
-			return 1;
+	if ((info->var.bits_per_pixel > 16) && (lcd_revision = LCD_VERSION_1))
+		return 1;
 
-		if (info->var.grayscale) {
-			pal = regno;
-		} else {
+	switch (info->fix.visual) {
+	case FB_VISUAL_TRUECOLOR:
+		red = CNVT_TOHW(red, info->var.red.length);
+		green = CNVT_TOHW(green, info->var.green.length);
+		blue = CNVT_TOHW(blue, info->var.blue.length);
+		break;
+	case FB_VISUAL_PSEUDOCOLOR:
+		if (info->var.bits_per_pixel = 4) {
+			if (regno > 15)
+				return 1;
+
+			if (info->var.grayscale) {
+				pal = regno;
+			} else {
+				red >>= 4;
+				green >>= 8;
+				blue >>= 12;
+
+				pal = (red & 0x0f00);
+				pal |= (green & 0x00f0);
+				pal |= (blue & 0x000f);
+			}
+			if (regno = 0)
+				pal |= 0x2000;
+			palette[regno] = pal;
+
+		} else if (info->var.bits_per_pixel = 8) {
 			red >>= 4;
 			green >>= 8;
 			blue >>= 12;
@@ -575,36 +613,35 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
 			pal = (red & 0x0f00);
 			pal |= (green & 0x00f0);
 			pal |= (blue & 0x000f);
-		}
-		if (regno = 0)
-			pal |= 0x2000;
-		palette[regno] = pal;
-
-	} else if (info->var.bits_per_pixel = 8) {
-		red >>= 4;
-		green >>= 8;
-		blue >>= 12;
-
-		pal = (red & 0x0f00);
-		pal |= (green & 0x00f0);
-		pal |= (blue & 0x000f);
 
-		if (palette[regno] != pal) {
-			update_hw = 1;
-			palette[regno] = pal;
+			if (palette[regno] != pal) {
+				update_hw = 1;
+				palette[regno] = pal;
+			}
 		}
-	} else if ((info->var.bits_per_pixel = 16) && regno < 16) {
-		red >>= (16 - info->var.red.length);
-		red <<= info->var.red.offset;
+		break;
+	}
 
-		green >>= (16 - info->var.green.length);
-		green <<= info->var.green.offset;
+	/* Truecolor has hardware independent palette */
+	if (info->fix.visual = FB_VISUAL_TRUECOLOR) {
+		u32 v;
 
-		blue >>= (16 - info->var.blue.length);
-		blue <<= info->var.blue.offset;
+		if (regno > 15)
+			return -EINVAL;
 
-		par->pseudo_palette[regno] = red | green | blue;
+		v = (red << info->var.red.offset) |
+			(green << info->var.green.offset) |
+			(blue << info->var.blue.offset);
 
+		switch (info->var.bits_per_pixel) {
+		case 16:
+			((u16 *) (info->pseudo_palette))[regno] = v;
+			break;
+		case 24:
+		case 32:
+			((u32 *) (info->pseudo_palette))[regno] = v;
+			break;
+		}
 		if (palette[0] != 0x4000) {
 			update_hw = 1;
 			palette[0] = 0x4000;
@@ -617,6 +654,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
 
 	return 0;
 }
+#undef CNVT_TOHW
 
 static void lcd_reset(struct da8xx_fb_par *par)
 {
@@ -824,6 +862,9 @@ static int fb_check_var(struct fb_var_screeninfo *var,
 {
 	int err = 0;
 
+	if ((var->bits_per_pixel > 16) && (lcd_revision = LCD_VERSION_1))
+		return -EINVAL;
+
 	switch (var->bits_per_pixel) {
 	case 1:
 	case 8:
@@ -859,6 +900,26 @@ static int fb_check_var(struct fb_var_screeninfo *var,
 		var->transp.length = 0;
 		var->nonstd = 0;
 		break;
+	case 24:
+		var->red.offset = 16;
+		var->red.length = 8;
+		var->green.offset = 8;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		var->nonstd = 0;
+		break;
+	case 32:
+		var->transp.offset = 24;
+		var->transp.length = 8;
+		var->red.offset = 16;
+		var->red.length = 8;
+		var->green.offset = 8;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		var->nonstd = 0;
+		break;
 	default:
 		err = -EINVAL;
 	}
-- 
1.7.1

RE: [PATCH v3] video: da8xx-fb: add 24bpp LCD configuration support

[Manjunathappa, Prakash]

Hi,

On Thu, Jul 19, 2012 at 21:07:46, Manjunathappa, Prakash wrote:
> LCD controller on am335x supports 24bpp raster configuration in addition
> to ones on da850. LCDC also supports 24bpp in unpacked format having
> ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
> component of the data.
> 
> Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
> Since v2:
> Fixed additional configurations for 24bpp support.
> Since v1:
> Simplified calculation of pseudopalette for FB_VISUAL_TRUECOLOR type.
> 
>  drivers/video/da8xx-fb.c |  127 ++++++++++++++++++++++++++++++++++------------
>  1 files changed, 94 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 47118c7..3d2d0d1 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -83,6 +83,8 @@
>  #define LCD_V2_LIDD_CLK_EN		BIT(1)
>  #define LCD_V2_CORE_CLK_EN		BIT(0)
>  #define LCD_V2_LPP_B10			26
> +#define LCD_V2_TFT_24BPP_MODE		BIT(25)
> +#define LCD_V2_TFT_24BPP_UNPACK		BIT(26)
>  
>  /* LCD Raster Timing 2 Register */
>  #define LCD_AC_BIAS_TRANSITIONS_PER_INT(x)	((x) << 16)
> @@ -153,7 +155,7 @@ struct da8xx_fb_par {
>  	unsigned int		dma_end;
>  	struct clk *lcdc_clk;
>  	int irq;
> -	unsigned short pseudo_palette[16];
> +	unsigned long pseudo_palette[16];

I am still not convinced as sizes of "unsigned long" and "unsigned int" are not
guaranteed to be 32bit across platforms and compilers, so planning to retain u32.

Florian Tobias Schandinat,
Can you please comment?

Here is the history:
http://marc.info/?l=linux-fbdev&m\x134259216714719&w=2

>  	unsigned int palette_sz;
>  	unsigned int pxl_clk;
>  	int blank;
> @@ -482,6 +484,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>  {
>  	u32 reg;
>  
> +	if ((bpp > 16) && (lcd_revision = LCD_VERSION_1))
> +		return -EINVAL;
> +
>  	/* Set the Panel Width */
>  	/* Pixels per line = (PPL + 1)*16 */
>  	if (lcd_revision = LCD_VERSION_1) {
> @@ -525,6 +530,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>  	reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8);
>  	if (raster_order)
>  		reg |= LCD_RASTER_ORDER;
> +
> +	if (bpp = 24)
> +		reg |= LCD_V2_TFT_24BPP_MODE;
> +	else if (bpp = 32)
> +		reg |= (LCD_V2_TFT_24BPP_MODE | LCD_V2_TFT_24BPP_UNPACK);
> +
>  	lcdc_write(reg, LCD_RASTER_CTRL_REG);
>  
>  	switch (bpp) {
> @@ -532,6 +543,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>  	case 2:
>  	case 4:
>  	case 16:
> +	case 24:
> +	case 32:
>  		par->palette_sz = 16 * 2;
>  		break;
>  
> @@ -546,6 +559,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>  	return 0;
>  }
>  
> +
> +#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)

since multiple FB drivers have re-defined this macro, I will move this to common place(linux/fb.h) and
convert it as inline function.

Thanks,
Prakash

>  static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
[...]

Re: [PATCH v3] video: da8xx-fb: add 24bpp LCD configuration support

[Florian Tobias Schandinat]

Hi,

On 07/23/2012 08:26 AM, Manjunathappa, Prakash wrote:
> Hi,
> 
> On Thu, Jul 19, 2012 at 21:07:46, Manjunathappa, Prakash wrote:
>> LCD controller on am335x supports 24bpp raster configuration in addition
>> to ones on da850. LCDC also supports 24bpp in unpacked format having
>> ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
>> component of the data.
>>
>> Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
>> Cc: Anatolij Gustschin <agust@denx.de>
>> ---
>> Since v2:
>> Fixed additional configurations for 24bpp support.
>> Since v1:
>> Simplified calculation of pseudopalette for FB_VISUAL_TRUECOLOR type.
>>
>>  drivers/video/da8xx-fb.c |  127 ++++++++++++++++++++++++++++++++++------------
>>  1 files changed, 94 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
>> index 47118c7..3d2d0d1 100644
>> --- a/drivers/video/da8xx-fb.c
>> +++ b/drivers/video/da8xx-fb.c
>> @@ -83,6 +83,8 @@
>>  #define LCD_V2_LIDD_CLK_EN		BIT(1)
>>  #define LCD_V2_CORE_CLK_EN		BIT(0)
>>  #define LCD_V2_LPP_B10			26
>> +#define LCD_V2_TFT_24BPP_MODE		BIT(25)
>> +#define LCD_V2_TFT_24BPP_UNPACK		BIT(26)
>>  
>>  /* LCD Raster Timing 2 Register */
>>  #define LCD_AC_BIAS_TRANSITIONS_PER_INT(x)	((x) << 16)
>> @@ -153,7 +155,7 @@ struct da8xx_fb_par {
>>  	unsigned int		dma_end;
>>  	struct clk *lcdc_clk;
>>  	int irq;
>> -	unsigned short pseudo_palette[16];
>> +	unsigned long pseudo_palette[16];
> 
> I am still not convinced as sizes of "unsigned long" and "unsigned int" are not
> guaranteed to be 32bit across platforms and compilers, so planning to retain u32.

Yes, if you want something that is always 32 bit you probably should use
u32, that is at least more obvious.
There are a few guarantees in C like
sizeof(short)<=sizeof(int)<=sizeof(long) and short at least being 16
bits and long at least 32 bits that any standard compliant compiler
should honor. And if you limit it to a specific platform/CPU even more
may be assured. But if you want to highlight that you always want to use
32bit u32 is the best choice.

> 
> Florian Tobias Schandinat,
> Can you please comment?

Best regards,

Florian Tobias Schandinat

> 
> Here is the history:
> http://marc.info/?l=linux-fbdev&m\x134259216714719&w=2
> 
>>  	unsigned int palette_sz;
>>  	unsigned int pxl_clk;
>>  	int blank;
>> @@ -482,6 +484,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>>  {
>>  	u32 reg;
>>  
>> +	if ((bpp > 16) && (lcd_revision = LCD_VERSION_1))
>> +		return -EINVAL;
>> +
>>  	/* Set the Panel Width */
>>  	/* Pixels per line = (PPL + 1)*16 */
>>  	if (lcd_revision = LCD_VERSION_1) {
>> @@ -525,6 +530,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>>  	reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8);
>>  	if (raster_order)
>>  		reg |= LCD_RASTER_ORDER;
>> +
>> +	if (bpp = 24)
>> +		reg |= LCD_V2_TFT_24BPP_MODE;
>> +	else if (bpp = 32)
>> +		reg |= (LCD_V2_TFT_24BPP_MODE | LCD_V2_TFT_24BPP_UNPACK);
>> +
>>  	lcdc_write(reg, LCD_RASTER_CTRL_REG);
>>  
>>  	switch (bpp) {
>> @@ -532,6 +543,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>>  	case 2:
>>  	case 4:
>>  	case 16:
>> +	case 24:
>> +	case 32:
>>  		par->palette_sz = 16 * 2;
>>  		break;
>>  
>> @@ -546,6 +559,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
>>  	return 0;
>>  }
>>  
>> +
>> +#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)
> 
> since multiple FB drivers have re-defined this macro, I will move this to common place(linux/fb.h) and
> convert it as inline function.
> 
> Thanks,
> Prakash
> 
>>  static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> [...]
> 

RE: [PATCH v3] video: da8xx-fb: add 24bpp LCD configuration support

[Manjunathappa, Prakash]

Hi,

On Mon, Jul 23, 2012 at 13:56:42, Manjunathappa, Prakash wrote:
> Hi,
> 
> On Thu, Jul 19, 2012 at 21:07:46, Manjunathappa, Prakash wrote:
> > LCD controller on am335x supports 24bpp raster configuration in addition
> > to ones on da850. LCDC also supports 24bpp in unpacked format having
> > ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
> > component of the data.
> > 
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> > Cc: Anatolij Gustschin <agust@denx.de>
> > ---
[...]
> > @@ -546,6 +559,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> >  	return 0;
> >  }
> >  
> > +
> > +#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)
> 
> since multiple FB drivers have re-defined this macro, I will move this to common place(linux/fb.h) and
> convert it as inline function.
> 

This macro has lot of variations in various hardware and as I do not have bigger picture to address them
all, so for now I will be dropping the attempt of moving it to common place. This will be taken up later
point of time.

Thanks,
Prakash
> 
> >  static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> [...]
>