Re: [Linux-fbdev-devel] [PATCH v4] davinci: fb: Frame Buffer driver for TI DA8xx/OMAP-L1xx

[Andrew M. <ak...@li...>]

On Wed, 22 Jul 2009 00:47:00 -0400
Sudhakar Rajashekhara <sud...@ti...> wrote:

> 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 <sud...@ti...>
> Signed-off-by: Pavel Kiryukhin <pki...@ru...>
> Signed-off-by: Steve Chen <sc...@mv...>
> Acked-by: Krzysztof Helt <krz...@wp...>
> ---
>  This patch applies to Linus's Kernel tree.
> 
>  Since the previous version, return values in ioctl()
>  function have been modified.

That wasn't the only change in version 4:

--- a/drivers/video/da8xx-fb.c~davinci-fb-frame-buffer-driver-for-ti-da8xx-omap-l1xx-v4
+++ a/drivers/video/da8xx-fb.c
@@ -201,14 +201,13 @@ static int lcd_disable_raster(struct da8
 		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)
+			ret = -ETIMEDOUT;
 	}
 
-	if (ret < 0)
-		return ret;
-	if (ret == 0)
-		return -ETIMEDOUT;
-
-	return 0;
+	return ret;
 }
 
 static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
@@ -634,11 +633,11 @@ static int fb_ioctl(struct fb_info *info
 	case FBIPUT_BRIGHTNESS:
 	case FBIGET_COLOR:
 	case FBIPUT_COLOR:
-		return -EINVAL;
+		return -ENOTTY;
 	case FBIPUT_HSYNC:
 		if (copy_from_user(&sync_arg, (char *)arg,
 				sizeof(struct lcd_sync_arg)))
-			return -EINVAL;
+			return -EFAULT;
 		lcd_cfg_horizontal_sync(sync_arg.back_porch,
 					sync_arg.pulse_width,
 					sync_arg.front_porch);
@@ -646,7 +645,7 @@ static int fb_ioctl(struct fb_info *info
 	case FBIPUT_VSYNC:
 		if (copy_from_user(&sync_arg, (char *)arg,
 				sizeof(struct lcd_sync_arg)))
-			return -EINVAL;
+			return -EFAULT;
 		lcd_cfg_vertical_sync(sync_arg.back_porch,
 					sync_arg.pulse_width,
 					sync_arg.front_porch);
_


Was that intentional?


The change to lcd_disable_raster() is a bit odd:

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)
			ret = -ETIMEDOUT;
	}

	return ret;
}


We can do this, yes?

--- a/drivers/video/da8xx-fb.c~davinci-fb-frame-buffer-driver-for-ti-da8xx-omap-l1xx-v4-cleanup
+++ a/drivers/video/da8xx-fb.c
@@ -201,8 +201,6 @@ static int lcd_disable_raster(struct da8
 		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)
 			ret = -ETIMEDOUT;
 	}
_