Re: [PATCH v1 11/20] dt-bindings: pinctrl: Add starfive,jhb100-per1-pinctrl

Conor Dooley
2026-04-28T18:51:14Z

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

On Tue, Apr 28, 2026 at 01:28:05AM +0000, Changhuang Liang wrote:
> > On Fri, Apr 24, 2026 at 04:13:21AM -0700, Changhuang Liang wrote:
> > > Add pinctrl bindings for StarFive JHB100 SoC Peripheral-1(per1)
> > > pinctrl controller.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > + properties:
> > > + pinmux:
> > > + description: |
> > > + The list of GPIOs and their function select.
> > > + The PINMUX macros are used to configure the
> > > + function selection.
> >
> > Why is the pinmux property needed?
> > Can you use pins and function instead?
> >
> > Looking at the defines that you have added, it appears that lots of defines for
> > the same peripheral share the same numerical values, suggesting that across
> > peripheral, all (or most) pins would share the same mux setting/"function
> > select", suggesting that pins/function would suffice.
> >
> > I'd like to see some justification for pinmux being the right solution here, like
> > the "function select" used by one peripheral being significantly different for
> > many of its pins.
>
> We think that implementing this in the pinmux will be relatively simple. It avoids
> the need to create a large number of mapping relationships in the driver, which
> simplifies our driver implementation. I'm not sure if you'll find this explanation
> acceptable.

I don't really see how pins + functions would require lots of "mapping
relationships". Instead of having
+/* pinctrl_sys2 pad function selection */
+#define FUNC_SYS2_UART_CTS 1
+#define FUNC_SYS2_UART_RTS 1
+#define FUNC_SYS2_UART_DCD 1
+#define FUNC_SYS2_UART_DSR 1
+#define FUNC_SYS2_UART_DTR 1
+#define FUNC_SYS2_UART_RI 1
+#define FUNC_SYS2_UART0_TX 1
+#define FUNC_SYS2_UART0_RX 1
+#define FUNC_SYS2_UART1_TX 1
+#define FUNC_SYS2_UART1_RX 1
+#define FUNC_SYS2_UART2_TX 1
+#define FUNC_SYS2_UART2_RX 1
+#define FUNC_SYS2_UART3_TX 1
+#define FUNC_SYS2_UART3_RX 1
+#define FUNC_SYS2_UART4_TX 1
+#define FUNC_SYS2_UART4_RX 1
+#define FUNC_SYS2_UART5_TX 1
+#define FUNC_SYS2_UART5_RX 1
+#define FUNC_SYS2_UART6_TX 1
+#define FUNC_SYS2_UART6_RX 1
+#define FUNC_SYS2_UART7_TX 1
+#define FUNC_SYS2_UART7_RX 1
+#define FUNC_SYS2_UART8_TX 1
+#define FUNC_SYS2_UART8_RX 1
+#define FUNC_SYS2_UART9_TX 1
+#define FUNC_SYS2_UART9_RX 1
+#define FUNC_SYS2_UART10_TX 1
+#define FUNC_SYS2_UART10_RX 1
+#define FUNC_SYS2_UART11_TX 1
+#define FUNC_SYS2_UART11_RX 1
+#define FUNC_SYS2_UART12_TX 1
+#define FUNC_SYS2_UART12_RX 1
+#define FUNC_SYS2_UART13_TX 1
+#define FUNC_SYS2_UART13_RX 1
+#define FUNC_SYS2_UART14_TX 1
+#define FUNC_SYS2_UART14_RX 1
you just define a function called "uart" and have a simple map of
that string to the number 1. You end up with a single array with the
relationships, not lots.

Frankly, pinmux just does not seem appropriate to me when it looks like
90%+ of the pin mappings for a peripheral share the same function value.
There appears only to be a rare number of cases where that doesn't
apply, but that could be handled by having them represented by a
different group/pins node with a different function.

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