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

[Conor Dooley <conor@kernel.org>]

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

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-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-per1-pinctrl.yaml
> new file mode 100644
> index 000000000000..b2af4df874df
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-per1-pinctrl.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.yaml#
> +$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.

> +    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.

> +
> +          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.


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
> 

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