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@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
