driver: adc: add adc driver for rts5912 #86753

dylanHsieh4963 · 2025-03-07T05:59:08Z

Add adc driver for Realtek rts5912.

github-actions · 2025-03-07T05:59:45Z

Hello @dylanHsieh4963, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

keith-zephyr · 2025-03-12T20:30:07Z

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

+#define ADC_CTRL_EN_Pos         (0U)
+#define ADC_CTRL_EN_Msk         BIT(ADC_CTRL_EN_Pos)


The _Msk suffix implies that this field is multiple bits. You can simplify by dropping the _Msk suffix. The _Pos helper macro makes this harder to read so I recommend deleting it. Do this for all the macros define for the ADC.

Suggested change

#define ADC_CTRL_EN_Pos (0U)

#define ADC_CTRL_EN_Msk BIT(ADC_CTRL_EN_Pos)

#define ADC_CTRL_EN BIT(0)

keith-zephyr · 2025-03-12T20:34:25Z

drivers/adc/adc_realtek_rts5912.c

+	ADC_CONTEXT_INIT_SYNC(adc_rts5912_dev_data_0, ctx),
+};
+
+DEVICE_DT_INST_DEFINE(0, adc_rts5912_init, NULL, &adc_rts5912_dev_data_0, &adc_rts5912_dev_cfg_0,


Device drivers should generally support multiple instances. If the driver is limited to a single instance, it needs a BUILD_ASSERT() to verify only one instance is enabled.

Modify to support multiple instances, thanks

keith-zephyr · 2025-03-12T20:36:24Z

drivers/adc/adc_realtek_rts5912.c

+static struct adc_rts5912_config adc_rts5912_dev_cfg_0 = {.regs = (ADC_Type *)(DT_INST_REG_ADDR(0)),
+							  .pcfg = PINCTRL_DT_INST_DEV_CONFIG_GET(0),
+							  DEV_CONFIG_CLK_DEV_INIT(0)};


The doesn't match the coding-style use for braces. Please run clang-format on the file.

keith-zephyr · 2025-03-12T20:37:51Z

dts/arm/realtek/ec/rts5912.dtsi

+			compatible = "realtek,rts5912-adc";
+			reg = <0x4000fe00 0x38>;
+			clocks = <&sccon RTS5912_SCCON_ADC ADC0_CLKPWR>;
+			clock-names = "adc";


If there is only one clock, you can delete the clock-names property,

keith-zephyr · 2025-03-12T20:39:24Z

drivers/adc/adc_realtek_rts5912.c

+#define DEV_CONFIG_CLK_DEV_INIT(n)                                                                 \
+	.clk_dev = DEVICE_DT_GET(DT_INST_CLOCKS_CTLR(n)),                                          \
+	.sccon_cfg = {                                                                             \
+		.clk_grp = DT_INST_CLOCKS_CELL_BY_NAME(n, adc, clk_grp),                           \


You can use DT_INST_CLOCKS_CELL() here instead. It's not needed to use the clocks-name property when there is only one clock.

keith-zephyr · 2025-03-12T20:39:44Z

drivers/adc/adc_realtek_rts5912.c

+		return ret;
+	}
+
+#if defined(CONFIG_CLOCK_CONTROL)


Suggested change

#if defined(CONFIG_CLOCK_CONTROL)

#ifdef CONFIG_CLOCK_CONTROL

keith-zephyr · 2025-03-12T20:53:09Z

You also need to add the ADC driver to one of the board files so the driver gets compiled by the tests.

keith-zephyr · 2025-03-14T22:30:48Z

drivers/adc/adc_realtek_rts5912.c

+#define RTS5912_ADC_ENABLE_TIMEOUT  100
+
+struct adc_rts5912_config {
+	ADC_Type *regs;


Your register definitions need to be volatile.

keith-zephyr · 2025-03-14T22:31:45Z

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

+#ifndef ZEPHYR_SOC_REALTEK_RTS5912_REG_ADC_H
+#define ZEPHYR_SOC_REALTEK_RTS5912_REG_ADC_H
+
+typedef struct {


Don't use typedefs. Use struct adc_regs instead. Also, declare the whole structure as volatile, not just the individual fields.

keith-zephyr · 2025-03-14T22:32:58Z

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

+#define ADC_CTRL_START_Pos      (1U)
+#define ADC_CTRL_START_Msk      BIT(ADC_CTRL_START_Pos)


Simplify all the single-bit definitions.

Suggested change

#define ADC_CTRL_START_Pos (1U)

#define ADC_CTRL_START_Msk BIT(ADC_CTRL_START_Pos)

#define ADC_CTRL_START BIT(1)

boards/realtek/rts5912_evb/rts5912_evb.dts

keith-zephyr · 2025-03-18T16:56:48Z

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

+	uint32_t CTRL;
+	uint32_t CHCTRL;
+	uint32_t STS;
+	uint32_t CHDATA[12];
+	uint32_t COEFFA;
+	uint32_t COEFFB;


Use snake case for code and variables, so all these should be lowercase.
https://docs.zephyrproject.org/latest/contribute/style/code.html

keith-zephyr · 2025-03-27T15:12:08Z

@anangl and @JasonLin-RealTek please give this a review.

kartben · 2025-04-11T14:17:21Z

@dylanHsieh4963 this needs to be rebased

JasonLin-RealTek · 2025-04-17T08:11:32Z

Hi @kartben , we rebase the commit in here #88751, thanks!

keith-zephyr · 2025-04-17T17:54:58Z

Hi @kartben , we rebase the commit in here #88751, thanks!

Creating a new PR loses the comment history. Can you rebase this PR instead?

JasonLin-RealTek · 2025-04-18T02:20:21Z

Hi @keith-zephyr,
Here is the same situation with #87536
I will borrow his computer to rebase the commit as he back.

Add adc driver for Realtek rts5912. Signed-off-by: Dylan Hsieh <[email protected]>

zephyrbot added platform: Realtek EC area: ADC Analog-to-Digital Converter (ADC) labels Mar 7, 2025

zephyrbot requested review from anangl and JasonLin-RealTek March 7, 2025 05:59

zephyrbot assigned anangl Mar 7, 2025

JasonLin-RealTek requested a review from keith-zephyr March 7, 2025 07:27

keith-zephyr reviewed Mar 12, 2025

View reviewed changes

dylanHsieh4963 force-pushed the rts5912_enable_adc branch 3 times, most recently from d8fbca5 to 94aff50 Compare March 14, 2025 07:51

keith-zephyr requested changes Mar 14, 2025

View reviewed changes

dylanHsieh4963 force-pushed the rts5912_enable_adc branch 2 times, most recently from 5ef1685 to 035a300 Compare March 18, 2025 09:12

keith-zephyr requested changes Mar 18, 2025

View reviewed changes

dylanHsieh4963 force-pushed the rts5912_enable_adc branch 3 times, most recently from d62da9d to 28d2933 Compare March 20, 2025 09:01

keith-zephyr previously approved these changes Mar 20, 2025

View reviewed changes

keith-zephyr requested a review from tristan-google March 27, 2025 15:12

anangl previously approved these changes Mar 27, 2025

View reviewed changes

JasonLin-RealTek previously approved these changes Mar 28, 2025

View reviewed changes

driver: adc: add adc driver for rts5912

75ec1bf

Add adc driver for Realtek rts5912. Signed-off-by: Dylan Hsieh <[email protected]>

kartben dismissed JasonLin-RealTek’s stale review via 75ec1bf April 18, 2025 06:52

kartben dismissed stale reviews from anangl and keith-zephyr via 75ec1bf April 18, 2025 06:52

kartben force-pushed the rts5912_enable_adc branch from 28d2933 to 75ec1bf Compare April 18, 2025 06:52

keith-zephyr approved these changes Apr 18, 2025

View reviewed changes

JasonLin-RealTek self-requested a review April 21, 2025 03:56

JasonLin-RealTek approved these changes Apr 21, 2025

View reviewed changes

anangl approved these changes Apr 22, 2025

View reviewed changes

kartben merged commit f3bc550 into zephyrproject-rtos:main Apr 22, 2025
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver: adc: add adc driver for rts5912 #86753

driver: adc: add adc driver for rts5912 #86753

dylanHsieh4963 commented Mar 7, 2025

github-actions bot commented Mar 7, 2025

keith-zephyr Mar 12, 2025

dylanHsieh4963 Mar 14, 2025

keith-zephyr Mar 12, 2025

dylanHsieh4963 Mar 14, 2025

keith-zephyr Mar 12, 2025

dylanHsieh4963 Mar 14, 2025

keith-zephyr Mar 12, 2025

dylanHsieh4963 Mar 14, 2025

keith-zephyr Mar 12, 2025

dylanHsieh4963 Mar 14, 2025

keith-zephyr Mar 12, 2025

dylanHsieh4963 Mar 14, 2025

keith-zephyr commented Mar 12, 2025

keith-zephyr Mar 14, 2025

dylanHsieh4963 Mar 18, 2025

keith-zephyr Mar 14, 2025

dylanHsieh4963 Mar 18, 2025

keith-zephyr Mar 14, 2025

dylanHsieh4963 Mar 18, 2025

keith-zephyr Mar 18, 2025

dylanHsieh4963 Mar 20, 2025

keith-zephyr commented Mar 27, 2025

kartben commented Apr 11, 2025

JasonLin-RealTek commented Apr 17, 2025

keith-zephyr commented Apr 17, 2025

JasonLin-RealTek commented Apr 18, 2025

		#define ADC_CTRL_EN_Pos (0U)
		#define ADC_CTRL_EN_Msk BIT(ADC_CTRL_EN_Pos)

	#define ADC_CTRL_EN_Pos (0U)
	#define ADC_CTRL_EN_Msk BIT(ADC_CTRL_EN_Pos)
	#define ADC_CTRL_EN BIT(0)

	#if defined(CONFIG_CLOCK_CONTROL)
	#ifdef CONFIG_CLOCK_CONTROL

		#define ADC_CTRL_START_Pos (1U)
		#define ADC_CTRL_START_Msk BIT(ADC_CTRL_START_Pos)

	#define ADC_CTRL_START_Pos (1U)
	#define ADC_CTRL_START_Msk BIT(ADC_CTRL_START_Pos)
	#define ADC_CTRL_START BIT(1)

driver: adc: add adc driver for rts5912 #86753

driver: adc: add adc driver for rts5912 #86753

Conversation

dylanHsieh4963 commented Mar 7, 2025

github-actions bot commented Mar 7, 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 commented Mar 12, 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

keith-zephyr commented Mar 27, 2025

kartben commented Apr 11, 2025

JasonLin-RealTek commented Apr 17, 2025

keith-zephyr commented Apr 17, 2025

JasonLin-RealTek commented Apr 18, 2025