Re: [PATCH 09/13] m68k: coldfire: add devicetree for mcf5475-evb platform

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/02/2025 09:23, Greg Ungerer wrote:
> Hi Krzysztof,
> 
> On 19/2/25 17:06, Krzysztof Kozlowski wrote:
>> On 18/02/2025 13:46, Greg Ungerer wrote:
>>> Add a simple devicetree to support the Freescale MCF5475-EVB platform.
>>>
>>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
>>> ---
>>>   arch/m68k/boot/dts/Makefile       |  1 +
>>>   arch/m68k/boot/dts/mcf5475evb.dts | 55 +++++++++++++++++++++++++++++++
>>>   2 files changed, 56 insertions(+)
>>>   create mode 100644 arch/m68k/boot/dts/mcf5475evb.dts
>>>
>>> diff --git a/arch/m68k/boot/dts/Makefile b/arch/m68k/boot/dts/Makefile
>>> index d69e37b48558..f0e9643e6b71 100644
>>> --- a/arch/m68k/boot/dts/Makefile
>>> +++ b/arch/m68k/boot/dts/Makefile
>>> @@ -1,4 +1,5 @@
>>>   # SPDX-License-Identifier: GPL-2.0
>>>   
>>>   dtb-y += mcf5208evb.dtb
>>> +dtb-y += mcf5475evb.dtb
>>>   
>>> diff --git a/arch/m68k/boot/dts/mcf5475evb.dts b/arch/m68k/boot/dts/mcf5475evb.dts
>>> new file mode 100644
>>> index 000000000000..247a938bfafd
>>> --- /dev/null
>>> +++ b/arch/m68k/boot/dts/mcf5475evb.dts
>>> @@ -0,0 +1,55 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/dts-v1/;
>>> +
>>> +/ {
>>> +	model = "Freescale MCF5475EVB";
>>> +	compatible = "mcf5475evb";
>>
>> Incorrect format.
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. After that,
>> run also `scripts/checkpatch.pl --strict` and (probably) fix more
>> warnings. Some warnings can be ignored, especially from --strict run,
>> but the code here looks like it needs a fix. Feel free to get in touch
>> if the warning is not clear.
> 
> If that is an incorrect format then checkpatch is not reporting it.

There are two issues here.
1. Incorrect format: see DT spec or any other example.
2. Below checkpatch warning.

> 
> Checkpatch here (even --strict) only reports that it is undocumented:
> 
>      WARNING: DT compatible string "mcf5475evb" appears un-documented -- check ./Documentation/devicetree/bindings/
>      #36: FILE: arch/m68k/boot/dts/mcf5475evb.dts:6:
>      +	compatible = "mcf5475evb";
> 
> Which I totally expect at this point. As pointed out in the cover letter this
> work is very much an incomplete proof-of-concept still. Much work still to do here.


Patch should be marked as RFC with that explanation in cover letter, so
you won't get such review comments.

>>> +	#address-cells = <1>;
>>> +	#size-cells = <1>;
>>> +	interrupt-parent = <&intc>;
>>> +
>>> +	coreclk: clock-266000000 {
>>> +		#clock-cells = <0>;
>>> +		compatible = "fixed-clock";
>>> +		clock-frequency = <266000000>;
>>> +	};
>>> +
>>> +	intc: interrupt-controller@ff000700 {
>>> +		compatible = "fsl,intc-2";
>>> +		interrupt-controller;
>>> +		#interrupt-cells = <1>;
>>> +		reg = <0xff000700 0x80>;
>>> +	};
>>> +
>>> +	uart0: serial@ff008600 {
>>> +		compatible = "fsl,mcfuart";
>>> +		reg = <0xff008600 0x80>;
>>> +		interrupts = <99>;
>>> +		clocks = <&coreclk>;
>>> +		status = "okay";
>>
>> Did you disable it anywhere?
> 
> No.


So no need for that status.

Best regards,
Krzysztof