[PATCH 2/7] drm/exynos: fimd: add fimd_channel_win() to clean up code

[Inki Dae]

On 2014년 10월 01일 15:19, YoungJun Cho wrote:
> The ENWIN_F in WINCON# register and C#_EN_Fs in SHADOWCON register
> should be always matched together, so adds fimd_channel_win()
> to clean up code.
> And this fimd_channel_win() should be called before unprotecting
> window in fimd_win_commit().

Sorry for late. below is my comment.

> 
> Signed-off-by: YoungJun Cho <yj44.cho at samsung.com>
> Acked-by: Inki Dae <inki.dae at samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park at samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 62 ++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 8b31b7e..b2f6007 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -214,6 +214,33 @@ static void fimd_wait_for_vblank(struct exynos_drm_manager *mgr)
>  		DRM_DEBUG_KMS("vblank wait timed out.\n");
>  }
>  
> +static void fimd_channel_win(struct fimd_context *ctx, int win, bool enable)
> +{
> +	u32 val;
> +
> +	/* for DMA output */
> +	val = readl(ctx->regs + WINCON(win));
> +
> +	if (enable)
> +		val |= WINCONx_ENWIN;
> +	else
> +		val &= ~WINCONx_ENWIN;
> +
> +	writel(val, ctx->regs + WINCON(win));
> +
> +	/* for shadow channel */
> +	if (ctx->driver_data->has_shadowcon) {
> +		val = readl(ctx->regs + SHADOWCON);
> +
> +		if (enable)
> +			val |= SHADOWCON_CHx_ENABLE(win);
> +		else
> +			val &= ~SHADOWCON_CHx_ENABLE(win);
> +
> +		writel(val, ctx->regs + SHADOWCON);
> +	}
> +}


This function includes two functionalities. One is controlling DMA
output. Other is controlling shadow channel. How about using separated
two functions instead? And let's call shadowcon control function only if
has_shadowcon is 1.

Thanks,
Inki Dae


> +
>  static void fimd_clear_channel(struct exynos_drm_manager *mgr)
>  {
>  	struct fimd_context *ctx = mgr->ctx;
> @@ -226,16 +253,7 @@ static void fimd_clear_channel(struct exynos_drm_manager *mgr)
>  		u32 val = readl(ctx->regs + WINCON(win));
>  
>  		if (val & WINCONx_ENWIN) {
> -			/* wincon */
> -			val &= ~WINCONx_ENWIN;
> -			writel(val, ctx->regs + WINCON(win));
> -
> -			/* unprotect windows */
> -			if (ctx->driver_data->has_shadowcon) {
> -				val = readl(ctx->regs + SHADOWCON);
> -				val &= ~SHADOWCON_CHx_ENABLE(win);
> -				writel(val, ctx->regs + SHADOWCON);
> -			}
> +			fimd_channel_win(ctx, win, false);
>  			ch_enabled = 1;
>  		}
>  	}
> @@ -730,20 +748,11 @@ static void fimd_win_commit(struct exynos_drm_manager *mgr, int zpos)
>  	if (win != 0)
>  		fimd_win_set_colkey(ctx, win);
>  
> -	/* wincon */
> -	val = readl(ctx->regs + WINCON(win));
> -	val |= WINCONx_ENWIN;
> -	writel(val, ctx->regs + WINCON(win));
> +	fimd_channel_win(ctx, win, true);
>  
>  	/* Enable DMA channel and unprotect windows */
>  	fimd_shadow_protect_win(ctx, win, false);
>  
> -	if (ctx->driver_data->has_shadowcon) {
> -		val = readl(ctx->regs + SHADOWCON);
> -		val |= SHADOWCON_CHx_ENABLE(win);
> -		writel(val, ctx->regs + SHADOWCON);
> -	}
> -
>  	win_data->enabled = true;
>  
>  	if (ctx->i80_if)
> @@ -755,7 +764,6 @@ static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos)
>  	struct fimd_context *ctx = mgr->ctx;
>  	struct fimd_win_data *win_data;
>  	int win = zpos;
> -	u32 val;
>  
>  	if (win == DEFAULT_ZPOS)
>  		win = ctx->default_win;
> @@ -774,17 +782,7 @@ static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos)
>  	/* protect windows */
>  	fimd_shadow_protect_win(ctx, win, true);
>  
> -	/* wincon */
> -	val = readl(ctx->regs + WINCON(win));
> -	val &= ~WINCONx_ENWIN;
> -	writel(val, ctx->regs + WINCON(win));
> -
> -	/* unprotect windows */
> -	if (ctx->driver_data->has_shadowcon) {
> -		val = readl(ctx->regs + SHADOWCON);
> -		val &= ~SHADOWCON_CHx_ENABLE(win);
> -		writel(val, ctx->regs + SHADOWCON);
> -	}
> +	fimd_channel_win(ctx, win, false);
>  
>  	fimd_shadow_protect_win(ctx, win, false);
>  
>