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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <PH0PR03MB636669F27992B59B8A7B603D99879@PH0PR03MB6366.namprd03.prod.outlook.com>
Date:   Fri, 29 Oct 2021 07:49:41 +0000
From:   "Sa, Nuno" <Nuno.Sa@...log.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>
Subject: RE: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013

Hi Jonathan,

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

- Nuno Sá