Silabs gpio driver #92824

fimohame · 2025-07-08T09:08:07Z

The commit enables the gpio driver for EFR series 2 devices.

drivers/gpio/gpio_silabs.c

dts/arm/silabs/xg24/xg24.dtsi

Martinhoff-maker

I haven't gone through all the gpio_silabs.c files but in general:

We need to only use sl_hal_gpio functions. sl_gpio.h is already a driver in the HAL and it initializes things like clocks which we want to handle in the Zephyr driver.
We also need to have a new Kconfig symbol in zephyr/modules/hal_silabs/simplicity_sdk like "SILABS_SISDK_GPIO" and make changes to CMakeLists.txt accordingly. You can take inspiration from #92010
We should update the dtsi files for all Series 2 boards with this new GPIO driver and create an entry in the migration guide 4.3.
Don't forget to add a binding file for the driver.

drivers/gpio/Kconfig.silabs

dts/arm/silabs/xg24/xg24.dtsi

drivers/gpio/Kconfig.silabs

drivers/gpio/gpio_silabs.c

Martinhoff-maker

As general comment, as done in gpio_silabs_siwx91x.c, it could be good to have clear device name in general : is it the parent device ( the gpio controller) or the port device rather ? than just having "dev" names everywhere.

Adding PM would be perfect also (might be part of another PR)

dts/arm/silabs/xg21/xg21.dtsi

modules/hal_silabs/simplicity_sdk/CMakeLists.txt

drivers/gpio/gpio_silabs.c

asmellby

Looks mostly good to me, a few minor comments.

asmellby · 2025-08-06T11:43:58Z

drivers/gpio/gpio_silabs.c

