STM32 Comparator support for h7 and g4 SoC series #96325

KozhinovAlexander · 2025-09-20T21:26:16Z

Add STM32 Comparator support for h7 and g4 SoC series

drivers/comparator/comparator_stm32_comp.c

dts/bindings/comparator/st,stm32-comp.yaml

dts/arm/st/g4/stm32g4.dtsi

drivers/comparator/Kconfig.stm32_comp

drivers/comparator/comparator_stm32_comp.c

etienne-lms

For commit "dts: bindings: comparator", could you refine the commit message?

dts: bindings: comparator: add stm32 description

Add STM32 comparator DT bindings description.

dts/bindings/comparator/st,stm32-comp.yaml

drivers/comparator/Kconfig.stm32_comp

drivers/comparator/comparator_stm32_comp.c

KozhinovAlexander · 2025-09-22T13:08:01Z

@gautierg-st I think you shall assign yourself to this PR, if you're from official ST side. Cause in the end h7 and g4 needed to be run on your hw testing farm and your review will be obligatory. If I am wrong please nominate right ST responsible person.

etienne-lms

Aside remaining comments, this looks consistent to me.

drivers/comparator/Kconfig.stm32_comp

drivers/comparator/comparator_stm32_comp.c

KozhinovAlexander · 2025-09-22T20:55:00Z

@gautierg-st @etienne-lms Hi guys, thank you for your very-very fast review response. I've got all the feedback and was able to adjust some parts in almost real-time, cause I had free from work till lunch time.
I will adjust the rest of the change request next two days. Anyways the code were tested by me on g4 and h7 (see overlays in the test case). I think you can also test it on your hw if you wish.

Copilot

Pull Request Overview

This PR adds STM32 comparator driver support for H7 and G4 series microcontrollers. The implementation provides a complete comparator driver framework including device tree bindings, driver implementation, and test configurations.

Key changes:

Adds STM32 comparator driver with support for H7 and G4 series
Includes device tree bindings with series-specific configurations
Provides test overlays for nucleo_h745zi_q and nucleo_g474re boards

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
drivers/comparator/comparator_stm32_comp.c	Main comparator driver implementation with interrupt handling and power management
dts/bindings/comparator/st,stm32-comp.yaml	Base STM32 comparator device tree bindings
dts/bindings/comparator/st,stm32h7-comp.yaml	H7-specific comparator bindings with power modes and blanking
dts/bindings/comparator/st,stm32g4-comp.yaml	G4-specific comparator bindings with extended features
dts/arm/st/h7/stm32h7.dtsi	H7 device tree with comparator node definitions
dts/arm/st/g4/stm32g4.dtsi	G4 device tree with comparator node definitions
tests/drivers/comparator/gpio_loopback/	Test configurations and board overlays for validation
drivers/comparator/Kconfig*	Build configuration for STM32 comparator driver
drivers/comparator/CMakeLists.txt	Build system integration

Comments suppressed due to low confidence (1)

drivers/comparator/comparator_stm32_comp.c:1

The return value from clock_control_configure is not being checked. The function call on line 240 does not assign its return value to 'ret', but the error check on line 242 still references the previous value of 'ret'.

