drivers: pwm: rts5912: port pwm driver on Zephyr #86275

Titan-Realtek · 2025-02-25T09:04:14Z

Add PWM driver support for Realtek RTS5912

keith-zephyr · 2025-03-05T19:03:40Z

dts/arm/realtek/ec/rts5912.dtsi

+		pwm0: pwm@4000f000 {
+			compatible = "realtek,rts5912-pwm";
+			clocks = <&sccon RTS5912_SCCON_PERIPH_GRP0 PERIPH_GRP0_PWM0_CLKPWR>;
+			clock-names = "pwm";


Non-blocking comment: you only have one clock specifier, so you don't need to assign a clock-names property. You can use DT_INST_CLOCKS_CELL as a shorthand for DT_INST_CLOCKS_CELL_BY_IDX(inst, 0, cell).

thanks, done.

keith-zephyr · 2025-03-05T19:07:06Z

soc/realtek/ec/rts5912/reg/reg_pwm.h

+		struct {
+			uint32_t: 28;
+			volatile uint32_t CLKSRC: 1;
+			volatile uint32_t INVT: 1;
+			volatile uint32_t RST: 1;
+			volatile uint32_t EN: 1;
+		} CTRL_b;


You have also defined macros to access these bits below. You should pick one style here - bit fields or macros.

For single bits, I find C macros for the bit names easier to read.

thanks, done.

keith-zephyr · 2025-03-05T19:08:57Z

soc/realtek/ec/rts5912/reg/reg_pwm.h

+#define PWM_CTRL_CLKSRC_Pos (28UL)
+#define PWM_CTRL_CLKSRC_Msk BIT(28)
+#define PWM_CTRL_INVT_Pos   (29UL)
+#define PWM_CTRL_INVT_Msk   BIT(29)
+#define PWM_CTRL_RST_Pos    (30UL)
+#define PWM_CTRL_RST_Msk    BIT(30)
+#define PWM_CTRL_EN_Pos     (31UL)
+#define PWM_CTRL_EN_Msk     BIT(31)


If you keep these macros - delete the _Pos macros as they aren't used. You can also drop the _Msk' suffix for single-bit fields.

Suggested change

#define PWM_CTRL_CLKSRC_Pos (28UL)

#define PWM_CTRL_CLKSRC_Msk BIT(28)

#define PWM_CTRL_INVT_Pos (29UL)

#define PWM_CTRL_INVT_Msk BIT(29)

#define PWM_CTRL_RST_Pos (30UL)

#define PWM_CTRL_RST_Msk BIT(30)

#define PWM_CTRL_EN_Pos (31UL)

#define PWM_CTRL_EN_Msk BIT(31)

#define PWM_CTRL_CLKSR BIT(28)

#define PWM_CTRL_INVT BIT(29)

#define PWM_CTRL_RST BIT(30)

#define PWM_CTRL_EN BIT(31)

thanks, done.

keith-zephyr · 2025-03-11T22:21:17Z

drivers/pwm/pwm_realtek_rts5912.c

+	struct rts5912_sccon_subsys sccon;
+
+	int rc = 0;
+#if defined(CONFIG_PINCTRL)


Use #ifdef CONFIG_PINCTRL

thanks, done.

keith-zephyr · 2025-03-11T22:21:58Z

drivers/pwm/pwm_realtek_rts5912.c

+		return rc;
+	}
+#endif
+#if defined(CONFIG_CLOCK_CONTROL)


Suggested change

#if defined(CONFIG_CLOCK_CONTROL)

#ifdef CONFIG_CLOCK_CONTROL

thanks, done.

keith-zephyr · 2025-03-11T22:24:36Z

drivers/pwm/pwm_realtek_rts5912.c

+		.pwm_clk_grp = DT_INST_CLOCKS_CELL(inst, 0, clk_grp),                    \
+		.pwm_clk_idx = DT_INST_CLOCKS_CELL(inst, 0, clk_idx),                    \


Suggested change

.pwm_clk_grp = DT_INST_CLOCKS_CELL(inst, 0, clk_grp), \

.pwm_clk_idx = DT_INST_CLOCKS_CELL(inst, 0, clk_idx), \

.pwm_clk_grp = DT_INST_CLOCKS_CELL(inst, clk_grp), \

.pwm_clk_idx = DT_INST_CLOCKS_CELL(inst, clk_idx), \

Wrong number of arguments passed to DT_INST_CLOCKS_CELL. Does the rts5912 EVB support PWM? Can you add a commit to the PR to enable PWM on the EVB? That will help catch compile errors like this.

thanks, done.

keith-zephyr

You also need to this to the pwm_api test by adding an entry for your board.

keith-zephyr · 2025-03-14T22:06:39Z

soc/realtek/ec/rts5912/reg/reg_pwm.h