+	} else {
+		ARRAY_FOR_EACH(data->irq_pin_map, i) {
+			if ((data->int_enabled_mask & BIT(i)) && (data->irq_pin_map[i] == pin)) {
+				sl_hal_gpio_disable_interrupts(BIT(i));


What is the purpose of disabling the interrupt in the "enable" branch?
If the purpose is to trick the configure function into reallocating the same interrupt number if an interrupt was already configured to this pin, I would instead just set int_no = i; here, since the configure function only tries to allocate if given the magic "unavailable" value.

asmellby · 2025-08-06T11:49:07Z

drivers/gpio/gpio_silabs.c

+	/* bitmask of pins with interrupt enabled */
+	uint32_t int_enabled_mask;
+	/* maps interrupt line index to GPIO pin number, 0xFF if unused */
+	uint8_t irq_pin_map[16];


Could this be added to struct gpio_silabs_common_data instead?

I guess a const struct device * would need to be added to the port struct to point back to the common node, but that would still be more efficient (4*4 + 16 = 32 bytes instead of 4*16 = 64 bytes).

It would correspond to the hardware better, since two ports can't disagree on the irq-pin mapping, and it would also set us up better for adding em4wu interrupt support in the future.

asmellby · 2025-08-06T13:37:28Z

drivers/gpio/gpio_silabs.c

+		}
+		const struct device *port_dev = data->ports[i];
+		struct gpio_silabs_port_data *port_data = port_dev->data;
+		struct gpio_silabs_common_data *common = port_data->common_dev->data;


This can be hoisted outside the for loop -- dev is already the gpio peripheral (same as port_data->common_dev).

drivers/gpio/gpio_silabs.c

Added the gpio driver for EFR series 2 devices. The SILABS_SISDK_GPIO symbol is added to enable support for the new GPIO driver. The SOC_GECKO_GPIO symbol is retained for now to maintain compatibility with existing drivers and will be removed in a subsequent commit. Signed-off-by: S Mohamed Fiaz <[email protected]>

Updated Kconfig for EFR series 2 devices. Signed-off-by: S Mohamed Fiaz <[email protected]>

Added the xg29_rb4412a.overlay for GPIO API 1-pin This overlay sets the configuration needed to validate the GPIO driver on the xg29_rb4412a board during test execution. Signed-off-by: S Mohamed Fiaz <[email protected]>

sonarqubecloud · 2025-08-07T11:40:50Z

Quality Gate passed

Issues
1 New issue
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

drivers/gpio/gpio_silabs.c

Changes seem to be addressed

jerome-pouiller · 2025-08-07T13:16:50Z

drivers/gpio/gpio_silabs.c

+		.clock = DEVICE_DT_GET(DT_INST_CLOCKS_CTLR(idx)),                                  \
+		.clock_cfg = SILABS_DT_INST_CLOCK_CFG(idx),                                        \
+	};                                                                                         \
+	DEVICE_DT_INST_DEFINE(idx, gpio_silabs_common_init, NULL, &gpio_silabs_common_data_inst,   \


I suggest to be consistent. If you consider there can be several GPIO devices, we probably need one gpio_silabs_common_data per instance.

jerome-pouiller · 2025-08-07T13:17:55Z

drivers/gpio/gpio_silabs.c

+		return 0;                                                                          \
+	}                                                                                          \
+	static const struct gpio_silabs_port_config gpio_silabs_port_##n##_config = {              \
+		.common = {.port_pin_mask = (gpio_port_pins_t)(-1)},                               \


This should be enough: .common.port_pin_mask = (gpio_port_pins_t)(-1),

jerome-pouiller · 2025-08-07T13:19:57Z

drivers/gpio/gpio_silabs.c

+	static int gpio_silabs_port_##n##_init(const struct device *dev)                           \
+	{                                                                                          \
+		struct gpio_silabs_port_data *data = dev->data;                                    \
+		data->common_dev = DEVICE_DT_GET(DT_PARENT(n));                                    \


DEVICE_DT_GET(DT_PARENT(n)) can be located in gpio_silabs_port_config. Then, you won't have to define one function per port.

jerome-pouiller · 2025-08-07T13:20:58Z

drivers/gpio/gpio_silabs.c

+	};                                                                                         \
+	static struct gpio_silabs_port_data gpio_silabs_port_##n##_data;                           \
+	DEVICE_DT_DEFINE(n, gpio_silabs_port_##n##_init, NULL, &gpio_silabs_port_##n##_data,       \
+			 &gpio_silabs_port_##n##_config, PRE_KERNEL_1, CONFIG_GPIO_INIT_PRIORITY,  \


We rather tend to place index value at the end of the names: gpio_silabs_port_init##n, gpio_silabs_port_data##n, etc...

jerome-pouiller · 2025-08-07T13:26:41Z

drivers/gpio/Kconfig.silabs

+
+config GPIO_SILABS_COMMON_INIT_PRIORITY
+	int "Common initialization priority"
+	depends on GPIO_SILABS


You already said if GPIO_SILABS above.

jerome-pouiller · 2025-08-07T13:47:42Z

drivers/gpio/gpio_silabs.c

+};
+
+static DEVICE_API(gpio, gpio_common_driver_api) = {
+	.manage_callback = gpio_silabs_port_manage_callback,


gpio_silabs_port_manage_callback() expect a gpio_silabs_port as parameter. It won't be the case here.

jerome-pouiller · 2025-08-07T13:48:48Z

drivers/gpio/gpio_silabs.c

+		}
+		const struct device *port_dev = data->ports[i];
+		struct gpio_silabs_port_data *port_data = port_dev->data;
+		uint32_t enabled_int = int_status & port_data->int_enabled_mask;


No declarations after statements.

drivers/gpio/gpio_silabs.c

jerome-pouiller · 2025-08-07T13:56:21Z

drivers/gpio/gpio_silabs.c

+
+#if GPIO_SILABS_COMMON_INIT_PRIORITY >= CONFIG_GPIO_INIT_PRIORITY
+#error GPIO_SILABS_COMMON_INIT_PRIORITY must be less than CONFIG_GPIO_INIT_PRIORITY
+#endif


Since the GPIO_Ports are children of GPIO_Common, I believe Zephyr would raise an error, even without this check.

jerome-pouiller · 2025-08-07T13:57:27Z

drivers/gpio/gpio_silabs.c

+};
+
+static inline void gpio_silabs_add_port(struct gpio_silabs_common_data *data,
+					const struct device *dev)


Indentation

jerome-pouiller · 2025-08-07T14:07:01Z

tests/drivers/gpio/gpio_api_1pin/boards/xg29_rb4412a.overlay

+		compatible = "gpio-leds";
+		led0: led_0 {
+			gpios = <&gpioc 7 GPIO_ACTIVE_HIGH>;
+		};


Why the untouched DTS does not work?

it is clearing the port bit of the uart, even if the uart is port A, it will kill VCOM and then the uart output over USB

jerome-pouiller · 2025-08-07T14:41:07Z

drivers/gpio/gpio_silabs.c

