| Message ID | cover.1649443080.git.hns@goldelico.com |
|---|---|
| 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: nemc@13410000: $nodename:0: 'nemc@13410000' does not match '^memory-controller@[0-9a-f]+$' > From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.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(-) > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 08/04/2022 20:38, H. Nikolaus Schaller wrote: > arch/mips/boot/dts/ingenic/ci20.dtb: bluetooth: vcc-supply does not match any of the regexes: pinctrl-[0-9]+ > From schema: Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > arch/mips/boot/dts/ingenic/ci20.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts > index dc587b4b36009..8a120f9374331 100644 > --- a/arch/mips/boot/dts/ingenic/ci20.dts > +++ b/arch/mips/boot/dts/ingenic/ci20.dts > @@ -207,7 +207,7 @@ &uart2 { > bluetooth { > compatible = "brcm,bcm4330-bt"; > reset-gpios = <&gpf 8 GPIO_ACTIVE_HIGH>; > - vcc-supply = <&wlan0_power>; > + vbat-supply = <&wlan0_power>; Could be also vddio... Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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
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: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.
> Am 09.04.2022 um 13:14 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >> arch/mips/boot/dts/ingenic/ci20.dtb: rtc@10003000: compatible: 'oneOf' conditional failed, one must be fixed: >> ['ingenic,jz4780-rtc'] is too short >> 'ingenic,jz4780-rtc' is not one of ['ingenic,jz4740-rtc', 'ingenic,jz4760-rtc'] >> 'ingenic,jz4725b-rtc' was expected >> From schema: Documentation/devicetree/bindings/rtc/ingenic,rtc.yaml > > Why? Maybe the schema is wrong. These devices might not be compatible. Here you may be right that the .yaml is wrong. Paul?
> Am 09.04.2022 um 13:18 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dmas' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dma-names' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dmas' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dma-names' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dmas' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dma-names' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dmas' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dma-names' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dmas' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dma-names' is a required property >> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >> arch/mips/boot/dts/ingenic/ci20.dtb: i2c@10050000: 'dmas' is a required property > > All these warnings are the same two warnings... See my earlier explanation that without them you can't verify by just reading commit message and diff that all existing warnings have been addressed.
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
On 09/04/2022 15:07, H. Nikolaus Schaller wrote: > > >> Am 09.04.2022 um 13:18 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: >> >> On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dmas' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dma-names' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dmas' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dma-names' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dmas' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dma-names' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dmas' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dma-names' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dmas' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dma-names' is a required property >>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>> arch/mips/boot/dts/ingenic/ci20.dtb: i2c@10050000: 'dmas' is a required property >> >> All these warnings are the same two warnings... > > See my earlier explanation that without them you can't verify by just reading commit message > and diff that all existing warnings have been addressed. Which does not make sense and there is no need... Automation does it (see Rob's tools). Don't make human life more difficult... Best regards, Krzysztof
> Am 09.04.2022 um 15:16 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: > > On 09/04/2022 15:07, H. Nikolaus Schaller wrote: >> >> >>> Am 09.04.2022 um 13:18 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: >>> >>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote: >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dmas' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dma-names' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dmas' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dma-names' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dmas' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dma-names' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dmas' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dma-names' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dmas' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dma-names' is a required property >>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml >>>> arch/mips/boot/dts/ingenic/ci20.dtb: i2c@10050000: 'dmas' is a required property >>> >>> All these warnings are the same two warnings... >> >> See my earlier explanation that without them you can't verify by just reading commit message >> and diff that all existing warnings have been addressed. > > Which does not make sense and there is no need... Automation does it > (see Rob's tools). Don't make human life more difficult... Ok, you are right. If you apply this patch and then run dtbscheck again, there would be a warning left over. But may I honestly ask why you review the commits and read the commit message at all? You could simply ignore it... And it would be easier for both of us to leave it completely to Rob's tools :) BR and thanks, Nikolaus
On 09/04/2022 15:26, H. Nikolaus Schaller wrote: >> >> Which does not make sense and there is no need... Automation does it >> (see Rob's tools). Don't make human life more difficult... > > Ok, you are right. If you apply this patch and then run dtbscheck again, there would be > a warning left over. > > But may I honestly ask why you review the commits and read the commit message at all? > You could simply ignore it... And it would be easier for both of us to leave it completely > to Rob's tools :) > I am not reading it. :) It takes more effort to scroll to the actual contents. Best regards, Krzysztof