[1/3] drm/i915: extract gmbus_wait_hw_status

Commit Message

Daniel Vetter Sept. 5, 2012, 7:24 p.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).

Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/i915/intel_i2c.c | 52 +++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

Comments

Chris Wilson Sept. 5, 2012, 9:25 p.m. UTC | #1

On Wed,  5 Sep 2012 21:24:39 +0200, Daniel Vetter <[e-mail]> 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).

There are some subtle changes in that we introduce new error detection
which is promptly ignored with a different wait period, but the changes
look good.
Reviewed-by: Chris Wilson <[e-mail]>

Daniel Vetter Sept. 5, 2012, 9:30 p.m. UTC | #2

On Wed, Sep 5, 2012 at 11:25 PM, Chris Wilson <[e-mail]> wrote:
> On Wed,  5 Sep 2012 21:24:39 +0200, Daniel Vetter <[e-mail]> 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).
>
> There are some subtle changes in that we introduce new error detection
> which is promptly ignored with a different wait period, but the changes
> look good.

Hm, I've tried to match the old code (at least where there are no
interrupts, i.e. gen2/3) as closely as possible. Hence the
schedule_timeout(1) instead of a simple wait_event_timeout. And the
error return values /should/ match what has been there before.

The only thing that changes is that I use 50ms for the timeout
everywhere - I don't see any reason why we should use something else.
So can you please elaborate for I don't really see the change in error
handling?
-Daniel

Chris Wilson Sept. 5, 2012, 10:12 p.m. UTC | #3

Who wrote wait_for()? It is a deadly booby trap!

On Wed,  5 Sep 2012 21:24:39 +0200, Daniel Vetter <[e-mail]> wrote:
> -	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
> -		     10)) {
> +	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {

This is reversing the status being waited for. :(
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b9755f6..3a90b87 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
@@ -381,8 +385,7 @@  gmbus_xfer(struct i2c_adapter *adapter,
 	 * We will re-enable it at the start of the next xfer,
 	 * till then let it sleep.
 	 */
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
-		     10)) {
+	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out waiting for idle\n",
 			 adapter->name);
 		ret = -ETIMEDOUT;
@@ -406,8 +409,7 @@  clear_err:
 	 * it's slow responding and only answers on the 2nd retry.
 	 */
 	ret = -ENXIO;
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
-		     10)) {
+	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
 			      adapter->name);
 		ret = -ETIMEDOUT;