Re: [PATCH v2 1/9] dt-bindings: iio: dac: ad3552r: add io-backend property

[David Lechner]

On 9/9/24 12:19 PM, David Lechner wrote:
> On 9/9/24 9:03 AM, Nuno Sá wrote:
>> On Mon, 2024-09-09 at 13:46 +0100, Conor Dooley wrote:
>>> On Sun, Sep 08, 2024 at 01:29:25PM +0100, Jonathan Cameron wrote:
>>>> On Thu, 05 Sep 2024 17:17:31 +0200
>>>> Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
>>>>
>>>>> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
>>>>>
>>>>> There is a version AXI DAC IP block (for FPGAs) that provides
>>>>> a physical bus for AD3552R and similar chips. This can be used
>>>>> instead of a typical SPI controller to be able to use the chip
>>>>> in ways that typical SPI controllers are not capable of.
>>>>>
>>>>> The binding is modified so that either the device is a SPI
>>>>> peripheral or it uses an io-backend.
>>>>>
>>>>> Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
>>>>
>>>>>  
>>>>>  required:
>>>>>    - compatible
>>>>> -  - reg
>>>>> -  - spi-max-frequency
>>>> Sort of feels like both reg and spi-max-frequency
>>>> are valid things to specify. 
>>>>
>>>> Maybe we have an excellent IP and dodgy wiring so want
>>>> to clamp the frequency (long term - don't need to support
>>>> in the driver today).
>>>>
>>>> Maybe we have an axi_dac IP that supports multiple
>>>> front end devices?  So maybe just keep reg?
>>>
>>> I'd like to be convinced that this incarnation of the AXI DAC IP is not
>>> a spi controller and that a ref to spi-controller.yaml is not out of
>>> place here. It may not be something that you'd ever use generally, given
>>> the "weird" interface to it, but it does seem to be one regardless.
>>>
>>
>> Agreed.. As weird as it get's, it's acting as a spi controller.
>>
>>> I'd also really like to know how this fits in with spi-offloads. It
>>> /feels/, and I'd like to reiterate the word feels, like a rather similar
>>> idea just applied to a DAC instead of an ADC.
>>
>> The offload main principle is to replay a spi transfer periodically given an
>> input trigger. I'm not so sure we have that same principle in here. In here I
>> guess we stream data over the qspi interface based on SCLK which can look
>> similar. The difference is that this IP does not need any trigger for any spi
>> transfer replay (I think). 
>>
> 
> Looking at the AD3552R from a SPI offload perspective of triggered SPI
> messages, I think it still works.
> 
> The trigger doesn't have to be a clock/PWM. In this case, the trigger would
> be whenever the IIO buffer is full and ready to send a burst of data (not
> sure if this would be a hardware or software trigger - but it works either
> way).
> 
> Also, the DAC_CUSTOM_CTRL::ADDRESS register field in the AXI DAC IP core
> acts as an offload to record and play back a SPI write transfer.
> 
> If we were using the AXI SPI Engine, this would be one SPI message with
> two xfers, one for the address write followed by one for the data write.
> The size of the data write would be the size of the IIO buffer - or in
> the case of a cyclic DMA, the size of the write data would be channel
> data size * num channels and the xfer would have a special cyclic offload
> flag set.
> 
> So I think we could make a single binding that works for the the AXI DAC
> backend/offload and the AXI SPI Engine offload. (I don't think it would
> be so easy to integrate the AXI DAC into the SPI framework on the driver
> side - and hopefully we won't have to, but the DT still could use the
> proposed SPI offload bindings.)
> 
>     axi_dac: spi@44a70000 {
>         compatible = "adi,axi-ad3225r";
>         reg = <0x44a70000 0x1000>;
>         dmas = <&dac_tx_dma_1 0>;
>         dma-names = "tx";
>         clocks = <&ref_clk>;
>         #spi-offload-cells = <0>;

One thing I forgot...

The AXI AD3552R IP core can be wired up as a loopback to pipe
data directly from some ADC instead of using DMA.

In the case of the ADC loopback, we would also have

	io-channels = <&adc1>, <&adc2>;

here in the controller.

And we would need #spi-offload-cell = <1>; to have a cell to
specify the data source.

> 
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         dac@0 {
>             compatible = "adi,ad3552r";
>             reg = <0>;
> 
>             spi-max-frequency = <30000000>;
>             spi-3-wire;
>             spi-tx-bus-width = <4>;
>             spi-rx-bus-width = <4>;
> 
>             reset-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;
>             spi-offloads = <&axi_dac>;

And then here I guess it would be:

	spi-offloads = <&axi_dac 0>, <&axi_dac 1>;
	spi-offload-names = "dma", "adc";

where 0 would select the DMA stream and 1 would select the ADC stream.

Or of the ADC part wasn't wired up, just:

	spi-offloads = <&axi_dac 0>;
	spi-offload-names = "dma";


> 
>             #address-cells = <1>;
>             #size-cells = <0>;
> 
>             channel@0 {
>                 reg = <0>;
>                 adi,output-range-microvolt = <(-10000000) (10000000)>;
>             };
>         };
>     };
> 
>     axi_spi_engine: spi@44a80000 {
>         compatible = "adi,axi-spi-engine-1.00.a";
>         reg = <0x44a80000 0x1000>;
>         dmas = <&dac_tx_dma_2 0>;
>         dma-names = "offload0-tx";
>         clocks = <&ref_clk>;
>         #spi-offload-cells = <1>;
> 
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         dac@0 {
>             compatible = "adi,ad3552r";
>             reg = <0>;
> 
>             spi-max-frequency = <30000000>;
>             spi-3-wire;
>             spi-tx-bus-width = <4>;
>             spi-rx-bus-width = <4>;
> 
>             reset-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;
>             spi-offloads = <&axi_spi_engine 0>;
> 
>             #address-cells = <1>;
>             #size-cells = <0>;
> 
>             channel@0 {
>                 reg = <0>;
>                 adi,output-range-microvolt = <(-10000000) (10000000)>;
>             };
>         };
>     };
>