diff mbox

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

Message ID 1488548452-4011-1-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive)
State New, archived
Headers show

Commit Message

Tobias Jakobi March 3, 2017, 1:40 p.m. UTC 
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(-)

Comments

Andrzej Hajda March 6, 2017, 8:22 a.m. UTC | #1 
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. UTC | #2 
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);
> 
>
diff mbox

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);