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

[Jonathan Cameron]

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

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

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

> 
> Not sure if labels could also be something to use... Any suggestion from your
> side?

Let me think a bit more on this for a day or two...

Jonathan

> 
> - Nuno Sá
>