Re: [PATCH v2 2/4] drm/i915/icl: Add register definitions for Multi Segmented gamma

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Shashank Sharma <shashank.sharma@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/4] drm/i915/icl: Add register definitions for Multi Segmented gamma
Date: Fri, 3 May 2019 18:05:32 +0300	[thread overview]
Message-ID: <20190503150532.GQ24299@intel.com> (raw)
In-Reply-To: <20190430152108.31814-3-shashank.sharma@intel.com>

On Tue, Apr 30, 2019 at 08:51:06PM +0530, Shashank Sharma wrote:
> From: Uma Shankar <uma.shankar@intel.com>
> 
> Add macros to define multi segmented gamma registers
> 
> V2: Addressed Ville's comments:
>     	Add gen-lable before bit definition
>     Addressed Jani's comment
> 	- Use REG_GENMASK() and REG_BIT()
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6f0a0866c802..7d10b8d00d64 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7198,7 +7198,10 @@ enum {
>  #define  GAMMA_MODE_MODE_8BIT	(0 << 0)
>  #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
>  #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
> +/* ivb-bdw */
>  #define  GAMMA_MODE_MODE_SPLIT	(3 << 0)
> +/* icl + */
> +#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0)

I would put the comments at the end of the line
#define ... /* ivb-bdw */

>  
>  /* DMC/CSR */
>  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
> @@ -10145,6 +10148,22 @@ enum skl_power_gate {
>  #define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
>  #define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
>  
> +/* Add registers for Gen11 Multi Segmented Gamma Mode */

Weird comment. 's/Add registers for //' might make it somewhat useful.
And no point in capitalizing every word. This isn't a movie title/etc.

With those sorted this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +#define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
> +#define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
> +#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		REG_BIT(15)
> +#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
> +
> +#define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
> +#define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
> +
> +#define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
> +					_PAL_PREC_MULTI_SEG_INDEX_A, \
> +					_PAL_PREC_MULTI_SEG_INDEX_B)
> +#define PREC_PAL_MULTI_SEG_DATA(pipe)	_MMIO_PIPE(pipe, \
> +					_PAL_PREC_MULTI_SEG_DATA_A, \
> +					_PAL_PREC_MULTI_SEG_DATA_B)
> +
>  /* pipe CSC & degamma/gamma LUTs on CHV */
>  #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
>  #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx