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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 3 Sep 2024 11:17:24 -0500
David Lechner <dlechner@baylibre.com> wrote:

> 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?
Definitely not where it currently is..
> 
> 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.

I'd be amazed if they managed to keep things that similar
given totally different buses.

> 
> 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.

I wonder if there is any precedence of restricted controllers
for SPI?  (For i2c we have the smbus ones as a vaguely similar
example). +CC Mark.

>  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)>;
>             };
>         };
>     };

That's definitely an improvement.  It's a little strange to have
a reference back to the parent but I'm fine with that.

Jonathan

> 
>