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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0e72e176-d7b0-ed10-08f3-ba7d4729a931@linaro.org>
Date:   Fri, 13 May 2022 08:57:52 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Jacky Huang <ychuang3@...oton.com>, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, ychuang570808@...il.com
Cc:     robh+dt@...nel.org, sboyd@...nel.org, krzk+dt@...nel.org,
        arnd@...db.de, olof@...om.net, catalin.marinas@....com,
        will@...nel.org, soc@...nel.org, cfli0@...oton.com
Subject: Re: [PATCH V4 3/5] arm64: dts: nuvoton: Add initial support for
 MA35D1

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@...60200 {
>          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@...60000 {
>>> +		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