Re: [PATCH 1/3] dt-bindings: pinctrl: rt2880: add binding document

[Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>]

Hi Linus,

Thanks for the review. There weren't too many yaml samples for this so
as you had seen this was a bit messy and I really needed this review,
especially in the 'if' clause part :).

On Mon, Dec 7, 2020 at 11:42 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> Hi Sergio,
>
> thanks for driving this!
>
> On Mon, Dec 7, 2020 at 8:21 PM Sergio Paracuellos
> <sergio.paracuellos@xxxxxxxxx> wrote:
>
> > The commit adds rt2880 compatible node in binding document.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> (...)
> > +description:
> > +  The rt2880 pinmux can only set the muxing of pin groups. muxing indiviual pins
> > +  is not supported. There is no pinconf support.
>
> OK!
>
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ralink,rt2880-pinmux
> > +
> > +  pinctrl-0:
> > +    description:
> > +      A phandle to the node containing the subnodes containing default
> > +      configurations.
>
> As it is a node on the pin controller itself, this is a hog so write something
> about that this is for pinctrl hogs.

Ok, will do.

>
> > +  pinctrl-names:
> > +    description:
> > +      A pinctrl state named "default" must be defined.
> > +    const: default
>
> Is it really compulsory?

Not really, I guess. The current device tree contains one so I added
here because of this.
>
> > +required:
> > +  - compatible
> > +  - pinctrl-names
> > +  - pinctrl-0
>
> I wonder if the hogs are really compulsory.

Ok, so I guess I should remove both 'pinctrl-names' and ' pinctrl-0'
from the required but maintain its desciption.

>
> > +patternProperties:
> > +  '^.*$':
>
> That's liberal node naming!
> What about [a-z0-9_-]+ or something?

hahaha. Yeah, I like freedom :), but yes, you are right, I will change
the pattern using the one proposed here.

>
> > +    if:
> > +      type: object
> > +      description: |
> > +        A pinctrl node should contain at least one subnodes representing the
> > +        pinctrl groups available on the machine.
> > +      $ref: "pinmux-node.yaml"
> > +      required:
> > +        - groups
> > +        - function
> > +      additionalProperties: false
> > +    then:
> > +      properties:
> > +        groups:
> > +          description:
> > +            Name of the pin group to use for the functions.
> > +          $ref: "/schemas/types.yaml#/definitions/string"
> > +          enum: [i2c, spi, uart1, uart2, uart3, rgmii1, rgmii2, mdio,
> > +                 pcie, sdhci]
> > +        function:
> > +          description:
> > +            The mux function to select
> > +          $ref: "/schemas/types.yaml#/definitions/string"
> > +          enum: [gpio, i2c, spi, uart1, uart2, uart3, rgmii1, rgmii2,
> > +                 mdio, nand1, nand2, sdhci]
>
> Why do we have this complex if: clause?

To be honest to avoid problems with other pinctrl root nodes because
they are not type 'object' and not having real idea in what way this
should be achieved :).

> $ref: "pinmux-node.yaml" should bring in the groups and
> function properties. Then you can add some further restrictions
> on top of that, right?
>
> I would just do:
>
> patternProperties:
>   '^[a-z0-9_]+$':
>     type: object
>       description: node for pinctrl
>       $ref "pinmux-node.yaml"
>
>       properties:
>         groups:
>           description: groups...
>           enum: [i2c, spi, uart1, uart2, uart3, rgmii1, rgmii2, mdio,
> pcie, sdhci]
>         function:
>           description: function...
>           enum: [gpio, i2c, spi, uart1, uart2, uart3, rgmii1, rgmii2,
> mdio, nand1, nand2, sdhci]
>
> Note: the function names are fine but the group names are a bit
> confusion since often a group can be used for more than one
> function, and e.g.
>
> function = "i2c";
> group = "uart1";
>
> to use the uart1 pins for an i2c is gonna look mildly confusing.
>
> But if this is what the hardware calls it I suppose it is
> fine.

This is the way is currently being used in the device tree.

>
> Yours,
> Linus Walleij

Thanks again. I will change this and send v2.

Best regards,
     Sergio Paracuellos