* Re: [PATCH 1/1] i2c: imx: honour I2C_M_IGNORE_NAK for write messages
2026-08-29 14:21 ` [PATCH 1/1] i2c: imx: honour I2C_M_IGNORE_NAK for write messages Haobin Jiang
@ 2026-09-01 7:30 ` Carlos Song
0 siblings, 0 replies; 2+ messages in thread
From: Carlos Song @ 2026-09-01 7:30 UTC (permalink / raw)
To: Haobin Jiang, andi.shyti; +Cc: linux-i2c, linux-kernel
Hi, Haobin
Thank you for your fix.
1. Please use ./scripts/checkpatch.pl to get all maintrainer and
reviewer and send your patch to every one and cc all subsystem.
2. Notice sashiko has reported some issue about this fix[1]. I reviewed
the comments, they looks real, so you can try to fix in V2.
[1]
https://sashiko.dev/#/patchset/tencent_0D2E46A782AC418BC52A1BBF04CA1AC98E09%40qq.com
Best Regards,
Carlos Song
> 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 and DMA paths, i2c_imx_isr_acked() on the non-DMA ISR path. Any
> NAK is treated as a fatal error and aborts the transfer with -ENXIO.
>
> There are legitimate use cases where a target NAKs a write on purpose,
> and when it does, the transfer needs to continue rather than abort. The
> standard way to express this is the I2C_M_IGNORE_NAK flag, which the
> core honours through i2c_transfer_buffer_flags() only when the adapter
> advertises I2C_FUNC_PROTOCOL_MANGLING. The i.MX driver never advertised
> it, so callers using the flag are rejected by the core before any byte
> is sent. One such caller is the in-tree crypto driver atmel-i2c.c,
> whose wake sequence for the ATECC508A/608A requires exactly this: the
> still-asleep device NAKs every byte of the wake token, and the write
> must complete anyway.
>
> Advertise I2C_FUNC_PROTOCOL_MANGLING and honour I2C_M_IGNORE_NAK on the
> write path by passing it through to both ACK checks. With the flag set a
> NAK no longer aborts the transfer and the byte loop continues so the
> remaining bytes are still transmitted. The read path always passes
> false, and every transfer without the flag keeps the previous behaviour
> bit-for-bit unchanged.
>
> 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>
> ---
> drivers/i2c/busses/i2c-imx.c | 27 ++++++++++++++++++---------
> 1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 28313d0fad37..737c19688ecc 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,8 @@ 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);
> + /* Read path always checks NAK; I2C_M_IGNORE_NAK is write-only. */
> + result = i2c_imx_isr_acked(i2c_imx, false);
> if (result)
> return result;
>
> @@ -1213,7 +1221,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> if (result)
> return result;
>
> - return i2c_imx_acked(i2c_imx);
> + 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,8 @@ 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);
> + /* Read address NAK must always be reported; I2C_M_IGNORE_NAK is write-only. */
> + result = i2c_imx_acked(i2c_imx, false);
> if (result)
> return result;
>
> @@ -1358,7 +1367,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 +1381,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;
> }
> @@ -1697,7 +1706,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 = {
> --
> 2.34.1
^ permalink raw reply [flat|nested] 2+ messages in thread