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

[wen . ping . teh]

>From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>> From: Niravkumar L Rabara <niravkumar.l.rabara@xxxxxxxxx>
>> 
>> Add clock and reset ID definitions for Intel Agilex5 SoCFPGA
>> 
>> Co-developed-by: Teh Wen Ping <wen.ping.teh@xxxxxxxxx>
>> Signed-off-by: Teh Wen Ping <wen.ping.teh@xxxxxxxxx>
>> Signed-off-by: Niravkumar L Rabara <niravkumar.l.rabara@xxxxxxxxx>
>> ---
>>  .../bindings/clock/intel,agilex5.yaml         |  42 ++++++++
>>  include/dt-bindings/clock/agilex5-clock.h     | 100 ++++++++++++++++++
>>  .../dt-bindings/reset/altr,rst-mgr-agilex5.h  |  79 ++++++++++++++
>>  3 files changed, 221 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/intel,agilex5.yaml
>>  create mode 100644 include/dt-bindings/clock/agilex5-clock.h
>>  create mode 100644 include/dt-bindings/reset/altr,rst-mgr-agilex5.h
>> 
>> diff --git a/Documentation/devicetree/bindings/clock/intel,agilex5.yaml b/Documentation/devicetree/bindings/clock/intel,agilex5.yaml
>> new file mode 100644
>> index 000000000000..e408c52deefa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/intel,agilex5.yaml
>
>Filename matching compatible, so missing "clk"
>

Will update in V2 patch, rename file to intel,agilex5-clk.yaml

>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/intel,agilex5.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Intel SoCFPGA Agilex5 platform clock controller binding
>
>Drop "binding"
>

Will update in V2 patch.

>> +
>> +maintainers:
>> +  - Teh Wen Ping <wen.ping.teh@xxxxxxxxx>
>> +  - Niravkumar L Rabara <niravkumar.l.rabara@xxxxxxxxx>
>> +
>> +description:
>> +  The Intel Agilex5 Clock controller is an integrated clock controller, which
>> +  generates and supplies to all modules.
>
>"generates and supplies" what?

Will change to "generates and supplies clock" in V2 patch.

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

>> +
>> +  '#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@10d10000 {
>> +      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.

>> @@ -0,0 +1,100 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2022, Intel Corporation
>> + */
>
>...
>
>> +
>> +#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.

Best Regards,
Wen Ping