Thanks for your comments...

On Thu, Jul 16, 2009 at 02:30:12, Andrew Morton wrote:

On Wed, 15 Jul 2009 03:57:34 -0400
Sudhakar Rajashekhara [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/.


...

--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig

@@ -2038,6 +2038,17 @@ config FB_SH7760
 	  and 8, 15 or 16 bpp color; 90 degrees clockwise display rotation for
 	  panels <= 320 pixel horizontal resolution.
 
+config FB_DA8XX
+        tristate "DA8xx/OMAP-L1xx Framebuffer support"
+        depends on FB && ARCH_DAVINCI_DA8XX
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	---help---
+          This is the frame buffer device driver for the TI LCD controller
+	  found on DA8xx/OMAP-L1xx SoCs.
+          If unsure, say N.

Leading whitespace is all mucked up there - I fixed it.

Thanks. I'll take care of it in future.

quoted

 config FB_VIRTUAL
 	tristate "Virtual Frame Buffer support (ONLY FOR TESTING!)"
 	depends on FB

...

+/* Disable the Raster Engine of the LCD Controller */
+static int lcd_disable_raster(struct da8xx_fb_par *par)
+{
+	int ret = 0;
+	u32 reg;
+
+	reg = lcdc_read(LCD_RASTER_CTRL_REG);
+	if (reg & LCD_RASTER_ENABLE) {
+		lcdc_write(reg & ~LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
+		ret = wait_event_interruptible_timeout(par->da8xx_wq,
+						!lcdc_read(LCD_STAT_REG) &
+						LCD_END_OF_FRAME0, WSI_TIMEOUT);
+	}
+
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		return -ETIMEDOUT;
+
+	return 0;
+}

This looks wrongish.  If LCD_RASTER_ENABLE is not set, it will return
-ETIMEDOUT without ever having waited for anything.

I'll correct this.

quoted

...

+static int fb_ioctl(struct fb_info *info, unsigned int cmd,
+			  unsigned long arg)
+{
+	struct lcd_sync_arg sync_arg;
+
+	switch (cmd) {
+	case FBIOGET_CONTRAST:
+	case FBIOPUT_CONTRAST:
+	case FBIGET_BRIGHTNESS:
+	case FBIPUT_BRIGHTNESS:
+	case FBIGET_COLOR:
+	case FBIPUT_COLOR:
+		return -EINVAL;

As per the man page of ioctl, I think the above should be -ENOTTY.
I'll change it to -ENOTTY.

quoted

+	case FBIPUT_HSYNC:
+		if (copy_from_user(&sync_arg, (char *)arg,
+				sizeof(struct lcd_sync_arg)))
+			return -EINVAL;

-EFAULT?

Agree, I'll change it.

quoted

+		lcd_cfg_horizontal_sync(sync_arg.back_porch,
+					sync_arg.pulse_width,
+					sync_arg.front_porch);
+		break;
+	case FBIPUT_VSYNC:
+		if (copy_from_user(&sync_arg, (char *)arg,
+				sizeof(struct lcd_sync_arg)))
+			return -EINVAL;

-EFAULT?

I'll change it here as well.

quoted

+		lcd_cfg_vertical_sync(sync_arg.back_porch,
+					sync_arg.pulse_width,
+					sync_arg.front_porch);
+		break;
+	default:
+		return -EINVAL;

-ENOTTY?  (Maybe not - I forget)

I think -EINVAL is the proper return value here. I'll retain it.

I'll submit an updated version of this patch with these changes.

Regards, Sudhakar



------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time, 
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize  
details at: http://p.sf.net/sfu/Challenge