[PATCH] fix i2c_pca_pf_waitforcompletion() return value

[PATCH] 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. That's why only ret should be returned.

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-08-27 01:47:12.000000000 +0200
+++ b/drivers/i2c/busses/i2c-pca-platform.c	2010-09-20 10:21:05.000000000 +0200
@@ -96,7 +96,7 @@
 			udelay(100);
 	}
 
-	return ret > 0;
+	return ret;
 }
 
 static void i2c_pca_pf_dummyreset(void *pd)

Re: [PATCH] fix i2c_pca_pf_waitforcompletion() return value

[Wolfram Sang]

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

On Mon, Sep 20, 2010 at 11:14:32AM +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. That's why only ret should be returned.
> 
> Signed-off-by: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

Thanks for reporting. In deed, this needs to be corrected. However, I think we
should clean up the code some more and get rid of the initialization of ret
altogether (and hopefully make it more readable with this). Do you have time
for this, otherwise I'll have a look later.

Regards,

   Wolfram

> 
> Index: b/drivers/i2c/busses/i2c-pca-platform.c
> ===================================================================
> --- a/drivers/i2c/busses/i2c-pca-platform.c	2010-08-27 01:47:12.000000000 +0200
> +++ b/drivers/i2c/busses/i2c-pca-platform.c	2010-09-20 10:21:05.000000000 +0200
> @@ -96,7 +96,7 @@
>  			udelay(100);
>  	}
>  
> -	return ret > 0;
> +	return ret;
>  }
>  
>  static void i2c_pca_pf_dummyreset(void *pd)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
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] fix i2c_pca_pf_waitforcompletion() return value

[Yegor Yefremov]

> Thanks for reporting. In deed, this needs to be corrected. However, I think we
> should clean up the code some more and get rid of the initialization of ret
> altogether (and hopefully make it more readable with this). Do you have time
> for this, otherwise I'll have a look later.
>

This is a fix of a problem discussed in this thread http://www.spinics.net/lists/linux-i2c/msg02861.html.

Best regards,
Yegor

Re: [PATCH] fix i2c_pca_pf_waitforcompletion() return value

[Yegor Yefremov]

Index: b/drivers/i2c/busses/i2c-pca-platform.c
===================================================================
--- a/drivers/i2c/busses/i2c-pca-platform.c	2010-09-22 09:31:12.000000000 +0200
+++ b/drivers/i2c/busses/i2c-pca-platform.c	2010-09-22 10:47:13.000000000 +0200
@@ -80,8 +80,8 @@
 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 @@
 	} 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;
Index: b/drivers/i2c/busses/i2c-pca-isa.c
===================================================================
--- a/drivers/i2c/busses/i2c-pca-isa.c	2010-08-27 01:47:12.000000000 +0200
+++ b/drivers/i2c/busses/i2c-pca-isa.c	2010-09-22 10:47:10.000000000 +0200
@@ -71,8 +71,8 @@
 
 static int pca_isa_waitforcompletion(void *pd)
 {
-	long ret = ~0;
 	unsigned long timeout;
+	long ret;
 
 	if (irq > -1) {
 		ret = wait_event_timeout(pca_wait,
@@ -81,11 +81,15 @@
 	} else {
 		/* Do polling */
 		timeout = jiffies + pca_isa_ops.timeout;
-		while (((pca_isa_readbyte(pd, I2C_PCA_CON)
-				& I2C_PCA_CON_SI) == 0)
-				&& (ret = time_before(jiffies, timeout)))
+		do {
+			ret = time_before(jiffies, timeout);
+			if (pca_isa_readbyte(pd, I2C_PCA_CON)
+				& I2C_PCA_CON_SI)
+				break;
 			udelay(100);
+		}while(ret);
 	}
+
 	return ret > 0;
 }
 

Re: [PATCH] fix i2c_pca_pf_waitforcompletion() return value

[Wolfram Sang]

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

Hi Yegor,

looks good in general. Just a few whitespace issues. When you fixed them
you can send a proper patch with the renamed patch description and my

Reviewed-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>


On Wed, Sep 22, 2010 at 10:55:21AM +0200, Yegor Yefremov wrote:
> Index: b/drivers/i2c/busses/i2c-pca-platform.c
> ===================================================================
> --- a/drivers/i2c/busses/i2c-pca-platform.c	2010-09-22 09:31:12.000000000 +0200
> +++ b/drivers/i2c/busses/i2c-pca-platform.c	2010-09-22 10:47:13.000000000 +0200
> @@ -80,8 +80,8 @@
>  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 @@
>  	} 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)

Add one or two tabs to the above line. I missed that, too. It looks
strange if this line is on the same indentation level as the 'break'
below.

> +				break;
>  			udelay(100);
> +		} while (ret);
>  	}
>  
>  	return ret > 0;
> Index: b/drivers/i2c/busses/i2c-pca-isa.c
> ===================================================================
> --- a/drivers/i2c/busses/i2c-pca-isa.c	2010-08-27 01:47:12.000000000 +0200
> +++ b/drivers/i2c/busses/i2c-pca-isa.c	2010-09-22 10:47:10.000000000 +0200
> @@ -71,8 +71,8 @@
>  
>  static int pca_isa_waitforcompletion(void *pd)
>  {
> -	long ret = ~0;
>  	unsigned long timeout;
> +	long ret;
>  
>  	if (irq > -1) {
>  		ret = wait_event_timeout(pca_wait,
> @@ -81,11 +81,15 @@
>  	} else {
>  		/* Do polling */
>  		timeout = jiffies + pca_isa_ops.timeout;
> -		while (((pca_isa_readbyte(pd, I2C_PCA_CON)
> -				& I2C_PCA_CON_SI) == 0)
> -				&& (ret = time_before(jiffies, timeout)))
> +		do {
> +			ret = time_before(jiffies, timeout);
> +			if (pca_isa_readbyte(pd, I2C_PCA_CON)
> +				& I2C_PCA_CON_SI)

ditto.

> +				break;
>  			udelay(100);
> +		}while(ret);

Missing space.

>  	}
> +
>  	return ret > 0;
>  }
>  
> 

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

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