Re: [PATCH RFC v3 0/2] Modern pinctrl for Exynos5250 devices

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Lukas,

On 5/19/26 6:32 PM, Lukas Timmermann wrote:
> [You don't often get email from linux@timmermann.space. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Wed, May 06, 2026 at 03:14:11PM +0200, Quentin Schulz wrote:
>> Hi Lukas,
>>
>> On 5/6/26 1:13 AM, Lukas Timmermann wrote:
>>> This patch series adds the necessary files to use a modern pinctrl driver
>>> with an exynos5250 SoC. The changes are well tested and are working.
>>>
>>> In contrast to v1, this reenables GPIO functionality. But as I'm still
>>> lacking experience, I'm not quite sure on how to prevent s5p_gpio to
>>> load when using upstream DTS in a clean way. Because of that, this
>>> patch series is marked as an RFC.
>>> checkpatch complains about using #ifdef.
>>>
>>
>> We already have an issue with samsung,exynos78x0-gpio so we'd need a similar
>> work-around for that one.
>>
>> An easy way to sort this out would be to know if we know of devices which
>> are still using the old binding? Qualcomm is rather special, is Samsung
>> Exynos also special with regard to how it handles Device Trees? Do we get it
>> from an earlier boot stage or is everything coming from U-Boot itself? If
>> the latter, we can always migrate the existing devices to the new binding
>> and remove support for the old binding and with it the same compatible in
>> two drivers. Another question would be, does it make sense for both drivers
>> to be compiled? If not (e.g. it's only pinctrl IPs inside an SoC and there's
>> only one pinctrl device per SoC, so if your device supports the new binding,
>> you only need the new driver). Then we can add a
>> depends on !PINCTRL_EXYNOS5250
>> for CONFIG_S5P (we probably should have a different symbol for the GPIO
>> driver though, maybe CONFIG_GPIO_S5P?).
>>
>> Cheers,
>> Quentin
> 
> I'm sorry. Some of these Infos should've been in my original message:
> 
> This driver replaces the pinctrl functionality in the old s5p_gpio

I don't see how s5p_gpio is currently doing pinctrl. Do you rather mean 
that the s5p_gpio driver is matching the pinctrl compatible and thus we 
cannot have two drivers match the same compatible?

> driver. It doesn't replace GPIO functionality itself. So both drivers
> have to be compiled in order to work with modern upstream DTs. This
> driver binds the older s5p_gpio driver during its own binding.
> 
> The pinctrl dt-bindings are incompatible to each other. This driver fixes

Can you highlight how they are incompatible? I think the pinctrl part is 
identical?

arch/arm/dts/exynos5250.dtsi

/ {
[...]
         pinctrl_0: pinctrl@11400000 {
                 compatible = "samsung,exynos5250-pinctrl";
                 reg = <0x11400000 0x1000>;
                 interrupts = <0 46 0>;

                 wakup_eint: wakeup-interrupt-controller {
                         compatible = "samsung,exynos4210-wakeup-eint";
                         interrupt-parent = <&gic>;
                         interrupts = <0 32 0>;
                 };
         };

         pinctrl_1: pinctrl@13400000 {
                 compatible = "samsung,exynos5250-pinctrl";
                 reg = <0x13400000 0x1000>;
                 interrupts = <0 45 0>;
         };

         pinctrl_2: pinctrl@10d10000 {
                 compatible = "samsung,exynos5250-pinctrl";
                 reg = <0x10d10000 0x1000>;
                 interrupts = <0 50 0>;
         };

         pinctrl_3: pinctrl@03860000 {
                 compatible = "samsung,exynos5250-pinctrl";
                 reg = <0x03860000 0x1000>;
                 interrupts = <0 47 0>;
         };
[...]
};

+ arch/arm/dts/exynos5250-pinctrl.dtsi

/ {
         pinctrl@11400000 {
                 gpa0: gpa0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpa1: gpa1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpa2: gpa2 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpb0: gpb0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpb1: gpb1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpb2: gpb2 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpb3: gpb3 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpc0: gpc0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpc1: gpc1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpc2: gpc2 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpc3: gpc3 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpd0: gpd0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpd1: gpd1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpy0: gpy0 {
                         gpio-controller;
                         #gpio-cells = <2>;
                 };

                 gpy1: gpy1 {
                         gpio-controller;
                         #gpio-cells = <2>;
                 };

                 gpy2: gpy2 {
                         gpio-controller;
                         #gpio-cells = <2>;
                 };

                 gpy3: gpy3 {
                         gpio-controller;
                         #gpio-cells = <2>;
                 };

                 gpy4: gpy4 {
                         gpio-controller;
                         #gpio-cells = <2>;
                 };

                 gpy5: gpy5 {
                         gpio-controller;
                         #gpio-cells = <2>;
                 };

                 gpy6: gpy6 {
                         gpio-controller;
                         #gpio-cells = <2>;
                 };

                 gpc4: gpc4 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpx0: gpx0 {
                         gpio-controller;
                         #gpio-cells = <2>;

                         interrupt-controller;
                         interrupt-parent = <&combiner>;
                         #interrupt-cells = <2>;
                         interrupts = <23 0>, <24 0>, <25 0>, <25 1>,
                                      <26 0>, <26 1>, <27 0>, <27 1>;
                 };

                 gpx1: gpx1 {
                         gpio-controller;
                         #gpio-cells = <2>;

                         interrupt-controller;
                         interrupt-parent = <&combiner>;
                         #interrupt-cells = <2>;
                         interrupts = <28 0>, <28 1>, <29 0>, <29 1>,
                                      <30 0>, <30 1>, <31 0>, <31 1>;
                 };

                 gpx2: gpx2 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpx3: gpx3 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

         pinctrl@13400000 {
                 gpe0: gpe0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

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

                 gpe1: gpe1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpf0: gpf0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpf1: gpf1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpg0: gpg0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpg1: gpg1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpg2: gpg2 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gph0: gph0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gph1: gph1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

         };

         pinctrl@10d10000 {
                 gpv0: gpv0 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpv1: gpv1 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpv2: gpv2 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpv3: gpv3 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

                 gpv4: gpv4 {
                         gpio-controller;
                         #gpio-cells = <2>;

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

         };

         pinctrl@03860000 {
                 gpz: gpz {
                         gpio-controller;
                         #gpio-cells = <2>;

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

         };
};

vs dts/upstream/src/arm/samsung/exynos5250.dtsi

/{
[...]
                 pinctrl_0: pinctrl@11400000 {
                         compatible = "samsung,exynos5250-pinctrl";
                         reg = <0x11400000 0x1000>;
                         interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;

                         wakup_eint: wakeup-interrupt-controller {
                                 compatible = 
"samsung,exynos4210-wakeup-eint";
                                 interrupt-parent = <&gic>;
                                 interrupts = <GIC_SPI 32 
IRQ_TYPE_LEVEL_HIGH>;
                         };
                 };

                 pinctrl_1: pinctrl@13400000 {
                         compatible = "samsung,exynos5250-pinctrl";
                         reg = <0x13400000 0x1000>;
                         interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
                 };

                 pinctrl_2: pinctrl@10d10000 {
                         compatible = "samsung,exynos5250-pinctrl";
                         reg = <0x10d10000 0x1000>;
                         interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
                 };

                 pinctrl_3: pinctrl@3860000 {
                         compatible = "samsung,exynos5250-pinctrl";
                         reg = <0x03860000 0x1000>;
                         interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
                         power-domains = <&pd_mau>;
                 };

+ dts/upstream/src/arm/samsung/exynos5250-pinctrl.dtsi

&pinctrl_0 {
         gpa0: gpa0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpa1: gpa1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpa2: gpa2-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpb0: gpb0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpb1: gpb1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpb2: gpb2-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpb3: gpb3-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpc0: gpc0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpc1: gpc1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpc2: gpc2-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpc3: gpc3-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpd0: gpd0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpd1: gpd1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpy0: gpy0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;
         };

         gpy1: gpy1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;
         };

         gpy2: gpy2-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;
         };

         gpy3: gpy3-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;
         };

         gpy4: gpy4-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;
         };

         gpy5: gpy5-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;
         };

         gpy6: gpy6-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;
         };

         gpc4: gpc4-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpx0: gpx0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

                 interrupt-controller;
                 interrupt-parent = <&combiner>;
                 #interrupt-cells = <2>;
                 interrupts = <23 0>, <24 0>, <25 0>, <25 1>,
                              <26 0>, <26 1>, <27 0>, <27 1>;
         };

         gpx1: gpx1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

                 interrupt-controller;
                 interrupt-parent = <&combiner>;
                 #interrupt-cells = <2>;
                 interrupts = <28 0>, <28 1>, <29 0>, <29 1>,
                              <30 0>, <30 1>, <31 0>, <31 1>;
         };

         gpx2: gpx2-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpx3: gpx3-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

                 interrupt-controller;
                 #interrupt-cells = <2>;
         };
