diff mbox series

[25/38] dt-bindings: gpio: tegra: Convert to json-schema

Message ID 20200612141903.2391044-26-thierry.reding@gmail.com
State Changes Requested
Headers show
Series dt-bindings: json-schema conversions and cleanups | expand

Commit Message

Thierry Reding June 12, 2020, 2:18 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

Convert the NVIDIA Tegra GPIO controller device tree bindings from
free-form text format to json-schema.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../bindings/gpio/nvidia,tegra20-gpio.txt     |  40 -------
 .../bindings/gpio/nvidia,tegra20-gpio.yaml    | 111 ++++++++++++++++++
 2 files changed, 111 insertions(+), 40 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.txt
 create mode 100644 Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml

Comments

Dmitry Osipenko June 17, 2020, 4:24 a.m. UTC | #1
12.06.2020 17:18, Thierry Reding пишет:
...
> +patternProperties:
> +  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
> +  "^gpios(-[a-zA-Z0-9-]+)?$":
> +    type: object
> +    required:
> +      - gpio-hog

There are two problems here:

1. This naming limitation didn't exist before this patch, so it's not a
part of the conversion.

2. GPIO core uses the node's name for the hog's name. Hence by imposing
the "gpios-" prefix, you're forcing all hogs to be named as gpios-xxx,
which doesn't make much sense to me.

Please explain the rationale of this change.
Thierry Reding June 17, 2020, 2:17 p.m. UTC | #2
On Wed, Jun 17, 2020 at 07:24:16AM +0300, Dmitry Osipenko wrote:
> 12.06.2020 17:18, Thierry Reding пишет:
> ...
> > +patternProperties:
> > +  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
> > +  "^gpios(-[a-zA-Z0-9-]+)?$":
> > +    type: object
> > +    required:
> > +      - gpio-hog
> 
> There are two problems here:
> 
> 1. This naming limitation didn't exist before this patch, so it's not a
> part of the conversion.
> 
> 2. GPIO core uses the node's name for the hog's name. Hence by imposing
> the "gpios-" prefix, you're forcing all hogs to be named as gpios-xxx,
> which doesn't make much sense to me.
> 
> Please explain the rationale of this change.

We could probably do without this if we didn't enforce additional or
unevaluated properties. Because if we don't match on a pattern here then
all of those GPIO hog nodes would show up as "extra" properties and they
are currently not allowed. If we do allow them, then we can drop this,
but we then have no way to fail validation for whatever else somebody
might want to put into these device tree nodes.

That said, I think additionalProperties can be a schema in itself, so
maybe there's a way to only allow additional properties if they are of
type object and have a gpio-hog property. I'll look into that.

Thierry
Dmitry Osipenko June 17, 2020, 2:24 p.m. UTC | #3
17.06.2020 17:17, Thierry Reding пишет:
> On Wed, Jun 17, 2020 at 07:24:16AM +0300, Dmitry Osipenko wrote:
>> 12.06.2020 17:18, Thierry Reding пишет:
>> ...
>>> +patternProperties:
>>> +  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
>>> +  "^gpios(-[a-zA-Z0-9-]+)?$":
>>> +    type: object
>>> +    required:
>>> +      - gpio-hog
>>
>> There are two problems here:
>>
>> 1. This naming limitation didn't exist before this patch, so it's not a
>> part of the conversion.
>>
>> 2. GPIO core uses the node's name for the hog's name. Hence by imposing
>> the "gpios-" prefix, you're forcing all hogs to be named as gpios-xxx,
>> which doesn't make much sense to me.
>>
>> Please explain the rationale of this change.
> 
> We could probably do without this if we didn't enforce additional or
> unevaluated properties. Because if we don't match on a pattern here then
> all of those GPIO hog nodes would show up as "extra" properties and they
> are currently not allowed. If we do allow them, then we can drop this,
> but we then have no way to fail validation for whatever else somebody
> might want to put into these device tree nodes.
> 
> That said, I think additionalProperties can be a schema in itself, so
> maybe there's a way to only allow additional properties if they are of
> type object and have a gpio-hog property. I'll look into that.

Isn't it possible to validate the additional properties by checking what
properties they have?

For example, if sub-node has a gpio-hog property then this sub-node is
okay, otherwise fail.
Dmitry Osipenko June 17, 2020, 2:33 p.m. UTC | #4
17.06.2020 17:24, Dmitry Osipenko пишет:
> 17.06.2020 17:17, Thierry Reding пишет:
>> On Wed, Jun 17, 2020 at 07:24:16AM +0300, Dmitry Osipenko wrote:
>>> 12.06.2020 17:18, Thierry Reding пишет:
>>> ...
>>>> +patternProperties:
>>>> +  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
>>>> +  "^gpios(-[a-zA-Z0-9-]+)?$":
>>>> +    type: object
>>>> +    required:
>>>> +      - gpio-hog
>>>
>>> There are two problems here:
>>>
>>> 1. This naming limitation didn't exist before this patch, so it's not a
>>> part of the conversion.
>>>
>>> 2. GPIO core uses the node's name for the hog's name. Hence by imposing
>>> the "gpios-" prefix, you're forcing all hogs to be named as gpios-xxx,
>>> which doesn't make much sense to me.
>>>
>>> Please explain the rationale of this change.
>>
>> We could probably do without this if we didn't enforce additional or
>> unevaluated properties. Because if we don't match on a pattern here then
>> all of those GPIO hog nodes would show up as "extra" properties and they
>> are currently not allowed. If we do allow them, then we can drop this,
>> but we then have no way to fail validation for whatever else somebody
>> might want to put into these device tree nodes.
>>
>> That said, I think additionalProperties can be a schema in itself, so
>> maybe there's a way to only allow additional properties if they are of
>> type object and have a gpio-hog property. I'll look into that.
> 
> Isn't it possible to validate the additional properties by checking what
> properties they have?
> 
> For example, if sub-node has a gpio-hog property then this sub-node is
> okay, otherwise fail.
> 

Ah, I haven't finished reading yours last sentence before started to
type :) Yes, it will be nice if we could avoid the naming limitation, or
at least change it to something like xxx-hog.
Thierry Reding June 17, 2020, 4:50 p.m. UTC | #5
On Wed, Jun 17, 2020 at 05:33:00PM +0300, Dmitry Osipenko wrote:
> 17.06.2020 17:24, Dmitry Osipenko пишет:
> > 17.06.2020 17:17, Thierry Reding пишет:
> >> On Wed, Jun 17, 2020 at 07:24:16AM +0300, Dmitry Osipenko wrote:
> >>> 12.06.2020 17:18, Thierry Reding пишет:
> >>> ...
> >>>> +patternProperties:
> >>>> +  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
> >>>> +  "^gpios(-[a-zA-Z0-9-]+)?$":
> >>>> +    type: object
> >>>> +    required:
> >>>> +      - gpio-hog
> >>>
> >>> There are two problems here:
> >>>
> >>> 1. This naming limitation didn't exist before this patch, so it's not a
> >>> part of the conversion.
> >>>
> >>> 2. GPIO core uses the node's name for the hog's name. Hence by imposing
> >>> the "gpios-" prefix, you're forcing all hogs to be named as gpios-xxx,
> >>> which doesn't make much sense to me.
> >>>
> >>> Please explain the rationale of this change.
> >>
> >> We could probably do without this if we didn't enforce additional or
> >> unevaluated properties. Because if we don't match on a pattern here then
> >> all of those GPIO hog nodes would show up as "extra" properties and they
> >> are currently not allowed. If we do allow them, then we can drop this,
> >> but we then have no way to fail validation for whatever else somebody
> >> might want to put into these device tree nodes.
> >>
> >> That said, I think additionalProperties can be a schema in itself, so
> >> maybe there's a way to only allow additional properties if they are of
> >> type object and have a gpio-hog property. I'll look into that.
> > 
> > Isn't it possible to validate the additional properties by checking what
> > properties they have?
> > 
> > For example, if sub-node has a gpio-hog property then this sub-node is
> > okay, otherwise fail.
> > 
> 
> Ah, I haven't finished reading yours last sentence before started to
> type :) Yes, it will be nice if we could avoid the naming limitation, or
> at least change it to something like xxx-hog.

So according to the json-schema specification, both additionalProperties
and unevaluatedProperties must be a valid JSON schema, which means they
can be objects rather than just booleans. Unfortunately, dt-schema tools
don't allow these to be objects, so the below currently fails with these
tools at the moment.

I can make it work with the following patch against dt-schema.git:

--- >8 ---
diff --git a/meta-schemas/keywords.yaml b/meta-schemas/keywords.yaml
index ed543235d7e7..aa88f726ea3b 100644
--- a/meta-schemas/keywords.yaml
+++ b/meta-schemas/keywords.yaml
@@ -79,7 +79,11 @@ properties:
   additionalItems:
     type: boolean
   additionalProperties:
-    type: boolean
+    oneOf:
+      - type: object
+        allOf:
+          - $ref: "#/definitions/sub-schemas"
+      - type: boolean
   allOf:
     items:
       $ref: "#/definitions/sub-schemas"
@@ -140,7 +144,11 @@ properties:
   type: true
   typeSize: true
   unevaluatedProperties:
-    type: boolean
+    oneOf:
+      - type: object
+        allOf:
+          - $ref: "#/definitions/sub-schemas"
+      - type: boolean
   uniqueItems:
     type: boolean
 
--- >8 ---

With that applied, I can make validation of gpio-hog nodes work without
requiring the names to change, which incidentally will allow me to drop
one of the fixup patches from the ARM/arm64 DTS series.

Here's a hunk that applies on top of this patch and makes this work.
I'll squash it in for the next version.

--- >8 ---
diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml b/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
index b2debdb0caff..3f8a9c988305 100644
--- a/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
@@ -57,13 +57,6 @@ properties:
   interrupt-controller:
     description: Marks the device node as an interrupt controller.
 
-patternProperties:
-  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
-  "^gpios(-[a-zA-Z0-9-]+)?$":
-    type: object
-    required:
-      - gpio-hog
-
 allOf:
   - if:
       properties:
@@ -90,7 +83,10 @@ required:
   - "#interrupt-cells"
   - interrupt-controller
 
-unevaluatedProperties: false
+unevaluatedProperties:
+  type: object
+  required:
+    - gpio-hog
 
 examples:
   - |
--- >8 ---

Thierry
Dmitry Osipenko June 18, 2020, 3:07 p.m. UTC | #6
17.06.2020 19:50, Thierry Reding пишет:
> On Wed, Jun 17, 2020 at 05:33:00PM +0300, Dmitry Osipenko wrote:
>> 17.06.2020 17:24, Dmitry Osipenko пишет:
>>> 17.06.2020 17:17, Thierry Reding пишет:
>>>> On Wed, Jun 17, 2020 at 07:24:16AM +0300, Dmitry Osipenko wrote:
>>>>> 12.06.2020 17:18, Thierry Reding пишет:
>>>>> ...
>>>>>> +patternProperties:
>>>>>> +  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
>>>>>> +  "^gpios(-[a-zA-Z0-9-]+)?$":
>>>>>> +    type: object
>>>>>> +    required:
>>>>>> +      - gpio-hog
>>>>>
>>>>> There are two problems here:
>>>>>
>>>>> 1. This naming limitation didn't exist before this patch, so it's not a
>>>>> part of the conversion.
>>>>>
>>>>> 2. GPIO core uses the node's name for the hog's name. Hence by imposing
>>>>> the "gpios-" prefix, you're forcing all hogs to be named as gpios-xxx,
>>>>> which doesn't make much sense to me.
>>>>>
>>>>> Please explain the rationale of this change.
>>>>
>>>> We could probably do without this if we didn't enforce additional or
>>>> unevaluated properties. Because if we don't match on a pattern here then
>>>> all of those GPIO hog nodes would show up as "extra" properties and they
>>>> are currently not allowed. If we do allow them, then we can drop this,
>>>> but we then have no way to fail validation for whatever else somebody
>>>> might want to put into these device tree nodes.
>>>>
>>>> That said, I think additionalProperties can be a schema in itself, so
>>>> maybe there's a way to only allow additional properties if they are of
>>>> type object and have a gpio-hog property. I'll look into that.
>>>
>>> Isn't it possible to validate the additional properties by checking what
>>> properties they have?
>>>
>>> For example, if sub-node has a gpio-hog property then this sub-node is
>>> okay, otherwise fail.
>>>
>>
>> Ah, I haven't finished reading yours last sentence before started to
>> type :) Yes, it will be nice if we could avoid the naming limitation, or
>> at least change it to something like xxx-hog.
> 
> So according to the json-schema specification, both additionalProperties
> and unevaluatedProperties must be a valid JSON schema, which means they
> can be objects rather than just booleans. Unfortunately, dt-schema tools
> don't allow these to be objects, so the below currently fails with these
> tools at the moment.
> 
> I can make it work with the following patch against dt-schema.git:
> 
> --- >8 ---
> diff --git a/meta-schemas/keywords.yaml b/meta-schemas/keywords.yaml
> index ed543235d7e7..aa88f726ea3b 100644
> --- a/meta-schemas/keywords.yaml
> +++ b/meta-schemas/keywords.yaml
> @@ -79,7 +79,11 @@ properties:
>    additionalItems:
>      type: boolean
>    additionalProperties:
> -    type: boolean
> +    oneOf:
> +      - type: object
> +        allOf:
> +          - $ref: "#/definitions/sub-schemas"
> +      - type: boolean
>    allOf:
>      items:
>        $ref: "#/definitions/sub-schemas"
> @@ -140,7 +144,11 @@ properties:
>    type: true
>    typeSize: true
>    unevaluatedProperties:
> -    type: boolean
> +    oneOf:
> +      - type: object
> +        allOf:
> +          - $ref: "#/definitions/sub-schemas"
> +      - type: boolean
>    uniqueItems:
>      type: boolean
>  
> --- >8 ---
> 
> With that applied, I can make validation of gpio-hog nodes work without
> requiring the names to change, which incidentally will allow me to drop
> one of the fixup patches from the ARM/arm64 DTS series.
> 
> Here's a hunk that applies on top of this patch and makes this work.
> I'll squash it in for the next version.
> 
> --- >8 ---
> diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml b/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
> index b2debdb0caff..3f8a9c988305 100644
> --- a/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
> +++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
> @@ -57,13 +57,6 @@ properties:
>    interrupt-controller:
>      description: Marks the device node as an interrupt controller.
>  
> -patternProperties:
> -  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
> -  "^gpios(-[a-zA-Z0-9-]+)?$":
> -    type: object
> -    required:
> -      - gpio-hog
> -
>  allOf:
>    - if:
>        properties:
> @@ -90,7 +83,10 @@ required:
>    - "#interrupt-cells"
>    - interrupt-controller
>  
> -unevaluatedProperties: false
> +unevaluatedProperties:
> +  type: object
> +  required:
> +    - gpio-hog
>  
>  examples:
>    - |
> --- >8 ---

Thank you for figuring this out! I see that the dt-schema tool is
already updated in the git, very nice!
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.txt b/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.txt
deleted file mode 100644
index 023c9526e5f8..000000000000
--- a/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.txt
+++ /dev/null
@@ -1,40 +0,0 @@ 
-NVIDIA Tegra GPIO controller
-
-Required properties:
-- compatible : "nvidia,tegra<chip>-gpio"
-- reg : Physical base address and length of the controller's registers.
-- interrupts : The interrupt outputs from the controller. For Tegra20,
-  there should be 7 interrupts specified, and for Tegra30, there should
-  be 8 interrupts specified.
-- #gpio-cells : Should be two. The first cell is the pin number and the
-  second cell is used to specify optional parameters:
-  - bit 0 specifies polarity (0 for normal, 1 for inverted)
-- gpio-controller : Marks the device node as a GPIO controller.
-- #interrupt-cells : Should be 2.
-  The first cell is the GPIO number.
-  The second cell is used to specify flags:
-    bits[3:0] trigger type and level flags:
-      1 = low-to-high edge triggered.
-      2 = high-to-low edge triggered.
-      4 = active high level-sensitive.
-      8 = active low level-sensitive.
-      Valid combinations are 1, 2, 3, 4, 8.
-- interrupt-controller : Marks the device node as an interrupt controller.
-
-Example:
-
-gpio: gpio@6000d000 {
-	compatible = "nvidia,tegra20-gpio";
-	reg = < 0x6000d000 0x1000 >;
-	interrupts = < 0 32 0x04
-		       0 33 0x04
-		       0 34 0x04
-		       0 35 0x04
-		       0 55 0x04
-		       0 87 0x04
-		       0 89 0x04 >;
-	#gpio-cells = <2>;
-	gpio-controller;
-	#interrupt-cells = <2>;
-	interrupt-controller;
-};
diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml b/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
new file mode 100644
index 000000000000..b2debdb0caff
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.yaml
@@ -0,0 +1,111 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/nvidia,tegra20-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NVIDIA Tegra GPIO Controller (Tegra20 - Tegra210)
+
+maintainers:
+  - Thierry Reding <thierry.reding@gmail.com>
+  - Jon Hunter <jonathanh@nvidia.com>
+
+properties:
+  compatible:
+    oneOf:
+      - const: nvidia,tegra20-gpio
+      - const: nvidia,tegra30-gpio
+      - items:
+          - enum:
+              - nvidia,tegra114-gpio
+              - nvidia,tegra124-gpio
+              - nvidia,tegra210-gpio
+          - const: nvidia,tegra30-gpio
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description: The interrupt outputs from the controller. For Tegra20,
+      there should be 7 interrupts specified, and for Tegra30, there should
+      be 8 interrupts specified.
+
+  "#gpio-cells":
+    description: The first cell is the pin number and the second cell is used
+      to specify the GPIO polarity (0 = active high, 1 = active low).
+    $ref: "/schemas/types.yaml#/definitions/uint32"
+    const: 2
+
+  gpio-controller:
+    description: Marks the device node as a GPIO controller.
+    type: boolean
+
+  "#interrupt-cells":
+    description: |
+      Should be 2. The first cell is the GPIO number. The second cell is
+      used to specify flags:
+
+        bits[3:0] trigger type and level flags:
+          1 = low-to-high edge triggered.
+          2 = high-to-low edge triggered.
+          4 = active high level-sensitive.
+          8 = active low level-sensitive.
+
+      Valid combinations are 1, 2, 3, 4, 8.
+    const: 2
+
+  interrupt-controller:
+    description: Marks the device node as an interrupt controller.
+
+patternProperties:
+  # GPIO hogs; /schemas/gpio/gpio-hog.yaml will match
+  "^gpios(-[a-zA-Z0-9-]+)?$":
+    type: object
+    required:
+      - gpio-hog
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: nvidia,tegra30-gpio
+    then:
+      properties:
+        interrupts:
+          minItems: 8
+          maxItems: 8
+    else:
+      properties:
+        interrupts:
+          minItems: 7
+          maxItems: 7
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#gpio-cells"
+  - gpio-controller
+  - "#interrupt-cells"
+  - interrupt-controller
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    gpio: gpio@6000d000 {
+        compatible = "nvidia,tegra20-gpio";
+        reg = <0x6000d000 0x1000>;
+        interrupts = <0 32 0x04>,
+                     <0 33 0x04>,
+                     <0 34 0x04>,
+                     <0 35 0x04>,
+                     <0 55 0x04>,
+                     <0 87 0x04>,
+                     <0 89 0x04>;
+        #gpio-cells = <2>;
+        gpio-controller;
+        #interrupt-cells = <2>;
+        interrupt-controller;
+    };