/*

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

drivers/comparator/comparator_stm32_comp.c

etienne-lms · 2025-10-03T05:38:47Z

drivers/comparator/comparator_stm32_comp.c

+	COND_CODE_1(DT_INST_NODE_HAS_PROP(inst, st_power_mode),			\
+		(CONCAT(LL_COMP_POWERMODE_,					\
+			DT_INST_STRING_TOKEN(inst, st_power_mode))),		\
+			(0))


Indentation suggestion

COND_CODE_1(DT_INST_NODE_HAS_PROP(inst, st_power_mode), \ (CONCAT(LL_COMP_POWERMODE_, \ DT_INST_STRING_TOKEN(inst, st_power_mode))), \ (0))

Could we leave it as it is? There are already style checks successfully running CI. In my EXTI driver PR there were already a long description, why and how other interpretations than in CI are pretty subjective and could have different outcomes.

I agree this is not a blocking issue. Yet, it would make the code more readable for others and from a maintenance perspective.

This is pretty common reason for such request - readability of the code :) But. There is a big interpretation difference. GitHub is know for showing the code with own indentations and thus the code may look different than checked one in the editor. The root folder of zephyr project contains.editorconfig - this file defines also for vscode how to display tabs and spaces. Also any other ide shall show you same code style. Therefore please do not rely just on that, GitHub shows you. I can assure you - you will see different visual representation as soon as you open it in vim or vscode.

Please take a look here: #85508 (comment)
So far I remember, I used to use .editorconfig.

@erwango Nice. Thnx.

Therefore please do not rely just on that, GitHub shows you. I can assure you - you will see different visual representation as soon as you open it in vim or vscode.

I fully agree :-) Personally use vim over various SW projects each with its own style preferences.
I don't know if Github can be tuned to adapt the tab size to the project being sourced.

There are already style checks successfully running CI.

Despite CI Compliance Checks processing reports many issues, it does not address all of them.
In the end, I think what's best is what makes the implementation easier to understand.

@etienne-lms He he. Here we come to the next problem: How shall I adapt the visual appearance of the code, if that, what you see is different than, what I see? I think this could be major point of disagreement. Therefore I prefer to leave this job to proper CI configuration. No CI complaints - the code is good with allowed freedom interpretation range of the creator.

@etienne-lms BTW: I could highly recommend https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig if you use vscode with zephyr and 8 in #96325 (comment)

Well, leave as is if you prefer. That said, as-is tend to show CONCAT() operates on 3 arguments, while it's rather COND_CODE_1() that does.
Not a blocking issue.

etienne-lms · 2025-10-03T05:41:38Z

drivers/comparator/comparator_stm32_comp.c

+	const uint32_t input_minus;
+	const uint32_t hysteresis;
+	const uint32_t invert_output;
+	const uint32_t blank_sel;


Nitpicking: I don't think it makes sense to have const for non-pointers in a struct definition. It's rather the instance of this struct that is const (which is already the case).

Thnx. I will change.

etienne-lms · 2025-10-03T05:42:53Z

drivers/comparator/comparator_stm32_comp.c

+	if (LL_COMP_IsLocked(comp)) {
+		LOG_ERR("%s is locked", dev->name);
+		return -EACCES;
+


Could you remove this extra empty line?

etienne-lms · 2025-10-03T05:55:57Z

drivers/comparator/comparator_stm32_comp.c

+
+#if defined(CONFIG_SOC_SERIES_STM32G4X)
+	/* Undocumented bit, refer to st,miller-effect-hold-disable DT binding */
+	WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable);


Suggested change

WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable);

WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable ? 0 : 1);

Suggested change

WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable);

WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable ? 0 : 1);

This makes no sense. The macro is already doing exactly same check.

Maybe I'm wrong but miller_effect_hold_disable is true/1 when set in which case the bit shall be cleared, while if the property is not set you expect the bit to be set.

Here is a reverse logic. Please try to print csr register value before and after writing, to see if it that, what to expect.

Do you mean with st,miller-effect-hold-disable property not being set (which is G4 default config), comp->CSR bit 1 is set by the above implementation?

No. I've tested it like this:

No st,miller-effect-hold-disable in .overlay is present - gives CSR bit 1 staying 0.

2.st,miller-effect-hold-disable present gives CSR bit 1 being 1.

But I see discussion necessary. Thnx for it. It would be great if you try to test it your own.

Sorry to insist. As stated in your YAML, the property shall be set to:

Disable undocumented hold feature (COMP_CxCSR bit 1).

Implicitly, if the property is not set, we would expect that the bit is set which must be done here since the COMP_CxCSR register is 0 at reset.

I see the logic as:

No st,miller-effect-hold-disable in .overlay is present gives CSR bit 1 is set to 1.

st,miller-effect-hold-disable present gives CSR bit 1 is cleared to 0 (which is the reset value).

Maybe @erwan's comment #96325 (comment) was misunderstood.

Let me step back:

Originally you proposed a boolean property st,miller-effect-hold-enable to enable this anti-Miller-effect feature on stm32g4xx SoCs COMP.

