[PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

[PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

[Haobin Jiang]

The i.MX controller has no hardware "ignore NAK" enable bit and checks
each byte in software after transmitting it: i2c_imx_acked() on the
atomic paths, i2c_imx_isr_acked() on the ISR path.  Any NAK is
therefore treated as a fatal error and aborts the transfer with -ENXIO.

There are legitimate cases where a target NAKs on purpose and the
transfer must continue anyway.  The standard way to express this is
I2C_M_IGNORE_NAK, which the core passes through to the bus driver
unchanged: i2c_transfer_buffer_flags() performs no validation of the
message flags, and the i.MX driver has never advertised
I2C_FUNC_PROTOCOL_MANGLING.  Today such a transfer transmits bytes
until the first NAK and then fails with -ENXIO, silently dropping the
flag.  One in-tree caller that hits this on i.MX buses is the wake
sequence of the Atmel crypto drivers (drivers/crypto/atmel-i2c.c, used
by atmel-ecc.c for the ATECC508A/608A): the sleeping device NAKs the
wake token and the write must complete anyway; the ATECC508A is wired
to an i.MX I2C controller e.g. on the Gateworks GW5904.

Advertise I2C_FUNC_PROTOCOL_MANGLING and honour I2C_M_IGNORE_NAK at
every software ACK check: on the PIO write paths, which check the ACK
of the address and of every data byte, and on the PIO read paths, which
check the target's ACK of the address byte, mirroring i2c-img-scb.
With the flag set a NAK no longer aborts the transfer and the byte loop
continues so the remaining bytes are still transmitted.  Every transfer
without the flag keeps the previous behaviour unchanged.

A NAK'd byte does not generate a further DMA request, so a DMA write to
a deliberately-NAKing target would stall until DMA_TIMEOUT instead of
completing.  Force the PIO path for every message carrying
I2C_M_IGNORE_NAK, again mirroring i2c-img-scb, which implements the
flag in atomic mode only.  The DMA write path therefore never sees the
flag, but it passes the flag through at the final ACK check like the
other paths.

I2C_FUNC_PROTOCOL_MANGLING is a blanket capability bit, so advertising
it while implementing only I2C_M_IGNORE_NAK would silently ignore the
other mangling flags.  Reject I2C_M_NO_RD_ACK and I2C_M_REV_DIR_ADDR in
xfer_common() instead; no in-tree user of these flags transfers through
i2c-imx.  I2C_M_NO_RD_ACK is not implementable at all on this
controller: it always clocks the ninth acknowledge bit and I2CR[TXAK]
only selects ACK versus NAK.  I2C_M_STOP is deliberately not rejected:
the controller already terminates every transfer with a STOP and
separates messages with repeated STARTs, so leaving the flag unhandled
changes nothing on the bus, while rejecting it would break the ov2659
camera on imx6qdl-ds: I2C_CLIENT_SCCB is I2C_M_STOP | I2C_M_IGNORE_NAK
and the driver stamps client->flags on every message, including the
chip-ID reads done at probe time.

No retry is introduced: the meaning of a NAK is device-specific [1], so
it stays a device-driver decision.  I2C_M_IGNORE_NAK is only honoured
when the device driver explicitly sets it, which is exactly how the
flag is meant to be used.

Link: https://patchwork.ozlabs.org/project/linux-i2c/patch/1467900229-5262-1-git-send-email-tharvey@gateworks.com/ # [1]
Link: https://lore.kernel.org/linux-i2c/4D46D571.5010907@armadeus.com/ # [2]
Link: https://patchwork.ozlabs.org/project/linux-i2c/patch/1378857490-30968-1-git-send-email-luka@openwrt.org/ # [3]
Signed-off-by: Haobin Jiang <lemonoutput@foxmail.com>

---
v1 -> v2:
	- Honour I2C_M_IGNORE_NAK on the read path as well, at the checks
	  of the target's ACK of the address byte (i2c_imx_isr_read(),
	  i2c_imx_prepare_read()), mirroring i2c-img-scb, instead of
	  silently dropping the flag there.  Read requests carrying the
	  flag cannot be rejected, because in-tree SCCB sensors such as
	  ov2659 set I2C_CLIENT_SCCB, i.e. I2C_M_STOP |
	  I2C_M_IGNORE_NAK, on their read messages too.
	- Force the PIO path for every message with I2C_M_IGNORE_NAK: a
	  NAK'd byte does not generate a further DMA request, so a DMA
	  write to a deliberately-NAKing target would stall until
	  DMA_TIMEOUT (mirrors i2c-img-scb).  i2c_imx_dma_write() can
	  therefore never see the flag, though it still passes it
	  through at the final ACK check like the other paths.
	- Reject I2C_M_NO_RD_ACK and I2C_M_REV_DIR_ADDR in
	  xfer_common() instead of advertising blanket support and
	  silently ignoring them.  I2C_M_STOP is not rejected: the
	  controller already emits a STOP after the final message and
	  repeated STARTs between messages, and rejecting it would break
	  the ov2659 camera on imx6qdl-ds.
	- Correct the description of the status quo: the core does not
	  reject I2C_M_IGNORE_NAK; i2c_transfer_buffer_flags() passes the
	  message flags through unchanged, so the current driver sends
	  bytes until the first NAK and then fails with -ENXIO.
---
 drivers/i2c/busses/i2c-imx.c | 44 ++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 28313d0fad37..5b22a497a791 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -612,9 +612,12 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic)
 	return 0;
 }
 
-static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx, bool ignore_nak)
 {
 	if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
+		if (ignore_nak)
+			return 0;
+
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> No ACK\n", __func__);
 		return -ENXIO;  /* No ACK */
 	}
@@ -968,11 +971,15 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
 	return ret;
 }
 
-static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx)
+static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx,
+				    bool ignore_nak)
 {
 	i2c_imx->isr_result = 0;
 
 	if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
+		if (ignore_nak)
+			return 0;
+
 		i2c_imx->state = IMX_I2C_STATE_FAILED;
 		i2c_imx->isr_result = -ENXIO;
 		wake_up(&i2c_imx->queue);
@@ -985,7 +992,7 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
 {
 	int result;
 
-	result = i2c_imx_isr_acked(i2c_imx);
+	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
 	if (result)
 		return result;
 
@@ -1002,7 +1009,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
 	int result;
 	unsigned int temp;
 
-	result = i2c_imx_isr_acked(i2c_imx);
+	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
 	if (result)
 		return result;
 
@@ -1213,7 +1220,8 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
 	if (result)
 		return result;
 
-	return i2c_imx_acked(i2c_imx);
+	/* I2C_M_IGNORE_NAK messages always take the PIO path. */
+	return i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
 }
 
 static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
@@ -1227,7 +1235,7 @@ static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
 	result = i2c_imx_trx_complete(i2c_imx, !use_dma);
 	if (result)
 		return result;
-	result = i2c_imx_acked(i2c_imx);
+	result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
 	if (result)
 		return result;
 
@@ -1358,7 +1366,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
 	result = i2c_imx_trx_complete(i2c_imx, true);
 	if (result)
 		return result;
-	result = i2c_imx_acked(i2c_imx);
+	result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
 	if (result)
 		return result;
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
@@ -1372,7 +1380,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
 		result = i2c_imx_trx_complete(i2c_imx, true);
 		if (result)
 			return result;
-		result = i2c_imx_acked(i2c_imx);
+		result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
 		if (result)
 			return result;
 	}
@@ -1549,6 +1557,16 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 	bool is_lastmsg = false;
 	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
 	int use_dma = 0;
+	u16 mangling = I2C_M_NO_RD_ACK | I2C_M_REV_DIR_ADDR;
+
+	/*
+	 * Only I2C_M_IGNORE_NAK is implemented; reject the unsupported
+	 * mangling flags.  I2C_M_STOP is the controller's native
+	 * behaviour and must not be rejected.
+	 */
+	for (i = 0; i < num; i++)
+		if (msgs[i].flags & mangling)
+			return -EOPNOTSUPP;
 
 	/* Start I2C transfer */
 	result = i2c_imx_start(i2c_imx, atomic);
@@ -1604,6 +1622,14 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 
 		use_dma = i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD &&
 			msgs[i].flags & I2C_M_DMA_SAFE;
