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

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

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