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

From: Jani Nikula <jani.nikula@intel.com>
To: Madhav Chauhan <madhav.chauhan@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: paulo.r.zanoni@intel.com, rodrigo.vivi@intel.com
Subject: Re: [PATCH v5 06/13] drm/i915/icl: Define data/clock lanes dphy timing registers
Date: Tue, 11 Sep 2018 22:14:29 +0300	[thread overview]
Message-ID: <875zzbetii.fsf@intel.com> (raw)
In-Reply-To: <1531215614-6828-7-git-send-email-madhav.chauhan@intel.com>

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.

> +
> +#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)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx