| Message ID | c48277625f0ab5afc86d89deb1b87537e9c592f6.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: > arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: 'oneOf' conditional failed, one must be fixed: > ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too long > 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] > 'simple-mfd' was expected > 'ingenic,jz4760-tcu' was expected Trim it a bit... > From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml You need to explain this. You're changing the effective compatible of the device and doing so based only on schema warning does not look enough. Please write real reason instead of this fat warning, e.g. that both devices are actually compatible and this has no real effect except schema checks. Best regards, Krzysztof
Hi Krzysztof, Le sam., avril 9 2022 at 13:11:48 +0200, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> a écrit : > On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >> arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: >> 'oneOf' conditional failed, one must be fixed: >> ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too >> long >> 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', >> 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] >> 'simple-mfd' was expected >> 'ingenic,jz4760-tcu' was expected > > Trim it a bit... > >> From schema: >> Documentation/devicetree/bindings/timer/ingenic,tcu.yaml > > You need to explain this. You're changing the effective compatible of > the device and doing so based only on schema warning does not look > enough. Please write real reason instead of this fat warning, e.g. > that > both devices are actually compatible and this has no real effect > except > schema checks. Well, if the schema says that it should use a particular fallback string, then that's what the DTS should use, right? If making the DTS schema-compliant causes breakages, then that means the schema is wrong and should be fixed. Cheers, -Paul
On 09/04/2022 14:24, Paul Cercueil wrote: > Hi Krzysztof, > > Le sam., avril 9 2022 at 13:11:48 +0200, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> a écrit : >> On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >>> arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: >>> 'oneOf' conditional failed, one must be fixed: >>> ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too >>> long >>> 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', >>> 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] >>> 'simple-mfd' was expected >>> 'ingenic,jz4760-tcu' was expected >> >> Trim it a bit... >> >>> From schema: >>> Documentation/devicetree/bindings/timer/ingenic,tcu.yaml >> >> You need to explain this. You're changing the effective compatible of >> the device and doing so based only on schema warning does not look >> enough. Please write real reason instead of this fat warning, e.g. >> that >> both devices are actually compatible and this has no real effect >> except >> schema checks. > > Well, if the schema says that it should use a particular fallback > string, then that's what the DTS should use, right? Or the schema is wrong. :) > If making the DTS schema-compliant causes breakages, then that means > the schema is wrong and should be fixed. Exactly, so the commit needs a bit of explanation why one solution was chosen over the other. BTW, I am not saying that schema or DTS is wrong, just that commit is not explained enough. Best regards, Krzysztof
> Am 09.04.2022 um 13:11 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >> arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: 'oneOf' conditional failed, one must be fixed: >> ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too long >> 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] >> 'simple-mfd' was expected >> 'ingenic,jz4760-tcu' was expected > > Trim it a bit... > >> From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml > > You need to explain this. You're changing the effective compatible of > the device and doing so based only on schema warning does not look > enough. Please write real reason instead of this fat warning, e.g. that > both devices are actually compatible and this has no real effect except > schema checks. both use jz4740_soc_info / jz4770_soc_info and there is no ingenic,jz4780-tcu... So it doesn't change function, just makes it fit to the bindings. We could solve it differently add ingenic,jz4780-tcu to bindings and the driver compatible table.
On 09/04/2022 15:03, H. Nikolaus Schaller wrote: > > >> Am 09.04.2022 um 13:11 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: >> >> On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >>> arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: 'oneOf' conditional failed, one must be fixed: >>> ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too long >>> 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] >>> 'simple-mfd' was expected >>> 'ingenic,jz4760-tcu' was expected >> >> Trim it a bit... >> >>> From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml >> >> You need to explain this. You're changing the effective compatible of >> the device and doing so based only on schema warning does not look >> enough. Please write real reason instead of this fat warning, e.g. that >> both devices are actually compatible and this has no real effect except >> schema checks. > > both use jz4740_soc_info / jz4770_soc_info and there is no ingenic,jz4780-tcu... > So it doesn't change function, just makes it fit to the bindings. > > We could solve it differently add ingenic,jz4780-tcu to bindings and the > driver compatible table. Just please use it in commit msg instead of or next to the warning. Don't focus on the bindings check but focus on actual hardware and its description. Best regards, Krzysztof
> Am 09.04.2022 um 14:38 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 09/04/2022 14:24, Paul Cercueil wrote: >> Hi Krzysztof, >> >> Le sam., avril 9 2022 at 13:11:48 +0200, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> a écrit : >>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >>>> arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: >>>> 'oneOf' conditional failed, one must be fixed: >>>> ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too >>>> long >>>> 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', >>>> 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] >>>> 'simple-mfd' was expected >>>> 'ingenic,jz4760-tcu' was expected >>> >>> Trim it a bit... >>> >>>> From schema: >>>> Documentation/devicetree/bindings/timer/ingenic,tcu.yaml >>> >>> You need to explain this. You're changing the effective compatible of >>> the device and doing so based only on schema warning does not look >>> enough. Please write real reason instead of this fat warning, e.g. >>> that >>> both devices are actually compatible and this has no real effect >>> except >>> schema checks. >> >> Well, if the schema says that it should use a particular fallback >> string, then that's what the DTS should use, right? > > Or the schema is wrong. :) > >> If making the DTS schema-compliant causes breakages, then that means >> the schema is wrong and should be fixed. Well, in this case making the DTS fit the schema does not break. So why think or explain why the schema is right? > > Exactly, so the commit needs a bit of explanation why one solution was > chosen over the other. BTW, I am not saying that schema or DTS is wrong, > just that commit is not explained enough. > > Best regards, > Krzysztof
> Am 09.04.2022 um 15:11 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 09/04/2022 15:03, H. Nikolaus Schaller wrote: >> >> >>> Am 09.04.2022 um 13:11 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: >>> >>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >>>> arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: 'oneOf' conditional failed, one must be fixed: >>>> ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too long >>>> 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] >>>> 'simple-mfd' was expected >>>> 'ingenic,jz4760-tcu' was expected >>> >>> Trim it a bit... >>> >>>> From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml >>> >>> You need to explain this. You're changing the effective compatible of >>> the device and doing so based only on schema warning does not look >>> enough. Please write real reason instead of this fat warning, e.g. that >>> both devices are actually compatible and this has no real effect except >>> schema checks. >> >> both use jz4740_soc_info / jz4770_soc_info and there is no ingenic,jz4780-tcu... >> So it doesn't change function, just makes it fit to the bindings. >> >> We could solve it differently add ingenic,jz4780-tcu to bindings and the >> driver compatible table. > > Just please use it in commit msg instead of or next to the warning. > Don't focus on the bindings check but focus on actual hardware and its > description. Well, again, my assumption is that bindings and .yaml files formally describe the actual hardware components. And they have been reviewed. So they have a higher level of authority than any current driver or .dts implementation. Unless there is evidence that the bindings are wrong. I.e. if the bindings feel right why is there a need to argue for that? It is like test-driven development model. There you have to write code that passes the tests. Not argue against the tests. BR and thanks, Nikolaus
On 09/04/2022 15:18, H. Nikolaus Schaller wrote: > > Well, again, my assumption is that bindings and .yaml files formally describe the actual > hardware components. And they have been reviewed. The bindings try to describe it. They are pretty often incomplete or might have mistakes. The true reason of doing a change is not that some tool tells you "do like this". The true reason is because the change properly describes hardware. > > So they have a higher level of authority than any current driver or .dts implementation. > Unless there is evidence that the bindings are wrong. This is just a tool, not an authority. > I.e. if the bindings feel right why is there a need to argue for that? Because doing things "just because bindings told me" hides the true explanation and makes the code review, code management more difficult. Later person will look at this and wonder why this was done like this. If you write "because some tool me" this is not a good help. But if you write "because hardware is like this exactly" this is proper comment. > > It is like test-driven development model. There you have to write code that passes > the tests. Not argue against the tests. Again, don't focus on the tool... Tool is just a tool... Best regards, Krzysztof
> Am 09.04.2022 um 15:22 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 09/04/2022 15:18, H. Nikolaus Schaller wrote: >> >> Well, again, my assumption is that bindings and .yaml files formally describe the actual >> hardware components. And they have been reviewed. > > The bindings try to describe it. They are pretty often incomplete or > might have mistakes. Indeed they have. But what If I have found that they are right. Why should I comment on that? It should at least be the default assumption. > The true reason of doing a change is not that some > tool tells you "do like this". The true reason is because the change > properly describes hardware. > >> >> So they have a higher level of authority than any current driver or .dts implementation. >> Unless there is evidence that the bindings are wrong. > > This is just a tool, not an authority. > >> I.e. if the bindings feel right why is there a need to argue for that? > > Because doing things "just because bindings told me" hides the true > explanation and makes the code review, code management more difficult. Well, I always wonder why schemas were done that way they were done since their introduction. If I would write down commetns every time nobody would be happy... > Later person will look at this and wonder why this was done like this. > If you write "because some tool me" this is not a good help. But if you > write "because hardware is like this exactly" this is proper comment. > >> >> It is like test-driven development model. There you have to write code that passes >> the tests. Not argue against the tests. > > Again, don't focus on the tool... Tool is just a tool... A tool I can't ignore because Rob's robot tells me it is "the truth"... BR and thanks, Nikolaus
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi index 6fb6229c3186b..6027f14c393e3 100644 --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi @@ -91,7 +91,7 @@ rng: rng@d8 { tcu: timer@10002000 { compatible = "ingenic,jz4780-tcu", - "ingenic,jz4770-tcu", + "ingenic,jz4760-tcu", "simple-mfd"; reg = <0x10002000 0x1000>; #address-cells = <1>;
arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: 'oneOf' conditional failed, one must be fixed: ['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too long 'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu'] 'simple-mfd' was expected 'ingenic,jz4760-tcu' was expected From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)