Re: [PATCH v2 04/22] dt-bindings: pinctrl: Add starfive,jhb100-sys0-pinctrl

[Conor Dooley <conor.dooley@microchip.com>]

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

On Fri, May 15, 2026 at 06:10:28AM +0000, Changhuang Liang wrote:
> Hi, Conor
> 
> Thanks for the review.
> 
> > On Thu, May 14, 2026 at 04:12:00AM -0700, Changhuang Liang wrote:
> > > Add pinctrl bindings for StarFive JHB100 SoC System-0(sys0) pinctrl
> > > controller.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >  .../pinctrl/starfive,jhb100-sys0-pinctrl.yaml | 175 ++++++++++++++++++
> > >  .../pinctrl/starfive,jhb100-pinctrl.h         |  17 ++
> > >  2 files changed, 192 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pinctrl/starfive,jhb100-sys0-pinctrl
> > > .yaml  create mode 100644
> > > include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-sys0-pinct
> > > rl.yaml
> > > b/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-sys0-pinct
> > > rl.yaml
> > > new file mode 100644
> > > index 000000000000..21d3693587fd
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pinctrl/starfive,jhb100-sys0-p
> > > +++ inctrl.yaml
> > > @@ -0,0 +1,175 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > > +---
> > > +$id:
> > > +http://devicetree.org/schemas/pinctrl/starfive,jhb100-sys0-pinctrl.ya
> > > +ml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: StarFive JHB100 System-0 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 "sys0" pinctrl domain.
> > > +
> > > +  The "sys0" domain has a pin controller which provides
> > > +  - function selection for GPIO pads.
> > > +  - GPIO pad configuration.
> > > +  - GPIO interrupt handling.
> > > +
> > > +  In the SYS0 Pin Controller, there are 4 multi-function GPIO_PADs.
> > > + Each of  them can be multiplexed to different hardware blocks
> > > + through function  selection and each iopad has a maximum of up to 2
> > functions - 0 and 1.
> > > +  Function 0 is the default function which is generally the GPIO
> > > + function  (or occasionally, it can be a peripheral signal).
> > > +  Function 1 is the alternate function or peripheral signal that can
> > > + be  routed to the iopad. The function selection is 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,  input  debounce nanoseconds, power source and drive type
> > (open-drain or push-pull).
> > > +
> > > +maintainers:
> > > +  - Alex Soo <yuklin.soo@starfivetech.com>
> > 
> > Why is Alex the maintainer when you are the sole author?
> > 
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: starfive,jhb100-sys0-pinctrl
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-controller: true
> > > +
> > > +  '#interrupt-cells':
> > > +    const: 3
> > > +
> > > +  gpio-controller: true
> > > +
> > > +  '#gpio-cells':
> > > +    const: 3
> > > +
> > > +  gpio-ranges: true
> > > +
> > > +  gpio-line-names: true
> > > +
> > > +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, input debounce
> > nanoseconds,
> > > +          drive-open-drain, drive-push-pull, power-source and
> > drive-strength.
> > > +        allOf:
> > > +          - $ref: /schemas/pinctrl/pincfg-node.yaml
> > > +          - $ref: /schemas/pinctrl/pinmux-node.yaml
> > > +        unevaluatedProperties: false
> > 
> > I think this should be additionalProperties, since you're citing all the
> > properties you do support below.
> > 
> > > +
> > > +        properties:
> > > +          pins:
> > > +            description:
> > > +              The list of IOs that properties in the pincfg node apply to.
> > > +
> > > +          function:
> > > +            description:
> > > +              A string containing the name of the function to mux for
> > these
> > > +              pins.
> > > +            enum: [ auxpwrgood, gpio, hbled, pe2rst_out ]
> > > +
> > > +          bias-disable: true
> > > +
> > > +          bias-pull-down:
> > > +            type: boolean
> > > +
> > > +          bias-pull-up:
> > > +            oneOf:
> > > +              - type: boolean
> > > +              - enum: [ 600, 900, 1200, 2000 ]
> > > +                description: Pull up RSEL type resistance values (in
> > ohms)
> > > +            description:
> > > +              For normal pull up type there is no need to specify a
> > resistance
> > > +              value, hence this can be specified as a boolean property.
> > > +              For RSEL pull up type a resistance value (in ohms) can be
> > added.
> > > +
> > > +          drive-open-drain: true
> > > +
> > > +          drive-push-pull: true
> > > +
> > > +          drive-strength:
> > > +            enum: [ 2, 4, 8, 12 ]
> > > +
> > > +          drive-strength-microamp:
> > > +            enum: [ 2000, 4000, 8000, 12000 ]
> > > +
> > > +          input-debounce-nanoseconds:
> > > +            minimum: 0
> > > +            maximum: 4294967295
> > > +
> > > +          input-disable: true
> > > +
> > > +          input-enable: true
> > > +
> > > +          input-schmitt-enable: true
> > > +
> > > +          input-schmitt-disable: true
> > > +
> > > +          power-source:
> > > +             enum: [ 0, 1, 2 ]
> > > +
> > > +          slew-rate:
> > > +            enum: [ 0, 1 ]
> > > +            default: 0
> > > +            description: |
> > > +                0: slow (half frequency)
> > > +                1: fast
> > > +
> > > +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_sys0: pinctrl@13080000 {
> > > +            compatible = "starfive,jhb100-sys0-pinctrl";
> > > +            reg = <0x0 0x13080000 0x0 0x800>;
> > > +            resets = <&sys0crg 2>;
> > > +            interrupts = <56>;
> > > +            interrupt-controller;
> > > +            #interrupt-cells = <3>;
> > > +            gpio-controller;
> > > +            #gpio-cells = <3>;
> > > +            gpio-ranges = <&pinctrl_sys0 0 0 0 4>;
> > > +        };
> > > +    };
> > > diff --git a/include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> > > b/include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> > > new file mode 100644
> > > index 000000000000..6d8f5516a178
> > > --- /dev/null
> > > +++ b/include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> > > @@ -0,0 +1,17 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > > +/*
> > > + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> > > + *
> > > + * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > + */
> > > +
> > > +#ifndef __DT_BINDINGS_PINCTRL_STARFIVE_JHB100_H__
> > > +#define __DT_BINDINGS_PINCTRL_STARFIVE_JHB100_H__
> > > +
> > > +/* sys0 pad numbers */
> > > +#define PADNUM_SYS0_GPIO_A0				0
> > > +#define PADNUM_SYS0_GPIO_A1				1
> > > +#define PADNUM_SYS0_GPIO_A2				2
> > > +#define PADNUM_SYS0_GPIO_A3				3
> > 
> > Does this provide any actual value? Across the whole series, most numbers
> > you put in this binding headers do not appear in any drivers at all. Seems like
> > these defines should appear in the dts directly?
> 
> However, the current series of drivers will use some of these definitions.
> 
> Copied from patch 12:
> +static const struct pinvref_desc pinvref_desc_sys2[] = {
> +	{
> +		.name = "gpiow0",
> +		.pin_grp = {
> +			PADNUM_SYS2_GPIO_A36,
> +			PADNUM_SYS2_GPIO_A37,
> +			PADNUM_SYS2_GPIO_A38,
> +			PADNUM_SYS2_GPIO_A39
> +		},
> +		.num_pins = 4,
> +		.range = BIT(JHB100_PINVREF_1_8V) | BIT(JHB100_PINVREF_3_3V)

Can you explain why you need something like this when you're using pins
and functions? These look a lot like your own home-rolled groups, that
exist just to set the range of permitted values for vref?

As far as I can tell, the only controller that supports something other
than 1.8v and 3.3v is "per2", so you should be able to apply the
restrictions here entirely in the dt-binding?

I notice you have
> > > +          power-source:
> > > +             enum: [ 0, 1, 2 ]
in all bindings, but no explanation of what the values are. That needs
to change, and you should not permit 1 in anything other than "per2",
since that appears to be the only user.

Also, you've not responded to my other comments, one of which was a
question. Why not?

Thanks,
Conor.

> +	},
> +	{
> +		.name = "gpiow-inner",
> +		.pin_grp = {
> +			PADNUM_SYS2_GPIO_A40,
> +			PADNUM_SYS2_GPIO_A41,
> +			PADNUM_SYS2_GPIO_A42,
> +			PADNUM_SYS2_GPIO_A43
> +		},
> +		.num_pins = 4,
> +		.range = BIT(JHB100_PINVREF_1_8V) | BIT(JHB100_PINVREF_3_3V)
> +	},
> +	{ NULL },
> +};
> 
> Therefore, I have uniformly created include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> 
> Best Regards,
> Changhuang
> 

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