Re: [PATCH/RFC v2] dt-bindings: pinctrl: sh-pfc: Convert to json-schema

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Rob,

On Tue, Aug 18, 2020 at 1:32 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> On Fri, Aug 07, 2020 at 04:13:45PM +0200, Geert Uytterhoeven wrote:
> > Convert the Renesas Pin Function Controller (PFC) Device Tree binding
> > documentation to json-schema.
> >
> > Document missing properties.
> > Drop deprecated and obsolete #gpio-range-cells property.
> > Update the example to match reality.
> > Drop consumer examples, as they do not belong here.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > ---
> > Still RFC, due to the FIXMEs near the enum descriptions.
> > If I enable the enums checks, I get e.g.:
> >
> >     [[1800]] is not one of [1800, 3300]
> >
> > Note the double square brackets around 1800.
> > The usual error message doesn't have them, e.g.:
> >
> >     2000 is not one of [1800, 3300]
> >
> > So this looks like a bug in the tooling?
>
> Yes, we only recently started supporting schemas under
> 'additionalProperties', but failed to apply fixups.
>
> I have a fix I'm testing out. I'm bumping the version requirement in
> 5.10, so I'll make sure it is there.

Thanks, looking forward to it.

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc.yaml

> > +  interrupts-extended:
>
> Just use 'interrupts' here. 'interrupt-extended' is always magically
> supported.

Apparently not everywhere...

    Documentation/devicetree/bindings/pinctrl/renesas,pfc.example.dt.yaml:
pin-controller@e6050000: 'interrupts' is a required property

Hence I kept it in both places, for consistency.

Cfr. https://lore.kernel.org/linux-gpio/CAMuHMdWSPDQBABXZVaPecETbSRsP2yyZXLHiL_+_R2n_-09jRg@xxxxxxxxxxxxxx/

> > +additionalProperties:
> > +  anyOf:
> > +    - type: object
> > +      allOf:
> > +        - $ref: pincfg-node.yaml#
> > +        - $ref: pinmux-node.yaml#
> > +
> > +      description:
> > +        Pin controller client devices use pin configuration subnodes (children
> > +        and grandchildren) for desired pin configuration.
> > +        Client device subnodes use below standard properties.
> > +
> > +      properties:
> > +        phandle: true
>
> Once fixed, this won't be necessary.

OK.

> > +        function: true
> > +        groups: true
> > +        pins: true
> > +        bias-disable: true
> > +        bias-pull-down: true
> > +        bias-pull-up: true
> > +        drive-strength:
> > +          true # FIXME enum: [ 3, 6, 9, 12, 15, 18, 21, 24 ] # Superset of supported values
> > +          # avb:pins_mdio:drive-strength: [[24]] is not one of [3, 6, 9, 12, 15, 18, 21, 24]
> > +        power-source:
> > +          true # FIXME enum: [ 1800, 3300 ]
> > +          # sd0_uhs:power-source: [[1800]] is not one of [1800, 3300]
> > +        gpio-hog: true
> > +        gpios: true
> > +        input: true
> > +        output-high: true
> > +        output-low: true
> > +
> > +      additionalProperties: false
> > +
> > +    - type: object
> > +      properties:
> > +        phandle: true
>
> For this one, you can just link it back to the first entry:
>
> - type: object
>   additionalProperties:
>     $ref: "#/additionalProperties/anyOf/0"

Thanks, cool!

> > +examples:
> > +  - |
> > +    pfc: pin-controller@e6050000 {
>
> Because we're been really consistent ( :( ):
>
> pinctrl@...

Oh, I had never noticed that was added in devicetree-specification-v0.2.pdf.
Yes, consistency... everything else is *-controller ;-)

Will fix.

>
> > +            compatible = "renesas,pfc-sh73a0";
> > +            reg = <0xe6050000 0x8000>, <0xe605801c 0x1c>;
> > +            gpio-controller;
> > +            #gpio-cells = <2>;
> > +            gpio-ranges =
> > +                <&pfc 0 0 119>, <&pfc 128 128 37>, <&pfc 192 192 91>,
> > +                <&pfc 288 288 22>;
> > +            interrupts-extended =
> > +                <&irqpin0 0 0>, <&irqpin0 1 0>, <&irqpin0 2 0>, <&irqpin0 3 0>,
> > +                <&irqpin0 4 0>, <&irqpin0 5 0>, <&irqpin0 6 0>, <&irqpin0 7 0>,
> > +                <&irqpin1 0 0>, <&irqpin1 1 0>, <&irqpin1 2 0>, <&irqpin1 3 0>,
> > +                <&irqpin1 4 0>, <&irqpin1 5 0>, <&irqpin1 6 0>, <&irqpin1 7 0>,
> > +                <&irqpin2 0 0>, <&irqpin2 1 0>, <&irqpin2 2 0>, <&irqpin2 3 0>,
> > +                <&irqpin2 4 0>, <&irqpin2 5 0>, <&irqpin2 6 0>, <&irqpin2 7 0>,
> > +                <&irqpin3 0 0>, <&irqpin3 1 0>, <&irqpin3 2 0>, <&irqpin3 3 0>,
> > +                <&irqpin3 4 0>, <&irqpin3 5 0>, <&irqpin3 6 0>, <&irqpin3 7 0>;
> > +            power-domains = <&pd_c5>;
>
> child and grandchild nodes exercising the above schema and issues would
> be good here.

Good idea, will add.  I'll probably have to replace the example completely,
as SH73A0 doesn't support drive-strength and power-source.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds