[PATCH v2 0/4] Further i2c-pxa cleanups

[PATCH v2 0/4] Further i2c-pxa cleanups

[Russell King - ARM Linux admin]

Hi,

This small series contains some further cleanups to i2c-pxa:

Patch 1 gets rid of the duplication between i2c_pxa_pio_xfer() and
i2c_pxa_xfer(), which are mostly identical.

Patch 2 avoids the "exhausted retries" spamming the kernel log when
running i2cdetect - this occurs when a slave does not respond.  We
keep the existing behaviour for all other retries.

Patch 3 changes the kernel messages printed when timeouts happen so
they are unique - thus allowing identification of which one triggered.

Patch 4 removes an unnecessary show_state() while waiting for the
bus to become free.

v2: fix compile error in patch 1.

 drivers/i2c/busses/i2c-pxa.c | 56 +++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 29 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

[PATCH v2 1/4] i2c: pxa: consolidate i2c_pxa_*xfer() implementations

[Russell King]

Most of i2c_pxa_pio_xfer() and i2c_pxa_xfer() are identical; the only
differences are that i2c_pxa_pio_xfer() may reset the bus, and they
use different underlying transfer functions. The retry loop is the
same. Consolidate these two functions.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/i2c/busses/i2c-pxa.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index c1e50c0b9756..46f1cf97d955 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1102,18 +1102,20 @@ static int i2c_pxa_do_xfer(struct pxa_i2c *i2c, struct i2c_msg *msg, int num)
 	return ret;
 }
 
