[i2c] [PATCH] i2c-au1550: do i2c stop on errors

[Jean Delvare]

Hi Manuel,

Sorry for the late answer.

On Sat, 6 Oct 2007 18:14:34 +0200, Manuel Lauss wrote:
> From c1d94ed0881608ea1b2bcd4dcf7002d8d551437a Mon Sep 17 00:00:00 2001
> From: Manuel Lauss <mano at roarinelk.homelinux.net>
> Date: Sat, 6 Oct 2007 17:10:21 +0200
> Subject: [PATCH] i2c-au1550: do i2c stop on errors
> 
> The i2c-au1550.c driver does not do a stop condition in case of NAKs
> from the bus (non-existant slave/slave sent NAKs), which led to the
> RTC minute register on board being overwritten on subsequent transfers.
> 
> This fix however is suboptimal due to hardware constraints: The I2C
> hardware can only send a stop after it has sent/received 8 databits;
> this fix essentially does an extra byte transfer before doing the
> actual stop; however I have not yet seen any errors due to this.
> 
> Signed-off-by: Manuel Lauss <mano at roarinelk.homelinux.net>
> ---
>  drivers/i2c/busses/i2c-au1550.c |   37 ++++++++++++++++++++++++++++++-------
>  1 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-au1550.c b/drivers/i2c/busses/i2c-au1550.c
> index d7e7c35..df778eb 100644
> --- a/drivers/i2c/busses/i2c-au1550.c
> +++ b/drivers/i2c/busses/i2c-au1550.c
> @@ -180,6 +180,19 @@ wait_for_rx_byte(struct i2c_au1550_data *adap, u32 *ret_data)
>  	return 0;
>  }
>  
> +/* send a stop over I2C in case the transfer aborted abnormally.
> + * NOTE: This is very suboptimal: This function here will cause the hw
> + *	 to read a byte from / write zero to the bus and do the stop
> + *	 right after it;  the hw won't let us do it any other way.
> + */
> +static void i2c_au1550_stop(struct i2c_au1550_data *adap)
> +{
> +	volatile psc_smb_t *sp = (volatile psc_smb_t *)(adap->psc_base);
> +	sp->psc_smbtxrx = PSC_SMBTXRX_STP;
> +	au_sync();
> +	wait_master_done(adap);
> +}
> +
>  static int
>  i2c_read(struct i2c_au1550_data *adap, unsigned char *buf,
>  		    unsigned int len)
> @@ -203,7 +216,7 @@ i2c_read(struct i2c_au1550_data *adap, unsigned char *buf,
>  		sp->psc_smbtxrx = 0;
>  		au_sync();
>  		if (wait_for_rx_byte(adap, &data))
> -			return -EIO;
> +			goto out_err;
>  
>  		buf[i] = data;
>  		i++;
> @@ -214,12 +227,16 @@ i2c_read(struct i2c_au1550_data *adap, unsigned char *buf,
>  	sp->psc_smbtxrx = PSC_SMBTXRX_STP;
>  	au_sync();
>  	if (wait_master_done(adap))
> -		return -EIO;
> +		goto out_err;

I'm a bit skeptical about this one...

>  
>  	data = sp->psc_smbtxrx;
>  	au_sync();
>  	buf[i] = data;
>  	return 0;
> +
> +out_err:
> +	i2c_au1550_stop(adap);
> +	return -EIO;
>  }
>  
>  static int
> @@ -241,7 +258,7 @@ i2c_write(struct i2c_au1550_data *adap, unsigned char *buf,
>  		sp->psc_smbtxrx = data;
>  		au_sync();
>  		if (wait_ack(adap))
> -			return -EIO;
> +			goto out_err;
>  		i++;
>  	}
>  
> @@ -251,9 +268,12 @@ i2c_write(struct i2c_au1550_data *adap, unsigned char *buf,
>  	data |= PSC_SMBTXRX_STP;
>  	sp->psc_smbtxrx = data;
>  	au_sync();
> -	if (wait_master_done(adap))
> -		return -EIO;
> -	return 0;
> +	if (wait_master_done(adap) == 0)
> +		return 0;

... and this one. In both cases, you already sent the STOP condition as
part of the last byte read resp. written. A failure suggests that the
slave rejected the read or write, but does it also mean that the STOP
condition wasn't issued? I doubt it. If it were the case, then why
would a subsequent call to i2c_au1550_stop() succeed, while it's doing
exactly the same?

> +
> +out_err:
> +	i2c_au1550_stop(adap);
> +	return -EIO;
>  }
>  
>  static int
> @@ -266,8 +286,11 @@ au1550_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs, int num)
>  	for (i = 0; !err && i < num; i++) {
>  		p = &msgs[i];
>  		err = do_address(adap, p->addr, p->flags & I2C_M_RD);
> -		if (err || !p->len)
> +		if (err || ((!p->len) && (i == (num - 1)))) {
> +			/* slave NAK or no more data; need to do i2c stop */
> +			i2c_au1550_stop(adap);
>  			continue;
> +		}

Doesn't look correct. If you issue a STOP at this point, then you want
to break, NOT continue. It doesn't make a difference for the second
condition ((!p->len) && (i == (num - 1))) as this is the last
iteration, but it does make a big difference for the first condition
(err).

>  		if (p->flags & I2C_M_RD)
>  			err = i2c_read(adap, p->buf, p->len);
>  		else

BTW... We know that the AU1550 I2C module can't issue a STOP condition
without a data byte being read or written and this is a problem for
0-byte transactions. Did you try issuing the STOP condition directly in
do_address() in this case? I wonder if it would work. If it does, it
would be cleaner than your approach, I think.

-- 
Jean Delvare