| Message ID | 84adfe6237cd4cfd52cb9723416f69926e556e55.1649443080.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MIPS: DTS: fix some findings by "make ci20_defconfig dt_binding_check dtbs_check" | expand |
On 08/04/2022 20:37, H. Nikolaus Schaller wrote: > jz4780-nemc needs to be compatible to simple-mfd as well or we get > > arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed: > ['ingenic,jz4780-nemc', 'simple-mfd'] is too long > 'ingenic,jz4725b-nemc' was expected > 'ingenic,jz4740-nemc' was expected > From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml > index 24f9e19820282..3b1116588de3d 100644 > --- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml > +++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml > @@ -17,7 +17,7 @@ properties: > oneOf: > - enum: > - ingenic,jz4740-nemc > - - ingenic,jz4780-nemc > + - [ ingenic,jz4780-nemc, simple-mfd ] This is not correct representation. If you really need simple-mfd, then this should be a separate item below oneOf. The true question is whether you need simple-mfd. Isn't the binding (and the driver) expected to instantiate its children? > - items: > - const: ingenic,jz4725b-nemc > - const: ingenic,jz4740-nemc Best regards, Krzysztof
Hi Krzysztof, Le sam., avril 9 2022 at 13:26:25 +0200, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> a écrit : > On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >> jz4780-nemc needs to be compatible to simple-mfd as well or we get >> >> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: >> compatible: 'oneOf' conditional failed, one must be fixed: >> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long >> 'ingenic,jz4725b-nemc' was expected >> 'ingenic,jz4740-nemc' was expected >> From schema: >> Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | >> 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git >> a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> index 24f9e19820282..3b1116588de3d 100644 >> --- >> a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> +++ >> b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> @@ -17,7 +17,7 @@ properties: >> oneOf: >> - enum: >> - ingenic,jz4740-nemc >> - - ingenic,jz4780-nemc >> + - [ ingenic,jz4780-nemc, simple-mfd ] > > This is not correct representation. If you really need simple-mfd, > then > this should be a separate item below oneOf. Correct. > The true question is whether you need simple-mfd. Isn't the binding > (and > the driver) expected to instantiate its children? I can explain that one. There is the EFUSE controller located inside the nemc's memory area, and the two are pretty much unrelated, hence the "simple-mfd" compatible string. Cheers, -Paul
On 09/04/2022 14:37, Paul Cercueil wrote: >> The true question is whether you need simple-mfd. Isn't the binding >> (and >> the driver) expected to instantiate its children? > > I can explain that one. There is the EFUSE controller located inside > the nemc's memory area, and the two are pretty much unrelated, hence > the "simple-mfd" compatible string. I saw the efuse children and that's why I asked who is expected to populate them. You said that simple-mfd is required for this, I say no. It should work without simple-mfd... I am kind of repeating myself but I really do not see the need of simple-mfd in the bindings. Best regards, Krzysztof
Le sam., avril 9 2022 at 14:47:23 +0200, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> a écrit : > On 09/04/2022 14:37, Paul Cercueil wrote: >>> The true question is whether you need simple-mfd. Isn't the binding >>> (and >>> the driver) expected to instantiate its children? >> >> I can explain that one. There is the EFUSE controller located inside >> the nemc's memory area, and the two are pretty much unrelated, hence >> the "simple-mfd" compatible string. > > I saw the efuse children and that's why I asked who is expected to > populate them. You said that simple-mfd is required for this, I say > no. > It should work without simple-mfd... > > I am kind of repeating myself but I really do not see the need of > simple-mfd in the bindings. Well, it is a "simple MFD", so I don't see why we can't use the "simple-mfd" compatible. Why would we not want to use it? Besides, if the nemc driver is responsible for populating the efuse device, that means the nemc driver must be enabled for the efuse to work, which is nonsense, the two IP blocks being unrelated. -Paul
On 09/04/2022 14:55, Paul Cercueil wrote: >> >> I saw the efuse children and that's why I asked who is expected to >> populate them. You said that simple-mfd is required for this, I say >> no. >> It should work without simple-mfd... >> >> I am kind of repeating myself but I really do not see the need of >> simple-mfd in the bindings. > > Well, it is a "simple MFD", It's not a simple MFD, it is a memory controller. MFD is a purely Linux/software term, so there are no devices which are MFD. Everything which we model as MFD is actually something else in real life (e.g. PMIC, memory controller, system controller). > so I don't see why we can't use the > "simple-mfd" compatible. Why would we not want to use it? No one said that you cannot. You just might not need... > > Besides, if the nemc driver is responsible for populating the efuse > device, that means the nemc driver must be enabled for the efuse to > work, which is nonsense, the two IP blocks being unrelated. That's actually the explanation I was looking for. It would be nice to use it in commit msg instead of the dtbs_check warning. Best regards, Krzysztof
> Am 09.04.2022 um 13:26 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >> jz4780-nemc needs to be compatible to simple-mfd as well or we get >> >> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed: >> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long >> 'ingenic,jz4725b-nemc' was expected >> 'ingenic,jz4740-nemc' was expected >> From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> index 24f9e19820282..3b1116588de3d 100644 >> --- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> +++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >> @@ -17,7 +17,7 @@ properties: >> oneOf: >> - enum: >> - >> - - ingenic,jz4780-nemc >> + - [ , simple-mfd ] > > This is not correct representation. If you really need simple-mfd, then > this should be a separate item below oneOf. Well, it is valid YAML syntax and seems to be accepted by dtbscheck. > The true question is whether you need simple-mfd. Isn't the binding (and > the driver) expected to instantiate its children? I had expected that but current ingenic,jz4780-nemc code doesn't. Everything is explained in 190607f2d59e174bcf8415efb1bb390737f8d428 But if someone writes a fix for the ingenic,jz4740-nemc driever we can of course live without this patch. And partially revert 190607f2d59e174bcf8415efb1bb390737f8d428
On 09/04/2022 15:09, H. Nikolaus Schaller wrote: > > >> Am 09.04.2022 um 13:26 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: >> >> On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >>> jz4780-nemc needs to be compatible to simple-mfd as well or we get >>> >>> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed: >>> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long >>> 'ingenic,jz4725b-nemc' was expected >>> 'ingenic,jz4740-nemc' was expected >>> From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >>> >>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>> --- >>> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >>> index 24f9e19820282..3b1116588de3d 100644 >>> --- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >>> +++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml >>> @@ -17,7 +17,7 @@ properties: >>> oneOf: >>> - enum: >>> - >>> - - ingenic,jz4780-nemc >>> + - [ , simple-mfd ] >> >> This is not correct representation. If you really need simple-mfd, then >> this should be a separate item below oneOf. > > Well, it is valid YAML syntax and seems to be accepted by dtbscheck. It's not how we code it. Please do not introduce inconsistent - even if valid - blocks. > >> The true question is whether you need simple-mfd. Isn't the binding (and >> the driver) expected to instantiate its children? > > I had expected that but current ingenic,jz4780-nemc code doesn't. Paul provided good reason for the simple-mfd. Use this one instead of dt check warning. DT check warning means nothing, does not bring the actual answer to "why", because it is artificial tool. The answer to "why" is in what Paul wrote. Best regards, Krzysztof
On Fri, 08 Apr 2022 20:37:56 +0200, H. Nikolaus Schaller wrote: > jz4780-nemc needs to be compatible to simple-mfd as well or we get > > arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed: > ['ingenic,jz4780-nemc', 'simple-mfd'] is too long > 'ingenic,jz4725b-nemc' was expected > 'ingenic,jz4740-nemc' was expected > From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > 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: ./Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml:20:23: [warning] too few spaces after comma (commas) dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml: properties:compatible:oneOf:0:enum:1: ['ingenic', 'jz4780-nemc', 'simple-mfd'] is not of type 'string' from schema $id: http://devicetree.org/meta-schemas/string-array.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml: ignoring, error in schema: properties: compatible: oneOf: 0: enum: 1 Documentation/devicetree/bindings/mtd/ingenic,nand.example.dtb:0:0: /example-0/memory-controller@13410000: failed to match any schema with compatible: ['ingenic,jz4780-nemc'] Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.example.dtb:0:0: /example-0/memory-controller@13410000: failed to match any schema with compatible: ['ingenic,jz4780-nemc'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On 09/04/2022 15:18, Krzysztof Kozlowski wrote: > On 09/04/2022 15:09, H. Nikolaus Schaller wrote: (...) >>>> @@ -17,7 +17,7 @@ properties: >>>> oneOf: >>>> - enum: >>>> - >>>> - - ingenic,jz4780-nemc >>>> + - [ , simple-mfd ] >>> >>> This is not correct representation. If you really need simple-mfd, then >>> this should be a separate item below oneOf. >> >> Well, it is valid YAML syntax and seems to be accepted by dtbscheck. Minor update: Well, it is not a valid schema. Rob's checker now confirmed. If you run dt_bindings_check by yourself you will see the error: properties:compatible:oneOf:0:enum:1: ['ingenic', 'jz4780-nemc', 'simple-mfd'] is not of type 'string' Probably because enum expects string, not another enum (so enum inside enum is not correct). If you do not see the error, you might be missing some packages (mentioned in writing-schema + yamllint for a different issue) or your dtschema is old. > > It's not how we code it. Please do not introduce inconsistent - even if > valid - blocks. Best regards, Krzysztof
Hi, > Am 10.04.2022 um 17:35 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 09/04/2022 15:18, Krzysztof Kozlowski wrote: >> On 09/04/2022 15:09, H. Nikolaus Schaller wrote: > > (...) > >>>>> @@ -17,7 +17,7 @@ properties: >>>>> oneOf: >>>>> - enum: >>>>> - >>>>> - - ingenic,jz4780-nemc >>>>> + - [ , simple-mfd ] >>>> >>>> This is not correct representation. If you really need simple-mfd, then >>>> this should be a separate item below oneOf. >>> >>> Well, it is valid YAML syntax and seems to be accepted by dtbscheck. > > Minor update: > Well, it is not a valid schema. Rob's checker now confirmed. If you run > dt_bindings_check by yourself you will see the error: I did run dt_bindings_check. Otherwise I wouldn't have seen that there is a problem at all, before suggesting these changes... > > properties:compatible:oneOf:0:enum:1: ['ingenic', 'jz4780-nemc', > 'simple-mfd'] is not of type 'string' > > Probably because enum expects string, not another enum (so enum inside > enum is not correct). > > If you do not see the error, you might be missing some packages > (mentioned in writing-schema + yamllint for a different issue) or your > dtschema is old. Neither. yamllint etc. are all the most recent versions. Updated just two days ago. But my filter looking for ci20.dtb did not catch it because the report doesn't mention that file. So Rob's robot is more mature than mine... BR, Nikolaus
diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml index 24f9e19820282..3b1116588de3d 100644 --- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml @@ -17,7 +17,7 @@ properties: oneOf: - enum: - ingenic,jz4740-nemc - - ingenic,jz4780-nemc + - [ ingenic,jz4780-nemc, simple-mfd ] - items: - const: ingenic,jz4725b-nemc - const: ingenic,jz4740-nemc
jz4780-nemc needs to be compatible to simple-mfd as well or we get arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed: ['ingenic,jz4780-nemc', 'simple-mfd'] is too long 'ingenic,jz4725b-nemc' was expected 'ingenic,jz4740-nemc' was expected From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)