+
+		/*
+		 * I2C_M_IGNORE_NAK is honoured on the PIO paths only; a
+		 * NAK'd byte does not generate a further DMA request
+		 * (mirrors i2c-img-scb).
+		 */
+		if (msgs[i].flags & I2C_M_IGNORE_NAK)
+			use_dma = false;
 		if (msgs[i].flags & I2C_M_RD) {
 			int block_data = msgs->flags & I2C_M_RECV_LEN;
 
@@ -1697,7 +1723,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 static u32 i2c_imx_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
-		| I2C_FUNC_SMBUS_READ_BLOCK_DATA;
+		| I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_PROTOCOL_MANGLING;
 }
 
 static const struct i2c_algorithm i2c_imx_algo = {

base-commit: 04e9bf1648f846976b543e91c1838a712433772a
-- 
2.34.1

Re: [PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

[Carlos Song]

Hi, Haobin

Are you using AI to make this patch?
You can refer the Linux doc:Documentation/process/generated-content.rst.

Please adjust all commit log also code comment,there are two spaces after every period.
Before V3, I have some questions about this patch.

> The i.MX controller has no hardware "ignore NAK" enable bit and checks
> each byte in software after transmitting it: i2c_imx_acked() on the
> atomic paths, i2c_imx_isr_acked() on the ISR path.  Any NAK is
> therefore treated as a fatal error and aborts the transfer with -ENXIO.
>
> There are legitimate cases where a target NAKs on purpose and the
> transfer must continue anyway.  The standard way to express this is
> I2C_M_IGNORE_NAK, which the core passes through to the bus driver
> unchanged: i2c_transfer_buffer_flags() performs no validation of the
> message flags, and the i.MX driver has never advertised
> I2C_FUNC_PROTOCOL_MANGLING.  Today such a transfer transmits bytes
> until the first NAK and then fails with -ENXIO, silently dropping the
> flag.  One in-tree caller that hits this on i.MX buses is the wake
> sequence of the Atmel crypto drivers (drivers/crypto/atmel-i2c.c, used
> by atmel-ecc.c for the ATECC508A/608A): the sleeping device NAKs the
> wake token and the write must complete anyway; the ATECC508A is wired
> to an i.MX I2C controller e.g. on the Gateworks GW5904.

Have you meet one issue on one real platform? Can you share the detail about your
devices and platform?

> Advertise I2C_FUNC_PROTOCOL_MANGLING and honour I2C_M_IGNORE_NAK at
> every software ACK check: on the PIO write paths, which check the ACK
> of the address and of every data byte, and on the PIO read paths, which
> check the target's ACK of the address byte, mirroring i2c-img-scb.
> With the flag set a NAK no longer aborts the transfer and the byte loop
> continues so the remaining bytes are still transmitted.  Every transfer
> without the flag keeps the previous behaviour unchanged.
>
> A NAK'd byte does not generate a further DMA request, so a DMA write to
> a deliberately-NAKing target would stall until DMA_TIMEOUT instead of
> completing.  Force the PIO path for every message carrying
> I2C_M_IGNORE_NAK, again mirroring i2c-img-scb, which implements the
> flag in atomic mode only.  The DMA write path therefore never sees the
> flag, but it passes the flag through at the final ACK check like the
> other paths.
>
> I2C_FUNC_PROTOCOL_MANGLING is a blanket capability bit, so advertising
> it while implementing only I2C_M_IGNORE_NAK would silently ignore the
> other mangling flags.  Reject I2C_M_NO_RD_ACK and I2C_M_REV_DIR_ADDR in
> xfer_common() instead; no in-tree user of these flags transfers through
> i2c-imx.  I2C_M_NO_RD_ACK is not implementable at all on this
> controller: it always clocks the ninth acknowledge bit and I2CR[TXAK]
> only selects ACK versus NAK.  I2C_M_STOP is deliberately not rejected:
> the controller already terminates every transfer with a STOP and
> separates messages with repeated STARTs, so leaving the flag unhandled
> changes nothing on the bus, while rejecting it would break the ov2659
> camera on imx6qdl-ds: I2C_CLIENT_SCCB is I2C_M_STOP | I2C_M_IGNORE_NAK
> and the driver stamps client->flags on every message, including the
> chip-ID reads done at probe time.

You don't really support theI2C_M_STOP in the code and intentionallyignore I2C_M_STOP check to avoid breaking the imx6qdl-ds,right? Do you work on this platform?

> No retry is introduced: the meaning of a NAK is device-specific [1], so
> it stays a device-driver decision.  I2C_M_IGNORE_NAK is only honoured
> when the device driver explicitly sets it, which is exactly how the
> flag is meant to be used.
>
> Link: https://patchwork.ozlabs.org/project/linux-i2c/patch/1467900229-5262-1-git-send-email-tharvey@gateworks.com/ # [1]
> Link: https://lore.kernel.org/linux-i2c/4D46D571.5010907@armadeus.com/ # [2]
> Link: https://patchwork.ozlabs.org/project/linux-i2c/patch/1378857490-30968-1-git-send-email-luka@openwrt.org/ # [3]
> Signed-off-by: Haobin Jiang <lemonoutput@foxmail.com>

Because there is a similar patch[4] in upstream thread. It is to add I2C_M_REV_DIR_ADDR and
I2C_M_NOSTART flags. So your patch will confict with this patch[4].

So can you please rebase your patch on patch[4]?

[4] https://lore.kernel.org/linux-i2c/apcYp0Zc7CrJMiOh@lizhi-Precision-Tower-5810/T/#t

>
> ---
> v1 -> v2:
>          - Honour I2C_M_IGNORE_NAK on the read path as well, at the checks
>            of the target's ACK of the address byte (i2c_imx_isr_read(),
>            i2c_imx_prepare_read()), mirroring i2c-img-scb, instead of
>            silently dropping the flag there.  Read requests carrying the
>            flag cannot be rejected, because in-tree SCCB sensors such as
>            ov2659 set I2C_CLIENT_SCCB, i.e. I2C_M_STOP |
>            I2C_M_IGNORE_NAK, on their read messages too.
>          - Force the PIO path for every message with I2C_M_IGNORE_NAK: a
>            NAK'd byte does not generate a further DMA request, so a DMA
>            write to a deliberately-NAKing target would stall until
>            DMA_TIMEOUT (mirrors i2c-img-scb).  i2c_imx_dma_write() can
>            therefore never see the flag, though it still passes it
>            through at the final ACK check like the other paths.
>          - Reject I2C_M_NO_RD_ACK and I2C_M_REV_DIR_ADDR in
>            xfer_common() instead of advertising blanket support and
>            silently ignoring them.  I2C_M_STOP is not rejected: the
>            controller already emits a STOP after the final message and
>            repeated STARTs between messages, and rejecting it would break
>            the ov2659 camera on imx6qdl-ds.
>          - Correct the description of the status quo: the core does not
>            reject I2C_M_IGNORE_NAK; i2c_transfer_buffer_flags() passes the
>            message flags through unchanged, so the current driver sends
>            bytes until the first NAK and then fails with -ENXIO.
> ---
>   drivers/i2c/busses/i2c-imx.c | 44 ++++++++++++++++++++++++++++--------
>   1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 28313d0fad37..5b22a497a791 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -612,9 +612,12 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic)
>          return 0;
>   }
>
> -static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
> +static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx, bool ignore_nak)
>   {
>          if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
> +               if (ignore_nak)
> +                       return 0;
> +
>                  dev_dbg(&i2c_imx->adapter.dev, "<%s> No ACK\n", __func__);
>                  return -ENXIO;  /* No ACK */
>          }
> @@ -968,11 +971,15 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
>          return ret;
>   }

