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@xxxxxxxxx>
> Signed-off-by: Deepak M <m.deepak@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
