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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 29 Oct 2021 07:49:41 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> Hi Jonathan,
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Thursday, October 28, 2021 12:31 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa, Nuno
> > <Nuno.Sa@analog.com>; Lars-Peter Clausen <lars@metafoo.de>
> > 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@analog.com> 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.

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

Jonathan

> 
> - Nuno Sá