After meet NACK, can i2c-imx controller continue to write or read data from FIFO,
after NACK normally stop or repeat start, so I am not sure the i2c-imx controller HW
will work normally, do you test this?

> -static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx)
> +static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx,
> +                                   bool ignore_nak)
>   {
>          i2c_imx->isr_result = 0;
>
>          if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
> +               if (ignore_nak)
> +                       return 0;
> +
>                  i2c_imx->state = IMX_I2C_STATE_FAILED;
>                  i2c_imx->isr_result = -ENXIO;
>                  wake_up(&i2c_imx->queue);
> @@ -985,7 +992,7 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
>   {
>          int result;
>
> -       result = i2c_imx_isr_acked(i2c_imx);
> +       result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>
> @@ -1002,7 +1009,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
>          int result;
>          unsigned int temp;
>
> -       result = i2c_imx_isr_acked(i2c_imx);
> +       result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>
> @@ -1213,7 +1220,8 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
>          if (result)
>                  return result;
>
> -       return i2c_imx_acked(i2c_imx);

Always goto the PIO path with I2C_M_IGNORE_NAK flag,
it is better "return i2c_imx_acked(i2c_imx, false);"?

> +       /* I2C_M_IGNORE_NAK messages always take the PIO path. */
> +       return i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>   }
>
>   static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
> @@ -1227,7 +1235,7 @@ static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
>          result = i2c_imx_trx_complete(i2c_imx, !use_dma);
>          if (result)
>                  return result;
> -       result = i2c_imx_acked(i2c_imx);
> +       result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>
> @@ -1358,7 +1366,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
>          result = i2c_imx_trx_complete(i2c_imx, true);
>          if (result)
>                  return result;
> -       result = i2c_imx_acked(i2c_imx);
> +       result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>          dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
> @@ -1372,7 +1380,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
>                  result = i2c_imx_trx_complete(i2c_imx, true);
>                  if (result)
>                          return result;
> -               result = i2c_imx_acked(i2c_imx);
> +               result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>                  if (result)
>                          return result;
>          }
> @@ -1549,6 +1557,16 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>          bool is_lastmsg = false;
>          struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
>          int use_dma = 0;

