Re: [PATCH] drm/exynos: set the active aspect ratio as per mode

[Tomasz Figa <t.figa@samsung.com>]

Hi Shirish,

On 10.03.2014 04:44, Shirish S wrote:
> now that the drm_display_mode also provides aspect
> ratio for all resolutions, this patch adds its usage
> to set the active aspect ratio of AVI info frame
> packets as per CEA-861-D standard's Table 9.
>
> This is also needed to abide by the 7-27
> compliance test of HDMI.
>
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> ---
>   drivers/gpu/drm/exynos/exynos_hdmi.c |   33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index c021ddc..8aca52a 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -53,8 +53,6 @@
>   /* AVI header and aspect ratio */
>   #define HDMI_AVI_VERSION		0x02
>   #define HDMI_AVI_LENGTH		0x0D
> -#define AVI_PIC_ASPECT_RATIO_16_9	(2 << 4)
> -#define AVI_SAME_AS_PIC_ASPECT_RATIO	8
>
>   /* AUI header info */
>   #define HDMI_AUI_VERSION	0x01
> @@ -65,6 +63,12 @@ enum hdmi_type {
>   	HDMI_TYPE14,
>   };
>
> +enum active_aspect_ratio {
> +	AVI_SAME_AS_PIC_ASPECT_RATIO = 8,
> +	AVI_4_3_Center_RATIO,
> +	AVI_16_9_Center_RATIO,
> +};
> +

CodingStyle: Please define these using preprocessor macros, since they 
are bitfield values. Also coding style implies using uppercase for 
constants.

>   struct hdmi_resources {
>   	struct clk			*hdmi;
>   	struct clk			*sclk_hdmi;
> @@ -162,6 +166,7 @@ struct hdmi_v14_conf {
>   struct hdmi_conf_regs {
>   	int pixel_clock;
>   	int cea_video_id;
> +	enum hdmi_picture_aspect aspect_ratio;
>   	union {
>   		struct hdmi_v13_conf v13_conf;
>   		struct hdmi_v14_conf v14_conf;
> @@ -668,7 +673,6 @@ static void hdmi_reg_infoframe(struct hdmi_context *hdata,
>   {
>   	u32 hdr_sum;
>   	u8 chksum;
> -	u32 aspect_ratio;
>   	u32 mod;
>   	u32 vic;
>
> @@ -697,10 +701,25 @@ static void hdmi_reg_infoframe(struct hdmi_context *hdata,
>   			AVI_ACTIVE_FORMAT_VALID |
>   			AVI_UNDERSCANNED_DISPLAY_VALID);
>
> -		aspect_ratio = AVI_PIC_ASPECT_RATIO_16_9;
> -
> -		hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(2), aspect_ratio |
> +		/*
> +		 * Set the aspect ratio as per the mode, mentioned in
> +		 * Table 9 AVI InfoFrame Data Byte 2 of CEA-861-D Standard
> +		 */
> +		switch (hdata->mode_conf.aspect_ratio) {
> +		case HDMI_PICTURE_ASPECT_4_3:
> +			hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(2), aspect_ratio |
> +				AVI_4_3_Center_RATIO);

CodingStyle: Please keep wrapped function calls aligned using tabs at 
least to the opening parenthesis.

> +			break;
> +		case HDMI_PICTURE_ASPECT_16_9:
> +			hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(2), aspect_ratio |
> +				AVI_16_9_Center_RATIO);

Ditto.

> +			break;
> +		case HDMI_PICTURE_ASPECT_NONE:
> +		default:
> +			hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(2), aspect_ratio |
>   				AVI_SAME_AS_PIC_ASPECT_RATIO);

Ditto.

Best regards,
Tomasz