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) > > - 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@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,
