drivers: sensors: silabs: add gecko acmp #83531

RomainPelletant · 2025-01-03T12:03:05Z

Update module/hal_silabs to include acmp related files from HAL
Update soc to include acmp related files if ACMP is required
Add ACMP sensor driver
Add ACMP nodes support for EFR32MG24 devices
Add sample for ACMP sensor driver

Fixes #83529

- Enable ACMP hal in hal_silabs module Signed-off-by: Romain Pelletant <[email protected]>

- Enable ACMP HAL module for silabs gecko Signed-off-by: Romain Pelletant <[email protected]>

- Add SiLabs Gecko ACMP sensor driver Signed-off-by: Romain Pelletant <[email protected]>

- Add ACMP peripheral nodes for EFR32MG24 Signed-off-by: Romain Pelletant <[email protected]>

- Add sample for Gecko ACMP sensor driver Signed-off-by: Romain Pelletant <[email protected]>

Martinhoff-maker

Thanks for the contribution @RomainPelletant !
I have 2 general questions:

Is there any reason why you didn't use the comparator device API? It feels that it is a better place for this driver ?
I see that you use em_cmu.h for clock and use function like CMU_ClockEnable(). Normally, on all series 2 boards (like the efr32mg24), we support clock control API which allows to directly use clock_control() functions. CMU_ClockEnable only need to be used in case that you want to use the driver for an old device (series 0 and series 1 boards). As it is also not tested on series 0 and 1, I suggest to keep the code clearer and only use clock_control API. What's your opinion ?

Martinhoff-maker · 2025-01-06T09:12:57Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+	return 0;
+}
+
+static const struct sensor_driver_api gecko_acmp_driver_api = {


It's a fairly new convention but I believe this should be using DEVICE_API().

RomainPelletant · 2025-01-06T10:17:09Z

Hello @Martinhoff-maker, thank you for reviewing and good questions.

I hesitate to use comparator API but the lack of configuration on runtime (set/get attributes for example) for future integration led me to choose the sensor API.
Obviously:

Do we need attribute changes on runtime, even in the future for a comparator?
Do we need counters or custom values integrations?
Maybe the comparator API could be improved to integrate attributes like functions directly?
If you think comparator API is still the way to go: please let me know and I will update this PR

Let's go for clock_control() functions usage :)

jerome-pouiller · 2025-01-13T10:44:07Z

soc/silabs/Kconfig

About commit 5191033, since the previous commit use SOC_GECKO_ACMP, I suggest to merge 5191033 with the previous one.

jerome-pouiller · 2025-01-13T11:04:56Z

include/zephyr/drivers/sensor/gecko_acmp.h

+	SENSOR_CHAN_GECKO_ACMP_RISING_EDGE_COUNTER,
+	/** ACMP falling edge counter. */
+	SENSOR_CHAN_GECKO_ACMP_FALLING_EDGE_COUNTER,
+#endif /* CONFIG_GECKO_ACMP_EDGE_COUNTER */


I suggest to keep definitions of SENSOR_CHAN_GECKO_ACMP_*_EDGE_COUNTER even if CONFIG_GECKO_ACMP_EDGE_COUNTER. Thus, the caller code will simpler (it will be probably able to remove some a #ifdef, or at least to change them is IS_ENABLE()).

BTW, I wonder if it make sense to make all these features optional. This leads to increased maintenance cost. It make sense to make a feature optional if you could get rid of a dependency.

jerome-pouiller · 2025-01-13T11:08:31Z

dts/bindings/sensor/silabs,gecko-acmp.yaml

+      - "FallingEdge"                # Interrupt on falling edge
+      - "RisingEdge"                 # Interrupt on rising edge
+      - "BothEdge"                   # Interrupt on both edges
+    default: "Off"


Device Tree aims to includes required hardware configuration to have a working system. I feel the options above should rather be set during runtime. Maybe some of them should be replaced by sensor_attribute?

Sensor attribute is only available on Sensor API. Can you tell me why you are talking about that while you're considering comparator API?

I have to admit this is subjective thing.

comparator_set_trigger() allows to change the trigger during the runtime (which is the attribute that make me raise the eyebrows).

Documentation of the comparator framework explicitly explain it is designed to watch power supply. In this case, it make sense the DT describe what is the power supply.

Sensor framework is designed to make data acquisition. In this case, it sound more logical the application choose what data it wants to read.

jerome-pouiller · 2025-01-13T12:16:32Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+	static struct gecko_acmp_data gecko_acmp_data_##n = {                                      \
+		.acmp_config = ACMP_INIT_DEFAULT,                                                  \
+	};                                                                                         \
+	static const struct gecko_acmp_config gecko_acmp_config_##n;                               \


You can call GECKO_ACMP_INIT_CONFIG() before SENSOR_DEVICE_DT_INST_DEFINE() and get rid of this line.

jerome-pouiller · 2025-01-13T12:17:19Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+				     &gecko_acmp_config_##n, POST_KERNEL,                          \
+				     CONFIG_SENSOR_INIT_PRIORITY, &gecko_acmp_driver_api);         \
+	GECKO_ACMP_CONFIG_FUNC(n)                                                                  \
+	GECKO_ACMP_INIT_CONFIG(n);


