Re: [PATCH v2 11/11] iio: dac: support the ad9739a RF DAC

[Jonathan Cameron <jic23@kernel.org>]

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á 
>