+ * @brief PWM Controller (PWM)
+ */
+
+typedef struct {


Don't declare new typedefs. https://www.kernel.org/doc/html/v4.10/process/coding-style.html#typedefs

Use struct pwm or struct pwm_regs.

keith-zephyr · 2025-03-14T22:09:29Z

drivers/pwm/pwm_realtek_rts5912.c

+#define PWM_CYCLE_PER_SEC MHZ(50)
+
+struct pwm_rts5912_config {
+	uint32_t pwm_base;


Use the struct pwm_regs type directly to avoid casting every time you use this. This also needs to be declared volatile to prevent compiler reorderng.

Suggested change

uint32_t pwm_base;

volatile struct pwm_regs *pwm_regs;

keith-zephyr

Please also add this to the tests/drivers/build_all/pwm test. You can add a rts5912_evb_defconfig.overlay file to the board directory and enable one of the "pwm" drivers.

keith-zephyr · 2025-04-14T17:03:36Z

soc/realtek/ec/rts5912/reg/reg_pwm.h

+	uint32_t DUTY;
+	uint32_t DIV;
+	uint32_t CTRL;


Use snake case C code. https://docs.zephyrproject.org/latest/contribute/style/code.html

keith-zephyr · 2025-04-15T18:05:34Z

tests/drivers/build_all/pwm/boards/rts5912_evb_defconfig.overlay

+	};
+};
+
+&pwm0 {


This file should be called rts5912_evb.overlay - drop the the "defconfig" portion of the name.

You also need to add a testcase entry for the RTS5912 so that the rts5912_evb board and PWM driver are built.

You can verify this locally using ./scripts/twister -ivc -T tests/drivers/build_all/pwm/

keith-zephyr · 2025-04-18T16:25:25Z

@anangl please take a look.

JasonLin-RealTek · 2025-04-21T14:31:24Z

drivers/pwm/pwm_realtek_rts5912.c

+#include <zephyr/drivers/pinctrl.h>
+#include <zephyr/logging/log.h>
+#include <zephyr/drivers/clock_control.h>
+#include <zephyr/drivers/clock_control/clock_control_rts5912.h>


Hi @Titan-Realtek ,

please remove unused header.

Thanks!

Add PWM driver support for Realtek RTS5912 Signed-off-by: Titan Chen <[email protected]>

zephyrbot added platform: Realtek EC area: PWM Pulse Width Modulation labels Feb 25, 2025

zephyrbot requested review from anangl and henrikbrixandersen February 25, 2025 09:04

zephyrbot assigned anangl Feb 25, 2025

Titan-Realtek force-pushed the drivers_pwm_rts5912 branch from 3aaba9f to f3cc34c Compare February 26, 2025 08:01

JasonLin-RealTek requested a review from keith-zephyr March 5, 2025 06:52

keith-zephyr reviewed Mar 5, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_pwm_rts5912 branch from f3cc34c to 7d94728 Compare March 6, 2025 05:22

zephyrbot requested a review from JasonLin-RealTek March 6, 2025 05:22

keith-zephyr requested changes Mar 11, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_pwm_rts5912 branch 2 times, most recently from 54ba42a to 5d04790 Compare March 12, 2025 23:55

keith-zephyr requested changes Mar 14, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_pwm_rts5912 branch 2 times, most recently from a63db21 to 8fea7df Compare April 14, 2025 12:04

keith-zephyr reviewed Apr 14, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_pwm_rts5912 branch 2 times, most recently from d2a3734 to d372d12 Compare April 15, 2025 06:55

keith-zephyr requested changes Apr 15, 2025

View reviewed changes

Titan-Realtek force-pushed the drivers_pwm_rts5912 branch 2 times, most recently from 4fc4d5c to 42cfb15 Compare April 16, 2025 01:03

keith-zephyr previously approved these changes Apr 16, 2025

View reviewed changes

JasonLin-RealTek requested changes Apr 21, 2025

View reviewed changes

drivers: pwm: rts5912: port pwm driver on Zephyr

81f731b

Add PWM driver support for Realtek RTS5912 Signed-off-by: Titan Chen <[email protected]>

Titan-Realtek dismissed keith-zephyr’s stale review via 81f731b April 22, 2025 06:03

Titan-Realtek force-pushed the drivers_pwm_rts5912 branch from 42cfb15 to 81f731b Compare April 22, 2025 06:03

anangl approved these changes Apr 22, 2025

View reviewed changes

keith-zephyr approved these changes Apr 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: pwm: rts5912: port pwm driver on Zephyr #86275

drivers: pwm: rts5912: port pwm driver on Zephyr #86275

Titan-Realtek commented Feb 25, 2025

keith-zephyr Mar 5, 2025

Titan-Realtek Mar 6, 2025

keith-zephyr Mar 5, 2025

Titan-Realtek Mar 6, 2025

keith-zephyr Mar 5, 2025

Titan-Realtek Mar 6, 2025

keith-zephyr Mar 11, 2025

Titan-Realtek Mar 11, 2025

keith-zephyr Mar 11, 2025

Titan-Realtek Mar 11, 2025

keith-zephyr Mar 11, 2025

Titan-Realtek Mar 11, 2025

keith-zephyr left a comment

keith-zephyr Mar 14, 2025

Titan-Realtek Apr 14, 2025

keith-zephyr Mar 14, 2025

Titan-Realtek Apr 14, 2025

keith-zephyr left a comment

keith-zephyr Apr 14, 2025

Titan-Realtek Apr 15, 2025

keith-zephyr Apr 15, 2025

Titan-Realtek Apr 15, 2025

keith-zephyr commented Apr 18, 2025

JasonLin-RealTek Apr 21, 2025

Titan-Realtek Apr 22, 2025

	#if defined(CONFIG_CLOCK_CONTROL)
	#ifdef CONFIG_CLOCK_CONTROL

		.pwm_clk_grp = DT_INST_CLOCKS_CELL(inst, 0, clk_grp), \
		.pwm_clk_idx = DT_INST_CLOCKS_CELL(inst, 0, clk_idx), \

+              	};
+              };
+              &pwm0 {

drivers: pwm: rts5912: port pwm driver on Zephyr #86275

Are you sure you want to change the base?

drivers: pwm: rts5912: port pwm driver on Zephyr #86275

Conversation

Titan-Realtek commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-zephyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-zephyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-zephyr commented Apr 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment