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@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > Signed-off-by: Dragos Bogdan <dragos.bogdan@xxxxxxxxxx>
> > > > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > >
> > > 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á
