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

[claudiu beznea]

On 04.03.2024 18:33, Varshini.Rajendran@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
>>> Signed-off-by: Varshini Rajendran <varshini.rajendran@xxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxx/
>>> (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@fffffe00 {
>>> +                     compatible = "microchip,sam9x7-rstc", "microchip,sam9x60-rstc";
>>> +                     reg = <0xfffffe00 0x10>;
>>> +                     clocks = <&clk32k 0>;
>>> +             };
>>> +
>>> +             power_management: power-management@fffffe10 {
>>
>> 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@xxxxxxxxxxxxx/#:~: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@fffffe20 {
>>> +                     compatible = "microchip,sam9x7-rtt", "atmel,at91sam9260-rtt";
>>> +                     reg = <0xfffffe20 0x20>;
>>> +                     interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
>>> +                     clocks = <&clk32k 0>;
>>> +             };
>>> +
>>> +             clk32k: sckc@fffffe50 {
>>
>> 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@fffffe60 {
>>> +                     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@xxxxxxxxxx/

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