| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGb2v64CarfN5_pedrLDDic4AE1NbTKiB=tS12p9RyTW3h8rGg@mail.gmail.com>
Date: Wed, 11 Feb 2026 00:21:41 +0800
From: Chen-Yu Tsai <wens@...nel.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Krzysztof Kozlowski <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, David Lechner <david@...hnology.com>,
Cathy Xu <ot_cathy.xu@...iatek.com>, Matthias Brugger <matthias.bgg@...il.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
Yong Mao <yong.mao@...iatek.com>, Wenbin Mei <Wenbin.Mei@...iatek.com>,
Axe Yang <Axe.Yang@...iatek.com>, Lei Xue <Lei.Xue@...iatek.com>
Subject: Re: [PATCH] arm64: dts: mediatek: mt8189: Add pinmux macro header file
On Tue, Feb 10, 2026 at 10:26 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> wrote:
>
> Il 10/02/26 13:48, Chen-Yu Tsai ha scritto:
> > On Tue, Feb 10, 2026 at 7:03 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@...labora.com> 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@...iatek.com>
> >>>> ---
> >>>> 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.
> >
>
> Not saying that MediaTek is the only one that uses such bindings style, at all.
>
> I admit I was too tough about that, but as of the current state, the *binding*
> is not generic, and it's strictly tied to the GPIO Controller IP version of one
> specific SoC.
>
> While this style is generic, the actual pinmux *definitions* in the header are
> not generic - that's what I wanted to say, and I admit I went a bit too vague
> with words that are easy to misunderstand.
>
> >> ....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"?
> >
>
> PINMUX_GPIO138__FUNC_SPIM1_MO -> s/PINMUX_GPIO138__FUNC_//g/ and s/_MO//g
>
> M1 stands for "Master 1" - that's because technically there could be a different
> pinfunc for SPI "Slave 1" function.
>
> That's SoC-specific anyway, not all of them have SPIS1, not all of them need
> a different function, and... you get the point, I'm sure :-)
>
> >
> > 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@arm.com/
> > https://patchwork.ozlabs.org/project/linux-gpio/patch/20171113012523.2328-2-andre.przywara@arm.com/
> >
> > And this is what finally landed:
> >
> > https://lore.kernel.org/linux-gpio/20250306235827.4895-7-andre.przywara@arm.com/
> >
> > 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.
> >
>
> Then we must find a way to decouple hardware-specific information from the actual
> header I think?
By "hardware-specific information" I assume you mean how the different I/O
blocks are arranged and ordered to form a contiguous pin range?
We could take a look at the Rockchip design: IIRC it has a number of GPIO
controllers which handle the pin specific configs, but the pinmuxing is
done from a separate GRF (general register field) region. The GPIO
controllers' registers have the same layout, and the binding and driver
assume the same number of pins per block. Actual missing pins are just
skipped over.
I don't remember off the top of my head how the MediaTek hardware cuts
its set of controls across the hardware, but it might be similar. But
as I mentioned earlier, MediaTek doesn't number pins, and it certainly
doesn't split them into banks.
Hope that gives you some ideas to work with.
> Alternatively - that's what I have understood - and if I've understood that wrong,
> this needs clarification from the bindings maintainers, and why they wanted the
> MediaTek pinctrl bindings to get moved to arch/arm64/boot/dts/mediatek/ instead of
> include/dt-bindings/pinctrl/
>
> Bindings maintainers, any word on this?
>
> Did I misunderstand anything in past reviews ... from krzk if I remember correctly?
I think the reason was that they looked like macros for every pin/function
combination, and nothing more.
For any other SoC that had a more *rigid* PIO block where the numbering
is predictable, then yes, it would seem like helpful macros to make the
raw numbers more human readable.
ChenYu
> Cheers,
> Angelo
>
> >
> >
> > 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
> >>
>
>
Powered by blists - more mailing lists