adsp: new combined pinctrl + GPIO driver by sipraga · Pull Request #3397 · analogdevicesinc/linux

sipraga · 2026-06-24T13:55:12Z

PR Description

Solution for #3381

Basically merge the pinctl and GPIO drivers into one under drivers/pinctrl. As a learning exercise I tried implementing this from scratch, also cause I found the existing drivers a bit hard to follow.

The main advantage, besides following upstream best practices and cleaning up, is that the design now allows for the mapping between PORT and PINT to be configurable in device tree. Doing so allows for race-free EDGE_BOTH interrupts. pinmux bindings are also nicer and now impossible to misconfigure for a given SoC.

You might also notice that OPEN_DRAIN configuration is missing, but that's deliberate, I'm just relying on gpiolib's default emulation since it should be good enough. Makes the code way simpler too.

NOTE: There is a [HACK] ... patch at the start, because I was hitting a timing issue. Working on solving it in #3420 and analogdevicesinc/lnxdsp-adi-meta#132 but should not affect the review process.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have compiled my changes, including the documentation
I have tested the changes on the relevant hardware (only SC598 so far!)
I have updated the documentation outside this repo accordingly
I have provided links for the relevant upstream lore

nunojsa · 2026-06-24T15:26:26Z

Have you checked the possibility of having the gpio controllers pieces under drivers/gpio? AFAIU, that's now the preferred way...

"The ADSP pinmux/conf/ctrl hardware configuration is smeared across both the PADS and PORT register spaces. This has led to an ugly interaction between pinctrl-adsp.c and gpio-adi-adsp-port.c, where the pinctrl driver dips into the GPIO driver to access the PORT MMR and takes a spinlock from its private driver data."

I understand that going from pinctrl into a gpioctrl driver is not good but the other way around (using the existing generic gpio APIs is fine)

nunojsa · 2026-06-24T15:27:44Z

Also given that you're re-writing the driver, it's maybe a good time to start naming them in a nicer way :). But I guess this is now just to show the intent

sipraga · 2026-07-07T15:23:53Z

damn updating these device trees is so exhausting

sipraga · 2026-07-08T14:40:52Z

I'll resolve the conflicts tomorrow. I also need to test this on sc58x and sc7x and sc594 somehow.

sipraga · 2026-07-10T13:42:18Z

Have you checked the possibility of having the gpio controllers pieces under drivers/gpio? AFAIU, that's now the preferred way...

"The ADSP pinmux/conf/ctrl hardware configuration is smeared across both the PADS and PORT register spaces. This has led to an ugly interaction between pinctrl-adsp.c and gpio-adi-adsp-port.c, where the pinctrl driver dips into the GPIO driver to access the PORT MMR and takes a spinlock from its private driver data."

I understand that going from pinctrl into a gpioctrl driver is not good but the other way around (using the existing generic gpio APIs is fine)

Ah sorry, I didn't actually give you an answer @nunojsa.

As usual with a gpiochip, the following ops need implementing:

	gc->request = gpiochip_generic_request;
	gc->free = gpiochip_generic_free;
	gc->get = adsp_gpio_get;
	gc->set = adsp_gpio_set;
	gc->get_direction = adsp_gpio_get_direction;
	gc->direction_input = adsp_gpio_direction_input;
	gc->direction_output = adsp_gpio_direction_output;
	gc->set_config = gpiochip_generic_config;

Right now I have this in the pinctrl-adsp.c driver. You are asking, why don't I put it in drivers/gpio? It's true that some of them can be done through the pinctrl API, but not all of them. In particular, .get can't be implemented through the pinctrl API (there is no way of reading pin value normally). But even for .set I could not find a single example of somebody using PIN_CONFIG_LEVEL to set output high/low/etc.

