* [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c
@ 2007-01-31 10:04 Joakim Tjernlund
2007-01-31 15:58 ` Timur Tabi
0 siblings, 1 reply; 4+ messages in thread
From: Joakim Tjernlund @ 2007-01-31 10:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c
2007-01-31 10:04 [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c Joakim Tjernlund
@ 2007-01-31 15:58 ` Timur Tabi
2007-01-31 16:26 ` Joakim Tjernlund
0 siblings, 1 reply; 4+ messages in thread
From: Timur Tabi @ 2007-01-31 15:58 UTC (permalink / raw)
To: u-boot
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
> + && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0)
> i = __i2c_read(data, length);
> - }
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.
--
Timur Tabi
Linux Kernel Developer @ Freescale
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c
2007-01-31 15:58 ` Timur Tabi
@ 2007-01-31 16:26 ` Joakim Tjernlund
2007-01-31 16:27 ` Timur Tabi
0 siblings, 1 reply; 4+ messages in thread
From: Joakim Tjernlund @ 2007-01-31 16:26 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Timur Tabi [mailto:timur at freescale.com]
> Sent: den 31 januari 2007 16:58
> To: joakim.tjernlund at 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
> > + && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0)
> > i = __i2c_read(data, length);
> > - }
>
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c
2007-01-31 16:26 ` Joakim Tjernlund
@ 2007-01-31 16:27 ` Timur Tabi
0 siblings, 0 replies; 4+ messages in thread
From: Timur Tabi @ 2007-01-31 16:27 UTC (permalink / raw)
To: u-boot
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.
--
Timur Tabi
Linux Kernel Developer @ Freescale
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-01-31 16:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-31 10:04 [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c Joakim Tjernlund
2007-01-31 15:58 ` Timur Tabi
2007-01-31 16:26 ` Joakim Tjernlund
2007-01-31 16:27 ` Timur Tabi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.