RE: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013

[Sa, Nuno]

Hi Jonathan,

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Thursday, October 28, 2021 12:31 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sa, Nuno
> <Nuno.Sa@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>
> Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for
> ADMV1013
> 
> On Thu, 28 Oct 2021 10:08:08 +0000
> "Miclaus, Antoniu" <Antoniu.Miclaus@xxxxxxxxxx> wrote:
> 
> > Hello Jonathan,
> >
> > Thanks for the review!
> >
> > Regarding the interface for the Mixer Offset adjustments:
> > ADMV1013_MIXER_OFF_ADJ_P
> > ADMV1013_MIXER_OFF_ADJ_N
> >
> > These parameters are related to the LO feedthrough offset
> calibration.
> > (LO and sideband suppression)
> >
> > On this matter, my suggestion would be to add IIO calibration types,
> something like:
> > IIO_CHAN_INFO_CALIBFEEDTROUGH_POS
> > IIO_CHAN_INFO_CALIBFEEDTROUGH_NEG
> 
> These sound too specific to me - we want an interface that is
> potentially useful
> in other places.  They are affecting the 'channel' which here is
> simply an alt voltage channel, but I'm assuming it's something like
> separate analog tweaks to the positive and negative of the differential
> pair?

That's also my understanding. This kind of calibration seems to be very
specific for TX local oscillators...

> Current channel is represented as a single index, but one route to this
> would be
> to have it as a differential pair.
> 
> out_altvoltage0-1_phase
> for the attribute that applies at the level of the differential pair and
> 
> out_altvoltage0_calibbias
> out_altvoltage1_calibbias
> For the P and N signal specific attributes.

Honestly, I'm not very enthusiastic with having out_altvoltage{0|1} as the
this applies to a single channel... Having this with separate indexes feels
odd to me. Even though we have the phase with "0-1", this can be a place
for misuse and confusion...

I was thinking about modifiers (to kind of represent differential channels)
but I don't think it would work out here... What about IIO_CHAN_INFO_CALIBBIAS_P
and  IIO_CHAN_INFO_CALIBBIAS_N? Or maybe just something like
IIO_CHAN_INFO_CALIBBIAS_DIFF and internally in IIO we would automatically
create both P and N sysfs files since I don't think it makes senses in any case to
just define one of the calibrations... Anyways, these are my 5 cents :)

- Nuno Sá