bsp: k230: add dma support to the i2c driver #10973
Conversation
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
📌 Code Review Assignment🏷️ Tag: bsp_k230Reviewers: unicornx Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-11-24 21:18 CST)
📝 Review Instructions
|
Rbb666
added
Arch: RISC-V
There was a problem hiding this comment.
Pull request overview
This pull request adds DMA support to the K230 I2C driver to improve data transfer performance. However, the PR has several critical issues that need to be addressed.
Key Changes:
- Added PDMA (Peripheral DMA) support for I2C read and write operations
- Added Kconfig option
BSP_USING_I2C_DMAto enable/disable DMA functionality - Updated test cases to verify DMA operations with different data sizes
- Modified driver to fall back to CPU polling when data size is not 4-byte aligned
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
bsp/k230/drivers/interdrv/i2c/drv_i2c.c |
Core driver changes: added DMA transfer functions, modified device structure to include physical address and DMA configuration |
bsp/k230/drivers/utest/test_i2c.c |
Updated test documentation and adjusted test data sizes to exercise both DMA and non-DMA code paths |
bsp/k230/board/Kconfig |
Added configuration option to enable I2C DMA support |
bsp/k230/.ci/attachconfig/ci.attachconfig.yml |
Enabled I2C DMA in CI testing configuration |
Critical Issues Found (Must Fix Before Merge):
🔴 Multiple Resource Leaks: The DMA implementation has systematic memory and resource leaks on error paths. DMA channels are not released when operations fail, which will eventually exhaust available channels.
🔴 Missing I2C Transaction Finish: Both DMA read and write functions don't call k230_i2c_xfer_finish(), which may leave the I2C bus in an incorrect state.
🔴 Incorrect Error Handling: Event receive errors and timeout events are not checked, allowing functions to return success even when DMA operations fail.
🟡 Code Style Violations: The PR removes spaces between if/switch keywords and parentheses (e.g., if(...) instead of if (...)), violating RT-Thread coding standards.
🟡 Code Duplication: Device name to PDMA device ID mapping is duplicated in read and write functions.
PR Metadata Issues:
According to RT-Thread PR review guidelines, this PR has the following issues:
-
Title Format: The title "bsp: k230: add dma support to the i2c driver" doesn't follow the required format. It should be:
[k230][i2c] Add DMA support to I2C driver -
Empty Description: The PR description template was not filled out. According to the guidelines, it must include:
- Why this PR is being submitted
- What problem is solved and the solution
- Which BSP and config were used for verification
- Action link showing successful compilation
Please update the PR title and fill in the description before this PR can be reviewed for merge.
| RT_ASSERT(bus != RT_NULL); | ||
|
|
||
| switch (cmd) | ||
| switch(cmd) |
There was a problem hiding this comment.
Code Style Issue: Inconsistent spacing with if, switch / 代码风格问题：if、switch 的空格不一致
English: Missing space between keyword and (. Should be if (...) and switch (...) according to RT-Thread coding standards.
中文：关键字和 ( 之间缺少空格。根据 RT-Thread 编码标准，应该是 if (...) 和 switch (...)。
| switch(cmd) | |
| switch (cmd) |
| err = k230_pdma_set_callback(ch, k230_i2c_pdma_call_back); | ||
| if(err != RT_EOK) | ||
| { | ||
| LOG_E("i2c pdma set callback failed"); |
There was a problem hiding this comment.
Bug: Memory leak on early return / 错误：提前返回时的内存泄漏
English: If k230_pdma_request_channel or k230_pdma_set_callback fails, the function returns without releasing the allocated DMA channel. The channel should be released before returning.
中文：如果 k230_pdma_request_channel 或 k230_pdma_set_callback 失败，函数会在没有释放已分配的 DMA 通道的情况下返回。应该在返回前释放通道。
Suggested fix / 建议修复:
err = k230_pdma_set_callback(ch, k230_i2c_pdma_call_back);
if(err != RT_EOK)
{
LOG_E("i2c pdma set callback failed");
k230_pdma_release_channel(ch);
dev->pdma_cfg.ch = 0xFF;
return err;
}| LOG_E("i2c pdma set callback failed"); | |
| LOG_E("i2c pdma set callback failed"); | |
| k230_pdma_release_channel(ch); | |
| dev->pdma_cfg.ch = 0xFF; |
| K230_I2C_PDMA_EVENT_COMPLETE | K230_I2C_PDMA_EVENT_TIMEOUT, | ||
| RT_EVENT_FLAG_OR | RT_EVENT_FLAG_CLEAR, | ||
| RT_WAITING_FOREVER, | ||
| &recv_event); |
There was a problem hiding this comment.
Bug: Incorrect return value handling / 错误：不正确的返回值处理
English: The function checks the return value of rt_event_recv but doesn't handle errors. If the event receive fails, the function continues to execute and may return success even though the DMA operation failed. Additionally, the recv_event should be checked to determine if a timeout occurred.
中文：函数检查 rt_event_recv 的返回值但没有处理错误。如果事件接收失败，函数会继续执行，即使 DMA 操作失败也可能返回成功。此外，应该检查 recv_event 以确定是否发生超时。
Suggested fix / 建议修复:
err = rt_event_recv(dev->pdma_cfg.event,
K230_I2C_PDMA_EVENT_COMPLETE | K230_I2C_PDMA_EVENT_TIMEOUT,
RT_EVENT_FLAG_OR | RT_EVENT_FLAG_CLEAR,
RT_WAITING_FOREVER,
&recv_event);
if (err != RT_EOK || (recv_event & K230_I2C_PDMA_EVENT_TIMEOUT))
{
LOG_E("i2c pdma event receive failed or timeout");
k230_pdma_stop(ch);
k230_pdma_release_channel(ch);
dev->pdma_cfg.ch = 0xFF;
rt_free_align(buf);
return -RT_ERROR;
}| &recv_event); | |
| &recv_event); | |
| if (err != RT_EOK || (recv_event & K230_I2C_PDMA_EVENT_TIMEOUT)) | |
| { | |
| LOG_E("i2c pdma event receive failed or timeout"); | |
| k230_pdma_stop(ch); | |
| k230_pdma_release_channel(ch); | |
| dev->pdma_cfg.ch = 0xFF; | |
| rt_free_align(buf); | |
| return -RT_ERROR; | |
| } |
| { | ||
| return -RT_ETIMEOUT; | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Missing xfer_finish call / 错误：缺少 xfer_finish 调用
English: The k230_i2c_pdma_read function doesn't call k230_i2c_xfer_finish() before returning, unlike the non-DMA k230_i2c_read function (line 340). This may cause the I2C bus to remain in an incorrect state. The finish call should be added before returning RT_EOK.
中文：k230_i2c_pdma_read 函数在返回前没有调用 k230_i2c_xfer_finish()，这与非 DMA 的 k230_i2c_read 函数（第 340 行）不同。这可能导致 I2C 总线保持在不正确的状态。应该在返回 RT_EOK 前添加 finish 调用。
| k230_i2c_xfer_finish(dev); |
| return -RT_EINVAL; | ||
| } | ||
|
|
||
| buf = (rt_uint32_t *)rt_malloc_align(tran_len * 4, 64); |
There was a problem hiding this comment.
[nitpick] Performance: Inefficient buffer allocation / 性能：低效的缓冲区分配
English: The buffer is allocated as tran_len * 4 bytes but only tran_len bytes of data are actually used (one byte per 32-bit word). This wastes 3x memory. A more memory-efficient approach would allocate only tran_len bytes for a temporary buffer to hold the expanded data.
中文：缓冲区分配了 tran_len * 4 字节，但实际只使用了 tran_len 字节的数据（每个 32 位字一个字节）。这浪费了 3 倍的内存。更高效的方法是只为临时缓冲区分配 tran_len 字节来保存扩展数据。
Note: This is the correct approach for the I2C data_cmd register which requires 32-bit writes, but the allocation size should match the actual size needed.
注意：这是 I2C data_cmd 寄存器的正确方法（需要 32 位写入），但分配大小应该与实际需要的大小匹配。
| buf = (rt_uint32_t *)rt_malloc_align(tran_len * 4, 64); | |
| buf = (rt_uint32_t *)rt_malloc_align(tran_len * sizeof(rt_uint32_t), 64); |
| read_len = dev->msg->len; | ||
| if(read_len % 4) | ||
| { | ||
| LOG_E("i2c pdma write length must be 4 byte aligned"); |
There was a problem hiding this comment.
Error Message Issue: Incorrect error message / 错误消息问题：错误的错误消息
English: The error message says "i2c pdma write length must be 4 byte aligned" but this is in the read function. It should say "i2c pdma read length must be 4 byte aligned".
中文：错误消息说"i2c pdma write length must be 4 byte aligned"，但这是在读取函数中。应该说"i2c pdma read length must be 4 byte aligned"。
| LOG_E("i2c pdma write length must be 4 byte aligned"); | |
| LOG_E("i2c pdma read length must be 4 byte aligned"); |
| if(buf_pa == RT_NULL) | ||
| { | ||
| LOG_E("i2c pdma get phy addr failed"); | ||
| rt_free_align(buf); |
There was a problem hiding this comment.
Bug: Memory leak on error path / 错误：错误路径上的内存泄漏
English: If rt_kmem_v2p fails, the allocated buffer is freed but the DMA channel is not released. Need to add channel cleanup before returning.
中文：如果 rt_kmem_v2p 失败，分配的缓冲区被释放，但 DMA 通道没有被释放。需要在返回前添加通道清理。
Suggested fix / 建议修复:
if(buf_pa == RT_NULL)
{
LOG_E("i2c pdma get phy addr failed");
rt_free_align(buf);
k230_pdma_release_channel(ch);
dev->pdma_cfg.ch = 0xFF;
return -RT_ERROR;
}| rt_free_align(buf); | |
| rt_free_align(buf); | |
| k230_pdma_release_channel(ch); | |
| dev->pdma_cfg.ch = 0xFF; |
| err = k230_pdma_config(ch, &dev->pdma_cfg.cfg); | ||
| if(err != RT_EOK) | ||
| { | ||
| LOG_E("i2c pdma config failed"); | ||
| rt_free_align(buf); | ||
| return err; | ||
| } | ||
|
|
||
| /* If no start condition is sent before reading, then send a repeated start. */ | ||
| if(dev->msg->flags & RT_I2C_NO_START) | ||
| { | ||
| cmd |= I2C_DATA_CMD_RESTART; | ||
| } | ||
| else | ||
| { | ||
| if(k230_i2c_xfer_init(dev) != RT_EOK) | ||
| { | ||
| return -RT_EBUSY; | ||
| } | ||
| } | ||
|
|
||
| start_time_tx = k230_i2c_get_timer(0); | ||
| err = k230_pdma_start(ch); | ||
| if(err != RT_EOK) | ||
| { | ||
| LOG_E("i2c pdma start failed"); | ||
| rt_free_align(buf); | ||
| return err; | ||
| } |
There was a problem hiding this comment.
Bug: Multiple memory leaks on error paths / 错误：错误路径上的多个内存泄漏
English: If k230_pdma_config, k230_i2c_xfer_init, or k230_pdma_start fails, the allocated buffer is freed but the DMA channel is not released. Need to add proper cleanup.
中文：如果 k230_pdma_config、k230_i2c_xfer_init 或 k230_pdma_start 失败，分配的缓冲区被释放，但 DMA 通道没有被释放。需要添加适当的清理。
Suggested fix for all error paths / 所有错误路径的建议修复:
if(err != RT_EOK)
{
LOG_E("i2c pdma config failed");
rt_free_align(buf);
k230_pdma_release_channel(ch);
dev->pdma_cfg.ch = 0xFF;
return err;
}| err = k230_pdma_set_callback(ch, k230_i2c_pdma_call_back); | ||
| if(err != RT_EOK) | ||
| { | ||
| LOG_E("i2c pdma set callback failed"); |
There was a problem hiding this comment.
Bug: Memory leak on early return / 错误：提前返回时的内存泄漏
English: If k230_pdma_set_callback fails, the function returns without releasing the DMA channel that was successfully allocated.
中文：如果 k230_pdma_set_callback 失败，函数会在没有释放已成功分配的 DMA 通道的情况下返回。
Suggested fix / 建议修复:
err = k230_pdma_set_callback(ch, k230_i2c_pdma_call_back);
if(err != RT_EOK)
{
LOG_E("i2c pdma set callback failed");
k230_pdma_release_channel(ch);
dev->pdma_cfg.ch = 0xFF;
return err;
}| LOG_E("i2c pdma set callback failed"); | |
| LOG_E("i2c pdma set callback failed"); | |
| k230_pdma_release_channel(ch); | |
| dev->pdma_cfg.ch = 0xFF; |
| buf_pa = rt_kmem_v2p(buf); | ||
| if(buf_pa == RT_NULL) | ||
| { | ||
| LOG_E("i2c pdma get phy addr failed"); | ||
| rt_free_align(buf); | ||
| return -RT_ERROR; | ||
| } | ||
|
|
||
| rt_event_control(dev->pdma_cfg.event, RT_IPC_CMD_RESET, NULL); | ||
|
|
||
| if(strcmp(dev->name, "i2c0") == 0) | ||
| { | ||
| dev->pdma_cfg.cfg.device = I2C0_TX; | ||
| } | ||
| else if(strcmp(dev->name, "i2c1") == 0) | ||
| { | ||
| dev->pdma_cfg.cfg.device = I2C1_TX; | ||
| } | ||
| else if(strcmp(dev->name, "i2c2") == 0) | ||
| { | ||
| dev->pdma_cfg.cfg.device = I2C2_TX; | ||
| } | ||
| else if(strcmp(dev->name, "i2c3") == 0) | ||
| { | ||
| dev->pdma_cfg.cfg.device = I2C3_TX; | ||
| } | ||
| else if(strcmp(dev->name, "i2c4") == 0) | ||
| { | ||
| dev->pdma_cfg.cfg.device = I2C4_TX; | ||
| } | ||
| dev->pdma_cfg.cfg.src_addr = (rt_uint8_t *)buf_pa; | ||
| dev->pdma_cfg.cfg.dst_addr = (rt_uint8_t *)(dev->base_pa + 0x10); | ||
| dev->pdma_cfg.cfg.line_size = tran_len * 4; | ||
| dev->pdma_cfg.cfg.pdma_ch_cfg.ch_src_type = CONTINUE; | ||
| dev->pdma_cfg.cfg.pdma_ch_cfg.ch_dev_hsize = PSBYTE4; | ||
| dev->pdma_cfg.cfg.pdma_ch_cfg.ch_dat_endian = PDEFAULT; | ||
| dev->pdma_cfg.cfg.pdma_ch_cfg.ch_dev_blen = PBURST_LEN_4; | ||
| dev->pdma_cfg.cfg.pdma_ch_cfg.ch_priority = 7; | ||
| dev->pdma_cfg.cfg.pdma_ch_cfg.ch_dev_tout = 0xFFF; | ||
|
|
||
| err = k230_pdma_config(ch, &dev->pdma_cfg.cfg); | ||
| if(err != RT_EOK) | ||
| { | ||
| LOG_E("i2c pdma config failed"); | ||
| rt_free_align(buf); | ||
| return err; | ||
| } | ||
|
|
||
| if(k230_i2c_xfer_init(dev) != RT_EOK) | ||
| { | ||
| return -RT_EBUSY; | ||
| } | ||
|
|
||
| start_time_tx = k230_i2c_get_timer(0); | ||
| err = k230_pdma_start(ch); | ||
| if(err != RT_EOK) | ||
| { | ||
| LOG_E("i2c pdma start failed"); | ||
| rt_free_align(buf); | ||
| return err; | ||
| } |
There was a problem hiding this comment.
Bug: Multiple memory leaks on error paths / 错误：错误路径上的多个内存泄漏
English: Similar to the read function, if rt_kmem_v2p, k230_pdma_config, k230_i2c_xfer_init, or k230_pdma_start fails, the allocated buffer is freed but the DMA channel is not released.
中文：与读取函数类似，如果 rt_kmem_v2p、k230_pdma_config、k230_i2c_xfer_init 或 k230_pdma_start 失败，分配的缓冲区被释放，但 DMA 通道没有被释放。
All error paths should include:
所有错误路径都应包括：
k230_pdma_release_channel(ch);
dev->pdma_cfg.ch = 0xFF;
Ze-Hou
force-pushed
the
dev
branch
2 times, most recently
from
f88e4ff to
4813319
Compare
unicornx
left a comment
unicornx
left a comment
There was a problem hiding this comment.
除了相关 review 意见外，有一个需要注意的就是，以后逻辑修改和代码格式修改不要作为一个 commit 提交评审，否则看起来实在费劲。这次就算了吧。
There was a problem hiding this comment.
.config 没有实质的改动，不要提交.
同理对于 rtconfig.h
There was a problem hiding this comment.
这边应该是其他地方有删减，我restore .config rtconfig.h后重新打开menuconfig什么都不做然后保存后就有删除的改动了
| rt_uint16_t spklen; | ||
| }; | ||
|
|
||
| struct _i2c_pdma_cfg |
There was a problem hiding this comment.
这个结构体应该只有启用了 BSP_USING_I2C_DMA 才被定义。
| rt_uint32_t clock; | ||
| struct rt_i2c_msg *msg; | ||
| struct _i2c_speed_cfg speed_cfg; | ||
| struct _i2c_pdma_cfg pdma_cfg; |
There was a problem hiding this comment.
这个 pdma_cfg 成员应该只有启用了 BSP_USING_I2C_DMA 才被定义。
| { | ||
| ret = k230_i2c_pdma_write(i2c_dev); | ||
| } | ||
| } |
There was a problem hiding this comment.
我看你在 测试中有注释 “因为pdma要求地址与数据都要4字节对齐”，但从这里代码看，知识判断了数据长度是4 的倍数，这个是不是不够充分啊?
另外相关的注释应该写在驱动中而不是测试代码中。
There was a problem hiding this comment.
地址会在pdma read与write做处理，所以不关心传进的地址是不是4字节对齐的
| else if (strcmp(dev->name, "i2c4") == 0) | ||
| { | ||
| dev->pdma_cfg.cfg.device = is_tx ? I2C4_TX : I2C4_RX; | ||
| } |
There was a problem hiding this comment.
I2C0_TX 和 I2C0_RX 这样的值建议直接作为 k230_i2c_dev 的成员在初始化时直接赋值保存算了，然后在 k230_i2c_pdma_read 和 k230_i2c_pdma_write 中直接对 dev->pdma_cfg.cfg.device 进行赋值。
这个函数也不用了，估计还省点空间。
| else | ||
| { | ||
| ret = _k230_i2c_write(i2c_dev); | ||
| ret = k230_i2c_write(i2c_dev); |
There was a problem hiding this comment.
另外我觉得目前的这段代码写的有改进的地方，我有个方案如下：
以读为例，修改 k230_i2c_read 如下，这样原 k230_i2c_xfer 函数也不用改了。
static int k230_i2c_read(struct k230_i2c_dev *dev)
{
#ifdef BSP_USING_I2C_DMA
if (地址与数据长度都是 4 的倍数)
return k230_i2c_pdma_read(i2c_dev);
#endif
...... 原 k230_i2c_read 逻辑
}写操作部分类似。
bsp/k230/drivers/utest/test_i2c.c
Outdated
| * 第二次会调用dma进行读。（前提是BSP_USING_I2C_DMA宏被定义， | ||
| * 因为pdma要求地址与数据都要4字节对齐，若写/读数据大小非4字节 | ||
| * 对齐，即使启用了dma功能，实际也是调用的cpu轮询读写，这一点需要 | ||
| * 上层程序注意并处理） |
There was a problem hiding this comment.
这里我想表达的意思是，如果读或写的长度不是4字节对齐，用户可以先处理对齐的部分，然后再处理非对齐的部分，因为i2c在写的时候第一部分往往是写的地址，这如果在bsp先处理对齐部分，然后再处理非对其部分了，就有点臃肿了
1. add dma support to the I2C driver to improve efficiency. Signed-off-by: Ze-Hou <yingkezhou@qq.com>
|
@Rbb666 please review, thanks. |
| #include <rtdevice.h> | ||
| #include <riscv_io.h> | ||
| #include <ioremap.h> | ||
| #include <mmu.h> |
There was a problem hiding this comment.
这个是rt_kmem_v2p用到了，<mmu.h>里边声明了
3 participants
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up