Fixes mimxrt10xx: Wrong I2C transfer status #13819

[CristXu]

Fixes : mimxrt10xx: Wrong I2C transfer status #13819

Adding a delay between a transfer, then detect the NACK flag

Signed-off-by: [email protected] [email protected]

[pabigot]

I can confirm this fixes scanning, but I'm not convinced it's a good idea to have a delay of unclear duration before every transfer.

[pabigot]

Can you replace this with k_busy_wait()? It's gotta be a time-based delay, and an empty loop isn't the right way to do that.

[CristXu]

ok, Has updated

[MaureenHelm]

This looks like it uses the system clock frequency (600 MHz), will it work with other clock frequencies? You probably need to use clock_control_get_rate similar to mcux_lpi2c_configure.

[pabigot]

Yes; and the logic behind this calculation needs some explanation. Why is the numerator 1.5E12?

[CristXu]

Hi，Be honest, i test many values, the 10000 * 150 / (baudrate / 1000) is an experience one. I do not know why too. But it works. bigger is maybe ok, but lower, not.
The values: the 1e6 means -> us, 10 is a simmliar ticks with (for (i=0;i<xx;i++), this will expand to 10 instructs costs about
10 ticks), the 600000000 is sure, the cpu frequency.

[CristXu]

This looks like it uses the system clock frequency (600 MHz), will it work with other clock frequencies? You probably need to use clock_control_get_rate similar to mcux_lpi2c_configure.

Ok， I will update the code soon.

[CristXu]

This looks like it uses the system clock frequency (600 MHz), will it work with other clock frequencies? You probably need to use clock_control_get_rate similar to mcux_lpi2c_configure.

Hi, I have updated the macro with a function to get the CPU_FREQ, But I use CLOCK_GetCoreSysClkFreq instead, because the clock_control_get_rate can not return the core_freq, or do we need to add this into the mcux_ccm_get_subsys_rate()?

[MaureenHelm]

Please also update the PR summary to say Fixes #xxxxx
https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue

[pabigot]

Looking more closely: this approach will not work.

The solution involves generating a start condition followed by a stop condition before every call to the I2C transfer function. This will break transactions that span multiple calls, i.e. starting in one then finishing in another.

(That the cost of doing the check affects every transaction, and that the duration of the delay is not based on analysis, is also a concern: but the fact this will break I2C is what rules it out.)

[CristXu]

Looking more closely: this approach will not work.

The solution involves generating a start condition followed by a stop condition before every call to the I2C transfer function. This will break transactions that span multiple calls, i.e. starting in one then finishing in another.

(That the cost of doing the check affects every transaction, and that the duration of the delay is not based on analysis, is also a concern: but the fact this will break I2C is what rules it out.)

Hi, I do some changes, move the delay inside the transfer body, remove the start + stop, but the delay time is neccessary still. Or Adding a judge, so we can add this delay only when run the i2c scan command?

[pabigot]

If you do this wait only when msgs->len == 0 does that fix the problem? Because that's the case that isn't being handled correctly by the HAL.

I'd still like to see the delay duration based on something that makes sense, say a specific number of cycles of the I2C clock.

[CristXu]

Hi， Yes ， I add one if(msgs->len == 0), it works. Now the delay time is depend on CPU_FREQ & i2c's baudrate.

[pabigot]

Please change this to:

/* Wait for the duration of 12 bits to detect a NAK after a bus
 * address scan.  (10 appears sufficient, 20% safety factor.)
 */
#define SCAN_DELAY_US(baudrate) (12 * USEC_PER_SEC / baudrate)

and update the name of the macro below. This decouples from the clock rate (which is irrelevant once we wait for microseconds) and makes the purpose of the delay more clear.

[CristXu]

Please change this to:

/* Wait for the duration of 12 bits to detect a NAK after a bus
 * address scan.  (10 appears sufficient, 20% safety factor.)
 */
#define SCAN_DELAY_US(baudrate) (12 * USEC_PER_SEC / baudrate)

and update the name of the macro below. This decouples from the clock rate (which is irrelevant once we wait for microseconds) and makes the purpose of the delay more clear.

Hi，Yes, this macro looks more clearly now and I have tried it, it works and I have updated the new code.

[MaureenHelm]

Thanks @pabigot for your help. This change looks much better now.

@CristXu please amend the commit message to indicate this change is for the mcux i2c driver, something like this:

drivers: i2c: Fix mcux driver transfer status after NACK

Adds a delay after transferring zero-length messages to correctly detect a NACK.

[CristXu]

Thanks @pabigot for your help. This change looks much better now.

@CristXu please amend the commit message to indicate this change is for the mcux i2c driver, something like this:

drivers: i2c: Fix mcux driver transfer status after NACK

Adds a delay after transferring zero-length messages to correctly detect a NACK.

Updated