diff mbox series

dt-bindings: gpio: Convert Aspeed binding to YAML schema

Message ID 20240220052918.742793-1-andrew@codeconstruct.com.au
State New
Headers
Series dt-bindings: gpio: Convert Aspeed binding to YAML schema |

Commit Message

Andrew Jeffery Feb. 20, 2024, 5:29 a.m. UTC 
  Squash warnings such as:

```
arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-galaxy100.dtb: /ahb/apb@1e600000/gpio@1e780000: failed to match any schema with compatible: ['aspeed,ast2400-gpio']
```

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 .../bindings/gpio/aspeed,ast2400-gpio.yaml    | 64 +++++++++++++++++++
 .../devicetree/bindings/gpio/gpio-aspeed.txt  | 39 -----------
 2 files changed, 64 insertions(+), 39 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
 delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-aspeed.txt


base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d

Comments

Krzysztof Kozlowski Feb. 20, 2024, 8:27 a.m. UTC | #1 
On 20/02/2024 06:29, Andrew Jeffery wrote:
> Squash warnings such as:
> 

Missing subject prefix: aspeed,ast2400-gpio


> ```
> arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-galaxy100.dtb: /ahb/apb@1e600000/gpio@1e780000: failed to match any schema with compatible: ['aspeed,ast2400-gpio']
> ```
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Thank you for your patch. There is something to discuss/improve.


> ---
>  .../bindings/gpio/aspeed,ast2400-gpio.yaml    | 64 +++++++++++++++++++
>  .../devicetree/bindings/gpio/gpio-aspeed.txt  | 39 -----------
>  2 files changed, 64 insertions(+), 39 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
>  delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> new file mode 100644
> index 000000000000..353c7620013f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/aspeed,ast2400-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed GPIO controller
> +
> +maintainers:
> +  - Andrew Jeffery <andrew@codeconstruct.com.au>
> +
> +allOf:
> +  - $ref: /schemas/gpio/gpio.yaml#

From where did you take it? None of the bindings have such code. Start
from recent bindings in given category when writing new ones.

Please drop it.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-gpio
> +    then:
> +      required:
> +        - ngpios

Please put entire allOf: after required: block. That's the convention
when it has something more than $ref, because we still want the most
important parts at the top of the file.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2400-gpio
> +      - aspeed,ast2500-gpio
> +      - aspeed,ast2600-gpio
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description: The clock to use for debounce timings
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +

ngpios with some constraints

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-controller
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +unevaluatedProperties: false

Instead additionalProperties: false.


Best regards,
Krzysztof
Andrew Jeffery Feb. 21, 2024, 2:41 a.m. UTC | #2 
Hi Krzysztof, thanks for the feedback.

On Tue, 2024-02-20 at 09:27 +0100, Krzysztof Kozlowski wrote:
> > On 20/02/2024 06:29, Andrew Jeffery wrote:
> > > > Squash warnings such as:
> > > > 
> > 
> > Missing subject prefix: aspeed,ast2400-gpio
> > 
> > 
> > > > ```
> > > > arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-galaxy100.dtb: /ahb/apb@1e600000/gpio@1e780000: failed to match any schema with compatible: ['aspeed,ast2400-gpio']
> > > > ```
> > > > 
> > > > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > 
> > Thank you for your patch. There is something to discuss/improve.
> > 
> > 
> > > > ---
> > > >  .../bindings/gpio/aspeed,ast2400-gpio.yaml    | 64 +++++++++++++++++++
> > > >  .../devicetree/bindings/gpio/gpio-aspeed.txt  | 39 -----------
> > > >  2 files changed, 64 insertions(+), 39 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> > > >  delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> > > > new file mode 100644
> > > > index 000000000000..353c7620013f
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> > > > @@ -0,0 +1,64 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/gpio/aspeed,ast2400-gpio.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Aspeed GPIO controller
> > > > +
> > > > +maintainers:
> > > > +  - Andrew Jeffery <andrew@codeconstruct.com.au>
> > > > +
> > > > +allOf:
> > > > +  - $ref: /schemas/gpio/gpio.yaml#
> > 
> > From where did you take it? None of the bindings have such code. Start
> > from recent bindings in given category when writing new ones.

I didn't take it from anywhere so much as try to apply some reasoning
via the commentary in the example at [1]. Maybe I could have fought
that approach by contrasting what I wrote to a wider set of existing
binding documents (I did look at some and obviously didn't find
anything similar).

Anyway reflecting on the misunderstanding, is the ref unnecessary
because the gpio binding sets `select: true`[2] and so is applied to
the node regardless?

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/example-schema.yaml?h=v6.7#n238
[2]: https://github.com/devicetree-org/dt-schema/blob/v2023.11/dtschema/schemas/gpio/gpio.yaml#L12

> > 
> > Please drop it.

Ack.

> > 
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            const: aspeed,ast2600-gpio
> > > > +    then:
> > > > +      required:
> > > > +        - ngpios
> > 
> > Please put entire allOf: after required: block. That's the convention
> > when it has something more than $ref, because we still want the most
> > important parts at the top of the file.

Ack.

> > 
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - aspeed,ast2400-gpio
> > > > +      - aspeed,ast2500-gpio
> > > > +      - aspeed,ast2600-gpio
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    maxItems: 1
> > > > +    description: The clock to use for debounce timings
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupt-controller: true
> > > > +
> > > > +  "#interrupt-cells":
> > > > +    const: 2
> > > > +
> > 
> > ngpios with some constraints

Is this just with regard to the constraints I have under allOf above?
Or something further (constrain the values of ngpios for the various
controllers across the Aspeed SoCs)?

Maybe I'll look at some more of the existing bindings for this as
well...

> > 
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - interrupts
> > > > +  - interrupt-controller
> > > > +  - "#gpio-cells"
> > > > +  - gpio-controller
> > > > +
> > > > +unevaluatedProperties: false
> > 
> > Instead additionalProperties: false.

Ack - this is the same misunderstanding as the gpio schema ref
discussed above.

Andrew
Krzysztof Kozlowski Feb. 21, 2024, 7:22 a.m. UTC | #3 
On 21/02/2024 03:41, Andrew Jeffery wrote:
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - aspeed,ast2400-gpio
>>>>> +      - aspeed,ast2500-gpio
>>>>> +      - aspeed,ast2600-gpio
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 1
>>>>> +    description: The clock to use for debounce timings
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupt-controller: true
>>>>> +
>>>>> +  "#interrupt-cells":
>>>>> +    const: 2
>>>>> +
>>>
>>> ngpios with some constraints
> 
> Is this just with regard to the constraints I have under allOf above?

You miss ngpios.



Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
new file mode 100644
index 000000000000..353c7620013f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
@@ -0,0 +1,64 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/aspeed,ast2400-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed GPIO controller
+
+maintainers:
+  - Andrew Jeffery <andrew@codeconstruct.com.au>
+
+allOf:
+  - $ref: /schemas/gpio/gpio.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2600-gpio
+    then:
+      required:
+        - ngpios
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2400-gpio
+      - aspeed,ast2500-gpio
+      - aspeed,ast2600-gpio
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: The clock to use for debounce timings
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-controller
+  - "#gpio-cells"
+  - gpio-controller
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    gpio@1e780000 {
+        compatible = "aspeed,ast2400-gpio";
+        reg = <0x1e780000 0x1000>;
+        interrupts = <20>;
+        interrupt-controller;
+        #gpio-cells = <2>;
+        gpio-controller;
+    };
diff --git a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
deleted file mode 100644
index b2033fc3a71a..000000000000
--- a/Documentation/devicetree/bindings/gpio/gpio-aspeed.txt
+++ /dev/null
@@ -1,39 +0,0 @@ 
-Aspeed GPIO controller Device Tree Bindings
--------------------------------------------
-
-Required properties:
-- compatible		: Either "aspeed,ast2400-gpio", "aspeed,ast2500-gpio",
-					or "aspeed,ast2600-gpio".
-
-- #gpio-cells 		: Should be two
-			  - First cell is the GPIO line number
-			  - Second cell is used to specify optional
-			    parameters (unused)
-
-- reg			: Address and length of the register set for the device
-- gpio-controller	: Marks the device node as a GPIO controller.
-- interrupts		: Interrupt specifier (see interrupt bindings for
-			  details)
-- interrupt-controller	: Mark the GPIO controller as an interrupt-controller
-
-Optional properties:
-
-- clocks		: A phandle to the clock to use for debounce timings
-- ngpios		: Number of GPIOs controlled by this controller. Should	be set
-				  when there are multiple GPIO controllers on a SoC (ast2600).
-
-The gpio and interrupt properties are further described in their respective
-bindings documentation:
-
-- Documentation/devicetree/bindings/gpio/gpio.txt
-- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
-
-  Example:
-	gpio@1e780000 {
-		#gpio-cells = <2>;
-		compatible = "aspeed,ast2400-gpio";
-		gpio-controller;
-		interrupts = <20>;
-		reg = <0x1e780000 0x1000>;
-		interrupt-controller;
-	};