Message-ID: <74e0b200-d4c0-4aa3-9ee6-f49ac3f1467d@baylibre.com>
Date: Tue, 3 Sep 2024 10:34:22 +0200
From: Angelo Dureghello <adureghello@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
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

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


&amba {

     ref_clk: clk@...00000 {
         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@...4a30000 {
         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@...70000 {
         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@...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,

-- 
  ,,,      Angelo Dureghello
:: :.     BayLibre -runtime team- Developer
:`___:
  `____:

Powered by blists - more mailing lists