Re: [PATCH 1/1] i2c: imx: honour I2C_M_IGNORE_NAK for write messages

[Carlos Song <carlos.song@oss.nxp.com>]

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