Re: [PATCH v3 1/5] dt-bindings: pinctrl: Add support for Amlogic SoCs

[Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>]

Hi Krzysztof,
   Thanks for reply.

On 2025/1/17 16:48, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed, Jan 15, 2025 at 02:41:59PM +0800, Xianwei Zhao wrote:
> > +properties:
> > +  compatible:
> > +    const: amlogic,pinctrl-a4
> > +
> > +  "#address-cells":
> > +    const: 2
> > +
> > +  "#size-cells":
> > +    const: 2
>
> Why this moved to '2'? 32-bit was not enough?
Previously only represented a register address offset value, now 
directly represents a register, through 'ranges" which to represent. 
32-bit is enough, but since APB bus is defined as 64-bit, here is 
following. I increase support for 32-bit. Like the following values:
  "#address-cells":
    enum: [1, 2]

  "#size-cells":
    enum: [1, 2]

> > +
> > +  ranges: true
> > +
> > +required:
> > +  - compatible
> > +  - "#address-cells"
> > +  - "#size-cells"
>
> Keep required after patternProperties

Will do.

> > +
> > +patternProperties:
> > +  "^gpio@[0-9a-f]+$":
> > +    type: object
> > +
> > +    properties:
> > +      reg:
> > +        minItems: 1
> > +        items:
> > +          - description: pin config register
> > +          - description: pin mux setting register (some special pin fixed function)
> > +          - description: pin drive strength register (optionanl)
>
> typo: optional
Will fix.
> > +
> > +      reg-names:
> > +        minItems: 1
> > +        items:
> > +          - const: gpio
> > +          - const: mux
> > +          - const: ds
> > +
> > +      gpio-controller: true
> > +
> > +      "#gpio-cells":
> > +        const: 2
> > +
> > +      gpio-ranges:
> > +        maxItems: 1
> > +
> > +      bank-number:
> > +        description: |
> > +          bank-number are provided by the pin controller header file at:
> > +          <include/dt-bindings/pinctrl/amlogic,pinctrl.h>
> > +        $ref: /schemas/types.yaml#/definitions/uint32
>
> gpio-ranges tell you that, don't they?
Yes. Get it from gpio-ranges.

> Anyway, you would need here minimum and maximum.
Will drop bank-number property.
> > +
> > +    required:
> > +      - reg
> > +      - reg-names
> > +      - gpio-controller
> > +      - "#gpio-cells"
> > +      - gpio-ranges
> > +      - bank-number
> > +
> > +    additionalProperties: false
> > +
> > +  "^func-[0-9a-z-]+$":
> > +    type: object
> > +    patternProperties:
> > +      "^group-[0-9a-z-]+$":
> > +        type: object
> > +        allOf:
> > +          - $ref: /schemas/pinctrl/pincfg-node.yaml
> > +          - $ref: /schemas/pinctrl/pinmux-node.yaml
>
> Missing required pinmux and maybe other properties.

Will add required.

> > +    additionalProperties: false
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/pinctrl/amlogic,pinctrl.h>
> > +    apb {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +      periphs_pinctrl: pinctrl {
> > +        compatible = "amlogic,pinctrl-a4";
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +        ranges;
> > +
> > +        gpio@14 {
> > +          reg = <0 0x14 0 0x10>,
> > +                <0 0x14 0 0x10>;
>
> One line.

Will do.

> > +          reg-names = "gpio", "mux";
> > +          gpio-controller;
> > +          #gpio-cells = <2>;
> > +          bank-number = <AMLOGIC_GPIO_B>;
> > +          gpio-ranges = <&periphs_pinctrl 0 8 10>;
> > +        };
>
> Best regards,
> Krzysztof