Patchwork [1/2] drm/exynos: mixer: simplify mixer_cfg_rgb_fmt()

login
register
mail settings
Submitter Tobias Jakobi
Date March 3, 2017, 1:40 p.m.
Message ID <1488548452-4011-1-git-send-email-tjakobi@math.uni-bielefeld.de>
Download mbox | patch
Permalink /patch/9602839/
State New
Headers show

Comments

Tobias Jakobi - March 3, 2017, 1:40 p.m.
Convert if-statements to switch statement. Removes
duplicated code.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)
Andrzej Hajda - March 6, 2017, 8:22 a.m.
Hi Tobias,

On 03.03.2017 14:40, Tobias Jakobi wrote:
> Convert if-statements to switch statement. Removes
> duplicated code.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 72143ac..41d0c36 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -382,29 +382,14 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>  	struct mixer_resources *res = &ctx->mixer_res;
>  	u32 val;
>  
> -	if (height == 480) {
> +	switch (height) {
> +	case 480:
> +	case 576:
>  		val = MXR_CFG_RGB601_0_255;
> -	} else if (height == 576) {
> -		val = MXR_CFG_RGB601_0_255;
> -	} else if (height == 720) {
> -		val = MXR_CFG_RGB709_16_235;
> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
> -				(1 << 30) | (94 << 20) | (314 << 10) |
> -				(32 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
> -				(972 << 20) | (851 << 10) | (225 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
> -				(225 << 20) | (820 << 10) | (1004 << 0));
> -	} else if (height == 1080) {
> -		val = MXR_CFG_RGB709_16_235;
> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
> -				(1 << 30) | (94 << 20) | (314 << 10) |
> -				(32 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
> -				(972 << 20) | (851 << 10) | (225 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
> -				(225 << 20) | (820 << 10) | (1004 << 0));
> -	} else {
> +		break;
> +	case 720:
> +	case 1080:
> +	default:
>  		val = MXR_CFG_RGB709_16_235;
>  		mixer_reg_write(res, MXR_CM_COEFF_Y,
>  				(1 << 30) | (94 << 20) | (314 << 10) |
> @@ -413,6 +398,7 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>  				(972 << 20) | (851 << 10) | (225 << 0));
>  		mixer_reg_write(res, MXR_CM_COEFF_CR,
>  				(225 << 20) | (820 << 10) | (1004 << 0));
> +		break;
>  	}

Good change.
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

One small nitpick.
As I see this conditional/switch is to decide about BT standard
depending on the height. The similar problem is addressed in exynos-gsc
patches [1].
It would be good to have the same criteria to distinguish SD/HD mode. I
think ((height > 576) || (width > 720)) is more generic, in this case
even (height > 576) looks better, but as this changes logic of the code
it could be in separate patch.

[1]: https://lkml.org/lkml/2017/2/21/864

Regards
Andrzej

>  
>  	mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
Tobias Jakobi - March 6, 2017, 9:43 a.m.
Hello Andrzej,

Andrzej Hajda wrote:
> Hi Tobias,
> 
> On 03.03.2017 14:40, Tobias Jakobi wrote:
>> Convert if-statements to switch statement. Removes
>> duplicated code.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++----------------------
>>  1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 72143ac..41d0c36 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -382,29 +382,14 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>  	struct mixer_resources *res = &ctx->mixer_res;
>>  	u32 val;
>>  
>> -	if (height == 480) {
>> +	switch (height) {
>> +	case 480:
>> +	case 576:
>>  		val = MXR_CFG_RGB601_0_255;
>> -	} else if (height == 576) {
>> -		val = MXR_CFG_RGB601_0_255;
>> -	} else if (height == 720) {
>> -		val = MXR_CFG_RGB709_16_235;
>> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
>> -				(1 << 30) | (94 << 20) | (314 << 10) |
>> -				(32 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
>> -				(972 << 20) | (851 << 10) | (225 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
>> -				(225 << 20) | (820 << 10) | (1004 << 0));
>> -	} else if (height == 1080) {
>> -		val = MXR_CFG_RGB709_16_235;
>> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
>> -				(1 << 30) | (94 << 20) | (314 << 10) |
>> -				(32 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
>> -				(972 << 20) | (851 << 10) | (225 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
>> -				(225 << 20) | (820 << 10) | (1004 << 0));
>> -	} else {
>> +		break;
>> +	case 720:
>> +	case 1080:
>> +	default:
>>  		val = MXR_CFG_RGB709_16_235;
>>  		mixer_reg_write(res, MXR_CM_COEFF_Y,
>>  				(1 << 30) | (94 << 20) | (314 << 10) |
>> @@ -413,6 +398,7 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>  				(972 << 20) | (851 << 10) | (225 << 0));
>>  		mixer_reg_write(res, MXR_CM_COEFF_CR,
>>  				(225 << 20) | (820 << 10) | (1004 << 0));
>> +		break;
>>  	}
> 
> Good change.
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Thanks for the review.

> 
> One small nitpick.
> As I see this conditional/switch is to decide about BT standard
> depending on the height. The similar problem is addressed in exynos-gsc
> patches [1].
> It would be good to have the same criteria to distinguish SD/HD mode. I
> think ((height > 576) || (width > 720)) is more generic, in this case
> even (height > 576) looks better, but as this changes logic of the code
> it could be in separate patch.
I'm not submitting functional changes anymore. You probably still
remember how that ended up last time. It's just not worth my time, sorry.

With best wishes,
Tobias


> [1]: https://lkml.org/lkml/2017/2/21/864
> 
> Regards
> Andrzej
> 
>>  
>>  	mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
> 
>

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 72143ac..41d0c36 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -382,29 +382,14 @@  static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
 	struct mixer_resources *res = &ctx->mixer_res;
 	u32 val;
 
-	if (height == 480) {
+	switch (height) {
+	case 480:
+	case 576:
 		val = MXR_CFG_RGB601_0_255;
-	} else if (height == 576) {
-		val = MXR_CFG_RGB601_0_255;
-	} else if (height == 720) {
-		val = MXR_CFG_RGB709_16_235;
-		mixer_reg_write(res, MXR_CM_COEFF_Y,
-				(1 << 30) | (94 << 20) | (314 << 10) |
-				(32 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CB,
-				(972 << 20) | (851 << 10) | (225 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CR,
-				(225 << 20) | (820 << 10) | (1004 << 0));
-	} else if (height == 1080) {
-		val = MXR_CFG_RGB709_16_235;
-		mixer_reg_write(res, MXR_CM_COEFF_Y,
-				(1 << 30) | (94 << 20) | (314 << 10) |
-				(32 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CB,
-				(972 << 20) | (851 << 10) | (225 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CR,
-				(225 << 20) | (820 << 10) | (1004 << 0));
-	} else {
+		break;
+	case 720:
+	case 1080:
+	default:
 		val = MXR_CFG_RGB709_16_235;
 		mixer_reg_write(res, MXR_CM_COEFF_Y,
 				(1 << 30) | (94 << 20) | (314 << 10) |
@@ -413,6 +398,7 @@  static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
 				(972 << 20) | (851 << 10) | (225 << 0));
 		mixer_reg_write(res, MXR_CM_COEFF_CR,
 				(225 << 20) | (820 << 10) | (1004 << 0));
+		break;
 	}
 
 	mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);