Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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 +, 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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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 +, 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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[Wolfram Sang]

> 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!



signature.asc
Description: PGP signature

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[Wolfram Sang]

> 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!



signature.asc
Description: PGP signature

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[Wolfram Sang]

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 +, 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?



signature.asc
Description: PGP signature

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[Wolfram Sang]

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 +, 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?



signature.asc
Description: PGP signature

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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 +, 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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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 +, 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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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 +, 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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[George Cherian]

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 +, 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

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[Wolfram Sang]

On Thu, Jan 18, 2018 at 05:39:24AM +, 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.
> 


signature.asc
Description: PGP signature

Re: [PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[Wolfram Sang]

On Thu, Jan 18, 2018 at 05:39:24AM +, 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.
> 


signature.asc
Description: PGP signature

[PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[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 
---
 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 
 #include 
 #include 
+#include 
 
 #define XLP9XX_I2C_DIV 0x0
 #define XLP9XX_I2C_CTRL0x1
@@ -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_STOPBIT(6)
 #define XLP9XX_I2C_CMD_READBIT(5)
@@ -71,6 +74,7 @@
 #define XLP9XX_I2C_HIGH_FREQ   40
 #define XLP9XX_I2C_FIFO_SIZE   0x80U
 #define XLP9XX_I2C_TIMEOUT_MS  1000
+#define XLP9XX_I2C_BUSY_TIMEOUT50
 
 #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

[PATCH 4/4] i2c: xlp9xx: Check for Bus state after every transfer

[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 
---
 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 
 #include 
 #include 
+#include 
 
 #define XLP9XX_I2C_DIV 0x0
 #define XLP9XX_I2C_CTRL0x1
@@ -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_STOPBIT(6)
 #define XLP9XX_I2C_CMD_READBIT(5)
@@ -71,6 +74,7 @@
 #define XLP9XX_I2C_HIGH_FREQ   40
 #define XLP9XX_I2C_FIFO_SIZE   0x80U
 #define XLP9XX_I2C_TIMEOUT_MS  1000
+#define XLP9XX_I2C_BUSY_TIMEOUT50
 
 #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