| Message ID | 55534BA0.3000506@nokia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 05/13/2015 04:03 PM, Alexander Sverdlin wrote: > There are several problems in the function: > - "to_cnt" variable does nothing > - schedule_timeout() call without setting current state does nothing > - "allow_sleep" parameter is not really used > > Refactor the function so that it really tries to wait. In case of timeout try > to recover the bus. Reviewed-by: Grygorii Strashko <grygorii.strashko@linaro.org> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > Changes in v2: > - rebased on 110bc76729d4 of Linus's tree; > > drivers/i2c/busses/i2c-davinci.c | 42 +++++++++++++++++-------------------- > 1 files changed, 19 insertions(+), 23 deletions(-)
On Wed, May 13, 2015 at 03:03:28PM +0200, Alexander Sverdlin wrote: > There are several problems in the function: > - "to_cnt" variable does nothing > - schedule_timeout() call without setting current state does nothing > - "allow_sleep" parameter is not really used > > Refactor the function so that it really tries to wait. In case of timeout try > to recover the bus. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> Don't you get a build warning? drivers/i2c/busses/i2c-davinci.c: In function 'i2c_davinci_wait_bus_not_busy': drivers/i2c/busses/i2c-davinci.c:364:18: warning: passing argument 1 of 'i2c_recover_bus' from incompatible pointer type i2c_recover_bus(dev); ^ In file included from drivers/i2c/busses/i2c-davinci.c:26:0: include/linux/i2c.h:446:5: note: expected 'struct i2c_adapter *' but argument is of type 'struct davinci_i2c_dev *' int i2c_recover_bus(struct i2c_adapter *adap); Or sparse complains, too: drivers/i2c/busses/i2c-davinci.c:364:25: warning: incorrect type in argument 1 (different base types) drivers/i2c/busses/i2c-davinci.c:364:25: expected struct i2c_adapter *adap drivers/i2c/busses/i2c-davinci.c:364:25: got struct davinci_i2c_dev *dev
Hello Wolfram, On 02/06/15 17:53, ext Wolfram Sang wrote: >> There are several problems in the function: >> > - "to_cnt" variable does nothing >> > - schedule_timeout() call without setting current state does nothing >> > - "allow_sleep" parameter is not really used >> > >> > Refactor the function so that it really tries to wait. In case of timeout try >> > to recover the bus. >> > >> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > Don't you get a build warning? > > drivers/i2c/busses/i2c-davinci.c: In function 'i2c_davinci_wait_bus_not_busy': > drivers/i2c/busses/i2c-davinci.c:364:18: warning: passing argument 1 of 'i2c_recover_bus' from incompatible pointer type > i2c_recover_bus(dev); > ^ > In file included from drivers/i2c/busses/i2c-davinci.c:26:0: > include/linux/i2c.h:446:5: note: expected 'struct i2c_adapter *' but argument is of type 'struct davinci_i2c_dev *' > int i2c_recover_bus(struct i2c_adapter *adap); oh, this is awful copy-paster error... I'll re-send!
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index df96ab6..03afdee 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -350,29 +350,25 @@ static struct i2c_bus_recovery_info davinci_i2c_scl_recovery_info = { /* * Waiting for bus not busy */ -static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev, - char allow_sleep) +static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev) { - unsigned long timeout; - static u16 to_cnt; - - timeout = jiffies + dev->adapter.timeout; - while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG) - & DAVINCI_I2C_STR_BB) { - if (to_cnt <= DAVINCI_I2C_MAX_TRIES) { - if (time_after(jiffies, timeout)) { - dev_warn(dev->dev, - "timeout waiting for bus ready\n"); - to_cnt++; - return -ETIMEDOUT; - } else { - to_cnt = 0; - i2c_recover_bus(&dev->adapter); - } - } - if (allow_sleep) - schedule_timeout(1); - } + unsigned long timeout = jiffies + dev->adapter.timeout; + + do { + if (!(davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG) & DAVINCI_I2C_STR_BB)) + return 0; + schedule_timeout_uninterruptible(1); + } while (time_before_eq(jiffies, timeout)); + + dev_warn(dev->dev, "timeout waiting for bus ready\n"); + i2c_recover_bus(dev); + + /* + * if bus is still "busy" here, it's most probably a HW problem like + * short-circuit + */ + if (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG) & DAVINCI_I2C_STR_BB) + return -EIO; return 0; } @@ -508,7 +504,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num); - ret = i2c_davinci_wait_bus_not_busy(dev, 1); + ret = i2c_davinci_wait_bus_not_busy(dev); if (ret < 0) { dev_warn(dev->dev, "timeout waiting for bus ready\n"); return ret;
There are several problems in the function: - "to_cnt" variable does nothing - schedule_timeout() call without setting current state does nothing - "allow_sleep" parameter is not really used Refactor the function so that it really tries to wait. In case of timeout try to recover the bus. Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> --- Changes in v2: - rebased on 110bc76729d4 of Linus's tree; drivers/i2c/busses/i2c-davinci.c | 42 +++++++++++++++++-------------------- 1 files changed, 19 insertions(+), 23 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html