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

["Nuno Sá" <noname.nuno@gmail.com>]

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á