[PATCH RESEND] rockchip: Make gpiod pin control work and add gpio-ranges

[PATCH RESEND] rockchip: Make gpiod pin control work and add gpio-ranges

[Matthijs Kooijman]

Hi folks,

This is a resend of a two patches I submitted last december. Lacking any
replies, I'm resending it (and also explicitly sending it to the GPIO
subsystem maintainers Linus and Bartosz this time - I'm still a bit
unsure whom to address exactly).

The first patch fixes pin config (e.g. bias) done by userspace via the
gpiod interface, which was not implemented for all rockchip boards.

The second patch is just because I was messing with this code already
and had a test setup ready. It makes gpio-ranges explicit for the rk3308
instead of relying on the (possibly fragile) legacy workaround in
gpio-rockchip to add them automatically. I tested this be removing the
legacy workaround from the code during my testing.

I think the first patch might be a good candidate to backport to the
stable releases, since it makes a userspace interface functional that
currently silently fails. However, it is not a clear bugfix and I am not
super familiar with the rules for -stable, so I left out any stable Cc
tags. Feel free to add them if this seems appropriate.

Both patches were tested on a rock pi s with a rk3308.

Gr.

Matthijs

[PATCH RESEND 1/2] arm64: dts: rockchip: rk3308: Add gpio-ranges properties

[Matthijs Kooijman]

This makes the mapping between gpio and pinctrl explicit.

