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
