Re: [PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

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

Just change one new email app. Sorry for wrong format in previous email.

Let us continue discussing here!
> 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?

>> 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