On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare at intel.com> wrote: > Cc: dri-devel at lists.freedesktop.org > Cc: Jani Nikula <jani.nikula at linux.intel.com> > Cc: Daniel Vetter <daniel.vetter at intel.com> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare at 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
