mbox series

[v2,00/11] iio: dac: support IIO backends on the output direction

Message ID 20240405-iio-backend-axi-dac-v2-0-293bab7d5552@analog.com
Headers show
Series iio: dac: support IIO backends on the output direction | expand

Message

Nuno Sa April 5, 2024, 2:59 p.m. UTC
Hi Jonathan,

Here it goes version 2 of the output backend series. The main points in
here:
 - The refactoring the DMA BUF api for setup. I pretty much like how it
   turned out. Note that Paul's patch ("iio: buffer-dmaengine: Support
   specifying buffer direction") had to be updated accordingly.
 - Introduction of the struct iio_info callback for getting the backend.
   I'm not sure about this one as we have no user for it and we may not
   have one for sometime. I like how the "default" implementation for
   getting the backend turned out and it should cover 99% of the cases. It
   will only fail if the iio parent device is not the same device where we
   bound the backend.
 - As mentioned above, we now get the backend from the iio device
   matching the IIO parent device with the device used when getting the
   backend. This should cover almost all the cases I think. Should be very
   unlikely to use a different device in devm_iio_backend_get() and
   devm_iio_device_alloc().

For the bindings, I still did not addressed Rob's point about dma-names.
I did reply [1] but still no feedback.

Anyways, full log:

v1:
 * https://lore.kernel.org/all/20240328-iio-backend-axi-dac-v1-0-afc808b3fde3@analog.com/

v2:
 * Patch 1:
  - New patch.

 * Patch 4:
  - Make things consistent with the triggered buffer case.

 * Patch 6:
  - Fixed description as it's an output device;
  - Avoid duplicating the "bindings" word in the commit title.
 
 * Patch 7:
  - Renamed vdd_3_3-supply -> vdd-3p3-supply;
  - Added IRQ and vref properties;
  - Avoid duplicating the "bindings" word in the commit title.

 * Patch 8:
  - New patch.

 * Patch 9:
  - Fixed some typos in kerneldocs;
  - Add iio_backend_from_indio_dev_parent(). Default way of getting backends
    from IIO devices;
  - Explicitly differentiate frontends and backends ext_info in
    iio_backend_extend_chan_spec().
  - Spell out CW as CONTINUOUS_WAVE;
  - Add _hz suffix in set_sample_rate().

 * Patch 10:
  - Rephrase comment in axi_dac_set_sample_rate() when DDS is disabled;
  - Use the new iio_dmaengine_buffer_setup_ext() API;
  - Passed tone as 0,1 value being 1 second tone.

 * Patch 11:
  - Fixed mixed spaces with tabs in ABI file and dac -> DAC;
  - Add COMPILE_TEST to kconfig;
  - Dropped operating mode enum. Use defines;
  - Add comments for IIO enum operating mode and the value we need to
    set on the device;
  - Add spaces around {} in the reg_sequence;
  - Always use Mu instead of mixture of Mu and MU;
  - Don't error out if we do not recognize the part id;
  - Make sure to deal with other errors than TIMEOUT in ad9739a_init().

[1]: https://lore.kernel.org/linux-iio/04e2a0569953792673319f7fcab3fe03e6670c03.camel@gmail.com/

---
Nuno Sa (7):
      iio: buffer-dma: add iio_dmaengine_buffer_setup()
      dt-bindings: iio: dac: add docs for AXI DAC IP
      dt-bindings: iio: dac: add docs for AD9739A
      iio: core: add get_iio_backend() callback
      iio: backend: add new functionality
      iio: dac: add support for AXI DAC IP core
      iio: dac: support the ad9739a RF DAC