+static int gpio_silabs_pin_configure(const struct device *dev, gpio_pin_t pin, gpio_flags_t flags)
+{
+	const struct gpio_silabs_port_config *config = dev->config;
+	sl_gpio_t gpio = {.port = config->gpio_index, .pin = pin};


sl_gpio_t gpio = { .port = config->gpio_index, .pin = pin };

jerome-pouiller · 2025-08-07T14:43:46Z

drivers/gpio/gpio_silabs.c

+			out = 0U;
+		} else {
+			pin_out = sl_hal_gpio_get_pin_output(&gpio);
+			out = pin_out;


Probably pin_out is here for bad reasons. Why not out = sl_hal_gpio_get_pin_output(&gpio)? or out = !!sl_hal_gpio_get_pin_output(&gpio)?

jerome-pouiller · 2025-08-07T14:46:18Z

drivers/gpio/gpio_silabs.c

+	const struct gpio_silabs_port_config *config = dev->config;
+	sl_gpio_t gpio = {.port = config->gpio_index, .pin = pin};
+	sl_gpio_mode_t mode;
+	uint16_t out = 0U;


Why a uint16_t? sl_hal_gpio_set_pin_mode() takes a bool as input.

jerome-pouiller · 2025-08-07T14:48:11Z

drivers/gpio/gpio_silabs.c

+			mode = SL_GPIO_MODE_INPUT_PULL;
+			out = 1U;
+		} else if (flags & GPIO_PULL_DOWN) {
+			mode = SL_GPIO_MODE_INPUT_PULL;


I suggest to set value for out since it matters:

out = false; // Pull-down

jerome-pouiller · 2025-08-07T14:49:56Z

drivers/gpio/gpio_silabs.c

+
+#ifdef CONFIG_GPIO_GET_CONFIG
+static int gpio_silabs_pin_get_config(const struct device *dev, gpio_pin_t pin,
+				      gpio_flags_t *out_flags)


Indentation

jerome-pouiller · 2025-08-07T14:58:50Z

drivers/gpio/gpio_silabs.c

+		common->irq_pin_map[int_no] = pin;
+
+		sl_hal_gpio_enable_interrupts(BIT(int_no));
+		WRITE_BIT(data->int_enabled_mask, int_no, true);


Because we prefer to keep only one source of truth, I suggest to get rid of int_enabled_mask and used sl_hal_gpio_get_enabled_interrupts() instead.

jerome-pouiller · 2025-08-07T15:00:47Z

drivers/gpio/gpio_silabs.c

+
+	if (mode == GPIO_INT_MODE_DISABLED) {
+		ARRAY_FOR_EACH(common->irq_pin_map, i) {
+			if ((data->int_enabled_mask & BIT(i)) && (common->irq_pin_map[i] == pin)) {


Maybe irq_pin_map could be replaced by calls to sl_hal_gpio_get_external_interrupt_number() (this is prefered solution because we prefer a single source of trust).

When an interrupt occurs on any line, you need to examine the EXTIPINSEL* bitfield to determine which pin is associated with that interrupt line, and then construct the appropriate pin bitmask to pass to the callback dispatcher.

Currently, sl_hal_gpio does not provide a helper function for this task.

jerome-pouiller · 2025-08-07T15:05:50Z

drivers/gpio/gpio_silabs.c

+	uint32_t int_status = GPIO->IF;
+
+	ARRAY_FOR_EACH(data->ports, i) {
+		if (!int_status) {


This early exit make this function more complex to understand and it probably does not improve the performance.

jerome-pouiller · 2025-08-07T15:07:05Z

drivers/gpio/gpio_silabs.c

+	.port_clear_bits_raw = gpio_silabs_port_clear_bits_raw,
+	.port_toggle_bits = gpio_silabs_port_toggle_bits,
+	.pin_interrupt_configure = gpio_silabs_pin_interrupt_configure,
+	.manage_callback = gpio_silabs_port_manage_callback,


nit: you can align the = signs.

jhedberg · 2025-08-07T15:49:38Z

@fimohame looks like there were plenty of valid points raised by @jerome-pouiller which came a bit too late. Will you work on a follow-up PR to address those?

fimohame · 2025-08-07T16:27:20Z

@jhedberg, Yes, I'm on it. Will be working on the follow-up PR

fimohame · 2025-08-08T04:13:24Z

@jerome-pouiller @jhedberg @asmellby @Martinhoff-maker

I have addressed the comments in PR: #94251

Could you please review the changes?

fimohame closed this Jul 8, 2025

fimohame reopened this Jul 8, 2025

fimohame closed this Jul 8, 2025

fimohame reopened this Jul 8, 2025

fimohame closed this Jul 8, 2025

fimohame reopened this Jul 8, 2025

fimohame force-pushed the silabs_gpio_driver branch from 74b3e13 to eca5b4b Compare July 8, 2025 10:18

smalae previously requested changes Jul 8, 2025

View reviewed changes

fimohame force-pushed the silabs_gpio_driver branch 3 times, most recently from 6c73c92 to e46e7bc Compare July 9, 2025 05:59

fimohame requested a review from smalae July 9, 2025 06:04

fimohame marked this pull request as ready for review July 10, 2025 08:05

github-actions bot added area: GPIO platform: Silabs Silicon Labs labels Jul 10, 2025

github-actions bot requested review from asmellby, henrikbrixandersen, jerome-pouiller, jhedberg, Martinhoff-maker and mnkp July 10, 2025 08:05

github-actions bot assigned jhedberg and asmellby Jul 10, 2025

Martinhoff-maker requested changes Jul 11, 2025

View reviewed changes

fimohame force-pushed the silabs_gpio_driver branch from e46e7bc to 165546d Compare July 16, 2025 13:45

fimohame requested a review from Martinhoff-maker July 16, 2025 14:04

fimohame force-pushed the silabs_gpio_driver branch from 165546d to aedd2cc Compare July 17, 2025 06:29

zephyrbot requested review from rettichschnidi and yonsch July 17, 2025 06:30

Martinhoff-maker reviewed Jul 21, 2025

View reviewed changes

fimohame closed this Aug 6, 2025

fimohame reopened this Aug 6, 2025

fimohame requested a review from asmellby August 6, 2025 11:17

asmellby reviewed Aug 6, 2025

View reviewed changes

fimohame force-pushed the silabs_gpio_driver branch from 3bdb92d to 0b1a809 Compare August 6, 2025 13:20

fimohame requested a review from asmellby August 6, 2025 13:22

fimohame force-pushed the silabs_gpio_driver branch from 0b1a809 to a6f0d6e Compare August 6, 2025 13:32

asmellby reviewed Aug 6, 2025

View reviewed changes

fimohame force-pushed the silabs_gpio_driver branch from a6f0d6e to 54200a9 Compare August 6, 2025 13:49

fimohame requested a review from asmellby August 6, 2025 13:50

asmellby reviewed Aug 6, 2025

View reviewed changes

drivers/gpio/gpio_silabs.c Outdated Show resolved Hide resolved

fimohame force-pushed the silabs_gpio_driver branch from 54200a9 to df878a5 Compare August 6, 2025 18:11

fimohame requested a review from asmellby August 6, 2025 18:13

fimohame force-pushed the silabs_gpio_driver branch from df878a5 to 7fdbce6 Compare August 7, 2025 11:17

fimohame added 3 commits August 7, 2025 16:52

soc: silabs: series_s2: Kconfig updates for EFR series 2 devices

9de8a95

Updated Kconfig for EFR series 2 devices. Signed-off-by: S Mohamed Fiaz <[email protected]>

tests: gpio: boards: add overlay for GPIO test driver

fdfe2a7

Added the xg29_rb4412a.overlay for GPIO API 1-pin This overlay sets the configuration needed to validate the GPIO driver on the xg29_rb4412a board during test execution. Signed-off-by: S Mohamed Fiaz <[email protected]>

fimohame force-pushed the silabs_gpio_driver branch from 7fdbce6 to fdfe2a7 Compare August 7, 2025 11:23

asmellby approved these changes Aug 7, 2025

View reviewed changes

Martinhoff-maker approved these changes Aug 7, 2025

View reviewed changes

jhedberg reviewed Aug 7, 2025

View reviewed changes

drivers/gpio/gpio_silabs.c Show resolved Hide resolved

cfriedt merged commit dd83b64 into zephyrproject-rtos:main Aug 7, 2025
30 checks passed

jerome-pouiller requested changes Aug 7, 2025

View reviewed changes

jerome-pouiller reviewed Aug 7, 2025

View reviewed changes

loopj mentioned this pull request Aug 8, 2025

silabs: new gpio driver is broken when mcuboot bootloader is used #94281

Closed

1 task

Silabs gpio driver #92824

Silabs gpio driver #92824

Uh oh!

Conversation

fimohame commented Jul 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Martinhoff-maker 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!

Uh oh!

Martinhoff-maker 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!

asmellby left a comment

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!

Uh oh!

sonarqubecloud bot commented Aug 7, 2025

Quality Gate passed

Uh oh!

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!

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