| Message ID | 1488548452-4011-1-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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 --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);
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(-)