> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Thursday, November 4, 2021 7:23 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 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.
Yeah, it took me a bit to remember to look at the uapi but then I found
those defines...
- Nuno Sá
