[PATCH] i2c,algo: timeout reaches -1

[PATCH] i2c,algo: timeout reaches -1

[Roel Kluin]

The postfix decrement decrements timeout till -1, but the
warning is already triggered on 0

Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 3e01992..0e2933f 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -115,7 +115,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
 
 	status = get_pcf(adap, 1);
 #ifndef STUB_I2C
-	while (timeout-- && !(status & I2C_PCF_BB)) {
+	while (--timeout && !(status & I2C_PCF_BB)) {
 		udelay(100); /* wait for 100 us */
 		status = get_pcf(adap, 1);
 	}
@@ -123,7 +123,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
 	if (timeout <= 0) {
 		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
 	}
-	
+
 	return (timeout<=0);
 }
 
@@ -134,7 +134,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
 
 	*status = get_pcf(adap, 1);
 #ifndef STUB_I2C
-	while (timeout-- && (*status & I2C_PCF_PIN)) {
+	while (--timeout && (*status & I2C_PCF_PIN)) {
 		adap->waitforpin(adap->data);
 		*status = get_pcf(adap, 1);
 	}

Re: [PATCH] i2c,algo: timeout reaches -1

[Jean Delvare]

Hi Roel,

On Sat, 31 Jan 2009 16:04:43 +0100, Roel Kluin wrote:
> The postfix decrement decrements timeout till -1, but the
> warning is already triggered on 0
> 
> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
> index 3e01992..0e2933f 100644
> --- a/drivers/i2c/algos/i2c-algo-pcf.c
> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
> @@ -115,7 +115,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>  
>  	status = get_pcf(adap, 1);
>  #ifndef STUB_I2C
> -	while (timeout-- && !(status & I2C_PCF_BB)) {
> +	while (--timeout && !(status & I2C_PCF_BB)) {
>  		udelay(100); /* wait for 100 us */
>  		status = get_pcf(adap, 1);
>  	}
> @@ -123,7 +123,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>  	if (timeout <= 0) {
>  		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
>  	}
> -	
> +
>  	return (timeout<=0);
>  }

Never include unrelated whitespace cleanups in patches which fix bugs.

>  
> @@ -134,7 +134,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
>  
>  	*status = get_pcf(adap, 1);
>  #ifndef STUB_I2C
> -	while (timeout-- && (*status & I2C_PCF_PIN)) {
> +	while (--timeout && (*status & I2C_PCF_PIN)) {
>  		adap->waitforpin(adap->data);
>  		*status = get_pcf(adap, 1);
>  	}

Not as critical as the ones in i2c-amd8111 and i2c-pxa, but still bugs,
I agree. I am, however, not totally happy with your fix. Leaving the
"<= 0" tests while the timeout will now stop at 0 is confusing. I think
you should change these tests to "== 0". Other odd things in these
functions:

* The timeout decrement should be _after_ the status test, otherwise
  you can exit with a timeout while the status was correct.

* Mixing actual error codes (-EINTR) with arbitrary negative error
  values (-1) isn't wise. We are lucky than EINTR isn't equal to 1. I
  think it would be better to return -ETIMEDOUT for timeouts rather
  than an arbitrary number.

Could you please submit a new patch fixing all the above?

Thanks,
-- 
Jean Delvare

Re: [PATCH] i2c,algo: timeout reaches -1

[Roel Kluin]

Jean Delvare wrote:
> Hi Roel,
> 
> On Sat, 31 Jan 2009 16:04:43 +0100, Roel Kluin wrote:
>> The postfix decrement decrements timeout till -1, but the
>> warning is already triggered on 0
>>
>> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
>> index 3e01992..0e2933f 100644
>> --- a/drivers/i2c/algos/i2c-algo-pcf.c
>> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
>> @@ -115,7 +115,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>>  
>>  	status = get_pcf(adap, 1);
>>  #ifndef STUB_I2C
>> -	while (timeout-- && !(status & I2C_PCF_BB)) {
>> +	while (--timeout && !(status & I2C_PCF_BB)) {
>>  		udelay(100); /* wait for 100 us */
>>  		status = get_pcf(adap, 1);
>>  	}
>> @@ -123,7 +123,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>>  	if (timeout <= 0) {
>>  		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
>>  	}
>> -	
>> +
>>  	return (timeout<=0);
>>  }
> 
> Never include unrelated whitespace cleanups in patches which fix bugs.

I thought it was nice in this case, since it shows where things go wrong.
Also I think it is generally considered wrong only if it affects code
that are in totally different sections - because it confuses, which it
does not if it's in the same section.

>> @@ -134,7 +134,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
>>  
>>  	*status = get_pcf(adap, 1);
>>  #ifndef STUB_I2C
>> -	while (timeout-- && (*status & I2C_PCF_PIN)) {
>> +	while (--timeout && (*status & I2C_PCF_PIN)) {
>>  		adap->waitforpin(adap->data);
>>  		*status = get_pcf(adap, 1);
>>  	}
> 
> Not as critical as the ones in i2c-amd8111 and i2c-pxa, but still bugs,
> I agree. I am, however, not totally happy with your fix. Leaving the
> "<= 0" tests while the timeout will now stop at 0 is confusing. I think
> you should change these tests to "== 0". Other odd things in these
> functions:

I consider the "<= 0" test to be safer considering possible future
changes.

 
> * The timeout decrement should be _after_ the status test, otherwise
>   you can exit with a timeout while the status was correct.

I agree, fixed in the patch below

> * Mixing actual error codes (-EINTR) with arbitrary negative error
>   values (-1) isn't wise. We are lucky than EINTR isn't equal to 1. I
>   think it would be better to return -ETIMEDOUT for timeouts rather
>   than an arbitrary number.

Also fixed (for both functions).

Also I noted that in wait_for_pin() on a timeout, dependent on the
status, still a handle_lab(adap, status) and return -EINTR may occur,
which I think are wrong.

> Could you please submit a new patch fixing all the above?
> 
> Thanks,

here it is:

---------------------------->8----------------8<------------------------------
* Fix that the warning was already triggered on 0, which was not yet a timeout.
* Move timeout decrement after the status test so that we don't exit with a
timeout while the status was correct.
* Replace arbitrary values with error codes: return -ETIMEDOUT; upon timeout. 
* Ensure that upon a timeout we do not handle dependent on the last status.
* Local whitespace cleanups.

Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-pcf.c |   41 ++++++++++++++++---------------------
 1 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 3e01992..05f0f56 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -108,45 +108,40 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
 		get_pcf(adap, 1)));
 }
 
-static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
-
+static int wait_for_bb(struct i2c_algo_pcf_data *adap)
+{
+#ifndef STUB_I2C
 	int timeout = DEF_TIMEOUT;
-	int status;
 
-	status = get_pcf(adap, 1);
-#ifndef STUB_I2C
-	while (timeout-- && !(status & I2C_PCF_BB)) {
+	while (!(get_pcf(adap, 1) & I2C_PCF_BB) && --timeout)
 		udelay(100); /* wait for 100 us */
-		status = get_pcf(adap, 1);
-	}
-#endif
+
 	if (timeout <= 0) {
 		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
+		return -ETIMEDOUT;
 	}
-	
-	return (timeout<=0);
+#endif
+	return 0;
 }
 
 
-static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
-
+static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
+{
+#ifndef STUB_I2C
 	int timeout = DEF_TIMEOUT;
 
-	*status = get_pcf(adap, 1);
-#ifndef STUB_I2C
-	while (timeout-- && (*status & I2C_PCF_PIN)) {
+	while ((*status = get_pcf(adap, 1)) & I2C_PCF_PIN && --timeout)
 		adap->waitforpin(adap->data);
-		*status = get_pcf(adap, 1);
-	}
+
+	if (timeout <= 0)
+		return -ETIMEDOUT;
+
 	if (*status & I2C_PCF_LAB) {
 		handle_lab(adap, status);
-		return(-EINTR);
+		return -EINTR;
 	}
 #endif
-	if (timeout <= 0)
-		return(-1);
-	else
-		return(0);
+	return 0;
 }
 
 /* 

Re: [PATCH] i2c,algo: timeout reaches -1

[Jean Delvare]

On Mon, 02 Feb 2009 22:09:11 +0100, Roel Kluin wrote:
> Jean Delvare wrote:
> > Hi Roel,
> > 
> > On Sat, 31 Jan 2009 16:04:43 +0100, Roel Kluin wrote:
> >> The postfix decrement decrements timeout till -1, but the
> >> warning is already triggered on 0
> >>
> >> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
> >> index 3e01992..0e2933f 100644
> >> --- a/drivers/i2c/algos/i2c-algo-pcf.c
> >> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
> >> @@ -115,7 +115,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
> >>  
> >>  	status = get_pcf(adap, 1);
> >>  #ifndef STUB_I2C
> >> -	while (timeout-- && !(status & I2C_PCF_BB)) {
> >> +	while (--timeout && !(status & I2C_PCF_BB)) {
> >>  		udelay(100); /* wait for 100 us */
> >>  		status = get_pcf(adap, 1);
> >>  	}
> >> @@ -123,7 +123,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
> >>  	if (timeout <= 0) {
> >>  		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
> >>  	}
> >> -	
> >> +
> >>  	return (timeout<=0);
> >>  }
> > 
> > Never include unrelated whitespace cleanups in patches which fix bugs.
> 
> I thought it was nice in this case, since it shows where things go wrong.

If you really want to show this, feel free to increase the patch
context. But in general, people can go read the original source file if
they have to (which is what I did.)

> Also I think it is generally considered wrong only if it affects code
> that are in totally different sections - because it confuses, which it
> does not if it's in the same section.

I tend to believe that it becomes confusing as soon as it creates an
additional hunk in the patch. It also increases the risk of patch
rejection when one ports the fix to a different tree.

> >> @@ -134,7 +134,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
> >>  
> >>  	*status = get_pcf(adap, 1);
> >>  #ifndef STUB_I2C
> >> -	while (timeout-- && (*status & I2C_PCF_PIN)) {
> >> +	while (--timeout && (*status & I2C_PCF_PIN)) {
> >>  		adap->waitforpin(adap->data);
> >>  		*status = get_pcf(adap, 1);
> >>  	}
> > 
> > Not as critical as the ones in i2c-amd8111 and i2c-pxa, but still bugs,
> > I agree. I am, however, not totally happy with your fix. Leaving the
> > "<= 0" tests while the timeout will now stop at 0 is confusing. I think
> > you should change these tests to "== 0". Other odd things in these
> > functions:
> 
> I consider the "<= 0" test to be safer considering possible future
> changes.

Do you realize that this is exactly the kind of thinking which in the
first place led to the bug you're fixing?

> > * The timeout decrement should be _after_ the status test, otherwise
> >   you can exit with a timeout while the status was correct.
> 
> I agree, fixed in the patch below
> 
> > * Mixing actual error codes (-EINTR) with arbitrary negative error
> >   values (-1) isn't wise. We are lucky than EINTR isn't equal to 1. I
> >   think it would be better to return -ETIMEDOUT for timeouts rather
> >   than an arbitrary number.
> 
> Also fixed (for both functions).
> 
> Also I noted that in wait_for_pin() on a timeout, dependent on the
> status, still a handle_lab(adap, status) and return -EINTR may occur,
> which I think are wrong.

Depends on the hardware implementation. If I2C_PCF_LAB can be set while
I2C_PCF_PIN is set then indeed this is wrong. But I suspect the
hardware is such that I2C_PCF_LAB can't be set without I2C_PCF_PIN
being clear. You'd have to check the PCF8584 datasheet to make sure.

> 
> > Could you please submit a new patch fixing all the above?
> > 
> > Thanks,
> 
> here it is:
> 
> ---------------------------->8----------------8<------------------------------
> * Fix that the warning was already triggered on 0, which was not yet a timeout.
> * Move timeout decrement after the status test so that we don't exit with a
> timeout while the status was correct.
> * Replace arbitrary values with error codes: return -ETIMEDOUT; upon timeout. 
> * Ensure that upon a timeout we do not handle dependent on the last status.
> * Local whitespace cleanups.
> 
> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-pcf.c |   41 ++++++++++++++++---------------------
>  1 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
> index 3e01992..05f0f56 100644
> --- a/drivers/i2c/algos/i2c-algo-pcf.c
> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
> @@ -108,45 +108,40 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
>  		get_pcf(adap, 1)));
>  }
>  
> -static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
> -
> +static int wait_for_bb(struct i2c_algo_pcf_data *adap)
> +{
> +#ifndef STUB_I2C

As a side note, I think it's about time to get rid of STUB_I2C, as it
is totally undocumented and at first sight it doesn't seem terribly
useful anymore (if it ever was).

>  	int timeout = DEF_TIMEOUT;
> -	int status;
>  
> -	status = get_pcf(adap, 1);
> -#ifndef STUB_I2C
> -	while (timeout-- && !(status & I2C_PCF_BB)) {
> +	while (!(get_pcf(adap, 1) & I2C_PCF_BB) && --timeout)
>  		udelay(100); /* wait for 100 us */
> -		status = get_pcf(adap, 1);
> -	}
> -#endif
> +
>  	if (timeout <= 0) {
>  		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
> +		return -ETIMEDOUT;
>  	}
> -	
> -	return (timeout<=0);
> +#endif
> +	return 0;
>  }
>  
>  
> -static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
> -
> +static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
> +{
> +#ifndef STUB_I2C
>  	int timeout = DEF_TIMEOUT;
>  
> -	*status = get_pcf(adap, 1);
> -#ifndef STUB_I2C

As long as STUB_I2C is there, the above is incorrect, as you will
return 0 with *status unset. I would like to suggest that we go with 2
patches, one removing STUB_I2C and fixing any coding style issue you
like, and a second one with the actual logic fixes.

> -	while (timeout-- && (*status & I2C_PCF_PIN)) {
> +	while ((*status = get_pcf(adap, 1)) & I2C_PCF_PIN && --timeout)

This lacks parentheses. Also, you'll get a warning from checkpatch for
this construct (not sure if we care.)

>  		adap->waitforpin(adap->data);
> -		*status = get_pcf(adap, 1);
> -	}
> +
> +	if (timeout <= 0)
> +		return -ETIMEDOUT;
> +
>  	if (*status & I2C_PCF_LAB) {
>  		handle_lab(adap, status);
> -		return(-EINTR);
> +		return -EINTR;
>  	}
>  #endif
> -	if (timeout <= 0)
> -		return(-1);
> -	else
> -		return(0);
> +	return 0;
>  }
>  
>  /* 


-- 
Jean Delvare

[PATCH 1/2] i2c,algo: cleanup i2c-algo-pcf.c

[Roel Kluin]

Jean Delvare wrote:
> On Mon, 02 Feb 2009 22:09:11 +0100, Roel Kluin wrote:
>> Jean Delvare wrote:
>>> Hi Roel,
>>>
>>> On Sat, 31 Jan 2009 16:04:43 +0100, Roel Kluin wrote:
>>>> The postfix decrement decrements timeout till -1, but the
>>>> warning is already triggered on 0
>>>>
>>>> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
>>>> index 3e01992..0e2933f 100644
>>>> --- a/drivers/i2c/algos/i2c-algo-pcf.c
>>>> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
>>>> @@ -115,7 +115,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>>>>  
>>>>  	status = get_pcf(adap, 1);
>>>>  #ifndef STUB_I2C
>>>> -	while (timeout-- && !(status & I2C_PCF_BB)) {
>>>> +	while (--timeout && !(status & I2C_PCF_BB)) {
>>>>  		udelay(100); /* wait for 100 us */
>>>>  		status = get_pcf(adap, 1);
>>>>  	}
>>>> @@ -123,7 +123,7 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>>>>  	if (timeout <= 0) {
>>>>  		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
>>>>  	}
>>>> -	
>>>> +
>>>>  	return (timeout<=0);
>>>>  }
>>> Never include unrelated whitespace cleanups in patches which fix bugs.
>> I thought it was nice in this case, since it shows where things go wrong.
> 
> If you really want to show this, feel free to increase the patch
> context. But in general, people can go read the original source file if
> they have to (which is what I did.)
> 
>> Also I think it is generally considered wrong only if it affects code
>> that are in totally different sections - because it confuses, which it
>> does not if it's in the same section.
> 
> I tend to believe that it becomes confusing as soon as it creates an
> additional hunk in the patch. It also increases the risk of patch
> rejection when one ports the fix to a different tree.
> 
>>>> @@ -134,7 +134,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
>>>>  
>>>>  	*status = get_pcf(adap, 1);
>>>>  #ifndef STUB_I2C
>>>> -	while (timeout-- && (*status & I2C_PCF_PIN)) {
>>>> +	while (--timeout && (*status & I2C_PCF_PIN)) {
>>>>  		adap->waitforpin(adap->data);
>>>>  		*status = get_pcf(adap, 1);
>>>>  	}
>>> Not as critical as the ones in i2c-amd8111 and i2c-pxa, but still bugs,
>>> I agree. I am, however, not totally happy with your fix. Leaving the
>>> "<= 0" tests while the timeout will now stop at 0 is confusing. I think
>>> you should change these tests to "== 0". Other odd things in these
>>> functions:
>> I consider the "<= 0" test to be safer considering possible future
>> changes.
> 
> Do you realize that this is exactly the kind of thinking which in the
> first place led to the bug you're fixing?

I disagree. It made it as harmless as it was. If there is a '== 0' test,
while the result is -1, there will be no printk, and a return as if
everything is ok, while it is not. Such bugs are hard to catch.
 
>>> * The timeout decrement should be _after_ the status test, otherwise
>>>   you can exit with a timeout while the status was correct.
>> I agree, fixed in the patch below
>>
>>> * Mixing actual error codes (-EINTR) with arbitrary negative error
>>>   values (-1) isn't wise. We are lucky than EINTR isn't equal to 1. I
>>>   think it would be better to return -ETIMEDOUT for timeouts rather
>>>   than an arbitrary number.
>> Also fixed (for both functions).
>>
>> Also I noted that in wait_for_pin() on a timeout, dependent on the
>> status, still a handle_lab(adap, status) and return -EINTR may occur,
>> which I think are wrong.
> 
> Depends on the hardware implementation. If I2C_PCF_LAB can be set while
> I2C_PCF_PIN is set then indeed this is wrong. But I suspect the
> hardware is such that I2C_PCF_LAB can't be set without I2C_PCF_PIN
> being clear. You'd have to check the PCF8584 datasheet to make sure.

Regardless of the hardware implementation, if there is a timeout, that
is a reason to return -ETIMEDOUT, we can test later if not. That is the
sensible order in my opinion.

>>> Could you please submit a new patch fixing all the above?
>>>
>>> Thanks,
>> here it is:
>>
>> ---------------------------->8----------------8<------------------------------
>> * Fix that the warning was already triggered on 0, which was not yet a timeout.
>> * Move timeout decrement after the status test so that we don't exit with a
>> timeout while the status was correct.
>> * Replace arbitrary values with error codes: return -ETIMEDOUT; upon timeout. 
>> * Ensure that upon a timeout we do not handle dependent on the last status.
>> * Local whitespace cleanups.
>>
>> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/i2c/algos/i2c-algo-pcf.c |   41 ++++++++++++++++---------------------
>>  1 files changed, 18 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
>> index 3e01992..05f0f56 100644
>> --- a/drivers/i2c/algos/i2c-algo-pcf.c
>> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
>> @@ -108,45 +108,40 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
>>  		get_pcf(adap, 1)));
>>  }
>>  
>> -static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
>> -
>> +static int wait_for_bb(struct i2c_algo_pcf_data *adap)
>> +{
>> +#ifndef STUB_I2C
> 
> As a side note, I think it's about time to get rid of STUB_I2C, as it
> is totally undocumented and at first sight it doesn't seem terribly
> useful anymore (if it ever was).
> 
>>  	int timeout = DEF_TIMEOUT;
>> -	int status;
>>  
>> -	status = get_pcf(adap, 1);
>> -#ifndef STUB_I2C
>> -	while (timeout-- && !(status & I2C_PCF_BB)) {
>> +	while (!(get_pcf(adap, 1) & I2C_PCF_BB) && --timeout)
>>  		udelay(100); /* wait for 100 us */
>> -		status = get_pcf(adap, 1);
>> -	}
>> -#endif
>> +
>>  	if (timeout <= 0) {
>>  		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
>> +		return -ETIMEDOUT;
>>  	}
>> -	
>> -	return (timeout<=0);
>> +#endif
>> +	return 0;
>>  }
>>  
>>  
>> -static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
>> -
>> +static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
>> +{
>> +#ifndef STUB_I2C
>>  	int timeout = DEF_TIMEOUT;
>>  
>> -	*status = get_pcf(adap, 1);
>> -#ifndef STUB_I2C
> 
> As long as STUB_I2C is there, the above is incorrect, as you will
> return 0 with *status unset. I would like to suggest that we go with 2
> patches, one removing STUB_I2C and fixing any coding style issue you
> like, and a second one with the actual logic fixes.

You are right, thanks.

>> -	while (timeout-- && (*status & I2C_PCF_PIN)) {
>> +	while ((*status = get_pcf(adap, 1)) & I2C_PCF_PIN && --timeout)
> 
> This lacks parentheses. Also, you'll get a warning from checkpatch for
> this construct (not sure if we care.)
> 
>>  		adap->waitforpin(adap->data);
>> -		*status = get_pcf(adap, 1);
>> -	}
>> +
>> +	if (timeout <= 0)
>> +		return -ETIMEDOUT;
>> +
>>  	if (*status & I2C_PCF_LAB) {
>>  		handle_lab(adap, status);
>> -		return(-EINTR);
>> +		return -EINTR;
>>  	}
>>  #endif
>> -	if (timeout <= 0)
>> -		return(-1);
>> -	else
>> -		return(0);
>> +	return 0;
>>  }
>>  
>>  /* 
> 
> 

cleanup whitespace, fix comments and remove the unused STUB_I2C.

Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 3e01992..47c6266 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -1,31 +1,30 @@
-/* ------------------------------------------------------------------------- */
-/* i2c-algo-pcf.c i2c driver algorithms for PCF8584 adapters		     */
-/* ------------------------------------------------------------------------- */
-/*   Copyright (C) 1995-1997 Simon G. Vogl
-                   1998-2000 Hans Berglund
-
-    This program is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    This program is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with this program; if not, write to the Free Software
-    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.		     */
-/* ------------------------------------------------------------------------- */
-
-/* With some changes from Kyösti Mälkki <kmalkki-kf+aQKke1yb1KXRcyAk9cg@public.gmane.org> and
-   Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> ,and also from Martin Bailey
-   <mbailey-0IcqLCOdkweesDZOCDqTHtBPR1lH4CV8@public.gmane.org> */
-
-/* Partially rewriten by Oleg I. Vdovikin <vdovikin-EllQnLNTEGo@public.gmane.org> to handle multiple
-   messages, proper stop/repstart signaling during receive,
-   added detect code */
+/*
+ * i2c-algo-pcf.c i2c driver algorithms for PCF8584 adapters
+ *
+ *   Copyright (C) 1995-1997 Simon G. Vogl
+ *		   1998-2000 Hans Berglund
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * With some changes from Kyösti Mälkki <kmalkki-kf+aQKke1yb1KXRcyAk9cg@public.gmane.org> and
+ * Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org>, and also from Martin Bailey
+ * <mbailey-0IcqLCOdkweesDZOCDqTHtBPR1lH4CV8@public.gmane.org>
+ *
+ * Partially rewriten by Oleg I. Vdovikin <vdovikin-EllQnLNTEGo@public.gmane.org> to handle multiple
+ * messages, proper stop/repstart signaling during receive, added detect code
+ */
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -38,17 +37,18 @@
 #include "i2c-algo-pcf.h"
 
 
