Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drivers: comparator: silabs_acmp implementation #83980

Conversation

silabs-chgalant
Copy link
Contributor

@silabs-chgalant silabs-chgalant commented Jan 14, 2025

This pull request does the following:

  • Enables the hal_silabs ACMP module to be compiled for series 2.
  • Adds a generated bindings header to aid with acmp input selection
  • Defines a compatible binding which can be used for series 2 parts that have an acmp peripheral
  • Instantiates the xg2x parts with an acmp peripheral with an acmp node in their respective part dts
  • Implements the comparator driver compatible with silabs,acmp
  • Adds support for the comparator driver in the relevant series 2 development kits and radio boards.
  • Extends the comparator gpio_loopback tests for xg24_dk2601b and xg29_rb4412a boards.
  • Extends comparator build testing for all current series 2 boards that support the comparator.

The input-positive and input-negative properties of the silabs,acmp compatible binding are part specific, therefore binding headers are generated per part (as per @asmellby's feedback). However, since most of the input values are common between parts, and there is no overlapping of input values, it was decided (as per @jerome-pouiller's feedback) that we compile the total set of possible ACMP inputs into a single bindings header for the silabs_acmp. It would therefore be up to the user to reference their part's design book when selecting the input-positive and input-negative values for their application.

Copy link

Hello @silabs-chgalant, 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. 😊

drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
dts/bindings/comparator/silabs,acmp.yaml Outdated Show resolved Hide resolved
dts/bindings/comparator/silabs,acmp.yaml Outdated Show resolved Hide resolved
dts/bindings/comparator/silabs,acmp.yaml Outdated Show resolved Hide resolved
dts/bindings/comparator/silabs,acmp.yaml Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
modules/hal_silabs/gecko/CMakeLists.txt Outdated Show resolved Hide resolved
drivers/comparator/Kconfig.silabs_acmp Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Outdated Show resolved Hide resolved
drivers/comparator/CMakeLists.txt Outdated Show resolved Hide resolved
@silabs-chgalant silabs-chgalant force-pushed the drivers/comparator/gecko-acmp-implementation branch from 0c45311 to 3ae5eec Compare January 16, 2025 18:29
silabs-chgalant

This comment was marked as outdated.

@silabs-chgalant silabs-chgalant force-pushed the drivers/comparator/gecko-acmp-implementation branch from 3ae5eec to 280e2c1 Compare January 16, 2025 20:47
@jerome-pouiller jerome-pouiller self-requested a review January 17, 2025 08:27
include/zephyr/dt-bindings/acmp/silabs/xg21-acmp.h Outdated Show resolved Hide resolved
include/zephyr/dt-bindings/acmp/silabs/xg21-acmp.h Outdated Show resolved Hide resolved
dts/arm/silabs/efr32bg27.dtsi Outdated Show resolved Hide resolved
*
* SPDX-License-Identifier: Apache-2.0
*
* This file was generated by the script gen_acmp.py in the hal_silabs module.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you are going to send a PR to hal_silabs to publish gen_acmp.py? Ideally, this current PR should update the reference in west.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will open a PR in hal_silabs for that script shortly, then update the reference in west.yml once that gets merged.

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 opened the PR for hal_silabs here zephyrproject-rtos/hal_silabs#80 (cc @asmellby)

include/zephyr/dt-bindings/acmp/silabs/xg21-acmp.h Outdated Show resolved Hide resolved
include/zephyr/dt-bindings/acmp/silabs/xg21-acmp.h Outdated Show resolved Hide resolved
drivers/comparator/comparator_silabs_acmp.c Show resolved Hide resolved
@silabs-chgalant silabs-chgalant force-pushed the drivers/comparator/gecko-acmp-implementation branch 3 times, most recently from db4001c to 966b99e Compare January 20, 2025 15:39
@jerome-pouiller
Copy link
Contributor

I believe this PR is no more a draft.

@bjarki-andreasen
Copy link
Collaborator

bjarki-andreasen commented Jan 20, 2025

Looks good :) could you extend the test suite https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/comparator/gpio_loopback with the new comparator and test it out locally as well? and extend https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/build_all/comparator? That allows CI to build various configurations for various boards too :)

@silabs-chgalant
Copy link
Contributor Author

Looks good :) could you extend the test suite https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/comparator/gpio_loopback with the new comparator and test it out locally as well? and extend https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/build_all/comparator? That allows CI to build various configurations for various boards too :)

Thank you for the review!

I'm in the process of extending these tests, I will let you know when this work has been pushed.

@silabs-chgalant silabs-chgalant force-pushed the drivers/comparator/gecko-acmp-implementation branch from b01d533 to 676ed91 Compare January 31, 2025 16:22
@silabs-chgalant
Copy link
Contributor Author

@Martinhoff-maker @jerome-pouiller @bjarki-andreasen, I pushed a fix for the copyright declarations.

@@ -0,0 +1,12 @@
# Copyright (c) 2025 Silicon Labs Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, we tend to use "Silicon Laboratories Inc." 🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops... okay I'll try again 😅

Defines bindings that are compatible with silabs acmp.
`input-positive` and `input-negative` are required
properties to be configured by an application.
It is recommended to use the bindings generated in
`include/zephyr/dt-bindings/comparator/silabs_acmp.h`
and reference your part's design book when configuring
values for these properties.

Signed-off-by: Christian Galante <[email protected]>
Defines an acmp node for xg21, xg23, xg24, xg27 and xg29
parts, which are all compatible with the silabs,acmp
binding.

Signed-off-by: Christian Galante <[email protected]>
This implements the comparator driver for silabs acmp peripherals
using the silabs,acmp compatible binding.

Signed-off-by: Christian Galante <[email protected]>
The xg24_dk2601b, xg24_ek2703a, xg27_dk2602a, xg23_rb4210a,
xg24_rb4187c and xg29_rb4412a board yaml files were updated
to support the comparator driver. Documentation hardware
support was updated for each of these boards as well to
mention support for the ACMP hardware block and comparator
driver.

Signed-off-by: Christian Galante <[email protected]>
…ards

Add board overlays to execute the comparator gpio_loopback testsuite on
xg24_dk2601b and xg29_rb4412a. The testsuite was executed and was seen
to pass on both boards locally with twister.

Signed-off-by: Christian Galante <[email protected]>
…tions

Add build test configurations for the silabs_acmp comparator. Enable this
build testing on all series 2 silabs boards that currently support the
comparator driver.

Signed-off-by: Christian Galante <[email protected]>
@silabs-chgalant silabs-chgalant force-pushed the drivers/comparator/gecko-acmp-implementation branch from 2cdc1cf to c54a8db Compare January 31, 2025 17:48
@jhedberg
Copy link
Member

jhedberg commented Feb 3, 2025

@bjarki-andreasen looks like this should be ready to go in - just missing your review/approval. Thanks!

Copy link
Collaborator

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

Nice work!

@kartben kartben merged commit 2fdfa97 into zephyrproject-rtos:main Feb 3, 2025
30 checks passed
Copy link

github-actions bot commented Feb 3, 2025

Hi @silabs-chgalant!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants