| Message ID | 20240905-wip-bl-ad3552r-axi-v0-iio-testing-v2-1-87d669674c00@baylibre.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: add support for the ad3552r AXI DAC IP | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Thu, 05 Sep 2024 17:17:31 +0200, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > 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@baylibre.com> > --- > .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 42 ++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/iio/dac/adi,ad3552r.example.dtb: /example-1/axi_dac@44a70000: failed to match any schema with compatible: ['adi,axi-dac-ad3552r'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240905-wip-bl-ad3552r-axi-v0-iio-testing-v2-1-87d669674c00@baylibre.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 9/5/24 11:28 AM, Rob Herring (Arm) wrote: > > On Thu, 05 Sep 2024 17:17:31 +0200, Angelo Dureghello wrote: >> From: Angelo Dureghello <adureghello@baylibre.com> >> >> 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@baylibre.com> >> --- >> .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 42 ++++++++++++++++++++-- >> 1 file changed, 40 insertions(+), 2 deletions(-) >> > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/iio/dac/adi,ad3552r.example.dtb: /example-1/axi_dac@44a70000: failed to match any schema with compatible: ['adi,axi-dac-ad3552r'] I think this can be fixed by putting commit "dt-bindings: iio: dac: add ad3552r axi-dac compatible" first in the series before commit "dt-bindings: iio: dac: ad3552r: add io-backend property". > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240905-wip-bl-ad3552r-axi-v0-iio-testing-v2-1-87d669674c00@baylibre.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. >
On Thu, 05 Sep 2024 17:17:31 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > 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@baylibre.com> > > 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? > > additionalProperties: false > > @@ -238,4 +254,26 @@ examples: > }; > }; > }; > + > + - | > + backend: axi_dac@44a70000 { > + compatible = "adi,axi-dac-ad3552r"; > + reg = <0x44a70000 0x1000>; > + dmas = <&dac_tx_dma 0>; > + dma-names = "tx"; > + #io-backend-cells = <0>; > + clocks = <&ref_clk>; > + dac { > + compatible = "adi,ad3552r"; > + reset-gpios = <&gpio0 92 1>; > + io-backends = <&backend>; > + #address-cells = <1>; > + #size-cells = <0>; > + channel@0 { > + reg = <0>; > + adi,output-range-microvolt = <(-10000000) (10000000)>; > + }; > + }; > + }; > + > ... >
On 08/09/24 2:29 PM, Jonathan Cameron wrote: > On Thu, 05 Sep 2024 17:17:31 +0200 > Angelo Dureghello <adureghello@baylibre.com> wrote: > >> From: Angelo Dureghello <adureghello@baylibre.com> >> >> 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@baylibre.com> >> >> required: >> - compatible >> - - reg >> - - spi-max-frequency > Sort of feels like both reg and spi-max-frequency > are valid things to specify. This specific backend IP generates a fixed non-configurable clock frequency, so i don't think the spi-max-frequency is needed. > 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? yes, this is what i am wondering now too, i simplified with just one frontend node, are multimple frontends (and so reg property) needed ? >> >> additionalProperties: false >> >> @@ -238,4 +254,26 @@ examples: >> }; >> }; >> }; >> + >> + - | >> + backend: axi_dac@44a70000 { >> + compatible = "adi,axi-dac-ad3552r"; >> + reg = <0x44a70000 0x1000>; >> + dmas = <&dac_tx_dma 0>; >> + dma-names = "tx"; >> + #io-backend-cells = <0>; >> + clocks = <&ref_clk>; >> + dac { >> + compatible = "adi,ad3552r"; >> + reset-gpios = <&gpio0 92 1>; >> + io-backends = <&backend>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + channel@0 { >> + reg = <0>; >> + adi,output-range-microvolt = <(-10000000) (10000000)>; >> + }; >> + }; >> + }; >> + >> ... >> Regards,
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@baylibre.com> wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > 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@baylibre.com> > > > > > 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. 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. We kinda hit a dead end with generalising the spi-offload stuff due to lack of similar devices, but maybe we were looking for other ADCs when instead it should have been at all other types of IIO device? > > > > additionalProperties: false > > > > @@ -238,4 +254,26 @@ examples: > > }; > > }; > > }; > > + > > + - | > > + backend: axi_dac@44a70000 { "axi_dac": - no underscores in node names please - shouldn't it be "io-backend@" or something like that? Or even spi@? axi_dac isn't a class of device, and this device is not, AFAICT a DAC at all since the ad3552r is what actually does the conversion. Cheers, Conor. > > + compatible = "adi,axi-dac-ad3552r"; > > + reg = <0x44a70000 0x1000>; > > + dmas = <&dac_tx_dma 0>; > > + dma-names = "tx"; > > + #io-backend-cells = <0>; > > + clocks = <&ref_clk>; > > + dac { > > + compatible = "adi,ad3552r"; > > + reset-gpios = <&gpio0 92 1>; > > + io-backends = <&backend>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + channel@0 { > > + reg = <0>; > > + adi,output-range-microvolt = <(-10000000) (10000000)>; > > + }; > > + }; > > + }; > > + > > ... > > >
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@baylibre.com> wrote: > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > 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@baylibre.com> > > > > > > > > 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). - Nuno Sá
On Mon, Sep 09, 2024 at 04:03:17PM +0200, 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: > > 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). Right, if the trigger part is what decides it for you then I'm wildin here.
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@baylibre.com> wrote: >>> >>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>> >>>> 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@baylibre.com> >>> >>>> >>>> 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>; #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>; #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)>; }; }; };
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@baylibre.com> wrote: >>>> >>>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>>> >>>>> 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@baylibre.com> >>>> >>>>> >>>>> 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)>; > }; > }; > }; >
On Mon, 9 Sep 2024 13:39:26 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > On 08/09/24 2:29 PM, Jonathan Cameron wrote: > > On Thu, 05 Sep 2024 17:17:31 +0200 > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > >> From: Angelo Dureghello <adureghello@baylibre.com> > >> > >> 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@baylibre.com> > >> > >> required: > >> - compatible > >> - - reg > >> - - spi-max-frequency > > Sort of feels like both reg and spi-max-frequency > > are valid things to specify. > > This specific backend IP generates a fixed non-configurable clock > frequency, so i don't think the spi-max-frequency is needed. Ah fair enough. > > > > 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? > > yes, this is what i am wondering now too, i simplified with just one > frontend node, are multimple frontends (and so reg property) needed ? It does little harm to have one. So I'd say keep it as required. Detection of what is required should be based on something more specific than reg being there or not. Jonathan
On Mon, 2024-09-09 at 17:06 +0100, Conor Dooley wrote: > On Mon, Sep 09, 2024 at 04:03:17PM +0200, 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: > > > > 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). > > Right, if the trigger part is what decides it for you then I'm wildin > here. I mean, not only the trigger. These IPs (axi-dac/adc) are meant to deal with data while in theory the spi offload principle is about replaying any spi transfers. But yeah, the above reasoning does not make sense as a data transfer is still a transfer. FWIW, these IPs are inherently offload HW as their goal is really to stream data without any SW intervention (so called HW_BUFFERING in IIO world). Just that typically you have LVDS/CMOS data interfaces and now we have a qspi interface and a spi-offload concept already introduced. So, yeah, as we want to have spi-offloads documented in the bindings, we can also document this setup with the same bindings. - Nuno Sá
On Mon, 2024-09-09 at 12:19 -0500, 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@baylibre.com> wrote: > > > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > 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@baylibre.com> > > > > > > > > > > > > > > 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). > Right... That's what we already have for DACs with HW buffering. > 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. Just a nipick comment. At least from the current implementation the address is only writen once before starting to stream. So I guess we would not want to replay that xfer for every sample. > 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.) > Hopefully not... - Nuno Sá
On 10/09/24 10:16 AM, Nuno Sá wrote: > On Mon, 2024-09-09 at 12:19 -0500, 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@baylibre.com> wrote: >>>>> >>>>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>>>> >>>>>> 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@baylibre.com> >>>>>> >>>>>> 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). >> > Right... That's what we already have for DACs with HW buffering. > >> 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. > Just a nipick comment. At least from the current implementation the address is > only writen once before starting to stream. So I guess we would not want to > replay that xfer for every sample. > >> 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.) >> > Hopefully not... As of now, i am proposing to stay in the simpler node as possible for the current case, like: axi_dac: spi@44a70000 { compatible = "adi,axi-ad3552r"; 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 { reg = <0>; compatible = "adi,ad3552r"; reset-gpios = <&gpio0 92 GPIO_ACTIVE_HIGH>; io-backends = <&axi_dac>; #address-cells = <1>; #size-cells = <0>; channel@0 { reg = <0>; adi,output-range-microvolt = <(-10000000) (10000000)>; }; }; }; and extend things later on, in case. SPI lines and other info are all obtained from the compatible. What do you think ? > > - Nuno Sá
On Tue, Sep 10, 2024 at 10:16:24AM +0200, Nuno Sá wrote: > On Mon, 2024-09-09 at 12:19 -0500, David Lechner wrote: > > 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.) > > > > Hopefully not... Yeah, I wasn't really trying to place any expectations on how the driver would look. My motivation in talking about offloads here was to see if the data direct switch would help at all in trying to ensure the spi-offloads stuff was being handled generically. That said, I do think the current implementation binding wise is probably capable of supporting both directions with little to no problems, it'd mostly be the kernel's (proposed) interpretation that'd not be up to it?
diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml index fc8b97f82077..a6ce863ca93c 100644 --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml @@ -60,12 +60,30 @@ properties: $ref: /schemas/types.yaml#/definitions/uint32 enum: [0, 1, 2, 3] + io-backends: + description: The iio backend reference. + An example backend can be found at + https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html + maxItems: 1 + '#address-cells': const: 1 '#size-cells': const: 0 +if: + required: + - reg + +then: + $ref: /schemas/spi/spi-peripheral-props.yaml# + + properties: + io-backends: false + + required: [ spi-max-frequency ] + patternProperties: "^channel@([0-1])$": type: object @@ -207,8 +225,6 @@ allOf: required: - compatible - - reg - - spi-max-frequency additionalProperties: false @@ -238,4 +254,26 @@ examples: }; }; }; + + - | + backend: axi_dac@44a70000 { + compatible = "adi,axi-dac-ad3552r"; + reg = <0x44a70000 0x1000>; + dmas = <&dac_tx_dma 0>; + dma-names = "tx"; + #io-backend-cells = <0>; + clocks = <&ref_clk>; + dac { + compatible = "adi,ad3552r"; + reset-gpios = <&gpio0 92 1>; + io-backends = <&backend>; + #address-cells = <1>; + #size-cells = <0>; + channel@0 { + reg = <0>; + adi,output-range-microvolt = <(-10000000) (10000000)>; + }; + }; + }; + ...