-#define DEB2(x) if (i2c_debug>=2) x
-#define DEB3(x) if (i2c_debug>=3) x /* print several statistical values*/
-#define DEBPROTO(x) if (i2c_debug>=9) x;
- 	/* debug the protocol by showing transferred bits */
+#define DEB2(x) if (i2c_debug >= 2) x
+#define DEB3(x) if (i2c_debug >= 3) x /* print several statistical values */
+#define DEBPROTO(x) if (i2c_debug >= 9) x;
+	/* debug the protocol by showing transferred bits */
 #define DEF_TIMEOUT 16
 
-/* module parameters:
+/*
+ * module parameters:
  */
 static int i2c_debug;
 
-/* --- setting states on the bus with the right timing: ---------------	*/
+/* setting states on the bus with the right timing: */
 
 #define set_pcf(adap, ctl, val) adap->setpcf(adap->data, ctl, val)
 #define get_pcf(adap, ctl) adap->getpcf(adap->data, ctl)
@@ -57,22 +57,21 @@ static int i2c_debug;
 #define i2c_outb(adap, val) adap->setpcf(adap->data, 0, val)
 #define i2c_inb(adap) adap->getpcf(adap->data, 0)
 
-/* --- other auxiliary functions --------------------------------------	*/
+/* other auxiliary functions */
 
-static void i2c_start(struct i2c_algo_pcf_data *adap) 
+static void i2c_start(struct i2c_algo_pcf_data *adap)
 {
 	DEBPROTO(printk("S "));
 	set_pcf(adap, 1, I2C_PCF_START);
 }
 
-static void i2c_repstart(struct i2c_algo_pcf_data *adap) 
+static void i2c_repstart(struct i2c_algo_pcf_data *adap)
 {
 	DEBPROTO(printk(" Sr "));
 	set_pcf(adap, 1, I2C_PCF_REPSTART);
 }
 
-
-static void i2c_stop(struct i2c_algo_pcf_data *adap) 
+static void i2c_stop(struct i2c_algo_pcf_data *adap)
 {
 	DEBPROTO(printk("P\n"));
 	set_pcf(adap, 1, I2C_PCF_STOP);
@@ -82,17 +81,17 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
 {
 	DEB2(printk(KERN_INFO
 		"i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n",
-		 *status));
-
-	/* Cleanup from LAB -- reset and enable ESO.
+		*status));
+	/*
+	 * Cleanup from LAB -- reset and enable ESO.
 	 * This resets the PCF8584; since we've lost the bus, no
 	 * further attempts should be made by callers to clean up
 	 * (no i2c_stop() etc.)
 	 */
 	set_pcf(adap, 1, I2C_PCF_PIN);
 	set_pcf(adap, 1, I2C_PCF_ESO);
-
-	/* We pause for a time period sufficient for any running
+	/*
+	 * We pause for a time period sufficient for any running
 	 * I2C transaction to complete -- the arbitration logic won't
 	 * work properly until the next START is seen.
 	 * It is assumed the bus driver or client has set a proper value.
@@ -108,48 +107,48 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
 		get_pcf(adap, 1)));
 }
 
-static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
+static int wait_for_bb(struct i2c_algo_pcf_data *adap)
+{
 
 	int timeout = DEF_TIMEOUT;
 	int status;
 
 	status = get_pcf(adap, 1);
-#ifndef STUB_I2C
+
 	while (timeout-- && !(status & I2C_PCF_BB)) {
 		udelay(100); /* wait for 100 us */
 		status = get_pcf(adap, 1);
 	}
-#endif
-	if (timeout <= 0) {
+
+	if (timeout <= 0)
 		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
-	}
-	
-	return (timeout<=0);
-}
 
+	return timeout <= 0;
+}
 
-static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
+static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
+{
 
 	int timeout = DEF_TIMEOUT;
 
 	*status = get_pcf(adap, 1);
-#ifndef STUB_I2C
+
 	while (timeout-- && (*status & I2C_PCF_PIN)) {
 		adap->waitforpin(adap->data);
 		*status = get_pcf(adap, 1);
 	}
 	if (*status & I2C_PCF_LAB) {
 		handle_lab(adap, status);
-		return(-EINTR);
+		return -EINTR;
 	}
-#endif
+
 	if (timeout <= 0)
-		return(-1);
+		return -1;
 	else
-		return(0);
+		return 0;
 }
 
-/* 
+/*
  * This should perform the 'PCF8584 initialization sequence' as described
  * in the Philips IC12 data book (1995, Aug 29).
  * There should be a 30 clock cycle wait after reset, I assume this
@@ -164,18 +163,21 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 {
 	unsigned char temp;
 
-	DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: PCF state 0x%02x\n", get_pcf(adap, 1)));
+	DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: PCF state 0x%02x\n",
+				get_pcf(adap, 1)));
 
 	/* S1=0x80: S0 selected, serial interface off */
 	set_pcf(adap, 1, I2C_PCF_PIN);
-	/* check to see S1 now used as R/W ctrl -
-	   PCF8584 does that when ESO is zero */
+	/*
+	 * check to see S1 now used as R/W ctrl -
+	 * PCF8584 does that when ESO is zero
+	 */
 	if (((temp = get_pcf(adap, 1)) & 0x7f) != (0)) {
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't select S0 (0x%02x).\n", temp));
 		return -ENXIO; /* definetly not PCF8584 */
 	}
 
-	/* load own address in S0, effective address is (own << 1)	*/
+	/* load own address in S0, effective address is (own << 1) */
 	i2c_outb(adap, get_own(adap));
 	/* check it's really written */
 	if ((temp = i2c_inb(adap)) != get_own(adap)) {
@@ -183,7 +185,7 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		return -ENXIO;
 	}
 
-	/* S1=0xA0, next byte in S2					*/
+	/* S1=0xA0, next byte in S2 */
 	set_pcf(adap, 1, I2C_PCF_PIN | I2C_PCF_ES1);
 	/* check to see S2 now selected */
 	if (((temp = get_pcf(adap, 1)) & 0x7f) != I2C_PCF_ES1) {
@@ -191,7 +193,7 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		return -ENXIO;
 	}
 
-	/* load clock register S2					*/
+	/* load clock register S2 */
 	i2c_outb(adap, get_clock(adap));
 	/* check it's really written, the only 5 lowest bits does matter */
 	if (((temp = i2c_inb(adap)) & 0x1f) != get_clock(adap)) {
@@ -199,7 +201,7 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		return -ENXIO;
 	}
 
-	/* Enable serial interface, idle, S0 selected			*/
+	/* Enable serial interface, idle, S0 selected */
 	set_pcf(adap, 1, I2C_PCF_IDLE);
 
 	/* check to see PCF is really idled and we can access status register */
@@ -207,57 +209,47 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't select S1` (0x%02x).\n", temp));
 		return -ENXIO;
 	}
-	
+
 	printk(KERN_DEBUG "i2c-algo-pcf.o: detected and initialized PCF8584.\n");
 
 	return 0;
 }
 
-
-/* ----- Utility functions
- */
-
 static int pcf_sendbytes(struct i2c_adapter *i2c_adap, const char *buf,
-                         int count, int last)
+			 int count, int last)
 {
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
 	int wrcount, status, timeout;
-    
+
 	for (wrcount=0; wrcount<count; ++wrcount) {
 		DEB2(dev_dbg(&i2c_adap->dev, "i2c_write: writing %2.2X\n",
-				buf[wrcount]&0xff));
+				buf[wrcount] & 0xff));
 		i2c_outb(adap, buf[wrcount]);
 		timeout = wait_for_pin(adap, &status);
 		if (timeout) {
-			if (timeout == -EINTR) {
-				/* arbitration lost */
-				return -EINTR;
-			}
+			if (timeout == -EINTR)
+				return -EINTR; /* arbitration lost */
+
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "i2c_write: error - timeout.\n");
 			return -EREMOTEIO; /* got a better one ?? */
 		}
-#ifndef STUB_I2C
 		if (status & I2C_PCF_LRB) {
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "i2c_write: error - no ack.\n");
 			return -EREMOTEIO; /* got a better one ?? */
 		}
-#endif
 	}
-	if (last) {
+	if (last)
 		i2c_stop(adap);
-	}
-	else {
+	else
 		i2c_repstart(adap);
-	}
 
-	return (wrcount);
+	return wrcount;
 }
 
-
 static int pcf_readbytes(struct i2c_adapter *i2c_adap, char *buf,
-                         int count, int last)
+			 int count, int last)
 {
 	int i, status;
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
@@ -267,42 +259,36 @@ static int pcf_readbytes(struct i2c_adapter *i2c_adap, char *buf,
 	for (i = 0; i <= count; i++) {
 
 		if ((wfp = wait_for_pin(adap, &status))) {
-			if (wfp == -EINTR) {
-				/* arbitration lost */
-				return -EINTR;
-			}
+			if (wfp == -EINTR)
+				return -EINTR; /* arbitration lost */
+
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "pcf_readbytes timed out.\n");
-			return (-1);
+			return -1;
 		}
 
-#ifndef STUB_I2C
 		if ((status & I2C_PCF_LRB) && (i != count)) {
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "i2c_read: i2c_inb, No ack.\n");
-			return (-1);
+			return -1;
 		}
-#endif
-		
+
 		if (i == count - 1) {
 			set_pcf(adap, 1, I2C_PCF_ESO);
-		} else 
-		if (i == count) {
-			if (last) {
+		} else if (i == count) {
+			if (last)
 				i2c_stop(adap);
-			} else {
+			else
 				i2c_repstart(adap);
-			}
-		};
+		}
 
-		if (i) {
+		if (i)
 			buf[i - 1] = i2c_inb(adap);
-		} else {
+		else
 			i2c_inb(adap); /* dummy read */
-		}
 	}
 
-	return (i - 1);
+	return i - 1;
 }
 
 
@@ -313,24 +299,26 @@ static int pcf_doAddress(struct i2c_algo_pcf_data *adap,
 	unsigned char addr;
 
 	addr = msg->addr << 1;
+
 	if (flags & I2C_M_RD)
 		addr |= 1;
 	if (flags & I2C_M_REV_DIR_ADDR)
 		addr ^= 1;
+
 	i2c_outb(adap, addr);
 
 	return 0;
 }
 
 static int pcf_xfer(struct i2c_adapter *i2c_adap,
-		    struct i2c_msg *msgs, 
+		    struct i2c_msg *msgs,
 		    int num)
 {
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
 	struct i2c_msg *pmsg;
 	int i;
 	int ret=0, timeout, status;
-    
+
 	if (adap->xfer_begin)
 		adap->xfer_begin(adap->data);
 
@@ -338,25 +326,24 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 	timeout = wait_for_bb(adap);
 	if (timeout) {
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
-		            "Timeout waiting for BB in pcf_xfer\n");)
+			    "Timeout waiting for BB in pcf_xfer\n");)
 		i = -EIO;
 		goto out;
 	}
-	
+
 	for (i = 0;ret >= 0 && i < num; i++) {
 		pmsg = &msgs[i];
 
 		DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: Doing %s %d bytes to 0x%02x - %d of %d messages\n",
 		     pmsg->flags & I2C_M_RD ? "read" : "write",
-                     pmsg->len, pmsg->addr, i + 1, num);)
-    
+		     pmsg->len, pmsg->addr, i + 1, num);)
+
 		ret = pcf_doAddress(adap, pmsg);
 
 		/* Send START */
-		if (i == 0) {
-			i2c_start(adap); 
-		}
-    
+		if (i == 0)
+			i2c_start(adap);
+
 		/* Wait for PIN (pending interrupt NOT) */
 		timeout = wait_for_pin(adap, &status);
 		if (timeout) {
@@ -371,8 +358,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 			i = -EREMOTEIO;
 			goto out;
 		}
-    
-#ifndef STUB_I2C
+
 		/* Check LRB (last rcvd bit - slave ack) */
 		if (status & I2C_PCF_LRB) {
 			i2c_stop(adap);
@@ -380,57 +366,53 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 			i = -EREMOTEIO;
 			goto out;
 		}
-#endif
-    
+
 		DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: Msg %d, addr=0x%x, flags=0x%x, len=%d\n",
 			    i, msgs[i].addr, msgs[i].flags, msgs[i].len);)
-    
-		/* Read */
+
 		if (pmsg->flags & I2C_M_RD) {
-			/* read bytes into buffer*/
 			ret = pcf_readbytes(i2c_adap, pmsg->buf, pmsg->len,
-                                            (i + 1 == num));
-        
-			if (ret != pmsg->len) {
+					    (i + 1 == num));
+
+			if (ret != pmsg->len)
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
 					    "only read %d bytes.\n",ret));