-static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+static int i2c_pxa_internal_xfer(struct pxa_i2c *i2c,
+				 struct i2c_msg *msgs, int num,
+				 int (*xfer)(struct pxa_i2c *,
+					     struct i2c_msg *, int num))
 {
-	struct pxa_i2c *i2c = adap->algo_data;
 	int ret, i;
 
-	for (i = adap->retries; i >= 0; i--) {
-		ret = i2c_pxa_do_xfer(i2c, msgs, num);
+	for (i = i2c->adap.retries; i >= 0; i--) {
+		ret = xfer(i2c, msgs, num);
 		if (ret != I2C_RETRY)
 			goto out;
 
 		if (i2c_debug)
-			dev_dbg(&adap->dev, "Retrying transmission\n");
+			dev_dbg(&i2c->adap.dev, "Retrying transmission\n");
 		udelay(100);
 	}
 	i2c_pxa_scream_blue_murder(i2c, "exhausted retries");
@@ -1123,6 +1125,14 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num
 	return ret;
 }
 
+static int i2c_pxa_xfer(struct i2c_adapter *adap,
+			struct i2c_msg msgs[], int num)
+{
+	struct pxa_i2c *i2c = adap->algo_data;
+
+	return i2c_pxa_internal_xfer(i2c, msgs, num, i2c_pxa_do_xfer);
+}
+
 static u32 i2c_pxa_functionality(struct i2c_adapter *adap)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
@@ -1210,7 +1220,6 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
 			    struct i2c_msg msgs[], int num)
 {
 	struct pxa_i2c *i2c = adap->algo_data;
-	int ret, i;
 
 	/* If the I2C controller is disabled we need to reset it
 	  (probably due to a suspend/resume destroying state). We do
@@ -1219,20 +1228,7 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
 	if (!(readl(_ICR(i2c)) & ICR_IUE))
 		i2c_pxa_reset(i2c);
 
-	for (i = adap->retries; i >= 0; i--) {
-		ret = i2c_pxa_do_pio_xfer(i2c, msgs, num);
-		if (ret != I2C_RETRY)
-			goto out;
-
-		if (i2c_debug)
-			dev_dbg(&adap->dev, "Retrying transmission\n");
-		udelay(100);
-	}
-	i2c_pxa_scream_blue_murder(i2c, "exhausted retries");
-	ret = -EREMOTEIO;
- out:
-	i2c_pxa_set_slave(i2c, ret);
-	return ret;
+	return i2c_pxa_internal_xfer(i2c, msgs, num, i2c_pxa_do_pio_xfer);
 }
 
 static const struct i2c_algorithm i2c_pxa_pio_algorithm = {
-- 
2.20.1

[PATCH v2 2/4] i2c: pxa: avoid complaints with non-responsive slaves

[Russell King]

Running i2cdetect on a PXA I2C adapter is very noisy; it complains
whenever a slave fails to respond to the address cycle.  Since it is
normal to probe for slaves in this way, we should not fill the kernel
log.  This is especially true with SFP modules that take a while to
respond on the I2C bus, and probing via the I2C bus is the only way to
detect that they are ready.

Fix this by changing the internal transfer return code from I2C_RETRY
to a new NO_SLAVE code (mapped to -ENXIO, as per the I2C documentation
for this condition, but we still return -EREMOTEIO to the I2C stack to
maintain long established driver behaviour.)

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/i2c/busses/i2c-pxa.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 46f1cf97d955..f20f8b905793 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -90,6 +90,7 @@
  */
 #define DEF_TIMEOUT             32
 
+#define NO_SLAVE		(-ENXIO)
 #define BUS_ERROR               (-EREMOTEIO)
 #define XFER_NAKED              (-ECONNREFUSED)
 #define I2C_RETRY               (-2000) /* an error has occurred retry transmit */
@@ -881,7 +882,7 @@ static void i2c_pxa_irq_txempty(struct pxa_i2c *i2c, u32 isr)
 		 */
 		if (isr & ISR_ACKNAK) {
 			if (i2c->msg_ptr == 0 && i2c->msg_idx == 0)
-				ret = I2C_RETRY;
+				ret = NO_SLAVE;
 			else
 				ret = XFER_NAKED;
 		}
@@ -1109,16 +1110,19 @@ static int i2c_pxa_internal_xfer(struct pxa_i2c *i2c,
 {
 	int ret, i;
 
-	for (i = i2c->adap.retries; i >= 0; i--) {
+	for (i = 0; ; ) {
 		ret = xfer(i2c, msgs, num);
-		if (ret != I2C_RETRY)
+		if (ret != I2C_RETRY && ret != NO_SLAVE)
 			goto out;
+		if (++i >= i2c->adap.retries)
+			break;
 
 		if (i2c_debug)
 			dev_dbg(&i2c->adap.dev, "Retrying transmission\n");
 		udelay(100);
 	}
-	i2c_pxa_scream_blue_murder(i2c, "exhausted retries");
+	if (ret != NO_SLAVE)
+		i2c_pxa_scream_blue_murder(i2c, "exhausted retries");
 	ret = -EREMOTEIO;
  out:
 	i2c_pxa_set_slave(i2c, ret);
-- 
2.20.1

[PATCH v2 3/4] i2c: pxa: ensure timeout messages are unique

[Russell King]

Ensure that the various timeout messages can identify where in the code
they were produced from to aid debugging.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/i2c/busses/i2c-pxa.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index f20f8b905793..0becab239476 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1095,7 +1095,7 @@ static int i2c_pxa_do_xfer(struct pxa_i2c *i2c, struct i2c_msg *msg, int num)
 	ret = i2c->msg_idx;
 
 	if (!timeout && i2c->msg_num) {
-		i2c_pxa_scream_blue_murder(i2c, "timeout");
+		i2c_pxa_scream_blue_murder(i2c, "timeout with active message");
 		ret = I2C_RETRY;
 	}
 
@@ -1169,7 +1169,7 @@ static int i2c_pxa_pio_set_master(struct pxa_i2c *i2c)
 	if (timeout < 0) {
 		show_state(i2c);
 		dev_err(&i2c->adap.dev,
-			"i2c_pxa: timeout waiting for bus free\n");
+			"i2c_pxa: timeout waiting for bus free (set_master)\n");
 		return I2C_RETRY;
 	}
 
@@ -1213,7 +1213,7 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c,
 
 out:
 	if (timeout == 0) {
-		i2c_pxa_scream_blue_murder(i2c, "timeout");
+		i2c_pxa_scream_blue_murder(i2c, "timeout (do_pio_xfer)");
 		ret = I2C_RETRY;
 	}
 
-- 
2.20.1

[PATCH v2 4/4] i2c: pxa: remove some unnecessary debug

[Russell King]

Remove unnecessary show_state() in the loop inside
i2c_pxa_pio_set_master(), which can be unnecessarily verbose.

Remove the i2c_pxa_scream_blue_murder() in i2c_pxa_pio_xfer(), which
will trigger if we are probing the I2C bus and a slave does not
respond; this is a normal event, and not something to report.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/i2c/busses/i2c-pxa.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 0becab239476..db739cce93ac 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1161,10 +1161,8 @@ static int i2c_pxa_pio_set_master(struct pxa_i2c *i2c)
 	/*
 	 * Wait for the bus to become free.
 	 */
-	while (timeout-- && readl(_ISR(i2c)) & (ISR_IBB | ISR_UB)) {
+	while (timeout-- && readl(_ISR(i2c)) & (ISR_IBB | ISR_UB))
 		udelay(1000);
-		show_state(i2c);
-	}
 
 	if (timeout < 0) {
 		show_state(i2c);
-- 
2.20.1

Re: [PATCH v2 1/4] i2c: pxa: consolidate i2c_pxa_*xfer() implementations

[Wolfram Sang]

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

On Mon, May 11, 2020 at 10:10:27PM +0100, Russell King wrote:
> Most of i2c_pxa_pio_xfer() and i2c_pxa_xfer() are identical; the only
> differences are that i2c_pxa_pio_xfer() may reset the bus, and they
> use different underlying transfer functions. The retry loop is the
> same. Consolidate these two functions.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-next, thanks!


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

Re: [PATCH v2 2/4] i2c: pxa: avoid complaints with non-responsive slaves

[Wolfram Sang]

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

On Mon, May 11, 2020 at 10:10:32PM +0100, Russell King wrote:
> Running i2cdetect on a PXA I2C adapter is very noisy; it complains
> whenever a slave fails to respond to the address cycle.  Since it is
> normal to probe for slaves in this way, we should not fill the kernel
> log.  This is especially true with SFP modules that take a while to
> respond on the I2C bus, and probing via the I2C bus is the only way to
> detect that they are ready.
> 
> Fix this by changing the internal transfer return code from I2C_RETRY
> to a new NO_SLAVE code (mapped to -ENXIO, as per the I2C documentation
> for this condition, but we still return -EREMOTEIO to the I2C stack to
> maintain long established driver behaviour.)
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-next, thanks!


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

Re: [PATCH v2 3/4] i2c: pxa: ensure timeout messages are unique

[Wolfram Sang]

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

On Mon, May 11, 2020 at 10:10:37PM +0100, Russell King wrote:
> Ensure that the various timeout messages can identify where in the code
> they were produced from to aid debugging.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-next, thanks!


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

Re: [PATCH v2 4/4] i2c: pxa: remove some unnecessary debug

[Wolfram Sang]

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

On Mon, May 11, 2020 at 10:10:42PM +0100, Russell King wrote:
> Remove unnecessary show_state() in the loop inside
> i2c_pxa_pio_set_master(), which can be unnecessarily verbose.
> 
> Remove the i2c_pxa_scream_blue_murder() in i2c_pxa_pio_xfer(), which
> will trigger if we are probing the I2C bus and a slave does not
> respond; this is a normal event, and not something to report.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied to for-next, thanks!


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