linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK
@ 2018-01-18  5:39 George Cherian
  2018-01-18  5:39 ` [PATCH 2/4] i2c: xlp9xx: Handle transactions with I2C_M_RECV_LEN properly George Cherian
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: George Cherian @ 2018-01-18  5:39 UTC (permalink / raw)
  To: linux-kernel, linux-i2c; +Cc: wsa, Dmitry Bazhenov, George Cherian

From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>

Fix the driver violation of the common practice to return
ENXIO error on a slave address NACK.

Signed-off-by: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
Signed-off-by: George Cherian <george.cherian@cavium.com>
---
 drivers/i2c/busses/i2c-xlp9xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index b970bf8..6d78cdc 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -324,7 +324,8 @@ static int xlp9xx_i2c_xfer_msg(struct xlp9xx_i2c_dev *priv, struct i2c_msg *msg,
 		dev_dbg(priv->dev, "transfer error %x!\n", priv->msg_err);
 		if (priv->msg_err & XLP9XX_I2C_INTEN_BUSERR)
 			xlp9xx_i2c_init(priv);
-		return -EIO;
+		return (priv->msg_err & XLP9XX_I2C_INTEN_NACKADDR) ?
+			-ENXIO : -EIO;
 	}
 
 	if (timeleft == 0) {
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/4] i2c: xlp9xx: Handle transactions with I2C_M_RECV_LEN properly
  2018-01-18  5:39 [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
@ 2018-01-18  5:39 ` George Cherian
  2018-02-26 20:20   ` Wolfram Sang
  2018-01-18  5:39 ` [PATCH 3/4] i2c: xlp9xx: report SMBus block read functionality George Cherian
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: George Cherian @ 2018-01-18  5:39 UTC (permalink / raw)
  To: linux-kernel, linux-i2c; +Cc: wsa, George Cherian

In case of transaction with I2C_M_RECV_LEN set, make sure the driver reads
the first byte and then updates the RX fifo with the expected length. Set
threshold to 1 byte so that driver gets an interrupt on receiving the first byte.
After which the transfer length is updated depending on the received length.

Signed-off-by: George Cherian <george.cherian@cavium.com>
---
 drivers/i2c/busses/i2c-xlp9xx.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index 6d78cdc..b5b224e 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -125,7 +125,16 @@ static void xlp9xx_i2c_update_rx_fifo_thres(struct xlp9xx_i2c_dev *priv)
 {
 	u32 thres;
 
-	thres = min(priv->msg_buf_remaining, XLP9XX_I2C_FIFO_SIZE);
+	if (priv->len_recv)
+		/* interrupt after the first read to examine
+		 * the length byte before proceeding further
+		 */
+		thres = 1;
+	else if (priv->msg_buf_remaining > XLP9XX_I2C_FIFO_SIZE)
+		thres = XLP9XX_I2C_FIFO_SIZE;
+	else
+		thres = priv->msg_buf_remaining;
+
 	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MFIFOCTRL,
 			     thres << XLP9XX_I2C_MFIFOCTRL_HITH_SHIFT);
 }
@@ -144,7 +153,7 @@ static void xlp9xx_i2c_fill_tx_fifo(struct xlp9xx_i2c_dev *priv)
 
 static void xlp9xx_i2c_drain_rx_fifo(struct xlp9xx_i2c_dev *priv)
 {
-	u32 len, i;
+	u32 len, i, val;
 	u8 rlen, *buf = priv->msg_buf;
 
 	len = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_FIFOWCNT) &
@@ -156,19 +165,27 @@ static void xlp9xx_i2c_drain_rx_fifo(struct xlp9xx_i2c_dev *priv)
 		rlen = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_MRXFIFO);
 		*buf++ = rlen;
 		len--;
+
 		if (priv->client_pec)
 			++rlen;
 		/* update remaining bytes and message length */
 		priv->msg_buf_remaining = rlen;
 		priv->msg_len = rlen + 1;
 		priv->len_recv = false;
-	}
 
-	len = min(priv->msg_buf_remaining, len);
-	for (i = 0; i < len; i++, buf++)
-		*buf = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_MRXFIFO);
+		/* Update transfer length to read only actual data */
+		val = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_CTRL);
+		val = (val & ~XLP9XX_I2C_CTRL_MCTLEN_MASK) |
+			((rlen + 1) << XLP9XX_I2C_CTRL_MCTLEN_SHIFT);
+		xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, val);
+	} else {
+		len = min(priv->msg_buf_remaining, len);
+		for (i = 0; i < len; i++, buf++)
+			*buf = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_MRXFIFO);
+
+		priv->msg_buf_remaining -= len;
+	}
 