-			} else {
+			else
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: read %d bytes.\n",ret));
-			}
-		} else { /* Write */
+
+		} else {
 			ret = pcf_sendbytes(i2c_adap, pmsg->buf, pmsg->len,
-                                            (i + 1 == num));
-        
-			if (ret != pmsg->len) {
+					    (i + 1 == num));
+
+			if (ret != pmsg->len)
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
 					    "only wrote %d bytes.\n",ret));
-			} else {
+			else
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: wrote %d bytes.\n",ret));
-			}
+
 		}
 	}
 
 out:
 	if (adap->xfer_end)
 		adap->xfer_end(adap->data);
-	return (i);
+	return i;
 }
 
 static u32 pcf_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | 
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
 	       I2C_FUNC_PROTOCOL_MANGLING;
 }
 
-/* -----exported algorithm data: -------------------------------------	*/
-
+/* exported algorithm data: */
 static const struct i2c_algorithm pcf_algo = {
 	.master_xfer	= pcf_xfer,
 	.functionality	= pcf_func,
 };
 
-/* 
- * registering functions to load algorithms at runtime 
+/*
+ * registering functions to load algorithms at runtime
  */
 int i2c_pcf_add_bus(struct i2c_adapter *adap)
 {
@@ -458,4 +440,4 @@ MODULE_LICENSE("GPL");
 
 module_param(i2c_debug, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(i2c_debug,
-        "debug level - 0 off; 1 normal; 2,3 more verbose; 9 pcf-protocol");
+	"debug level - 0 off; 1 normal; 2,3 more verbose; 9 pcf-protocol");

Re: [PATCH 1/2] i2c,algo: cleanup i2c-algo-pcf.c

[Jean Delvare]

On Tue, 03 Feb 2009 13:31:52 +0100, Roel Kluin wrote:
> Jean Delvare wrote:
> > On Mon, 02 Feb 2009 22:09:11 +0100, Roel Kluin wrote:
> >> Also I noted that in wait_for_pin() on a timeout, dependent on the
> >> status, still a handle_lab(adap, status) and return -EINTR may occur,
> >> which I think are wrong.
> > 
> > Depends on the hardware implementation. If I2C_PCF_LAB can be set while
> > I2C_PCF_PIN is set then indeed this is wrong. But I suspect the
> > hardware is such that I2C_PCF_LAB can't be set without I2C_PCF_PIN
> > being clear. You'd have to check the PCF8584 datasheet to make sure.
> 
> Regardless of the hardware implementation, if there is a timeout, that
> is a reason to return -ETIMEDOUT, we can test later if not. That is the
> sensible order in my opinion.

This can be argued the other way around just as well: if there is a
lost arbitration condition, it should be handled, regardless of the
timeout condition.

I you don't have a PCF8584-based adapter to test, and aren't going to
read the datasheet to see how the device is supposed to behave, and
have no proof that the current code is buggy, than please don't touch
it.

> (...)
> cleanup whitespace, fix comments and remove the unused STUB_I2C.
> 
> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

It seems you have been a bit overzealous. I now get the following
warning when building the i2c-algo-pcf driver:

drivers/i2c/algos/i2c-algo-pcf.c: In function ‘pcf_xfer’:
drivers/i2c/algos/i2c-algo-pcf.c:377: warning: suggest explicit braces to avoid ambiguous ‘else’
drivers/i2c/algos/i2c-algo-pcf.c:387: warning: suggest explicit braces to avoid ambiguous ‘else’

> @@ -313,24 +299,26 @@ static int pcf_doAddress(struct i2c_algo_pcf_data *adap,
>  	unsigned char addr;
>  
>  	addr = msg->addr << 1;
> +
>  	if (flags & I2C_M_RD)
>  		addr |= 1;
>  	if (flags & I2C_M_REV_DIR_ADDR)
>  		addr ^= 1;
> +
>  	i2c_outb(adap, addr);
>  
>  	return 0;
>  }

Not sure why you want to add blank lines there.

All the rest looks OK to me. Please fix at least the warnings and
resubmit.

-- 
Jean Delvare

[PATCH 1/2 v2] i2c,algo: cleanup i2c-algo-pcf.c

[Roel Kluin]

Jean Delvare wrote:
> It seems you have been a bit overzealous. I now get the following
> warning when building the i2c-algo-pcf driver:
> 
> drivers/i2c/algos/i2c-algo-pcf.c: In function ‘pcf_xfer’:
> drivers/i2c/algos/i2c-algo-pcf.c:377: warning: suggest explicit braces to avoid ambiguous ‘else’
> drivers/i2c/algos/i2c-algo-pcf.c:387: warning: suggest explicit braces to avoid ambiguous ‘else’

should be fixed now.

>> @@ -313,24 +299,26 @@ static int pcf_doAddress(struct i2c_algo_pcf_data *adap,
>>  	unsigned char addr;
>>  
>>  	addr = msg->addr << 1;
>> +
>>  	if (flags & I2C_M_RD)
>>  		addr |= 1;
>>  	if (flags & I2C_M_REV_DIR_ADDR)
>>  		addr ^= 1;
>> +
>>  	i2c_outb(adap, addr);
>>  
>>  	return 0;
>>  }
> 
> Not sure why you want to add blank lines there.

Ok, removed.

------------------>8-----------------8<-------------------------
cleanup whitespace, fix comments and remove the unused STUB_I2C.

Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 3e01992..ce916d7 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -1,31 +1,30 @@
-/* ------------------------------------------------------------------------- */
-/* i2c-algo-pcf.c i2c driver algorithms for PCF8584 adapters		     */
-/* ------------------------------------------------------------------------- */
-/*   Copyright (C) 1995-1997 Simon G. Vogl
-                   1998-2000 Hans Berglund
-
-    This program is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    This program is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with this program; if not, write to the Free Software
-    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.		     */
-/* ------------------------------------------------------------------------- */
-
-/* With some changes from Kyösti Mälkki <kmalkki-kf+aQKke1yb1KXRcyAk9cg@public.gmane.org> and
-   Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org> ,and also from Martin Bailey
-   <mbailey-0IcqLCOdkweesDZOCDqTHtBPR1lH4CV8@public.gmane.org> */
-
-/* Partially rewriten by Oleg I. Vdovikin <vdovikin-EllQnLNTEGo@public.gmane.org> to handle multiple
-   messages, proper stop/repstart signaling during receive,
-   added detect code */
+/*
+ * i2c-algo-pcf.c i2c driver algorithms for PCF8584 adapters
+ *
+ *   Copyright (C) 1995-1997 Simon G. Vogl
+ *		   1998-2000 Hans Berglund
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * With some changes from Kyösti Mälkki <kmalkki-kf+aQKke1yb1KXRcyAk9cg@public.gmane.org> and
+ * Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org>, and also from Martin Bailey
+ * <mbailey-0IcqLCOdkweesDZOCDqTHtBPR1lH4CV8@public.gmane.org>
+ *
+ * Partially rewriten by Oleg I. Vdovikin <vdovikin-EllQnLNTEGo@public.gmane.org> to handle multiple
+ * messages, proper stop/repstart signaling during receive, added detect code
+ */
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -38,17 +37,18 @@
 #include "i2c-algo-pcf.h"
 
 
-#define DEB2(x) if (i2c_debug>=2) x
-#define DEB3(x) if (i2c_debug>=3) x /* print several statistical values*/
-#define DEBPROTO(x) if (i2c_debug>=9) x;
- 	/* debug the protocol by showing transferred bits */
+#define DEB2(x) if (i2c_debug >= 2) x
+#define DEB3(x) if (i2c_debug >= 3) x /* print several statistical values */
+#define DEBPROTO(x) if (i2c_debug >= 9) x;
+	/* debug the protocol by showing transferred bits */
 #define DEF_TIMEOUT 16
 
-/* module parameters:
+/*
+ * module parameters:
  */
 static int i2c_debug;
 
-/* --- setting states on the bus with the right timing: ---------------	*/
+/* setting states on the bus with the right timing: */
 
 #define set_pcf(adap, ctl, val) adap->setpcf(adap->data, ctl, val)
 #define get_pcf(adap, ctl) adap->getpcf(adap->data, ctl)
@@ -57,22 +57,21 @@ static int i2c_debug;
 #define i2c_outb(adap, val) adap->setpcf(adap->data, 0, val)
 #define i2c_inb(adap) adap->getpcf(adap->data, 0)
 
-/* --- other auxiliary functions --------------------------------------	*/
+/* other auxiliary functions */
 
-static void i2c_start(struct i2c_algo_pcf_data *adap) 
+static void i2c_start(struct i2c_algo_pcf_data *adap)
 {
 	DEBPROTO(printk("S "));
 	set_pcf(adap, 1, I2C_PCF_START);
 }
 
-static void i2c_repstart(struct i2c_algo_pcf_data *adap) 
+static void i2c_repstart(struct i2c_algo_pcf_data *adap)
 {
 	DEBPROTO(printk(" Sr "));
 	set_pcf(adap, 1, I2C_PCF_REPSTART);
 }
 
-
-static void i2c_stop(struct i2c_algo_pcf_data *adap) 
+static void i2c_stop(struct i2c_algo_pcf_data *adap)
 {
 	DEBPROTO(printk("P\n"));
 	set_pcf(adap, 1, I2C_PCF_STOP);
@@ -82,17 +81,17 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
 {
 	DEB2(printk(KERN_INFO
 		"i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n",
-		 *status));
-
-	/* Cleanup from LAB -- reset and enable ESO.
+		*status));
+	/*
+	 * Cleanup from LAB -- reset and enable ESO.
 	 * This resets the PCF8584; since we've lost the bus, no
 	 * further attempts should be made by callers to clean up
 	 * (no i2c_stop() etc.)
 	 */
 	set_pcf(adap, 1, I2C_PCF_PIN);
 	set_pcf(adap, 1, I2C_PCF_ESO);
-
-	/* We pause for a time period sufficient for any running
+	/*
+	 * We pause for a time period sufficient for any running
 	 * I2C transaction to complete -- the arbitration logic won't
 	 * work properly until the next START is seen.
 	 * It is assumed the bus driver or client has set a proper value.
@@ -108,48 +107,48 @@ static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status)
 		get_pcf(adap, 1)));
 }
 
-static int wait_for_bb(struct i2c_algo_pcf_data *adap) {
+static int wait_for_bb(struct i2c_algo_pcf_data *adap)
+{
 
 	int timeout = DEF_TIMEOUT;
 	int status;
 
 	status = get_pcf(adap, 1);
-#ifndef STUB_I2C
+
 	while (timeout-- && !(status & I2C_PCF_BB)) {
 		udelay(100); /* wait for 100 us */
 		status = get_pcf(adap, 1);
 	}
-#endif
-	if (timeout <= 0) {
+
+	if (timeout <= 0)
 		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
-	}
-	
-	return (timeout<=0);
-}
 
+	return timeout <= 0;
+}
 
