Re: [PATCH] dt-bindings: can: xilinx_can: Convert Xilinx CAN binding to YAML

On 08/02/2022 16:52, Amit Kumar Mahapatra wrote:

Filename: xilinx,can.yaml to match common format.

+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title:
+  Xilinx Axi CAN/Zynq CANPS controller Binding
+
+maintainers:
+  - Appana Durga Kedareswara rao [off-list ref]
+
+properties:
+  compatible:
+    oneOf:
+      - const: xlnx,zynq-can-1.0
+        description: For Zynq CAN controller
+      - const: xlnx,axi-can-1.00.a
+        description: For Axi CAN controller
+      - const: xlnx,canfd-1.0
+        description: For CAN FD controller
+      - const: xlnx,canfd-2.0
+        description: For CAN FD 2.0 controller

This should be enum, not oneOf. Any reason for using oneOf?

+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description: |
+      CAN functional clock phandle
+    maxItems: 2

1. minItems
2. You miss clock-names.

+
+  tx-fifo-depth:
+    description: |
+      CAN Tx fifo depth (Zynq, Axi CAN).

Non-standard fields need type. Here and in all other places.

+
+  rx-fifo-depth:
+    description: |
+      CAN Rx fifo depth (Zynq, Axi CAN, CAN FD in sequential Rx mode)
+
+  tx-mailbox-count:
+    description: |
+      CAN Tx mailbox buffer count (CAN FD)
+
+  rx-mailbox-count:
+    description: |
+      CAN Rx mailbox buffer count (CAN FD in mailbox Rx  mode)
+
+  clock-names:
+    maxItems: 2

Oh... here are the clock-names.... why sorted randomly?

+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: xlnx,zynq-can-1.0
+
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: can_clk
+            - const: pclk
+      required:
+        - tx-fifo-depth
+        - rx-fifo-depth
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: xlnx,axi-can-1.00.a
+
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: can_clk
+            - const: s_axi_aclk
+      required:
+        - tx-fifo-depth
+        - rx-fifo-depth
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: xlnx,canfd-1.0
+
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: can_clk
+            - const: s_axi_aclk
+      required:
+        - tx-mailbox-count
+        - rx-fifo-depth
+
+examples:
+  - |
+    can@e0008000 {
+        compatible = "xlnx,zynq-can-1.0";
+        clocks = <&clkc 19>, <&clkc 36>;
+        clock-names = "can_clk", "pclk";
+        reg = <0xe0008000 0x1000>;
+        interrupts = <0 28 4>;

Isn't it a regular GIC interrupt? If yes, use defines instead of
hard-coded values.

+        interrupt-parent = <&intc>;
+        tx-fifo-depth = <0x40>;
+        rx-fifo-depth = <0x40>;
+    };

Blank line.

+  - |
+    axi-can@40000000 {

Generic node name, so "can".

+        compatible = "xlnx,axi-can-1.00.a";
+        clocks = <&clkc 0>, <&clkc 1>;
+        clock-names = "can_clk","s_axi_aclk" ;
+        reg = <0x40000000 0x10000>;
+        interrupt-parent = <&intc>;
+        interrupts = <0 59 1>;
+        tx-fifo-depth = <0x40>;
+        rx-fifo-depth = <0x40>;
+    };

Blank line.

+  - |
+    canfd@40000000 {

Generic node name, so "can".

+        compatible = "xlnx,canfd-1.0";
+        clocks = <&clkc 0>, <&clkc 1>;
+        clock-names = "can_clk", "s_axi_aclk";
+        reg = <0x40000000 0x2000>;
+        interrupt-parent = <&intc>;
+        interrupts = <0 59 1>;
+        tx-mailbox-count = <0x20>;
+        rx-fifo-depth = <0x20>;
+    };

Best regards,
Krzysztof