[PATCH V4 3/5] arm64: dts: nuvoton: Add initial support for MA35D1

[Krzysztof Kozlowski]

On 13/05/2022 08:48, Jacky Huang wrote:
>>> +
>>> +	hxt_24m: hxt_24mhz {
>> No underscores in node name. Generic node names, so "clock-X" or
>> "clock-some-suffix"
> 
> OK, I will modify it as
>   hxt-24m: hxt-24mhz

No, it is not a generic node name. Please read my reply again.

> 
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>> This does not look like property of SoC. Where is this clock defined? In
>> the SoC or on the board?
> 
> It's an external crystal on the board.
> I add this node, because it's the clock source of clock controller.
> It always present on all ma35d1 boards.
> 
>      clk: clock-controller at 40460200 {
>          compatible = "nuvoton,ma35d1-clk";
>          reg = <0x0 0x40460200 0x0 0x100>;
>          #clock-cells = <1>;
>          clocks = <&hxt_24m>;
>          clock-names = "HXT_24MHz";
> ...
> 
>>> +		clock-output-names = "HXT_24MHz";
>>> +	};
>>> +
>>> +	timer {
>>> +		compatible = "arm,armv8-timer";
>>> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
>>> +					  IRQ_TYPE_LEVEL_LOW)>,
>>> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
>>> +					  IRQ_TYPE_LEVEL_LOW)>,
>>> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
>>> +					  IRQ_TYPE_LEVEL_LOW)>,
>>> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
>>> +					  IRQ_TYPE_LEVEL_LOW)>;
>>> +		clock-frequency = <12000000>;
>>> +	};
>>> +
>>> +	sys: system-controller at 40460000 {
>>> +		compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
>> Why is this a simple-mfd if there are no children here? What do you want
>> to instantiate here?
> 
> It's not a device, but a set of registers for system level control.
> I want to provide a register base mapping for other devices to access 
> system control registers.

This does not explain why you need simple-mfd. simple-mfd is not for
providing a register base mapping for other devices.

> 
>> Where is the nuvoton,ma35d1-sys compatible documented?
> 
> OK, I will add the compatible document in next version.
> 
> 
>>> +		reg = <0x0 0x40460000 0x0 0x400>;
>>> +	};
>>> +
>>> +	reset: reset-controller {
>>> +		compatible = "nuvoton,ma35d1-reset";
>> Also not documented.
> 
> I will also add the document for it.

All of these should fail on checkpatch which points that you either did
not run it or ignored the result.

Please run checkpatch on all your submissions to Linux kernel and be
sure that there is no warning or error.



Best regards,
Krzysztof