On 20/06/2023 12:39, wen.ping.teh@intel.com wrote:

quoted

quoted

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

quoted

quoted

+
+  '#clock-cells':
+    const: 1
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'

Keep the same order as in properties:

Will update in V2 patch.

quoted

quoted

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

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

quoted

quoted

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