diff mbox series

[01/18] MIPS: DTS: jz4780: remove cpu clock-names as reported by dtbscheck

Message ID 064a1f029a82ea3895109efe8fb1f472581d4581.1649443080.git.hns@goldelico.com
State New
Headers show
Series MIPS: DTS: fix some findings by "make ci20_defconfig dt_binding_check dtbs_check" | expand

Commit Message

H. Nikolaus Schaller April 8, 2022, 6:37 p.m. UTC
arch/mips/boot/dts/ingenic/ci20.dtb: cpu@0: clock-names does not match any of the regexes: pinctrl-[0-9]+
	From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: cpu@1: clock-names does not match any of the regexes: pinctrl-[0-9]+
	From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 --
 1 file changed, 2 deletions(-)

Comments

Krzysztof Kozlowski April 9, 2022, 11:07 a.m. UTC | #1
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> arch/mips/boot/dts/ingenic/ci20.dtb: cpu@0: clock-names does not match any of the regexes: pinctrl-[0-9]+
> 	From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
> arch/mips/boot/dts/ingenic/ci20.dtb: cpu@1: clock-names does not match any of the regexes: pinctrl-[0-9]+
> 	From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml

No need to put the same warning twice (cpu index really does not
matter). One warning is enough.

Fixing warnings is good, but what if the property should be there and
the bindings are not correct? If you know the answer, please add it to
the commit msg.

This applies to all your patches. Blind fixing of DTS warnings my
produce incorrect results. :(


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof
Krzysztof Kozlowski April 9, 2022, 1:10 p.m. UTC | #2
On 09/04/2022 15:02, H. Nikolaus Schaller wrote:
> Hi Krzysztof,
> 
>> Am 09.04.2022 um 13:07 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>>
>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>>> arch/mips/boot/dts/ingenic/ci20.dtb: cpu@0: clock-names does not match any of the regexes: pinctrl-[0-9]+
>>> 	From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
>>> arch/mips/boot/dts/ingenic/ci20.dtb: cpu@1: clock-names does not match any of the regexes: pinctrl-[0-9]+
>>> 	From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
>>
>> No need to put the same warning twice (cpu index really does not
>> matter). One warning is enough.
> 
> Well, how can you as a reviewer see immediately that dtbscheck
> warns for both instances and that they are both really fixed by the
> complete patch?
> 
> Assume there were only the first one mentioned and the second one
> forgotten to fix...
> 
> If it is not included here (completely), you have to run dtbscheck 
> yourself to see that there are two instances involved. Or deduce it
> from looking into the source file.
> 
> In both cases much more work for you or other reviewers.
> 
> This is why I have kept this redundancy. One commit hunk for every
> warning line.
> 
> And it may only look redundant in the commit message, not in code 
> where it would really hurt to have duplication.

Sorry, but the second warning is obvious. It really does not bring any
information and better to keep things simple and concise. Concise helps
to read/understand the commit.

> 
>>
>> Fixing warnings is good, but what if the property should be there and
>> the bindings are not correct? If you know the answer, please add it to
>> the commit msg.
> 
> It has of course been tested on real hardware.

Good, but it's not everything. DTS is a description of hardware and we
might want to have the extended description, e.g. with clock-names.

> There was only one case where the bindings seem to be wrong (patch 13/18).
> 
> And usually I simply assume such generic bindings are tested on many
> other platforms and therefore the reference. What are bindings and
> dtbscheck good for if we doubt them as DTS developers?

You should always doubt, knowing how incompletely or poorly bindings
were written or converted (including the ones I was doing myself).

> 
>> This applies to all your patches. Blind fixing of DTS warnings my
>> produce incorrect results. :(
> 
> Every patch of this series has been tested on real hardware to have
> no negative side-effects.

Which is not everything. DTS might be used in other projects - did you
test them as well?


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b998301f179ce..710605df40ed3 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -18,7 +18,6 @@  cpu0: cpu@0 {
 			reg = <0>;
 
 			clocks = <&cgu JZ4780_CLK_CPU>;
-			clock-names = "cpu";
 		};
 
 		cpu1: cpu@1 {
@@ -27,7 +26,6 @@  cpu1: cpu@1 {
 			reg = <1>;
 
 			clocks = <&cgu JZ4780_CLK_CORE1>;
-			clock-names = "cpu";
 		};
 	};