diff mbox series

dt-bindings: pinctrl: aspeed: Drop referenced nodes in examples

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

Commit Message

Rob Herring (Arm) April 22, 2022, 7:21 p.m. UTC
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(-)

Comments

Joel Stanley April 27, 2022, 8:40 a.m. UTC | #1
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
>
Rob Herring (Arm) April 27, 2022, 6:44 p.m. UTC | #2
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
Krzysztof Kozlowski April 28, 2022, 6:53 a.m. UTC | #3
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
Linus Walleij May 4, 2022, 9:25 p.m. UTC | #4
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 mbox series

Patch

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;
-    };