Paul Cercueil (4):
      iio: buffer-dma: Rename iio_dma_buffer_data_available()
      iio: buffer-dma: Enable buffer write support
      iio: buffer-dmaengine: Support specifying buffer direction
      iio: buffer-dmaengine: Enable write support

 Documentation/ABI/testing/sysfs-bus-iio-ad9739a    |  19 +
 .../devicetree/bindings/iio/dac/adi,ad9739a.yaml   |  94 +++
 .../devicetree/bindings/iio/dac/adi,axi-dac.yaml   |  62 ++
 MAINTAINERS                                        |  17 +
 drivers/iio/adc/adi-axi-adc.c                      |  16 +-
 drivers/iio/buffer/industrialio-buffer-dma.c       | 100 +++-
 drivers/iio/buffer/industrialio-buffer-dmaengine.c |  83 +--
 drivers/iio/dac/Kconfig                            |  37 ++
 drivers/iio/dac/Makefile                           |   2 +
 drivers/iio/dac/ad9739a.c                          | 454 +++++++++++++++
 drivers/iio/dac/adi-axi-dac.c                      | 635 +++++++++++++++++++++
 drivers/iio/industrialio-backend.c                 | 179 ++++++
 include/linux/iio/backend.h                        |  49 ++
 include/linux/iio/buffer-dma.h                     |   4 +-
 include/linux/iio/buffer-dmaengine.h               |  24 +-
 include/linux/iio/iio.h                            |   2 +
 16 files changed, 1698 insertions(+), 79 deletions(-)
---
base-commit: 6020ca4de8e5404b20f15a6d9873cd6eb5f6d8d6
change-id: 20240405-iio-backend-axi-dac-be99373b036b
--

Thanks!
- Nuno Sá

Comments

Jonathan Cameron April 6, 2024, 4:19 p.m. UTC | #1
On Fri, 5 Apr 2024 16:59:58 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> Hi Jonathan,
> 
> Here it goes version 2 of the output backend series. The main points in
> here:
>  - The refactoring the DMA BUF api for setup. I pretty much like how it
>    turned out. Note that Paul's patch ("iio: buffer-dmaengine: Support
>    specifying buffer direction") had to be updated accordingly.
>  - Introduction of the struct iio_info callback for getting the backend.
>    I'm not sure about this one as we have no user for it and we may not
>    have one for sometime. I like how the "default" implementation for
>    getting the backend turned out and it should cover 99% of the cases. It
>    will only fail if the iio parent device is not the same device where we
>    bound the backend.

I've not looked at it yet, but this description makes me think perhaps
that should be ripped out for now?  We can bring it back if we ever need it.

>  - As mentioned above, we now get the backend from the iio device
>    matching the IIO parent device with the device used when getting the
>    backend. This should cover almost all the cases I think. Should be very
>    unlikely to use a different device in devm_iio_backend_get() and
>    devm_iio_device_alloc().
> 
> For the bindings, I still did not addressed Rob's point about dma-names.
> I did reply [1] but still no feedback.
> 
> Anyways, full log:
> 
> v1:
>  * https://lore.kernel.org/all/20240328-iio-backend-axi-dac-v1-0-afc808b3fde3@analog.com/
> 
> v2:
>  * Patch 1:
>   - New patch.
> 
>  * Patch 4:
>   - Make things consistent with the triggered buffer case.
> 
>  * Patch 6:
>   - Fixed description as it's an output device;
>   - Avoid duplicating the "bindings" word in the commit title.
>  
>  * Patch 7:
>   - Renamed vdd_3_3-supply -> vdd-3p3-supply;
>   - Added IRQ and vref properties;
>   - Avoid duplicating the "bindings" word in the commit title.
> 
>  * Patch 8:
>   - New patch.
> 
>  * Patch 9:
>   - Fixed some typos in kerneldocs;
>   - Add iio_backend_from_indio_dev_parent(). Default way of getting backends
>     from IIO devices;
>   - Explicitly differentiate frontends and backends ext_info in
>     iio_backend_extend_chan_spec().
>   - Spell out CW as CONTINUOUS_WAVE;
>   - Add _hz suffix in set_sample_rate().
> 
>  * Patch 10:
>   - Rephrase comment in axi_dac_set_sample_rate() when DDS is disabled;
>   - Use the new iio_dmaengine_buffer_setup_ext() API;
>   - Passed tone as 0,1 value being 1 second tone.
> 
>  * Patch 11:
>   - Fixed mixed spaces with tabs in ABI file and dac -> DAC;
>   - Add COMPILE_TEST to kconfig;
>   - Dropped operating mode enum. Use defines;
>   - Add comments for IIO enum operating mode and the value we need to
>     set on the device;
>   - Add spaces around {} in the reg_sequence;
>   - Always use Mu instead of mixture of Mu and MU;
>   - Don't error out if we do not recognize the part id;
>   - Make sure to deal with other errors than TIMEOUT in ad9739a_init().
> 
> [1]: https://lore.kernel.org/linux-iio/04e2a0569953792673319f7fcab3fe03e6670c03.camel@gmail.com/
> 
> ---
> Nuno Sa (7):
>       iio: buffer-dma: add iio_dmaengine_buffer_setup()
>       dt-bindings: iio: dac: add docs for AXI DAC IP
>       dt-bindings: iio: dac: add docs for AD9739A
>       iio: core: add get_iio_backend() callback
>       iio: backend: add new functionality
>       iio: dac: add support for AXI DAC IP core
>       iio: dac: support the ad9739a RF DAC
> 
> Paul Cercueil (4):
>       iio: buffer-dma: Rename iio_dma_buffer_data_available()
>       iio: buffer-dma: Enable buffer write support
>       iio: buffer-dmaengine: Support specifying buffer direction
>       iio: buffer-dmaengine: Enable write support
> 
>  Documentation/ABI/testing/sysfs-bus-iio-ad9739a    |  19 +
>  .../devicetree/bindings/iio/dac/adi,ad9739a.yaml   |  94 +++
>  .../devicetree/bindings/iio/dac/adi,axi-dac.yaml   |  62 ++
>  MAINTAINERS                                        |  17 +
>  drivers/iio/adc/adi-axi-adc.c                      |  16 +-
>  drivers/iio/buffer/industrialio-buffer-dma.c       | 100 +++-
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c |  83 +--
>  drivers/iio/dac/Kconfig                            |  37 ++
>  drivers/iio/dac/Makefile                           |   2 +
>  drivers/iio/dac/ad9739a.c                          | 454 +++++++++++++++
>  drivers/iio/dac/adi-axi-dac.c                      | 635 +++++++++++++++++++++
>  drivers/iio/industrialio-backend.c                 | 179 ++++++
>  include/linux/iio/backend.h                        |  49 ++
>  include/linux/iio/buffer-dma.h                     |   4 +-
>  include/linux/iio/buffer-dmaengine.h               |  24 +-
>  include/linux/iio/iio.h                            |   2 +
>  16 files changed, 1698 insertions(+), 79 deletions(-)
> ---
> base-commit: 6020ca4de8e5404b20f15a6d9873cd6eb5f6d8d6
> change-id: 20240405-iio-backend-axi-dac-be99373b036b
> --
> 
> Thanks!
> - Nuno Sá
>
Jonathan Cameron April 6, 2024, 4:23 p.m. UTC | #2
On Fri, 5 Apr 2024 17:00:01 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> From: Paul Cercueil <paul@crapouillou.net>
> 
> Adding write support to the buffer-dma code is easy - the write()
> function basically needs to do the exact same thing as the read()
> function: dequeue a block, read or write the data, enqueue the block
> when entirely processed.
> 
> Therefore, the iio_buffer_dma_read() and the new iio_buffer_dma_write()
> now both call a function iio_buffer_dma_io(), which will perform this
> task.
> 
> Note that we preemptively reset block->bytes_used to the buffer's size
> in iio_dma_buffer_request_update(), as in the future the
> iio_dma_buffer_enqueue() function won't reset it.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

One trivial comment on alignment that I noticed whilst reminding
myself of this patch. Otherwise looks good.


> +
> +/**
> + * iio_dma_buffer_read() - DMA buffer read callback
> + * @buffer: Buffer to read form
> + * @n: Number of bytes to read
> + * @user_buffer: Userspace buffer to copy the data to
> + *
> + * Should be used as the read callback for iio_buffer_access_ops
> + * struct for DMA buffers.
> + */
> +int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n,
> +	char __user *user_buffer)

