Re: [RFC PATCH 0/8] iio: dac: introducing ad3552r-axi

[David Lechner <dlechner@baylibre.com>]

On 9/3/24 3:34 AM, Angelo Dureghello wrote:
> Hi Jonathan and all,
> 
> 
> On 31/08/24 1:38 PM, Jonathan Cameron wrote:
>> On Thu, 29 Aug 2024 14:31:58 +0200
>> Angelo Dureghello <adureghello@baylibre.com> wrote:
>>
>>> Hi, asking for comments for this patchset, that is mostly
>>> ready, at least feature-complete and functionally tested.
>>>
>>> I am introducing ad3552r-axi variant, controlled from a fpga-based
>>> AXI IP, as a platform driver, using the DAC backend. The patchset is
>>> actually based on linux-iio, since some needed DAC backend features
>>> was already there on that repo only, still to be merged in mainline.
>>>
>>> Comments i would like to ask are:
>>>
>>> - i added some devicetree bindings inside current ad3552r yaml,
>>>    device is the same, so i wouldn't create a different yaml file.
>> Agreed. If same device, it's usually better to keep it in one file.
>>
>>> - if it's ok adding the bus-type property in the DAC backend:
>>>    actually, this platform driver uses a 4 lanes parallel bus, plus
>>>    a clock line, similar to a qspi. This to read an write registers
>>>    and as well to send samples at double data rate. Other DAC may
>>>    need "parallel" or "lvds" in the future.
>> If it is for register read + write as well, sounds to me like you need
>> to treat this as a new bus type, possibly then combined with a
>> backend, or something similar to spi offload?
>>
>> What bus does this currently sit on in your DT bindings?
>> (add an example)
> 
> 
> &amba {
> 
>     ref_clk: clk@44B00000 {
>         compatible = "adi,axi-clkgen-2.00.a";
>         reg = <0x44B00000 0x10000>;
>         #clock-cells = <0>;
>         clocks = <&clkc 15>, <&clkc 15>;
>         clock-names = "s_axi_aclk", "clkin1";
>         clock-output-names = "ref_clk";
>     };
> 
>     dac_tx_dma: dma-controller@0x44a30000 {
>         compatible = "adi,axi-dmac-1.00.a";
>         reg = <0x44a30000 0x10000>;
>         #dma-cells = <1>;
>         interrupt-parent = <&intc>;
>         interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
>         clocks = <&clkc 15>;
> 
>         adi,channels {
>             #size-cells = <0>;
>             #address-cells = <1>;
> 
>             dma-channel@0 {
>                 reg = <0>;
>                 adi,source-bus-width = <32>;
>                 adi,source-bus-type = <0>;
>                 adi,destination-bus-width = <32>;
>                 adi,destination-bus-type = <1>;
>             };
>         };
>     };
> 
>     backend: controller@44a70000 {
>         compatible = "adi,axi-dac-9.1.b";
>         reg = <0x44a70000 0x1000>;
>         dmas = <&dac_tx_dma 0>;
>         dma-names = "tx";
>         #io-backend-cells = <0>;
>         clocks = <&ref_clk>;
>         bus-type = <1>;  /* IIO QSPI */
>     };
> 
>     axi-ad3552r {
>         compatible = "adi,ad3552r";
>         reset-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;
>         io-backends = <&backend>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         channel@0 {
>             reg = <0>;
>             adi,output-range-microvolt = <(-10000000) (10000000)>;
>         };
>     };

Shouldn't the axi-ad3552r node be one level higher since it isn't
a memory-mapped device, but rather an external chip?

But based on the other feedback we got in this series and some
#devicetree IRC chat here is an alternate binding suggestion we
could consider.

First, even though the FPGA IP block for use with AD3225R uses
the same register map as the AXI DAC IP block, some of the
registers behave differently, so it makes sense to have a
different compatible string rather than using the bus-type
property to tell the difference between the two IP blocks.
There are likely more differences than just the bus type.

Second, technically, the AXI DAC IP block can't be used as
a generic SPI controller, so it wouldn't make sense to put
it in drivers/spi. But, from wiring point of view, it could
still make sense to use SPI DT bindings since we have SPI
wiring. At the same time, the AXI DAC IP block is also
providing extra functionality in addition to the SPI bus
so it makes sense to keep the io-backend bindings for those
extra bits.

    backend: spi@44a70000 {
        compatible = "adi,axi-dac-ad3225r";
        reg = <0x44a70000 0x1000>;
        dmas = <&dac_tx_dma 0>;
        dma-names = "tx";
        #io-backend-cells = <0>;
        clocks = <&ref_clk>;

        #address-cells = <1>;
        #size-cells = <0>;

        dac@0 {
            compatible = "adi,ad3552r";
            reg = <0>;

            /* 
             * Not sure how right this is - attempting to say that
             * the QSPI select pin is hardwired high, so the 4 SPI I/O
             * pins on the DAC are always functioning as SDIO0/1/2/3
             * as opposed to the usual 2 SDI/SDO pins and 2 unused.
             */
            spi-3-wire;
            spi-tx-bus-width = <4>;
            spi-rx-bus-width = <4>;

            reset-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;
            io-backends = <&backend>;

            #address-cells = <1>;
            #size-cells = <0>;

            channel@0 {
                reg = <0>;
                adi,output-range-microvolt = <(-10000000) (10000000)>;
            };
        };
    };