On Tue, 2 Nov 2021 10:00:58 +0000
"Sa, Nuno" [off-list ref] wrote:

quoted

From: Jonathan Cameron <jic23@kernel.org>
Sent: Saturday, October 30, 2021 5:15 PM
To: Sa, Nuno <Nuno.Sa@analog.com>
Cc: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>;
robh+dt@kernel.org; linux-iio@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Lars-Peter
Clausen [off-list ref]
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" [off-list ref] wrote:
  

quoted

Hi Jonathan,
 

quoted

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

quoted

quoted

[off-list ref]; Lars-Peter Clausen [off-list ref]
Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support  

for  

quoted

quoted

ADMV1013

On Thu, 28 Oct 2021 10:08:08 +0000
"Miclaus, Antoniu" [off-list ref] wrote:
 

quoted

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.  

quoted

(LO and sideband suppression)

On this matter, my suggestion would be to add IIO calibration  

types,  

quoted

quoted

something like:  

quoted

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  

quoted

quoted

pair?  

That's also my understanding. This kind of calibration seems to be  

very  

quoted

specific for TX local oscillators...
 

quoted

Current channel is represented as a single index, but one route to  

this  

quoted

quoted

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  

quoted

this applies to a single channel... Having this with separate indexes  

feels  

quoted

odd to me. Even though we have the phase with "0-1", this can be a  

place  

quoted

for misuse and confusion...

I was thinking about modifiers (to kind of represent differential  

channels)  

quoted

but I don't think it would work out here... What about  

IIO_CHAN_INFO_CALIBBIAS_P  

quoted

and  IIO_CHAN_INFO_CALIBBIAS_N? Or maybe just something like
IIO_CHAN_INFO_CALIBBIAS_DIFF and internally in IIO we would  

automatically  

quoted

create both P and N sysfs files since I don't think it makes senses in  

any case to  

quoted

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)?

It's a fairly small set in most cases, but as it is a bitmap that doesn't
help us.  We still need to fit in an unsigned long (so 32 bits).

Any change to how we do this may be painful.  We might just jump to the
BIT_ULL() approach for now and be a bit more resistant to adding more
entries in future.

quoted

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.

differential channel is X-Y so P-N would be how I would interpret it.

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.

ouch.  That indeed is going to blow up.  Can't have modified differential channels
and that is very hard to work around because of lack of space in the events format.

I couldn't think of a reason why we'd have differential modified channels when
I made this stuff up...

Jonathan

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

- Nuno Sá

quoted

quoted

- Nuno Sá