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

["Sa, Nuno" <Nuno.Sa@analog.com>]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, November 4, 2021 7:23 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
> Subject: Re: [PATCH v3 1/2] iio: frequency: admv1013: add support for
> ADMV1013
> 
> [External]
> 
> On Thu, 4 Nov 2021 08:11:12 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Wednesday, November 3, 2021 9:04 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
> > > 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@analog.com> 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.

Yeah, it took me a bit to remember to look at the uapi but then  I found
those defines...

- Nuno Sá