Re: [PATCH] arm64: dts: mediatek: mt8189: Add pinmux macro header file

[Chen-Yu Tsai]

On Tue, Feb 10, 2026 at 7:03 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
>
> Il 09/02/26 22:48, David Lechner ha scritto:
> > On 9/18/25 9:03 PM, Cathy Xu wrote:
> >> Add the pinctrl header file on MediaTek mt8189.
> >>
> >> Signed-off-by: Cathy Xu <ot_cathy.xu@xxxxxxxxxxxx>
> >> ---
> >> This patch is base on the patch series:
> >> https://patchwork.kernel.org/project/linux-mediatek/list/?series=981475
> >> [1] dt-bindings: pinctrl: mediatek: Add support for mt8189
> >> [2] arm64: dts: mediatek: mt8189: Add pinmux macro header file
> >> [3] pinctrl: mediatek: Add pinctrl driver on mt8189
> >> Since patch [1] and [3] of the series have already been merged, this
> >> patch(patch [2]) is being resent individually after modifications.
> >> ---
> >>   arch/arm64/boot/dts/mediatek/mt8189-pinfunc.h | 1125 +++++++++++++++++
> >>   1 file changed, 1125 insertions(+)
> >>   create mode 100644 arch/arm64/boot/dts/mediatek/mt8189-pinfunc.h
> >>
> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8189-pinfunc.h b/arch/arm64/boot/dts/mediatek/mt8189-pinfunc.h
> >> new file mode 100644
> >> index 000000000000..df69f50c267a
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/mediatek/mt8189-pinfunc.h
> >
> > General question:
> >
> > Why do we have similar files in two different places different places?
> >
> > $ ls arch/arm64/boot/dts/mediatek/*-pin*
> > arch/arm64/boot/dts/mediatek/mt2712-pinfunc.h
> > arch/arm64/boot/dts/mediatek/mt6878-pinfunc.h
> > arch/arm64/boot/dts/mediatek/mt6893-pinfunc.h
> > arch/arm64/boot/dts/mediatek/mt8167-pinfunc.h
> > arch/arm64/boot/dts/mediatek/mt8173-pinfunc.h
> > arch/arm64/boot/dts/mediatek/mt8196-pinfunc.h
> > arch/arm64/boot/dts/mediatek/mt8516-pinfunc.h
> >
> > $ ls include/dt-bindings/pinctrl/mt*
> > include/dt-bindings/pinctrl/mt65xx.h
> > include/dt-bindings/pinctrl/mt6779-pinfunc.h
> > include/dt-bindings/pinctrl/mt6795-pinfunc.h
> > include/dt-bindings/pinctrl/mt6797-pinfunc.h
> > include/dt-bindings/pinctrl/mt7623-pinfunc.h
> > include/dt-bindings/pinctrl/mt8135-pinfunc.h
> > include/dt-bindings/pinctrl/mt8183-pinfunc.h
> > include/dt-bindings/pinctrl/mt8186-pinfunc.h
> > include/dt-bindings/pinctrl/mt8192-pinfunc.h
> > include/dt-bindings/pinctrl/mt8195-pinfunc.h
> > include/dt-bindings/pinctrl/mt8365-pinfunc.h
> >
> >
> > Plus one different naming pattern.
> >
> > $ ls include/dt-bindings/pinctrl/mediatek,*
> > include/dt-bindings/pinctrl/mediatek,mt8188-pinfunc.h
> >
> >
> >
> > Which one is preferred?
> >
> >
> The MediaTek pinctrl must gain compatibility with standard pinctrl bindings. Until
> then, bindings maintainers decided that these headers must go to the dts/mediatek
> folder.
>
> It is my desire to (but lack of time on my side hits hard) do the right thing and
> make the MediaTek pinctrl drivers to actually "understand" standard bindings.

The headers encode the pin numbers and mux values in a way that the
"pinmux" property requires, all the while giving them meaningful names.

I suppose you could consider them part of the binding, as the pin controller
binding assembles all the individual PIO blocks in the SoC to produce one
unified view of all the pins. How they are ordered is important.

Plus the datasheets are horrible to read, as the pins aren't always numbered,
but are referred to using symbolic names like I2S2_MCLK.

> I'd be - of course - happy if anyone else beats me on time (which wouldn't be hard
> really) and pushes a series to fix this situation.
>
> Just to be clear - right now, the MTK pinctrl DT looks like:
>
>         panel_default_pins: panel-default-pins {
>                 pins-rst {
>                         pinmux = <PINMUX_GPIO108__FUNC_GPIO108>;
>                         output-high;
>                 };
>
>                 pins-en {
>                         pinmux = <PINMUX_GPIO48__FUNC_GPIO48>;
>                         output-low;
>                 };
>         };
>
>         spi1_pins: spi1-pins {
>                 pins {
>                         pinmux = <PINMUX_GPIO136__FUNC_SPIM1_CSB>,
>                                  <PINMUX_GPIO137__FUNC_SPIM1_CLK>,
>                                  <PINMUX_GPIO138__FUNC_SPIM1_MO>,
>                                  <PINMUX_GPIO139__FUNC_SPIM1_MI>;
>                         bias-disable;
>                 };
>         };

To be fair, the above is one valid kind of generic pinmux description.

>From Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml :

While not required to be used, there are 3 generic forms of pin muxing nodes
which pin controller devices can use.

For hardware where pin multiplexing configurations have to be specified for
each single pin the number of required sub-nodes containing "pin" and
"function" properties can quickly escalate and become hard to write and
maintain.

For cases like this, the pin controller driver may use the pinmux helper
property, where the pin identifier is provided with mux configuration settings
in a pinmux group. A pinmux group consists of the pin identifier and mux
settings represented as a single integer or an array of integers.

The pinmux property accepts an array of pinmux groups, each of them describing
a single pin multiplexing configuration.

 - end quote -

So Mediatek is following one of the generic pinmux bindings. It's not the
only one using this scheme either. STM32 and some of the Renesas platforms
also follow it.

> ....but the driver should gain compatibility with nodes which would look like:
>
>         panel_default_pins: panel-default-pins {
>                 pins-rst {
>                         pins = "gpio108";
>                         function = "gpio";
>                         output-high;
>                 };
>
>                 pins-en {
>                         pins = "gpio48";
>                         function = "gpio";
>                         output-low;
>                 };
>         };
>
>         spi1_pins: spi1-pins {
>                 pins-bus {
>                         pins = "gpio136", "gpio137", "gpio138", "gpio139",
>                         function = "spi_m1";

Why is it "spi_m1", not "spi1"?


Honestly you likely don't want this, or rather you don't want a huge table
of pins and pinmux values and strings in the kernel. It takes a lot of time
to write, even more time to review, and takes up a lot of space for each
pinctrl driver. And those are generally built-in.

The Allwinner platform has gone in the reverse direction: instead of having
a huge table, we put the mux value in the DT using a custom property.
See the following for discussions:

https://patchwork.ozlabs.org/project/linux-gpio/cover/20171113012523.2328-1-andre.przywara@xxxxxxx/
https://patchwork.ozlabs.org/project/linux-gpio/patch/20171113012523.2328-2-andre.przywara@xxxxxxx/

And this is what finally landed:

https://lore.kernel.org/linux-gpio/20250306235827.4895-7-andre.przywara@xxxxxxx/

Has it caused a bit of trouble? Perhaps. I was working on various peripherals
on a new board and put in the wrong mux value and didn't notice for a couple
days.



ChenYu

>                         bias-disable;
>                 };
>         };
>
> .... or
>
>         spi1_pins: spi1-pins {
>                 pins-bus {
>                         function = "spi_m1";
>                         groups = "spi_m1_pins";
>                         bias-disable;
>                 };
>         };
>
> That's the entire situation.
>
> Cheers,
> Angelo
>