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