Prefer aligning char with after the (

> +{
> +	return iio_dma_buffer_io(buffer, n, user_buffer, false);
> +}
>  EXPORT_SYMBOL_GPL(iio_dma_buffer_read);
>  
> +/**
> + * iio_dma_buffer_write() - DMA buffer write callback
> + * @buffer: Buffer to read form
> + * @n: Number of bytes to read
> + * @user_buffer: Userspace buffer to copy the data from
> + *
> + * Should be used as the write callback for iio_buffer_access_ops
> + * struct for DMA buffers.
> + */
> +int iio_dma_buffer_write(struct iio_buffer *buffer, size_t n,
> +			 const char __user *user_buffer)
> +{
> +	return iio_dma_buffer_io(buffer, n,
> +				 (__force __user char *)user_buffer, true);
> +}
> +EXPORT_SYMBOL_GPL(iio_dma_buffer_write);
>
Jonathan Cameron April 6, 2024, 4:32 p.m. UTC | #3
On Fri, 5 Apr 2024 17:00:07 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> This adds the needed backend ops for supporting a backend inerfacing
> with an high speed dac. The new ops are:
> 
> * data_source_set();
> * set_sampling_freq();
> * extend_chan_spec();
> * ext_info_set();
> * ext_info_get().
> 
> Also to note the new helpers that are meant to be used by the backends
> when extending an IIO channel (adding extended info):
> 
> * iio_backend_ext_info_set();
> * iio_backend_ext_info_get().
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

Whilst the code for the backend retrieval callback is simple
I wonder if we are better off just not having it for now.

Keep the infrastructure that checks for the default approach not working
but don't actually provide the alternative until we need it.

Advantage is pretty minor though so maybe just keep it.
Unless others have strong opinions, up to you to decide whether to keep it.

One trivial thing noticed inline.

> ---
>  drivers/iio/industrialio-backend.c | 179 +++++++++++++++++++++++++++++++++++++
>  include/linux/iio/backend.h        |  49 ++++++++++
>  2 files changed, 228 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> index 2fea2bbbe47f..ac554798897f 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -29,6 +29,7 @@
>   *
>   * Copyright (C) 2023-2024 Analog Devices Inc.
>   */
> +#include "asm-generic/errno-base.h"

You'll need a strong reason if you want to do that include rather than
a normal one like linux/errno.h

>  #define dev_fmt(fmt) "iio-backend: " fmt
>  
>  #include <linux/cleanup.h>
> @@ -43,10 +44,12 @@
>  #include <linux/types.h>
>  
>  #include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
>
Jonathan Cameron April 6, 2024, 4:41 p.m. UTC | #4
On Fri, 5 Apr 2024 17:00:09 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are capable
> of synthesizing wideband signals from DC up to 3 GHz.
> 
> A dual-port, source synchronous, LVDS interface simplifies the digital
> interface with existing FGPA/ASIC technology. On-chip controllers are used
> to manage external and internal clock domain variations over temperature to
> ensure reliable data transfer from the host to the DAC core.
> 
> Co-developed-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

The only thing I really have remaining questions on is the choice
of chan_spec with altvoltage and voltage channels.  Why does that
split make sense?  It's odd enough that some comments in the code would
be a good thing to add.

Jonathan

> new file mode 100644
> index 000000000000..9b91d66f826c
> --- /dev/null
> +++ b/drivers/iio/dac/ad9739a.c
> @@ -0,0 +1,454 @@

> +
> +static struct iio_chan_spec ad9739a_channels[] = {
> +	{
> +		.type = IIO_ALTVOLTAGE,

So this looks a little unusual. Perhaps some comments on why it
is appropriate to have this channel.

In reality there is only one channel I think?

> +		.indexed = 1,
> +		.output = 1,
> +		.scan_index = -1,
> +	},
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.output = 1,
> +		.ext_info = ad9739a_ext_info,
> +		.scan_type = {
> +			.sign = 's',
> +			.storagebits = 16,
> +			.realbits = 16,
> +		},
> +	}
> +};
Nuno Sá April 8, 2024, 8:41 a.m. UTC | #5
On Sat, 2024-04-06 at 17:32 +0100, Jonathan Cameron wrote:
> On Fri, 5 Apr 2024 17:00:07 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > This adds the needed backend ops for supporting a backend inerfacing
> > with an high speed dac. The new ops are:
> > 
> > * data_source_set();
> > * set_sampling_freq();
> > * extend_chan_spec();
> > * ext_info_set();
> > * ext_info_get().
> > 
> > Also to note the new helpers that are meant to be used by the backends
> > when extending an IIO channel (adding extended info):
> > 
> > * iio_backend_ext_info_set();
> > * iio_backend_ext_info_get().
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> Whilst the code for the backend retrieval callback is simple
> I wonder if we are better off just not having it for now.
> 
> Keep the infrastructure that checks for the default approach not working
> but don't actually provide the alternative until we need it.
> 

