apache / apache/nuttx

[BUG] imxrt:lpi2c kconfig and source code discrepancy

Open
#13,114 3 comments 0 reactions 0 assignees View on GitHub
Arch: arm Area: Kconfig OS: Linux Type: Bug
Dominant language
C
Stars
4k
Forks
1.7k
Avg merge
1d 17h
Merged PRs (30d)
237

Description

### Description / Steps to reproduce the issue

The [imxrt's kconfig](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/Kconfig) provides configs for I2C transaction timeouts:
- You can specify in terms of seconds and milliseconds ([IMXRT_LPI2C_TIMEOSEC](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/Kconfig#L2662C8-L2662C28) and [IMXRT_LPI2C_TIMEOMS](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/Kconfig#L2667C8-L2667C27)), or
- CPU clock ticks ([IMXRT_LPI2C_TIMEOTICKS](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/Kconfig#L2672C8-L2672C30))

The compile-time configuration logic is performed in [these lines](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_lpi2c.c#L74-L95) of `imcrt_lpi2c.c`. At the end of this code block, assuming you're not using dynamic timeouts, `CONFIG_IMXRT_LPI2C_TIMEOTICKS` will be the value the driver uses for timeouts (whether it uses the value set by the user in a defconfig/kconfig or it's manually computed from the supplied seconds and ms)

### Issue 1
Assuming the top-level I2C config is enabled and `IMXRT_LPI2C_DYNTIMEO` is not set, **these are always defined and given default values** in the kconfig. If you want to define timeouts in terms of s and ms instead of ticks, then the lines on 86 - 90 should handle that, but in reality they don't and are dead code because **CONFIG_IMXRT_LPI2C_TIMEOTICKS is always defined (since it has a default value)**.
**Solution**: Remove the default value of `IMXRT_LPI2C_TIMEOTICKS` from the kconfig
**Solution Impact on Users**: When users upgrade to the latest nuttx, if users never explicitly set this value in a `defconfig`, then the defaults values of `IMXRT_LPI2C_TIMEOSEC` and `IMXRT_LPI2C_TIMEOMS` will be used instead, which may not result in the same number of timeout ticks as before

### Issue 2

[IMXRT_LPI2C_DYNTIMEO_STARTSTOP](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/Kconfig#L2657) isn't really used in the file. It's value is [just set to a value](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_lpi2c.c#L92-L95) in the config block on lines 92 - 95 and never used again.
**Solution**: Remove this from the kconfig and from the source file
**Solution Impact on Users**: none, except a compile-time failure because of a missing config option if they were explicitly setting it in a `defconfig`

### On which OS does this issue occur?

[Linux]

### What is the version of your OS?

Ubuntu 22.04

### NuttX Version

master

### Issue Architecture

[arm]

### Issue Area

[Kconfig]

### Verification

- [X] I have verified before submitting the report.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.