On Tue, 3 Sep 2024 11:17:24 -0500
David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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
>
>
