Re: [PATCH] dt-bindings: misc: axi-fifo: Add binding for AXI-Stream FIFO

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/06/2024 03:06, Mark Watson wrote:
> This resolves a checkpatch warning in drivers/staging/axis-fifo
> regarding a missing devie-tree binding. The full warning is included
> below.
> 
> WARNING: DT compatible string "xlnx,axi-fifo-mm-s-4.1" appears
> un-documented -- check ./Documentation/devicetree/bindings/
> +       { .compatible = "xlnx,axi-fifo-mm-s-4.1", },

That's not needed. Drop above reasoning but instead say that you add new
bindings for foo bar (explain what is the hardware).

You did not test your code, so limited review follows.

There is a binding. Just grep for compatible. You might want to do
conversion but then:

Thank you for your patch. There are no DTS users of this binding, so
while such conversions are useful, they have significantly smaller
impact. In the future, please consider converting bindings from active
platforms (arm64 defconfig, arm multi_v7). This would have significantly
bigger impact.

See also:
https://lore.kernel.org/all/6552bcb8-e046-4882-91da-1094fff3d239@linaro.org/

> 
> Signed-off-by: Mark Watson <markus.c.watson@gmail.com>
> ---
>  .../bindings/misc/xlnx,axi-fifo-mm-s-4.1.yaml | 214 ++++++++++++++++++
>  1 file changed, 214 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s-4.1.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s-4.1.yaml b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s-4.1.yaml
> new file mode 100644
> index 000000000000..cfb335752054
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s-4.1.yaml
> @@ -0,0 +1,214 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/xlnx,axi-fifo-mm-s-4.1.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx AXI-Stream FIFO v4.1 IP core
> +

Missing maintainers. Why would we care about hardware if there is no one
to interested in it?

> +description: |
> +  The Xilinx AXI-Stream FIFO v4.1 IP core has read and write AXI-Stream FIFOs,
> +  the contents of which can be accessed from the AXI4 memory-mapped interface.
> +  This is useful for transferring data from a processor into the FPGA fabric.
> +  The driver creates a character device that can be read/written to with
> +  standard open/read/write/close operations.
> +
> +  See Xilinx PG080 document for IP details.
> +
> +  Currently supports only store-forward mode with a 32-bit AXI4-Lite
> +  interface.
> +
> +  DOES NOT support:
> +    - cut-through mode
> +    - AXI4 (non-lite)
> +
> +properties:
> +  compatible:
> +    const: xlnx,axi-fifo-mm-s-4.1
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    items:
> +      - const: interrupt

No, drop entire property.

> +
> +  interrupt-parent:
> +    $ref: /schemas/types.yaml#/definitions/phandle

Drop property.

> +
> +  xlnx,axi-str-rxd-protocol:
> +    const: XIL_AXI_STREAM_ETH_DATA
> +
> +  xlnx,axi-str-rxd-tdata-width:
> +    const: 0x20
> +
> +  xlnx,axi-str-txc-protocol:
> +    const: XIL_AXI_STREAM_ETH_CTRL
> +
> +  xlnx,axi-str-txc-tdata-width:
> +    const: 0x20
> +
> +  xlnx,axi-str-txd-protocol:
> +    const: XIL_AXI_STREAM_ETH_DATA
> +
> +  xlnx,axi-str-txd-tdata-width:
> +    const: 0x20

Drop all these.

> +
> +  xlnx,axis-tdest-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,axis-tid-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,axis-tuser-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32

No clue what are these. Drop.

> +
> +  xlnx,data-interface-type:
> +    const: 0x0
> +
> +  xlnx,has-axis-tdest:
> +    const: 0x0
> +
> +  xlnx,has-axis-tid:
> +    const: 0x0
> +
> +  xlnx,has-axis-tkeep:
> +    const: 0x1
> +
> +  xlnx,has-axis-tstrb:
> +    const: 0x0
> +
> +  xlnx,has-axis-tuser:
> +    const: 0x0

Drop everything above.

> +
> +  xlnx,rx-fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,rx-fifo-pe-threshold:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,rx-fifo-pf-threshold:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,s-axi-id-width:
> +    const: 0x4
> +
> +  xlnx,s-axi4-data-width:
> +    const: 0x20
> +
> +  xlnx,select-xpm:
> +    const: 0x0
> +
> +  xlnx,tx-fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,tx-fifo-pe-threshold:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,tx-fifo-pf-threshold:
> +    $ref: /schemas/types.yaml#/definitions/uint32

No standard properties?

> +
> +  xlnx,use-rx-cut-through:
> +    const: 0x0
> +
> +  xlnx,use-rx-data:
> +    const: 0x1
> +
> +  xlnx,use-tx-ctrl:
> +    const: 0x0
> +
> +  xlnx,use-tx-cut-through:
> +    const: 0x0
> +
> +  xlnx,use-tx-data:
> +    const: 0x1
> +
> +  xlnx,tx-max-pkt-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  xlnx,rx-min-pkt-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32

No, drop all these.


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - interrupt-parent
> +  - xlnx,axi-str-rxd-protocol
> +  - xlnx,axi-str-rxd-tdata-width
> +  - xlnx,axi-str-txc-protocol
> +  - xlnx,axi-str-txc-tdata-width
> +  - xlnx,axi-str-txd-protocol
> +  - xlnx,axi-str-txd-tdata-width
> +  - xlnx,axis-tdest-width
> +  - xlnx,axis-tid-width
> +  - xlnx,axis-tuser-width
> +  - xlnx,data-interface-type
> +  - xlnx,has-axis-tdest
> +  - xlnx,has-axis-tid
> +  - xlnx,has-axis-tkeep
> +  - xlnx,has-axis-tstrb
> +  - xlnx,has-axis-tuser
> +  - xlnx,rx-fifo-depth
> +  - xlnx,rx-fifo-pe-threshold
> +  - xlnx,rx-fifo-pf-threshold
> +  - xlnx,s-axi-id-width
> +  - xlnx,s-axi4-data-width
> +  - xlnx,select-xpm
> +  - xlnx,tx-fifo-depth
> +  - xlnx,tx-fifo-pe-threshold
> +  - xlnx,tx-fifo-pf-threshold
> +  - xlnx,use-rx-cut-through
> +  - xlnx,use-rx-data
> +  - xlnx,use-tx-ctrl
> +  - xlnx,use-tx-cut-through
> +  - xlnx,use-tx-data
> +  - xlnx,tx-max-pkt-size
> +  - xlnx,rx-min-pkt-size
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    axi_fifo_mm_s_0: axi_fifo_mm_s@43c00000 {

Drop label. No underscores in node names.


Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +      compatible = "xlnx,axi-fifo-mm-s-4.1";
> +      interrupt-names = "interrupt";
> +      interrupt-parent = <&intc>;
> +      interrupts = <0 29 4>;

Use proper defines.


Best regards,
Krzysztof