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

Hi Sergei,

Thanks for reviewing the patch.

On Wed, Aug 08, 2012 at 21:44:55, Sergei Shtylyov wrote:

Hello.

On 08-08-2012 19:35, Manjunathappa, Prakash wrote:

quoted

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.

quoted

Signed-off-by: Manjunathappa, Prakash <redacted>
Cc: Anatolij Gustschin <agust@denx.de>

[...]

quoted

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7ae9d53..1abcfa9 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c

[...]

quoted

@@ -499,6 +501,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))

    Parens around operands of && not necessary.

I agree it is not necessary, isn't it more readable?

quoted

@@ -542,6 +547,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);
+

    This asks to be a *switch* statement.

I will move it to *switch* statement below.

quoted

  	lcdc_write(reg, LCD_RASTER_CTRL_REG);

  	switch (bpp) {

@@ -549,6 +560,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;

@@ -578,13 +591,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))

    Parens around operands of && not necessary.

Same as above, can I retain it for purpose of readability?

quoted

+	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 -EINVAL;
+
+			if (info->var.grayscale) {
+				pal = regno;
+			} else {
+				red >>= 4;
+				green >>= 8;
+				blue >>= 12;
+
+				pal = (red & 0x0f00);
+				pal |= (green & 0x00f0);
+				pal |= (blue & 0x000f);

    Parens not needed.

Will remove parens around.

quoted

+			}
+			if (regno = 0)
+				pal |= 0x2000;
+			palette[regno] = pal;
+
+		} else if (info->var.bits_per_pixel = 8) {

    This asks to be a *switch* statement.

Agreed, I will replace with *switch* statement.

Thanks,
Prakash

quoted

@@ -842,6 +877,9 @@ static int fb_check_var(struct fb_var_screeninfo *var,
  {
  	int err = 0;

[...]