Re: [PATCH v4 3/3] dt-bindings: misc: Add binding for Xilinx AXI-Stream FIFO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The YAML schema incorrectly rejects valid hardware device trees by defining software driver limits instead of hardware IP properties and using `additionalProperties: false`.
- [High] The axis-fifo driver incorrectly uses register I/O accessors for stream FIFOs, causing data corruption on big-endian architectures.
- [Medium] Stale text binding documentation is not removed.
--

commit 26ea5f81e17b89c4409b6dcc57391cb51c42f3f2
Author: Pramod Maurya <pramod.nexgen@gmail.com>

dt-bindings: misc: Add binding for Xilinx AXI-Stream FIFO

This patch adds a YAML schema for the Xilinx AXI-Stream FIFO IP core to
resolve checkpatch warnings about undocumented compatible strings.

>     The binding documents the three supported compatible strings and the
>     vendor-specific properties that the axis-fifo staging driver reads from
>     the device tree.

Device tree bindings are generally meant to describe the hardware IP rather
than only what a specific software driver currently uses. By only including
properties the driver reads, are we missing fundamental hardware properties?

Also, since this introduces a formal YAML binding for these compatible
strings, should this patch also remove the obsolete text binding at
drivers/staging/axis-fifo/axis-fifo.txt to prevent conflicting documentation?

>     This resolves the following checkpatch.pl warnings in
>     drivers/staging/axis-fifo/axis-fifo.c:

While not introduced by this patch, the axis-fifo driver mentioned here
appears to use ioread32() and iowrite32() in loops for bulk data transfers
to the FIFO data registers.

Since register accessors perform implicit CPU-to-device byteswapping on
big-endian architectures, does this cause silent data corruption of the byte
stream? Would it be safer to use stream accessors like ioread32_rep() or
readsl() for the FIFO data transfers?

> diff --git a/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml
> new file mode 100644
> index 0000000000000..f4ef7c277cd7a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml

[ ... ]

> +  xlnx,axi-str-rxd-tdata-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Width of the receive AXI-Stream data bus in bits. Currently only 32
> +      is supported.
> +    const: 32

Is it appropriate to encode a temporary software driver limitation into the
permanent schema? By strictly constraining this to 32, does this artificially
reject valid hardware device trees that correctly model an IP configured with
a different data width?

> +
> +  xlnx,axi-str-txd-tdata-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Width of the transmit AXI-Stream data bus in bits. Currently only 32
> +      is supported.
> +    const: 32

Similar to the above, does this const constraint improperly embed a driver
limitation into the hardware binding?

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - xlnx,axi-str-rxd-tdata-width
> +  - xlnx,axi-str-txd-tdata-width
> +  - xlnx,rx-fifo-depth
> +  - xlnx,tx-fifo-depth
> +  - xlnx,use-rx-data
> +  - xlnx,use-tx-data
> +
> +additionalProperties: false

Since this sets additionalProperties to false but omits properties like
clocks (required for any synchronous AXI block), interrupt-names, and other
properties that were documented in the previous text binding, will valid
hardware device trees falsely fail dt_binding_check validation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515075736.172172-1-pramod.nexgen@gmail.com?part=3