drivers: gpio: add NXP pcal9722 gpio driver #91728

singular0770 · 2025-06-16T23:10:11Z

Adds a driver for NXP's PCAL9722 22-pin SPI GPIO expander. The driver supports interrupts via the external interrupt line with individual pin configurable trigger type (rising edge, falling edge, level), selectable addressing via the addr pin in the device tree, and configuration of the internal pull-up/pull-down resistors.

Add DTS binding for NXP PCAL9722 SPI GPIO expander Signed-off-by: Michael Estes <michael.estes@byteserv.io>

decsny

the red change request is mostly because of the Z_SEM_INITIALIZER, everything else is mostly strong suggestions

decsny · 2025-06-23T16:36:15Z

drivers/gpio/gpio_pcal9722.c

+	};                                                                                         \
+                                                                                                   \
+	static struct pcal9722_drv_data pcal9722_drvdata_##n = {                                   \
+		.lock = Z_SEM_INITIALIZER(pcal9722_drvdata_##n.lock, 1, 1),                        \


this macro is internal implementation and should not be used in client source of kernel.h. Instead use K_SEM_DEFINE

Not sure K_SEM_DEFINE is equivalent. Other drivers that use K_SEM_DEFINE create a single static semaphore that is used across instances of the driver, whereas in this case I want to use a unique semaphore per instance of the driver as if you have multiple instances of the PCAL9722 there doesn't seem to be a reason to prevent the driver from handling both simultaneously. Directly replacing Z_SEM_INITIALIZER with K_SEM_DEFINE leads to a compiler error in the xmacro expansion which I don't entirely understand. I got this usage from basically all of the other PCA/PCAL drivers in the tree currently.

Not sure K_SEM_DEFINE is equivalent. Other drivers that use K_SEM_DEFINE create a single static semaphore that is used across instances of the driver, whereas in this case I want to use a unique semaphore per instance of the driver as if you have multiple instances of the PCAL9722 there doesn't seem to be a reason to prevent the driver from handling both simultaneously. Directly replacing Z_SEM_INITIALIZER with K_SEM_DEFINE leads to a compiler error in the xmacro expansion which I don't entirely understand. I got this usage from basically all of the other PCA/PCAL drivers in the tree currently.

You are correct that it is not equivalent and that you can't directly replace it inline, and that isn't what I was suggesting. What you should do it call it within the init macro somewhere, name the semaphore unique using the instance number (sem##n), then refer to it in this struct initialization. Then you will have a semaphore per instance and that is the syntax to do it.

Not sure K_SEM_DEFINE is equivalent. Other drivers that use K_SEM_DEFINE create a single static semaphore that is used across instances of the driver, whereas in this case I want to use a unique semaphore per instance of the driver as if you have multiple instances of the PCAL9722 there doesn't seem to be a reason to prevent the driver from handling both simultaneously. Directly replacing Z_SEM_INITIALIZER with K_SEM_DEFINE leads to a compiler error in the xmacro expansion which I don't entirely understand. I got this usage from basically all of the other PCA/PCAL drivers in the tree currently.

You are correct that it is not equivalent and that you can't directly replace it inline, and that isn't what I was suggesting. What you should do it call it within the init macro somewhere, name the semaphore unique using the instance number (sem##n), then refer to it in this struct initialization. Then you will have a semaphore per instance and that is the syntax to do it.

Thank you for the guidance. I had to change the lock to be a pointer type otherwise I got an "initializer element is not constant" error from gcc, but now it's compiling without errors. Will push after I have a chance to test it next week.

yes, when I said reference, I meant to imply that you will need a pointer in the struct

decsny · 2025-06-23T16:37:21Z

drivers/gpio/gpio_pcal9722.c

+	if (rc) {
+		LOG_ERR("%s failed to initialize: %d", dev->name, rc);
+	} else {
+		LOG_INF("%s successfully initialized", dev->name);


not sure that this log is really useful to have in the driver

Agreed. Will remove. I'm out of town currently for the Open Source Summit and won't be able to test until next week so holding off on committing any changes until then though.

decsny · 2025-06-23T16:41:10Z

drivers/gpio/gpio_pcal9722.c

+	if (cfg->interrupt_enabled) {
+		if (!gpio_is_ready_dt(&cfg->gpio_int)) {
+			LOG_ERR("Interrupt GPIO not ready");
+			rc = -EINVAL;
+			goto out;
+		}
+
+		drv_data->dev = dev;
+
+		k_work_init(&drv_data->work, gpio_pcal9722_work_handler);
+
+		rc = gpio_pin_configure_dt(&cfg->gpio_int, GPIO_INPUT);
+		if (rc) {
+			goto out;
+		}
+
+		rc = gpio_pin_interrupt_configure_dt(&cfg->gpio_int, GPIO_INT_EDGE_TO_ACTIVE);
+		if (rc) {
+			goto out;
+		}
+
+		gpio_init_callback(&drv_data->gpio_cb, gpio_pcal9722_init_cb,
+				   BIT(cfg->gpio_int.pin));
+		rc = gpio_add_callback(cfg->gpio_int.port, &drv_data->gpio_cb);
+
+		if (rc) {
+			goto out;
+		}
+	}


perhaps make this a helper function and #if out if that DT property has no occurrences?

#if (dt any inst has irq--gpios prop)
static int helper_irqs()
...
#else
#define helper_irqs()
#endif

would be nice just to be able to reduce the cognitive complexity of the init function in this way with the encapsulation of this block and removing the level of indentation

I like this idea, will definitely refactor the code to use #if DT_ANY_INST_HAS_PROP_STATUS_OKAY(irq_gpios) when I get back home next week and can test.

decsny · 2025-06-23T16:42:06Z

drivers/gpio/gpio_pcal9722.c

+	int rc = 0;
+	uint32_t pins = 0;


nit , reverse these lines to follow de facto standard of reverse christmas tree declarations

decsny · 2025-06-23T16:43:04Z

drivers/gpio/gpio_pcal9722.c

+	return rc;
+}
+
+static __maybe_unused int gpio_pcal9722_manage_callback(const struct device *dev,


following the other comment I left, don't need this maybe_unused if it is just not compiled in the case where it isnt used. also I have never seen this qualifier, is it generic to all toolchains?

Seems this is equivalent to __attribute__((unused)) which suppresses compiler warnings when the function is unused due to preprocessor macros. Might be specific to gcc though. I took it from one of the other gpio drivers (there are a few which use it), but agree it might be best to not use it if it's gcc specific.

decsny · 2025-06-23T17:03:41Z

drivers/gpio/gpio_pcal9722.c

+/* Mask for available pins */
+#define ALL_PINS ((uint32_t)BIT_MASK(NUM_PINS))


comment is also redundant here

decsny · 2025-06-23T17:04:00Z

drivers/gpio/gpio_pcal9722.c

+/* Number of pins supported by the device */
+#define NUM_PINS 22


the comment offends me here also

decsny · 2025-06-23T17:04:37Z

drivers/gpio/gpio_pcal9722.c

+/* PCAL9722 Register addresses */
+#define PCAL9722_INPUT_PORT0     0x00
+#define PCAL9722_INPUT_PORT1     0x01
+#define PCAL9722_INPUT_PORT2     0x02
+#define PCAL9722_OUTPUT_PORT0    0x04
+#define PCAL9722_OUTPUT_PORT1    0x05
+#define PCAL9722_OUTPUT_PORT2    0x06
+#define PCAL9722_POL_INV_PORT0   0x08
+#define PCAL9722_POL_INV_PORT1   0x09
+#define PCAL9722_POL_INV_PORT2   0x0A
+#define PCAL9722_CONFIG_PORT0    0x0C
+#define PCAL9722_CONFIG_PORT1    0x0D
+#define PCAL9722_CONFIG_PORT2    0x0E
+#define PCAL9722_PULL_EN0        0x4C
+#define PCAL9722_PULL_EN1        0x4D
+#define PCAL9722_PULL_EN2        0x4E
+#define PCAL9722_PULL_SEL0       0x50
+#define PCAL9722_PULL_SEL1       0x51
+#define PCAL9722_PULL_SEL2       0x52
+#define PCAL9722_IRQMASK_PORT0   0x54
+#define PCAL9722_IRQMASK_PORT1   0x55
+#define PCAL9722_IRQMASK_PORT2   0x56
+#define PCAL9722_IRQ_STAT_PORT0  0x58
+#define PCAL9722_IRQ_STAT_PORT1  0x59
+#define PCAL9722_IRQ_STAT_PORT2  0x5A
+#define PCAL9722_IRQEDGE_PORT0_A 0x60
+#define PCAL9722_IRQEDGE_PORT0_B 0x61
+#define PCAL9722_IRQEDGE_PORT1_A 0x62
+#define PCAL9722_IRQEDGE_PORT1_B 0x63
+#define PCAL9722_IRQEDGE_PORT2_A 0x64
+#define PCAL9722_IRQEDGE_PORT2_B 0x65
+#define PCAL9722_IRQ_CLEAR_PORT0 0x68
+#define PCAL9722_IRQ_CLEAR_PORT1 0x68
+#define PCAL9722_IRQ_CLEAR_PORT2 0x68


why exactly are all these definitions here? most of them don't seem used. if they are for future convenience then maybe at least would be better in a private header file

decsny · 2025-06-23T17:07:39Z

drivers/gpio/Kconfig.pcal9722

+	bool "PCAL9722 SPI GPIO chip"
+	default y
+	depends on DT_HAS_NXP_PCAL9722_ENABLED
+	depends on SPI


maybe select SPI instead, although I don't think best practice has been agreed upon.

I would have liked to have done this, but select SPI causes a dependency loop right now seemingly due to some boards where SPI depends on GPIO or vice versa.

decsny · 2025-06-23T17:10:36Z

dts/bindings/gpio/nxp,pcal9722.yaml

+  addr:
+    required: true
+    type: int
+    enum: [0x40, 0x42]
+    default: 0x40
+    description:
+      Address used to address device on the SPI bus. Two PCAL9722 devices can be connected to the
+      same SPI chip select by pulling the ADDR pin high or low. If pulled low, the address should
+      be 0x40. If pulled high, the address should be 0x42.


I'm confused what is meant by "address device on the SPI bus"? SPI does not have addressing, it has chip select lines. I'm not familiar with this device, can you explain?

I'm confused what is meant by "address device on the SPI bus"? SPI does not have addressing, it has chip select lines. I'm not familiar with this device, can you explain?

The PCAL9722 datasheet mentions that the first byte sent on the SPI bus can be used to "address" two PCAL9722 devices on the same chip select line, and that byte takes a value of either 0x40 or 0x42, depending on whether an external pin on the chip is pulled high or low. I vacillated on what to call this since I agree it's slightly confusing given that SPI doesn't support addressing in the I2C sense, but the datasheet refers to this as an "address" so that's what I went with. Hence why I tried to document it thoroughly with the description but if you think there's a better way to describe this or manage this in general I'm open to suggestions.

mmahadevan108 · 2025-06-23T17:41:50Z

@singular0770 , thanks for the contribution. How was this new driver tested?

singular0770 · 2025-06-25T01:07:42Z

@singular0770 , thanks for the contribution. How was this new driver tested?

I bench tested with a PCAL9722HN-ARD dev kit connected to a Nucleo board's Arduino header (both input and output, with and without the interrupt line). I also tested the "addressing" functionality with the jumper provided on the dev kit. I could probably add an overlay relatively easily for the gpio_basic_api test if advisable.

Adds a driver for NXP's PCAL9722 SPI GPIO expander Signed-off-by: Michael Estes <michael.estes@byteserv.io>

sonarqubecloud · 2025-06-29T23:27:25Z

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

dleach02 · 2025-07-09T16:28:01Z

@decsny did @singular0770 address your comments/requests?

github-actions bot added area: GPIO platform: NXP Drivers NXP Semiconductors, drivers labels Jun 16, 2025

github-actions bot requested review from dbaluta, decsny, dleach02, henrikbrixandersen, manuargue, mmahadevan108, mnkp and Raymond0225 June 16, 2025 23:11

github-actions bot assigned dleach02 and mmahadevan108 Jun 16, 2025

dts: bindings: gpio: add dts binding for NXP PCAL9722 SPI GPIO expander

1cfbc34

Add DTS binding for NXP PCAL9722 SPI GPIO expander Signed-off-by: Michael Estes <michael.estes@byteserv.io>

singular0770 force-pushed the add-pcal9722-gpio-driver branch 3 times, most recently from c429166 to 8165a93 Compare June 16, 2025 23:58

decsny requested changes Jun 23, 2025

View reviewed changes

drivers: gpio: add pcal9722 SPI GPIO expander driver

47178a6

Adds a driver for NXP's PCAL9722 SPI GPIO expander Signed-off-by: Michael Estes <michael.estes@byteserv.io>

singular0770 force-pushed the add-pcal9722-gpio-driver branch from 8165a93 to 47178a6 Compare June 29, 2025 22:44

singular0770 requested a review from decsny June 30, 2025 02:24

		/* Mask for available pins */
		#define ALL_PINS ((uint32_t)BIT_MASK(NUM_PINS))

		/* Number of pins supported by the device */
		#define NUM_PINS 22

drivers: gpio: add NXP pcal9722 gpio driver #91728

Are you sure you want to change the base?

drivers: gpio: add NXP pcal9722 gpio driver #91728

Conversation

singular0770 commented Jun 16, 2025

Uh oh!

decsny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singular0770 Jun 24, 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!

decsny Jun 25, 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!

Choose a reason for hiding this comment

Uh oh!

singular0770 Jun 24, 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!

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!

singular0770 Jun 25, 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!

mmahadevan108 commented Jun 23, 2025

Uh oh!

singular0770 commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 29, 2025

Quality Gate passed

Uh oh!

dleach02 commented Jul 9, 2025

Uh oh!

Uh oh!

singular0770 Jun 24, 2025 •

edited

Loading

decsny Jun 25, 2025 •

edited

Loading

singular0770 Jun 24, 2025 •

edited

Loading

singular0770 Jun 25, 2025 •

edited

Loading

singular0770 commented Jun 25, 2025 •

edited

Loading