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
