[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240831123837.26a1070a@jic23-huawei>
Date: Sat, 31 Aug 2024 12:38:37 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
<Michael.Hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Olivier Moysan
<olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
dlechner@...libre.com
Subject: Re: [RFC PATCH 0/8] iio: dac: introducing ad3552r-axi
On Thu, 29 Aug 2024 14:31:58 +0200
Angelo Dureghello <adureghello@...libre.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@...afoo.de>
> To: Michael Hennerich <Michael.Hennerich@...log.com>
> To: Nuno Sá <nuno.sa@...log.com>
> To: Jonathan Cameron <jic23@...nel.org>
> To: Rob Herring <robh@...nel.org>
> To: Krzysztof Kozlowski <krzk+dt@...nel.org>
> To: Conor Dooley <conor+dt@...nel.org>
> To: Olivier Moysan <olivier.moysan@...s.st.com>
> Cc: linux-iio@...r.kernel.org
> Cc: devicetree@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: dlechner@...libre.com
>
> Signed-off-by: Angelo Dureghello <adureghello@...libre.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,
Powered by blists - more mailing lists