[...]
};

&pinctrl_1 {
         gpe0: gpe0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpe1: gpe1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpf0: gpf0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpf1: gpf1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpg0: gpg0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpg1: gpg1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpg2: gpg2-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gph0: gph0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>

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

         gph1: gph1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

                 interrupt-controller;
                 #interrupt-cells = <2>;
         };
[...]
};

&pinctrl_2 {
         gpv0: gpv0-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpv1: gpv1-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpv2: gpv2-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpv3: gpv3-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

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

         gpv4: gpv4-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

                 interrupt-controller;
                 #interrupt-cells = <2>;
         };
[...]
};

&pinctrl_3 {
         gpz: gpz-gpio-bank {
                 gpio-controller;
                 #gpio-cells = <2>;

                 interrupt-controller;
                 #interrupt-cells = <2>;
         };
[...]
};

I can see the interrupts property in the pinctrl@ node is 0 vs 4 
(IRQ_TYPE_LEVEL_HIGH) and the node name is slightly different (I don't 
think it matters).

The pinctrl nodes are also in the root (/) node in U-Boot, while in 
upstream DTS it's under /soc (don't think it matters provided something 
takes care of probing /soc and its children?).

The gpio nodes have an additional -gpio-bank suffix in upstream DTS, 
don't think it matters either.

