[U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c

This is against the freescale tree at opensource.freescale.com What is the status of this three? is it going into u-boot any time soon?
Jocke
From c96a4510db734a3b2a3096dfc84fdaf928d8bcc4 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund Joakim.Tjernlund@transmode.se Date: Wed, 31 Jan 2007 10:54:10 +0100 Subject: [PATCH] Fix empty i2c reads/writes, i2c_write(0x50, 0x00, 0, NULL, 0) which is used to se if an slave will ACK after receiving its address.
Correct i2c probing to use this method as the old method could upset a slave as it wrote a data byte to it.
Add a small delay in i2c_init() to let the controller shutdown any ongoing I2C activity.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- drivers/fsl_i2c.c | 29 +++++++++++++++-------------- 1 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/fsl_i2c.c b/drivers/fsl_i2c.c index c929096..7b7138f 100644 --- a/drivers/fsl_i2c.c +++ b/drivers/fsl_i2c.c @@ -58,6 +58,7 @@ i2c_init(int speed, int slaveadd) dev = (struct fsl_i2c *) (CFG_IMMR + CFG_I2C_OFFSET);
writeb(0, &dev->cr); /* stop I2C controller */ + udelay(5); /* let it shutdown in peace */ writeb(0x3F, &dev->fdr); /* set bus speed */ writeb(0x3F, &dev->dfsrr); /* set default filter */ writeb(slaveadd << 1, &dev->adr); /* write slave address */ @@ -191,15 +192,17 @@ __i2c_read(u8 *data, int length) int i2c_read(u8 dev, uint addr, int alen, u8 *data, int length) { - int i = 0; + int i = -1; /* signal error */ u8 *a = (u8*)&addr;
if (i2c_wait4bus() >= 0 && i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0 - && __i2c_write(&a[4 - alen], alen) == alen - && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0) { + && __i2c_write(&a[4 - alen], alen) == alen) + i = 0; /* No error so far */ + + if (length + && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0) i = __i2c_read(data, length); - }
writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
@@ -212,7 +215,7 @@ i2c_read(u8 dev, uint addr, int alen, u8 *data, int length) int i2c_write(u8 dev, uint addr, int alen, u8 *data, int length) { - int i = 0; + int i = -1; /* signal error */ u8 *a = (u8*)&addr;
if (i2c_wait4bus() >= 0 @@ -232,16 +235,14 @@ i2c_write(u8 dev, uint addr, int alen, u8 *data, int length) int i2c_probe(uchar chip) { - int tmp; - - /* - * Try to read the first location of the chip. The underlying - * driver doesn't appear to support sending just the chip address - * and looking for an <ACK> back. - */ - udelay(10000); + /* For unknow reason the controller will ACK when + * probing for a slave with the same address, so skip + * it. + */ + if (chip == (readb(&i2c_dev[i2c_bus_num]->adr) >> 1)) + return -1;
- return i2c_read(chip, 0, 1, (uchar *)&tmp, 1); + return i2c_read(chip, 0, 0, NULL, 0); }
uchar

Joakim Tjernlund wrote:
This is against the freescale tree at opensource.freescale.com What is the status of this three? is it going into u-boot any time soon?
Yes.
@@ -191,15 +192,17 @@ __i2c_read(u8 *data, int length) int i2c_read(u8 dev, uint addr, int alen, u8 *data, int length) {
- int i = 0;
int i = -1; /* signal error */ u8 *a = (u8*)&addr;
if (i2c_wait4bus() >= 0 && i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
&& __i2c_write(&a[4 - alen], alen) == alen
&& i2c_write_addr(dev, I2C_READ_BIT, 1) != 0) {
&& __i2c_write(&a[4 - alen], alen) == alen)
i = 0; /* No error so far */
- if (length
i = __i2c_read(data, length);&& i2c_write_addr(dev, I2C_READ_BIT, 1) != 0)
- }
This is a good idea, but I see a bug in the code. Say the first block succeeds, and sets i to 0. Then the "i2c_write_addr(dev, I2C_READ_BIT, 1)" fails. It will never call "__i2c_read(data, length)", and so it will never set the value of i. Then it will return, indicating success.
Frankly, I think the original code is just an abuse of short-circuit boolean operations and should be completely rewritten. Something like this:
if (i2c_wait4bus() < 0) return -1;
if (i2c_write_addr(dev, I2C_WRITE_BIT, 0) == 0)) return -1;
if (__i2c_write(&a[4 - alen], alen) != alen)) return -1;
if (length) { if (i2c_write_addr(dev, I2C_READ_BIT, 1) == 0)) return -1;
if (__i2c_read(data, length) != length) return -1; }
writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
return 0;
This version is a lot easier to read and understand, I think.
- /* For unknow reason the controller will ACK when
Typo.

-----Original Message----- From: Timur Tabi [mailto:timur@freescale.com] Sent: den 31 januari 2007 16:58 To: joakim.tjernlund@transmode.se Cc: Kim Phillips; U-BOOT Subject: Re: [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c
Joakim Tjernlund wrote:
This is against the freescale tree at opensource.freescale.com What is the status of this three? is it going into u-boot
any time soon?
Yes.
@@ -191,15 +192,17 @@ __i2c_read(u8 *data, int length) int i2c_read(u8 dev, uint addr, int alen, u8 *data, int length) {
- int i = 0;
int i = -1; /* signal error */ u8 *a = (u8*)&addr;
if (i2c_wait4bus() >= 0 && i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
&& __i2c_write(&a[4 - alen], alen) == alen
&& i2c_write_addr(dev, I2C_READ_BIT, 1) != 0) {
&& __i2c_write(&a[4 - alen], alen) == alen)
i = 0; /* No error so far */
- if (length
i = __i2c_read(data, length);&& i2c_write_addr(dev, I2C_READ_BIT, 1) != 0)
- }
This is a good idea, but I see a bug in the code. Say the first block succeeds, and sets i to 0. Then the "i2c_write_addr(dev, I2C_READ_BIT, 1)" fails. It will never call "__i2c_read(data, length)", and so it will never set the value of i. Then it will return, indicating success.
No, i will be 0 and length will be !=0 so no success.
Frankly, I think the original code is just an abuse of short-circuit boolean operations and should be completely rewritten. Something like this:
if (i2c_wait4bus() < 0) return -1;
if (i2c_write_addr(dev, I2C_WRITE_BIT, 0) == 0)) return -1;
if (__i2c_write(&a[4 - alen], alen) != alen)) return -1;
if (length) { if (i2c_write_addr(dev, I2C_READ_BIT, 1) == 0)) return -1;
if (__i2c_read(data, length) != length) return -1; }
writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
return 0;
This version is a lot easier to read and understand, I think.
Yes, I agree. The orginal freescale code looked something like that, but I am not going to rewrite it back to that form. Someone did change it to this form.
- /* For unknow reason the controller will ACK when
Typo.
Yeah, mind fixing that if the other parts are OK?
Jocke

Joakim Tjernlund wrote:
will never call "__i2c_read(data, length)", and so it will never set the value of i. Then it will return, indicating success.
No, i will be 0 and length will be !=0 so no success.
Oh yeah, right.
Yes, I agree. The orginal freescale code looked something like that, but I am not going to rewrite it back to that form. Someone did change it to this form.
Really? That's weird. Oh well. In that case, your patch is okay.
participants (2)
-
Joakim Tjernlund
-
Timur Tabi