Re: [PATCH v5 06/13] drm/i915/icl: Define data/clock lanes dphy timing registers

[Madhav Chauhan <madhav.chauhan@intel.com>]

On 9/12/2018 12:44 AM, Jani Nikula wrote:
> On Tue, 10 Jul 2018, Madhav Chauhan <madhav.chauhan@intel.com> wrote:
>> This patch defines DSI_CLK_TIMING_PARAM, DPHY_CLK_TIMING_PARAM,
>> DSI_DATA_TIMING_PARAM, DPHY_DATA_TIMING_PARAM register used in
>> dphy programming.
>>
>> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 6129372..0dbdd57 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -10075,6 +10075,46 @@ enum skl_power_gate {
>>   						   _ICL_DSI_T_INIT_MASTER_0,\
>>   						   _ICL_DSI_T_INIT_MASTER_1)
>>   
>> +#define _DPHY_CLK_TIMING_PARAM_0	0x162180
>> +#define _DPHY_CLK_TIMING_PARAM_1	0x6c180
>> +#define DPHY_CLK_TIMING_PARAM(port)	_MMIO_PORT(port,	\
>> +						   _DPHY_CLK_TIMING_PARAM_0,\
>> +						   _DPHY_CLK_TIMING_PARAM_1)
>> +#define _DSI_CLK_TIMING_PARAM_0		0x6b080
>> +#define _DSI_CLK_TIMING_PARAM_1		0x6b880
>> +#define DSI_CLK_TIMING_PARAM(port)	_MMIO_PORT(port,	\
>> +						   _DSI_CLK_TIMING_PARAM_0,\
>> +						   _DSI_CLK_TIMING_PARAM_1)
>> +#define  CLK_PREP_OVERRIDE		(1 << 31)
>> +#define  CLK_PREP_TIME(x)		(x << 28)
>> +#define  CLK_ZERO_OVERRIDE		(1 << 27)
>> +#define  CLK_ZERO_TIME(x)		(x << 20)
>> +#define  CLK_PRE_OVERRIDE		(1 << 19)
>> +#define  CLK_PRE_TIME(x)		(x << 16)
>> +#define  CLK_POST_OVERRIDE		(1 << 15)
>> +#define  CLK_POST_TIME(x)		(x << 8)
>> +#define  CLK_TRAIL_OVERRIDE		(1 << 7)
>> +#define  CLK_TRAIL_TIME(x)		(x << 0)
> I would prefer we stuck to the convention of defining _SHIFT and _MASK
> macros for the bitfields. Even if the above style has started to creep
> in without proper discussion. (I approve of the function-like macros for
> things that aren't straight shifts; stuff with split bitfields or
> calculations.)
>
> No matter what, you need to wrap the macro arguments in parens!
>
> Also, please don't do your own abbreviations or renames of the field
> names when the bspec name is short/good enough.

Ok. I will add the mask and wrap the arguments in parens.
Also will recheck the names as per BSPEC.

Regards,
Madhav

>
>> +
>> +#define _DPHY_DATA_TIMING_PARAM_0	0x162184
>> +#define _DPHY_DATA_TIMING_PARAM_1	0x6c184
>> +#define DPHY_DATA_TIMING_PARAM(port)	_MMIO_PORT(port,	\
>> +						   _DPHY_DATA_TIMING_PARAM_0,\
>> +						   _DPHY_DATA_TIMING_PARAM_1)
>> +#define _DSI_DATA_TIMING_PARAM_0	0x6B084
>> +#define _DSI_DATA_TIMING_PARAM_1	0x6B884
>> +#define DSI_DATA_TIMING_PARAM(port)	_MMIO_PORT(port,	\
>> +						   _DSI_DATA_TIMING_PARAM_0,\
>> +						   _DSI_DATA_TIMING_PARAM_1)
>> +#define  HS_PREP_OVERRIDE		(1 << 31)
>> +#define  HS_PREP_TIME(x)		(x << 24)
>> +#define  HS_ZERO_OVERRIDE		(1 << 23)
>> +#define  HS_ZERO_TIME(x)		(x << 16)
>> +#define  HS_TRAIL_OVERRIDE		(1 << 15)
>> +#define  HS_TRAIL_TIME(x)		(x << 8)
>> +#define  HS_EXIT_OVERRIDE		(1 << 7)
>> +#define  HS_EXIT_TIME(x)		(x << 0)
> Same as above.
>
> The register offsets and shifts etc. look ok.
>
> BR,
> Jani.
>
>> +
>>   /* bits 31:0 */
>>   #define _MIPIA_DBI_BW_CTRL		(dev_priv->mipi_mmio_base + 0xb084)
>>   #define _MIPIC_DBI_BW_CTRL		(dev_priv->mipi_mmio_base + 0xb884)

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