On Thu, Apr 23, 2009 at 04:35:07PM -0700, Andrew Morton wrote:
> 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,
Yup.
Acked-by: Wolfram Sang <w.sang@xxxxxxxxxxxxxx>
>
> : 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!
Well, it only adds 4 if there was its own I2C-slave address detected. Without
knowing the details, I assume there is a reason; I wouldn't dare changing it.
> 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.
This driver seems to have quite some more potential for cleaning up :)
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Attachment:
signature.asc
Description: Digital signature
