Re: [PATCH 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml

On 04.05.2023 15:47, Krzysztof Kozlowski wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On 04/05/2023 08:07, Claudiu Beznea wrote:

quoted

Convert Atmel PMC documentation to yaml.

Signed-off-by: Claudiu Beznea <redacted>

Thank you for your patch. There is something to discuss/improve.

quoted

diff --git a/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
new file mode 100644
index 000000000000..c4023c3a85f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml

@@ -0,0 +1,161 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/clock/atmel,at91rm9200-pmc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes from both.

quoted

+
+title: Atmel Power Management Controller (PMC)
+
+maintainers:
+  - Claudiu Beznea [off-list ref]
+
+description:
+  The power management controller optimizes power consumption by controlling all
+  system and user peripheral clocks. The PMC enables/disables the clock inputs
+  to many of the peripherals and to the processor.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: atmel,at91sam9260-pmc

Why this is separate, not part of bottom enum?

Current device trees uses the following compatibles (among others):
- "atmel,at91sam9260-pmc, "syscon" or
- "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc, "syscon"

I haven't found another way to make dtbs_check happy.
Is there another way for this?

quoted

+          - const: syscon

quoted

+      - items:
+          - enum:
+              - atmel,at91sam9g20-pmc
+          - enum:
+              - atmel,at91sam9260-pmc

This should be const. You cannot have here different compatibles.

quoted

+          - const: syscon
+      - items:
+          - enum:
+              - atmel,at91sam9g15-pmc
+              - atmel,at91sam9g25-pmc
+              - atmel,at91sam9g35-pmc
+              - atmel,at91sam9x25-pmc
+              - atmel,at91sam9x35-pmc
+          - enum:
+              - atmel,at91sam9x5-pmc
+          - const: syscon
+      - items:
+          - enum:
+              - atmel,at91sam9g45-pmc
+              - atmel,at91sam9n12-pmc
+              - atmel,at91sam9rl-pmc
+              - atmel,at91rm9200-pmc

Order by name?

quoted

+              - atmel,sama5d4-pmc
+              - atmel,sama5d3-pmc
+              - atmel,sama5d2-pmc
+              - microchip,sam9x60-pmc
+              - microchip,sama7g5-pmc
+          - const: syscon
+
+  reg:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 2

Explain what the cells are for in description. Having '2' for clock
controller is not obvious.

quoted

+
+  clocks:
+    minItems: 2
+    maxItems: 3
+
+  clock-names:
+    minItems: 2
+    maxItems: 3
+
+  interrupts:
+    maxItems: 1
+
+  atmel,osc-bypass:
+    type: boolean
+    description: set when a clock signal is directly provided on XIN
+
+

Just one blank line.

quoted

+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#clock-cells"
+  - clocks
+  - clock-names

Keep the same order here as they appear in properties:.

quoted

+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - microchip,sam9x60-pmc
+              - microchip,sama7g5-pmc
+    then:
+      properties:
+        clocks:
+          minItems: 3
+          maxItems: 3
+        clock-names:
+          items:
+            - const: td_slck
+            - const: md_slck
+            - const: main_xtal
+      required:
+        - clock-names
+        - clocks

Drop required: here. It's already in top-level. Same in places below.

quoted

+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - atmel,at91rm9200-pmc
+              - atmel,at91sam9260-pmc
+              - atmel,at91sam9g20-pmc
+    then:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+        clock-names:
+          items:
+            - const: slow_xtal
+            - const: main_xtal
+      required:
+        - clock-names
+        - clocks
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - atmel,sama5d4-pmc
+              - atmel,sama5d3-pmc
+              - atmel,sama5d2-pmc
+    then:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+        clock-names:
+          items:
+            - const: slow_clk
+            - const: main_xtal
+      required:
+        - clock-names
+        - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    pmc: clock-controller@f0018000 {
+        compatible = "atmel,sama5d4-pmc", "syscon";
+        reg = <0xf0018000 0x120>;
+        interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;

interrupt looks a bit odd. Are you sure it is correct?

This example is from SAMA5D4 SoC which uses a vendor specific interrupt
controller (Atmel AIC) where:
- 1st cell is the interrupt number
- 2nd cell is the interrupt type (level/edge sensitive)
- 3rd cell is the IRQ priority

Thank you,
Claudiu

quoted

+        #clock-cells = <2>;
+        clocks = <&clk32k>, <&main_xtal>;
+        clock-names = "slow_clk", "main_xtal";
+    };
+
+...

Best regards,
Krzysztof

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