Re: [PATCH 3/8] drm/i915/dsi: do not define VLV gpio pad registers separately

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: Deepak M <m.deepak@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/8] drm/i915/dsi: do not define VLV gpio pad registers separately
Date: Thu, 17 Mar 2016 17:32:01 +0200	[thread overview]
Message-ID: <20160317153201.GA4329@intel.com> (raw)
In-Reply-To: <ea89a284999c70ce60b82a6ba3c744a3e488c86e.1458226863.git.jani.nikula@intel.com>

On Thu, Mar 17, 2016 at 05:04:41PM +0200, Jani Nikula wrote:
> They seem to be all just function register offset + 8. No functional
> changes, apart from saving some space.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 41 +++++++++++-------------------
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 765dd5cd23ac..5e4d92491de7 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -59,49 +59,38 @@ static inline struct vbt_panel *to_vbt_panel(struct drm_panel *panel)
>  #define NS_KHZ_RATIO 1000000
>  
>  #define GPI0_NC_0_HV_DDI0_HPD           0x4130

The naming is rather inconsistent here. The first register for
each pad should always be called foo_PCONF0,

Alternatively we could leave out the PCONF0 here and think of
these as base offsets for the block of registers for each pad.

> -#define GPIO_NC_0_HV_DDI0_PAD           0x4138
>  #define GPIO_NC_1_HV_DDI0_DDC_SDA       0x4120
> -#define GPIO_NC_1_HV_DDI0_DDC_SDA_PAD   0x4128
>  #define GPIO_NC_2_HV_DDI0_DDC_SCL       0x4110
> -#define GPIO_NC_2_HV_DDI0_DDC_SCL_PAD   0x4118
>  #define GPIO_NC_3_PANEL0_VDDEN          0x4140
> -#define GPIO_NC_3_PANEL0_VDDEN_PAD      0x4148
>  #define GPIO_NC_4_PANEL0_BLKEN          0x4150
> -#define GPIO_NC_4_PANEL0_BLKEN_PAD      0x4158
>  #define GPIO_NC_5_PANEL0_BLKCTL         0x4160
> -#define GPIO_NC_5_PANEL0_BLKCTL_PAD     0x4168
>  #define GPIO_NC_6_PCONF0                0x4180
> -#define GPIO_NC_6_PAD                   0x4188
>  #define GPIO_NC_7_PCONF0                0x4190
> -#define GPIO_NC_7_PAD                   0x4198
>  #define GPIO_NC_8_PCONF0                0x4170
> -#define GPIO_NC_8_PAD                   0x4178
>  #define GPIO_NC_9_PCONF0                0x4100
> -#define GPIO_NC_9_PAD                   0x4108
>  #define GPIO_NC_10_PCONF0               0x40E0
> -#define GPIO_NC_10_PAD                  0x40E8
>  #define GPIO_NC_11_PCONF0               0x40F0
> -#define GPIO_NC_11_PAD                  0x40F8
> +
> +#define VLV_FUNCTION_TO_PAD_REG(reg) ((reg) + 8)

I might do something like this (assuming the "base" approach
I listed before is adopted):
#define VLV_GPIO_PCONF0(base) (base)
#define VLV_GPIO_PAD_VAL(base) ((base) + 8)

those would match the spec better. Especially there's nothing called a
function reg listed in configdb, so calling pconf0 that just creates
unwarranted confusion IMO.

>  
>  struct gpio_table {
>  	u16 function_reg;
> -	u16 pad_reg;
>  	u8 init;
>  };
>  
>  static struct gpio_table gtable[] = {
> -	{ GPI0_NC_0_HV_DDI0_HPD, GPIO_NC_0_HV_DDI0_PAD, 0 },
> -	{ GPIO_NC_1_HV_DDI0_DDC_SDA, GPIO_NC_1_HV_DDI0_DDC_SDA_PAD, 0 },
> -	{ GPIO_NC_2_HV_DDI0_DDC_SCL, GPIO_NC_2_HV_DDI0_DDC_SCL_PAD, 0 },
> -	{ GPIO_NC_3_PANEL0_VDDEN, GPIO_NC_3_PANEL0_VDDEN_PAD, 0 },
> -	{ GPIO_NC_4_PANEL0_BLKEN, GPIO_NC_4_PANEL0_BLKEN_PAD, 0 },
> -	{ GPIO_NC_5_PANEL0_BLKCTL, GPIO_NC_5_PANEL0_BLKCTL_PAD, 0 },
> -	{ GPIO_NC_6_PCONF0, GPIO_NC_6_PAD, 0 },
> -	{ GPIO_NC_7_PCONF0, GPIO_NC_7_PAD, 0 },
> -	{ GPIO_NC_8_PCONF0, GPIO_NC_8_PAD, 0 },
> -	{ GPIO_NC_9_PCONF0, GPIO_NC_9_PAD, 0 },
> -	{ GPIO_NC_10_PCONF0, GPIO_NC_10_PAD, 0},
> -	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
> +	{ GPI0_NC_0_HV_DDI0_HPD, 0 },
> +	{ GPIO_NC_1_HV_DDI0_DDC_SDA, 0 },
> +	{ GPIO_NC_2_HV_DDI0_DDC_SCL, 0 },
> +	{ GPIO_NC_3_PANEL0_VDDEN, 0 },
> +	{ GPIO_NC_4_PANEL0_BLKEN, 0 },
> +	{ GPIO_NC_5_PANEL0_BLKCTL, 0 },
> +	{ GPIO_NC_6_PCONF0, 0 },
> +	{ GPIO_NC_7_PCONF0, 0 },
> +	{ GPIO_NC_8_PCONF0, 0 },
> +	{ GPIO_NC_9_PCONF0, 0 },
> +	{ GPIO_NC_10_PCONF0, 0},
> +	{ GPIO_NC_11_PCONF0, 0}
>  };
>  
>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
> @@ -242,7 +231,7 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  	}
>  
>  	function = gtable[gpio_index].function_reg;
> -	pad = gtable[gpio_index].pad_reg;
> +	pad = VLV_FUNCTION_TO_PAD_REG(function);
>  
>  	mutex_lock(&dev_priv->sb_lock);
>  	if (!gtable[gpio_index].init) {
> -- 
> 2.1.4

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