[PATCH v2] fix i2c_pca_pf_waitforcompletion() return value

[PATCH v2] fix i2c_pca_pf_waitforcompletion() return value

[Yegor Yefremov]

ret is still -1, if during the polling read_byte() returns at once
with I2C_PCA_CON_SI set. So ret > 0 would lead 
i2c_pca_pf_waitforcompletion() to return 0, in spite of
the proper behavior. 

Solution: get rid of ret and make immediate return where possible.

Signed-off-by: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

Index: b/drivers/i2c/busses/i2c-pca-platform.c
===================================================================
--- a/drivers/i2c/busses/i2c-pca-platform.c	2010-09-21 10:53:46.000000000 +0200
+++ b/drivers/i2c/busses/i2c-pca-platform.c	2010-09-21 10:59:18.000000000 +0200
@@ -80,23 +80,26 @@
 static int i2c_pca_pf_waitforcompletion(void *pd)
 {
 	struct i2c_pca_pf_data *i2c = pd;
-	long ret = ~0;
 	unsigned long timeout;
 
 	if (i2c->irq) {
-		ret = wait_event_timeout(i2c->wait,
+		return wait_event_timeout(i2c->wait,
 			i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
 			& I2C_PCA_CON_SI, i2c->adap.timeout);
 	} else {
 		/* Do polling */
 		timeout = jiffies + i2c->adap.timeout;
-		while (((i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
-				& I2C_PCA_CON_SI) == 0)
-				&& (ret = time_before(jiffies, timeout)))
-			udelay(100);
+		while (!(i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
+				& I2C_PCA_CON_SI))
+		{
+			if(time_before(jiffies, timeout))
+				udelay(100);
+			else
+				return 0;
+		}
 	}
 
-	return ret > 0;
+	return ~0;
 }
 
 static void i2c_pca_pf_dummyreset(void *pd)

Re: [PATCH v2] fix i2c_pca_pf_waitforcompletion() return value

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]

Hello Yegor,

On Tue, Sep 21, 2010 at 11:15:47AM +0200, Yegor Yefremov wrote:
> ret is still -1, if during the polling read_byte() returns at once
> with I2C_PCA_CON_SI set. So ret > 0 would lead 
> i2c_pca_pf_waitforcompletion() to return 0, in spite of
> the proper behavior. 
> 
> Solution: get rid of ret and make immediate return where possible.
> 
> Signed-off-by: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> 
> Index: b/drivers/i2c/busses/i2c-pca-platform.c
> ===================================================================
> --- a/drivers/i2c/busses/i2c-pca-platform.c	2010-09-21 10:53:46.000000000 +0200
> +++ b/drivers/i2c/busses/i2c-pca-platform.c	2010-09-21 10:59:18.000000000 +0200
> @@ -80,23 +80,26 @@
>  static int i2c_pca_pf_waitforcompletion(void *pd)
>  {
>  	struct i2c_pca_pf_data *i2c = pd;
> -	long ret = ~0;
>  	unsigned long timeout;
>  
>  	if (i2c->irq) {
> -		ret = wait_event_timeout(i2c->wait,
> +		return wait_event_timeout(i2c->wait,
>  			i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
>  			& I2C_PCA_CON_SI, i2c->adap.timeout);
>  	} else {
>  		/* Do polling */
>  		timeout = jiffies + i2c->adap.timeout;
> -		while (((i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
> -				& I2C_PCA_CON_SI) == 0)
> -				&& (ret = time_before(jiffies, timeout)))
> -			udelay(100);
> +		while (!(i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
> +				& I2C_PCA_CON_SI))
> +		{
> +			if(time_before(jiffies, timeout))
> +				udelay(100);
> +			else
> +				return 0;
> +		}
>  	}
>  
> -	return ret > 0;
> +	return ~0;
>  }
>  
>  static void i2c_pca_pf_dummyreset(void *pd)

Hmm, I think I'd prefer a centralized exit point. What about this? I think it
might be easier to read. Only compile tested, though. The final patch needs to
be applied to the isa version, too. BTW, good debugging there, fixing this old
issue you reported. Thanks a lot!

Regards,

   Wolfram

diff --git a/drivers/i2c/busses/i2c-pca-platform.c b/drivers/i2c/busses/i2c-pca-platform.c
index ef5c784..a13aecb 100644
--- a/drivers/i2c/busses/i2c-pca-platform.c
+++ b/drivers/i2c/busses/i2c-pca-platform.c
@@ -80,8 +80,8 @@ static void i2c_pca_pf_writebyte32(void *pd, int reg, int val)
 static int i2c_pca_pf_waitforcompletion(void *pd)
 {
 	struct i2c_pca_pf_data *i2c = pd;
-	long ret = ~0;
 	unsigned long timeout;
+	long ret;
 
 	if (i2c->irq) {
 		ret = wait_event_timeout(i2c->wait,
@@ -90,10 +90,13 @@ static int i2c_pca_pf_waitforcompletion(void *pd)
 	} else {
 		/* Do polling */
 		timeout = jiffies + i2c->adap.timeout;
-		while (((i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
-				& I2C_PCA_CON_SI) == 0)
-				&& (ret = time_before(jiffies, timeout)))
+		do {
+			ret = time_before(jiffies, timeout);
+			if (i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
+				& I2C_PCA_CON_SI)
+				break;
 			udelay(100);
+		} while (ret);
 	}
 
 	return ret > 0;


-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v2] fix i2c_pca_pf_waitforcompletion() return value

[Yegor Yefremov]

 Hello Wolfram,

> Hmm, I think I'd prefer a centralized exit point. What about this? I think it
> might be easier to read. Only compile tested, though. The final patch needs to
> be applied to the isa version, too. BTW, good debugging there, fixing this old
> issue you reported. Thanks a lot!

Everything is working as expected. I'll send the whole patch for both isa and platform flavor. I think we should then change the patch name to something like:

fix PCA waitforcompletion() return value

Best regards,
Yegor