| Message ID | 20230525050241.3700-1-william.zhang@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt_binding_check report false alarm? | expand |
Hey William, On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: > Hi, > > It seems dt_binding_check reports a false error when run on this > modified yaml. I picked this simple file just to demostrate this issue. > Basically I made the interrupts and interrupt-names as optional > properties. But when there are two interrupts present, then > interrupt-names are required. However in the example, I don't define > interrupts and interrupt-name at all, the dt binding check reports error > that interrupt-names are required: Rob and Krzysztof would know more than me, but since they're not around... > diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > index 563a31605d2b..c37a3a64a78c 100644 > --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > @@ -32,11 +32,18 @@ properties: > clock-names: > const: ipg > > +allOf: > + - if: > + properties: > + interrupts: > + minItems: 2 ...I don't think you can actually do this and "minItems: 2" will always evaluate to true because it is an assignment. Don't hold me to that though! The standard pattern here is to do: allOf: - if: properties: compatible: contains: const: foo then: required: - interrupt-names Cheers, Conor. > + then: > + required: > + - interrupt-names > + > required: > - compatible > - reg > - - interrupts > - - interrupt-names > - clocks > - clock-names > > @@ -49,6 +56,4 @@ examples: > reg = <0x53fac000 0x4000>; > clocks = <&clks 111>; > clock-names = "ipg"; > - interrupts = <49>, <50>; > - interrupt-names = "scm", "smn"; > }; > -- > 2.34.1 >
Sorry for the multiple emails. Our mail relay server was not working properly. Hi Conor, On 05/25/2023 06:23 AM, Conor Dooley wrote: > Hey William, > > On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: >> Hi, >> >> It seems dt_binding_check reports a false error when run on this >> modified yaml. I picked this simple file just to demostrate this issue. >> Basically I made the interrupts and interrupt-names as optional >> properties. But when there are two interrupts present, then >> interrupt-names are required. However in the example, I don't define >> interrupts and interrupt-name at all, the dt binding check reports error >> that interrupt-names are required: > > Rob and Krzysztof would know more than me, but since they're not > around... > >> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> index 563a31605d2b..c37a3a64a78c 100644 >> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> @@ -32,11 +32,18 @@ properties: >> clock-names: >> const: ipg >> >> +allOf: >> + - if: >> + properties: >> + interrupts: >> + minItems: 2 > > ...I don't think you can actually do this and "minItems: 2" will always > evaluate to true because it is an assignment. Don't hold me to that > though! The standard pattern here is to do: > allOf: > - if: > properties: > compatible: > contains: > const: foo > then: > required: > - interrupt-names > > Cheers, > Conor. > Our device can use one or two interrupt, or choose to not use interrupt at all(polling mode). Interrupt names is only required when there are two interrupts(so the driver code can tell which is which). So I will need to check if it contains two interrupts. My check does work if I have two interrupt but don't have interrupt name, the check catches the error. If I have one interrupt without interrupt name, the check pass. Only when I does not have interrupt and interrupt name, it falsely report error. Looks to me that it does not treat minItem = 0 case properly. >> + then: >> + required: >> + - interrupt-names >> + >> required: >> - compatible >> - reg >> - - interrupts >> - - interrupt-names >> - clocks >> - clock-names >> >> @@ -49,6 +56,4 @@ examples: >> reg = <0x53fac000 0x4000>; >> clocks = <&clks 111>; >> clock-names = "ipg"; >> - interrupts = <49>, <50>; >> - interrupt-names = "scm", "smn"; >> }; >> -- >> 2.34.1 >> > >
On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote: > Sorry for the multiple emails. Our mail relay server was not working > properly. I only got one /shrug > On 05/25/2023 06:23 AM, Conor Dooley wrote: > > Hey William, > > > > On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: > > > Hi, > > > > > > It seems dt_binding_check reports a false error when run on this > > > modified yaml. I picked this simple file just to demostrate this issue. > > > Basically I made the interrupts and interrupt-names as optional > > > properties. But when there are two interrupts present, then > > > interrupt-names are required. However in the example, I don't define > > > interrupts and interrupt-name at all, the dt binding check reports error > > > that interrupt-names are required: > > > > Rob and Krzysztof would know more than me, but since they're not > > around... > > > > > diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > > index 563a31605d2b..c37a3a64a78c 100644 > > > --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > > +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > > @@ -32,11 +32,18 @@ properties: > > > clock-names: > > > const: ipg > > > +allOf: > > > + - if: > > > + properties: > > > + interrupts: > > > + minItems: 2 > > > > ...I don't think you can actually do this and "minItems: 2" will always > > evaluate to true because it is an assignment. Don't hold me to that > > though! The standard pattern here is to do: > > allOf: > > - if: > > properties: > > compatible: > > contains: > > const: foo > > then: > > required: > > - interrupt-names > > > > Cheers, > > Conor. > > > Our device can use one or two interrupt, or choose to not use interrupt at > all(polling mode). Interrupt names is only required when there are two > interrupts(so the driver code can tell which is which). So I will need to > check if it contains two interrupts. My check does work if I have two > interrupt but don't have interrupt name, the check catches the error. If I > have one interrupt without interrupt name, the check pass. Only when I does > not have interrupt and interrupt name, it falsely report error. Looks to > me that it does not treat minItem = 0 case properly. Right. I would not bother with the "only interrupt-names when 2 interrupts" stuff & do the simple thing of always making it required when you have interrupts. Then you can use allOf and oneOf to allow for both schemes for the new device and keep enforcement of 2 items for the existing one. Cheers, Conor. > > > + then: > > > + required: > > > + - interrupt-names > > > + > > > required: > > > - compatible > > > - reg > > > - - interrupts > > > - - interrupt-names > > > - clocks > > > - clock-names > > > @@ -49,6 +56,4 @@ examples: > > > reg = <0x53fac000 0x4000>; > > > clocks = <&clks 111>; > > > clock-names = "ipg"; > > > - interrupts = <49>, <50>; > > > - interrupt-names = "scm", "smn"; > > > }; > > > -- > > > 2.34.1 > > > > > > >
On 05/25/2023 08:33 AM, Conor Dooley wrote: > On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote: >> Sorry for the multiple emails. Our mail relay server was not working >> properly. > > I only got one /shrug > That's good. Maybe it only flushed queued email to internal email accounts. >> On 05/25/2023 06:23 AM, Conor Dooley wrote: >>> Hey William, >>> >>> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: >>>> Hi, >>>> >>>> It seems dt_binding_check reports a false error when run on this >>>> modified yaml. I picked this simple file just to demostrate this issue. >>>> Basically I made the interrupts and interrupt-names as optional >>>> properties. But when there are two interrupts present, then >>>> interrupt-names are required. However in the example, I don't define >>>> interrupts and interrupt-name at all, the dt binding check reports error >>>> that interrupt-names are required: >>> >>> Rob and Krzysztof would know more than me, but since they're not >>> around... >>> >>>> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>>> index 563a31605d2b..c37a3a64a78c 100644 >>>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>>> @@ -32,11 +32,18 @@ properties: >>>> clock-names: >>>> const: ipg >>>> +allOf: >>>> + - if: >>>> + properties: >>>> + interrupts: >>>> + minItems: 2 >>> >>> ...I don't think you can actually do this and "minItems: 2" will always >>> evaluate to true because it is an assignment. Don't hold me to that >>> though! The standard pattern here is to do: >>> allOf: >>> - if: >>> properties: >>> compatible: >>> contains: >>> const: foo >>> then: >>> required: >>> - interrupt-names >>> >>> Cheers, >>> Conor. >>> >> Our device can use one or two interrupt, or choose to not use interrupt at >> all(polling mode). Interrupt names is only required when there are two >> interrupts(so the driver code can tell which is which). So I will need to >> check if it contains two interrupts. My check does work if I have two >> interrupt but don't have interrupt name, the check catches the error. If I >> have one interrupt without interrupt name, the check pass. Only when I does >> not have interrupt and interrupt name, it falsely report error. Looks to >> me that it does not treat minItem = 0 case properly. > > Right. I would not bother with the "only interrupt-names when 2 > interrupts" stuff & do the simple thing of always making it required > when you have interrupts. > Then you can use allOf and oneOf to allow for both schemes for the new > device and keep enforcement of 2 items for the existing one. > > Cheers, > Conor. > I agree it is better to keep it simple. Sorry I am still new to yaml but what is the best way to check if interrupt property exist? What I can think of is similar - if: properties: interrupts: minItems: 1 then: required: - interrupt-names But this still reports the same error when there is no interrupt. >>>> + then: >>>> + required: >>>> + - interrupt-names >>>> + >>>> required: >>>> - compatible >>>> - reg >>>> - - interrupts >>>> - - interrupt-names >>>> - clocks >>>> - clock-names >>>> @@ -49,6 +56,4 @@ examples: >>>> reg = <0x53fac000 0x4000>; >>>> clocks = <&clks 111>; >>>> clock-names = "ipg"; >>>> - interrupts = <49>, <50>; >>>> - interrupt-names = "scm", "smn"; >>>> }; >>>> -- >>>> 2.34.1 >>>> >>> >>> > >
On 25/05/2023 4:23 pm, William Zhang wrote: > Sorry for the multiple emails. Our mail relay server was not working > properly. > > Hi Conor, > > On 05/25/2023 06:23 AM, Conor Dooley wrote: >> Hey William, >> >> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: >>> Hi, >>> >>> It seems dt_binding_check reports a false error when run on this >>> modified yaml. I picked this simple file just to demostrate this issue. >>> Basically I made the interrupts and interrupt-names as optional >>> properties. But when there are two interrupts present, then >>> interrupt-names are required. However in the example, I don't define >>> interrupts and interrupt-name at all, the dt binding check reports error >>> that interrupt-names are required: >> >> Rob and Krzysztof would know more than me, but since they're not >> around... >> >>> diff --git >>> a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>> b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>> index 563a31605d2b..c37a3a64a78c 100644 >>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>> @@ -32,11 +32,18 @@ properties: >>> clock-names: >>> const: ipg >>> +allOf: >>> + - if: >>> + properties: >>> + interrupts: >>> + minItems: 2 >> >> ...I don't think you can actually do this and "minItems: 2" will always >> evaluate to true because it is an assignment. Don't hold me to that >> though! The standard pattern here is to do: >> allOf: >> - if: >> properties: >> compatible: >> contains: >> const: foo >> then: >> required: >> - interrupt-names >> >> Cheers, >> Conor. >> > Our device can use one or two interrupt, or choose to not use interrupt > at all(polling mode). Interrupt names is only required when there are > two interrupts(so the driver code can tell which is which). So I will > need to check if it contains two interrupts. My check does work if I > have two interrupt but don't have interrupt name, the check catches the > error. If I have one interrupt without interrupt name, the check pass. > Only when I does not have interrupt and interrupt name, it falsely > report error. Looks to me that it does not treat minItem = 0 case > properly. Note that minItems = 0 is *not* the same as the property being absent (that would represent an empty property, i.e. just "interrupts;") The conditional schema fragment only says that an "interrupts" property must have at least two entries in order to match. However it doesn't say that the property is required, and thus the example DT (without the property) does not fail to match the constraints of the given schema, and thus the condition ends up true. It certainly took me several goes to get my head round how conditionals work, and the notion that what goes under the "if:" is a schema definition in its own right :) Robin. > > >>> + then: >>> + required: >>> + - interrupt-names >>> + >>> required: >>> - compatible >>> - reg >>> - - interrupts >>> - - interrupt-names >>> - clocks >>> - clock-names >>> @@ -49,6 +56,4 @@ examples: >>> reg = <0x53fac000 0x4000>; >>> clocks = <&clks 111>; >>> clock-names = "ipg"; >>> - interrupts = <49>, <50>; >>> - interrupt-names = "scm", "smn"; >>> }; >>> -- >>> 2.34.1 >>> >> >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, May 25, 2023 at 10:10:46AM -0700, William Zhang wrote: > > > On 05/25/2023 08:33 AM, Conor Dooley wrote: > > On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote: > > > Sorry for the multiple emails. Our mail relay server was not working > > > properly. > > > > I only got one /shrug > > > That's good. Maybe it only flushed queued email to internal email accounts. > > > > On 05/25/2023 06:23 AM, Conor Dooley wrote: > > > > Hey William, > > > > > > > > On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: > > > > > Hi, > > > > > > > > > > It seems dt_binding_check reports a false error when run on this > > > > > modified yaml. I picked this simple file just to demostrate this issue. > > > > > Basically I made the interrupts and interrupt-names as optional > > > > > properties. But when there are two interrupts present, then > > > > > interrupt-names are required. However in the example, I don't define > > > > > interrupts and interrupt-name at all, the dt binding check reports error > > > > > that interrupt-names are required: > > > > > > > > Rob and Krzysztof would know more than me, but since they're not > > > > around... > > > > > > > > > diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > > > > index 563a31605d2b..c37a3a64a78c 100644 > > > > > --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > > > > +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > > > > @@ -32,11 +32,18 @@ properties: > > > > > clock-names: > > > > > const: ipg > > > > > +allOf: > > > > > + - if: > > > > > + properties: > > > > > + interrupts: > > > > > + minItems: 2 > > > > > > > > ...I don't think you can actually do this and "minItems: 2" will always > > > > evaluate to true because it is an assignment. Don't hold me to that > > > > though! The standard pattern here is to do: > > > > allOf: > > > > - if: > > > > properties: > > > > compatible: > > > > contains: > > > > const: foo > > > > then: > > > > required: > > > > - interrupt-names > > > Our device can use one or two interrupt, or choose to not use interrupt at > > > all(polling mode). Interrupt names is only required when there are two > > > interrupts(so the driver code can tell which is which). So I will need to > > > check if it contains two interrupts. My check does work if I have two > > > interrupt but don't have interrupt name, the check catches the error. If I > > > have one interrupt without interrupt name, the check pass. Only when I does > > > not have interrupt and interrupt name, it falsely report error. Looks to > > > me that it does not treat minItem = 0 case properly. > > > > Right. I would not bother with the "only interrupt-names when 2 > > interrupts" stuff & do the simple thing of always making it required > > when you have interrupts. > > Then you can use allOf and oneOf to allow for both schemes for the new > > device and keep enforcement of 2 items for the existing one. > I agree it is better to keep it simple. Sorry I am still new to yaml but > what is the best way to check if interrupt property exist? What I can think > of is similar > > - if: > properties: > interrupts: > minItems: 1 > then: > required: > - interrupt-names > > But this still reports the same error when there is no interrupt. Yeah, you don't need to do something like that. It's hard to say exactly without your actual patch, but I think you're over complicating things :) What you seem to want to do (from where I am sitting) is something like: dependencies: interrupts: interrupt-names and not put interrupts/interrupt-names in the required bit. That way, if you have interrupts, you need to have interrupt-names too but you can have neither. But yeah, without something resembling the "real" patch, rather than hacking at the fsl one, it is kinda hard to say. Cheers, Conor. > > > > > > + then: > > > > > + required: > > > > > + - interrupt-names > > > > > + > > > > > required: > > > > > - compatible > > > > > - reg > > > > > - - interrupts > > > > > - - interrupt-names > > > > > - clocks > > > > > - clock-names > > > > > @@ -49,6 +56,4 @@ examples: > > > > > reg = <0x53fac000 0x4000>; > > > > > clocks = <&clks 111>; > > > > > clock-names = "ipg"; > > > > > - interrupts = <49>, <50>; > > > > > - interrupt-names = "scm", "smn"; > > > > > }; > > > > > -- > > > > > 2.34.1 > > > > > > > > > > > > > > > > >
On Wed, 24 May 2023 22:02:41 -0700, William Zhang wrote: > Hi, > > It seems dt_binding_check reports a false error when run on this > modified yaml. I picked this simple file just to demostrate this issue. > Basically I made the interrupts and interrupt-names as optional > properties. But when there are two interrupts present, then > interrupt-names are required. However in the example, I don't define > interrupts and interrupt-name at all, the dt binding check reports error > that interrupt-names are required: > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > LINT Documentation/devicetree/bindings > CHKDT Documentation/devicetree/bindings/processed-schema.json > SCHEMA Documentation/devicetree/bindings/processed-schema.json > DTEX Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dts > DTC_CHK Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb > Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property > From schema: /home/william/projects/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > This does not make sense to me as name is required only when there are > two interrupts. Can someone familar with this please help to check if I > miss anything in the yaml file or it is indeed a bug in binding check? > > Thanks, > William > > --- > .../devicetree/bindings/crypto/fsl-imx-scc.yaml | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230525050241.3700-1-william.zhang@broadcom.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 05/25/2023 11:12 AM, Conor Dooley wrote: > On Thu, May 25, 2023 at 10:10:46AM -0700, William Zhang wrote: >> >> >> On 05/25/2023 08:33 AM, Conor Dooley wrote: >>> On Thu, May 25, 2023 at 08:23:30AM -0700, William Zhang wrote: >>>> Sorry for the multiple emails. Our mail relay server was not working >>>> properly. >>> >>> I only got one /shrug >>> >> That's good. Maybe it only flushed queued email to internal email accounts. >> >>>> On 05/25/2023 06:23 AM, Conor Dooley wrote: >>>>> Hey William, >>>>> >>>>> On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: >>>>>> Hi, >>>>>> >>>>>> It seems dt_binding_check reports a false error when run on this >>>>>> modified yaml. I picked this simple file just to demostrate this issue. >>>>>> Basically I made the interrupts and interrupt-names as optional >>>>>> properties. But when there are two interrupts present, then >>>>>> interrupt-names are required. However in the example, I don't define >>>>>> interrupts and interrupt-name at all, the dt binding check reports error >>>>>> that interrupt-names are required: >>>>> >>>>> Rob and Krzysztof would know more than me, but since they're not >>>>> around... >>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>>>>> index 563a31605d2b..c37a3a64a78c 100644 >>>>>> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>>>>> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >>>>>> @@ -32,11 +32,18 @@ properties: >>>>>> clock-names: >>>>>> const: ipg >>>>>> +allOf: >>>>>> + - if: >>>>>> + properties: >>>>>> + interrupts: >>>>>> + minItems: 2 >>>>> >>>>> ...I don't think you can actually do this and "minItems: 2" will always >>>>> evaluate to true because it is an assignment. Don't hold me to that >>>>> though! The standard pattern here is to do: >>>>> allOf: >>>>> - if: >>>>> properties: >>>>> compatible: >>>>> contains: >>>>> const: foo >>>>> then: >>>>> required: >>>>> - interrupt-names > >>>> Our device can use one or two interrupt, or choose to not use interrupt at >>>> all(polling mode). Interrupt names is only required when there are two >>>> interrupts(so the driver code can tell which is which). So I will need to >>>> check if it contains two interrupts. My check does work if I have two >>>> interrupt but don't have interrupt name, the check catches the error. If I >>>> have one interrupt without interrupt name, the check pass. Only when I does >>>> not have interrupt and interrupt name, it falsely report error. Looks to >>>> me that it does not treat minItem = 0 case properly. >>> >>> Right. I would not bother with the "only interrupt-names when 2 >>> interrupts" stuff & do the simple thing of always making it required >>> when you have interrupts. >>> Then you can use allOf and oneOf to allow for both schemes for the new >>> device and keep enforcement of 2 items for the existing one. > >> I agree it is better to keep it simple. Sorry I am still new to yaml but >> what is the best way to check if interrupt property exist? What I can think >> of is similar >> >> - if: >> properties: >> interrupts: >> minItems: 1 >> then: >> required: >> - interrupt-names >> >> But this still reports the same error when there is no interrupt. > > Yeah, you don't need to do something like that. > It's hard to say exactly without your actual patch, but I think you're > over complicating things :) > What you seem to want to do (from where I am sitting) is something like: > dependencies: > interrupts: interrupt-names > > and not put interrupts/interrupt-names in the required bit. > That way, if you have interrupts, you need to have interrupt-names too > but you can have neither. > > But yeah, without something resembling the "real" patch, rather than > hacking at the fsl one, it is kinda hard to say. > > Cheers, > Conor. > > Yeah this dependencies rule works and it is much simple. Just need to add the bracket around the interrupt-names to make it work: dependencies: interrupts: [interrupt-names] Do we have any document list all these supported keywords/rules/syntax for yaml dts? My file is actually brcm,brcmnand.yaml but it is little bit complicated to demonstrate the issue so just randomly pick this simple one. Thanks Conor, Robin and all the for help. Will submit an official patches for brcmnand.yaml change. >> >>>>>> + then: >>>>>> + required: >>>>>> + - interrupt-names >>>>>> + >>>>>> required: >>>>>> - compatible >>>>>> - reg >>>>>> - - interrupts >>>>>> - - interrupt-names >>>>>> - clocks >>>>>> - clock-names >>>>>> @@ -49,6 +56,4 @@ examples: >>>>>> reg = <0x53fac000 0x4000>; >>>>>> clocks = <&clks 111>; >>>>>> clock-names = "ipg"; >>>>>> - interrupts = <49>, <50>; >>>>>> - interrupt-names = "scm", "smn"; >>>>>> }; >>>>>> -- >>>>>> 2.34.1 >>>>>> >>>>> >>>>> >>> >>> > >
On Thu, May 25, 2023 at 10:56:45PM -0700, William Zhang wrote: > Do we have any document list all these supported keywords/rules/syntax for > yaml dts? The example schema is a good place to look: https://docs.kernel.org/devicetree/bindings/writing-schema.html?highlight=example+schema#annotated-example-schema
On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: > Hi, > > It seems dt_binding_check reports a false error when run on this > modified yaml. I picked this simple file just to demostrate this issue. > Basically I made the interrupts and interrupt-names as optional > properties. But when there are two interrupts present, then > interrupt-names are required. However in the example, I don't define > interrupts and interrupt-name at all, the dt binding check reports error > that interrupt-names are required: > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > LINT Documentation/devicetree/bindings > CHKDT Documentation/devicetree/bindings/processed-schema.json > SCHEMA Documentation/devicetree/bindings/processed-schema.json > DTEX Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dts > DTC_CHK Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb > Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property > From schema: /home/william/projects/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > > This does not make sense to me as name is required only when there are > two interrupts. Can someone familar with this please help to check if I > miss anything in the yaml file or it is indeed a bug in binding check? > > Thanks, > William > > --- > .../devicetree/bindings/crypto/fsl-imx-scc.yaml | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > index 563a31605d2b..c37a3a64a78c 100644 > --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml > @@ -32,11 +32,18 @@ properties: > clock-names: > const: ipg > > +allOf: > + - if: > + properties: > + interrupts: > + minItems: 2 This is a quirk of json-schema in that the 'if' will be true if the property is not present. Adding this to the 'if' should fix the problem: required: - interrupts > + then: > + required: > + - interrupt-names > + > required: > - compatible > - reg > - - interrupts > - - interrupt-names > - clocks > - clock-names > > @@ -49,6 +56,4 @@ examples: > reg = <0x53fac000 0x4000>; > clocks = <&clks 111>; > clock-names = "ipg"; > - interrupts = <49>, <50>; > - interrupt-names = "scm", "smn"; > }; > -- > 2.34.1 >
On 06/08/2023 07:57 AM, Rob Herring wrote: > On Wed, May 24, 2023 at 10:02:41PM -0700, William Zhang wrote: >> Hi, >> >> It seems dt_binding_check reports a false error when run on this >> modified yaml. I picked this simple file just to demostrate this issue. >> Basically I made the interrupts and interrupt-names as optional >> properties. But when there are two interrupts present, then >> interrupt-names are required. However in the example, I don't define >> interrupts and interrupt-name at all, the dt binding check reports error >> that interrupt-names are required: >> >> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> LINT Documentation/devicetree/bindings >> CHKDT Documentation/devicetree/bindings/processed-schema.json >> SCHEMA Documentation/devicetree/bindings/processed-schema.json >> DTEX Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dts >> DTC_CHK Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb >> Documentation/devicetree/bindings/crypto/fsl-imx-scc.example.dtb: crypto@53fac000: 'interrupt-names' is a required property >> From schema: /home/william/projects/linux/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> >> This does not make sense to me as name is required only when there are >> two interrupts. Can someone familar with this please help to check if I >> miss anything in the yaml file or it is indeed a bug in binding check? >> >> Thanks, >> William >> >> --- >> .../devicetree/bindings/crypto/fsl-imx-scc.yaml | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> index 563a31605d2b..c37a3a64a78c 100644 >> --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml >> @@ -32,11 +32,18 @@ properties: >> clock-names: >> const: ipg >> >> +allOf: >> + - if: >> + properties: >> + interrupts: >> + minItems: 2 > > This is a quirk of json-schema in that the 'if' will be true if the > property is not present. Adding this to the 'if' should fix the problem: > > required: > - interrupts > Thanks Rob! This works! >> + then: >> + required: >> + - interrupt-names >> + >> required: >> - compatible >> - reg >> - - interrupts >> - - interrupt-names >> - clocks >> - clock-names >> >> @@ -49,6 +56,4 @@ examples: >> reg = <0x53fac000 0x4000>; >> clocks = <&clks 111>; >> clock-names = "ipg"; >> - interrupts = <49>, <50>; >> - interrupt-names = "scm", "smn"; >> }; >> -- >> 2.34.1 >> > >
diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml index 563a31605d2b..c37a3a64a78c 100644 --- a/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-scc.yaml @@ -32,11 +32,18 @@ properties: clock-names: const: ipg +allOf: + - if: + properties: + interrupts: + minItems: 2 + then: + required: + - interrupt-names + required: - compatible - reg - - interrupts - - interrupt-names - clocks - clock-names @@ -49,6 +56,4 @@ examples: reg = <0x53fac000 0x4000>; clocks = <&clks 111>; clock-names = "ipg"; - interrupts = <49>, <50>; - interrupt-names = "scm", "smn"; };