Re: [PATCH RFC v2 3/5] dt-bindings: clock: merge all hisilicon clock bindings to hisilicon,clock-reset-generator

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/20/2024 6:14 PM, Krzysztof Kozlowski wrote:
On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen <forbidden405@xxxxxxxxxxx>

We don't need so many separated and duplicated dt-binding files. Merge
them all and convert them to YAML.
What was exactly duplicated? You created unspecific, lose binding...

You can take at the drivers at drivers/clk/hisilicon. All of them use the same sets of APIs to register a few clocks and resets. That's why i think they should be merged.



Why this is RFC? RFC means we should not review.

Signed-off-by: Yang Xiwen <forbidden405@xxxxxxxxxxx>
---
  .../devicetree/bindings/clock/hi3660-clock.txt     |  47 -------
  .../devicetree/bindings/clock/hi3670-clock.txt     |  43 -------
  .../devicetree/bindings/clock/hi6220-clock.txt     |  52 --------
  .../devicetree/bindings/clock/hisi-crg.txt         |  50 --------
  .../clock/hisilicon,clock-reset-generator.yaml     | 139 +++++++++++++++++++++
  .../clock/hisilicon,hi3559av100-clock.yaml         |  59 ---------
  6 files changed, 139 insertions(+), 251 deletions(-)


diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
new file mode 100644
index 000000000000..d37cd892473e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/hisilicon,clock-reset-generator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon SOC Clock and Reset Generator (CRG) module
+
+maintainers:
+  - Yang Xiwen <forbidden405@xxxxxxxxxxx>
+
+description: |
+  Hisilicon SOC clock control module which supports the clocks, resets and
+  power domains on various SoCs.
+
+properties:
+  compatible:
+    minItems: 1
No, it does not work like that. Compatibles are fixed, not fluid. It's
quite a hint that your merging is wrong approach.


+    items:
+      - enum:
+          - hisilicon,hi3559av100-clock
+          - hisilicon,hi3559av100-shub-clock
+          - hisilicon,hi3660-crgctrl
+          - hisilicon,hi3660-pctrl
+          - hisilicon,hi3660-pmuctrl
+          - hisilicon,hi3660-sctrl
+          - hisilicon,hi3660-iomcu
+          - hisilicon,hi3660-stub-clk
+          - hisilicon,hi3670-crgctrl
+          - hisilicon,hi3670-pctrl
+          - hisilicon,hi3670-pmuctrl
+          - hisilicon,hi3670-sctrl
+          - hisilicon,hi3670-iomcu
+          - hisilicon,hi3670-media1-crg
+          - hisilicon,hi3670-media2-crg
+          - hisilicon,hi6220-acpu-sctrl
+          - hisilicon,hi6220-aoctrl
+          - hisilicon,hi6220-sysctrl
+          - hisilicon,hi6220-mediactrl
+          - hisilicon,hi6220-pmctrl
+          - hisilicon,hi6220-stub-clk
+          - hisilicon,hi3516cv300-crg
+          - hisilicon,hi3516cv300-sysctrl
+          - hisilicon,hi3519-crg
+          - hisilicon,hi3798cv200-crg
+          - hisilicon,hi3798cv200-sysctrl
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+
+  '#reset-cells':
+    enum: [1, 2]
+    description: |
Previous bindings has only 2. Your patch is difficult to review and
understand.

+      First cell is reset request register offset.
+      Second cell is bit offset in reset request register.
All of these are reset controllers? I doubt.

+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
All of these have children? No, sorry, but this merging does not make
any sense.

+
+  mboxes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      Phandle to the mailbox for sending msg to MCU
+      (See ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
+
+  mbox-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: |
+      Names of the mailboxes.
+
+  hisilicon,hi6220-clk-sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      Phandle to the syscon managing the SoC internal sram
+      the driver needs using the sram to pass parameters for frequency change.
+
+  reset-controller:
+    type: object
+    description: |
+      Reset controller for Hi3798CV200 GMAC module
+
+required:
+  - compatible
+  - '#clock-cells'
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              enum:
+                - hisilicon,hi3798cv200-crg
+    then:
+      properties:
+        reset-controller: false
+  - oneOf:
+      - required:
+          - hisilicon,hi6220-clk-sram
+      - required:
+          - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/hi3559av100-clock.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@12010000 {
+            compatible = "hisilicon,hi3559av100-clock";
+            #clock-cells = <1>;
+            #reset-cells = <2>;
+            reg = <0x0 0x12010000 0x0 0x10000>;
+        };
+    };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/hi3660-clock.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@fff35000 {
+            compatible = "hisilicon,hi3660-crgctrl", "syscon";
+            reg = <0x0 0xfff35000 0x0 0x1000>;
+            #clock-cells = <1>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml b/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
deleted file mode 100644
index 3ceb29cec704..000000000000
--- a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
+++ /dev/null
@@ -1,59 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/clock/hisilicon,hi3559av100-clock.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
NAK, not related patch.


Okay. I'll revert most of the changes here. Maybe i should only convert hisi-crg.txt to yaml. That's what i really cares.



Please split all your patches into logical chunks.

Please read submitting-patches *BEFORE SENDING* further submissions.

Best regards,
Krzysztof


--
Regards,
Yang Xiwen





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux