Hi,
On Sat, Jul 25, 2009 at 02:53:36, Andrew Morton wrote:
> On Wed, 22 Jul 2009 00:47:00 -0400
> Sudhakar Rajashekhara <[email protected]> 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 <[email protected]>
> > Signed-off-by: Pavel Kiryukhin <[email protected]>
> > Signed-off-by: Steve Chen <[email protected]>
> > Acked-by: Krzysztof Helt <[email protected]>
> > ---
> > 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.
> ---
> 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.
> ---
> 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
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