GECKO_ACMP_INIT_CONFIG() seems useless. Instead, you can inline the declaration of gecko_acmp_config_##n.

jerome-pouiller · 2025-01-13T12:37:47Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+	CMU_Clock_TypeDef clock;
+#ifdef CONFIG_GECKO_ACMP_TRIGGER
+	void (*irq_config_func)(const struct device *dev);
+#endif /* CONFIG_GECKO_ACMP_TRIGGER */


Avoid to repeat the condition after #endif if the condition is obvious (because of the Don't Repeat Yourself rule).

jerome-pouiller · 2025-01-13T12:38:41Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+	data->falling_edge_counter = atomic_clear(&data->falling_events);
+#endif /* CONFIG_GECKO_ACMP_EDGE_COUNTER */
+
+	data->cout = (config->base->STATUS & ACMP_STATUS_ACMPOUT);


Parentheses are not required.

jerome-pouiller · 2025-01-13T12:48:53Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+	CMU_ClockSelectSet(config->clock, cmuSelect_LFRCO);
+#endif
+
+	CMU_ClockEnable(config->clock, true);


Zephyr prefers to use the Zephyr API to manage clocks (clock_enable()). This API only exist on Series-2. However, since Series-0/1 is deprecated and it seems you mainly target xg24, I suggest to use clock_enable() and drop support for Series-1.

Once you only support Series-2, it will probably not make sense anymore to call this driver "gecko" ("gecko" is for Series 0/1 only). Probably "silabs,acmp" will be better.

jerome-pouiller · 2025-01-13T15:23:10Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+	const struct gecko_acmp_config *config = dev->config;
+	struct gecko_acmp_data *data = dev->data;
+
+	__ASSERT_NO_MSG(val != NULL);


hmm... I believe val is not defined.

jerome-pouiller · 2025-01-13T15:56:06Z

drivers/sensor/silabs/gecko_acmp/gecko_acmp.c

+		CMU_ClockEnable(config->clock, false);
+#endif /* CONFIG_GECKO_ACMP_DISABLE_ON_SUSPEND */
+#if CONFIG_GECKO_ACMP_DISABLE_INTERRUPT_ON_SUSPEND
+		NVIC_DisableIRQ(ACMP_IRQ_NUM(config->base));


As for the clock, I suggest to use Zephyr API for IRQ management.

jerome-pouiller · 2025-01-13T16:04:29Z

I hesitate to use comparator API but the lack of configuration on runtime (set/get attributes for example) for future integration led me to choose the sensor API.

I believe both option can be accepted. The API described in doc/hardware/peripherals/comparator.rst really match with ACMP. However, sensor framework seems more versatile.

jerome-pouiller · 2025-01-13T16:09:45Z

@RomainPelletant FYI, another contributor also contacted me because he started a driver for ACMP. His implementation is rather different because it is based on the comparator framework.

RomainPelletant · 2025-01-13T20:42:12Z

@jerome-pouiller if someone else has a better proposal I won't force anything: please let me know and I will give up on this PR

jerome-pouiller · 2025-01-16T14:12:58Z

@RomainPelletant, @silabs-chgalant has shared an early version of his work here: #83980. Your feedback would be valuable.

zephyrbot added area: Samples Samples platform: Silabs Silicon Labs area: Sensors Sensors labels Jan 3, 2025

zephyrbot requested review from asemjonovs, asmellby, avisconti, jeppenodgaard, jerome-pouiller, jhedberg, kartben, MaureenHelm, mnkp, nashif, teburd, tristan-google, ubieda, yonsch and yperess January 3, 2025 12:03

zephyrbot assigned MaureenHelm Jan 3, 2025

Romain Pelletant added 2 commits January 3, 2025 13:05

modules: hal_silabs: enable acmp hal module

bb2f3e7

- Enable ACMP hal in hal_silabs module Signed-off-by: Romain Pelletant <[email protected]>

soc: silabs: enable gecko acmp hal module

5191033

- Enable ACMP HAL module for silabs gecko Signed-off-by: Romain Pelletant <[email protected]>

RomainPelletant force-pushed the drivers/sensors/silabs/implement-gecko-acmp branch from 90db07d to 26cafa8 Compare January 3, 2025 12:05

drivers: sensor: silabs: add gecko acmp

4a76bc5

- Add SiLabs Gecko ACMP sensor driver Signed-off-by: Romain Pelletant <[email protected]>

RomainPelletant force-pushed the drivers/sensors/silabs/implement-gecko-acmp branch 2 times, most recently from 8d965a7 to 91032c1 Compare January 3, 2025 12:10

Romain Pelletant added 2 commits January 3, 2025 13:14

dts: arm: silabs: add acmp nodes

d97a7e3

- Add ACMP peripheral nodes for EFR32MG24 Signed-off-by: Romain Pelletant <[email protected]>

sample: sensor: add gecko acmp sample

cf51ed1

- Add sample for Gecko ACMP sensor driver Signed-off-by: Romain Pelletant <[email protected]>

RomainPelletant force-pushed the drivers/sensors/silabs/implement-gecko-acmp branch from 91032c1 to cf51ed1 Compare January 3, 2025 12:14

RomainPelletant marked this pull request as draft January 3, 2025 12:21

Martinhoff-maker reviewed Jan 6, 2025

View reviewed changes

jerome-pouiller reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sensors: silabs: add gecko acmp #83531

drivers: sensors: silabs: add gecko acmp #83531

RomainPelletant commented Jan 3, 2025 •

edited

Loading

Martinhoff-maker left a comment

Martinhoff-maker Jan 6, 2025

RomainPelletant commented Jan 6, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

RomainPelletant Jan 13, 2025

jerome-pouiller Jan 14, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller Jan 13, 2025

jerome-pouiller commented Jan 13, 2025

jerome-pouiller commented Jan 13, 2025

RomainPelletant commented Jan 13, 2025 •

edited

Loading

jerome-pouiller commented Jan 16, 2025

drivers: sensors: silabs: add gecko acmp #83531

Are you sure you want to change the base?

drivers: sensors: silabs: add gecko acmp #83531

Conversation

RomainPelletant commented Jan 3, 2025 • edited Loading

Martinhoff-maker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainPelletant commented Jan 6, 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

jerome-pouiller commented Jan 13, 2025

jerome-pouiller commented Jan 13, 2025

RomainPelletant commented Jan 13, 2025 • edited Loading

jerome-pouiller commented Jan 16, 2025

RomainPelletant commented Jan 3, 2025 •

edited

Loading

RomainPelletant commented Jan 13, 2025 •

edited

Loading