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

[Jonathan Cameron]

On Thu, 4 Nov 2021 08:11:12 +0000
"Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote:

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

not that it really matters for this discussion, but meh - I know where
to look :)

/**
 * IIO_EVENT_CODE() - create event identifier
 * @chan_type:	Type of the channel. Should be one of enum iio_chan_type.
 * @diff:	Whether the event is for an differential channel or not.
 * @modifier:	Modifier for the channel. Should be one of enum iio_modifier.
 * @direction:	Direction of the event. One of enum iio_event_direction.
 * @type:	Type of the event. Should be one of enum iio_event_type.
 * @chan:	Channel number for non-differential channels.
 * @chan1:	First channel number for differential channels.
 * @chan2:	Second channel number for differential channels.
 */
#define IIO_EVENT_CODE(chan_type, diff, modifier, direction,		\
		       type, chan, chan1, chan2)			\
	(((u64)type << 56) | ((u64)diff << 55) |			\
	 ((u64)direction << 48) | ((u64)modifier << 40) |		\
	 ((u64)chan_type << 32) | (((u16)chan2) << 16) | ((u16)chan1) | \
	 ((u16)chan))

I'd forgotten the odd chan vs chan1 bit :)

Otherwise, key thing is we are running out of space in the 64 bits that
are pushed through the event kfifo.

> 
> > > 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? 
https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L135

The i and q are ABI already, so we are stuck with them if we want to maintain
consistency.

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

Ah good point.  Paul's stuff was to allow userspace to basically skip these
fields which in this case would be bad...

extended info is the best plan for now. We can always change that over to
a more standard option at a later date it turns out to be necessary.

Jonathan


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