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

Changhuang Liang
2026-04-28T01:43:33Z

Hi, Conor

Thanks for the review.

> 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>
>
> There's a lot of binding here, and I think a bunch of them have similar
> questions to be answered, so I am just going to review this one for now.
>
> > ---
> > .../pinctrl/starfive,jhb100-per1-pinctrl.yaml | 217
> > ++++++++++++++++++
> > 1 file changed, 217 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/pinctrl/starfive,jhb100-per1-pinctrl
> > .yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-per1-pinct
> > rl.yaml
> > b/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-per1-pinct
> > rl.yaml
> > new file mode 100644
> > index 000000000000..b2af4df874df
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-per1-p
> > +++ inctrl.yaml
> > @@ -0,0 +1,217 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> > +http://devicetree.org/schemas/pinctrl/starfive,jhb100-per1-pinctrl.ya
> > +ml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: StarFive JHB100 Peripheral-1 Pin Controller
> > +
> > +description: |
> > + Pinctrl bindings for JHB100 RISC-V SoC from StarFive Technology Ltd.
> > +
> > + The JHB100 SoC has 13 pinctrl domains - sys0, sys0h, sys1, sys2,
> > + per0, per1, per2, per2pok, per3, adc0, adc1, emmc, and vga.
> > + This document provides an overview of the "per1" pinctrl domain.
> > +
> > + The "per1" domain has a pin controller which provides
> > + - function selection for GPIO pads.
> > + - GPIO pad configuration.
> > + - GPIO interrupt handling.
> > +
> > + In the Peripheral-1 Pin Controller, there are 36 multi-function
> > + GPIO_PADs. Each of them can be multiplexed to several peripherals
> > + through function selection. Each iopad has a maximum of up to 3
> > + functions - 0, 1, and 2. Function 0 is the default function which is
> > + generally the GPIO function. Function 1 and 2 are the alternate
> > + functions or peripheral signals that can be routed to the iopad. The
> function selection can be carried out by writing the function number to the
> iopad function select register.
> > + Each iopad is configurable with parameters such as input-enable,
> > + internal pull-up/pull-down bias, drive strength, schmitt trigger, slew rate,
> and debounce width.
> > +
> > + This domain contains 4 IO groups which support voltage levels 1.8V
> > + and 3.3V gpioe-spi - comprises PAD_GPIO_C0 through PAD_GPIO_C4.
> > + gpioe-qspi0 - comprises PAD_GPIO_C5 through PAD_GPIO_C11.
> > + gpioe-qspi1 - comprises PAD_GPIO_C12 through PAD_GPIO_C19.
> > + gpioe-qspi2 - comprises PAD_GPIO_C20 through PAD_GPIO_C27.
> > +
> > + Each of the above IO groups must be configured with a voltage
> > + setting that matches the external voltage level provided to the IO group.
> > +
> > +maintainers:
> > + - Alex Soo <yuklin.soo@starfivetech.com>
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - const: starfive,jhb100-per1-pinctrl
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + clocks:
> > + maxItems: 1
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + interrupt-controller: true
> > +
> > + '#interrupt-cells':
> > + const: 2
> > +
> > + gpio-controller: true
> > +
> > + '#gpio-cells':
> > + const: 2
> > +
> > + gpio-ranges:
> > + maxItems: 1
> > +
> > + gpio-line-names: true
> > +
> > + gpioe-spi-vref:
>
> Why are these custom properties required?
> This sounds like the sort of information that could be gleaned from the
> "power-source" property.

The voltage configuration here applies to multiple pin groups, making it less suitable to use power-source.
In patch 13, we defined a starfive,gmac-vsel property, which can be applied to individual pins. It might be
more appropriate to use power-source.

>
> > + description: |
> > + Voltage reference value for the IO group "gpioe-spi"
> > + 0: voltage reference value for 3.3V
> > + 2: voltage reference value for 1.8V
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 2]
> > + default: 0
> > +
> > + gpioe-qspi0-vref:
> > + description: |
> > + Voltage reference value for the IO group "gpioe-qspi0"
> > + 0: voltage reference value for 3.3V
> > + 2: voltage reference value for 1.8V
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 2]
> > + default: 0
> > +
> > + gpioe-qspi1-vref:
> > + description: |
> > + Voltage reference value for the IO group "gpioe-qspi1"
> > + 0: voltage reference value for 3.3V
> > + 2: voltage reference value for 1.8V
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 2]
> > + default: 0
> > +
> > + gpioe-qspi2-vref:
> > + description: |
> > + Voltage reference value for the IO group "gpioe-qspi2"
> > + 0: voltage reference value for 3.3V
> > + 2: voltage reference value for 1.8V
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 2]
> > + default: 0
> > +
> > +patternProperties:
> > + '-grp$':
> > + type: object
> > + additionalProperties: false
> > + patternProperties:
> > + '-pins$':
> > + type: object
> > + description: |
> > + A pinctrl node should contain at least one subnode
> representing the
> > + pinctrl groups available in the domain. Each subnode will list
> the
> > + pins it needs, and how they should be configured, with regard
> to
> > + function selection, bias, input enable/disable, input schmitt
> > + trigger enable/disable, slew-rate and drive strength.
> > + allOf:
> > + - $ref: /schemas/pinctrl/pincfg-node.yaml
> > + - $ref: /schemas/pinctrl/pinmux-node.yaml
> > + unevaluatedProperties: false
> > +
> > + 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.

> > +
> > + bias-disable: true
> > +
> > + bias-pull-up:
> > + type: boolean
> > +
> > + bias-pull-down:
> > + type: boolean
> > +
> > + drive-strength:
> > + enum: [ 2, 4, 8, 12 ]
> > +
> > + drive-strength-microamp:
> > + enum: [ 2000, 4000, 8000, 12000 ]
> > +
> > + input-enable: true
> > +
> > + input-disable: true
> > +
> > + input-schmitt-enable: true
> > +
> > + input-schmitt-disable: true
> > +
> > + slew-rate:
> > + enum: [ 0, 1 ]
> > + default: 0
> > + description: |
> > + 0: slow (half frequency)
> > + 1: fast
> > +
> > + starfive,debounce-width:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + default: 0
> > + description:
> > + Debounce width 0 = Disabled, Others = 80ns*N stages
>
> This sounds like it should be called "debounce-stages".
>
> > +
> > + starfive,drive-i2c-fast-mode:
> > + type: boolean
> > + description:
> > + Enable I2C fast mode drive
> > +
> > + starfive,drive-i2c-fast-mode-plus:
> > + type: boolean
> > + description:
> > + Enable I2C fast mode plus drive
> > +
> > + starfive,i2c-open-drain-pull-up-ohm:
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> The unit of resistance is "ohms" in dt-schema, if you swap to that you won't
> need the $ref.
>
> > + description:
> > + open drain pull-up select
> > + enum: [600, 900, 1200, 2000]
> > + default: 600
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - resets
> > + - interrupts
> > + - interrupt-controller
> > + - '#interrupt-cells'
> > + - gpio-controller
> > + - '#gpio-cells'
> > + - gpio-ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + soc {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + pinctrl_per1: pinctrl@11b42000 {
>
> Drop the label here, since there's no users.

This label is referenced in gpio-ranges.

>
> Cheers,
> Conor.
>
> > + compatible = "starfive,jhb100-per1-pinctrl";
> > + reg = <0x0 0x11b42000 0x0 0x800>;
> > + resets = <&per1crg 0>;
> > + interrupts = <61>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + gpio-ranges = <&pinctrl_per1 0 0 36>;
> > + };
> > + };
> > --
> > 2.25.1
> >

Best Regards,
Changhuang