bsp: k230: add support for PWM driver #10536

DannyRay019 · 2025-07-24T16:53:50Z

Added a PWM driver and a test file test_pwm.c.
The test uses PWM to control the LED brightness,
to check if the driver works correctly.

Signed-off-by: XU HU 1337858472@qq.com

github-actions · 2025-07-25T01:30:28Z

📌 Code Review Assignment

🏷️ Tag: bsp_k230

Reviewers: unicornx

Changed Files (Click to expand)

bsp/k230/.config
bsp/k230/board/Kconfig
bsp/k230/drivers/interdrv/pwm/SConscript
bsp/k230/drivers/interdrv/pwm/drv_pwm.c
bsp/k230/drivers/interdrv/pwm/drv_pwm.h
bsp/k230/drivers/utest/SConscript
bsp/k230/drivers/utest/test_pwm.c
bsp/k230/rtconfig.h

📊 Current Review Status (Last Updated: 2025-07-27 01:32 CST)

⌛ unicornx Pending Review

📝 Review Instructions

维护者可以通过单击此处来刷新审查状态: 🔄 刷新状态
Maintainers can refresh the review status by clicking here: 🔄 Refresh Status
确认审核通过后评论 LGTM/lgtm
Comment LGTM/lgtm after confirming approval
PR合并前需至少一位维护者确认
PR must be confirmed by at least one maintainer before merging

ℹ️ 刷新CI状态操作需要具备仓库写入权限。
ℹ️ Refresh CI status operation requires repository Write permission.

unicornx

大的修改意见已经在代码中提出：
但我发现有些上次内部 review 的小问题你似乎没有改正。
github 上 review comments 比较多的时候会折叠，见下图。需要展开看。

unicornx · 2025-07-25T01:23:51Z

bsp/k230/.config

你这里 .config 的变化有点多啊。我基于 d23006e，应该也是你提的 pr 所基于的 master 吧，执行一下命令：

cd bsp/k230 scons -c scons --menuconfig # 什么也不做，只是保存后退出

得到的 .config 和原先的 .config 的 diff 见附件：
diff_config.txt

你是不是把你本地的一些配置修改全部提交上来了？请清理一下。原则上应该只有我这里看到的这些 diff（这主要是 master 升级后其它 kconfig 修改导致的 default 值的变化）加上你这次 pwm 相关的配置变化才对。

unicornx · 2025-07-25T01:27:33Z

bsp/k230/board/Kconfig

        select RT_USING_ADC
        default n

+    config BSP_USING_PWM


K230 应该有两个 PWM，我们在配置上应该支持独立配置。上次讨论时我发的这段消息你再理解一下，还是说有什么其他问题原因？

我下面有详细解释，你再理解一下。

