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

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

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

On 30/07/13 21:26, 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(-)
> 
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 8d73730..b25b810 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -243,6 +243,11 @@ static struct fb_videomode known_lcd_panels[] = {
>  	},
>  };
>  
> +static inline bool da8xx_fb_is_raster_enabled(void)
> +{
> +	return !!(lcdc_read(LCD_RASTER_CTRL_REG) & LCD_RASTER_ENABLE);
> +}

See Documentation/CodingStyle about inline.

I think, generally, it's better not to use inline at all in normal
functions. Let the compiler decide. Even more so with funcs like
da8xx_fb_is_raster_enabled(), which I guess is only used rarely.

There are some inlines added in other patches in the series also.

>  /* Enable the Raster Engine of the LCD Controller */
>  static inline void lcd_enable_raster(void)
>  {
> @@ -665,9 +670,6 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
>  
>  static void da8xx_fb_lcd_reset(void)
>  {
> -	/* Disable the Raster if previously Enabled */
> -	lcd_disable_raster(false);
> -
>  	/* DMA has to be disabled */
>  	lcdc_write(0, LCD_DMA_CTRL_REG);
>  	lcdc_write(0, LCD_RASTER_CTRL_REG);
> @@ -720,8 +722,6 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
>  	u32 bpp;
>  	int ret = 0;
>  
> -	da8xx_fb_lcd_reset();
> -
>  	da8xx_fb_calc_config_clk_divider(par, panel);
>  
>  	if (panel->sync & FB_SYNC_CLK_INVERT)
> @@ -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);

This looks odd. If raster is enabled, you disable it. And if raster is
disabled, you disable it.

 Tomi



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