| Message ID | 20190626071430.28556-3-andrew@aj.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: aspeed: Preparation for AST2600 | expand |
On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > Convert ASPEED pinctrl bindings to DT schema format using json-schema BTW, ASPEED is one of the remaining platforms needing the top-level board bindings converted. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 ------------------- > .../pinctrl/aspeed,ast2400-pinctrl.yaml | 73 +++++++++++++++++ > 2 files changed, 73 insertions(+), 80 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt > create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > new file mode 100644 > index 000000000000..3b8cf3e51506 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > @@ -0,0 +1,73 @@ > +# SPDX-License-Identifier: GPL-2.0+ Do you have rights to change the license? If so, the preference is to dual license with (GPL-2.0 OR BSD-2-Clause). BTW, '-or-later' is the preferred form over '+'. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ASPEED AST2400 Pin Controller > + > +maintainers: > + - Andrew Jeffery <andrew@aj.id.au> > + > +properties: > + compatible: > + oneOf: > + - items: > + - enum: > + - aspeed,ast2400-pinctrl > + - items: > + - enum: > + - aspeed,g4-pinctrl This can be simplified to: compatible: enum: - aspeed,ast2400-pinctrl - aspeed,g4-pinctrl > + > +required: > + - compatible > + > +description: |+ description goes before properties. > + The pin controller node should be the child of a syscon node with the > + required property: > + > + - compatible: Should be one of the following: > + "aspeed,ast2400-scu", "syscon", "simple-mfd" > + "aspeed,g4-scu", "syscon", "simple-mfd" > + > + Refer to the the bindings described in > + Documentation/devicetree/bindings/mfd/syscon.txt > + > + For the AST2400 pinmux, each mux function has only one associated pin group. > + Each group is named by its function. The following values for the function > + and groups properties are supported: > + > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 This should be a schema. You need to define child nodes and list these as values for 'function' and 'group'. Ideally, the child nodes would have some sort of pattern, but if not, you can just match on '^.*$' under patternProperties. BTW, You can put the names under a 'definitions' key and then use '$ref' to reference them from function and group to avoid duplicating the names. Or use patternProperties with '^(function|group)$'. Similar comments apply to AST2500 binding. Rob
On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote: > On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > Convert ASPEED pinctrl bindings to DT schema format using json-schema > > BTW, ASPEED is one of the remaining platforms needing the top-level > board bindings converted. Okay, I'll put together patches to fix that. > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 ------------------- > > .../pinctrl/aspeed,ast2400-pinctrl.yaml | 73 +++++++++++++++++ > > 2 files changed, 73 insertions(+), 80 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt > > create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > new file mode 100644 > > index 000000000000..3b8cf3e51506 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > @@ -0,0 +1,73 @@ > > +# SPDX-License-Identifier: GPL-2.0+ > > Do you have rights to change the license? Where are you coming from with this question? The bindings previously didn't list a license, is there some implicit license for them? I would have thought it was GPL-2.0? IBM's (my employer's) preferred contribution license is GPL 2.0-or-later, so I was just adding the SPDX marker to clarify. > If so, the preference is to > dual license with (GPL-2.0 OR BSD-2-Clause). You're asking if I have the power to relicense so I can dual license it this way? > > BTW, '-or-later' is the preferred form over '+'. Thanks for the pointer. > > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ASPEED AST2400 Pin Controller > > + > > +maintainers: > > + - Andrew Jeffery <andrew@aj.id.au> > > + > > +properties: > > + compatible: > > + oneOf: > > + - items: > > + - enum: > > + - aspeed,ast2400-pinctrl > > + - items: > > + - enum: > > + - aspeed,g4-pinctrl > > This can be simplified to: > > compatible: > enum: > - aspeed,ast2400-pinctrl > - aspeed,g4-pinctrl Ah, that makes more sense, I think I was thrown by some details of the example. > > > + > > +required: > > + - compatible > > + > > +description: |+ > > description goes before properties. Okay. I wouldn't have thought the ordering mattered. Is this just a preference? The tools seemed to run fine as is. I'll re-order it regardless. > > > + The pin controller node should be the child of a syscon node with the > > + required property: > > + > > + - compatible: Should be one of the following: > > + "aspeed,ast2400-scu", "syscon", "simple-mfd" > > + "aspeed,g4-scu", "syscon", "simple-mfd" > > + > > + Refer to the the bindings described in > > + Documentation/devicetree/bindings/mfd/syscon.txt > > + > > + For the AST2400 pinmux, each mux function has only one associated pin group. > > + Each group is named by its function. The following values for the function > > + and groups properties are supported: > > + > > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 > > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 > > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 > > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 > > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 > > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK > > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 > > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 > > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI > > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU > > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 > > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 > > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 > > This should be a schema. Yeah, I covered this in my cover letter. I was hoping to get away without that for the moment as this seems like the first pinctrl binding to be converted, however if you insist... > You need to define child nodes and list these > as values for 'function' and 'group'. Ideally, the child nodes would > have some sort of pattern, but if not, you can just match on '^.*$' > under patternProperties. > > BTW, You can put the names under a 'definitions' key and then use > '$ref' to reference them from function and group to avoid duplicating > the names. Or use patternProperties with '^(function|group)$'. Okay, I'll take some time to digest this while looking at the documentation. > > Similar comments apply to AST2500 binding. Yes, will fix that too. Thanks for the prompt review! Andrew
On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote: > On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > + The pin controller node should be the child of a syscon node with the > > + required property: > > + > > + - compatible: Should be one of the following: > > + "aspeed,ast2400-scu", "syscon", "simple-mfd" > > + "aspeed,g4-scu", "syscon", "simple-mfd" > > + > > + Refer to the the bindings described in > > + Documentation/devicetree/bindings/mfd/syscon.txt > > + > > + For the AST2400 pinmux, each mux function has only one associated pin group. > > + Each group is named by its function. The following values for the function > > + and groups properties are supported: > > + > > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 > > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 > > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 > > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 > > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 > > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK > > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 > > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 > > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI > > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU > > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 > > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 > > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 > > This should be a schema. You need to define child nodes and list these > as values for 'function' and 'group'. Ideally, the child nodes would > have some sort of pattern, but if not, you can just match on '^.*$' > under patternProperties. The children don't have any pattern in their node name, which drives me towards the '^.*$' pattern match, however, what I've found is that I get the following errors for some of the relevant dts files: ``` /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-names: ['default'] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-0: [[7, 8, 9, 10, 11, 12]] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: phandle: [[13]] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: $nodename: ['pinctrl'] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-names: ['default'] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-0: [[9, 10, 11, 12]] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: phandle: [[13]] is not of type 'object' /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: $nodename: ['pinctrl'] is not of type 'object' ``` We shouldn't be expecting these properties in the child nodes, so something is busted. Looking at processed-schema.yaml, we have: ``` - $filename: /home/andrew/src/linux/aspeed/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml $id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# patternProperties: ^.*$: patternProperties: ^function|groups$: allOf: - {$ref: /schemas/types.yaml#/definitions/string} - additionalItems: false items: enum: [ACPI, ADC0, ADC1, ADC10, ADC11, ADC12, ADC13, ADC14, ADC15, ADC2, ADC3, ADC4, ADC5, ADC6, ADC7, ADC8, ADC9, BMCINT, DDCCLK, DDCDAT, EXTRST, FLACK, FLBUSY, FLWP, GPID, GPID0, GPID2, GPID4, GPID6, GPIE0, GPIE2, GPIE4, GPIE6, I2C10, I2C11, I2C12, I2C13, I2C14, I2C3, I2C4, I2C5, I2C6, I2C7, I2C8, I2C9, LPCPD, LPCPME, LPCRST, LPCSMI, MAC1LINK, MAC2LINK, MDIO1, MDIO2, NCTS1, NCTS2, NCTS3, NCTS4, NDCD1, NDCD2, NDCD3, NDCD4, NDSR1, NDSR2, NDSR3, NDSR4, NDTR1, NDTR2, NDTR3, NDTR4, NDTS4, NRI1, NRI2, NRI3, NRI4, NRTS1, NRTS2, NRTS3, OSCCLK, PWM0, PWM1, PWM2, PWM3, PWM4, PWM5, PWM6, PWM7, RGMII1, RGMII2, RMII1, RMII2, ROM16, ROM8, ROMCS1, ROMCS2, ROMCS3, ROMCS4, RXD1, RXD2, RXD3, RXD4, SALT1, SALT2, SALT3, SALT4, SD1, SD2, SGPMCK, SGPMI, SGPMLD, SGPMO, SGPSCK, SGPSI0, SGPSI1, SGPSLD, SIOONCTRL, SIOPBI, SIOPBO, SIOPWREQ, SIOPWRGD, SIOS3, SIOS5, SIOSCI, SPI1, SPI1DEBUG, SPI1PASSTHRU, SPICS1, TIMER3, TIMER4, TIMER5, TIMER6, TIMER7, TIMER8, TXD1, TXD2, TXD3, TXD4, UART6, USB11D1, USB11H2, USB2D1, USB2H1, USBCKI, VGABIOS_ROM, VGAHS, VGAVS, VPI18, VPI24, VPI30, VPO12, VPO24, WDTRST1, WDTRST2] maxItems: 1 minItems: 1 type: array pinctrl-[0-9]+: true properties: {phandle: true, pinctrl-names: true, status: true} type: object pinctrl-[0-9]+: true properties: $nodename: true compatible: additionalItems: false items: - enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl'] maxItems: 1 minItems: 1 type: array phandle: true pinctrl-names: true status: true required: [compatible] select: properties: compatible: contains: enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl'] required: [compatible] title: ASPEED AST2400 Pin Controller ``` `properties: {phandle: true, pinctrl-names: true, status: true}` has been merged into my '^.*$' patternProperty, presumably partly from pinctrl-consumer.yaml, and this seems to be the source of the bad output. If as a hack I change my pattern to '^.*_default$' the problem goes away as we no longer try to enforce the constraints on properties provided by other bindings, but the problem is the node names are largely freeform[1] (unless I enforce a naming constraint as part of my bindings?). [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?h=v5.2-rc6#n112 > > BTW, You can put the names under a 'definitions' key and then use > '$ref' to reference them from function and group to avoid duplicating > the names. Or use patternProperties with '^(function|group)$'. I've used the patternProperties approach above as I couldn't get the definitions/$ref approach to work. I did the following: ``` definitions: pinctrl-value: allOf: - $ref: "/schemas/types.yaml#/definitions/string" - enum: [ "ACPI", "ADC0", "ADC1", "ADC10", "ADC11", "ADC12", "ADC13", "ADC14", "ADC15", "ADC2", "ADC3", "ADC4", "ADC5", "ADC6", "ADC7", "ADC8", "ADC9", "BMCINT", "DDCCLK", "DDCDAT", "EXTRST", "FLACK", "FLBUSY", "FLWP", "GPID", "GPID0", "GPID2", "GPID4", "GPID6", "GPIE0", "GPIE2", "GPIE4", "GPIE6", "I2C10", "I2C11", "I2C12", "I2C13", "I2C14", "I2C3", "I2C4", "I2C5", "I2C6", "I2C7", "I2C8", "I2C9", "LPCPD", "LPCPME", "LPCRST", "LPCSMI", "MAC1LINK", "MAC2LINK", "MDIO1", "MDIO2", "NCTS1", "NCTS2", "NCTS3", "NCTS4", "NDCD1", "NDCD2", "NDCD3", "NDCD4", "NDSR1", "NDSR2", "NDSR3", "NDSR4", "NDTR1", "NDTR2", "NDTR3", "NDTR4", "NDTS4", "NRI1", "NRI2", "NRI3", "NRI4", "NRTS1", "NRTS2", "NRTS3", "OSCCLK", "PWM0", "PWM1", "PWM2", "PWM3", "PWM4", "PWM5", "PWM6", "PWM7", "RGMII1", "RGMII2", "RMII1", "RMII2", "ROM16", "ROM8", "ROMCS1", "ROMCS2", "ROMCS3", "ROMCS4", "RXD1", "RXD2", "RXD3", "RXD4", "SALT1", "SALT2", "SALT3", "SALT4", "SD1", "SD2", "SGPMCK", "SGPMI", "SGPMLD", "SGPMO", "SGPSCK", "SGPSI0", "SGPSI1", "SGPSLD", "SIOONCTRL", "SIOPBI", "SIOPBO", "SIOPWREQ", "SIOPWRGD", "SIOS3", "SIOS5", "SIOSCI", "SPI1", "SPI1DEBUG", "SPI1PASSTHRU", "SPICS1", "TIMER3", "TIMER4", "TIMER5", "TIMER6", "TIMER7", "TIMER8", "TXD1", "TXD2", "TXD3", "TXD4", "UART6", "USB11D1", "USB11H2", "USB2D1", "USB2H1", "USBCKI", "VGABIOS_ROM", "VGAHS", "VGAVS", "VPI18", "VPI24", "VPI30", "VPO12", "VPO24", "WDTRST1", "WDTRST2" ] patternProperties: '^.*_default$': type: object properties: function: $ref: "#/definitions/pinctrl-value" groups: $ref: "#/definitions/pinctrl-value" ``` But it gave me output like: ``` /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: wdtrst2_default:function: ['WDTRST2'] is not one of ['ACPI', 'ADC0', 'ADC1', 'ADC10', 'ADC11', 'ADC12', 'ADC13', 'ADC14', 'ADC15', 'ADC2', 'ADC3', 'ADC4', 'ADC5', 'ADC6', 'ADC7', 'ADC8', 'ADC9', 'BMCINT', 'DDCCLK', 'DDCDAT', 'EXTRST', 'FLACK', 'FLBUSY', 'FLWP', 'GPID', 'GPID0', 'GPID2', 'GPID4', 'GPID6', 'GPIE0', 'GPIE2', 'GPIE4', 'GPIE6', 'I2C10', 'I2C11', 'I2C12', 'I2C13', 'I2C14', 'I2C3', 'I2C4', 'I2C5', 'I2C6', 'I2C7', 'I2C8', 'I2C9', 'LPCPD', 'LPCPME', 'LPCRST', 'LPCSMI', 'MAC1LINK', 'MAC2LINK', 'MDIO1', 'MDIO2', 'NCTS1', 'NCTS2', 'NCTS3', 'NCTS4', 'NDCD1', 'NDCD2', 'NDCD3', 'NDCD4', 'NDSR1', 'NDSR2', 'NDSR3', 'NDSR4', 'NDTR1', 'NDTR2', 'NDTR3', 'NDTR4', 'NDTS4', 'NRI1', 'NRI2', 'NRI3', 'NRI4', 'NRTS1', 'NRTS2', 'NRTS3', 'OSCCLK', 'PWM0', 'PWM1', 'PWM2', 'PWM3', 'PWM4', 'PWM5', 'PWM6', 'PWM7', 'RGMII1', 'RGMII2', 'RMII1', 'RMII2', 'ROM16', 'ROM8', 'ROMCS1', 'ROMCS2', 'ROMCS3', 'ROMCS4', 'RXD1 ', 'RXD2', 'RXD3', 'RXD4', 'SALT1', 'SALT2', 'SALT3', 'SALT4', 'SD1', 'SD2', 'SGPMCK', 'SGPMI', 'SGPMLD', 'SGPMO', 'SGPSCK', 'SGPSI0', 'SGPSI1', 'SGPSLD', 'SIOONCTRL', 'SIOPBI', 'SIOPBO', 'SIOPWREQ', 'SIOPWRGD', 'SIOS3', 'SIOS5', 'SIOSCI', 'SPI1', 'SPI1DEBUG', 'SPI1PASSTHRU', 'SPICS1', 'TIMER3', 'TIMER4', 'TIMER5', 'TIMER6', 'TIMER7', 'TIMER8', 'TXD1', 'TXD2', 'TXD3', 'TXD4', 'UART6', 'USB11D1', 'USB11H2', 'USB2D1', 'USB2H1', 'USBCKI', 'VGABIOS_ROM', 'VGAHS', 'VGAVS', 'VPI18', 'VPI24', 'VPI30', 'VPO12', 'VPO24', 'WDTRST1', 'WDTRST2'] /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: wdtrst2_default:groups: ['WDTRST2'] is not one of ['ACPI', 'ADC0', 'ADC1', 'ADC10', 'ADC11', 'ADC12', 'ADC13', 'ADC14', 'ADC15', 'ADC2', 'ADC3', 'ADC4', 'ADC5', 'ADC6', 'ADC7', 'ADC8', 'ADC9', 'BMCINT', 'DDCCLK', 'DDCDAT', 'EXTRST', 'FLACK', 'FLBUSY', 'FLWP', 'GPID', 'GPID0', 'GPID2', 'GPID4', 'GPID6', 'GPIE0', 'GPIE2', 'GPIE4', 'GPIE6', 'I2C10', 'I2C11', 'I2C12', 'I2C13', 'I2C14', 'I2C3', 'I2C4', 'I2C5', 'I2C6', 'I2C7', 'I2C8', 'I2C9', 'LPCPD', 'LPCPME', 'LPCRST', 'LPCSMI', 'MAC1LINK', 'MAC2LINK', 'MDIO1', 'MDIO2', 'NCTS1', 'NCTS2', 'NCTS3', 'NCTS4', 'NDCD1', 'NDCD2', 'NDCD3', 'NDCD4', 'NDSR1', 'NDSR2', 'NDSR3', 'NDSR4', 'NDTR1', 'NDTR2', 'NDTR3', 'NDTR4', 'NDTS4', 'NRI1', 'NRI2', 'NRI3', 'NRI4', 'NRTS1', 'NRTS2', 'NRTS3', 'OSCCLK', 'PWM0', 'PWM1', 'PWM2', 'PWM3', 'PWM4', 'PWM5', 'PWM6', 'PWM7', 'RGMII1', 'RGMII2', 'RMII1', 'RMII2', 'ROM16', 'ROM8', 'ROMCS1', 'ROMCS2', 'ROMCS3', 'ROMCS4', 'RXD1', 'RXD2', 'RXD3', 'RXD4', 'SALT1', 'SALT2', 'SALT3', 'SALT4', 'SD1', 'SD2', 'SGPMCK', 'SGPMI', 'SGPMLD', 'SGPMO', 'SGPSCK', 'SGPSI0', 'SGPSI1', 'SGPSLD', 'SIOONCTRL', 'SIOPBI', 'SIOPBO', 'SIOPWREQ', 'SIOPWRGD', 'SIOS3', 'SIOS5', 'SIOSCI', 'SPI1', 'SPI1DEBUG', 'SPI1PASSTHRU', 'SPICS1', 'TIMER3', 'TIMER4', 'TIMER5', 'TIMER6', 'TIMER7', 'TIMER8', 'TXD1', 'TXD2', 'TXD3', 'TXD4', 'UART6', 'USB11D1', 'USB11H2', 'USB2D1', 'USB2H1', 'USBCKI', 'VGABIOS_ROM', 'VGAHS', 'VGAVS', 'VPI18', 'VPI24', 'VPI30', 'VPO12', 'VPO24', 'WDTRST1', 'WDTRST2'] ``` Clearly I haven't got it quite right, but I'm not sure what's wrong with my approach. Can you tell me? It looks like the property is interpreted as a string-array rather than a string, but I'm not sure why. Cheers, Andrew
On Wed, Jun 26, 2019 at 6:44 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote: > > On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > Convert ASPEED pinctrl bindings to DT schema format using json-schema > > > > BTW, ASPEED is one of the remaining platforms needing the top-level > > board bindings converted. > > Okay, I'll put together patches to fix that. > > > > > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > --- > > > .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 ------------------- > > > .../pinctrl/aspeed,ast2400-pinctrl.yaml | 73 +++++++++++++++++ > > > 2 files changed, 73 insertions(+), 80 deletions(-) > > > delete mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt > > > create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > > new file mode 100644 > > > index 000000000000..3b8cf3e51506 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > > @@ -0,0 +1,73 @@ > > > +# SPDX-License-Identifier: GPL-2.0+ > > > > Do you have rights to change the license? > > Where are you coming from with this question? The bindings previously didn't list a > license, is there some implicit license for them? I would have thought it was GPL-2.0? Yes, it is implicitly GPL-2.0 since it is in the kernel tree and has no other license text. > IBM's (my employer's) preferred contribution license is GPL 2.0-or-later, so I was just > adding the SPDX marker to clarify. Adding 'or-later' is a licensing change. If IBM is the copyright holder on all this file, then that is fine. > > If so, the preference is to > > dual license with (GPL-2.0 OR BSD-2-Clause). > > You're asking if I have the power to relicense so I can dual license it this way? It would probably be up to your company. If that's an issue, then not dual licensing is fine. I don't want to hold things up on that. [...] > > > +required: > > > + - compatible > > > + > > > +description: |+ > > > > description goes before properties. > > Okay. I wouldn't have thought the ordering mattered. Is this just a preference? Yes, just a preference. > The tools seemed to run fine as is. > > I'll re-order it regardless. > > > > > > + The pin controller node should be the child of a syscon node with the > > > + required property: > > > + > > > + - compatible: Should be one of the following: > > > + "aspeed,ast2400-scu", "syscon", "simple-mfd" > > > + "aspeed,g4-scu", "syscon", "simple-mfd" > > > + > > > + Refer to the the bindings described in > > > + Documentation/devicetree/bindings/mfd/syscon.txt > > > + > > > + For the AST2400 pinmux, each mux function has only one associated pin group. > > > + Each group is named by its function. The following values for the function > > > + and groups properties are supported: > > > + > > > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 > > > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 > > > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 > > > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 > > > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 > > > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK > > > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 > > > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 > > > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI > > > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU > > > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 > > > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 > > > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 > > > > This should be a schema. > > Yeah, I covered this in my cover letter. I was hoping to get away without > that for the moment as this seems like the first pinctrl binding to be > converted, however if you insist... That generally doesn't matter. You can assume common properties will have a schema and you don't need to define common constraints (like 'function' is a string array). You only need what is specific to this binding which is possible values. Rob
On Wed, Jun 26, 2019 at 9:55 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote: > > On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > > + The pin controller node should be the child of a syscon node with the > > > + required property: > > > + > > > + - compatible: Should be one of the following: > > > + "aspeed,ast2400-scu", "syscon", "simple-mfd" > > > + "aspeed,g4-scu", "syscon", "simple-mfd" > > > + > > > + Refer to the the bindings described in > > > + Documentation/devicetree/bindings/mfd/syscon.txt > > > + > > > + For the AST2400 pinmux, each mux function has only one associated pin group. > > > + Each group is named by its function. The following values for the function > > > + and groups properties are supported: > > > + > > > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 > > > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 > > > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 > > > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 > > > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 > > > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK > > > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 > > > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 > > > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI > > > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU > > > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 > > > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 > > > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 > > > > This should be a schema. You need to define child nodes and list these > > as values for 'function' and 'group'. Ideally, the child nodes would > > have some sort of pattern, but if not, you can just match on '^.*$' > > under patternProperties. > > The children don't have any pattern in their node name, which drives > me towards the '^.*$' pattern match, however, what I've found is that > I get the following errors for some of the relevant dts files: > > ``` > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-names: ['default'] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-0: [[7, 8, 9, 10, 11, 12]] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: phandle: [[13]] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: $nodename: ['pinctrl'] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-names: ['default'] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-0: [[9, 10, 11, 12]] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: phandle: [[13]] is not of type 'object' > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: $nodename: ['pinctrl'] is not of type 'object' > ``` > The problem is "^.*$" matches both properties and child nodes. > We shouldn't be expecting these properties in the child nodes, so > something is busted. Looking at processed-schema.yaml, we have: > > ``` > - $filename: /home/andrew/src/linux/aspeed/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > $id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > patternProperties: > ^.*$: > patternProperties: > ^function|groups$: > allOf: > - {$ref: /schemas/types.yaml#/definitions/string} > - additionalItems: false > items: > enum: [ACPI, ADC0, ADC1, ADC10, ADC11, ADC12, ADC13, ADC14, ADC15, ADC2, > ADC3, ADC4, ADC5, ADC6, ADC7, ADC8, ADC9, BMCINT, DDCCLK, DDCDAT, > EXTRST, FLACK, FLBUSY, FLWP, GPID, GPID0, GPID2, GPID4, GPID6, GPIE0, > GPIE2, GPIE4, GPIE6, I2C10, I2C11, I2C12, I2C13, I2C14, I2C3, I2C4, > I2C5, I2C6, I2C7, I2C8, I2C9, LPCPD, LPCPME, LPCRST, LPCSMI, MAC1LINK, > MAC2LINK, MDIO1, MDIO2, NCTS1, NCTS2, NCTS3, NCTS4, NDCD1, NDCD2, > NDCD3, NDCD4, NDSR1, NDSR2, NDSR3, NDSR4, NDTR1, NDTR2, NDTR3, NDTR4, > NDTS4, NRI1, NRI2, NRI3, NRI4, NRTS1, NRTS2, NRTS3, OSCCLK, PWM0, > PWM1, PWM2, PWM3, PWM4, PWM5, PWM6, PWM7, RGMII1, RGMII2, RMII1, RMII2, > ROM16, ROM8, ROMCS1, ROMCS2, ROMCS3, ROMCS4, RXD1, RXD2, RXD3, RXD4, > SALT1, SALT2, SALT3, SALT4, SD1, SD2, SGPMCK, SGPMI, SGPMLD, SGPMO, > SGPSCK, SGPSI0, SGPSI1, SGPSLD, SIOONCTRL, SIOPBI, SIOPBO, SIOPWREQ, > SIOPWRGD, SIOS3, SIOS5, SIOSCI, SPI1, SPI1DEBUG, SPI1PASSTHRU, SPICS1, > TIMER3, TIMER4, TIMER5, TIMER6, TIMER7, TIMER8, TXD1, TXD2, TXD3, > TXD4, UART6, USB11D1, USB11H2, USB2D1, USB2H1, USBCKI, VGABIOS_ROM, > VGAHS, VGAVS, VPI18, VPI24, VPI30, VPO12, VPO24, WDTRST1, WDTRST2] > maxItems: 1 > minItems: 1 > type: array > pinctrl-[0-9]+: true > properties: {phandle: true, pinctrl-names: true, status: true} > type: object > pinctrl-[0-9]+: true > properties: > $nodename: true > compatible: > additionalItems: false > items: > - enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl'] > maxItems: 1 > minItems: 1 > type: array > phandle: true > pinctrl-names: true > status: true > required: [compatible] > select: > properties: > compatible: > contains: > enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl'] > required: [compatible] > title: ASPEED AST2400 Pin Controller > ``` > > `properties: {phandle: true, pinctrl-names: true, status: true}` has been > merged into my '^.*$' patternProperty, presumably partly from > pinctrl-consumer.yaml, and this seems to be the source of the bad > output. If as a hack I change my pattern to '^.*_default$' the problem > goes away as we no longer try to enforce the constraints on properties > provided by other bindings, but the problem is the node names are > largely freeform[1] (unless I enforce a naming constraint as part of my > bindings?). > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?h=v5.2-rc6#n112 > > > > > BTW, You can put the names under a 'definitions' key and then use > > '$ref' to reference them from function and group to avoid duplicating > > the names. Or use patternProperties with '^(function|group)$'. > > I've used the patternProperties approach above as I couldn't get the > definitions/$ref approach to work. I did the following: The problem is we'd need to process the schema under definitions. The YAML encoding we validate against always encodes strings as arrays as dtc has no way of knowing if a given property is a string array or single string. So to avoid a bunch of boilerplate in every binding, we process the schema to transform single strings into arrays of length 1. It's probably best to stick with the patternProperties approach. I think you can do something like this: "^.*$": if: type: object then: patternProperties: '^(function|group)$': ... I'm not completely certain this works though, so if you can send me an updated binding with what you have so far I can test it out. Rob
On Thu, 27 Jun 2019, at 23:40, Rob Herring wrote: > On Wed, Jun 26, 2019 at 6:44 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote: > > > On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > Convert ASPEED pinctrl bindings to DT schema format using json-schema > > > > > > BTW, ASPEED is one of the remaining platforms needing the top-level > > > board bindings converted. > > > > Okay, I'll put together patches to fix that. > > > > > > > > > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > > --- > > > > .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 ------------------- > > > > .../pinctrl/aspeed,ast2400-pinctrl.yaml | 73 +++++++++++++++++ > > > > 2 files changed, 73 insertions(+), 80 deletions(-) > > > > delete mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt > > > > create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > > > new file mode 100644 > > > > index 000000000000..3b8cf3e51506 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > > > @@ -0,0 +1,73 @@ > > > > +# SPDX-License-Identifier: GPL-2.0+ > > > > > > Do you have rights to change the license? > > > > Where are you coming from with this question? The bindings previously didn't list a > > license, is there some implicit license for them? I would have thought it was GPL-2.0? > > Yes, it is implicitly GPL-2.0 since it is in the kernel tree and has > no other license text. > > > IBM's (my employer's) preferred contribution license is GPL 2.0-or-later, so I was just > > adding the SPDX marker to clarify. > > Adding 'or-later' is a licensing change. If IBM is the copyright > holder on all this file, then that is fine. I authored the file for IBM and they hold the copyright, so the change is permitted. > > > > If so, the preference is to > > > dual license with (GPL-2.0 OR BSD-2-Clause). > > > > You're asking if I have the power to relicense so I can dual license it this way? > > It would probably be up to your company. If that's an issue, then not > dual licensing is fine. I don't want to hold things up on that. Okay. I've asked and the query is being resolved internally. I'm not sure when that will occur though, so I'll relicense it in a future patch if the request gets the go ahead. Just for the record, what's the motivation for the dual license? Understanding why will likely help resolve the request. > > [...] > > > > > +required: > > > > + - compatible > > > > + > > > > +description: |+ > > > > > > description goes before properties. > > > > Okay. I wouldn't have thought the ordering mattered. Is this just a preference? > > Yes, just a preference. > > > The tools seemed to run fine as is. > > > > I'll re-order it regardless. > > > > > > > > > + The pin controller node should be the child of a syscon node with the > > > > + required property: > > > > + > > > > + - compatible: Should be one of the following: > > > > + "aspeed,ast2400-scu", "syscon", "simple-mfd" > > > > + "aspeed,g4-scu", "syscon", "simple-mfd" > > > > + > > > > + Refer to the the bindings described in > > > > + Documentation/devicetree/bindings/mfd/syscon.txt > > > > + > > > > + For the AST2400 pinmux, each mux function has only one associated pin group. > > > > + Each group is named by its function. The following values for the function > > > > + and groups properties are supported: > > > > + > > > > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 > > > > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 > > > > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 > > > > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 > > > > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 > > > > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK > > > > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 > > > > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 > > > > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI > > > > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU > > > > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 > > > > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 > > > > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 > > > > > > This should be a schema. > > > > Yeah, I covered this in my cover letter. I was hoping to get away without > > that for the moment as this seems like the first pinctrl binding to be > > converted, however if you insist... > > That generally doesn't matter. You can assume common properties will > have a schema and you don't need to define common constraints (like > 'function' is a string array). You only need what is specific to this > binding which is possible values. Right, it just wasn't clear to me how much effort was involved. Having hacked around a bit now I've found it's not so much. Thanks for your feedback. Andrew
On Fri, 28 Jun 2019, at 00:02, Rob Herring wrote: > On Wed, Jun 26, 2019 at 9:55 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote: > > > On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > + The pin controller node should be the child of a syscon node with the > > > > + required property: > > > > + > > > > + - compatible: Should be one of the following: > > > > + "aspeed,ast2400-scu", "syscon", "simple-mfd" > > > > + "aspeed,g4-scu", "syscon", "simple-mfd" > > > > + > > > > + Refer to the the bindings described in > > > > + Documentation/devicetree/bindings/mfd/syscon.txt > > > > + > > > > + For the AST2400 pinmux, each mux function has only one associated pin group. > > > > + Each group is named by its function. The following values for the function > > > > + and groups properties are supported: > > > > + > > > > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 > > > > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 > > > > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 > > > > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 > > > > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 > > > > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK > > > > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 > > > > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 > > > > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI > > > > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU > > > > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 > > > > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 > > > > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 > > > > > > This should be a schema. You need to define child nodes and list these > > > as values for 'function' and 'group'. Ideally, the child nodes would > > > have some sort of pattern, but if not, you can just match on '^.*$' > > > under patternProperties. > > > > The children don't have any pattern in their node name, which drives > > me towards the '^.*$' pattern match, however, what I've found is that > > I get the following errors for some of the relevant dts files: > > > > ``` > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-names: ['default'] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-0: [[7, 8, 9, 10, 11, 12]] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: phandle: [[13]] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: $nodename: ['pinctrl'] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-names: ['default'] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-0: [[9, 10, 11, 12]] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: phandle: [[13]] is not of type 'object' > > /home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: $nodename: ['pinctrl'] is not of type 'object' > > ``` > > > > The problem is "^.*$" matches both properties and child nodes. > > > We shouldn't be expecting these properties in the child nodes, so > > something is busted. Looking at processed-schema.yaml, we have: > > > > ``` > > - $filename: /home/andrew/src/linux/aspeed/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml > > $id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > patternProperties: > > ^.*$: > > patternProperties: > > ^function|groups$: > > allOf: > > - {$ref: /schemas/types.yaml#/definitions/string} > > - additionalItems: false > > items: > > enum: [ACPI, ADC0, ADC1, ADC10, ADC11, ADC12, ADC13, ADC14, ADC15, ADC2, > > ADC3, ADC4, ADC5, ADC6, ADC7, ADC8, ADC9, BMCINT, DDCCLK, DDCDAT, > > EXTRST, FLACK, FLBUSY, FLWP, GPID, GPID0, GPID2, GPID4, GPID6, GPIE0, > > GPIE2, GPIE4, GPIE6, I2C10, I2C11, I2C12, I2C13, I2C14, I2C3, I2C4, > > I2C5, I2C6, I2C7, I2C8, I2C9, LPCPD, LPCPME, LPCRST, LPCSMI, MAC1LINK, > > MAC2LINK, MDIO1, MDIO2, NCTS1, NCTS2, NCTS3, NCTS4, NDCD1, NDCD2, > > NDCD3, NDCD4, NDSR1, NDSR2, NDSR3, NDSR4, NDTR1, NDTR2, NDTR3, NDTR4, > > NDTS4, NRI1, NRI2, NRI3, NRI4, NRTS1, NRTS2, NRTS3, OSCCLK, PWM0, > > PWM1, PWM2, PWM3, PWM4, PWM5, PWM6, PWM7, RGMII1, RGMII2, RMII1, RMII2, > > ROM16, ROM8, ROMCS1, ROMCS2, ROMCS3, ROMCS4, RXD1, RXD2, RXD3, RXD4, > > SALT1, SALT2, SALT3, SALT4, SD1, SD2, SGPMCK, SGPMI, SGPMLD, SGPMO, > > SGPSCK, SGPSI0, SGPSI1, SGPSLD, SIOONCTRL, SIOPBI, SIOPBO, SIOPWREQ, > > SIOPWRGD, SIOS3, SIOS5, SIOSCI, SPI1, SPI1DEBUG, SPI1PASSTHRU, SPICS1, > > TIMER3, TIMER4, TIMER5, TIMER6, TIMER7, TIMER8, TXD1, TXD2, TXD3, > > TXD4, UART6, USB11D1, USB11H2, USB2D1, USB2H1, USBCKI, VGABIOS_ROM, > > VGAHS, VGAVS, VPI18, VPI24, VPI30, VPO12, VPO24, WDTRST1, WDTRST2] > > maxItems: 1 > > minItems: 1 > > type: array > > pinctrl-[0-9]+: true > > properties: {phandle: true, pinctrl-names: true, status: true} > > type: object > > pinctrl-[0-9]+: true > > properties: > > $nodename: true > > compatible: > > additionalItems: false > > items: > > - enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl'] > > maxItems: 1 > > minItems: 1 > > type: array > > phandle: true > > pinctrl-names: true > > status: true > > required: [compatible] > > select: > > properties: > > compatible: > > contains: > > enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl'] > > required: [compatible] > > title: ASPEED AST2400 Pin Controller > > ``` > > > > `properties: {phandle: true, pinctrl-names: true, status: true}` has been > > merged into my '^.*$' patternProperty, presumably partly from > > pinctrl-consumer.yaml, and this seems to be the source of the bad > > output. If as a hack I change my pattern to '^.*_default$' the problem > > goes away as we no longer try to enforce the constraints on properties > > provided by other bindings, but the problem is the node names are > > largely freeform[1] (unless I enforce a naming constraint as part of my > > bindings?). > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?h=v5.2-rc6#n112 > > > > > > > > BTW, You can put the names under a 'definitions' key and then use > > > '$ref' to reference them from function and group to avoid duplicating > > > the names. Or use patternProperties with '^(function|group)$'. > > > > I've used the patternProperties approach above as I couldn't get the > > definitions/$ref approach to work. I did the following: > > The problem is we'd need to process the schema under definitions. The > YAML encoding we validate against always encodes strings as arrays as > dtc has no way of knowing if a given property is a string array or > single string. So to avoid a bunch of boilerplate in every binding, we > process the schema to transform single strings into arrays of length > 1. > > It's probably best to stick with the patternProperties approach. I > think you can do something like this: > > "^.*$": > if: > type: object > then: > patternProperties: > '^(function|group)$': > ... > > I'm not completely certain this works though, so if you can send me an > updated binding with what you have so far I can test it out. This works, thanks. I'll send an updated series. Andrew
diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt deleted file mode 100644 index 67e0325ccf2e..000000000000 --- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt +++ /dev/null @@ -1,80 +0,0 @@ -============================= -Aspeed AST2400 Pin Controller -============================= - -Required properties for the AST2400: -- compatible : Should be one of the following: - "aspeed,ast2400-pinctrl" - "aspeed,g4-pinctrl" - -The pin controller node should be the child of a syscon node with the required -property: - -- compatible : Should be one of the following: - "aspeed,ast2400-scu", "syscon", "simple-mfd" - "aspeed,g4-scu", "syscon", "simple-mfd" - -Refer to the the bindings described in -Documentation/devicetree/bindings/mfd/syscon.txt - -Subnode Format -============== - -The required properties of pinmux child nodes are: -- function: the mux function to select -- groups : the list of groups to select with this function - -Required properties of pinconf child nodes are: -- groups: A list of groups to select (either this or "pins" must be - specified) -- pins : A list of ball names as strings, eg "D14" (either this or "groups" - must be specified) - -Optional properties of pinconf child nodes are: -- bias-disable : disable any pin bias -- bias-pull-down: pull down the pin -- drive-strength: sink or source at most X mA - -Definitions are as specified in -Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt, with any -further limitations as described above. - -For pinmux, each mux function has only one associated pin group. Each group is -named by its function. The following values for the function and groups -properties are supported: - -ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 -ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 -GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 -I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 -MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 -NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK PWM0 -PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 ROMCS1 -ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 SD2 SGPMCK -SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI SIOPBO SIOPWREQ -SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU SPICS1 TIMER3 TIMER4 -TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 USB11D1 USB11H2 USB2D1 -USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 VPI30 VPO12 VPO24 WDTRST1 -WDTRST2 - -Example -======= - -syscon: scu@1e6e2000 { - compatible = "aspeed,ast2400-scu", "syscon", "simple-mfd"; - reg = <0x1e6e2000 0x1a8>; - - pinctrl: pinctrl { - compatible = "aspeed,g4-pinctrl"; - - pinctrl_i2c3_default: i2c3_default { - function = "I2C3"; - groups = "I2C3"; - }; - - pinctrl_gpioh0_unbiased_default: gpioh0 { - pins = "A8"; - bias-disable; - }; - }; -}; diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml new file mode 100644 index 000000000000..3b8cf3e51506 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml @@ -0,0 +1,73 @@ +# SPDX-License-Identifier: GPL-2.0+ +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ASPEED AST2400 Pin Controller + +maintainers: + - Andrew Jeffery <andrew@aj.id.au> + +properties: + compatible: + oneOf: + - items: + - enum: + - aspeed,ast2400-pinctrl + - items: + - enum: + - aspeed,g4-pinctrl + +required: + - compatible + +description: |+ + The pin controller node should be the child of a syscon node with the + required property: + + - compatible: Should be one of the following: + "aspeed,ast2400-scu", "syscon", "simple-mfd" + "aspeed,g4-scu", "syscon", "simple-mfd" + + Refer to the the bindings described in + Documentation/devicetree/bindings/mfd/syscon.txt + + For the AST2400 pinmux, each mux function has only one associated pin group. + Each group is named by its function. The following values for the function + and groups properties are supported: + + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6 + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2 + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1 + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4 + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 + VPI30 VPO12 VPO24 WDTRST1 WDTRST2 + +examples: + - | + syscon: scu@1e6e2000 { + compatible = "aspeed,ast2400-scu", "syscon", "simple-mfd"; + reg = <0x1e6e2000 0x1a8>; + + pinctrl: pinctrl { + compatible = "aspeed,g4-pinctrl"; + + pinctrl_i2c3_default: i2c3_default { + function = "I2C3"; + groups = "I2C3"; + }; + + pinctrl_gpioh0_unbiased_default: gpioh0 { + pins = "A8"; + bias-disable; + }; + }; + };
Convert ASPEED pinctrl bindings to DT schema format using json-schema Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 ------------------- .../pinctrl/aspeed,ast2400-pinctrl.yaml | 73 +++++++++++++++++ 2 files changed, 73 insertions(+), 80 deletions(-) delete mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml