[i2c] [PATCH] I2C bus driver for Philips ARM boards

Jean Delvare khali at linux-fr.org
Sat Sep 30 22:24:32 CEST 2006


Hi Vitaly,

> +static inline int wait_timeout(long timeout, struct i2c_pnx_algo_data *a)
> +{
> +	long tout = timeout;
> +	while (tout > 0 &&
> +			(ioread32(I2C_REG_STS(a)) & mstatus_active)) {
> +		mdelay(1);
> +		tout--;
> +	}
> +	return (tout <= 0);
> +}

I think there's something wrong here. You are calling this function in
several places now. It tests for mstatus_active, however in the
original code (previous patch) some code was testing for mstatus_active
and some for mstatus_reset. I doubt it's OK to change that. Shouldn't
you add a third parameter to this function to specify which bit you are
waiting for?

As a side note:
* One letter variable names are evil.
* You don't need the local "tout" variable above, you can work with
  "timeout" directly.

> +static inline int i2c_pnx_bus_busy(struct i2c_pnx_algo_data *a)
> +{
> +	return wait_timeout(I2C_PNX_TIMEOUT, a);
> +}

Isn't this function essantially redundant with wait_timeout?

-- 
Jean Delvare



More information about the i2c mailing list