-static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status) {
+static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
+{
 
 	int timeout = DEF_TIMEOUT;
 
 	*status = get_pcf(adap, 1);
-#ifndef STUB_I2C
+
 	while (timeout-- && (*status & I2C_PCF_PIN)) {
 		adap->waitforpin(adap->data);
 		*status = get_pcf(adap, 1);
 	}
 	if (*status & I2C_PCF_LAB) {
 		handle_lab(adap, status);
-		return(-EINTR);
+		return -EINTR;
 	}
-#endif
+
 	if (timeout <= 0)
-		return(-1);
+		return -1;
 	else
-		return(0);
+		return 0;
 }
 
-/* 
+/*
  * This should perform the 'PCF8584 initialization sequence' as described
  * in the Philips IC12 data book (1995, Aug 29).
  * There should be a 30 clock cycle wait after reset, I assume this
@@ -164,18 +163,21 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 {
 	unsigned char temp;
 
-	DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: PCF state 0x%02x\n", get_pcf(adap, 1)));
+	DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: PCF state 0x%02x\n",
+				get_pcf(adap, 1)));
 
 	/* S1=0x80: S0 selected, serial interface off */
 	set_pcf(adap, 1, I2C_PCF_PIN);
-	/* check to see S1 now used as R/W ctrl -
-	   PCF8584 does that when ESO is zero */
+	/*
+	 * check to see S1 now used as R/W ctrl -
+	 * PCF8584 does that when ESO is zero
+	 */
 	if (((temp = get_pcf(adap, 1)) & 0x7f) != (0)) {
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't select S0 (0x%02x).\n", temp));
 		return -ENXIO; /* definetly not PCF8584 */
 	}
 
-	/* load own address in S0, effective address is (own << 1)	*/
+	/* load own address in S0, effective address is (own << 1) */
 	i2c_outb(adap, get_own(adap));
 	/* check it's really written */
 	if ((temp = i2c_inb(adap)) != get_own(adap)) {
@@ -183,7 +185,7 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		return -ENXIO;
 	}
 
-	/* S1=0xA0, next byte in S2					*/
+	/* S1=0xA0, next byte in S2 */
 	set_pcf(adap, 1, I2C_PCF_PIN | I2C_PCF_ES1);
 	/* check to see S2 now selected */
 	if (((temp = get_pcf(adap, 1)) & 0x7f) != I2C_PCF_ES1) {
@@ -191,7 +193,7 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		return -ENXIO;
 	}
 
-	/* load clock register S2					*/
+	/* load clock register S2 */
 	i2c_outb(adap, get_clock(adap));
 	/* check it's really written, the only 5 lowest bits does matter */
 	if (((temp = i2c_inb(adap)) & 0x1f) != get_clock(adap)) {
@@ -199,7 +201,7 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		return -ENXIO;
 	}
 
-	/* Enable serial interface, idle, S0 selected			*/
+	/* Enable serial interface, idle, S0 selected */
 	set_pcf(adap, 1, I2C_PCF_IDLE);
 
 	/* check to see PCF is really idled and we can access status register */
@@ -207,57 +209,47 @@ static int pcf_init_8584 (struct i2c_algo_pcf_data *adap)
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: PCF detection failed -- can't select S1` (0x%02x).\n", temp));
 		return -ENXIO;
 	}
-	
+
 	printk(KERN_DEBUG "i2c-algo-pcf.o: detected and initialized PCF8584.\n");
 
 	return 0;
 }
 
-
-/* ----- Utility functions
- */
-
 static int pcf_sendbytes(struct i2c_adapter *i2c_adap, const char *buf,
-                         int count, int last)
+			 int count, int last)
 {
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
 	int wrcount, status, timeout;
-    
+
 	for (wrcount=0; wrcount<count; ++wrcount) {
 		DEB2(dev_dbg(&i2c_adap->dev, "i2c_write: writing %2.2X\n",
-				buf[wrcount]&0xff));
+				buf[wrcount] & 0xff));
 		i2c_outb(adap, buf[wrcount]);
 		timeout = wait_for_pin(adap, &status);
 		if (timeout) {
-			if (timeout == -EINTR) {
-				/* arbitration lost */
-				return -EINTR;
-			}
+			if (timeout == -EINTR)
+				return -EINTR; /* arbitration lost */
+
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "i2c_write: error - timeout.\n");
 			return -EREMOTEIO; /* got a better one ?? */
 		}
-#ifndef STUB_I2C
 		if (status & I2C_PCF_LRB) {
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "i2c_write: error - no ack.\n");
 			return -EREMOTEIO; /* got a better one ?? */
 		}
-#endif
 	}
-	if (last) {
+	if (last)
 		i2c_stop(adap);
-	}
-	else {
+	else
 		i2c_repstart(adap);
-	}
 
-	return (wrcount);
+	return wrcount;
 }
 
-
 static int pcf_readbytes(struct i2c_adapter *i2c_adap, char *buf,
-                         int count, int last)
+			 int count, int last)
 {
 	int i, status;
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
@@ -267,42 +259,36 @@ static int pcf_readbytes(struct i2c_adapter *i2c_adap, char *buf,
 	for (i = 0; i <= count; i++) {
 
 		if ((wfp = wait_for_pin(adap, &status))) {
-			if (wfp == -EINTR) {
-				/* arbitration lost */
-				return -EINTR;
-			}
+			if (wfp == -EINTR)
+				return -EINTR; /* arbitration lost */
+
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "pcf_readbytes timed out.\n");
-			return (-1);
+			return -1;
 		}
 
-#ifndef STUB_I2C
 		if ((status & I2C_PCF_LRB) && (i != count)) {
 			i2c_stop(adap);
 			dev_err(&i2c_adap->dev, "i2c_read: i2c_inb, No ack.\n");
-			return (-1);
+			return -1;
 		}
-#endif
-		
+
 		if (i == count - 1) {
 			set_pcf(adap, 1, I2C_PCF_ESO);
-		} else 
-		if (i == count) {
-			if (last) {
+		} else if (i == count) {
+			if (last)
 				i2c_stop(adap);
-			} else {
+			else
 				i2c_repstart(adap);
-			}
-		};
+		}
 
-		if (i) {
+		if (i)
 			buf[i - 1] = i2c_inb(adap);
-		} else {
+		else
 			i2c_inb(adap); /* dummy read */
-		}
 	}
 
-	return (i - 1);
+	return i - 1;
 }
 
 
@@ -323,14 +309,14 @@ static int pcf_doAddress(struct i2c_algo_pcf_data *adap,
 }
 
 static int pcf_xfer(struct i2c_adapter *i2c_adap,
-		    struct i2c_msg *msgs, 
+		    struct i2c_msg *msgs,
 		    int num)
 {
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
 	struct i2c_msg *pmsg;
 	int i;
 	int ret=0, timeout, status;
-    
+
 	if (adap->xfer_begin)
 		adap->xfer_begin(adap->data);
 
@@ -338,25 +324,24 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 	timeout = wait_for_bb(adap);
 	if (timeout) {
 		DEB2(printk(KERN_ERR "i2c-algo-pcf.o: "
-		            "Timeout waiting for BB in pcf_xfer\n");)
+			    "Timeout waiting for BB in pcf_xfer\n");)
 		i = -EIO;
 		goto out;
 	}
-	
+
 	for (i = 0;ret >= 0 && i < num; i++) {
 		pmsg = &msgs[i];
 
 		DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: Doing %s %d bytes to 0x%02x - %d of %d messages\n",
 		     pmsg->flags & I2C_M_RD ? "read" : "write",
-                     pmsg->len, pmsg->addr, i + 1, num);)
-    
+		     pmsg->len, pmsg->addr, i + 1, num);)
+
 		ret = pcf_doAddress(adap, pmsg);
 
 		/* Send START */
-		if (i == 0) {
-			i2c_start(adap); 
-		}
-    
+		if (i == 0)
+			i2c_start(adap);
+
 		/* Wait for PIN (pending interrupt NOT) */
 		timeout = wait_for_pin(adap, &status);
 		if (timeout) {
@@ -371,8 +356,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 			i = -EREMOTEIO;
 			goto out;
 		}
-    
-#ifndef STUB_I2C
+
 		/* Check LRB (last rcvd bit - slave ack) */
 		if (status & I2C_PCF_LRB) {
 			i2c_stop(adap);
@@ -380,57 +364,55 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 			i = -EREMOTEIO;
 			goto out;
 		}
-#endif
-    
+
 		DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: Msg %d, addr=0x%x, flags=0x%x, len=%d\n",
 			    i, msgs[i].addr, msgs[i].flags, msgs[i].len);)
-    
-		/* Read */
+
 		if (pmsg->flags & I2C_M_RD) {
-			/* read bytes into buffer*/
 			ret = pcf_readbytes(i2c_adap, pmsg->buf, pmsg->len,
-                                            (i + 1 == num));
-        
+					    (i + 1 == num));
+
 			if (ret != pmsg->len) {
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
 					    "only read %d bytes.\n",ret));
 			} else {
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: read %d bytes.\n",ret));
 			}
-		} else { /* Write */
+
+		} else {
 			ret = pcf_sendbytes(i2c_adap, pmsg->buf, pmsg->len,
-                                            (i + 1 == num));
-        
+					    (i + 1 == num));
+
 			if (ret != pmsg->len) {
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
 					    "only wrote %d bytes.\n",ret));
 			} else {
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: wrote %d bytes.\n",ret));
 			}
+
 		}
 	}
 
 out:
 	if (adap->xfer_end)
 		adap->xfer_end(adap->data);
-	return (i);
+	return i;
 }
 
 static u32 pcf_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | 
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
 	       I2C_FUNC_PROTOCOL_MANGLING;
 }
 
-/* -----exported algorithm data: -------------------------------------	*/
-
+/* exported algorithm data: */
 static const struct i2c_algorithm pcf_algo = {
 	.master_xfer	= pcf_xfer,
 	.functionality	= pcf_func,
 };
 
