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

[Amit Kumar Kumar Mahapatra]

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski at canonical.com>
> Sent: Thursday, March 10, 2022 10:25 PM
> To: Amit Kumar Kumar Mahapatra <akumarma at xilinx.com>;
> wg at grandegger.com; mkl at pengutronix.de; kuba at kernel.org;
> robh+dt at kernel.org; Appana Durga Kedareswara Rao
> <appanad at xilinx.com>
> Cc: linux-can at vger.kernel.org; netdev at vger.kernel.org;
> devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org; Michal Simek <michals at xilinx.com>; git
> <git at xilinx.com>; Amit Kumar Kumar Mahapatra <akumarma at xilinx.com>
> Subject: Re: [PATCH v3] dt-bindings: can: xilinx_can: Convert Xilinx CAN
> binding to YAML
> 
> On 10/03/2022 16:39, Amit Kumar Mahapatra wrote:
> > Convert Xilinx CAN binding documentation to YAML.
> >
> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
> mahapatra at xilinx.com>
> > ---
> > BRANCH: yaml
> >
> > Changes in v2:
> >  - Added reference to can-controller.yaml
> >  - Added example node for canfd-2.0
> >
> > Changes in v3:
> >  - Changed yaml file name from xilinx_can.yaml to xilinx,can.yaml
> >  - Added "power-domains" to fix dts_check warnings
> >  - Grouped "clock-names" and "clocks" together
> >  - Added type $ref for all non-standard fields
> >  - Defined compatible strings as enum
> >  - Used defines,instead of hard-coded values, for GIC interrupts
> >  - Droped unused labels in examples
> >  - Droped description for standard feilds
> > ---
> >  .../bindings/net/can/xilinx,can.yaml          | 161 ++++++++++++++++++
> >  .../bindings/net/can/xilinx_can.txt           |  61 -------
> >  2 files changed, 161 insertions(+), 61 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> >  delete mode 100644
> > Documentation/devicetree/bindings/net/can/xilinx_can.txt
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> > b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> > new file mode 100644
> > index 000000000000..78398826677d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml
> > @@ -0,0 +1,161 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/can/xilinx,can.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title:
> > +  Xilinx Axi CAN/Zynq CANPS controller
> > +
> > +maintainers:
> > +  - Appana Durga Kedareswara rao <appana.durga.rao at xilinx.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - xlnx,zynq-can-1.0
> > +      - xlnx,axi-can-1.00.a
> > +      - xlnx,canfd-1.0
> > +      - xlnx,canfd-2.0
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    maxItems: 2
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  tx-fifo-depth:
> > +    $ref: "/schemas/types.yaml#/definitions/uint32"
> > +    description: CAN Tx fifo depth (Zynq, Axi CAN).
> > +
> > +  rx-fifo-depth:
> > +    $ref: "/schemas/types.yaml#/definitions/uint32"
> > +    description: CAN Rx fifo depth (Zynq, Axi CAN, CAN FD in
> > + sequential Rx mode)
> > +
> > +  tx-mailbox-count:
> > +    $ref: "/schemas/types.yaml#/definitions/uint32"
> > +    description: CAN Tx mailbox buffer count (CAN FD)
> 
> I asked about vendor prefix and I think I did not get an answer from you
> about skipping it. Do you think it is not needed?

Sorry, I went through all your previous comments but I couldn't find the 
comment where you had asked about vendor prefix. Could you please point
me to it ?
We can add vendor prefix to non-standard fields, but we need to update 
driver to be aligned with it and deprecate original property which has been 
added in 2018 and acked by Rob and Marc at that time.
https://github.com/torvalds/linux/commit/7cb0f17f5252874ba0ecbda964e7e01587bf828e

Regards,
Amit
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> 
> This should be rather unevaluatedProperties:false, so you could use can-
> controller properties.
> 
> > +
> > +allOf:
> > +  - $ref: can-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - 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:
> > +            enum:
> > +              - 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:
> > +            enum:
> > +              - xlnx,canfd-1.0
> > +              - xlnx,canfd-2.0
> > +
> > +    then:
> > +      properties:
> > +        clock-names:
> > +          items:
> > +            - const: can_clk
> > +            - const: s_axi_aclk
> > +      required:
> > +        - tx-mailbox-count
> > +        - rx-fifo-depth
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    can at e0008000 {
> > +        compatible = "xlnx,zynq-can-1.0";
> > +        clocks = <&clkc 19>, <&clkc 36>;
> > +        clock-names = "can_clk", "pclk";
> > +        reg = <0xe0008000 0x1000>;
> 
> Put reg just after compatible in all DTS examples.
> 
> > +        interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-parent = <&intc>;
> > +        tx-fifo-depth = <0x40>;
> > +        rx-fifo-depth = <0x40>;
> > +    };
> > +
> > +  - |
> > +    can at 40000000 {
> > +        compatible = "xlnx,axi-can-1.00.a";
> > +        clocks = <&clkc 0>, <&clkc 1>;
> > +        clock-names = "can_clk","s_axi_aclk" ;
> 
> Missing space after ','.
> 
> > +        reg = <0x40000000 0x10000>;
> > +        interrupt-parent = <&intc>;
> > +        interrupts = <GIC_SPI 59 IRQ_TYPE_EDGE_RISING>;
> > +        tx-fifo-depth = <0x40>;
> > +        rx-fifo-depth = <0x40>;
> > +    };
> 
> Best regards,
> Krzysztof