On Thu, 23 Apr 2009 17:02:18 +0200
Jean Delvare <khali@xxxxxxxxxxxx> wrote:
> On Thu, 23 Apr 2009 16:27:39 +0200, Roel Kluin wrote:
> > Ok, here's for drivers/i2c/busses/i2c-pxa.c. Note that I found another,
> > the last hunk.
> > --------------------------->8-------------8<------------------------------
> > With `while (timeout--)' timeout reaches -1 after the loop, so the tests
> > below are off by one.
> >
> > Signed-off-by: Roel Kluin <roel.kluin@xxxxxxxxx>
> > ---
>
> Ben, Wolfram, I'll let you handle this one as it's an arm driver.
>
The patch looks OK, but the original code is weird.
: static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c)
: {
: int timeout = DEF_TIMEOUT;
:
: while (timeout-- && readl(_ISR(i2c)) & (ISR_IBB | ISR_UB)) {
: if ((readl(_ISR(i2c)) & ISR_SAD) != 0)
: timeout += 4;
:
: msleep(2);
: show_state(i2c);
: }
:
: if (timeout < 0)
: show_state(i2c);
:
: return timeout < 0 ? I2C_RETRY : 0;
: }
The timeout+=4 inside the loop makes my brain hurt. It makes the loop
potentially almost-infinite. By effectively doing timeout+=3 each time
we'll break out of the loop after we've wrapped through 0x100000000
three times. Or something. Help!
Also, i2c_pxa_pio_set_master() does
long timeout = 2 * DEF_TIMEOUT;
whereas i2c_pxa_wait_bus_not_bus() does
int timeout = DEF_TIMEOUT;
`int' seems an appropriate choice.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
