Re: [PATCH v3] Frame Buffer driver for TI DA8xx/OMAP-L1xx

Krzysztof,

-----Original Message-----
From: Krzysztof Helt [mailto:krzysztof.h1@poczta.fm]
Sent: Friday, June 26, 2009 10:49 AM
To: Rajashekhara, Sudhakar
Cc: linux-fbdev-devel@lists.sourceforge.net; davinci-linux-open-
source@linux.davincidsp.com; Steve Chen; Pavel Kiryukhin
Subject: Re: [Linux-fbdev-devel] [PATCH v3] Frame Buffer driver for TI
DA8xx/OMAP-L1xx

On Wed, 24 Jun 2009 06:58:21 -0400
"Rajashekhara, Sudhakar" [off-list ref] wrote:

quoted

Adds LCD controller (LCDC) driver for TI's DA8xx/OMAP-L1xx architecture.
LCDC specifications can be found at http://www.ti.com/litv/pdf/sprufm0a.

LCDC on DA8xx consists of two independent controllers, the Raster Controller
and the LCD Interface Display Driver (LIDD) controller. LIDD further supports
character and graphic displays.

This patch adds support for the graphic display (Sharp LQ035Q3DG01) found on
the DA830 based EVM. The EVM details can be found at:
http://support.spectrumdigital.com/boards/dskda830/revc/.

Signed-off-by: Sudhakar Rajashekhara <redacted>
Signed-off-by: Pavel Kiryukhin <redacted>
Signed-off-by: Steve Chen <redacted>
---
 Since the previous version, error paths in probing function has been
 corected, removed set_par() function and added code in fb_setcolreg()
 function for TRUECOLOR mode.

 drivers/video/Kconfig    |   11 +
 drivers/video/Makefile   |    1 +
 drivers/video/da8xx-fb.c |  887 ++++++++++++++++++++++++++++++++++++++++++++++
 include/video/da8xx-fb.h |  106 ++++++
 4 files changed, 1005 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/da8xx-fb.c
 create mode 100644 include/video/da8xx-fb.h

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
new file mode 100644
index 0000000..a41ae62
--- /dev/null
+++ b/drivers/video/da8xx-fb.c

@@ -0,0 +1,887 @@

(fast forward)

quoted

+
+static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
+			      unsigned blue, unsigned transp,
+			      struct fb_info *info)
+{
+	struct da8xx_fb_par *par = info->par;
+	unsigned short *palette = (unsigned short *)par->v_palette_base;
+	u_short pal;
+
+	if (regno > 255)
+		return 1;
+
+	if (info->fix.visual == FB_VISUAL_DIRECTCOLOR)
+		return 1;
+
+	if (info->var.bits_per_pixel == 8) {
+		red >>= 8;
+		green >>= 8;
+		blue >>= 8;

This function is still incorrect. Each color component value passed to the
function is 16 bit.

I assume you have 16-bit word for each palette entry and you want
to pack 4-bit colors like this 0RGB. The shift should be

red >>= 12;

so you reduced the component length to 4 bits then

red <<= 8;

so it is put on correct position in the palette entry.
You fold these two shifts so it is

red >>= 4;

The same logic should be used for all color components.

quoted

+
+		pal = (red & 0x0f00);
+		pal |= (green & 0x00f0);
+		pal |= (blue & 0x000f);
+
+		palette[regno] = pal;
+
+	} else if ((info->var.bits_per_pixel == 16) && regno < 16) {
+		memset(palette, 0, 16);

I do not understand what is achieved by zeroing the palette area.

This is what you should use:

red >>= (16 - info->var.red.len)
red <<= info->var.red.offset;

The same logic should be used for all color components.
Then pack them into palette register:

par->pseudo_palette[ regno ] = red | green | blue;

The true color modes do not use the palette entry. I assume
that the value below forces controller to use true color mode.

quoted

+		palette[0] = 0x4000;
+	}
+
+	return 0;
+}
+

Regards,
Krzysztof

I'll test the driver with the modifications suggested by you
and will submit the updated version of the patch tomorrow.

Thanks, Sudhakar



------------------------------------------------------------------------------