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

[Sudhakar R. <sud...@ti...>]

Hi,

On Sat, Jul 25, 2009 at 02:53:36, Andrew Morton wrote:
> 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:
> 

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