Re: [PATCH v2 3/3] iio: imu: adis16475.c: Add delta angle and delta velocity channels

[Jonathan Cameron]

On Thu, 20 Jul 2023 10:58:51 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> On Wed, 2023-07-19 at 15:31 +0300, Ramona Bolboaca wrote:
> > Add support for delta angle and delta velocity raw and buffer
> > readings to adis16475 driver.
> > 
> > Signed-off-by: Ramona Bolboaca <ramona.bolboaca@xxxxxxxxxx>
> > ---
Some follow up comments..

> > +
> > +#define ADIS16475_DELTANG_CHAN(_mod) \
> > +       ADIS16475_MOD_CHAN_DELTA(IIO_ROT, IIO_MOD_ ## _mod, \
> > +                          ADIS16475_REG_ ## _mod ## _DELTANG_L,
> > ADIS16475_SCAN_DELTANG_ ## _mod, 32, 32)
> > +
> > +#define ADIS16475_DELTVEL_CHAN(_mod) \
> > +       ADIS16475_MOD_CHAN_DELTA(IIO_VELOCITY, IIO_MOD_ ## _mod, \
> > +                          ADIS16475_REG_ ## _mod ## _DELTVEL_L,
> > ADIS16475_SCAN_DELTVEL_ ## _mod, 32, 32)
> > +
> > +#define ADIS16475_DELTANG_CHAN_NO_SCAN(_mod) \
> > +       ADIS16475_MOD_CHAN_DELTA(IIO_ROT, IIO_MOD_ ## _mod, \
> > +                          ADIS16475_REG_ ## _mod ## _DELTANG_L, -1, 32, 32)
> > +
> > +#define ADIS16475_DELTVEL_CHAN_NO_SCAN(_mod) \
> > +       ADIS16475_MOD_CHAN_DELTA(IIO_VELOCITY, IIO_MOD_ ## _mod, \
> > +                          ADIS16475_REG_ ## _mod ## _DELTVEL_L, -1, 32, 32)

I don't think we've come to a conclusion on whether IIO_ROT and IIO_VELOCITY
are valid descriptions of these channels..

> > +
> >  static const struct iio_chan_spec adis16475_channels[] = {
> >         ADIS16475_GYRO_CHANNEL(X),
> >         ADIS16475_GYRO_CHANNEL(Y),
> > @@ -559,7 +619,30 @@ static const struct iio_chan_spec adis16475_channels[] =
> > {
> >         ADIS16475_ACCEL_CHANNEL(Y),
> >         ADIS16475_ACCEL_CHANNEL(Z),
> >         ADIS16475_TEMP_CHANNEL(),
> > -       IIO_CHAN_SOFT_TIMESTAMP(7)
> > +       IIO_CHAN_SOFT_TIMESTAMP(7),
> > +       ADIS16475_DELTANG_CHAN_NO_SCAN(X),
> > +       ADIS16475_DELTANG_CHAN_NO_SCAN(Y),
> > +       ADIS16475_DELTANG_CHAN_NO_SCAN(Z),
> > +       ADIS16475_DELTVEL_CHAN_NO_SCAN(X),
> > +       ADIS16475_DELTVEL_CHAN_NO_SCAN(Y),
> > +       ADIS16475_DELTVEL_CHAN_NO_SCAN(Z)
> > +};
> > +
> > +static const struct iio_chan_spec adis16500_channels[] = {
> > +       ADIS16475_GYRO_CHANNEL(X),
> > +       ADIS16475_GYRO_CHANNEL(Y),
> > +       ADIS16475_GYRO_CHANNEL(Z),
> > +       ADIS16475_ACCEL_CHANNEL(X),
> > +       ADIS16475_ACCEL_CHANNEL(Y),
> > +       ADIS16475_ACCEL_CHANNEL(Z),
> > +       ADIS16475_TEMP_CHANNEL(),
> > +       ADIS16475_DELTANG_CHAN(X),
> > +       ADIS16475_DELTANG_CHAN(Y),
> > +       ADIS16475_DELTANG_CHAN(Z),
> > +       ADIS16475_DELTVEL_CHAN(X),
> > +       ADIS16475_DELTVEL_CHAN(Y),
> > +       ADIS16475_DELTVEL_CHAN(Z),
> > +       IIO_CHAN_SOFT_TIMESTAMP(13)
> >  };  
> 
> Hmm, IIRC I suggested this to you so apologizes. But I'm not sure we should do
> this. In theory, I think we provide all the interfaces so an userspace app would
> still work by changing the timestamp index but who knows what apps might be
> assuming... So maybe, it's just safer to keep timestamp at index 7.

That stops you using iio_push_to_buffers_with_timestamp()

This is in the category of cross our fingers no one notices the ABI change.
So I'd go ahead with moving the timestamp channel and hope no one has hand rolled
code that assume the channel won't change index.

> 
> I guess it will also make the available scan_masks neater.
> 

...


> > @@ -972,10 +1095,40 @@ static const struct adis16475_chip_info
> > adis16475_chip_info[] = {
> >         },
> >  };
> >  
> > +static const unsigned long adis16475_avail_scan_masks[] = {
> > +       ADIS16500_BURST_DATA_SEL_0_CHN_MASK,
> > +       ADIS16500_BURST_DATA_SEL_1_CHN_MASK,
> > +       0
> > +};
> > +  
> 
> Hmm I guess this ok if we don't have strict scan_mask match. AFAICT, that's only
> the case for HARDWARE buffering and not likely to change... right? Jonathan?

Should be.  The core will deal with demuxing the data otherwise.

> 
> > +static int adis16475_update_scan_mode(struct iio_dev *indio_dev,
> > +                                     const unsigned long *scan_mask)
> > +{
> > +       u16 en;
> > +       int ret;
> > +       struct adis16475 *st = iio_priv(indio_dev);
> > +
> > +       if (st->info->has_burst32) {  
> 
> Hmm, I get that buffering these channels are only available in devices that have
> burst32. But I would not assume that as there's no guarantee that will always be
> the case. Given that's straight to just have another flag for the presence of
> these channels in the burst data, I would prefer to have one right now.

Agreed.

> 
> As a nit I would flip the logic in here:
> 
> if (!st->info->has_burst32)
>     return adis_update_scan_mode(indio_dev, scan_mask);
> 
> Then you could keep indentation and even save some lines of code. Anyways, just
> my personal preference.
> 
> > +               if (*scan_mask & ADIS16500_BURST_DATA_SEL_0_CHN_MASK)
> > +                       en = ADIS16500_BURST_DATA_SEL(0);
> > +               else if (*scan_mask & ADIS16500_BURST_DATA_SEL_1_CHN_MASK)
> > +                       en = ADIS16500_BURST_DATA_SEL(1);
> > +               else
> > +                       return -EINVAL;
> > +  
> 
> I don't think the above is ever possible since you're guaranteed that a subset
> of the available_scan_masks is set. So, if() .. else should be enough.

I'm not 100% sure you can't just have a software timestamp even with available_scan_masks.
Definitely worth checking if you are going to drop the else.

> 
> Moreover will this actually work with BIT(13) or BIT(6) set? Won't we always
> fall in the first case if one of those bits are set even if we want
> ADIS16500_BURST_DATA_SEL(1)? So I guess you should explicitly check against
> GENMASK(5, 0) for normal accel/gyro or GENMASK(13, 8) for delta stuff. Right?
> 
> - Nuno Sá
>