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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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.

> +  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