linux-mips
[Top] [All Lists]

Re: [PATCH 1/2] i2c-au1550: send i2c stop on error #2

To: Manuel Lauss <mano@roarinelk.homelinux.net>
Subject: Re: [PATCH 1/2] i2c-au1550: send i2c stop on error #2
From: Jean Delvare <khali@linux-fr.org>
Date: Wed, 16 May 2007 15:38:22 +0200
Cc: i2c@lm-sensors.org, linux-mips@linux-mips.org, Domen Puncer <domen.puncer@ultra.si>
In-reply-to: <20070516053113.GA12986@roarinelk.homelinux.net>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20070516053113.GA12986@roarinelk.homelinux.net>
Sender: linux-mips-bounce@linux-mips.org
Hi Manuel,

On Wed, 16 May 2007 07:31:13 +0200, Manuel Lauss wrote:
> When the au1550 i2c driver encounteres an error while addressing a slave
> or has no data to send to a slave in the last i2c message, it returns to
> the upper layers without issuing a i2c stop condition.  This for example
> resulted in the minute register of the RTC on my board to be overwritten
> with a random value on a following transfer.
> 
> Fix the driver to send a stop over the i2c bus if one of the following
> 2 conditions are met:
> * error when addressing a slave
> * no data to send in the last i2c message
> 
> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
> 
> --- a/drivers/i2c/busses/i2c-au1550.c 2007-04-26 05:08:32.000000000 +0200
> +++ b/drivers/i2c/busses/i2c-au1550.c 2007-05-15 20:19:56.000000000 +0200
> @@ -260,13 +260,20 @@ static int
>  au1550_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs, int num)
>  {
>       struct i2c_au1550_data *adap = i2c_adap->algo_data;
> +     volatile psc_smb_t *sp = (volatile psc_smb_t *)(adap->psc_base);
>       struct i2c_msg *p;
>       int i, err = 0;
>  
>       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)))) {
> +                     sp->psc_smbtxrx = PSC_SMBTXRX_STP;
> +                     au_sync();
> +                     wait_master_done(adap);
> +                     continue;
> +             }
> +             if (!p->len)
>                       continue;
>               if (p->flags & I2C_M_RD)
>                       err = i2c_read(adap, p->buf, p->len);

Good catch. I'd have two comments though:

1* It looks to me like there are other error conditions which also
cause the driver to leave without issuing a stop condition on the bus:
if not all bytes of a write are acked by the target slave (in
i2c_write) or if the master receives less bytes than expected (in
i2c_read). I understand these are less likely to happen than the quick
write case which bit you, but shouldn't these bugs be fixed as well?

2* In i2c_write and i2c_read, the stop bit is always sent together with
the last byte, while your new code sends the stop bit on its own after
the address byte. Is it OK? I am wondering if your code isn't sending
an extra (0) byte after the address when asked to send a zero-byte
message. That would be bad. Do you have a bus analyzer or scope to
check what exactly is being sent on the bus in this case?

Domen, care to comment on this patch and/or my own comments?

Thanks,
-- 
Jean Delvare

<Prev in Thread] Current Thread [Next in Thread>