(edited: I've added this step) I proposed it is default enable at SoC level leaving boards overlay to disable it. However disabling a default enable boolean property would mean board overlay should use /delete-property/, not nice.

Therefore I suggested to use an int property, defaulted to 1 for G4 SoCs, allowing overlay to set it to 0.

Ewarn's comment was that since it's a boolean info, it's weird it is not a bool property. So, in order to be able to default enable the feature it and leave overlay to disable the feature, let's invert the config logic and use a boolean property, st,miller-effect-hold-disable:

DT property is default not set to have CSR bit 1 set and Miller effect addressed.

DT overlay files can set the property when the target does not want to compensate this Miller effect (and have CSR bit set to 0)

Our apologies for the possible confusion and mine especially for requesting you to change your P-R back and forth.

Maybe @erwan's comment #96325 (comment) was misunderstood.

Let me step back:

Originally you proposed a boolean property st,miller-effect-hold-enable to enable this anti-Miller-effect feature on stm32g4xx SoCs COMP.

(edited: I've added this step) I proposed it is default enable at SoC level leaving boards overlay to disable it. However disabling a default enable boolean property would mean board overlay should use /delete-property/, not nice.

Therefore I suggested to use an int property, defaulted to 1 for G4 SoCs, allowing overlay to set it to 0.

Ewarn's comment was that since it's a boolean info, it's weird it is not a bool property. So, in order to be able to default enable the feature it and leave overlay to disable the feature, let's invert the config logic and use a boolean property, st,miller-effect-hold-disable:

DT property is default not set to have CSR bit 1 set and Miller effect addressed.

DT overlay files can set the property when the target does not want to compensate this Miller effect (and have CSR bit set to 0)

Our apologies for the possible confusion and mine especially for requesting you to change your P-R back and forth.

Thank you for putting pieces together. Changing PR back-and-forth is fine, if you provide me strong arguments. I see now problem on my side 😄
In that case your suggestion from above makes totally sense. I'e applied it and extended description in .yml. Please take a look.

etienne-lms · 2025-10-03T06:01:18Z

drivers/comparator/comparator_stm32_comp.c

+										\
+	static struct stm32_comp_data _CONCAT(data, inst);			\
+										\
+	STM32_COMP_IRQ_HANDLER_DEFINE(inst)					\


Could you add an empty line below?

I think you forgot to add it.

I've added it after line 309 (see above). Maybe code suggestion would be better next time.

drivers/comparator/comparator_stm32_comp.c

dts/bindings/comparator/st,stm32g4-comp.yaml

dts/bindings/comparator/st,stm32h7-comp.yaml

Add STM32 COMP devices comparator DTS description Signed-off-by: Alexander Kozhinov <[email protected]>

adds comparator DTS entities for stm32h7 and stm32g4 series Signed-off-by: Alexander Kozhinov <[email protected]>

implement the driver for the stm32 comparator peripheral Signed-off-by: Alexander Kozhinov <[email protected]>

add support for nucleo_g474re and nucleo_h745zi_q boards Signed-off-by: Alexander Kozhinov <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

KozhinovAlexander · 2025-10-06T22:23:29Z

@etienne-lms @JarmouniA @erwango It shall be ready for the next review-round. Please take a look.

sonarqubecloud · 2025-10-06T22:37:08Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

etienne-lms

No blocking issues.

etienne-lms · 2025-10-07T07:07:25Z

drivers/comparator/comparator_stm32_comp.c

+										\
+	static struct stm32_comp_data _CONCAT(data, inst);			\
+										\
+	STM32_COMP_IRQ_HANDLER_DEFINE(inst)					\


I think you forgot to add it.

etienne-lms · 2025-10-07T07:10:02Z

drivers/comparator/comparator_stm32_comp.c

+	COND_CODE_1(DT_INST_NODE_HAS_PROP(inst, st_power_mode),			\
+		(CONCAT(LL_COMP_POWERMODE_,					\
+			DT_INST_STRING_TOKEN(inst, st_power_mode))),		\
+			(0))


Well, leave as is if you prefer. That said, as-is tend to show CONCAT() operates on 3 arguments, while it's rather COND_CODE_1() that does.
Not a blocking issue.

KozhinovAlexander · 2025-10-07T07:26:00Z

@bjarki-andreasen Could you please take a look here? I've lost your approval since there were additional change-requestes afterwards. Now ST folks approved and it would be a goot time for you to take a look too :)

bjarki-andreasen

Looks good!

zephyrbot added area: Comparator platform: STM32 ST Micro STM32 labels Sep 20, 2025

zephyrbot requested review from bjarki-andreasen, djiatsaf-st, erwango, etienne-lms, FRASTM, gautierg-st, GeorgeCGV, marwaiehm-st and mathieuchopstm September 20, 2025 21:27

zephyrbot assigned bjarki-andreasen Sep 20, 2025

KozhinovAlexander force-pushed the stm32_comparator branch 5 times, most recently from 9936176 to e918ba0 Compare September 21, 2025 20:33

gautierg-st reviewed Sep 22, 2025

View reviewed changes

drivers/comparator/comparator_stm32_comp.c Outdated Show resolved Hide resolved

etienne-lms reviewed Sep 22, 2025

View reviewed changes

KozhinovAlexander force-pushed the stm32_comparator branch 2 times, most recently from 5b0bd0d to cc05447 Compare September 22, 2025 10:37

KozhinovAlexander requested review from etienne-lms and gautierg-st September 22, 2025 10:37

etienne-lms reviewed Sep 22, 2025

View reviewed changes

gautierg-st reviewed Sep 22, 2025

View reviewed changes

drivers/comparator/comparator_stm32_comp.c Outdated Show resolved Hide resolved

drivers/comparator/comparator_stm32_comp.c Outdated Show resolved Hide resolved

drivers/comparator/comparator_stm32_comp.c Outdated Show resolved Hide resolved

gautierg-st assigned erwango and unassigned bjarki-andreasen Sep 22, 2025

etienne-lms reviewed Sep 22, 2025

View reviewed changes

drivers/comparator/Kconfig.stm32_comp Outdated Show resolved Hide resolved

drivers/comparator/comparator_stm32_comp.c Show resolved Hide resolved

KozhinovAlexander force-pushed the stm32_comparator branch from a6464e1 to 37d46c7 Compare October 2, 2025 21:11

KozhinovAlexander requested review from erwango and Copilot October 2, 2025 21:14

Copilot AI reviewed Oct 3, 2025

View reviewed changes

drivers/comparator/comparator_stm32_comp.c Outdated Show resolved Hide resolved

etienne-lms reviewed Oct 3, 2025

View reviewed changes

drivers/comparator/comparator_stm32_comp.c Outdated Show resolved Hide resolved

JarmouniA reviewed Oct 3, 2025

View reviewed changes

dts/bindings/comparator/st,stm32g4-comp.yaml Outdated Show resolved Hide resolved

JarmouniA reviewed Oct 3, 2025

View reviewed changes

dts/bindings/comparator/st,stm32h7-comp.yaml Outdated Show resolved Hide resolved

erwango requested review from etienne-lms and JarmouniA October 6, 2025 09:25

KozhinovAlexander force-pushed the stm32_comparator branch from 8b566a0 to db18424 Compare October 6, 2025 20:33

JarmouniA previously approved these changes Oct 6, 2025

View reviewed changes

KozhinovAlexander dismissed JarmouniA’s stale review via 1f9e18a October 6, 2025 21:42

KozhinovAlexander force-pushed the stm32_comparator branch 2 times, most recently from 746ea4e to d996ea0 Compare October 6, 2025 21:47

KozhinovAlexander added 4 commits October 7, 2025 00:20

dts: bindings: comparator: add STM32 COMP device driver

5bb80f4

Add STM32 COMP devices comparator DTS description Signed-off-by: Alexander Kozhinov <[email protected]>

dts: arm: st: add comparator to h7 and g4

f5dfb81

adds comparator DTS entities for stm32h7 and stm32g4 series Signed-off-by: Alexander Kozhinov <[email protected]>

drivers: comparator: implement stm32 comparator driver

0dd938c

implement the driver for the stm32 comparator peripheral Signed-off-by: Alexander Kozhinov <[email protected]>

tests: drivers: comparator: stm32 comparator support

71d810b

add support for nucleo_g474re and nucleo_h745zi_q boards Signed-off-by: Alexander Kozhinov <[email protected]>

KozhinovAlexander force-pushed the stm32_comparator branch from d996ea0 to 71d810b Compare October 6, 2025 22:21

KozhinovAlexander requested review from Copilot and JarmouniA October 6, 2025 22:22

Copilot AI reviewed Oct 6, 2025

View reviewed changes

etienne-lms approved these changes Oct 7, 2025

View reviewed changes

JarmouniA approved these changes Oct 7, 2025

View reviewed changes

erwango approved these changes Oct 7, 2025

View reviewed changes

bjarki-andreasen approved these changes Oct 7, 2025

View reviewed changes

	WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable);
	WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable ? 0 : 1);

STM32 Comparator support for h7 and g4 SoC series #96325

Are you sure you want to change the base?

STM32 Comparator support for h7 and g4 SoC series #96325

Conversation

KozhinovAlexander commented Sep 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KozhinovAlexander commented Sep 22, 2025

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KozhinovAlexander commented Sep 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

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!

KozhinovAlexander Oct 3, 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!

KozhinovAlexander Oct 6, 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!

KozhinovAlexander Oct 3, 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!

KozhinovAlexander Oct 3, 2025 •

edited

Loading

KozhinovAlexander Oct 6, 2025 •

edited

Loading

KozhinovAlexander Oct 3, 2025 •

edited

Loading

KozhinovAlexander Oct 3, 2025 •

edited

Loading

etienne-lms Oct 3, 2025 •

edited

Loading

etienne-lms Oct 3, 2025 •

edited

Loading

KozhinovAlexander Oct 6, 2025 •

edited

Loading