Re: [PATCH 4/5] drm: Add definitions for DP compliance Video pattern tests

From: Jani Nikula <jani.nikula@linux.intel.com>
To: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: Manasi Navare <manasi.d.navare@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 4/5] drm: Add definitions for DP compliance Video pattern tests
Date: Wed, 23 Nov 2016 15:27:45 +0200	[thread overview]
Message-ID: <87a8cqwcku.fsf@intel.com> (raw)
In-Reply-To: <1479850766-32748-5-git-send-email-manasi.d.navare@intel.com>

On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  include/drm/drm_dp_helper.h | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 55bbeb0..f2c04ec 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -415,7 +415,21 @@
>  
>  #define DP_TEST_LANE_COUNT		    0x220
>  
> -#define DP_TEST_PATTERN			    0x221
> +#define DP_TEST_PATTERN				0x221

Unnecessary indentation change. Please observe the code around you. It
may not have the best indentation style, but stick with what's there
for all the other DPCD address definitions.

> +#define DP_COLOR_RAMP				(1 << 0)

See how all the other address *content* definitions have a space between
"#" and "define". I'm not saying I like it, but it's uniform in the
file.

While at it, why not add all of the defines for TEST_PATTERN. And
observe how they are not bit patterns, so (1 << 0) should be just 1.

DP_NO_TEST_PATTERN
DP_COLOR_RAMPS
DP_BLACK_AND_WHITE_VERTICAL_LINES
DP_COLOR_SQUARE

> +#define DP_TEST_H_WIDTH				0x22E

Note that across the file, almost all addrses defines have a blank line
between them, to separate content definitions from other addresses.

> +#define DP_TEST_V_HEIGHT			0x230

I guess I'd do

#define DP_TEST_V_HEIGHT_HI			0x230
#define DP_TEST_V_HEIGHT_LO			0x231

You don't actually have to *use* both definitions if you can write both
in one go, but this saves the trouble of checking the DP spec when it's
documented as #defines here.

> +#define DP_TEST_MISC				0x232
> +#define DP_VIDEO_PATTERN_RGB_FORMAT		0

The convention is to shift the 0 too so it's obvious where it
fits. _MASK goes before the values. Please add all the values.

> +#define DP_TEST_COLOR_FORMAT_MASK		0x6
> +#define DP_TEST_DYNAMIC_RANGE_MASK		(1 << 3)

And the values?

> +#define DP_VIDEO_PATTERN_VESA			0
> +#define DP_TEST_BIT_DEPTH_MASK			0x00E0
> +#define DP_TEST_BIT_DEPTH_6			0

> +#define DP_TEST_BIT_DEPTH_8			1

Just add all of the values at once.

> +#define DP_TEST_MISC_BIT_1			1
> +#define DP_TEST_MISC_BIT_3			3
> +#define DP_TEST_MISC_BIT_5			5
>  
>  #define DP_TEST_CRC_R_CR		    0x240
>  #define DP_TEST_CRC_G_Y			    0x242

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel