Re: [RFC PATCH 0/8] iio: dac: introducing ad3552r-axi

[Jonathan Cameron]

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,