Re: [PATCH 10/23] video: da8xx-fb: fb_set_par support

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

On 25/06/13 17:22, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
> 
> fb_set_par helps in runtime configuration of lcd controller like
> changing resolution, pixel clock etc. (eg. using fbset utility)
> 
> Reconfigure lcd controller based on information passed by framework.
> Enable raster back if it was already enabled.
> 
> As fb_set_par would get invoked indirectly from probe via fb_set_var,
> remove existing lcdc initialization in probe and do lcdc reset in
> probe so that reset happens only at the begining.
> 
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
>  drivers/video/da8xx-fb.c |   60 +++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 49 insertions(+), 11 deletions(-)
> 

<snip>

> @@ -1201,9 +1201,52 @@ static int da8xx_pan_display(struct fb_var_screeninfo *var,
>  	return ret;
>  }
>  
> +static int da8xxfb_set_par(struct fb_info *info)
> +{
> +	struct da8xx_fb_par *par = info->par;
> +	int ret;
> +	bool raster = da8xx_fb_is_raster_enabled();
> +
> +	if (raster)
> +		lcd_disable_raster(true);
> +	else
> +		lcd_disable_raster(false);

Not actually part of this patch, but: that kind of functions are quite
unclear. What does disable(true) mean? You have to read the function to
understand the call.

I think it'd be much better to take the wait-for-frame-done code out
from the lcd_disable_raster function, and call the wait function
explicitly here in the if (raster) block.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]