[PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK
Carlos Song
carlos.song at oss.nxp.com
Thu Sep 3 03:12:06 PDT 2026
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 at 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
More information about the linux-arm-kernel
mailing list