[PATCH v5 1/1] dt-bindings: pinctrl: Update pinctrl-single to use yaml

[Krzysztof Kozlowski]

On 23/05/2023 11:20, Tony Lindgren wrote:
> Update binding for yaml and remove the old related txt bindings. Note that
> we are also adding the undocumented pinctrl-single,slew-rate property. And
> we only use the first example from the old binding.
> 
> Cc: Linus Walleij <linus.walleij at linaro.org>
> Cc: Nishanth Menon <nm at ti.com>
> Cc: Vignesh Raghavendra <vigneshr at ti.com>
> Signed-off-by: Tony Lindgren <tony at atomide.com>


Thank you for your patch. There is something to discuss/improve.


> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml
> @@ -0,0 +1,201 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/pinctrl-single.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: One-register-per-pin type device tree based pinctrl driver

Drop "device tree based pinctrl driver" and describe the hardware.

> +
> +maintainers:
> +  - Tony Lindgren <tony at atomide.com>
> +
> +description:
> +  This binding describes pinctrl devices that use one hardware register to
> +  configure each pin.

Drop "This binding describes" and just say what is the hardware here.

> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:

Drop items, you have just an enum.

> +          - enum:
> +              - pinctrl-single
> +              - pinconf-single
> +      - items:
> +          - enum:
> +              - ti,am437-padconf
> +              - ti,dra7-padconf
> +              - ti,omap2420-padconf
> +              - ti,omap2430-padconf
> +              - ti,omap3-padconf
> +              - ti,omap4-padconf
> +              - ti,omap5-padconf
> +          - const: pinctrl-single
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  '#pinctrl-cells':
> +    enum: [ 1, 2 ]

Describe in description what are the arguments. Old binding had it.

> +
> +  pinctrl-single,bit-per-mux:
> +    description: Optional flag to indicate register controls more than one pin
> +    type: boolean
> +
> +  pinctrl-single,function-mask:
> +    description: Mask of the allowed register bits
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  pinctrl-single,function-off:
> +    description: Optional function off mode for disabled state
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  pinctrl-single,register-width:
> +    description: Width of pin specific bits in the register
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 8, 16, 32 ]
> +
> +  pinctrl-single,gpio-range:
> +    description: Optional list of pin base, nr pins & gpio function
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle of a gpio-range node
> +          - description: pin base
> +          - description: number of pins
> +          - description: gpio function
> +
> +  '#gpio-range-cells':
> +    description: No longer needed, may exist in older files for gpio-ranges
> +    deprecated: true
> +    const: 3
> +
> +  gpio-range:
> +    description: Optional node for gpio range cells
> +    type: object

On this level of indentation:
additionalProperties: false

> +    properties:
> +      '#pinctrl-single,gpio-range-cells':
> +        description: Number of gpio range cells
> +        const: 3
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
> +patternProperties:
> +  '-pins((.*)?)$|-pin':

Why do you need outer ()?
-pin$

> +    description: Pin group node name using pins or pin naming
> +    type: object
> +    $ref: pinmux-node.yaml#

You don't use anything from this ref. Drop it, unless you plan to
deprecate old properties and use generic from pinmux-node.

> +
> +    additionalProperties: false
> +
> +    properties:
> +      pinctrl-single,pins:
> +        description:
> +          Array of pins as described in pinmux-node.yaml for pinctrl-pin-array
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> +      pinctrl-single,bits:
> +        description: Register bit configuration for pinctrl-single,bit-per-mux
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - description: register offset
> +          - description: value
> +          - description: pin bitmask in the register
> +
> +      pinctrl-single,bias-pullup:
> +        description: Optional bias pull up configuration
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - description: input
> +          - description: enabled pull up bits
> +          - description: disabled pull up bits
> +          - description: bias pull up mask
> +
> +      pinctrl-single,bias-pulldown:
> +        description: Optional bias pull down configuration
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - description: input
> +          - description: enabled pull down bits
> +          - description: disabled pull down bits
> +          - description: bias pull down mask
> +
> +      pinctrl-single,drive-strength:
> +        description: Optional drive strength configuration
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - description: drive strength current
> +          - description: drive strength mask
> +
> +      pinctrl-single,input-schmitt:
> +        description: Optional input schmitt configuration
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - description: input
> +          - description: enable bits
> +          - description: disable bits
> +          - description: input schmitt mask
> +
> +      pinctrl-single,low-power-mode:
> +        description: Optional low power mode configuration
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - description: low power mode value
> +          - description: low power mode mask
> +
> +      pinctrl-single,slew-rate:
> +        description: Optional slew rate configuration
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - description: slew rate
> +          - description: slew rate mask
> +
> +allOf:
> +  - $ref: pinctrl.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - pinctrl-single,register-width
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +        pinmux at 4a100040 {

Mixed up indentation.

> +          compatible = "pinctrl-single";
> +          reg = <0x4a100040 0x0196>;
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          #pinctrl-cells = <2>;
> +          #interrupt-cells = <1>;
> +          interrupt-controller;
> +          pinctrl-single,register-width = <16>;
> +          pinctrl-single,function-mask = <0xffff>;
> +          pinctrl-single,gpio-range = <&range 0 3 0>;
> +          range: gpio-range {
> +            #pinctrl-single,gpio-range-cells = <3>;
> +          };
> +
> +          uart2-pins {


Best regards,
Krzysztof