There are additional pinconf/pinmux/pingroups (I believe?) nodes but 
those don't have a gpio-controller property so they shouldn't matter?

I must be missing something, can you tell me what?

> pinctrl with upstream DTs and the older one must be used on it's own
> when relying on U-Boots deprecated DTs.
> 
> DTs are provided by U-Boot itself, at least that's the case on my device
> (exynos5250-manta).
> 
> If I understood you correctly, we could modify the deprecated exynos5250.dtsi
> to fix this situation. But I don't know about possible side effects
> and I'm not that experienced in kernel/bootloader development. Is this an option?
> 
> I had split the gpio symbol into it's own thing before and could add that
> back into v4 but it's functionally the same. (Both drivers needed for
> upstream DT)
> 

No, I think it's fine to have pinctrl and GPIO split like you suggested 
here.

Can you explain what makes your pinctrl driver require the upstream 
Device Tree (the one in dts/) and cannot work with the U-Boot device 
tree (the one in arch/arm/dts)?

We have a problem if it's requiring an upstream Device Tree because your 
new driver doesn't enforce it (via a depends on OF_UPSTREAM in Kconfig), 
and we have one if it doesn't, because the ifdefery in s5p_gpio is then 
incorrect as you can compile with CONFIG_S5P AND 
CONFIG_PINCTRL_EXYNOS5250 with both trying to bind against the same 
compatible.

I think what you want in the second patch (provided I'm right the 
pinctrl driver can work with the U-Boot DTS) is instead:

#if CONFIG_IS_ENABLED(PINCTRL) && IS_ENABLED(CONFIG_PINCTRL_EXYNOS5250)
  	{ .compatible = "samsung,exynos5250-pinctrl" },
#endif

and add a nice comment around those lines to say that we do this because 
drivers/pinctrl/exynos/pinctrl-exynos5250.c binds against the same 
compatible and will also bind s5p_gpio against the same node.

Otherwise, my suggestion was to maybe remove the compatible entirely 
from s5p_gpio. For that, you'd need to make sure the new driver is 
compatible with the device tree they are using (I'm assuming U-Boot 
device tree and not upstream) and you'd need to enable CONFIG_PINCTRL 
(and possibly CONFIG_SPL/TPL_PINCTRL) + CONFIG_EXYNOS5250 for defconfigs 
where CONFIG_S5P is enabled (and in all xPL/proper phases, depending on 
which CONFIG_GPIO and CONFIG_SPL/TPL_GPIO is enabled).

Maybe there's also a path to migrate the four Exynos5250 defconfigs to 
use upstream Device Tree but one would need to compare the current 
U-Boot device tree we have and the upstream one (in dts/) and check that 
all drivers are compatible with the upstream device tree before 
switching them. Not a requirement for your series, but it'd be nice as a 
follow if this is something I can convince you into having a look :)

Cheers,
Quentin