| Message ID | 20240829-wip-bl-ad3552r-axi-v0-v1-0-b6da6015327a@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: dac: introducing ad3552r-axi | expand |
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) > > - adding the bus-type property vs. a boolean property vs. adding > a new compatible string. > > - how external synchronization should be handled. Actually, i added > 2 backend calls to enable or disable this external trigger. That seems more or less fine. Is there any control over the external trigger? This feels a bit like some of the complex stm32 hardware triggers in that a 'hidden' trigger is being enabled. If it is controllable or selectable (between say a PWM or an external pin) then you may need to be careful how to expose that control. > > - is a read-only sampling-frequency useful ? Yes. If it is easy to provide, it can be useful to userspace to allow it to figure out how much data to expect. Jonathan > > Thanks a lot for your feedbacks. > > To: Lars-Peter Clausen <lars@metafoo.de> > To: Michael Hennerich <Michael.Hennerich@analog.com> > To: Nuno Sá <nuno.sa@analog.com> > To: Jonathan Cameron <jic23@kernel.org> > To: Rob Herring <robh@kernel.org> > To: Krzysztof Kozlowski <krzk+dt@kernel.org> > To: Conor Dooley <conor+dt@kernel.org> > To: Olivier Moysan <olivier.moysan@foss.st.com> > Cc: linux-iio@vger.kernel.org > Cc: devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: dlechner@baylibre.com > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > Angelo Dureghello (8): > dt-bindings: iio: dac: ad3552r: add io-backend property > iio: backend: extend features > iio: backend adi-axi-dac: backend features > dt-bindings: iio: dac: add adi axi-dac bus property > iio: dac: ad3552r: changes to use FIELD_PREP > iio: dac: ad3552r: extract common code (no changes in behavior intended) > iio: dac: ad3552r: add axi platform driver > iio: ABI: add DAC sysfs synchronous_mode parameter > > Documentation/ABI/testing/sysfs-bus-iio-dac | 7 + > .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 39 +- > .../devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 + > drivers/iio/dac/Kconfig | 11 + > drivers/iio/dac/Makefile | 3 +- > drivers/iio/dac/ad3552r-axi.c | 572 +++++++++++++++++++++ > drivers/iio/dac/ad3552r-common.c | 163 ++++++ > drivers/iio/dac/ad3552r.c | 394 +++----------- > drivers/iio/dac/ad3552r.h | 199 +++++++ > drivers/iio/dac/adi-axi-dac.c | 250 ++++++++- > drivers/iio/industrialio-backend.c | 151 ++++++ > include/linux/iio/backend.h | 24 + > 12 files changed, 1494 insertions(+), 328 deletions(-) > --- > base-commit: 7ccb2c2db44572deadb795c4637273cdabbe8b66 > change-id: 20240829-wip-bl-ad3552r-axi-v0-b1e379c986d3 > > Best regards,
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)>; }; }; }; > >> - adding the bus-type property vs. a boolean property vs. adding >> a new compatible string. >> >> - how external synchronization should be handled. Actually, i added >> 2 backend calls to enable or disable this external trigger. > That seems more or less fine. Is there any control over the external > trigger? This feels a bit like some of the complex stm32 hardware > triggers in that a 'hidden' trigger is being enabled. > If it is controllable or selectable (between say a PWM or an external > pin) then you may need to be careful how to expose that control. > Actually this synchronization is needed since ADI is going to use this IP also in a a dual layout, so the 2 IPs needs to have an external synchronization by a signal. But as default synch is not enabled. Yes, it looks like a trigger. I can check if i can do this in a different way. >> - is a read-only sampling-frequency useful ? > Yes. If it is easy to provide, it can be useful to userspace to > allow it to figure out how much data to expect. > > Jonathan So this is the last RFC mail i am handling, trying to wrap up the open points: - about DAC backend or spi offload, if possible i would not change approach at this stage, i worked on the provided HDL. - about reg_read/write, let me know if the void * can stay - about external synch, i am trying to see if i can do this by a trigger. Just as a note, Nuno and David was involved helping me on this, so will add them as co-developers. Thanks a lot, Regards, Angelo >> Thanks a lot for your feedbacks. >> >> To: Lars-Peter Clausen <lars@metafoo.de> >> To: Michael Hennerich <Michael.Hennerich@analog.com> >> To: Nuno Sá <nuno.sa@analog.com> >> To: Jonathan Cameron <jic23@kernel.org> >> To: Rob Herring <robh@kernel.org> >> To: Krzysztof Kozlowski <krzk+dt@kernel.org> >> To: Conor Dooley <conor+dt@kernel.org> >> To: Olivier Moysan <olivier.moysan@foss.st.com> >> Cc: linux-iio@vger.kernel.org >> Cc: devicetree@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: dlechner@baylibre.com >> >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >> --- >> Angelo Dureghello (8): >> dt-bindings: iio: dac: ad3552r: add io-backend property >> iio: backend: extend features >> iio: backend adi-axi-dac: backend features >> dt-bindings: iio: dac: add adi axi-dac bus property >> iio: dac: ad3552r: changes to use FIELD_PREP >> iio: dac: ad3552r: extract common code (no changes in behavior intended) >> iio: dac: ad3552r: add axi platform driver >> iio: ABI: add DAC sysfs synchronous_mode parameter >> >> Documentation/ABI/testing/sysfs-bus-iio-dac | 7 + >> .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 39 +- >> .../devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 + >> drivers/iio/dac/Kconfig | 11 + >> drivers/iio/dac/Makefile | 3 +- >> drivers/iio/dac/ad3552r-axi.c | 572 +++++++++++++++++++++ >> drivers/iio/dac/ad3552r-common.c | 163 ++++++ >> drivers/iio/dac/ad3552r.c | 394 +++----------- >> drivers/iio/dac/ad3552r.h | 199 +++++++ >> drivers/iio/dac/adi-axi-dac.c | 250 ++++++++- >> drivers/iio/industrialio-backend.c | 151 ++++++ >> include/linux/iio/backend.h | 24 + >> 12 files changed, 1494 insertions(+), 328 deletions(-) >> --- >> base-commit: 7ccb2c2db44572deadb795c4637273cdabbe8b66 >> change-id: 20240829-wip-bl-ad3552r-axi-v0-b1e379c986d3 >> >> Best regards,
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)>; }; }; };
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 > >
On Tue, 2024-09-03 at 20:39 +0100, Jonathan Cameron wrote: > 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. > Yeah, I was trying to avoid new compatibles as much as I can because things can get pretty confusing (with lots of new compatibles and quirks) pretty quickly. Typically yes, most designs have slight differences between them (with new features and so on) but so far I was trying (thinking) to have those as a generic new backend op (plus a matching binding property if needed). For this particular case, I'm fairly sure we could get away with the bus controller property and having different implementations depending on the bus being implemented. For the other bits that might differ between designs (eg: DDR support) is up to frontends to call it or not (depending on they having that feature or not). Naturally we need that the IPs having DDR support to not have the same thing supported in different registers but we do control that since these are FPGA cores. All the above said, I'm fine with new compatibles but we need to draw a line when we add new ones. If the reasoning is the IP has some new bits or new registers, then things can get very confusing (even more if we think about fallback compatibles) as most of the new designs have some quirks (even if minimal). So I would say to add new compatibles when things get different enough that a sane/generic API is not doable. > > > > 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. > Agreed... - Nuno Sá
On Thu, 05 Sep 2024 11:16:03 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Tue, 2024-09-03 at 20:39 +0100, Jonathan Cameron wrote: > > 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. > > > > Yeah, I was trying to avoid new compatibles as much as I can because things can > get pretty confusing (with lots of new compatibles and quirks) pretty quickly. > Typically yes, most designs have slight differences between them (with new > features and so on) but so far I was trying (thinking) to have those as a > generic new backend op (plus a matching binding property if needed). For this > particular case, I'm fairly sure we could get away with the bus controller > property and having different implementations depending on the bus being > implemented. For the other bits that might differ between designs (eg: DDR > support) is up to frontends to call it or not (depending on they having that > feature or not). That breaks down if the backend you happen to be using (maybe a new one hasn't been written yet) is missing the DDR feature but the front end device can run with or without it. Unless the hardware makes this discoverable you'll have the backend driver writing some enable bit that does nothing. Maybe it's a case of using fallback compatibles - so define more specific ones but with a fallback to one that doesn't provide the fancy features and only covers thins all IPs support. > Naturally we need that the IPs having DDR support to not have > the same thing supported in different registers but we do control that since > these are FPGA cores. > > All the above said, I'm fine with new compatibles but we need to draw a line > when we add new ones. If the reasoning is the IP has some new bits or new > registers, then things can get very confusing (even more if we think about > fallback compatibles) as most of the new designs have some quirks (even if > minimal). So I would say to add new compatibles when things get different enough > that a sane/generic API is not doable. If you can influence the IP designers, the usual solution to this is discoverability of features. So standard register that all IP carries that has flags for each feature that has ever been implemented. If not, best option is each IP gets a compatible but we assume fallbacks are fine until they aren't. Jonathan > > > > > > > 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. > > > > Agreed... > > - Nuno Sá >
On Sat, 2024-09-07 at 15:12 +0100, Jonathan Cameron wrote: > On Thu, 05 Sep 2024 11:16:03 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Tue, 2024-09-03 at 20:39 +0100, Jonathan Cameron wrote: > > > 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. > > > > > > > Yeah, I was trying to avoid new compatibles as much as I can because things > > can > > get pretty confusing (with lots of new compatibles and quirks) pretty > > quickly. > > Typically yes, most designs have slight differences between them (with new > > features and so on) but so far I was trying (thinking) to have those as a > > generic new backend op (plus a matching binding property if needed). For > > this > > particular case, I'm fairly sure we could get away with the bus controller > > property and having different implementations depending on the bus being > > implemented. For the other bits that might differ between designs (eg: DDR > > support) is up to frontends to call it or not (depending on they having that > > feature or not). > > That breaks down if the backend you happen to be using (maybe a new > one hasn't been written yet) is missing the DDR feature but the front end > device can run with or without it. > Unless the hardware makes this discoverable you'll have the backend driver > writing some enable bit that does nothing. > > Maybe it's a case of using fallback compatibles - so define more specific > ones but with a fallback to one that doesn't provide the fancy features > and only covers thins all IPs support. > > > Naturally we need that the IPs having DDR support to not have > > the same thing supported in different registers but we do control that since > > these are FPGA cores. > > > > All the above said, I'm fine with new compatibles but we need to draw a line > > when we add new ones. If the reasoning is the IP has some new bits or new > > registers, then things can get very confusing (even more if we think about > > fallback compatibles) as most of the new designs have some quirks (even if > > minimal). So I would say to add new compatibles when things get different > > enough > > that a sane/generic API is not doable. > > If you can influence the IP designers, the usual solution to this is > discoverability of features. So standard register that all IP carries that > has flags for each feature that has ever been implemented. > That get's messy. We do have some flags for some of the more generic features (I'm using them in the backends when available). But we have (and will have) so many variations of these designs that it get's hard to get it right all the time. And for thing like this bus quirk a flag itself may be not enough to distinguish between different implementations... Last time I spoke with the designers, they are thinking about just adding a set of custom registers that (always the same range I think) for these IPs and then leave it up to the driver implementation to deal with the different implementations of the registers. Not sure if it's the best approach but it feels like they're getting tired of dealing with all the subtle changes between the different devices these IPs connect too :) On the IIO backend "world", frontends are the ones with the knowledge of what these custom registers could implement and so, it's very doable for backends to export a range of valid registers for frontends to "directly" (of course not reading/writing directly :)) access. Feels a bit hacky but also a bit reasonable... Anyways, all of the above is still just speculation so not sure if it will happen at all. Just some ramblings from me :) > If not, best option is each IP gets a compatible but we assume fallbacks > are fine until they aren't. > Yeah, Conor made some compelling arguments about using new compatibles. At least, for the more complicated cases like this. - Nuno Sá
On Mon, 09 Sep 2024 09:37:35 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2024-09-07 at 15:12 +0100, Jonathan Cameron wrote: > > On Thu, 05 Sep 2024 11:16:03 +0200 > > Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > On Tue, 2024-09-03 at 20:39 +0100, Jonathan Cameron wrote: > > > > 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. > > > > > > > > > > Yeah, I was trying to avoid new compatibles as much as I can because things > > > can > > > get pretty confusing (with lots of new compatibles and quirks) pretty > > > quickly. > > > Typically yes, most designs have slight differences between them (with new > > > features and so on) but so far I was trying (thinking) to have those as a > > > generic new backend op (plus a matching binding property if needed). For > > > this > > > particular case, I'm fairly sure we could get away with the bus controller > > > property and having different implementations depending on the bus being > > > implemented. For the other bits that might differ between designs (eg: DDR > > > support) is up to frontends to call it or not (depending on they having that > > > feature or not). > > > > That breaks down if the backend you happen to be using (maybe a new > > one hasn't been written yet) is missing the DDR feature but the front end > > device can run with or without it. > > Unless the hardware makes this discoverable you'll have the backend driver > > writing some enable bit that does nothing. > > > > Maybe it's a case of using fallback compatibles - so define more specific > > ones but with a fallback to one that doesn't provide the fancy features > > and only covers thins all IPs support. > > > > > Naturally we need that the IPs having DDR support to not have > > > the same thing supported in different registers but we do control that since > > > these are FPGA cores. > > > > > > All the above said, I'm fine with new compatibles but we need to draw a line > > > when we add new ones. If the reasoning is the IP has some new bits or new > > > registers, then things can get very confusing (even more if we think about > > > fallback compatibles) as most of the new designs have some quirks (even if > > > minimal). So I would say to add new compatibles when things get different > > > enough > > > that a sane/generic API is not doable. > > > > If you can influence the IP designers, the usual solution to this is > > discoverability of features. So standard register that all IP carries that > > has flags for each feature that has ever been implemented. > > > > That get's messy. We do have some flags for some of the more generic features > (I'm using them in the backends when available). But we have (and will have) so > many variations of these designs that it get's hard to get it right all the > time. And for thing like this bus quirk a flag itself may be not enough to > distinguish between different implementations... > > Last time I spoke with the designers, they are thinking about just adding a set > of custom registers that (always the same range I think) for these IPs and then > leave it up to the driver implementation to deal with the different > implementations of the registers. Not sure if it's the best approach but it > feels like they're getting tired of dealing with all the subtle changes between > the different devices these IPs connect too :) > > On the IIO backend "world", frontends are the ones with the knowledge of what > these custom registers could implement and so, it's very doable for backends to > export a range of valid registers for frontends to "directly" (of course not > reading/writing directly :)) access. Feels a bit hacky but also a bit > reasonable... Anyways, all of the above is still just speculation so not sure if > it will happen at all. Just some ramblings from me :) > > > > If not, best option is each IP gets a compatible but we assume fallbacks > > are fine until they aren't. > > > > Yeah, Conor made some compelling arguments about using new compatibles. At > least, for the more complicated cases like this. Agreed. With explanation above, we would still want the backends to be able to protect against a front end asking for things they don't support. Might not need to do that all the time, but if we don't have a compatible to deal with incompatibilities we won't be able to fix it when problems occur. Old IP, newer driver would be the likely nasty case though other way around is possible too (new IP, old driver) Jonathan > > - Nuno Sá
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. - 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. - adding the bus-type property vs. a boolean property vs. adding a new compatible string. - how external synchronization should be handled. Actually, i added 2 backend calls to enable or disable this external trigger. - is a read-only sampling-frequency useful ? Thanks a lot for your feedbacks. To: Lars-Peter Clausen <lars@metafoo.de> To: Michael Hennerich <Michael.Hennerich@analog.com> To: Nuno Sá <nuno.sa@analog.com> To: Jonathan Cameron <jic23@kernel.org> To: Rob Herring <robh@kernel.org> To: Krzysztof Kozlowski <krzk+dt@kernel.org> To: Conor Dooley <conor+dt@kernel.org> To: Olivier Moysan <olivier.moysan@foss.st.com> Cc: linux-iio@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: dlechner@baylibre.com Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> --- Angelo Dureghello (8): dt-bindings: iio: dac: ad3552r: add io-backend property iio: backend: extend features iio: backend adi-axi-dac: backend features dt-bindings: iio: dac: add adi axi-dac bus property iio: dac: ad3552r: changes to use FIELD_PREP iio: dac: ad3552r: extract common code (no changes in behavior intended) iio: dac: ad3552r: add axi platform driver iio: ABI: add DAC sysfs synchronous_mode parameter Documentation/ABI/testing/sysfs-bus-iio-dac | 7 + .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 39 +- .../devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 + drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 3 +- drivers/iio/dac/ad3552r-axi.c | 572 +++++++++++++++++++++ drivers/iio/dac/ad3552r-common.c | 163 ++++++ drivers/iio/dac/ad3552r.c | 394 +++----------- drivers/iio/dac/ad3552r.h | 199 +++++++ drivers/iio/dac/adi-axi-dac.c | 250 ++++++++- drivers/iio/industrialio-backend.c | 151 ++++++ include/linux/iio/backend.h | 24 + 12 files changed, 1494 insertions(+), 328 deletions(-) --- base-commit: 7ccb2c2db44572deadb795c4637273cdabbe8b66 change-id: 20240829-wip-bl-ad3552r-axi-v0-b1e379c986d3 Best regards,