| Message ID | 20200612141903.2391044-26-thierry.reding@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dt-bindings: json-schema conversions and cleanups | expand |
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.
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
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.
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.
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
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 --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; + };