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

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.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: Mon, 6 May 2019 16:02:05 +0530	[thread overview]
Message-ID: <982fa686-7b4f-4dd0-5b90-40e153c63753@intel.com> (raw)
In-Reply-To: <20190503150532.GQ24299@intel.com>

On 5/3/2019 8:35 PM, Ville Syrjälä wrote:
> 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>

Thanks for the review, will do the changes as suggested.

- Shashank

>> +#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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx