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

The commit adds rt2880 compatible node in binding document.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../pinctrl/ralink,rt2880-pinmux.yaml         | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml

Hi Sergio,

thanks for driving this!

On Mon, Dec 7, 2020 at 8:21 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:

> The commit adds rt2880 compatible node in binding document.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
(...)
> +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.

> +  pinctrl-names:
> +    description:
> +      A pinctrl state named "default" must be defined.
> +    const: default

Is it really compulsory?

> +required:
> +  - compatible
> +  - pinctrl-names
> +  - pinctrl-0

I wonder if the hogs are really compulsory.

> +patternProperties:
> +  '^.*$':

That's liberal node naming!
What about [a-z0-9_-]+ or something?

> +    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?
$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.

Yours,
Linus Walleij

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@linaro.org> wrote:
>
> Hi Sergio,
>
> thanks for driving this!
>
> On Mon, Dec 7, 2020 at 8:21 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
>
> > The commit adds rt2880 compatible node in binding document.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> (...)
> > +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