linux-kernel - Re: [PATCH 2/4] dt-bindings: clock: Add Intel Agilex5 clocks and resets

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: <ed6f9ab8-9c4e-ec9f-efb7-81974d75f074@linaro.org>
Date:   Tue, 20 Jun 2023 13:06:29 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     wen.ping.teh@...el.com
Cc:     adrian.ho.yin.ng@...el.com, andrew@...n.ch, conor+dt@...nel.org,
        devicetree@...r.kernel.org, dinguyen@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, mturquette@...libre.com,
        netdev@...r.kernel.org, niravkumar.l.rabara@...el.com,
        p.zabel@...gutronix.de, richardcochran@...il.com,
        robh+dt@...nel.org, sboyd@...nel.org
Subject: Re: [PATCH 2/4] dt-bindings: clock: Add Intel Agilex5 clocks and
 resets

On 20/06/2023 12:39, wen.ping.teh@...el.com wrote:
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: intel,agilex5-clkmgr
>>
>>
>> Why "clkmgr", not "clk"? You did not call it Clock manager anywhere in
>> the description or title.
>>
> 
> The register in Agilex5 handling the clock is named clock_mgr.
> Previous IntelSocFPGA, Agilex and Stratix10, are also named clkmgr.

So use it in description.

> 
>>> +
>>> +  '#clock-cells':
>>> +    const: 1
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - '#clock-cells'
>>
>> Keep the same order as in properties:
>>
> 
> Will update in V2 patch.
> 
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  # Clock controller node:
>>> +  - |
>>> +    clkmgr: clock-controller@...10000 {
>>> +      compatible = "intel,agilex5-clkmgr";
>>> +      reg = <0x10d10000 0x1000>;
>>> +      #clock-cells = <1>;
>>> +    };
>>> +...
>>> diff --git a/include/dt-bindings/clock/agilex5-clock.h b/include/dt-bindings/clock/agilex5-clock.h
>>> new file mode 100644
>>> index 000000000000..4505b352cd83
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/agilex5-clock.h
>>
>> Filename the same as binding. Missing vendor prefix, entirely different
>> device name.
>>
> 
> Will change filename to intel,agilex5-clock.h in V2.

Read the comment - same as binding. You did not call binding that way...
unless you rename the binding.

>>
>>> +
>>> +#endif	/* __AGILEX5_CLOCK_H */
>>> diff --git a/include/dt-bindings/reset/altr,rst-mgr-agilex5.h b/include/dt-bindings/reset/altr,rst-mgr-agilex5.h
>>> new file mode 100644
>>> index 000000000000..81e5e8c89893
>>> --- /dev/null
>>> +++ b/include/dt-bindings/reset/altr,rst-mgr-agilex5.h
>>
>> Same filename as binding.
>>
>> But why do you need this file? Your device is not a reset controller.
> 
> Because Agilex5 device tree uses the reset definition from this file.

That's not the correct reason. The binding header has nothing to do with
this device. You miss another patch adding support for your device
(compatible) with this header.

Best regards,
Krzysztof