This code can be simple if rebase on patch[4] I think.

Regards,
Carlos

> +       u16 mangling = I2C_M_NO_RD_ACK | I2C_M_REV_DIR_ADDR;
> +
> +       /*
> +        * Only I2C_M_IGNORE_NAK is implemented; reject the unsupported
> +        * mangling flags.  I2C_M_STOP is the controller's native
> +        * behaviour and must not be rejected.
> +        */
> +       for (i = 0; i < num; i++)
> +               if (msgs[i].flags & mangling)
> +                       return -EOPNOTSUPP;
>
>          /* Start I2C transfer */
>          result = i2c_imx_start(i2c_imx, atomic);
> @@ -1604,6 +1622,14 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>
>                  use_dma = i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD &&
>                          msgs[i].flags & I2C_M_DMA_SAFE;
> +
> +               /*
> +                * I2C_M_IGNORE_NAK is honoured on the PIO paths only; a
> +                * NAK'd byte does not generate a further DMA request
> +                * (mirrors i2c-img-scb).
> +                */
> +               if (msgs[i].flags & I2C_M_IGNORE_NAK)
> +                       use_dma = false;
>                  if (msgs[i].flags & I2C_M_RD) {
>                          int block_data = msgs->flags & I2C_M_RECV_LEN;
>
> @@ -1697,7 +1723,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>   static u32 i2c_imx_func(struct i2c_adapter *adapter)
>   {
>          return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> -               | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> +               | I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_PROTOCOL_MANGLING;
>   }
>
>   static const struct i2c_algorithm i2c_imx_algo = {
>
> base-commit: 04e9bf1648f846976b543e91c1838a712433772a
> --
> 2.34.1

-- 
Best Regards,
Carlos

Re: [PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

[Haobin Jiang]

Hi Carlos,

Thanks for the review, and sorry for the confusion my v2 caused on
several points. Let me answer your questions.

> Are you using AI to make this patch?

Yes, I did use an AI tool while preparing this patch, and I
apologise for not stating that upfront. I will describe how it was
used in the v3 changelog, as described in
Documentation/process/generated-content.rst.

> Have you meet one issue on one real platform? Can you share the
> detail about your devices and platform?

Yes, an i.MX6ULL on an ALIENTEK i.MX6ULL mini board, with an
ATECC608A at 0x60 on I2C1. I am writing an out-of-tree ECDSA driver
for it that reuses the wake pattern of the upstream atmel-i2c
helper (drivers/crypto/atmel-i2c.c): a wake token sent with
I2C_M_IGNORE_NAK through i2c_transfer_buffer_flags(). On i2c-imx
the transfer aborts with -ENXIO at the first, expected NAK because
the flag is silently dropped, so my driver had to fall back to
bit-banging SDA over GPIO to wake the chip. I will verify the bus
behaviour on that board with a logic analyzer and include the
results and captures with v3.

> After meet NACK, can i2c-imx controller continue to write or read
> data from FIFO, after NACK normally stop or repeat start, so I am
> not sure the i2c-imx controller HW will work normally, do you test
> this?

I will cover this with the tests attached to v3.

> Always goto the PIO path with I2C_M_IGNORE_NAK flag, it is better
> "return i2c_imx_acked(i2c_imx, false);"?

Agreed, v3 will do that.

> You don't really support the I2C_M_STOP in the code and
> intentionally ignore I2C_M_STOP check to avoid breaking the
> imx6qdl-ds,right? Do you work on this platform?

No, I don't have an imx6qdl-ds. The ov2659/SCCB reasoning in the v2
changelog came from reading the DT and driver code, not from
hardware, and it will be removed from v3. I2C_M_STOP will stay
rejected, as in patch [4].

While checking the code I found that patch [4] rejects
I2C_M_STOP, and in-tree ov2659 on imx6qdl-ds sets I2C_CLIENT_SCCB
(I2C_M_IGNORE_NAK | I2C_M_STOP) on every message, including the
chip-ID read at probe, so after [4] lands that probe might fail
with -EOPNOTSUPP. This is only from reading the code and I might be
wrong. Similarly, rejecting I2C_M_IGNORE_NAK might affect the
ATECC508A wake token on imx6qdl-gw5904.

> So can you please rebase your patch on patch[4]?

Yes, v3 will be rebased on top of [4].

I will also fix the sentence spacing in the changelog and comments
as you asked.

Regards,
Haobin

Re: [PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

[Carlos Song]

> Hi Carlos,
>
> Thanks for the review, and sorry for the confusion my v2 caused on
> several points. Let me answer your questions.
>
>> Are you using AI to make this patch?
> Yes, I did use an AI tool while preparing this patch, and I
> apologise for not stating that upfront. I will describe how it was
> used in the v3 changelog, as described in
> Documentation/process/generated-content.rst.

Hi, haobin

It's okay, don't worry about it.

>> Have you meet one issue on one real platform? Can you share the
>> detail about your devices and platform?
> Yes, an i.MX6ULL on an ALIENTEK i.MX6ULL mini board, with an
> ATECC608A at 0x60 on I2C1. I am writing an out-of-tree ECDSA driver
> for it that reuses the wake pattern of the upstream atmel-i2c
> helper (drivers/crypto/atmel-i2c.c): a wake token sent with
> I2C_M_IGNORE_NAK through i2c_transfer_buffer_flags(). On i2c-imx
> the transfer aborts with -ENXIO at the first, expected NAK because
> the flag is silently dropped, so my driver had to fall back to
> bit-banging SDA over GPIO to wake the chip. I will verify the bus
> behaviour on that board with a logic analyzer and include the
> results and captures with v3.

In fact, you just need to support this I2C_M_IGNORE_NAK single flag.
Happy to see this. If you can help provide some strong evidence, it will
help a lot.

>> After meet NACK, can i2c-imx controller continue to write or read
>> data from FIFO, after NACK normally stop or repeat start, so I am
>> not sure the i2c-imx controller HW will work normally, do you test
>> this?
> I will cover this with the tests attached to v3.
>
>> Always goto the PIO path with I2C_M_IGNORE_NAK flag, it is better
>> "return i2c_imx_acked(i2c_imx, false);"?
> Agreed, v3 will do that.
>
>> You don't really support the I2C_M_STOP in the code and
>> intentionally ignore I2C_M_STOP check to avoid breaking the
>> imx6qdl-ds,right? Do you work on this platform?
> No, I don't have an imx6qdl-ds. The ov2659/SCCB reasoning in the v2
> changelog came from reading the DT and driver code, not from
> hardware, and it will be removed from v3. I2C_M_STOP will stay
> rejected, as in patch [4].
>
> While checking the code I found that patch [4] rejects
> I2C_M_STOP, and in-tree ov2659 on imx6qdl-ds sets I2C_CLIENT_SCCB
> (I2C_M_IGNORE_NAK | I2C_M_STOP) on every message, including the
> chip-ID read at probe, so after [4] lands that probe might fail
> with -EOPNOTSUPP. This is only from reading the code and I might be
> wrong. Similarly, rejecting I2C_M_IGNORE_NAK might affect the
> ATECC508A wake token on imx6qdl-gw5904.

This point actually concerns me as well. From my perspective, I agree 
with the V2 approach of skipping the |I2C_M_STOP| check by default. When 
adding new features, we should avoid breaking existing platforms. 
Historically, |i2c-imx.c| did not validate these flags, so ov2659/SCCB 
may have been running in a state that was not fully compliant with the 
intended driver design, but nevertheless worked in practice. Rejecting 
transfers with |I2C_M_STOP| could therefore break the existing 
ov2659/SCCB use case on |imx6qdl-ds|. While the code would continue to 
silently ignore |I2C_M_STOP| during multi-message transfers, this is not 
a new behavior. In fact, allowing |I2C_M_STOP| to pass through is 
intended to preserve existing behavior and maintain backward 
compatibility. What is your view on this? Regards, Carlos

>> So can you please rebase your patch on patch[4]?
> Yes, v3 will be rebased on top of [4].
>
> I will also fix the sentence spacing in the changelog and comments
> as you asked.
>
> Regards,
> Haobin

-- 
Best Regards,
Carlos

Re: [PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

[Carlos Song]

Just change one new email app. Sorry for wrong format in previous email.

Let us continue discussing here!
> Hi Carlos,
> 
> Thanks for the review, and sorry for the confusion my v2 caused on
> several points. Let me answer your questions.
> 
>> Are you using AI to make this patch?
> 
> Yes, I did use an AI tool while preparing this patch, and I
> apologise for not stating that upfront. I will describe how it was
> used in the v3 changelog, as described in
> Documentation/process/generated-content.rst.
> 

Hi, Haobin

It's okay, don't worry about it.

>> Have you meet one issue on one real platform? Can you share the
>> detail about your devices and platform?
> 
> Yes, an i.MX6ULL on an ALIENTEK i.MX6ULL mini board, with an
> ATECC608A at 0x60 on I2C1. I am writing an out-of-tree ECDSA driver
> for it that reuses the wake pattern of the upstream atmel-i2c
> helper (drivers/crypto/atmel-i2c.c): a wake token sent with
> I2C_M_IGNORE_NAK through i2c_transfer_buffer_flags(). On i2c-imx
> the transfer aborts with -ENXIO at the first, expected NAK because
> the flag is silently dropped, so my driver had to fall back to
> bit-banging SDA over GPIO to wake the chip. I will verify the bus
> behaviour on that board with a logic analyzer and include the
> results and captures with v3.
> 

In fact, you just need to support this I2C_M_IGNORE_NAK single flag.
Happy to see this. If you can help provide some strong evidence, it will
help a lot.

>> After meet NACK, can i2c-imx controller continue to write or read
>> data from FIFO, after NACK normally stop or repeat start, so I am
>> not sure the i2c-imx controller HW will work normally, do you test
>> this?
> 
> I will cover this with the tests attached to v3.
> 
>> Always goto the PIO path with I2C_M_IGNORE_NAK flag, it is better
>> "return i2c_imx_acked(i2c_imx, false);"?
> 
> Agreed, v3 will do that.
> 
>> You don't really support the I2C_M_STOP in the code and
>> intentionally ignore I2C_M_STOP check to avoid breaking the
>> imx6qdl-ds,right? Do you work on this platform?
> 
> No, I don't have an imx6qdl-ds. The ov2659/SCCB reasoning in the v2
> changelog came from reading the DT and driver code, not from
> hardware, and it will be removed from v3. I2C_M_STOP will stay
> rejected, as in patch [4].
> 
> While checking the code I found that patch [4] rejects
> I2C_M_STOP, and in-tree ov2659 on imx6qdl-ds sets I2C_CLIENT_SCCB
> (I2C_M_IGNORE_NAK | I2C_M_STOP) on every message, including the
> chip-ID read at probe, so after [4] lands that probe might fail
> with -EOPNOTSUPP. This is only from reading the code and I might be
> wrong. Similarly, rejecting I2C_M_IGNORE_NAK might affect the
> ATECC508A wake token on imx6qdl-gw5904.
> 

This point actually concerns me as well.

From my perspective, I agree with the V2 approach of skipping the
"I2C_M_STOP" check by default. When adding new features, we should avoid
breaking existing platforms.

Historically, "i2c-imx.c" did not validate these flags, so ov2659/SCCB
may have been running in a state that was not fully compliant with the
intended driver design, but nevertheless worked in practice.

Rejecting transfers with "I2C_M_STOP" could therefore break the existing
ov2659/SCCB use case on imx6qdl-ds. While the code would continue to
silently ignore "I2C_M_STOP" during multi-message transfers, this is not
a new behavior.

In fact, allowing "I2C_M_STOP" to pass through is intended to preserve
existing behavior and maintain backward compatibility.

What is your view on this?

>> So can you please rebase your patch on patch[4]?
> 
> Yes, v3 will be rebased on top of [4].
> 
> I will also fix the sentence spacing in the changelog and comments
> as you asked.
> 
> Regards,
> Haobin

-- 
Best Regards,
Carlos