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

@@ -5,6 +5,11 @@ menu "Drivers Configuration"
        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

@@ -314,12 +317,12 @@ CONFIG_CPUTIME_TIMER_FREQ=25000000
 # CONFIG_RT_USING_I2C is not set
 # 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_DAC is not set
 CONFIG_RT_USING_NULL=y
 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>

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

		}

		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

Are you sure you want to change the base?

bsp: k230: add support for PWM driver #10536

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!

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