Hello Wolfram,
Sorry for late response, I had some problems accessing my e-mail.
Thank you for your efforts on moving forward on this topic.
Indeed, this version of the patch combines all features/suggestions,
being discussed earlier, and looks quite compact and clear.
If you don't mind, I would propose one small "polishing" modification
(re-ordering of statements), that doesn't affect the functionality, see below.
________________________________________
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, April 5, 2022 13:07
> To: linux-i2c@xxxxxxxxxxxxxxx
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; Eugeniu Rosca; Wolfram Sang; Surachari, Bhuvanesh; Gabbasov, Andrew
> Subject: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN
>
> [skipped]
>
> @@ -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;
> + }
It looks like rcar_i2c_dma() starts DMA transfer at once, including to the transfer
the byte, currently residing in ICRXD register. (Because ICMSR.MDR bit is set.
That was the issue in my previous v2 patch: it copied the "current" byte to the
"next" buffer position too. To fix that, it would probably be necessary to clear
that bit before starting DMA, so that it starts with the next/pending bytes).
It means that this very first "length" byte will be copied to the buffer
by DMA transfer too (in addition to explicit assignment above).
So, I would propose to move the statement, placing the data byte to the buffer
("msg->buf[priv->pos] = data;"), to this place, after the "if (recv_len_init)" clause,
just before incrementing priv->pos.
Accordingly, adjustment of msg->len inside that "if" should be done using "data"
instead of "msg->buf[0]" ("msg->len += data;").
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).
And will place filling in the data and position incrementation closer to each
other, that looks more "natural".
Again, this is just a "stylish" change, the patch looks good without it too ;-)
Thanks!
Best regards,
Andrew
> 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;
> --
> 2.30.2
>
