[i2c] [patch 2.6.24-rc1-git] i2c-algo-bit, don't yield()

[Jean Delvare]

Hi David,

On Thu, 15 Nov 2007 00:43:32 -0800, David Brownell wrote:
> On Tuesday 06 November 2007, Jean Delvare wrote:
> > 	This automatic retry thing is misimplemented in at least
> > two bus drivers, and causes delays for everyone (e.g. when framebuffer
> > drivers probe for EDID EEPROMs). That's enough harm to warrant a swift
> > removal. If anybody really cares, we'll discuss the matter with them
> > when they complain.
> 
> OK ... here's a patch removing it from i2c-algo-bit
> (and thence its users).

Great, thanks.

> 
> - Dave
> 
> =============
> Remove the "retry address stage" mechanism from i2c-algo-bit.  That
> mechanism is not something that upper level code can rely on; hardly
> any I2C adapter drivers implement it, much less do it correctly.

Adapters which do not implement it, or do not implement it correctly,
do _not_ use i2c-algo-bit, so that comment is hardly relevant for a
patch that only touches i2c-algo-bit. It would be more suitable for the
future patch that will remove the structure field.

> Plus the notion itself is dubious.

Not sure what you mean here. The intent is rather clear: as you
underlined yourself below, I2C chips don't have to ack the address byte
if they're busy.

> 
> Return any actual fault code returned in preferece to -EREMOTEIO, when
> trying to address a specific device.

Typo: preference.

> 
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |   54 ++++++++++++---------------------------
>  1 files changed, 17 insertions(+), 37 deletions(-)
> 
> --- a/drivers/i2c/algos/i2c-algo-bit.c	2007-11-14 21:36:34.000000000 -0800
> +++ b/drivers/i2c/algos/i2c-algo-bit.c	2007-11-14 21:36:34.000000000 -0800
> @@ -309,36 +309,17 @@ bailout:
>  /* ----- Utility functions
>   */
>  
> -/* try_address tries to contact a chip for a number of
> - * times before it gives up.
> +/* try_address tries to contact a chip.  SMBus devices are required
> + * to respond; I2C devices don't need to, if they're too busy.
> + *
>   * return values:
>   * 1 chip answered
>   * 0 chip did not answer
> - * -x transmission error
> + * -x transmission error (or lost arbitration)
>   */
> -static int try_address(struct i2c_adapter *i2c_adap,
> -		       unsigned char addr, int retries)
> +static int try_address(struct i2c_adapter *i2c_adap, unsigned char addr)
>  {
> -	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> -	int i, ret = -1;
> -
> -	for (i = 0; i <= retries; i++) {
> -		ret = i2c_outb(i2c_adap, addr);
> -		if (ret == 1 || i == retries)
> -			break;
> -		bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n");
> -		i2c_stop(adap);
> -		udelay(adap->udelay);
> -		yield();
> -		bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
> -		i2c_start(adap);
> -	}
> -	if (i && ret)
> -		bit_dbg(1, &i2c_adap->dev, "Used %d tries to %s client at "
> -			"0x%02x: %s\n", i + 1,
> -			addr & 1 ? "read from" : "write to", addr >> 1,
> -			ret == 1 ? "success" : "failed, timeout?");
> -	return ret;
> +	return i2c_outb(i2c_adap, addr);
>  }

Just kill try_address and call i2c_outb() directly?

>  
>  static int sendbytes(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
> @@ -465,27 +446,25 @@ static int bit_doAddress(struct i2c_adap
>  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
>  
>  	unsigned char addr;
> -	int ret, retries;
> -
> -	retries = nak_ok ? 0 : i2c_adap->retries;
> +	int ret;
>  
>  	if (flags & I2C_M_TEN) {
>  		/* a ten bit address */
>  		addr = 0xf0 | ((msg->addr >> 7) & 0x03);
>  		bit_dbg(2, &i2c_adap->dev, "addr0: %d\n", addr);
>  		/* try extended address code...*/
> -		ret = try_address(i2c_adap, addr, retries);
> +		ret = try_address(i2c_adap, addr);
>  		if ((ret != 1) && !nak_ok)  {
>  			dev_err(&i2c_adap->dev,
>  				"died at extended address code\n");
> -			return -EREMOTEIO;
> +			goto fail;
>  		}
>  		/* the remaining 8 bit address */
>  		ret = i2c_outb(i2c_adap, msg->addr & 0x7f);
>  		if ((ret != 1) && !nak_ok) {
>  			/* the chip did not ack / xmission error occurred */
>  			dev_err(&i2c_adap->dev, "died at 2nd address code\n");
> -			return -EREMOTEIO;
> +			goto fail;
>  		}
>  		if (flags & I2C_M_RD) {
>  			bit_dbg(3, &i2c_adap->dev, "emitting repeated "
> @@ -493,11 +472,11 @@ static int bit_doAddress(struct i2c_adap
>  			i2c_repstart(adap);
>  			/* okay, now switch into reading mode */
>  			addr |= 0x01;
> -			ret = try_address(i2c_adap, addr, retries);
> +			ret = try_address(i2c_adap, addr);
>  			if ((ret != 1) && !nak_ok) {
>  				dev_err(&i2c_adap->dev,
>  					"died at repeated address code\n");
> -				return -EREMOTEIO;
> +				goto fail;
>  			}
>  		}
>  	} else {		/* normal 7bit address	*/
> @@ -506,12 +485,14 @@ static int bit_doAddress(struct i2c_adap
>  			addr |= 1;
>  		if (flags & I2C_M_REV_DIR_ADDR)
>  			addr ^= 1;
> -		ret = try_address(i2c_adap, addr, retries);
> +		ret = try_address(i2c_adap, addr);
>  		if ((ret != 1) && !nak_ok)
> -			return -EREMOTEIO;
> +			goto fail;
>  	}
>  
>  	return 0;
> +fail:
> +	return (ret < 0) ? ret : -EREMOTEIO;
>  }
>  
>  static int bit_xfer(struct i2c_adapter *i2c_adap,
> @@ -605,8 +586,7 @@ static int i2c_bit_prepare_bus(struct i2
>  	/* register new adapter to i2c module... */
>  	adap->algo = &i2c_bit_algo;
>  
> -	adap->timeout = 100;	/* default values, should	*/
> -	adap->retries = 3;	/* be replaced by defines	*/
> +	adap->timeout = 100;	/* default value */
>  
>  	return 0;
>  }

The rest looks good.

i2c-algo-pcf needs a similar update, can you please submit a patch?

On my side, I'll try to find out all the i2c adapter drivers that
set .retries but don't actually make use of it, so that they don't
break when we ultimately drop that field.

Thanks,
-- 
Jean Delvare