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

[Amit Kumar Kumar Mahapatra <akumarma@xilinx.com>]

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Sent: Thursday, March 10, 2022 10:25 PM
> To: Amit Kumar Kumar Mahapatra <akumarma@xilinx.com>;
> wg@grandegger.com; mkl@pengutronix.de; kuba@kernel.org;
> robh+dt@kernel.org; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; git
> <git@xilinx.com>; Amit Kumar Kumar Mahapatra <akumarma@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@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@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@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@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