unicornx · 2025-07-25T01:32:24Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+
+static struct rt_pwm_ops drv_ops =
+{
+    .control=kd_pwm_control


等号两边加空格隔开。原则是 c 编码时关键字要用空格凸显出来。

unicornx · 2025-07-25T01:33:31Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+#include "drv_pwm.h"
+#include <sys/ioctl.h>
+static struct rt_device_pwm kd_pwm;
+kd_pwm_t *reg_pwm;


不需要定义为全局变量

unicornx · 2025-07-25T01:36:00Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+#include "sysctl_clk.h"
+#include "drv_pwm.h"
+#include <sys/ioctl.h>
+static struct rt_device_pwm kd_pwm;


k230 有两个 pwm，但这里为什么只实现了一个?
可以参考 bsp/cvitek/drivers/drv_pwm.c 的实现做。

unicornx · 2025-07-25T01:49:31Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+
+static int check_channel(int channel)
+{
+    if (channel < 0 || channel > 5)


6 个通道的概念都是错误的，canaan 原来的代码做出来的效果是一个 PWM 上带 6 个 channel。但是我们要做的效果是两个 PWM，每个 3 个通道。

canaan 的说法也说明了这个问题。

他们也说了他们linux 的驱动也是按照两个 PWM 设备写的：

TRM 上也有相关描述，现在来看 TRM 上的描述只是针对一个 PWM 设备。

所以我觉得可以这么做：
我们严格按照 TRM 上的做，两个 PWM，每个 4 个通道。

寄存器上体现的也是有 4 个，但是我理解是说 0 号 channel 没有用。可用的是 1/2/3，按照 canaan 的说法：

第一个 PWM0 的基地址是 0x9140_A000，第二个 PWM1 的基地址是 0x9140_A000 + 0x40
每个 PWM 都对应一组寄存器组：

对于第一个通道我们做成 unavailable，如果api 传入 channelno为 0 就返回错误。

CLAassistant · 2025-07-25T12:07:11Z

All committers have signed the CLA.

unicornx · 2025-07-25T21:38:05Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

你需要签一下 CLA

unicornx

第二次 review，请继续修改。

unicornx · 2025-07-25T21:39:11Z

bsp/k230/.config

 # CONFIG_RT_USING_PHY is not set
 # CONFIG_RT_USING_PHY_V2 is not set
-# CONFIG_RT_USING_ADC is not set
+CONFIG_RT_USING_ADC=y


为什么会 enable ADC？

unicornx · 2025-07-25T21:49:22Z

bsp/k230/.config

 CONFIG_RT_USING_ZERO=y
 CONFIG_RT_USING_RANDOM=y
-# CONFIG_RT_USING_PWM is not set
+CONFIG_RT_USING_PWM=y


我感觉你这次提交上来的 .config 文件好像还是多了很多。
附件是我 checkout 到 master 后运行 scons --menuconfig，不做修改直接保存退出后的 .config 和 rtconfig.h, 你对比一下。你那边做出来的应该只会比附件多一些 bsp 的 pwm 的东西，而且 CONFIG_RT_USING_PWM 默认也是 disable 的。
config.txt
rtconfig.h.txt
文件名稍微改了一下，因为 github 贴附件对文件名有限制。

unicornx · 2025-07-25T21:55:26Z

bsp/k230/board/Kconfig

+        bool "Enable PWM"
+        select RT_USING_PWM
+        default n
+


你好像没有理解我说的配置里面做两个 PWM 的意思，摘录 bsp/k230/board/Kconfig 里 watchdog 的写法你再参考一下，参考改成 PWM 的样子。因为我们有两个 PWM 设备，允许用户通过配置开一个或者关一个。默认可以都关掉。做的好一点可以根据我们支持的 01studio 的板子的支持情况，譬如板子有默认开哪个就开哪个。这个目前我们做得比较粗糙后面可以改进。但至少要支持多个 PWM 的可配置。

menuconfig BSP_USING_WDT bool "Enable Watchdog Timer" select RT_USING_WDT default n if BSP_USING_WDT config BSP_USING_WDT0 bool "Enable WDT0" default n config BSP_USING_WDT1 bool "Enable WDT1" default n endif

unicornx · 2025-07-25T21:58:31Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.h

+} pwm_param_t;
+
+#endif
+


多余的文件尾部空行删掉

unicornx · 2025-07-25T22:02:41Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+int rt_hw_pwm_init(void)
+{
+    kd_pwm_t *reg_pwm0,*reg_pwm1;
+    static struct rt_device_pwm kd_pwm0;


建议将 pwm 设备封装一下，参考 bsp/k230/drivers/interdrv/wdt/drv_wdt.c 中的 struct k230_wdt_dev 的写法.

另外如果 Kcofnig 支持了配置，原则上初始化时也应该通过配置的宏来决定我们到底要注册几个设备。配置没有 enable 的设备不注册的。这个我看了一下 watchdog 的驱动做的不好，以后是一个改进点。
比较好的参考可以看 bsp/k230/drivers/interdrv/sdio/drv_sdhci.c 的 kd_sdhci_init

unicornx · 2025-07-25T22:11:26Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+#define PWM_SCALE_MAX_BITS   15   
+
+#define PWM0_BASE_ADDR   0x9140A000
+#define PWM1_BASE_ADDR   0x9140A040 


用 bsp/k230/board/board.h 里定义的 PWM_BASE_ADDR 来定义你的宏，不要重复写立即数。

unicornx · 2025-07-25T22:13:42Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+
+    pwmscale = reg->pwmcfg & 0xf;
+    pwm_pclock >>= pwmscale;
+    period = reg->pwmcmp0;


这里使用 pwmcmp0 是什么意思？第 0 号 channel 不是我们不用的么，是 canaan 说的作为基准的意思？

cannan那边说的是有0，1，2，3，四个通道，但是第0个通道用不了，只能用1，2，3通道。但是第1，2，3通道在驱动里面的的channel值分别是0，1，2。不知道那个禁用的第0个通道在驱动中代表的数值是几，这个我再向cannan问清楚吧

unicornx · 2025-07-25T22:14:34Z

bsp/k230/drivers/utest/test_pwm.c

+#define PWM_DEV_NAME        "pwm1"    /* PWM设备名称 */   
+#define PWM_DEV_CHANNEL       1      /* PWM通道 */
+#define TEST_GPIO_LED        52
+


多余的空行

unicornx · 2025-07-25T22:15:00Z

bsp/k230/drivers/utest/test_pwm.c

+ */
+
+#define PWM_DEV_NAME        "pwm1"    /* PWM设备名称 */   
+#define PWM_DEV_CHANNEL       1      /* PWM通道 */


代码没有对齐，包括注释。

unicornx · 2025-07-25T22:15:15Z

bsp/k230/drivers/utest/test_pwm.c

+}
+
+UTEST_TC_EXPORT(pwm_testcase, "pwm", utest_tc_init, utest_tc_cleanup, 10);
+


行尾空行

Added a PWM driver and a test file test_pwm.c. The test uses PWM to control the LED brightness, to check if the driver works correctly. Signed-off-by: XU HU <1337858472@qq.com>

DannyRay019 · 2025-07-27T01:38:43Z

关于.config和rtconfig.h文件多余的部分，我看了一下，都是些被注释了的选项，本身也没有选这些配置，我就手动删掉了大部分，关掉了utest，pwm等一些配置，不知道现在是否符合要求了。其他的都按照您的建议来修改了。

unicornx

第三次 review 后的评审意见如下。

此外，我建议你每次正式提交 PR 前用 format 工具规整以下你代码（这个要求以前提过）。因为我发现你的代码中存在很多格式问题，包括行尾空格。你用 git show 在命令行下应该会看到行尾多余空格的红色提醒。

另外你的 commit 的 title 的格式也有问题，你用 git log --oneline 会看到你的 title 比其他正常的缩进了一些。可能和你编辑 commit 的文本时的误操作有关。请改正。

unicornx · 2025-07-28T00:19:07Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.h

+#define KD_PWM_CMD_ENABLE           _IOW('P', 0, int)
+#define KD_PWM_CMD_DISABLE          _IOW('P', 1, int)
+#define KD_PWM_CMD_SET              _IOW('P', 2, int)
+#define KD_PWM_CMD_GET              _IOW('P', 3, int)


这些宏没用的请删掉

unicornx · 2025-07-28T00:21:56Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+ * - channel 0（pwmcmp0）用于设置周期，不对应输出；
+ * - channel 1~3（pwmcmp1~3）用于控制占空比，是实际的输出通道。
+ * 因此，驱动中将这三个通道映射为 channel 0~2。
+ */


这两段注释请合并为一段

第二段注释前面的 * 没有对齐

第二段注释请用英文。原则上代码的注释都是英文。唯一的例外是 k230 的 utest case 中的注释没有用英文，这个先暂时保留，以后再考虑统一改掉。

unicornx · 2025-07-28T00:22:45Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+#define PWM_MAX_CHANNELS     3
+
+#define PWM0_BASE_ADDR       PWM_BASE_ADDR
+#define PWM1_BASE_ADDR       PWM_BASE_ADDR+PWM_REG_OFFSET


“+” 作为关键字两边用空格隔开，不要挤在一起看不清。

unicornx · 2025-07-28T00:33:34Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+    {
+        .name = "pwm0",
+        .base = PWM0_BASE_ADDR,
+        .size = PWM_IO_SIZE,


这是 size 设置有问题。PWM_IO_SIZE （0x1000）是整个 PWM 地址空间的范围，但注意这个范围中包含了两个 PWM 设备。如果全部按照这个范围去映射，则对 pmw0 的映射覆盖了 pwm1 的范围，而对 pwm1 的映射则超出了 PWM 地址空间的下边界。
正确做法是根据每个 PWM 设备实际的寄存器范围进行映射。即 sizeof(kd_pwm_t)。而且这对于 PWM0 和 PWM1 来说是一个不变的常量，不需要作为配置体现，在 init 时 ioremap 时直接赋值即可。

好的，感谢汪老师提醒

unicornx · 2025-07-28T00:37:19Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+
+struct k230_pwm_dev
+{
+    struct rt_device_pwm* device;


没必要定义成指针，你参考 struct k230_wdt_dev 的定义。这样也就不要额外再定义 kd_pwm0/kd_pwm1 这些东西了。

unicornx · 2025-07-28T00:56:32Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+    rt_uint32_t channel = 0;
+    int ret;
+
+    kd_pwm_t *reg = (kd_pwm_t *)(device->parent.user_data);


不需要这么获取 kd_pwm_t, 具体参考 bsp/k230/drivers/interdrv/wdt/drv_wdt.c 中的写法。

我整体看了一下，我理解你的做法也可以，但不是最好的做法，因为本质上我们不推荐直接访问 rt_device_pwm 的内部成员，即 device->parent.user_data 这种，其实如果只是需要获取寄存器地址的话，更好的写法你再读一下 bsp/k230/drivers/interdrv/wdt/drv_wdt.c 中的写法，具体思路是：

定义 rt_watchdog_device 的子类 k230_wdt_dev（rt_watchdog_device 定义为第一个成员，表示基类），定义成员 base，以及其他属性不表

定义全局变量 wdt_devices，将 base 成员赋值为物理地址，其他属性赋值不表

rt_hw_wdt_init 中将 base 重新映射为虚拟地址（映射虚拟地址是 RT-Smart 的需求， k230 目前只考虑支持 smart）。rt_hw_watchdog_register 注册，data 部分如果没有特殊用途，可以直接 NULL

内核回调函数里，譬如 k230_wdt_init，通过 rt_container_of 方式从 rt_watchdog_device 得到 k230_wdt_dev，在我们的驱动中只需看到 k230_wdt_dev 已经足够。

unicornx · 2025-07-28T01:01:24Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+        .base = PWM1_BASE_ADDR,
+        .size = PWM_IO_SIZE,
+    }
+#endif


如果用户选择使能 PWM 但没有 enable 任何一个 PWM 设备，我们应该报错，可以在编译阶段就报错，具体做法参考 bsp/k230/drivers/interdrv/wdt/drv_wdt.c 中定义 wdt_devices 的做法。

好的，我参考一下，这个是一个bug

unicornx · 2025-07-28T01:02:01Z

bsp/k230/drivers/interdrv/pwm/drv_pwm.c

+    dev0->regs = (kd_pwm_t *)rt_ioremap((void *)(dev0->base), dev0->size);
+    dev0->device = &kd_pwm0;
+    dev0->device->ops = &drv_ops;
+    rt_device_pwm_register(dev0->device, dev0->name, &drv_ops, (void*)dev0->regs);


调用 API 如果有返回值为何不判断？

抱歉，是我的疏忽，我改一下

unicornx · 2025-07-28T01:03:21Z

bsp/k230/drivers/utest/test_pwm.c

+ *
+ */
+
+#define PWM_DEV_NAME        "pwm1"    /* PWM设备名称 */   


pwm0 还有机会测试一下？不用写在测试用例里，只是内部测试一下即可。

unicornx · 2025-07-28T01:26:41Z

bsp/k230/.config

 CONFIG_BSP_ROOTFS_TYPE_ELMFAT=y
 # CONFIG_BSP_ROOTFS_TYPE_CROMFS is not set
+# CONFIG_BSP_RISCV_FPU_SOFT is not set
+CONFIG_BSP_RISCV_FPU_D=y


你说的 ”我就手动删掉了大部分“ 这种做法不是合法的做法。我建议你参考以下操作步骤：

假设你的开发分支（pr 的分支）是基于 master 的 commit 为 A 拉出来做的。

checkout 到 master 的 A，将目前主线上的 bsp/k230/.config 文件先备份出来

checkout 到你提交 pr 的分支，假设 commit 对应的是 B，将上一步备份的 .config 文件复制过来替换你的修改

重新 scons --menuconfig，会生成一份新的 .config 和 rtconfig.h。将这个修改 add/commit, 假设生成的 commit 是 C

将 B 和 C 压缩成一个 D，比较 A 和 D 就是你实际应该提交的针对 .config 和 rtconfig.h 的修改。

好的，我尝试一下

unicornx · 2025-07-29T00:23:18Z

新开 #10556 继续这个工作。

github-actions bot added the BSP label Jul 24, 2025

unicornx self-requested a review July 25, 2025 00:44

unicornx suggested changes Jul 25, 2025

View reviewed changes

DannyRay019 force-pushed the master branch from 35a8cfd to 82d14fb Compare July 25, 2025 12:07

unicornx suggested changes Jul 25, 2025

View reviewed changes

bsp: k230: add support for PWM driver

2dce87d

Added a PWM driver and a test file test_pwm.c. The test uses PWM to control the LED brightness, to check if the driver works correctly. Signed-off-by: XU HU <1337858472@qq.com>

DannyRay019 force-pushed the master branch from 82d14fb to 2dce87d Compare July 26, 2025 17:32

DannyRay019 requested a review from unicornx July 27, 2025 01:35

unicornx suggested changes Jul 28, 2025

View reviewed changes

DannyRay019 closed this Jul 28, 2025

unicornx mentioned this pull request Jul 29, 2025

bsp: k230: add support for PWM driver #10556

Merged

		}

		UTEST_TC_EXPORT(pwm_testcase, "pwm", utest_tc_init, utest_tc_cleanup, 10);

		} pwm_param_t;

		#endif

bsp: k230: add support for PWM driver #10536

bsp: k230: add support for PWM driver #10536

Uh oh!

Conversation

DannyRay019 commented Jul 24, 2025

Uh oh!

github-actions bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌 Code Review Assignment

🏷️ Tag: bsp_k230

📊 Current Review Status (Last Updated: 2025-07-27 01:32 CST)

📝 Review Instructions

Uh oh!

unicornx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unicornx Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unicornx commented Jul 25, 2025

Uh oh!

unicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DannyRay019 commented Jul 27, 2025

Uh oh!

unicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unicornx Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 25, 2025 •

edited

Loading

unicornx left a comment •

edited

Loading

unicornx Jul 25, 2025 •

edited

Loading

CLAassistant commented Jul 25, 2025 •

edited

Loading

unicornx Jul 28, 2025 •

edited

Loading