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

[Conor Dooley <conor@kernel.org>]

[-- 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 --]