Hi, Jerome


Thanks for your review.


On 2025/1/14 1:50, Jerome Brunet wrote:

[ EXTERNAL EMAIL ]

On Fri 10 Jan 2025 at 09:54, Rob Herring [off-list ref] wrote:

quoted

On Wed, Jan 08, 2025 at 05:40:21PM +0800, Jian Hu wrote:

quoted

Add DT bindings for the PLL clock controller of the Amlogic T7 SoC family.

Signed-off-by: Jian Hu <redacted>
---
  .../bindings/clock/amlogic,t7-pll-clkc.yaml   | 103 ++++++++++++++++++
  .../dt-bindings/clock/amlogic,t7-pll-clkc.h   |  57 ++++++++++
  2 files changed, 160 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml
  create mode 100644 include/dt-bindings/clock/amlogic,t7-pll-clkc.h

diff --git
a/Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml
b/Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml
new file mode 100644
index 000000000000..fd0323678d37

--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml

@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+# Copyright (C) 2024 Amlogic, Inc. All rights reserved
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,t7-pll-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic T7 PLL Clock Control Controller
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+  - Jerome Brunet <jbrunet@baylibre.com>
+  - Jian Hu <jian.hu@amlogic.com>
+  - Xianwei Zhao <xianwei.zhao@amlogic.com>
+
+if:

Move this after 'required' section.

Generally we put 'if' under 'allOf' because we're likely to have another
if/then schema on the next compatible added. If you don't think this
binding will ever get used on another chip, then it is fine as-is.

quoted

+  properties:
+    compatible:
+      contains:
+        const: amlogic,t7-pll-mclk
+
+then:
+  properties:
+    clocks:
+      items:
+        - description: mclk pll input oscillator gate
+        - description: 24M oscillator input clock source for mclk_sel_0
+        - description: fix 50Mhz input clock source for mclk_sel_0

The rate is whatever the clock will actually be. Better not to mention
it in this doc.

OK, I will remove the rate here.

quoted

quoted

+
+    clock-names:
+      items:

one being "input" and other suffixed "_in" looks really odd

quoted

quoted

+        - const: input
+        - const: mclk_in0
+        - const: mclk_in1

or just in0, in1, in2 if you are going with Rob's suggestion.
Having "mclk_" in the top level would be confusing.

Ok, I will use in0/in1/in2 for them.

--
2.47.1

--
Jerome