[gd32][i2c] Add hardware I2C driver for GD32VW553xx #11151
Conversation
|
|
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
|
感谢PR，还请作者参考链接帮忙添加上ci-attach编译检查： https://club.rt-thread.org/ask/article/5edd0f0940a57129.html |
There was a problem hiding this comment.
Pull request overview
This PR adds hardware I2C driver support for the GD32VW553xx RISC-V device series. The implementation provides a complete I2C master mode driver with support for multiple pin configurations and configurable clock speeds.
Changes:
- Implements hardware I2C driver for GD32VW553xx with read/write operations and reload mode support
- Adds Kconfig configuration options for I2C0 and I2C1 with multiple pin selection choices
- Integrates build system changes to compile the new driver when hardware I2C is enabled
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| bsp/gd32/risc-v/libraries/gd32_drivers/drv_i2c.h | Header file defining I2C device and configuration structures |
| bsp/gd32/risc-v/libraries/gd32_drivers/drv_i2c.c | Main I2C driver implementation with read/write state machines and hardware initialization |
| bsp/gd32/risc-v/libraries/gd32_drivers/drv_config.h | Common driver configuration header (minimal, provides basic includes) |
| bsp/gd32/risc-v/libraries/gd32_drivers/SConscript | Build script additions for conditional compilation of hardware I2C driver |
| bsp/gd32/risc-v/gd32vw553h-eval/board/Kconfig | Menuconfig options for enabling and configuring I2C0/I2C1 peripherals |
| rt_uint32_t i2c_freq; | ||
| rt_uint32_t sclh_scll; | ||
|
|
||
| i2c_freq = rcu_clock_freq_get(CK_I2C0); |
There was a problem hiding this comment.
Hardcoded I2C clock source may not be correct for all I2C peripherals.
English: On line 608, the function uses rcu_clock_freq_get(CK_I2C0) hardcoded for all I2C instances. This is incorrect when configuring I2C1, as it should use the appropriate clock source based on which I2C peripheral is being configured. Consider using a parameter or determining the clock source from the i2c_periph value.
中文：在第 608 行，函数对所有 I2C 实例使用硬编码的 rcu_clock_freq_get(CK_I2C0)。这在配置 I2C1 时是不正确的，因为它应该根据正在配置的 I2C 外设使用适当的时钟源。考虑使用参数或根据 i2c_periph 值确定时钟源。
| i2c_freq = rcu_clock_freq_get(CK_I2C0); | |
| if (i2c_cfg->i2c_periph == I2C0) | |
| { | |
| i2c_freq = rcu_clock_freq_get(CK_I2C0); | |
| } | |
| else if (i2c_cfg->i2c_periph == I2C1) | |
| { | |
| i2c_freq = rcu_clock_freq_get(CK_I2C1); | |
| } | |
| else | |
| { | |
| /* fallback to I2C0 clock to keep backward compatibility */ | |
| i2c_freq = rcu_clock_freq_get(CK_I2C0); | |
| } |
| /* this driver can be enable at | ||
| menuconfig -> | ||
| Hardware Drivers Config -> | ||
| On-chip Peripheral Drivers -> | ||
| Enable Hardware I2C | ||
| */ | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Comment lacks proper English grammar and punctuation.
English: The comment on line 17 reads "this driver can be enable at" which should be "this driver can be enabled at". Also, the multi-line comment structure using block comments for menu path is unusual. Consider using standard single-line comments with proper grammar.
中文：第 17 行的注释显示 "this driver can be enable at"，应为 "this driver can be enabled at"。此外，使用块注释表示菜单路径的多行注释结构不寻常。考虑使用具有正确语法的标准单行注释。
| /* this driver can be enable at | |
| menuconfig -> | |
| Hardware Drivers Config -> | |
| On-chip Peripheral Drivers -> | |
| Enable Hardware I2C | |
| */ | |
| #endif | |
| /* This driver can be enabled in menuconfig: | |
| * Hardware Drivers Config -> | |
| * On-chip Peripheral Drivers -> | |
| * Enable Hardware I2C | |
| */ | |
| #endif |
| @@ -0,0 +1,729 @@ | |||
| /* | |||
There was a problem hiding this comment.
PR title should follow RT-Thread format: [module][subsystem] Description in lowercase. The current title uses colons instead of brackets.
English: The PR title should be formatted as [gd32][i2c] Add hardware I2C driver for GD32VW553xx to follow RT-Thread naming conventions. The prefix should use brackets instead of colons and be in lowercase.
中文：PR 标题应遵循 RT-Thread 格式：[gd32][i2c] Add hardware I2C driver for GD32VW553xx。前缀应使用方括号而不是冒号，并且应为小写。
| typedef enum | ||
| { | ||
| I2C_START = 0, | ||
| I2C_SEND_ADDRESS, | ||
| I2C_RESTART, | ||
| I2C_TRANSMIT_DATA, | ||
| I2C_RELOAD, | ||
| I2C_STOP, | ||
| I2C_END | ||
| } i2c_process_enum; |
There was a problem hiding this comment.
Naming convention issue - enum values should follow RT-Thread naming style.
English: The enum i2c_process_enum uses values like I2C_START, I2C_SEND_ADDRESS etc. According to RT-Thread coding standards, enum values should follow the pattern with lowercase prefix. However, this may be acceptable if it follows GD32 HAL conventions. Verify consistency with other RT-Thread drivers.
中文：枚举 i2c_process_enum 使用 I2C_START、I2C_SEND_ADDRESS 等值。根据 RT-Thread 编码标准，枚举值应遵循小写前缀模式。但是，如果它遵循 GD32 HAL 约定，这可能是可以接受的。验证与其他 RT-Thread 驱动程序的一致性。
| { | ||
| #define I2C_CLK_PSC 7 | ||
| #define I2C_SCLDELY 8 | ||
| #define I2C_SDADELY 0 |
There was a problem hiding this comment.
Unused variable warning - I2C_SDADELY is defined but never used.
English: The macro I2C_SDADELY is defined on line 603 but is never used in the function. It's set to 0 in the calculation on line 613 directly, making the macro definition unnecessary. Either use the macro or remove the definition to keep the code clean.
中文：宏 I2C_SDADELY 在第 603 行定义但从未使用。在第 613 行的计算中直接设置为 0，使宏定义变得不必要。要么使用该宏，要么删除定义以保持代码整洁。
| typedef enum | ||
| { | ||
| I2C_START = 0, | ||
| I2C_SEND_ADDRESS, |
There was a problem hiding this comment.
Unused enum value in i2c_process_enum.
English: The enum value I2C_SEND_ADDRESS is defined but never used in the state machine logic. This dead code should be removed to keep the codebase clean and maintainable.
中文：枚举值 I2C_SEND_ADDRESS 已定义但从未在状态机逻辑中使用。应删除此死代码以保持代码库整洁和可维护。
| I2C_SEND_ADDRESS, |
| /** | ||
| * @brief configure i2c alternate pin | ||
| * | ||
| * @param param onfigure param |
There was a problem hiding this comment.
Typo in comment: "onfigure" should be "configure".
English: The comment on line 574 has a typo. It should read "configure i2c alternate pin" not "onfigure i2c alternate pin".
中文：第 574 行的注释有拼写错误。应为 "configure i2c alternate pin" 而不是 "onfigure i2c alternate pin"。
| * @param param onfigure param | |
| * @param param configure param |
| #ifndef __DRV_I2C__ | ||
| #define __DRV_I2C__ |
There was a problem hiding this comment.
Header guard does not follow codebase conventions.
English: The header guard __DRV_I2C__ does not match the convention used in other drivers. Other drivers in the same directory use the pattern __DRV_XXX_H__ with _H suffix (see drv_usart.h:11, drv_spi.h:11, drv_adc.h:11). This should be __DRV_I2C_H__ for consistency.
中文：头文件保护宏 __DRV_I2C__ 不符合其他驱动程序使用的约定。同一目录中的其他驱动程序使用带 _H 后缀的 __DRV_XXX_H__ 模式（参见 drv_usart.h:11、drv_spi.h:11、drv_adc.h:11）。为保持一致性，这应该是 __DRV_I2C_H__。
| if GetDepend(['RT_USING_I2C', 'RT_USING_I2C_BITOPS']): | ||
| if GetDepend('BSP_USING_I2C0') or GetDepend('BSP_USING_I2C1') or GetDepend('BSP_USING_I2C2') or GetDepend('BSP_USING_I2C3'): | ||
| src += ['drv_soft_i2c.c'] | ||
| if GetDepend(['RT_USING_I2C', 'BSP_USING_HW_I2C']): | ||
| if GetDepend('BSP_USING_HW_I2C0') or GetDepend('BSP_USING_HW_I2C1'): | ||
| src += ['drv_i2c.c'] |
There was a problem hiding this comment.
Potential conflict with soft I2C configuration check.
English: The condition checking on lines 23-25 verifies BSP_USING_HW_I2C0 or BSP_USING_HW_I2C1, but this is nested inside a check for RT_USING_I2C and BSP_USING_HW_I2C. The check is redundant since if neither HW_I2C0 nor HW_I2C1 is defined, the drv_i2c.c file itself has an #error directive (lines 15-23 of drv_i2c.c) that would catch this. However, it's good defensive programming. Consider if both soft I2C and hardware I2C can be enabled simultaneously, as they check for the same RT_USING_I2C flag. This might need clarification in documentation.
中文：第 23-25 行的条件检查验证 BSP_USING_HW_I2C0 或 BSP_USING_HW_I2C1，但这嵌套在 RT_USING_I2C 和 BSP_USING_HW_I2C 的检查中。该检查是冗余的，因为如果既未定义 HW_I2C0 也未定义 HW_I2C1，drv_i2c.c 文件本身就有一个 #error 指令（drv_i2c.c 的第 15-23 行）会捕获这一点。但是，这是良好的防御性编程。考虑软件 I2C 和硬件 I2C 是否可以同时启用，因为它们检查相同的 RT_USING_I2C 标志。这可能需要在文档中澄清。
| if GetDepend(['RT_USING_I2C', 'RT_USING_I2C_BITOPS']): | |
| if GetDepend('BSP_USING_I2C0') or GetDepend('BSP_USING_I2C1') or GetDepend('BSP_USING_I2C2') or GetDepend('BSP_USING_I2C3'): | |
| src += ['drv_soft_i2c.c'] | |
| if GetDepend(['RT_USING_I2C', 'BSP_USING_HW_I2C']): | |
| if GetDepend('BSP_USING_HW_I2C0') or GetDepend('BSP_USING_HW_I2C1'): | |
| src += ['drv_i2c.c'] | |
| if GetDepend('RT_USING_I2C'): | |
| if GetDepend('RT_USING_I2C_BITOPS'): | |
| if GetDepend('BSP_USING_I2C0') or GetDepend('BSP_USING_I2C1') or GetDepend('BSP_USING_I2C2') or GetDepend('BSP_USING_I2C3'): | |
| src += ['drv_soft_i2c.c'] | |
| if GetDepend('BSP_USING_HW_I2C'): | |
| if GetDepend('BSP_USING_HW_I2C0') or GetDepend('BSP_USING_HW_I2C1'): | |
| src += ['drv_i2c.c'] |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
yefeng0
changed the title
3 participants
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
为gd32vw553系列芯片增加能适用于rtthread的i2c驱动
你的解决方案是什么 (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