Skip to content

Conversation

KozhinovAlexander
Copy link
Contributor

Add STM32 Comparator support for h7 and g4 SoC series

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

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dts: bindings: comparator: add stm32 description

Add STM32 comparator DT bindings description.

@KozhinovAlexander
Copy link
Contributor Author

@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.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside remaining comments, this looks consistent to me.

@KozhinovAlexander
Copy link
Contributor Author

@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.

Comment on lines +54 to +57
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwango Nice. Thnx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I've tested it like this:

  1. 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.

Copy link
Contributor

@etienne-lms etienne-lms Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Contributor

@etienne-lms etienne-lms Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Let me step back:

  1. Originally you proposed a boolean property st,miller-effect-hold-enable to enable this anti-Miller-effect feature on stm32g4xx SoCs COMP.
  2. (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.
  3. Therefore I suggested to use an int property, defaulted to 1 for G4 SoCs, allowing overlay to set it to 0.
  4. 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.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Let me step back:

  1. Originally you proposed a boolean property st,miller-effect-hold-enable to enable this anti-Miller-effect feature on stm32g4xx SoCs COMP.
  2. (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.
  3. Therefore I suggested to use an int property, defaulted to 1 for G4 SoCs, allowing overlay to set it to 0.
  4. 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.

\
static struct stm32_comp_data _CONCAT(data, inst); \
\
STM32_COMP_IRQ_HANDLER_DEFINE(inst) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an empty line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JarmouniA
JarmouniA previously approved these changes Oct 6, 2025
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]>
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

Copy link

sonarqubecloud bot commented Oct 6, 2025

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues.

\
static struct stm32_comp_data _CONCAT(data, inst); \
\
STM32_COMP_IRQ_HANDLER_DEFINE(inst) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to add it.

Comment on lines +54 to +57
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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 :)

Copy link
Contributor

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@henrikbrixandersen henrikbrixandersen merged commit 66a0f82 into zephyrproject-rtos:main Oct 7, 2025
26 checks passed
@KozhinovAlexander KozhinovAlexander deleted the stm32_comparator branch October 7, 2025 09:03
@etienne-lms
Copy link
Contributor

Thanks @KozhinovAlexander for the work and patience.

@KozhinovAlexander
Copy link
Contributor Author

KozhinovAlexander commented Oct 7, 2025

Thanks @KozhinovAlexander for the work and patience.

@etienne-lms Thank you for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Boards/SoCs area: Comparator area: Devicetree Bindings area: Tests Issues related to a particular existing or missing test platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants