On 23/11/2022 14:54, Yu Tu wrote:

Hi Neil,

On 2022/11/23 21:23, Neil Armstrong wrote:

quoted

[ EXTERNAL EMAIL ]

Hi,

On 23/11/2022 12:16, Yu Tu wrote:

quoted

Hi Krzysztof,
     Thank you for your reply.

On 2022/11/23 18:08, Krzysztof Kozlowski wrote:

quoted

[ EXTERNAL EMAIL ]

On 23/11/2022 03:13, Yu Tu wrote:

quoted

Add the S4 PLL clock controller found and bindings in the s4 SoC family.

Signed-off-by: Yu Tu <redacted>
---
  .../bindings/clock/amlogic,s4-pll-clkc.yaml   |  51 +

This is v5 and still bindings are here? Bindings are always separate
patches. Use subject prefixes matching the subsystem (git log --oneline
-- ...).

And this was split, wasn't it? What happened here?!?

Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history.
https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/

Jerome was asking you to send 2 patchsets, one with :
- bindings in separate patches
- drivers in separate patches
and a second with DT changes.

Then when the bindings + clocks patches are merged, a pull request of the bindings
can be done to me so I can merge it with DT.

I may have misunderstood Jerome's advice.So should I follow the V4 patch format and change as Jerome suggested from V3?

Let's wait for his input on this to see if a v4 is needed now or later.

quoted

quoted

quoted

quoted

  MAINTAINERS                                   |   1 +
  drivers/clk/meson/Kconfig                     |  13 +
  drivers/clk/meson/Makefile                    |   1 +
  drivers/clk/meson/s4-pll.c                    | 875 ++++++++++++++++++
  drivers/clk/meson/s4-pll.h                    |  88 ++
  .../dt-bindings/clock/amlogic,s4-pll-clkc.h   |  30 +
  7 files changed, 1059 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
  create mode 100644 drivers/clk/meson/s4-pll.c
  create mode 100644 drivers/clk/meson/s4-pll.h
  create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h

diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
new file mode 100644
index 000000000000..fd517e8ef14f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml

@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Meson S serials PLL Clock Controller
+
+maintainers:
+  - Neil Armstrong <narmstrong@baylibre.com>
+  - Jerome Brunet <jbrunet@baylibre.com>
+  - Yu Tu <yu.hu@amlogic.com>
+

One blank line.

  I will delete this, on next version patch.

quoted

quoted

+
+properties:
+  compatible:
+    const: amlogic,s4-pll-clkc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: xtal
+
+  "#clock-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    clkc_pll: clock-controller@fe008000 {
+      compatible = "amlogic,s4-pll-clkc";
+      reg = <0xfe008000 0x1e8>;
+      clocks = <&xtal>;
+      clock-names = "xtal";
+      #clock-cells = <1>;
+    };

quoted

+#endif /* __MESON_S4_PLL_H__ */

diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h
new file mode 100644
index 000000000000..345f87023886
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h

This belongs to bindings patch, not driver.

quoted

@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
+ * Author: Yu Tu <yu.tu@amlogic.com>
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H
+#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H
+
+/*
+ * CLKID index values
+ */
+
+#define CLKID_FIXED_PLL            1
+#define CLKID_FCLK_DIV2            3

Indexes start from 0 and are incremented by 1. Not by 2.

NAK.

I remember Jerome discussing this with you.You can look at this submission history.
https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ (local)

Historically we did that by only exposing part of the numbers, controlling which
clocks were part of the bindings.

But it seems this doesn't make sens anymore, maybe it would be time to put all the
clock ids in the bindings for this new SoC and break with the previous strategy.

That's OK with me. But I don't know if Jerome thinks it's ok?

Let's wait for his input on that aswell.

Neil

quoted

Neil

quoted

quoted

Best regards,
Krzysztof

.

.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel