| Message ID | 20220422192139.2592632-1-robh@kernel.org |
|---|---|
| State | Accepted |
| Commit | 6384f12461523844cec72af2b94504b7d6c218ac |
| Headers | show |
| Series | dt-bindings: pinctrl: aspeed: Drop referenced nodes in examples | expand |
On Fri, 22 Apr 2022 at 19:21, Rob Herring <robh@kernel.org> wrote: > > The additional nodes in the example referenced from the pinctrl node > 'aspeed,external-nodes' properties are either incorrect (aspeed,ast2500-lpc) > or not documented with a schema (aspeed,ast2500-gfx). There's no need to > show these nodes as part of the pinctrl example, so just remove them. > > Signed-off-by: Rob Herring <robh@kernel.org> Nak. This removes the information on how to use the bindings. Surely we prefer to over document rather than under document? > --- > .../pinctrl/aspeed,ast2500-pinctrl.yaml | 81 ++++--------------- > 1 file changed, 16 insertions(+), 65 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml > index 7c25c8d51116..9db904a528ee 100644 > --- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml > @@ -76,73 +76,24 @@ additionalProperties: false > examples: > - | > #include <dt-bindings/clock/aspeed-clock.h> > - apb { > - compatible = "simple-bus"; > - #address-cells = <1>; > - #size-cells = <1>; > - ranges; > - > - syscon: scu@1e6e2000 { > - compatible = "aspeed,ast2500-scu", "syscon", "simple-mfd"; > - reg = <0x1e6e2000 0x1a8>; > - #clock-cells = <1>; > - #reset-cells = <1>; > - > - pinctrl: pinctrl { > - compatible = "aspeed,ast2500-pinctrl"; > - aspeed,external-nodes = <&gfx>, <&lhc>; > - > - pinctrl_i2c3_default: i2c3_default { > - function = "I2C3"; > - groups = "I2C3"; > - }; > - > - pinctrl_gpioh0_unbiased_default: gpioh0 { > - pins = "A18"; > - bias-disable; > - }; > + scu@1e6e2000 { > + compatible = "aspeed,ast2500-scu", "syscon", "simple-mfd"; > + reg = <0x1e6e2000 0x1a8>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + > + pinctrl: pinctrl { > + compatible = "aspeed,ast2500-pinctrl"; > + aspeed,external-nodes = <&gfx>, <&lhc>; > + > + pinctrl_i2c3_default: i2c3_default { > + function = "I2C3"; > + groups = "I2C3"; > }; > - }; > - > - gfx: display@1e6e6000 { > - compatible = "aspeed,ast2500-gfx", "syscon"; > - reg = <0x1e6e6000 0x1000>; > - reg-io-width = <4>; > - clocks = <&syscon ASPEED_CLK_GATE_D1CLK>; > - resets = <&syscon ASPEED_RESET_CRT1>; > - interrupts = <0x19>; > - syscon = <&syscon>; > - memory-region = <&gfx_memory>; > - }; > - }; > - > - lpc: lpc@1e789000 { > - compatible = "aspeed,ast2500-lpc", "simple-mfd"; > - reg = <0x1e789000 0x1000>; > - > - #address-cells = <1>; > - #size-cells = <1>; > - ranges = <0x0 0x1e789000 0x1000>; > - > - lpc_host: lpc-host@80 { > - compatible = "aspeed,ast2500-lpc-host", "simple-mfd", "syscon"; > - reg = <0x80 0x1e0>; > - reg-io-width = <4>; > > - #address-cells = <1>; > - #size-cells = <1>; > - ranges = <0x0 0x80 0x1e0>; > - > - lhc: lhc@20 { > - compatible = "aspeed,ast2500-lhc"; > - reg = <0x20 0x24>, <0x48 0x8>; > + pinctrl_gpioh0_unbiased_default: gpioh0 { > + pins = "A18"; > + bias-disable; > }; > }; > }; > - > - gfx_memory: framebuffer { > - size = <0x01000000>; > - alignment = <0x01000000>; > - compatible = "shared-dma-pool"; > - reusable; > - }; > -- > 2.32.0 >
On Wed, Apr 27, 2022 at 08:40:31AM +0000, Joel Stanley wrote: > On Fri, 22 Apr 2022 at 19:21, Rob Herring <robh@kernel.org> wrote: > > > > The additional nodes in the example referenced from the pinctrl node > > 'aspeed,external-nodes' properties are either incorrect (aspeed,ast2500-lpc) > > or not documented with a schema (aspeed,ast2500-gfx). There's no need to > > show these nodes as part of the pinctrl example, so just remove them. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > Nak. I welcome patches that add schemas for the undocumented compatibles instead. Otherwise, I will be turning on this check by default and nagging people to fix them. > This removes the information on how to use the bindings. Surely we > prefer to over document rather than under document? How is what the 'gfx' and 'lpc' nodes contain relevant to how the pinctrl binding works? If a user wants to know, then they should go look at the aspeed,ast2500-lpc/aspeed,ast2500-gfx bindings and their examples. Which brings up my secondary issue which is having the same example multiple times. It is multiple chances for errors (that I end up fixing). How do we know the example is even correct without any schema checks? The 'framebuffer' node is not in a valid location is the most obvious thing I see. Rob
On 27/04/2022 10:40, Joel Stanley wrote: > On Fri, 22 Apr 2022 at 19:21, Rob Herring <robh@kernel.org> wrote: >> >> The additional nodes in the example referenced from the pinctrl node >> 'aspeed,external-nodes' properties are either incorrect (aspeed,ast2500-lpc) >> or not documented with a schema (aspeed,ast2500-gfx). There's no need to >> show these nodes as part of the pinctrl example, so just remove them. >> >> Signed-off-by: Rob Herring <robh@kernel.org> > > Nak. > > This removes the information on how to use the bindings. Surely we > prefer to over document rather than under document? There is no need to document consumers of generic providers, like syscon, clock or pinctrl. These are already well documented. The examples of consumers here do not bring any additional value. Best regards, Krzysztof
On Fri, Apr 22, 2022 at 9:21 PM Rob Herring <robh@kernel.org> wrote: > The additional nodes in the example referenced from the pinctrl node > 'aspeed,external-nodes' properties are either incorrect (aspeed,ast2500-lpc) > or not documented with a schema (aspeed,ast2500-gfx). There's no need to > show these nodes as part of the pinctrl example, so just remove them. > > Signed-off-by: Rob Herring <robh@kernel.org> Patch applied. Concerns about lost examples can be solved with incremental patches on top adding more schema. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml index 7c25c8d51116..9db904a528ee 100644 --- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml @@ -76,73 +76,24 @@ additionalProperties: false examples: - | #include <dt-bindings/clock/aspeed-clock.h> - apb { - compatible = "simple-bus"; - #address-cells = <1>; - #size-cells = <1>; - ranges; - - syscon: scu@1e6e2000 { - compatible = "aspeed,ast2500-scu", "syscon", "simple-mfd"; - reg = <0x1e6e2000 0x1a8>; - #clock-cells = <1>; - #reset-cells = <1>; - - pinctrl: pinctrl { - compatible = "aspeed,ast2500-pinctrl"; - aspeed,external-nodes = <&gfx>, <&lhc>; - - pinctrl_i2c3_default: i2c3_default { - function = "I2C3"; - groups = "I2C3"; - }; - - pinctrl_gpioh0_unbiased_default: gpioh0 { - pins = "A18"; - bias-disable; - }; + scu@1e6e2000 { + compatible = "aspeed,ast2500-scu", "syscon", "simple-mfd"; + reg = <0x1e6e2000 0x1a8>; + #clock-cells = <1>; + #reset-cells = <1>; + + pinctrl: pinctrl { + compatible = "aspeed,ast2500-pinctrl"; + aspeed,external-nodes = <&gfx>, <&lhc>; + + pinctrl_i2c3_default: i2c3_default { + function = "I2C3"; + groups = "I2C3"; }; - }; - - gfx: display@1e6e6000 { - compatible = "aspeed,ast2500-gfx", "syscon"; - reg = <0x1e6e6000 0x1000>; - reg-io-width = <4>; - clocks = <&syscon ASPEED_CLK_GATE_D1CLK>; - resets = <&syscon ASPEED_RESET_CRT1>; - interrupts = <0x19>; - syscon = <&syscon>; - memory-region = <&gfx_memory>; - }; - }; - - lpc: lpc@1e789000 { - compatible = "aspeed,ast2500-lpc", "simple-mfd"; - reg = <0x1e789000 0x1000>; - - #address-cells = <1>; - #size-cells = <1>; - ranges = <0x0 0x1e789000 0x1000>; - - lpc_host: lpc-host@80 { - compatible = "aspeed,ast2500-lpc-host", "simple-mfd", "syscon"; - reg = <0x80 0x1e0>; - reg-io-width = <4>; - #address-cells = <1>; - #size-cells = <1>; - ranges = <0x0 0x80 0x1e0>; - - lhc: lhc@20 { - compatible = "aspeed,ast2500-lhc"; - reg = <0x20 0x24>, <0x48 0x8>; + pinctrl_gpioh0_unbiased_default: gpioh0 { + pins = "A18"; + bias-disable; }; }; }; - - gfx_memory: framebuffer { - size = <0x01000000>; - alignment = <0x01000000>; - compatible = "shared-dma-pool"; - reusable; - };
The additional nodes in the example referenced from the pinctrl node 'aspeed,external-nodes' properties are either incorrect (aspeed,ast2500-lpc) or not documented with a schema (aspeed,ast2500-gfx). There's no need to show these nodes as part of the pinctrl example, so just remove them. Signed-off-by: Rob Herring <robh@kernel.org> --- .../pinctrl/aspeed,ast2500-pinctrl.yaml | 81 ++++--------------- 1 file changed, 16 insertions(+), 65 deletions(-)