Re: [PATCH v3 02/10] dt-bindings: iio: dac: axi-dac: add ad3552r axi variant

From: Krzysztof Kozlowski
Date: Sun Sep 22 2024 - 16:59:25 EST


On Thu, Sep 19, 2024 at 11:19:58AM +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
>
> Add a new compatible and related bindigns for the fpga-based
> "ad3552r" AXI IP core, a variant of the generic AXI DAC IP.
>
> The AXI "ad3552r" IP is a very similar HDL (fpga) variant of the
> generic AXI "DAC" IP, intended to control ad3552r and similar chips,
> mainly to reach high speed transfer rates using an additional QSPI
> DDR interface.
>
> The ad3552r device is defined as a child of the AXI DAC, that in
> this case is acting as an SPI controller.


So who acts as an SPI controller? adi,axi-ad3552r? Why this is not
reflected in the binding?

>
> Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> ---
> .../devicetree/bindings/iio/dac/adi,axi-dac.yaml | 40 ++++++++++++++++++++--
> 1 file changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
> index a55e9bfc66d7..6cf0c2cb84e7 100644
> --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
> @@ -19,11 +19,13 @@ description: |
> memory via DMA into the DAC.
>
> https://wiki.analog.com/resources/fpga/docs/axi_dac_ip
> + https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
>
> properties:
> compatible:
> enum:
> - adi,axi-dac-9.1.b
> + - adi,axi-ad3552r
>
> reg:
> maxItems: 1
> @@ -41,22 +43,54 @@ properties:
> '#io-backend-cells':
> const: 0
>
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> required:
> - compatible
> - dmas
> - reg
> - clocks
>
> +patternProperties:
> + "^.*@([0-9])$":

No need for ()

but this is pattern is quite permissive, so looks like you want to
achieve something else. Maybe spi controller? Not sure, I am just
guessing because empty example does not help me.

> + type: object
> + additionalProperties: true

No, this cannot be true.

> + properties:
> + io-backends:
> + description: |
> + AXI backend reference
> + required:
> + - io-backends

I don't get the point. Nodes with only one property are not really
useful.

> +
> additionalProperties: false
>
> examples:
> - |
> dac@44a00000 {
> - compatible = "adi,axi-dac-9.1.b";
> - reg = <0x44a00000 0x10000>;
> - dmas = <&tx_dma 0>;
> + compatible = "adi,axi-dac-9.1.b";
> + reg = <0x44a00000 0x10000>;

Why are you changing this? It's even messier now - other example uses
different indentayion...

> + dmas = <&tx_dma 0>;
> + dma-names = "tx";
> + #io-backend-cells = <0>;
> + clocks = <&axi_clk>;
> + };
> +
> + - |
> + axi_dac: spi@44a70000 {
> + compatible = "adi,axi-ad3552r";
> + reg = <0x44a70000 0x1000>;
> + dmas = <&dac_tx_dma 0>;
> dma-names = "tx";
> #io-backend-cells = <0>;
> clocks = <&axi_clk>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* DAC devices */

Your schema must be complete, example as well (minus repetitive pieces).

Best regards,
Krzysztof