linux-kernel - Re: [PATCH v4 37/39] ARM: dts: at91: sam9x7: add device tree for SoC

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 for Android: free password hash cracker in your pocket

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

Message-ID: <2f5ee8f2-9d07-471a-92e5-d547fdb9454d@tuxon.dev>
Date: Wed, 6 Mar 2024 10:38:36 +0200
From: claudiu beznea <claudiu.beznea@...on.dev>
To: Varshini.Rajendran@...rochip.com, robh+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 Nicolas.Ferre@...rochip.com, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 37/39] ARM: dts: at91: sam9x7: add device tree for SoC



On 04.03.2024 18:33, Varshini.Rajendran@...rochip.com wrote:
> Hi Claudiu,
> 
> Thanks for your time in reviewing this patch. I will address all your 
> comments in the next version. There are some clarifications provided 
> inline below.
> 
> On 03/03/24 5:54 pm, claudiu beznea wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 23.02.2024 19:30, Varshini Rajendran wrote:
>>> Add device tree file for SAM9X7 SoC family.
>>>
>>> Co-developed-by: Nicolas Ferre <nicolas.ferre@...rochip.com>
>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@...rochip.com>
>>> Signed-off-by: Varshini Rajendran <varshini.rajendran@...rochip.com>
>>> ---
>>> Changes in v4:
>>> - Added pwm node support
>>> - Added microchip,nr-irqs to the interrupt-controller node for the
>>>    driver to fetch the NIRQs
>>> - Dropped USB nodes owing to the discussion here
>>>   https://lore.kernel.org/linux-devicetree/CAL_JsqJ9PrX6fj-EbffeJce09MXs=B7t+KS_kOinxaRx38=WxA@mail.gmail.com/
>>> (Explained elaborartely in the cover letter)
>>> ---
>>>   arch/arm/boot/dts/microchip/sam9x7.dtsi | 1214 +++++++++++++++++++++++
>>>   1 file changed, 1214 insertions(+)
>>>   create mode 100644 arch/arm/boot/dts/microchip/sam9x7.dtsi
>>>
>>> diff --git a/arch/arm/boot/dts/microchip/sam9x7.dtsi b/arch/arm/boot/dts/microchip

[ ... ]

>>> +             reset_controller: reset-controller@...ffe00 {
>>> +                     compatible = "microchip,sam9x7-rstc", "microchip,sam9x60-rstc";
>>> +                     reg = <0xfffffe00 0x10>;
>>> +                     clocks = <&clk32k 0>;
>>> +             };
>>> +
>>> +             power_management: power-management@...ffe10 {
>>
>> Usually the node name for this is poweroff. Any reason you changed it like
>> this?
>>
> Yes Claudiu. Based on the comment given for the version 2.

I think poweroff fits better. Documentation is also using poweroff for node
name. The rest of at91 device uses it. And poweroff is what this controller
does (it is named shutdown controller).

> 
> https://patches.linaro.org/project/linux-mmc/patch/20230623203056.689705-44-varshini.rajendran@microchip.com/#:~:text=Usually%20power%2Dmanagement%20or%20reset%2Dcontroller%20or%20something%20like%20this.
> 
>>> +                     compatible = "microchip,sam9x7-shdwc", "microchip,sam9x60-shdwc";
>>> +                     reg = <0xfffffe10 0x10>;
>>> +                     clocks = <&clk32k 0>;
>>> +                     #address-cells = <1>;
>>> +                     #size-cells = <0>;
>>> +                     atmel,wakeup-rtc-timer;
>>> +                     atmel,wakeup-rtt-timer;
>>> +                     status = "disabled";
>>> +             };
>>> +
>>> +             rtt: rtc@...ffe20 {
>>> +                     compatible = "microchip,sam9x7-rtt", "atmel,at91sam9260-rtt";
>>> +                     reg = <0xfffffe20 0x20>;
>>> +                     interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
>>> +                     clocks = <&clk32k 0>;
>>> +             };
>>> +
>>> +             clk32k: sckc@...ffe50 {
>>
>> Node name should be generic, e.g. clock-controller
>>
>>> +                     compatible = "microchip,sam9x7-sckc", "microchip,sam9x60-sckc";
>>> +                     reg = <0xfffffe50 0x4>;
>>> +                     clocks = <&slow_xtal>;
>>> +                     #clock-cells = <1>;
>>> +             };
>>> +
>>> +             gpbr: syscon@...ffe60 {
>>> +                     compatible = "microchip,sam9x7-gbpr", "atmel,at91sam9260-gpbr", "syscon";
>>
>> microchip,sam9x7-gbpr seems undocummented.
> 
> The patch that adds support is already applied.
> https://lore.kernel.org/linux-arm-kernel/169226306696.928678.2345448260460546641.b4-ty@kernel.org/

Ok, then there is a typo in this patch:
s/microchip,sam9x7-gbpr/microchip,sam9x7-gpbr