[1/4] drm/i915: extract gmbus_wait_hw_status
diff mbox

Message ID 1346915402-9399-1-git-send-email-daniel.vetter@ffwll.ch
State Under Review
Headers show

Commit Message

Daniel Vetter Sept. 6, 2012, 7:09 a.m. UTC
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@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c | 46 ++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

Comments

Ben Widawsky Sept. 8, 2012, 5:11 p.m. UTC | #1
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@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

Patch
diff mbox

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)
 {
@@ -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