Re: [PATCH 2/2] drm/i915: Change Mipi register definitions

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, May 21, 2014 at 08:56:59PM +0530, Shashank Sharma wrote:
> Re-define MIPI register definitions in such a way that most of
> the existing DSI code can be re-used for future platforms. Register
> definitions are re-written using MMIO offset variable, so that without
> changing the existing sequence, same code can be generically applied.
> 
> V2: Addressing review comments by Damien, added follwing changes:
> 1. Re-defined MIPI_DSI_FUNC_PRG using _PIPE macro, to remove
> 	branching.
> 2. Re-written _MIPIB_DSI_FUNC_PRG and _MIPIA_DSI_FUNC_PRG
> 	in single line.
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>

Sorry but this patch is a mess. Way too many pointless formatting
changes in there.

> ---
>  drivers/gpu/drm/i915/i915_reg.h |  689 +++++++++++++++++++++++----------------
>  1 file changed, 416 insertions(+), 273 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c12a858..50d5e89 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5655,20 +5655,23 @@ enum punit_power_well {
>  #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>  #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>  
> -/* VLV MIPI registers */
>  
> +/* ==== MIPI registers ==== */

This change is not needed.

> +
> +/* VLV port control */
>  #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
>  #define _MIPIB_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61700)
>  #define MIPI_PORT_CTRL(pipe)		_PIPE(pipe, _MIPIA_PORT_CTRL, _MIPIB_PORT_CTRL)

Why isn't mipi_mmio_base used here? Does the register not need the new
offset? I htink it would still be cleaner to use the mipi_mmio_offset
for all the MIPI registers.

> -#define  DPI_ENABLE					(1 << 31) /* A + B */
> +
> +#define  DPI_ENABLE					(1 << 31)

Not needed.

>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT		27
>  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK		(0xf << 27)
>  #define  DUAL_LINK_MODE_MASK				(1 << 26)
>  #define  DUAL_LINK_MODE_FRONT_BACK			(0 << 26)
>  #define  DUAL_LINK_MODE_PIXEL_ALTERNATIVE		(1 << 26)
> -#define  DITHERING_ENABLE				(1 << 25) /* A + B */
> +#define  DITHERING_ENABLE				(1 << 25)
>  #define  FLOPPED_HSTX					(1 << 23)
> -#define  DE_INVERT					(1 << 19) /* XXX */
> +#define  DE_INVERT					(1 << 19)

More unneeded comment changes.

>  #define  MIPIA_FLISDSI_DELAY_COUNT_SHIFT		18
>  #define  MIPIA_FLISDSI_DELAY_COUNT_MASK			(0xf << 18)
>  #define  AFE_LATCHOUT					(1 << 17)
> @@ -5699,33 +5702,46 @@ enum punit_power_well {
>  #define  LANE_CONFIGURATION_DUAL_LINK_A			(1 << 0)
>  #define  LANE_CONFIGURATION_DUAL_LINK_B			(2 << 0)
>  
> +/* VLV tearing effect control */
>  #define _MIPIA_TEARING_CTRL			(VLV_DISPLAY_BASE + 0x61194)
>  #define _MIPIB_TEARING_CTRL			(VLV_DISPLAY_BASE + 0x61704)
>  #define MIPI_TEARING_CTRL(pipe)		_PIPE(pipe, _MIPIA_TEARING_CTRL, _MIPIB_TEARING_CTRL)
> -#define  TEARING_EFFECT_DELAY_SHIFT			0
> -#define  TEARING_EFFECT_DELAY_MASK			(0xffff << 0)
> +#define TEARING_EFFECT_DELAY_SHIFT		0
> +#define TEARING_EFFECT_DELAY_MASK		(0xffff << 0)

Bad formatting change.

etc. Did you run this through some autoformatting tool or something?
Please don't do that. A simple sed job should be all that's needed
for mipi_mmio_offset.

-- 
Ville Syrjälä
Intel OTC