That above is the main reason. But I will also give you a more nebulous/gut-feeling reason. Consider the example from the bindings I wrote:

    pinctrl: pinctrl {
        compatible = "adi,adsp-sc598-pinctrl";
        #address-cells = <1>;
        #size-cells = <1>;
        ranges;
        adi,pads-syscon = <&pads_syscon>;

        gpio@31004000 {
            reg = <0x31004000 0x80>;
            gpio-controller;
            #gpio-cells = <2>;
            gpio-ranges = <&pinctrl 0 0 16>;
            interrupt-controller;
            #interrupt-cells = <2>;
            interrupt-parent = <&pint0>;
            adi,interrupt-ranges = <0 0 16>;
        };

        gpio@31004080 {
            reg = <0x31004080 0x80>;
            gpio-controller;
            #gpio-cells = <2>;
            gpio-ranges = <&pinctrl 0 16 16>;
            interrupt-controller;
            #interrupt-cells = <2>;
            interrupt-parent = <&pint0>;
            adi,interrupt-ranges = <0 16 16>;
        };

Here pinctrl has no reg, just ranges and its child GPIO nodes specify their address for the corresponding PORT MMR. The reason for this structure is that, often, pinmux spans multiple PORTs (e.g. ETH0 in RGMII mode spans PORTS H and I). You therefore want a single pinctrl node; having one pinctrl for each PORT would lead to quite goofy situations where you need multiple pinctrl groups to mux a single peripheral.

If we were to separate the drivers, I would have to set all of those gpio@xxx.reg values into pinctrl.reg instead, otherwise pinctrl could not access the registers it needs to do its job. I would also move the gpio nodes to be siblings of pinctrl rather than children. But then these gpio nodes would then have no reg property (nodes can't share the same reg). What do I name these gpio nodes? DT spec says I should call them gpio@xxx, but now that the reg property has been removed, so I will get warnings about this from the DT checker. So I call them gpioa, gpiob, etc.? Idk, everything starts to look rather weird when I go down this path.

All of that dance just to have code in drivers/gpio, code which essentially is just shim code for the real stuff in drivers/pinctrl, to me seems a bit overkill.

And, indeed, there are many examples of gpiochips being registered in drivers/pinctrl:

alvin@sol ~/projects/vendor/linux-adsp/drivers/pinctrl a/adsp/pinctrl-...merge$ rg gpiochip_add_data --no-heading
pinctrl-microchip-sgpio.c:897:	ret = devm_gpiochip_add_data(dev, gc, bank);
pinctrl-st.c:1568:	err  = gpiochip_add_data(&bank->gpio_chip, bank);
intel/pinctrl-cherryview.c:1576:	ret = devm_gpiochip_add_data(dev, chip, pctrl);
intel/pinctrl-baytrail.c:1549:	ret = devm_gpiochip_add_data(vg->dev, gc, vg);
renesas/pinctrl-rza1.c:1216:	ret = devm_gpiochip_add_data(rza1_pctl->dev, chip,
intel/pinctrl-intel.c:1419:	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl);
intel/pinctrl-lynxpoint.c:809:	ret = devm_gpiochip_add_data(dev, gc, lg);
renesas/pinctrl-rzg2l.c:2828:	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
renesas/pinctrl-rzv2m.c:973:	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
meson/pinctrl-amlogic-a4.c:1152:		ret  = gpiochip_add_data(&info->banks[i].gpio_chip, &info->banks[i]);
renesas/pinctrl-rzt2h.c:678:	ret = devm_gpiochip_add_data(dev, chip, pctrl);
pinctrl-pistachio.c:1425:		ret = gpiochip_add_data(&bank->gpio_chip, bank);
renesas/gpio.c:312:	ret = devm_gpiochip_add_data(pfc->dev, &chip->gpio_chip, chip);
pinctrl-mcp23s08.c:709:	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
pinctrl-rk805.c:667:	ret = devm_gpiochip_add_data(&pdev->dev, &pci->gpio_chip, pci);
...
(69 matches in total)

nunojsa · 2026-07-13T15:02:03Z

In particular, .get can't be implemented through the pinctrl API (there is no way of reading pin value normally). But even for .set I could not find a single example of somebody using PIN_CONFIG_LEVEL to set output high/low/etc.

Not sure I understand the above. You would just implement those the same way you have right now. But I do see that we need the IO addresses for the muxing configuration which would make the whole thing weird.

Anyways, nevermind. I just asked about it because you have both "flavors" in the kernel (split drivers or all in pinctrl) and I had the feeling split was the preferred way. But looking at the docs there's no clear rule about this.

sipraga · 2026-07-13T15:40:03Z

In particular, .get can't be implemented through the pinctrl API (there is no way of reading pin value normally). But even for .set I could not find a single example of somebody using PIN_CONFIG_LEVEL to set output high/low/etc.

Not sure I understand the above. You would just implement those the same way you have right now. But I do see that we need the IO addresses for the muxing configuration which would make the whole thing weird.

I was under the impression that you wanted me to call into pinctrl APIs to implement the gpiochip ops. If that were possible, a split might have made more sense (notwithstanding the iomap issue I mentioned above). Below is a bit of an elaboration on my point, just FYI, but feel free to ignore ;D

...

What I have right now is:

	gc->request = gpiochip_generic_request;
	gc->free = gpiochip_generic_free;
	gc->get = adsp_gpio_get;
	gc->set = adsp_gpio_set;
	gc->get_direction = adsp_gpio_get_direction;
	gc->direction_input = adsp_gpio_direction_input;
	gc->direction_output = adsp_gpio_direction_output;
	gc->set_config = gpiochip_generic_config;

Some of these, namely those assigned gpiochip_generic_xxx, use gpiolib wrappers which call into the pinctrl API. There's some explanation for gpiochip_generic_config in the gpio docs:

GPIO electrical configuration
-----------------------------

GPIO lines can be configured for several electrical modes of operation by using
the .set_config() callback. Currently this API supports setting:

- Debouncing
- Single-ended modes (open drain/open source)
- Pull up and pull down resistor enablement

These settings are described below.

The .set_config() callback uses the same enumerators and configuration
semantics as the generic pin control drivers. This is not a coincidence: it is
possible to assign the .set_config() to the function gpiochip_generic_config()
which will result in pinctrl_gpio_set_config() being called and eventually
ending up in the pin control back-end "behind" the GPIO controller, usually
closer to the actual pins. This way the pin controller can manage the below
listed GPIO configurations.

If a pin controller back-end is used, the GPIO controller or hardware
description needs to provide "GPIO ranges" mapping the GPIO line offsets to pin
numbers on the pin controller so they can properly cross-reference each other.

For request and free, the documentation is in pin-control.rst:

Pin control interaction with the GPIO subsystem
===============================================

Note that the following implies that the use case is to use a certain pin
from the Linux kernel using the API in ``<linux/gpio/consumer.h>`` with gpiod_get()
and similar functions. There are cases where you may be using something
that your datasheet calls "GPIO mode", but actually is just an electrical
configuration for a certain device. See the section below named
`GPIO mode pitfalls`_ for more details on this scenario.

The public pinmux API contains two functions named ``pinctrl_gpio_request()``
and ``pinctrl_gpio_free()``. These two functions shall *ONLY* be called from
gpiolib-based drivers as part of their ``.request()`` and ``.free()`` semantics.
Likewise the ``pinctrl_gpio_direction_input()`` / ``pinctrl_gpio_direction_output()``
shall only be called from within respective ``.direction_input()`` /
``.direction_output()`` gpiolib implementation.

NOTE that platforms and individual drivers shall *NOT* request GPIO pins to be
controlled e.g. muxed in. Instead, implement a proper gpiolib driver and have
that driver request proper muxing and other control for its pins.

The main purpose of this request/free invocation is to inform the pinctrl (pinmux specifically) subsystem of the GPIO mode; there is also the struct pinmux_ops::strict boolean flag which then enforces mutual exclusivity between GPIO and peripheral (muxed) mode. In the case of our HW, there's no need to defer to pinctrl for GPIO direction, but the facility exists (and is documented above) should this be needed.

All of that said, the subsystems are still orthogonal. pinctrl handles electrical properties and muxing, while GPIO handles GPIO input (read val)/output (get val) operation.

In the case of .get, my point was simply that pinctrl does not even offer an API to get the value (due to orthogonality, presumably). In the case of .set, you can kind of force it, by using PIN_CONFIG_LEVEL, but that's pretty dank and nobody really does it.

Anyways, nevermind. I just asked about it because you have both "flavors" in the kernel (split drivers or all in pinctrl) and I had the feeling split was the preferred way. But looking at the docs there's no clear rule about this.

The docs are a bit confusing and I definitely could have misunderstood, so it's good that you ask! :D

[ NEEDS REWORK ] There is an obvious race between the probing of eMMC and the probing of the I2C GPIO expander which controls the eMMC enable signal. If eMMC is probed too early, the hog might be inactive, and probing can fail. Use the simple MMC power sequence provider to ensure proper synchronization. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

These SoCs don't have an MMC controller. Remove. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

The driver will be replaced with a new one. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

nunojsa

First pass from me...

nunojsa · 2026-07-16T11:46:18Z

+        #interrupt-cells = <2>;
+    };
+
+    pinctrl: pinctrl {


If there´s nothing referencing it, just pinctrl {...

I have to specify it in the gpio-ranges properties of the gpio@ nodes in order to conform with the existing bindings. I wanted to remove it (since it's redundant) but there's no such variant of that. Do you want me to try and make a patch to relax this constraint? It might require some code changes to generic gpio/pinctrl DT parsing code.

nunojsa · 2026-07-16T11:46:39Z

+    type: object
+    patternProperties:
+      "-pins$":
+        $ref: "#/$defs/pin-cfg"


Would be nice to have the above on the example

What do you mean? The button-pins is an example of a pin-cfg type

nunojsa · 2026-07-16T11:47:28Z

+      - required: [ pinmux ]
+      - required: [ pins ]
+
+    additionalProperties: false


maybe the def could be putted above the scope "-pins$" scope. Otherwise it makes it a bit weird to be in the middle of the two properties using it

not sure exactly what you mean here, can you rephrase?

nunojsa · 2026-07-16T12:05:30Z

+	.probe = adsp_pint_probe,
+	.remove = adsp_pint_remove,
+};
+module_platform_driver(adsp_pint_driver);


So I assume these device don't really need to exist all the time :)?

there's no hard requirement for it to be built-in (you can even run the SoC without pinctrl if you want)

nunojsa · 2026-07-16T13:09:34Z

+		if (num_configs) {
+			ret = pinctrl_utils_add_map_configs(
+				pctldev, map, reserved_maps, num_maps, group,
+				configs, num_configs, PIN_MAP_TYPE_CONFIGS_PIN);


nunojsa · 2026-07-16T13:11:17Z

+				pctldev, child, map, &reserved_maps, num_maps);
+			if (ret)
+				goto out;
+		}


You could return right away and then put the else part in a neater indentation level

I prefer it this way

nunojsa · 2026-07-16T13:15:09Z

+	girq->parent_domain = irq_find_host(parent_np);
+	of_node_put(parent_np);
+	if (!girq->parent_domain)
+		return -EPROBE_DEFER;


Any special reason for -EPROBE_DEFER? I see there are some drivers doing the same upstream but other just error out.

this is the irqdomain of PINT, which - if it hasn't yet probed - will mean that irq_find_host returns NULL. so I think it's correct to return -EPROBE_DEFER.

nunojsa · 2026-07-16T13:16:37Z

+				     "missing gpio-controller child nodes\n");
+
+	pc->ports =
+		devm_kcalloc(dev, pc->nports, sizeof(*pc->ports), GFP_KERNEL);


odd line break

clang-format again :p

to clarify, I'm keeping as-is because I trust clang-format

The driver will be replaced with a new one. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

The Pin Interrupt (PINT) controller is found on Analog Devices ADSP SoCs. It senses GPIO pins and demultiplexes the parent interrupt. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

Analog Devices ADSP SoCs have a combined pinmux/GPIO block known as PORT. Document the hardware and add the appropriate dt-bindings headers for consumer device trees to set up the pinmux functions correctly. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

Add support for the Pin Interrupt (PINT) controller found on Analog Devices ADSP SoCs. Each PINT is a hierarchical interrupt controller with 32 interrupt lines, cascaded from a single GIC SPI. It demultiplexes its parent interrupt to as many as 32 connected GPIO pins of the connected GPIO PORT peripherals. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

sipraga · 2026-07-17T12:49:21Z

@nunojsa addressed or ignored all your comments. please resolve what you want

Add pinctrl and GPIO support for the PORT peripheral found on Analog Devices ADSP SoCs. Each alphabetically indexed PORT consists of up to 16 physical pins: PORTA has PA_00, PA_01, etc., PORTB PB_00, etc. Each pin can function as a GPIO, or be muxed to up to four alternate peripheral signals. Standard GPIO functionality such as direction or output level is configured in the PORT MMR space. Since pinmux is also part of this register space, the GPIO driver registration is folded into the pinctrl driver itself. Electrical properties such as drive strength or bias is configured in the PADS MMR space, which also includes other unrelated miscellaneous SoC configuration registers. The access to these registers is handled via the PADS syscon interface. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

The drivers and their bindings have been replaced. Update the device tree to reflect the new usage. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

sipraga requested a review from nunojsa June 24, 2026 13:55

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from 63117ae to 7b92208 Compare June 25, 2026 10:58

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from 7b92208 to 653a58a Compare July 7, 2026 08:58

This was referenced Jul 7, 2026

arm/arm64: dts: adi: replace adi,port-sizes with adi,npins #3369

Open

arm/arm64: dts: adi: use ngpios DT property for port pin count #3371

Open

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from 653a58a to 02cd23b Compare July 7, 2026 15:20

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from 02cd23b to d9f6272 Compare July 8, 2026 14:15

sipraga marked this pull request as ready for review July 8, 2026 14:40

sipraga requested review from a team July 8, 2026 14:41

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from d9f6272 to feee947 Compare July 9, 2026 12:43

pamolloy assigned sipraga Jul 9, 2026

pamolloy added this to ADSP Jul 9, 2026

qasim-ijaz reviewed Jul 10, 2026

View reviewed changes

Comment thread drivers/irqchip/irq-adi-adsp-pint.c

qasim-ijaz reviewed Jul 10, 2026

View reviewed changes

Comment thread drivers/irqchip/irq-adi-adsp-pint.c Outdated

qasim-ijaz reviewed Jul 10, 2026

View reviewed changes

Comment thread drivers/irqchip/irq-adi-adsp-pint.c

qasim-ijaz reviewed Jul 10, 2026

View reviewed changes

Comment thread drivers/irqchip/irq-adi-adsp-pint.c Outdated

qasim-ijaz requested review from a team July 10, 2026 08:41

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from feee947 to 4226310 Compare July 10, 2026 09:23

sipraga added 3 commits July 16, 2026 09:53

ARM: dts: adi: sc59x: remove mmc

a89a644

These SoCs don't have an MMC controller. Remove. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

gpio: remove old ADSP GPIO driver

b1da641

The driver will be replaced with a new one. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

nunojsa reviewed Jul 16, 2026

View reviewed changes

sipraga added 5 commits July 17, 2026 14:43

pinctrl: remove old ADSP pinctrl driver

1569850

The driver will be replaced with a new one. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

irqchip: remove old ADSP PINT driver

54bbc59

The driver will be replaced with a new one. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

dt-bindings: interrupt-controller: adi,adsp-pint: Document ADSP PINT

841b110

The Pin Interrupt (PINT) controller is found on Analog Devices ADSP SoCs. It senses GPIO pins and demultiplexes the parent interrupt. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from 4226310 to b1bea2b Compare July 17, 2026 12:48

sipraga added 5 commits July 17, 2026 15:02

arm64: dts: adi: sc59x-64: use new bindings for PINT and pinctrl/GPIO

42a2fe1

The drivers and their bindings have been replaced. Update the device tree to reflect the new usage. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

ARM: dts: adi: sc59x: use new bindings for PINT and pinctrl/GPIO

330169c

The drivers and their bindings have been replaced. Update the device tree to reflect the new usage. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

ARM: dts: adi: sc58x: use new bindings for PINT and pinctrl/GPIO

b3a9055

The drivers and their bindings have been replaced. Update the device tree to reflect the new usage. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

ARM: dts: adi: sc57x: use new bindings for PINT and pinctrl/GPIO

071dac3

The drivers and their bindings have been replaced. Update the device tree to reflect the new usage. Signed-off-by: Alvin Šipraga <alvin.sipraga@analog.com>

sipraga force-pushed the alvin/adsp/pinctrl-gpio-merge branch from b1bea2b to 071dac3 Compare July 17, 2026 13:02

Uh oh!

Conversation

sipraga commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

PR Type

PR Checklist

Uh oh!

nunojsa commented Jun 24, 2026

Uh oh!

nunojsa commented Jun 24, 2026

Uh oh!

sipraga commented Jul 7, 2026

Uh oh!

sipraga commented Jul 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipraga commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nunojsa commented Jul 13, 2026

Uh oh!

sipraga commented Jul 13, 2026

Uh oh!

nunojsa 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!

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!

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!

sipraga commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sipraga commented Jun 24, 2026 •

edited

Loading

sipraga commented Jul 10, 2026 •

edited

Loading