Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <0e72e176-d7b0-ed10-08f3-ba7d4729a931@linaro.org>
Date:   Fri, 13 May 2022 08:57:52 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH V4 3/5] arm64: dts: nuvoton: Add initial support for
 MA35D1
Content-Language: en-US
To:     Jacky Huang <ychuang3@nuvoton.com>, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, ychuang570808@gmail.com
Cc:     robh+dt@kernel.org, sboyd@kernel.org, krzk+dt@kernel.org,
        arnd@arndb.de, olof@lixom.net, catalin.marinas@arm.com,
        will@kernel.org, soc@kernel.org, cfli0@nuvoton.com
References: <20220510032558.10304-1-ychuang3@nuvoton.com>
 <20220510032558.10304-4-ychuang3@nuvoton.com>
 <03ac0a67-bd1f-12ca-74f7-8d5b05857ea7@linaro.org>
 <46a55b01-ee9f-604f-72c9-916bc2f02a09@nuvoton.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <46a55b01-ee9f-604f-72c9-916bc2f02a09@nuvoton.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@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.

> 
>> 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