Re: [MIPI SEQ PARSING v3 08/13] drm/i915: Extend gpio read/write to other cores

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

On Tue, Dec 01, 2015 at 04:17:11AM +0530, Deepak M wrote:
> Adding a argument to the gpio read/write functions
> which accepts the block name.
> 
> v2: rebase
> v3: rebase
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 5 +++--
>  drivers/gpu/drm/i915/i915_reg.h            | 5 +++++
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
>  drivers/gpu/drm/i915/intel_sideband.c      | 9 +++++----
>  4 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 59a39d1..ca865f4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3379,8 +3379,9 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val
>  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
>  void vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val);
>  u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
> -u32 vlv_gpio_nc_read(struct drm_i915_private *dev_priv, u32 reg);
> -void vlv_gpio_nc_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
> +u32 vlv_gpio_read(struct drm_i915_private *dev_priv, u8 core_offset, u32 reg);
> +void vlv_gpio_write(struct drm_i915_private *dev_priv, u8 core_offset,
> +			u32 reg, u32 val);
>  u32 vlv_cck_read(struct drm_i915_private *dev_priv, u32 reg);
>  void vlv_cck_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>  u32 vlv_ccu_read(struct drm_i915_private *dev_priv, u32 reg);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bd2699..e29f7ef 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -572,6 +572,11 @@
>  #define   IOSF_PORT_DPIO			0x12
>  #define   IOSF_PORT_DPIO_2			0x1a
>  #define   IOSF_PORT_GPIO_NC			0x13
> +#define   IOSF_PORT_GPIO_SC			0x48

We already have another name for this (IOSF_PORT_GPS_CORE).
No idea why someone added the '_' in the middle of it. Now it
makes me think it has something to do with GPS rather than
GPIOs.

I'd be fine with using your new names instead, but the old
define should be removed, as should the unused
vlv_gps_core_{read,write}() functions.

> +#define   IOSF_PORT_GPIO_SUS			0xA8
> +#define   MAX_GPIO_NUM_NC			26
> +#define   MAX_GPIO_NUM_SC			128
> +#define   MAX_GPIO_NUM				172

These contant don't seem to have much to do with the actual
hardware (eg. there seem to be at lest 32 pads in NC). Rather
I take it they're constants defines by the VBT spec? If so
they should not be defined here.

>  #define   IOSF_PORT_CCK				0x14
>  #define   IOSF_PORT_CCU				0xA9
>  #define   IOSF_PORT_GPS_CORE			0x48
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index eb0697b..bc33e3a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -275,14 +275,14 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  	if (!gtable[gpio].init) {
>  		/* program the function */
>  		/* FIXME: remove constant below */
> -		vlv_gpio_nc_write(dev_priv, function, 0x2000CC00);
> +		vlv_gpio_write(dev_priv, IOSF_PORT_GPIO_NC, function, 0x2000CC00);

Side note:
Anyone ever figured out what this value actually means? Looking at
pinctrl-baytrail it seems that the right func_pin_mux value would maybe
depend on the specific pin you're dealing with. Would be nice if you
could add some defines or something for this stuff too. But that's
material for a separate patch clearly.

>  		gtable[gpio].init = 1;
>  	}
>  
>  	val = 0x4 | action;

0x4 could also use a define.

>  
>  	/* pull up/down */
> -	vlv_gpio_nc_write(dev_priv, pad, val);
> +	vlv_gpio_write(dev_priv, IOSF_PORT_GPIO_NC, pad, val);
>  	mutex_unlock(&dev_priv->sb_lock);
>  
>  	return data;
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index 8831fc5..3e0cbe6 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -129,17 +129,18 @@ u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
>  	return val;
>  }
>  
> -u32 vlv_gpio_nc_read(struct drm_i915_private *dev_priv, u32 reg)
> +u32 vlv_gpio_read(struct drm_i915_private *dev_priv, u8 core_offset, u32 reg)
>  {
>  	u32 val = 0;
> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_GPIO_NC,
> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), core_offset,
>  			SB_CRRDDA_NP, reg, &val);
>  	return val;
>  }
>  
> -void vlv_gpio_nc_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
> +void vlv_gpio_write(struct drm_i915_private *dev_priv, u8 core_offset,
> +				u32 reg, u32 val)
>  {
> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_GPIO_NC,
> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), core_offset,
>  			SB_CRWRDA_NP, reg, &val);
>  }

There's nothing GPIO specific in these functions after this change. So
I'd just call them vlv_iosf_sb_read/write(). And the argument I would
just call 'port'.

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

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