-	priv->msg_buf_remaining -= len;
 	priv->msg_buf = buf;
 
 	if (priv->msg_buf_remaining)
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/4] i2c: xlp9xx: report SMBus block read functionality
  2018-01-18  5:39 [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
  2018-01-18  5:39 ` [PATCH 2/4] i2c: xlp9xx: Handle transactions with I2C_M_RECV_LEN properly George Cherian
@ 2018-01-18  5:39 ` George Cherian
  2018-02-26 20:21   ` Wolfram Sang
  2018-01-18  5:39 ` [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer George Cherian
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: George Cherian @ 2018-01-18  5:39 UTC (permalink / raw)
  To: linux-kernel, linux-i2c; +Cc: wsa, Dmitry Bazhenov

From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>

Report SMBus block read functionality which is actually supported.

Signed-off-by: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
---
 drivers/i2c/busses/i2c-xlp9xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index b5b224e..1f6d780 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -374,8 +374,8 @@ static int xlp9xx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 static u32 xlp9xx_i2c_functionality(struct i2c_adapter *adapter)
 {
-	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C |
-		I2C_FUNC_10BIT_ADDR;
+	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_READ_BLOCK_DATA |
+			I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
 }
 
 static const struct i2c_algorithm xlp9xx_i2c_algo = {
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-01-18  5:39 [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
  2018-01-18  5:39 ` [PATCH 2/4] i2c: xlp9xx: Handle transactions with I2C_M_RECV_LEN properly George Cherian
  2018-01-18  5:39 ` [PATCH 3/4] i2c: xlp9xx: report SMBus block read functionality George Cherian
@ 2018-01-18  5:39 ` George Cherian
  2018-02-26 20:22   ` Wolfram Sang
  2018-01-30 14:28 ` [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
  2018-02-26 20:20 ` Wolfram Sang
  4 siblings, 1 reply; 16+ messages in thread
From: George Cherian @ 2018-01-18  5:39 UTC (permalink / raw)
  To: linux-kernel, linux-i2c; +Cc: wsa, George Cherian

I2C bus enters the STOP condition after the DATA_DONE interrupt is raised.
Essentially the driver should be checking the bus state before sending
the next transaction. In case the next transaction is initiated while the
bus is busy, the prior transactions stop condition is not acheived.
Add the check to make sure the bus is not busy before next transaction.

Signed-off-by: George Cherian <george.cherian@cavium.com>
---
 drivers/i2c/busses/i2c-xlp9xx.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index 1f6d780..fa9d5ee 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/delay.h>
 
 #define XLP9XX_I2C_DIV			0x0
 #define XLP9XX_I2C_CTRL			0x1
@@ -36,6 +37,8 @@
 #define XLP9XX_I2C_TIMEOUT		0X10
 #define XLP9XX_I2C_GENCALLADDR		0x11
 
+#define XLP9XX_I2C_STATUS_BUSY		BIT(0)
+
 #define XLP9XX_I2C_CMD_START		BIT(7)
 #define XLP9XX_I2C_CMD_STOP		BIT(6)
 #define XLP9XX_I2C_CMD_READ		BIT(5)
@@ -71,6 +74,7 @@
 #define XLP9XX_I2C_HIGH_FREQ		400000
 #define XLP9XX_I2C_FIFO_SIZE		0x80U
 #define XLP9XX_I2C_TIMEOUT_MS		1000
+#define XLP9XX_I2C_BUSY_TIMEOUT		50
 
 #define XLP9XX_I2C_FIFO_WCNT_MASK	0xff
 #define XLP9XX_I2C_STATUS_ERRMASK	(XLP9XX_I2C_INTEN_ARLOST | \
@@ -264,7 +268,8 @@ static int xlp9xx_i2c_xfer_msg(struct xlp9xx_i2c_dev *priv, struct i2c_msg *msg,
 			       int last_msg)
 {
 	unsigned long timeleft;
-	u32 intr_mask, cmd, val, len;
+	u32 intr_mask, cmd, val, len, status;
+	u32 busy_timeout = XLP9XX_I2C_BUSY_TIMEOUT;
 
 	priv->msg_buf = msg->buf;
 	priv->msg_buf_remaining = priv->msg_len = msg->len;
@@ -351,6 +356,19 @@ static int xlp9xx_i2c_xfer_msg(struct xlp9xx_i2c_dev *priv, struct i2c_msg *msg,
 		return -ETIMEDOUT;
 	}
 
+	while (last_msg && busy_timeout) {
+		status = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_STATUS);
+		if ((status & XLP9XX_I2C_STATUS_BUSY) == 0)
+			break;
+
+		busy_timeout--;
+		udelay(1);
+	}
+
+	if (!busy_timeout) {
+		dev_dbg(priv->dev, "i2c bus busy for too long after transfer\n");
+		return -EIO;
+	}
 	/* update msg->len with actual received length */
 	if (msg->flags & I2C_M_RECV_LEN)
 		msg->len = priv->msg_len;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK
  2018-01-18  5:39 [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
                   ` (2 preceding siblings ...)
  2018-01-18  5:39 ` [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer George Cherian
@ 2018-01-30 14:28 ` George Cherian
  2018-02-22 18:02   ` dann frazier
  2018-02-26 20:20 ` Wolfram Sang
  4 siblings, 1 reply; 16+ messages in thread
From: George Cherian @ 2018-01-30 14:28 UTC (permalink / raw)
  To: George Cherian, linux-kernel, linux-i2c; +Cc: wsa, Dmitry Bazhenov

Gentle Ping on this series.

On 01/18/2018 11:09 AM, George Cherian wrote:
> From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
> 
> Fix the driver violation of the common practice to return
> ENXIO error on a slave address NACK.
> 
> Signed-off-by: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
> Signed-off-by: George Cherian <george.cherian@cavium.com>
> ---
>   drivers/i2c/busses/i2c-xlp9xx.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> index b970bf8..6d78cdc 100644
> --- a/drivers/i2c/busses/i2c-xlp9xx.c
> +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> @@ -324,7 +324,8 @@ static int xlp9xx_i2c_xfer_msg(struct xlp9xx_i2c_dev *priv, struct i2c_msg *msg,
>   		dev_dbg(priv->dev, "transfer error %x!\n", priv->msg_err);
>   		if (priv->msg_err & XLP9XX_I2C_INTEN_BUSERR)
>   			xlp9xx_i2c_init(priv);
> -		return -EIO;
> +		return (priv->msg_err & XLP9XX_I2C_INTEN_NACKADDR) ?
> +			-ENXIO : -EIO;
>   	}
>   
>   	if (timeleft == 0) {
> 

Regards
-George

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK
  2018-01-30 14:28 ` [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
@ 2018-02-22 18:02   ` dann frazier
  0 siblings, 0 replies; 16+ messages in thread
From: dann frazier @ 2018-02-22 18:02 UTC (permalink / raw)
  To: George Cherian
  Cc: George Cherian, linux-kernel, linux-i2c, wsa, Dmitry Bazhenov

On Tue, Jan 30, 2018 at 7:28 AM, George Cherian
<gcherian@caviumnetworks.com> wrote:
> Gentle Ping on this series.

I've been using these on a few Cavium Sabre boards, which previously
had an unusable system interface (/dev/ipmi) due to timeouts or just
enumerations failures. So, fwiw:

Tested-by: dann frazier <dann.frazier@canonical.com>

 -dann

> On 01/18/2018 11:09 AM, George Cherian wrote:
>>
>> From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
>>
>> Fix the driver violation of the common practice to return
>> ENXIO error on a slave address NACK.
>>
>> Signed-off-by: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>> ---
>>   drivers/i2c/busses/i2c-xlp9xx.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-xlp9xx.c
>> b/drivers/i2c/busses/i2c-xlp9xx.c
>> index b970bf8..6d78cdc 100644
>> --- a/drivers/i2c/busses/i2c-xlp9xx.c
>> +++ b/drivers/i2c/busses/i2c-xlp9xx.c
>> @@ -324,7 +324,8 @@ static int xlp9xx_i2c_xfer_msg(struct xlp9xx_i2c_dev
>> *priv, struct i2c_msg *msg,
>>                 dev_dbg(priv->dev, "transfer error %x!\n", priv->msg_err);
>>                 if (priv->msg_err & XLP9XX_I2C_INTEN_BUSERR)
>>                         xlp9xx_i2c_init(priv);
>> -               return -EIO;
>> +               return (priv->msg_err & XLP9XX_I2C_INTEN_NACKADDR) ?
>> +                       -ENXIO : -EIO;
>>         }
>>         if (timeleft == 0) {
>>
>
> Regards
> -George

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK
  2018-01-18  5:39 [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
                   ` (3 preceding siblings ...)
  2018-01-30 14:28 ` [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
@ 2018-02-26 20:20 ` Wolfram Sang
  4 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2018-02-26 20:20 UTC (permalink / raw)
  To: George Cherian; +Cc: linux-kernel, linux-i2c, Dmitry Bazhenov

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

On Thu, Jan 18, 2018 at 05:39:21AM +0000, George Cherian wrote:
> From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
> 
> Fix the driver violation of the common practice to return
> ENXIO error on a slave address NACK.
> 
> Signed-off-by: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
> Signed-off-by: George Cherian <george.cherian@cavium.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] i2c: xlp9xx: Handle transactions with I2C_M_RECV_LEN properly
  2018-01-18  5:39 ` [PATCH 2/4] i2c: xlp9xx: Handle transactions with I2C_M_RECV_LEN properly George Cherian
@ 2018-02-26 20:20   ` Wolfram Sang
  0 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2018-02-26 20:20 UTC (permalink / raw)
  To: George Cherian; +Cc: linux-kernel, linux-i2c

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

On Thu, Jan 18, 2018 at 05:39:22AM +0000, George Cherian wrote:
> In case of transaction with I2C_M_RECV_LEN set, make sure the driver reads
> the first byte and then updates the RX fifo with the expected length. Set
> threshold to 1 byte so that driver gets an interrupt on receiving the first byte.
> After which the transfer length is updated depending on the received length.
> 
> Signed-off-by: George Cherian <george.cherian@cavium.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/4] i2c: xlp9xx: report SMBus block read functionality
  2018-01-18  5:39 ` [PATCH 3/4] i2c: xlp9xx: report SMBus block read functionality George Cherian
@ 2018-02-26 20:21   ` Wolfram Sang
  0 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2018-02-26 20:21 UTC (permalink / raw)
  To: George Cherian; +Cc: linux-kernel, linux-i2c, Dmitry Bazhenov

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

On Thu, Jan 18, 2018 at 05:39:23AM +0000, George Cherian wrote:
> From: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>
> 
> Report SMBus block read functionality which is actually supported.
> 
> Signed-off-by: Dmitry Bazhenov <dmitry.bazhenov@auriga.com>

Applied to for-next but squashed it with patch 2 because I'd think you
also want that fix before enabling it in 'functionality'. Let me know if
you disagree.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-01-18  5:39 ` [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer George Cherian
@ 2018-02-26 20:22   ` Wolfram Sang
  2018-02-27  5:00     ` George Cherian
  0 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2018-02-26 20:22 UTC (permalink / raw)
  To: George Cherian; +Cc: linux-kernel, linux-i2c

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


On Thu, Jan 18, 2018 at 05:39:24AM +0000, George Cherian wrote:
> I2C bus enters the STOP condition after the DATA_DONE interrupt is raised.
> Essentially the driver should be checking the bus state before sending
> the next transaction.

Yes.

> In case the next transaction is initiated while the
> bus is busy, the prior transactions stop condition is not acheived.

I didn't fully get why you can't check the BUSY bit and wait a little
just before you push out the next message?

> Add the check to make sure the bus is not busy before next transaction.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-02-26 20:22   ` Wolfram Sang
@ 2018-02-27  5:00     ` George Cherian
  2018-02-27  5:11       ` George Cherian
  2018-02-27  9:04       ` Wolfram Sang
  0 siblings, 2 replies; 16+ messages in thread
From: George Cherian @ 2018-02-27  5:00 UTC (permalink / raw)
  To: Wolfram Sang, George Cherian; +Cc: linux-kernel, linux-i2c

Hi Wolfram,

Thanks for the review.

On 02/27/2018 01:52 AM, Wolfram Sang wrote:
> 
> On Thu, Jan 18, 2018 at 05:39:24AM +0000, George Cherian wrote:
>> I2C bus enters the STOP condition after the DATA_DONE interrupt is raised.
>> Essentially the driver should be checking the bus state before sending
>> the next transaction.
> 
> Yes.
> 
>> In case the next transaction is initiated while the
>> bus is busy, the prior transactions stop condition is not achieved.
> 
> I didn't fully get why you can't check the BUSY bit and wait a little
> just before you push out the next message?
Yes, I am checking for the BUSY bit and looping.
Here for reference

+	while (last_msg && busy_timeout) {
+		status = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_STATUS);
+		if ((status & XLP9XX_I2C_STATUS_BUSY) == 0)
+			break;
+
+		busy_timeout--;
+		udelay(1);
+	}
+
+	if (!busy_timeout) {
+		dev_dbg(priv->dev, "i2c bus busy for too long after transfer\n");
+		return -EIO;
+	}

Did you mean to eliminate the udelay and use msleep?
In any case I will re-post another version of the patch, since
I have found some more issues and need to be fixed.

> 
>> Add the check to make sure the bus is not busy before next transaction.
>>

Regards,
-George

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-02-27  5:00     ` George Cherian
@ 2018-02-27  5:11       ` George Cherian
  2018-02-27  9:05         ` Wolfram Sang
  2018-02-27  9:04       ` Wolfram Sang
  1 sibling, 1 reply; 16+ messages in thread
From: George Cherian @ 2018-02-27  5:11 UTC (permalink / raw)
  To: Wolfram Sang, George Cherian; +Cc: linux-kernel, linux-i2c

Hi Wolfram,


On 02/27/2018 10:30 AM, George Cherian wrote:
> Hi Wolfram,
> 
> Thanks for the review.
> 
> On 02/27/2018 01:52 AM, Wolfram Sang wrote:
>>
>> On Thu, Jan 18, 2018 at 05:39:24AM +0000, George Cherian wrote:
>>> I2C bus enters the STOP condition after the DATA_DONE interrupt is 
>>> raised.
>>> Essentially the driver should be checking the bus state before sending
>>> the next transaction.
>>
>> Yes.
>>
>>> In case the next transaction is initiated while the
>>> bus is busy, the prior transactions stop condition is not achieved.
>>
>> I didn't fully get why you can't check the BUSY bit and wait a little
>> just before you push out the next message?
> Yes, I am checking for the BUSY bit and looping.
> Here for reference
> 
> +    while (last_msg && busy_timeout) {
> +        status = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_STATUS);
> +        if ((status & XLP9XX_I2C_STATUS_BUSY) == 0)
> +            break;
> +
> +        busy_timeout--;
> +        udelay(1);
> +    }
> +
> +    if (!busy_timeout) {
> +        dev_dbg(priv->dev, "i2c bus busy for too long after transfer\n");
> +        return -EIO;
> +    }
> 
> Did you mean to eliminate the udelay and use msleep?
> In any case I will re-post another version of the patch, since
> I have found some more issues and need to be fixed.

Since you raised concern on the patch I thought of reworking this patch.
But I can see that this patch is already applied for i2c/for-next.
Kindly let me know whether I should be sending follow-up patches on top
of i2c/for-next ?
> 
>>
>>> Add the check to make sure the bus is not busy before next transaction.
>>>
> 
> Regards,
> -George

Regards,
-George

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-02-27  5:00     ` George Cherian
  2018-02-27  5:11       ` George Cherian
@ 2018-02-27  9:04       ` Wolfram Sang
  2018-02-27  9:32         ` George Cherian
  1 sibling, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2018-02-27  9:04 UTC (permalink / raw)
  To: George Cherian; +Cc: George Cherian, linux-kernel, linux-i2c

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

On Tue, Feb 27, 2018 at 10:30:31AM +0530, George Cherian wrote:
> Hi Wolfram,
> 
> Thanks for the review.
> 
> On 02/27/2018 01:52 AM, Wolfram Sang wrote:
> > 
> > On Thu, Jan 18, 2018 at 05:39:24AM +0000, George Cherian wrote:
> > > I2C bus enters the STOP condition after the DATA_DONE interrupt is raised.
> > > Essentially the driver should be checking the bus state before sending
> > > the next transaction.
> > 
> > Yes.
> > 
> > > In case the next transaction is initiated while the
> > > bus is busy, the prior transactions stop condition is not achieved.
> > 
> > I didn't fully get why you can't check the BUSY bit and wait a little
> > just before you push out the next message?
> Yes, I am checking for the BUSY bit and looping.

Yes, but *after* the STOP, not *before* the next message. I haven't
fully understood why you don't do this before the next message is about
to be sent. That might save you some busy looping, or?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-02-27  5:11       ` George Cherian
@ 2018-02-27  9:05         ` Wolfram Sang
  2018-02-27  9:32           ` George Cherian
  0 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2018-02-27  9:05 UTC (permalink / raw)
  To: George Cherian; +Cc: George Cherian, linux-kernel, linux-i2c

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


> Since you raised concern on the patch I thought of reworking this patch.
> But I can see that this patch is already applied for i2c/for-next.
> Kindly let me know whether I should be sending follow-up patches on top
> of i2c/for-next ?

Oops, that was a mistake on my side. I'll revert that patch. Please
think of the patch as not-applied-yet.

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-02-27  9:04       ` Wolfram Sang
@ 2018-02-27  9:32         ` George Cherian
  0 siblings, 0 replies; 16+ messages in thread
From: George Cherian @ 2018-02-27  9:32 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: George Cherian, linux-kernel, linux-i2c

Hi Wolfram,

On 02/27/2018 02:34 PM, Wolfram Sang wrote:
> On Tue, Feb 27, 2018 at 10:30:31AM +0530, George Cherian wrote:
>> Hi Wolfram,
>>
>> Thanks for the review.
>>
>> On 02/27/2018 01:52 AM, Wolfram Sang wrote:
>>>
>>> On Thu, Jan 18, 2018 at 05:39:24AM +0000, George Cherian wrote:
>>>> I2C bus enters the STOP condition after the DATA_DONE interrupt is raised.
>>>> Essentially the driver should be checking the bus state before sending
>>>> the next transaction.
>>>
>>> Yes.
>>>
>>>> In case the next transaction is initiated while the
>>>> bus is busy, the prior transactions stop condition is not achieved.
>>>
>>> I didn't fully get why you can't check the BUSY bit and wait a little
>>> just before you push out the next message?
>> Yes, I am checking for the BUSY bit and looping.
> 
> Yes, but *after* the STOP, not *before* the next message. I haven't
> fully understood why you don't do this before the next message is about
> to be sent. That might save you some busy looping, or?
Yes, Thanks for the clarification. You are right It is better to
check before next message. I will make required changes and post the patch.
>

Regards
-George

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer
  2018-02-27  9:05         ` Wolfram Sang
@ 2018-02-27  9:32           ` George Cherian
  0 siblings, 0 replies; 16+ messages in thread
From: George Cherian @ 2018-02-27  9:32 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: George Cherian, linux-kernel, linux-i2c

Hi Wolfram,

On 02/27/2018 02:35 PM, Wolfram Sang wrote:
> 
>> Since you raised concern on the patch I thought of reworking this patch.
>> But I can see that this patch is already applied for i2c/for-next.
>> Kindly let me know whether I should be sending follow-up patches on top
>> of i2c/for-next ?
> 
> Oops, that was a mistake on my side. I'll revert that patch. Please
> think of the patch as not-applied-yet.
Okay Noted!!
> 
> Thanks!
> 

Regards,
-George

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-02-27  9:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-18  5:39 [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
2018-01-18  5:39 ` [PATCH 2/4] i2c: xlp9xx: Handle transactions with I2C_M_RECV_LEN properly George Cherian
2018-02-26 20:20   ` Wolfram Sang
2018-01-18  5:39 ` [PATCH 3/4] i2c: xlp9xx: report SMBus block read functionality George Cherian
2018-02-26 20:21   ` Wolfram Sang
2018-01-18  5:39 ` [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer George Cherian
2018-02-26 20:22   ` Wolfram Sang
2018-02-27  5:00     ` George Cherian
2018-02-27  5:11       ` George Cherian
2018-02-27  9:05         ` Wolfram Sang
2018-02-27  9:32           ` George Cherian
2018-02-27  9:04       ` Wolfram Sang
2018-02-27  9:32         ` George Cherian
2018-01-30 14:28 ` [PATCH 1/4] i2c: xlp9xx: return ENXIO on slave address NACK George Cherian
2018-02-22 18:02   ` dann frazier
2018-02-26 20:20 ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).