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