[PATCH v2] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[PATCH v2] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[Alexander Sverdlin]

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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
Changes in v2:
- rebased on 110bc76729d4 of Linus's tree;

 drivers/i2c/busses/i2c-davinci.c |   42 +++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 23 deletions(-)

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;

Re: [PATCH v2] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
> Changes in v2:
> - rebased on 110bc76729d4 of Linus's tree;
>
>   drivers/i2c/busses/i2c-davinci.c |   42 +++++++++++++++++--------------------
>   1 files changed, 19 insertions(+), 23 deletions(-)


-- 
regards,
-grygorii

Re: [PATCH v2] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[Alexander Sverdlin]

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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> 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!

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v3] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[Alexander Sverdlin]

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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
Changes in v3:
- corrected argument of i2c_recover_bus()
Changes in v2:
- rebased on 110bc76729d4 of Linus's tree;

 drivers/i2c/busses/i2c-davinci.c |   42 +++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 23 deletions(-)

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->adapter);
+
+	/*
+	 * 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;

Re: [PATCH v3] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Tue, Jun 09, 2015 at 12:58:29PM +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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]