Hi Jonathan and all,
On 31/08/24 1:38 PM, Jonathan Cameron wrote:
On Thu, 29 Aug 2024 14:31:58 +0200
Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
Hi, asking for comments for this patchset, that is mostly
ready, at least feature-complete and functionally tested.
I am introducing ad3552r-axi variant, controlled from a fpga-based
AXI IP, as a platform driver, using the DAC backend. The patchset is
actually based on linux-iio, since some needed DAC backend features
was already there on that repo only, still to be merged in mainline.
Comments i would like to ask are:
- i added some devicetree bindings inside current ad3552r yaml,
device is the same, so i wouldn't create a different yaml file.
Agreed. If same device, it's usually better to keep it in one file.
- if it's ok adding the bus-type property in the DAC backend:
actually, this platform driver uses a 4 lanes parallel bus, plus
a clock line, similar to a qspi. This to read an write registers
and as well to send samples at double data rate. Other DAC may
need "parallel" or "lvds" in the future.
If it is for register read + write as well, sounds to me like you need
to treat this as a new bus type, possibly then combined with a
backend, or something similar to spi offload?
What bus does this currently sit on in your DT bindings?
(add an example)
&amba {
ref_clk: clk@44B00000 {
compatible = "adi,axi-clkgen-2.00.a";
reg = <0x44B00000 0x10000>;
#clock-cells = <0>;
clocks = <&clkc 15>, <&clkc 15>;
clock-names = "s_axi_aclk", "clkin1";
clock-output-names = "ref_clk";
};
dac_tx_dma: dma-controller@0x44a30000 {
compatible = "adi,axi-dmac-1.00.a";
reg = <0x44a30000 0x10000>;
#dma-cells = <1>;
interrupt-parent = <&intc>;
interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clkc 15>;
adi,channels {
#size-cells = <0>;
#address-cells = <1>;
dma-channel@0 {
reg = <0>;
adi,source-bus-width = <32>;
adi,source-bus-type = <0>;
adi,destination-bus-width = <32>;
adi,destination-bus-type = <1>;
};
};
};
backend: controller@44a70000 {
compatible = "adi,axi-dac-9.1.b";
reg = <0x44a70000 0x1000>;
dmas = <&dac_tx_dma 0>;
dma-names = "tx";
#io-backend-cells = <0>;
clocks = <&ref_clk>;
bus-type = <1>; /* IIO QSPI */
};
axi-ad3552r {
compatible = "adi,ad3552r";
reset-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;
io-backends = <&backend>;
#address-cells = <1>;
#size-cells = <0>;
channel@0 {
reg = <0>;
adi,output-range-microvolt = <(-10000000) (10000000)>;
};
};
};
- 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@xxxxxxxxxx>
To: Michael Hennerich <Michael.Hennerich@xxxxxxxxxx>
To: Nuno Sá <nuno.sa@xxxxxxxxxx>
To: Jonathan Cameron <jic23@xxxxxxxxxx>
To: Rob Herring <robh@xxxxxxxxxx>
To: Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>
To: Conor Dooley <conor+dt@xxxxxxxxxx>
To: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
Cc: linux-iio@xxxxxxxxxxxxxxx
Cc: devicetree@xxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
Cc: dlechner@xxxxxxxxxxxx
Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
---
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,
--
,,, Angelo Dureghello
:: :. BayLibre -runtime team- Developer
:`___:
`____:
