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

[Sa, Nuno]

> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Saturday, October 30, 2021 5:15 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>;
> robh+dt@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Lars-Peter
> Clausen <lars@xxxxxxxxxx>
> Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for
> ADMV1013
> 
> [External]
> 
> On Fri, 29 Oct 2021 07:49:41 +0000
> "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote:
> 
> > 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 :)
> 
> Definitely not a modifier.  It's almost arguable that these are different
> characteristics of the channel so I can live with the ABI perhaps, but
> unless this is a very common thing I'm not sure I want to burn 2 chan
> info
> bits for them. Note we are running very low on those anyway without
> changing
> how those are handled.  We will need to tackle that anyway, but let's
> not
> tie that to this driver.

Hmm, Honestly I was not even thinking about the mask size and used
bits. But I guess it's very unlikely for a driver to define lots of bits in one
of the masks (just curious)?

> I don't like the idea of adding core magic to spin multiple files from one
> without more usecases.  So for now use extended attributes for these
> if
> we go this way.
> 
> I think I still prefer the separate channels approach.  Note this is how
> we deal with devices that are capable of either single ended or
> differential
> operation.  The channel numbering is reflecting the wire in both cases.
> However, I'm not sure we've ever made it clear the ABI would apply
> like this.
> We may have devices that use this interface but expect it to not apply
> to
> the differential case....
> 

Well, you actually gave me something to think about over the weekend and
I'm getting more convinced with the ABI you purposed here. Getting all
the bits in one differential channel could lead to having to "duplicate" bit masks
to differentiate between P and N. Or we would have to do some non obvious
handling in the core as I was suggesting.

With your ABI, the "single ended" files already differentiate the pair. The only
thing we might be missing is to have a clear rule in the ABI docs. Like, if we have
 
out_altvoltageX-Y_phase and then 

out_altvoltageX_calibbias
out_altvoltageY_calibbias,

it should be clear that X is the N part of the pair while Y is P. Or the other way
around... The point is to have a clear rule.

However, looking at the new series spin, it looks to me that we have an issue
that Antoniu might have to address in the series... These channels are both differential
and use modifiers and If I'm not missing nothing, we use channel2 for both cases.

I will leave a comment in the series which might be better...

- Nuno Sá
> 
> >
> > - Nuno Sá