This does not immediately change functionality, because the
gpio-rockchip.c driver has a workaround that defines ranges when they
are not present in DT, but that relies on global gpio numbering (so
AFAICS only works when the rockchip gpio banks are initialized first and
in-order).

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 arch/arm64/boot/dts/rockchip/rk3308.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3308.dtsi b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
index 31c25de2d689c..681d2429d541d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3308.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
@@ -889,6 +889,7 @@ gpio0: gpio@ff220000 {
 			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&cru PCLK_GPIO0>;
 			gpio-controller;
+			gpio-ranges = <&pinctrl 0 0 32>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
@@ -900,6 +901,7 @@ gpio1: gpio@ff230000 {
 			interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&cru PCLK_GPIO1>;
 			gpio-controller;
+			gpio-ranges = <&pinctrl 0 32 32>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
@@ -911,6 +913,7 @@ gpio2: gpio@ff240000 {
 			interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&cru PCLK_GPIO2>;
 			gpio-controller;
+			gpio-ranges = <&pinctrl 0 64 32>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
@@ -922,6 +925,7 @@ gpio3: gpio@ff250000 {
 			interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&cru PCLK_GPIO3>;
 			gpio-controller;
+			gpio-ranges = <&pinctrl 0 96 32>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
@@ -933,6 +937,7 @@ gpio4: gpio@ff260000 {
 			interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&cru PCLK_GPIO4>;
 			gpio-controller;
+			gpio-ranges = <&pinctrl 0 128 32>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
-- 
2.48.1

Re: [PATCH RESEND 1/2] arm64: dts: rockchip: rk3308: Add gpio-ranges properties

[Jonas Karlman]

Hi Matthijs,

On 3/2/2026 9:17 PM, Matthijs Kooijman wrote:
> This makes the mapping between gpio and pinctrl explicit.
> 
> This does not immediately change functionality, because the
> gpio-rockchip.c driver has a workaround that defines ranges when they
> are not present in DT, but that relies on global gpio numbering (so
> AFAICS only works when the rockchip gpio banks are initialized first and
> in-order).

This is strictly not correct, the driver use the gpioX alias id as
defined in the device tree and does not depend on the initialization
order.

This file explicitly define the aliases matching the hardware:

	aliases {
		gpio0 = &gpio0;
		gpio1 = &gpio1;
		gpio2 = &gpio2;
		gpio3 = &gpio3;
		gpio4 = &gpio4;
		[...]
	};

> 
> Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
> ---
>  arch/arm64/boot/dts/rockchip/rk3308.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3308.dtsi b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
> index 31c25de2d689c..681d2429d541d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3308.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
> @@ -889,6 +889,7 @@ gpio0: gpio@ff220000 {
>  			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&cru PCLK_GPIO0>;
>  			gpio-controller;
> +			gpio-ranges = <&pinctrl 0 0 32>;

This matches the driver, but not the hardware according to datasheet, it
only lists gpio0 A0-C5 used, and not all 32 pins supported by the gpio0
controller.

>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -900,6 +901,7 @@ gpio1: gpio@ff230000 {
>  			interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&cru PCLK_GPIO1>;
>  			gpio-controller;
> +			gpio-ranges = <&pinctrl 0 32 32>;

Same here, the datasheet only lists gpio1 A0-D1 used, and not all 32
pins supported by the gpio1 controller.

>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -911,6 +913,7 @@ gpio2: gpio@ff240000 {
>  			interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&cru PCLK_GPIO2>;
>  			gpio-controller;
> +			gpio-ranges = <&pinctrl 0 64 32>;

Same here, the datasheet only lists gpio1 A0-C0 used, and not all 32
pins supported by the gpio2 controller.

>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -922,6 +925,7 @@ gpio3: gpio@ff250000 {
>  			interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&cru PCLK_GPIO3>;
>  			gpio-controller;
> +			gpio-ranges = <&pinctrl 0 96 32>;

Same here, the datasheet only lists gpio1 A0-B5 used, and not all 32
pins supported by the gpio3 controller.

>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -933,6 +937,7 @@ gpio4: gpio@ff260000 {
>  			interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&cru PCLK_GPIO4>;
>  			gpio-controller;
> +			gpio-ranges = <&pinctrl 0 128 32>;

Same here, the datasheet only lists gpio1 A0-C1 and D0-D7 used, and not
all 32 pins supported by the gpio4 controller.

Regards,
Jonas

>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;

Re: [PATCH RESEND 1/2] arm64: dts: rockchip: rk3308: Add gpio-ranges properties

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

Hi Jonas,

Thanks for your review.

> > This does not immediately change functionality, because the
> > gpio-rockchip.c driver has a workaround that defines ranges when they
> > are not present in DT, but that relies on global gpio numbering (so
> > AFAICS only works when the rockchip gpio banks are initialized first and
> > in-order).
> 
> This is strictly not correct, the driver use the gpioX alias id as
> defined in the device tree and does not depend on the initialization
> order.

Ah, I had not realized these aliases existed. However, it seems they are
not actually relevant in this case. My assumption was that gpio
numbering was based on initalizating order, but I see in the code that
GPIO drivers decide themselves (by setting gc->base statically or maybe
based on DT). For the gpio-rockchip driver, these base numbers are
hardcoded to start from 0 in rockchip_pinctrl_get_soc_data(). Dynamic
initialization-order based numbering is also done for drivers that do
not set gc->base, but that starts at GPIO number 512 to prevent
conflicts.

In any case, I will update my commit message to better reflect what is
happening.


> > --- a/arch/arm64/boot/dts/rockchip/rk3308.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
> > @@ -889,6 +889,7 @@ gpio0: gpio@ff220000 {
> >  			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> >  			clocks = <&cru PCLK_GPIO0>;
> >  			gpio-controller;
> > +			gpio-ranges = <&pinctrl 0 0 32>;
> 
> This matches the driver, but not the hardware according to datasheet, it
> only lists gpio0 A0-C5 used, and not all 32 pins supported by the gpio0
> controller.

Indeed, this seems to be the case, but I wonder if this is a problem?

Isee other rockchip devicetree definitions (rk3528, rk3562, rk356x,
rk3576, rk3588) do not care about this and just map all 32 pins. See

	git grep -C 20 rockchip,gpio-bank | grep gpio-ranges

I also think there is no other provision for these missing GPIOs
anywhere - both pinctrl and gpiod seem to expose all 32 pins, even the
one that do not exist.

Limiting the gpio-ranges definitions would prevent writing to reserved
pinctrl registers via the userspace gpio API, which might be useful, but
you would still be able to write to them via a custom devicetree (that
uses non-existing pinctrl pins) and you would still be able to write to
non-existent gpio registers via the userspace API.

Given the above, do you still think it would be good to limit the
gpio-ranges to the existing GPIOs only (I have no strong opinion)?

Gr.

Matthijs

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND 1/2] arm64: dts: rockchip: rk3308: Add gpio-ranges properties

[Linus Walleij]

On Tue, Mar 10, 2026 at 9:53 AM Matthijs Kooijman <matthijs@stdin.nl> wrote:

> Ah, I had not realized these aliases existed. However, it seems they are
> not actually relevant in this case. My assumption was that gpio
> numbering was based on initalizating order,

It usually is.

We do not like it when people depend on the global GPIO numbering.

> but I see in the code that
> GPIO drivers decide themselves (by setting gc->base statically or maybe
> based on DT).

They *can* but they really shouldn't. This mechanism exists for
legacy reasons.

Neither kernel drivers nor userspace should depend on the
global GPIO number.

We even replaced the sysfs with a mechanism that avoids using
the global GPIO number in favour of chip-local offsets to drive
home the point.

The reasoning behind this is clear: it is impossible to maintain
the global numberspace in any system with enough hot-pluggable
GPIO expanders such as on USB and to some extent other
slow buses. Do we know how many will be plugged in? No.
So we can't have any reliable numberspace.

It's only embedded developers who has never seen a USB
GPIO expander who thinks the global numberspace can work.

Yours,
Linus Walleij

Re: [PATCH RESEND 1/2] arm64: dts: rockchip: rk3308: Add gpio-ranges properties

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

Hey Linus,

> We do not like it when people depend on the global GPIO numbering.

Yeah, I know. That's also why this patch is useful: It stops relying on
global numbering for auto-defining gpio-ranges and instead defines them
explicitly (for this particular DT at least).

Looking at the code (rockchip_gpiolib_register()), I do think it could
be rewritten to use dynamic GPIO numbering (since the same code
currently sets gc->base based on the bank->pin_base, and then defines
gpio-ranges based on gc->base, but I think it could just use
bank->pin_base directly and *not* set gc->base at all to use dynamic
numbering). However, actually making such invasive changes is out of
scope (and time budget) for me :-)

> > but I see in the code that
> > GPIO drivers decide themselves (by setting gc->base statically or maybe
> > based on DT).
>
> They *can* but they really shouldn't. This mechanism exists for
> legacy reasons.

I did read this comment today:

	/* dynamic allocation of GPIOs, e.g. on a hotplugged device */
	static int gpiochip_find_base_unlocked(u16 ngpio)

Which suggested to me that static allocation was still the recommended
default and dynamic allocation primarily used for cases where static
allocation cannot work (like hotplug). But your messages suggests that
static allocation is discouraged more actively than that.

Gr.

Matthijs

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND 1/2] arm64: dts: rockchip: rk3308: Add gpio-ranges properties

[Jonas Karlman]

Hi Matthijs,

On 3/10/2026 9:53 AM, Matthijs Kooijman wrote:
> Hi Jonas,
> 
> Thanks for your review.
> 
>>> This does not immediately change functionality, because the
>>> gpio-rockchip.c driver has a workaround that defines ranges when they
>>> are not present in DT, but that relies on global gpio numbering (so
>>> AFAICS only works when the rockchip gpio banks are initialized first and
>>> in-order).
>>
>> This is strictly not correct, the driver use the gpioX alias id as
>> defined in the device tree and does not depend on the initialization
>> order.
> 
> Ah, I had not realized these aliases existed. However, it seems they are
> not actually relevant in this case. My assumption was that gpio
> numbering was based on initalizating order, but I see in the code that
> GPIO drivers decide themselves (by setting gc->base statically or maybe
> based on DT). For the gpio-rockchip driver, these base numbers are
> hardcoded to start from 0 in rockchip_pinctrl_get_soc_data(). Dynamic
> initialization-order based numbering is also done for drivers that do
> not set gc->base, but that starts at GPIO number 512 to prevent
> conflicts.
> 
> In any case, I will update my commit message to better reflect what is
> happening.
> 
> 
>>> --- a/arch/arm64/boot/dts/rockchip/rk3308.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
>>> @@ -889,6 +889,7 @@ gpio0: gpio@ff220000 {
>>>  			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
>>>  			clocks = <&cru PCLK_GPIO0>;
>>>  			gpio-controller;
>>> +			gpio-ranges = <&pinctrl 0 0 32>;
>>
>> This matches the driver, but not the hardware according to datasheet, it
>> only lists gpio0 A0-C5 used, and not all 32 pins supported by the gpio0
>> controller.
> 
> Indeed, this seems to be the case, but I wonder if this is a problem?

The device tree should represent the hardware and not work as
configuration for a software implementation.

The SoC has X amounts of pins, however each gpio controller have support
to control up to 32 routed pins.

So to accurately describe the pincntrl <-> gpio controller relation we
should likely only include the routed pins.

> 
> Isee other rockchip devicetree definitions (rk3528, rk3562, rk356x,
> rk3576, rk3588) do not care about this and just map all 32 pins. See
> 
> 	git grep -C 20 rockchip,gpio-bank | grep gpio-ranges
> 
> I also think there is no other provision for these missing GPIOs
> anywhere - both pinctrl and gpiod seem to expose all 32 pins, even the
> one that do not exist.

Correct, and this is the issue, the pinctrl driver exposes pins that do
not exists, i.e. the software is wrong here and we should not add DT props
just to satisfy incorrect software.

> 
> Limiting the gpio-ranges definitions would prevent writing to reserved
> pinctrl registers via the userspace gpio API, which might be useful, but
> you would still be able to write to them via a custom devicetree (that
> uses non-existing pinctrl pins) and you would still be able to write to
> non-existent gpio registers via the userspace API.

Correct, the rockchip pinctrl and gpio drivers is in desperate need of a
big overhaul to fix this properly. And why I think it is better to avoid
adding incorrect gpio-ranges props. The DT is ABI and drivers must keep
backward compatibility, so adding incorrect props may make it even
harder to fix the underlying root issue in these drivers.

> 
> Given the above, do you still think it would be good to limit the
> gpio-ranges to the existing GPIOs only (I have no strong opinion)?

Yes, the DT should describe the HW, however the pinctrl driver report
too many pins so that probably need to be fixed in a backward compatible
way first.

Regards,
Jonas

> 
> Gr.
> 
> Matthijs

[PATCH RESEND 2/2] gpio: rockchip: Call pinctrl for gpio config

[Matthijs Kooijman]

Pinctrl is responsible for bias settings and possibly other pin config,
so call gpiochip_generic_config to apply such config values. This might
also include settings that pinctrl does not support, but then it can
return ENOTSUPP as appropriate.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 drivers/gpio/gpio-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 0fff4a699f12d..ac1b939283eb7 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -296,7 +296,7 @@ static int rockchip_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 		 */
 		return -ENOTSUPP;
 	default:
-		return -ENOTSUPP;
+		return gpiochip_generic_config(gc, offset, config);
 	}
 }
 
-- 
2.48.1

Re: [PATCH RESEND 2/2] gpio: rockchip: Call pinctrl for gpio config

[Linus Walleij]

On Mon, Mar 2, 2026 at 9:22 PM Matthijs Kooijman <matthijs@stdin.nl> wrote:

> Pinctrl is responsible for bias settings and possibly other pin config,
> so call gpiochip_generic_config to apply such config values. This might
> also include settings that pinctrl does not support, but then it can
> return ENOTSUPP as appropriate.
>
> Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

This patch 2/2 applied to the pinctrl tree.

Yours,
Linus Walleij

Re: [PATCH RESEND] rockchip: Make gpiod pin control work and add gpio-ranges

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]


> The second patch is just because I was messing with this code already
> and had a test setup ready. It makes gpio-ranges explicit for the rk3308
> instead of relying on the (possibly fragile) legacy workaround in
> gpio-rockchip to add them automatically. I tested this be removing the
> legacy workaround from the code during my testing.
>
> I think the first patch might be a good candidate to backport to the
> stable releases, since it makes a userspace interface functional that
> currently silently fails. However, it is not a clear bugfix and I am not
> super familiar with the rules for -stable, so I left out any stable Cc
> tags. Feel free to add them if this seems appropriate.

W00ps, seems the patches got reversed, so swap "first" and "second" when
reading the above paragraphs :-)

Gr.

Matthijs

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]