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

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- 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 --]