Messages in this thread | | | Date | Sun, 15 May 2022 13:53:49 +0800 | Subject | Re: [PATCH V4 3/5] arm64: dts: nuvoton: Add initial support for MA35D1 | From | Jacky Huang <> |
| |
On 2022/5/13 下午 02:57, Krzysztof Kozlowski wrote: > 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.
I would modify it as
clock-hxt: clock-hspd-ext-crystal
> >>>> + 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@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@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.
OK, I will remove the "simple-mfd" from sys node.
>>> 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
Sure, thank you for reminding.
Sincerely,.
Jacky Huang
| |