Yeps, agreed. That's why I brought it up in the cover. I'll place a comment
stating we're aware and what may be the proper solution and have it when needed.

> Advantage is pretty minor though so maybe just keep it.
> Unless others have strong opinions, up to you to decide whether to keep it.

> One trivial thing noticed inline.
> 
> > ---
> >  drivers/iio/industrialio-backend.c | 179
> > +++++++++++++++++++++++++++++++++++++
> >  include/linux/iio/backend.h        |  49 ++++++++++
> >  2 files changed, 228 insertions(+)
> > 
> > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > backend.c
> > index 2fea2bbbe47f..ac554798897f 100644
> > --- a/drivers/iio/industrialio-backend.c
> > +++ b/drivers/iio/industrialio-backend.c
> > @@ -29,6 +29,7 @@
> >   *
> >   * Copyright (C) 2023-2024 Analog Devices Inc.
> >   */
> > +#include "asm-generic/errno-base.h"
> 
> You'll need a strong reason if you want to do that include rather than
> a normal one like linux/errno.h
> 

Hmm crap, Fairly sure this was clangd automatically adding the header file.
Sometimes it's actually useful. Not in this case :)

- Nuno Sá
>
Nuno Sá April 8, 2024, 8:42 a.m. UTC | #6
On Sat, 2024-04-06 at 17:23 +0100, Jonathan Cameron wrote:
> On Fri, 5 Apr 2024 17:00:01 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > From: Paul Cercueil <paul@crapouillou.net>
> > 
> > Adding write support to the buffer-dma code is easy - the write()
> > function basically needs to do the exact same thing as the read()
> > function: dequeue a block, read or write the data, enqueue the block
> > when entirely processed.
> > 
> > Therefore, the iio_buffer_dma_read() and the new iio_buffer_dma_write()
> > now both call a function iio_buffer_dma_io(), which will perform this
> > task.
> > 
> > Note that we preemptively reset block->bytes_used to the buffer's size
> > in iio_dma_buffer_request_update(), as in the future the
> > iio_dma_buffer_enqueue() function won't reset it.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> One trivial comment on alignment that I noticed whilst reminding
> myself of this patch. Otherwise looks good.
> 
> 
> > +
> > +/**
> > + * iio_dma_buffer_read() - DMA buffer read callback
> > + * @buffer: Buffer to read form
> > + * @n: Number of bytes to read
> > + * @user_buffer: Userspace buffer to copy the data to
> > + *
> > + * Should be used as the read callback for iio_buffer_access_ops
> > + * struct for DMA buffers.
> > + */
> > +int iio_dma_buffer_read(struct iio_buffer *buffer, size_t n,
> > +	char __user *user_buffer)
> 
> Prefer aligning char with after the (

I was keeping it as it was. But I can fix it up while doing the change, yes.


- Nuno Sá
Nuno Sá April 8, 2024, 8:51 a.m. UTC | #7
On Sat, 2024-04-06 at 17:41 +0100, Jonathan Cameron wrote:
> On Fri, 5 Apr 2024 17:00:09 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are capable
> > of synthesizing wideband signals from DC up to 3 GHz.
> > 
> > A dual-port, source synchronous, LVDS interface simplifies the digital
> > interface with existing FGPA/ASIC technology. On-chip controllers are used
> > to manage external and internal clock domain variations over temperature to
> > ensure reliable data transfer from the host to the DAC core.
> > 
> > Co-developed-by: Dragos Bogdan <dragos.bogdan@analog.com>
> > Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> The only thing I really have remaining questions on is the choice
> of chan_spec with altvoltage and voltage channels.  Why does that
> split make sense?  It's odd enough that some comments in the code would
> be a good thing to add.
> 
> Jonathan
> 
> > new file mode 100644
> > index 000000000000..9b91d66f826c
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad9739a.c
> > @@ -0,0 +1,454 @@
> 
> > +
> > +static struct iio_chan_spec ad9739a_channels[] = {
> > +	{
> > +		.type = IIO_ALTVOLTAGE,
> 
> So this looks a little unusual. Perhaps some comments on why it
> is appropriate to have this channel.
> 
> In reality there is only one channel I think?

Yeah, I had this same discussion internally and was also thinking in having one
channel (just ALTVOLTAGE). I ended up doing it as we have done it internally so
far. The reasoning is that we have two sources of data:

ALTVOLTAGE: It's the internally continuous wave the backend can generate. That
is in fact alternate voltage.

VOLTAGE: Is kind of what I call external source where we assume is just typical
DAC data and that typically is VOLTAGE (but for a dac like this, I think it may
very well be, if not most of the time, also alternate - the thing is, we can't
know for sure as we should be able to have both)

- Nuno Sá
Jonathan Cameron April 13, 2024, 11 a.m. UTC | #8
On Mon, 08 Apr 2024 10:51:43 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2024-04-06 at 17:41 +0100, Jonathan Cameron wrote:
> > On Fri, 5 Apr 2024 17:00:09 +0200
> > Nuno Sa <nuno.sa@analog.com> wrote:
> >   
> > > The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are capable
> > > of synthesizing wideband signals from DC up to 3 GHz.
> > > 
> > > A dual-port, source synchronous, LVDS interface simplifies the digital
> > > interface with existing FGPA/ASIC technology. On-chip controllers are used
> > > to manage external and internal clock domain variations over temperature to
> > > ensure reliable data transfer from the host to the DAC core.
> > > 
> > > Co-developed-by: Dragos Bogdan <dragos.bogdan@analog.com>
> > > Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>  
> > 
> > The only thing I really have remaining questions on is the choice
> > of chan_spec with altvoltage and voltage channels.  Why does that
> > split make sense?  It's odd enough that some comments in the code would
> > be a good thing to add.
> > 
> > Jonathan
> >   
> > > new file mode 100644
> > > index 000000000000..9b91d66f826c
> > > --- /dev/null
> > > +++ b/drivers/iio/dac/ad9739a.c
> > > @@ -0,0 +1,454 @@  
> >   
> > > +
> > > +static struct iio_chan_spec ad9739a_channels[] = {
> > > +	{
> > > +		.type = IIO_ALTVOLTAGE,  
> > 
> > So this looks a little unusual. Perhaps some comments on why it
> > is appropriate to have this channel.
> > 
> > In reality there is only one channel I think?  
> 
> Yeah, I had this same discussion internally and was also thinking in having one
> channel (just ALTVOLTAGE). I ended up doing it as we have done it internally so
> far. The reasoning is that we have two sources of data:
> 
> ALTVOLTAGE: It's the internally continuous wave the backend can generate. That
> is in fact alternate voltage.
> 
> VOLTAGE: Is kind of what I call external source where we assume is just typical
> DAC data and that typically is VOLTAGE (but for a dac like this, I think it may
> very well be, if not most of the time, also alternate - the thing is, we can't
> know for sure as we should be able to have both)
> 

Ok. That makes some sense.  These are sort of different channels even if they
come out of the same physical pin and the phase etc definitions only make sense
for the DDS case.

The operating mode being tied to only the VOLTAGE channel is a little odd but
I suppose it works  if we think of it as

   DDS altvotage ----inputto----\ 
                                  MUX -->   voltage channel. --> pin.
   DAC data      ----input to---/

I we really wanted to make this complex, we'd use an actual IIO Mux for that
but that is going to lead to a more complex driver for what is a bit of a special
case so I don't think we need to do so.

Jonathan

> - Nuno Sá 
>
Nuno Sá April 15, 2024, 12:28 p.m. UTC | #9
On Sat, 2024-04-13 at 12:00 +0100, Jonathan Cameron wrote:
> On Mon, 08 Apr 2024 10:51:43 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Sat, 2024-04-06 at 17:41 +0100, Jonathan Cameron wrote:
> > > On Fri, 5 Apr 2024 17:00:09 +0200
> > > Nuno Sa <nuno.sa@analog.com> wrote:
> > >   
> > > > The AD9739A is a 14-bit, 2.5 GSPS high performance RF DACs that are
> > > > capable
> > > > of synthesizing wideband signals from DC up to 3 GHz.
> > > > 
> > > > A dual-port, source synchronous, LVDS interface simplifies the digital
> > > > interface with existing FGPA/ASIC technology. On-chip controllers are
> > > > used
> > > > to manage external and internal clock domain variations over temperature
> > > > to
> > > > ensure reliable data transfer from the host to the DAC core.
> > > > 
> > > > Co-developed-by: Dragos Bogdan <dragos.bogdan@analog.com>
> > > > Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>  
> > > 
> > > The only thing I really have remaining questions on is the choice
> > > of chan_spec with altvoltage and voltage channels.  Why does that
> > > split make sense?  It's odd enough that some comments in the code would
> > > be a good thing to add.
> > > 
> > > Jonathan
> > >   
> > > > new file mode 100644
> > > > index 000000000000..9b91d66f826c
> > > > --- /dev/null
> > > > +++ b/drivers/iio/dac/ad9739a.c
> > > > @@ -0,0 +1,454 @@  
> > >   
> > > > +
> > > > +static struct iio_chan_spec ad9739a_channels[] = {
> > > > +	{
> > > > +		.type = IIO_ALTVOLTAGE,  
> > > 
> > > So this looks a little unusual. Perhaps some comments on why it
> > > is appropriate to have this channel.
> > > 
> > > In reality there is only one channel I think?  
> > 
> > Yeah, I had this same discussion internally and was also thinking in having
> > one
> > channel (just ALTVOLTAGE). I ended up doing it as we have done it internally
> > so
> > far. The reasoning is that we have two sources of data:
> > 
> > ALTVOLTAGE: It's the internally continuous wave the backend can generate.
> > That
> > is in fact alternate voltage.
> > 
> > VOLTAGE: Is kind of what I call external source where we assume is just
> > typical
> > DAC data and that typically is VOLTAGE (but for a dac like this, I think it
> > may
> > very well be, if not most of the time, also alternate - the thing is, we
> > can't
> > know for sure as we should be able to have both)
> > 
> 
> Ok. That makes some sense.  These are sort of different channels even if they
> come out of the same physical pin and the phase etc definitions only make
> sense
> for the DDS case.
> 
> The operating mode being tied to only the VOLTAGE channel is a little odd but
> I suppose it works  if we think of it as
> 
>    DDS altvotage ----inputto----\ 
>                                   MUX -->   voltage channel. --> pin.
>    DAC data      ----input to---/
> 

Exactly. That's pretty much the dance we have when enabling/disabling buffering.

> I we really wanted to make this complex, we'd use an actual IIO Mux for that
> but that is going to lead to a more complex driver for what is a bit of a
> special
> case so I don't think we need to do so.
> 

Agreed...

I'm also not sure if you missed or just did not had the time but I already sent
v3 out last Friday :). I kept the two channels with a comment on it.

- Nuno Sá