diff mbox series

[v4] i2c: rcar: add support for I2C_M_RECV_LEN

Message ID 20220405100756.42920-1-wsa+renesas@sang-engineering.com
State Accepted
Commit 633c0e7559ea88d2cbd5a6a06d6accfddf55542f
Headers show
Series [v4] i2c: rcar: add support for I2C_M_RECV_LEN | expand

Commit Message

Wolfram Sang April 5, 2022, 10:07 a.m. UTC 
With this feature added, SMBus Block reads and Proc calls are now
supported. This patch is the best of two independent developments by
Wolfram and Bhuvanesh + Andrew, refactored again by Wolfram.

Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

For testing, I wired a Lager board (R-Car H2) and a Salvator-XS (R-Car
H3 ES2.0) together. The Lager board ran the testunit and provided SMBus
Proc Calls. The Salvator-XS board was requesting the data.

Compared to my previous version: sending 1 byte works now, sending with
DMA as well. Invalid sizes are detected, too. This is as much as I can
test, I'd think.

Compared to Bhuvanesh + Andrew's last version: less intrusive and more
self contained (no goto), Proc Calls are covered as well

I tried some other refactoring as well (like one single place where
rcar_i2c_dma() is called) but IMHO this is the most readable solution.

Thank you everyone for working on this. I am very interested in your
comments and test results!

 drivers/i2c/busses/i2c-rcar.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

Comments

Gabbasov, Andrew April 7, 2022, 12:42 p.m. UTC | #1 
Hi Wolfram,

> > Besides avoiding of double assignment of that "length" byte to the buffer,
> > this move will avoid pollution of the buffer in case of an error (invalid length).
> 
> This was intendend as a feature not a bug ;) This was the reason I put
> the data to the buffer at the beginning, so people could see the wrong
> data length. But yes, it can be argued if it should be logged somewhere
> instead of being in the buffer. I'll check what other drivers do.
> 
> Sidenote: It is planned to add SMBus3 features somewhen. Then, there
> won't be an invalid length anymore because it allows 255 byte transfers.

Fair enough. That makes sense.
I withdraw my proposal then ;-) Sorry for the noise.

Thanks.

Best regards,
Andrew
Wolfram Sang April 15, 2022, 9:36 p.m. UTC | #2 
On Tue, Apr 05, 2022 at 12:07:56PM +0200, Wolfram Sang wrote:
> With this feature added, SMBus Block reads and Proc calls are now
> supported. This patch is the best of two independent developments by
> Wolfram and Bhuvanesh + Andrew, refactored again by Wolfram.
> 
> Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f71c730f9838..f45991252993 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -105,6 +105,7 @@ 
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
+#define ID_EPROTO	(1 << 5)
 /* persistent flags */
 #define ID_P_HOST_NOTIFY	BIT(28)
 #define ID_P_REP_AFTER_RD	BIT(29)
@@ -522,6 +523,7 @@  static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
+	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
@@ -535,12 +537,29 @@  static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		rcar_i2c_dma(priv);
 	} else if (priv->pos < msg->len) {
 		/* get received data */
-		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
+		u8 data = rcar_i2c_read(priv, ICRXTX);
+
+		msg->buf[priv->pos] = data;
+		if (recv_len_init) {
+			if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) {
+				priv->flags |= ID_DONE | ID_EPROTO;
+				return;
+			}
+			msg->len += msg->buf[0];
+			/* Enough data for DMA? */
+			if (rcar_i2c_dma(priv))
+				return;
+			/* new length after RECV_LEN now properly initialized */
+			recv_len_init = false;
+		}
 		priv->pos++;
 	}
 
-	/* If next received data is the _LAST_, go to new phase. */
-	if (priv->pos + 1 == msg->len) {
+	/*
+	 * If next received data is the _LAST_ and we are not waiting for a new
+	 * length because of RECV_LEN, then go to a new phase.
+	 */
+	if (priv->pos + 1 == msg->len && !recv_len_init) {
 		if (priv->flags & ID_LAST_MSG) {
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
 		} else {
@@ -847,6 +866,8 @@  static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -909,6 +930,8 @@  static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -975,7 +998,7 @@  static u32 rcar_i2c_func(struct i2c_adapter *adap)
 	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
 	 */
 	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
-		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+		   (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);
 
 	if (priv->flags & ID_P_HOST_NOTIFY)
 		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;