Re: [PATCH v4] davinci: fb: Frame Buffer driver for TI DA8xx/OMAP-L1xx
From: Sudhakar Rajashekhara <hidden>
Date: 2009-07-27 12:55:20
Hi, On Sat, Jul 25, 2009 at 02:53:36, Andrew Morton wrote:
On Wed, 22 Jul 2009 00:47:00 -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/. Signed-off-by: Sudhakar Rajashekhara <redacted> Signed-off-by: Pavel Kiryukhin <redacted> Signed-off-by: Steve Chen <redacted> Acked-by: Krzysztof Helt <redacted> --- 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:
I missed documenting all the modifications. I'll take care of it.
quoted hunk ↗ jump to hunk
--- 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?
Yes. As per your review comment last time I have changed the return value in case of error from "copy_from_user" to -EFAULT. Changing the return value of non-implemented ioctls to -ENOTTY seems more appropriate as per the man page of ioctl.
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?Right. I'll submit an updated version of this patch with all these changes.
quoted hunk ↗ jump to hunk
--- 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; }_
Thanks, Sudhakar ------------------------------------------------------------------------------