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

[Sa, Nuno]

> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Wednesday, November 3, 2021 9:04 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>;
> robh+dt@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/2] iio: frequency: admv1013: add support for
> ADMV1013
> 
> [External]
> 
> On Tue, 2 Nov 2021 10:09:15 +0000
> "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote:
> 
> > > +#define ADMV1013_CHAN_PHASE(_channel, rf_comp) {		\
> > > +	.type = IIO_ALTVOLTAGE,					\
> > > +	.modified = 1,						\
> > > +	.output = 1,						\
> > > +	.indexed = 1,						\
> > > +	.channel2 = IIO_MOD_##rf_comp,				\
> > > +	.channel = _channel,					\
> > > +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE)		\
> > > +	}
> > > +
> > > +#define ADMV1013_CHAN_CALIB(_channel, rf_comp) {		\
> > > +	.type = IIO_ALTVOLTAGE,					\
> > > +	.modified = 1,						\
> > > +	.output = 1,						\
> > > +	.indexed = 1,						\
> > > +	.channel2 = IIO_MOD_##rf_comp,				\
> > > +	.channel = _channel,					\
> > > +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBBIAS)	\
> > > +	}
> > > +
> > > +static const struct iio_chan_spec admv1013_channels[] = {
> > > +	ADMV1013_CHAN_PHASE(0, I),
> > > +	ADMV1013_CHAN_PHASE(0, Q),
> > > +	ADMV1013_CHAN_CALIB(0, I),
> > > +	ADMV1013_CHAN_CALIB(0, Q),
> > > +	ADMV1013_CHAN_CALIB(1, I),
> > > +	ADMV1013_CHAN_CALIB(1, Q),
> > > +};
> > > +
> >
> > Hmm, If I'm not missing nothing this leads to something like:
> >
> > out_altvoltage0_i_phase
> > out_altvoltage0_q_phase
> > out_altvoltage0_i_calibbias
> > out_altvoltage0_q_calibbias
> > out_altvoltage1_i_calibbias
> > out_altvoltage1_q_calibbias
> >
> > To me it is really non obvious that index 1 also applies to the same
> > channel. I see that we have this like this probably because we
> > can't use modified and differential at the same time, right?
> >
> 
> Indeed, this is the problem you mentioned in the discussion on v2
> My suggestion of making it clear it is a differential channel and then
> applying calibbias to the parts doesn't work as it would need to
> have both modifiers and a second channel index.
> As for why that is an issue, it comes down to trying to keep the
> event interface descriptive, but still minimal.  We basically ran
> out of bits and at the time I couldn't think of a reason we'd want
> both differential and a modifier...

I'm not really seeing the issue with the event interface but that is mostly
because I still never had to deal with it, so I never looked that deeply into
the code. Might be a good time know :)

> > Jonathan, I'm not sure what should be the done here... From the top
> of my
> > head  I guess we can either:
> >
> > * drop the modifiers (not perfect but also not that bad IMO)
> > * tweak something adding extended info (not really neat)
> True, it's not neat but might be the way forwards for 'now'.. We don't
> have
> events or anything on this driver, so we could make it look right
> without any
> risk of not being able to extend it.  However it will probably come back
> to bite
> us and userspace may not expect whatever we do.
> Horrible though it is you could use extend_name - which was originally
> intended
> to let us 'label special purpose channels'.  It was a bad idea, but is there
> and
> not going way any time soon.
> 
> If you did the differential thing and set extend_name = "i" etc that
> might get us around the internal issue of reusing channel2 for mod and
> differential
> channel.

Couldn't we use the label to achieve kind of the same? Or do you think
that having the "i" and "q" in the filenames is really mandatory? I can
understand if you think they are as they are valid modifiers. OTOH,
IIRC, with the latest patches from Paul, adding the extended_name will
automatically get us the label sysfs file which might be a little odd but I
guess that's already true for all the legacy drivers using it... So yeah,
between this or extended info, we might have our "band aid" for this.

> > * or handling this in the core with a new variable. Of course, we
> would need
> > to be careful to not break existing drivers...
> 
> We would end up something hardly ever used so I'm doubtful that's a
> good
> idea until we have some visibility of how common it would be.

True, most likely we would end up with a public variable only used in
this use case... Better to wait if some users like this pop up.

- Nuno Sá