-/* 
- * registering functions to load algorithms at runtime 
+/*
+ * registering functions to load algorithms at runtime
  */
 int i2c_pcf_add_bus(struct i2c_adapter *adap)
 {
@@ -458,4 +440,4 @@ MODULE_LICENSE("GPL");
 
 module_param(i2c_debug, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(i2c_debug,
-        "debug level - 0 off; 1 normal; 2,3 more verbose; 9 pcf-protocol");
+	"debug level - 0 off; 1 normal; 2,3 more verbose; 9 pcf-protocol");

Re: [PATCH 1/2 v2] i2c,algo: cleanup i2c-algo-pcf.c

[Jean Delvare]

Hi Roel,

On Wed, 04 Feb 2009 17:07:16 +0100, Roel Kluin wrote:
> Jean Delvare wrote:
> > It seems you have been a bit overzealous. I now get the following
> > warning when building the i2c-algo-pcf driver:
> > 
> > drivers/i2c/algos/i2c-algo-pcf.c: In function 'pcf_xfer':
> > drivers/i2c/algos/i2c-algo-pcf.c:377: warning: suggest explicit braces to avoid ambiguous 'else'
> > drivers/i2c/algos/i2c-algo-pcf.c:387: warning: suggest explicit braces to avoid ambiguous 'else'
> 
> should be fixed now.
> 
> >> @@ -313,24 +299,26 @@ static int pcf_doAddress(struct i2c_algo_pcf_data *adap,
> >>  	unsigned char addr;
> >>  
> >>  	addr = msg->addr << 1;
> >> +
> >>  	if (flags & I2C_M_RD)
> >>  		addr |= 1;
> >>  	if (flags & I2C_M_REV_DIR_ADDR)
> >>  		addr ^= 1;
> >> +
> >>  	i2c_outb(adap, addr);
> >>  
> >>  	return 0;
> >>  }
> > 
> > Not sure why you want to add blank lines there.
> 
> Ok, removed.
> 
> ------------------>8-----------------8<-------------------------
> cleanup whitespace, fix comments and remove the unused STUB_I2C.
> 
> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Applied, thanks.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c,algo: cleanup i2c-algo-pcf.c

[Jean Delvare]

Hi Roel,

Adding back the linux-i2c list in the loop...

On Wed, 4 Feb 2009 16:54:20 +0100, roel kluin wrote:
> Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>:
> > Depends on the hardware implementation. If I2C_PCF_LAB can be set while
> > I2C_PCF_PIN is set then indeed this is wrong. But I suspect the
> > hardware is such that I2C_PCF_LAB can't be set without I2C_PCF_PIN
> > being clear. You'd have to check the PCF8584 datasheet to make sure.
> 
> I took a look at the datasheet, and I think this is not so:
> 
> From Fig 1: PIN and LAB are control status registers (s1)
> 
> From section 6.8.1.1, PIN (Pending Interrupt Not):
> "When the PIN bit is written with a logic 1, all status bits are reset
> to logic 0.
> [...] PIN is the only bit in S1 which may be both read and written to."
> 
> Later in 6.8.2.1: PIN bit:
> Each time a serial data transmission is initiated [...], the PIN bit
> will be set to
> logic 1 automatically (inactive).
> [...]
> After transmission or reception of one byte on the I2C-bus, the PIN bit will be
> automatically reset to logic 0 (active) indicating a complete byte
> transmission/reception.
> 
> and in 6.8.2.6 LAB:
> 'Lost Arbitration' Bit. This bit is set when, in multi-master operation,
> arbitration is lost to another master on the I2C-bus.
> 
> Note: setting I2C_PCF_PIN clears bits, including I2C_PCF_LAB, but
> unsetting does not.
> 
> now consider the function
> 
> static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
> {
> 
>         int timeout = DEF_TIMEOUT;
> 
>         *status = get_pcf(adap, 1);
> 
>         while (timeout-- && (*status & I2C_PCF_PIN)) {
>                 adap->waitforpin(adap->data);
>                 *status = get_pcf(adap, 1);
>         }
>         if (*status & I2C_PCF_LAB) {
>                 handle_lab(adap, status);
>                 return -EINTR;
>         }
> 
>         if (timeout <= 0)
>                 return -1;
>         else
>                 return 0;
> }
> 
> We wait until I2C_PCF_PIN is set (transmission/reception was succesful),
> but we want to ensure that arbitration is not lost to another master on
> the I2C-bus.
> 
> > I you don't have a PCF8584-based adapter to test, and aren't going to
> > read the datasheet to see how the device is supposed to behave, and
> > have no proof that the current code is buggy, than please don't touch
> > it.
> 
> I do not have a PCF8584-based adapter, but according to the datasheet
> it does seem wrong. I do think the timeout-based return should go first.

I fail to see how you come to this conclusion based on the datasheet
elements you listed above. Anyway, I suspect that lost arbitration and
timeout can't happen at the same time so it doesn't really matter what
test is done first.

If you really want to change the code, please discuss it with Eric
Brower and Dan Smolik (Cc'd). They worked on arbitration loss in
i2c-algo-pcf back in July 2008, so presumably they have systems where
they can test this specific condition.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c,algo: cleanup i2c-algo-pcf.c

[Eric Brower]

On Thu, Feb 5, 2009 at 12:38 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Roel,
>
> Adding back the linux-i2c list in the loop...
>
> On Wed, 4 Feb 2009 16:54:20 +0100, roel kluin wrote:
>> Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>:
>> > Depends on the hardware implementation. If I2C_PCF_LAB can be set while
>> > I2C_PCF_PIN is set then indeed this is wrong. But I suspect the
>> > hardware is such that I2C_PCF_LAB can't be set without I2C_PCF_PIN
>> > being clear. You'd have to check the PCF8584 datasheet to make sure.
>>
>> I took a look at the datasheet, and I think this is not so:
>>
>> From Fig 1: PIN and LAB are control status registers (s1)
>>
>> From section 6.8.1.1, PIN (Pending Interrupt Not):
>> "When the PIN bit is written with a logic 1, all status bits are reset
>> to logic 0.
>> [...] PIN is the only bit in S1 which may be both read and written to."
>>
>> Later in 6.8.2.1: PIN bit:
>> Each time a serial data transmission is initiated [...], the PIN bit
>> will be set to
>> logic 1 automatically (inactive).
>> [...]
>> After transmission or reception of one byte on the I2C-bus, the PIN bit will be
>> automatically reset to logic 0 (active) indicating a complete byte
>> transmission/reception.
>>
>> and in 6.8.2.6 LAB:
>> 'Lost Arbitration' Bit. This bit is set when, in multi-master operation,
>> arbitration is lost to another master on the I2C-bus.
>>
>> Note: setting I2C_PCF_PIN clears bits, including I2C_PCF_LAB, but
>> unsetting does not.
>>
>> now consider the function
>>
>> static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
>> {
>>
>>         int timeout = DEF_TIMEOUT;
>>
>>         *status = get_pcf(adap, 1);
>>
>>         while (timeout-- && (*status & I2C_PCF_PIN)) {
>>                 adap->waitforpin(adap->data);
>>                 *status = get_pcf(adap, 1);
>>         }
>>         if (*status & I2C_PCF_LAB) {
>>                 handle_lab(adap, status);
>>                 return -EINTR;
>>         }
>>
>>         if (timeout <= 0)
>>                 return -1;
>>         else
>>                 return 0;
>> }
>>
>> We wait until I2C_PCF_PIN is set (transmission/reception was succesful),
>> but we want to ensure that arbitration is not lost to another master on
>> the I2C-bus.
>>
>> > I you don't have a PCF8584-based adapter to test, and aren't going to
>> > read the datasheet to see how the device is supposed to behave, and
>> > have no proof that the current code is buggy, than please don't touch
>> > it.
>>
>> I do not have a PCF8584-based adapter, but according to the datasheet
>> it does seem wrong. I do think the timeout-based return should go first.
>
> I fail to see how you come to this conclusion based on the datasheet
> elements you listed above. Anyway, I suspect that lost arbitration and
> timeout can't happen at the same time so it doesn't really matter what
> test is done first.
>
> If you really want to change the code, please discuss it with Eric
> Brower and Dan Smolik (Cc'd). They worked on arbitration loss in
> i2c-algo-pcf back in July 2008, so presumably they have systems where
> they can test this specific condition.
>
> --
> Jean Delvare
>

I do have access to such a system (for some unknown duration of time)
and I am patient enough to wait for LAB conditions, so if you'd like
to specify a test case, Roel, I'm happy to give it a whirl.

-- 
E

[PATCH 2/2 v2] i2c,algo: handle timeout correctly

[Roel Kluin]

Let's first adress the other issues at hand.
This patch should be applied on top of my previous cleanup patch

--------------------------->8---------------------8<---------------------------
With a postfix decrement these timeouts reach -1 rather than 0, but after the
loop it is tested whether they have become 0.

As pointed out by Jean Delvare, the msg_num should be tested before the timeout.
With the current order, you could exit with a timeout error while all the
messages were successfully transferred.

Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index ce916d7..2862f11 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -115,15 +115,17 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap)
 
 	status = get_pcf(adap, 1);
 
-	while (timeout-- && !(status & I2C_PCF_BB)) {
+	while (!(status & I2C_PCF_BB) && --timeout) {
 		udelay(100); /* wait for 100 us */
 		status = get_pcf(adap, 1);
 	}
 
-	if (timeout <= 0)
+	if (timeout == 0) {
 		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
+		return -ETIMEDOUT;
+	}
 
-	return timeout <= 0;
+	return 0;
 }
 
 static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
@@ -133,7 +135,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
 
 	*status = get_pcf(adap, 1);
 
-	while (timeout-- && (*status & I2C_PCF_PIN)) {
+	while ((*status & I2C_PCF_PIN) && --timeout) {
 		adap->waitforpin(adap->data);
 		*status = get_pcf(adap, 1);
 	}
@@ -142,10 +144,10 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
 		return -EINTR;
 	}
 
-	if (timeout <= 0)
-		return -1;
-	else
-		return 0;
+	if (timeout == 0)
+		return -ETIMEDOUT;
+
+	return 0;
 }
 
 /*

Re: [PATCH 2/2 v2] i2c,algo: handle timeout correctly

[Jean Delvare]

On Fri, 06 Feb 2009 14:11:46 +0100, Roel Kluin wrote:
> Let's first adress the other issues at hand.
> This patch should be applied on top of my previous cleanup patch

Agreed.

> 
> --------------------------->8---------------------8<---------------------------
> With a postfix decrement these timeouts reach -1 rather than 0, but after the
> loop it is tested whether they have become 0.
> 
> As pointed out by Jean Delvare, the msg_num should be tested before the timeout.
> With the current order, you could exit with a timeout error while all the
> messages were successfully transferred.
> 
> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
> index ce916d7..2862f11 100644
> --- a/drivers/i2c/algos/i2c-algo-pcf.c
> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
> @@ -115,15 +115,17 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap)
>  
>  	status = get_pcf(adap, 1);
>  
> -	while (timeout-- && !(status & I2C_PCF_BB)) {
> +	while (!(status & I2C_PCF_BB) && --timeout) {
>  		udelay(100); /* wait for 100 us */
>  		status = get_pcf(adap, 1);
>  	}
>  
> -	if (timeout <= 0)
> +	if (timeout == 0) {
>  		printk(KERN_ERR "Timeout waiting for Bus Busy\n");
> +		return -ETIMEDOUT;
> +	}
>  
> -	return timeout <= 0;
> +	return 0;
>  }
>  
>  static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
> @@ -133,7 +135,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
>  
>  	*status = get_pcf(adap, 1);
>  
> -	while (timeout-- && (*status & I2C_PCF_PIN)) {
> +	while ((*status & I2C_PCF_PIN) && --timeout) {
>  		adap->waitforpin(adap->data);
>  		*status = get_pcf(adap, 1);
>  	}
> @@ -142,10 +144,10 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
>  		return -EINTR;
>  	}
>  
> -	if (timeout <= 0)
> -		return -1;
> -	else
> -		return 0;
> +	if (timeout == 0)
> +		return -ETIMEDOUT;
> +
> +	return 0;
>  }
>  
>  /*

Applied, thanks. It would be great if Eric (or anyone with a
PCF8584-based adapter) could test the two patches, just to make sure we
didn't accidentally break anything:

ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-algo-pcf-01-style-cleanups.patch
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-algo-pcf-02-handle-timeout-correctly.patch

-- 
Jean Delvare

Re: [PATCH 2/2 v2] i2c,algo: handle timeout correctly

[Eric Brower]

On Fri, Feb 6, 2009 at 12:51 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Fri, 06 Feb 2009 14:11:46 +0100, Roel Kluin wrote:
>> Let's first adress the other issues at hand.
>> This patch should be applied on top of my previous cleanup patch
>
> Agreed.
>
>>
>> --------------------------->8---------------------8<---------------------------
>> With a postfix decrement these timeouts reach -1 rather than 0, but after the
>> loop it is tested whether they have become 0.
>>
>> As pointed out by Jean Delvare, the msg_num should be tested before the timeout.
>> With the current order, you could exit with a timeout error while all the
>> messages were successfully transferred.
>>
>> Signed-off-by: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
>> index ce916d7..2862f11 100644
>> --- a/drivers/i2c/algos/i2c-algo-pcf.c
>> +++ b/drivers/i2c/algos/i2c-algo-pcf.c
>> @@ -115,15 +115,17 @@ static int wait_for_bb(struct i2c_algo_pcf_data *adap)
>>
>>       status = get_pcf(adap, 1);
>>
>> -     while (timeout-- && !(status & I2C_PCF_BB)) {
>> +     while (!(status & I2C_PCF_BB) && --timeout) {
>>               udelay(100); /* wait for 100 us */
>>               status = get_pcf(adap, 1);
>>       }
>>
>> -     if (timeout <= 0)
>> +     if (timeout == 0) {
>>               printk(KERN_ERR "Timeout waiting for Bus Busy\n");
>> +             return -ETIMEDOUT;
>> +     }
>>
>> -     return timeout <= 0;
>> +     return 0;
>>  }
>>
>>  static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
>> @@ -133,7 +135,7 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
>>
>>       *status = get_pcf(adap, 1);
>>
>> -     while (timeout-- && (*status & I2C_PCF_PIN)) {
>> +     while ((*status & I2C_PCF_PIN) && --timeout) {
>>               adap->waitforpin(adap->data);
>>               *status = get_pcf(adap, 1);
>>       }
>> @@ -142,10 +144,10 @@ static int wait_for_pin(struct i2c_algo_pcf_data *adap, int *status)
>>               return -EINTR;
>>       }
>>
>> -     if (timeout <= 0)
>> -             return -1;
>> -     else
>> -             return 0;
>> +     if (timeout == 0)
>> +             return -ETIMEDOUT;
>> +
>> +     return 0;
>>  }
>>
>>  /*
>
> Applied, thanks. It would be great if Eric (or anyone with a
> PCF8584-based adapter) could test the two patches, just to make sure we
> didn't accidentally break anything:
>
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-algo-pcf-01-style-cleanups.patch
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-algo-pcf-02-handle-timeout-correctly.patch
>
> --
> Jean Delvare
>

Ack'ed and Ack'ed.

Basic operation seems fine, and two LAB events were properly detected
and handled (best as one can tell, without a bus analyzer...).

-- 
E