| Message ID | 20201207192104.6046-1-sergio.paracuellos@gmail.com |
|---|---|
| Headers | show |
| Series | pinctrl: ralink: pinctrl driver for the rt2880 family | expand |
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
This series adds a pinctrl driver for ralink rt2880 SoC. After last cleanup in staging I was told [0] this driver is ready to be promoted from staging. This series are rebased on the top of staging-testing. Thanks in advance for your time. Best regards, Sergio Paracuellos [0]: http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2020-December/149178.html Sergio Paracuellos (3): dt-bindings: pinctrl: rt2880: add binding document pinctrl: ralink: add a pinctrl driver for the rt2880 family staging: mt7621-pinctrl: remove driver from staging .../pinctrl/ralink,rt2880-pinmux.yaml | 82 +++++++++++++++++++ drivers/pinctrl/Kconfig | 6 ++ drivers/pinctrl/Makefile | 1 + .../pinctrl-rt2880.c | 0 drivers/staging/Kconfig | 2 - drivers/staging/Makefile | 1 - drivers/staging/mt7621-pinctrl/Kconfig | 6 -- drivers/staging/mt7621-pinctrl/Makefile | 4 - drivers/staging/mt7621-pinctrl/TODO | 6 -- 9 files changed, 89 insertions(+), 19 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml rename drivers/{staging/mt7621-pinctrl => pinctrl}/pinctrl-rt2880.c (100%) delete mode 100644 drivers/staging/mt7621-pinctrl/Kconfig delete mode 100644 drivers/staging/mt7621-pinctrl/Makefile delete mode 100644 drivers/staging/mt7621-pinctrl/TODO