[Intel-gfx] [PATCH 1/4] drm/i915: extract gmbus_wait_hw_status

[Ben Widawsky]

On 2012-09-06 00:09, Daniel Vetter wrote:
> The gmbus interrupt generation is rather fiddly: We can only ever
> enable one interrupt source (but we always want to check for NAK
> in addition to the real bit). And the bits in the gmbus status
> register don't map at all to the bis in the irq register.
>
> To prepare for this mess, start by extracting the hw status wait
> loop into it's own function, consolidate the NAK error handling a
> bit. To keep things flexible, pass in the status bit we care about
> (in addition to any NAK signalling).
>
> v2: I've failed to notice that the sens of GMBUS_ACTIVE is inverted,
> Chris Wilson gladly pointed that out for me. To keep things simple,
> ignore that case for  now (we only need to idle the gmbus controller
> at the end of an entire i2c transaction, not after every message).
>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 46
> ++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> b/drivers/gpu/drm/i915/intel_i2c.c
> index b9755f6..57decac 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -204,6 +204,24 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 
> pin)
>  }
>
>  static int
> +gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
> +		     u32 gmbus2_status)
> +{
> +	int ret;
> +	int reg_offset = dev_priv->gpio_mmio_base;
> +	u32 gmbus2;
> +
> +	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> +		       (GMBUS_SATOER | gmbus2_status),
> +		       50);
> +
> +	if (gmbus2 & GMBUS_SATOER)
> +		return -ENXIO;
> +
> +	return ret;
> +}
> +
> +static int
>  gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg 
> *msg,
>  		u32 gmbus1_index)
>  {

I know this isn't new to your patch, but the consolidation leaves a 
good opportunity for clarification of the timeout via a comment.

I started digging in to the math a bit here for the 50ms timeout and 
was left confused. I believe we're off by a factor of 10 here for a 
worst case which would be a 50KHz bus speed. Comments?

> @@ -220,15 +238,10 @@ gmbus_xfer_read(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg,
>  	while (len) {
>  		int ret;
>  		u32 val, loop = 0;
> -		u32 gmbus2;
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_RDY),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
>  		if (ret)
> -			return -ETIMEDOUT;
> -		if (gmbus2 & GMBUS_SATOER)
> -			return -ENXIO;
> +			return ret;
>
>  		val = I915_READ(GMBUS3 + reg_offset);
>  		do {
> @@ -262,7 +275,6 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
>  		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
>  	while (len) {
>  		int ret;
> -		u32 gmbus2;
>
>  		val = loop = 0;
>  		do {
> @@ -271,13 +283,9 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
>
>  		I915_WRITE(GMBUS3 + reg_offset, val);
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_RDY),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
>  		if (ret)
> -			return -ETIMEDOUT;
> -		if (gmbus2 & GMBUS_SATOER)
> -			return -ENXIO;
> +			return ret;
>  	}
>  	return 0;
>  }
> @@ -346,8 +354,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>
>  	for (i = 0; i < num; i++) {
> -		u32 gmbus2;
> -
>  		if (gmbus_is_index_read(msgs, i, num)) {
>  			ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
>  			i += 1;  /* set i to the index of the read xfer */
> @@ -362,13 +368,11 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  		if (ret == -ENXIO)
>  			goto clear_err;
>
> -		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> -			       (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
> -			       50);
> +		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
> +		if (ret == -ENXIO)
> +			goto clear_err;
>  		if (ret)
>  			goto timeout;
> -		if (gmbus2 & GMBUS_SATOER)
> -			goto clear_err;
>  	}
>
>  	/* Generate a STOP condition on the bus. Note that gmbus can't 
> generata

-- 
Ben Widawsky, Intel Open Source Technology Center