Re: [PATCH 1/3] iio: gyro: adxrs290: Add triggered buffer support

[Jonathan Cameron]

On Thu, 3 Sep 2020 18:07:00 +0530
Nishant Malpani <nish.malpani25@xxxxxxxxx> wrote:

> Hello,
> 
> Thanks for the review, Jonathan. Comments inline...
> 
...

> > >
> > > +static void adxrs290_chip_off_action(void *data)
> > > +{
> > > +     struct iio_dev *indio_dev = data;
> > > +
> > > +     adxrs290_set_mode(indio_dev, ADXRS290_MODE_STANDBY);
> > > +}
> > > +
> > >  static int adxrs290_read_raw(struct iio_dev *indio_dev,
> > >                            struct iio_chan_spec const *chan,
> > >                            int *val,
> > > @@ -215,24 +287,34 @@ static int adxrs290_read_raw(struct iio_dev *indio_dev,
> > >
> > >       switch (mask) {
> > >       case IIO_CHAN_INFO_RAW:
> > > +             ret = iio_device_claim_direct_mode(indio_dev);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > >               switch (chan->type) {
> > >               case IIO_ANGL_VEL:
> > >                       ret = adxrs290_get_rate_data(indio_dev,
> > >                                                    ADXRS290_READ_REG(chan->address),
> > >                                                    val);
> > >                       if (ret < 0)
> > > -                             return ret;
> > > +                             goto err_release;
> > >
> > > -                     return IIO_VAL_INT;
> > > +                     ret = IIO_VAL_INT;
> > > +                     break;
> > >               case IIO_TEMP:
> > >                       ret = adxrs290_get_temp_data(indio_dev, val);
> > >                       if (ret < 0)
> > > -                             return ret;
> > > +                             goto err_release;
> > >
> > > -                     return IIO_VAL_INT;
> > > +                     ret = IIO_VAL_INT;
> > > +                     break;
> > >               default:
> > > -                     return -EINVAL;
> > > +                     ret = -EINVAL;
> > > +                     break;
> > >               }
> > > +err_release:
> > > +             iio_device_release_direct_mode(indio_dev);
> > > +             return ret;
> > >       case IIO_CHAN_INFO_SCALE:
> > >               switch (chan->type) {
> > >               case IIO_ANGL_VEL:
> > > @@ -279,34 +361,53 @@ static int adxrs290_write_raw(struct iio_dev *indio_dev,
> > >                             long mask)
> > >  {
> > >       struct adxrs290_state *st = iio_priv(indio_dev);
> > > -     int lpf_idx, hpf_idx;
> > > +     int ret, lpf_idx, hpf_idx;
> > > +
> > > +     ret = iio_device_claim_direct_mode(indio_dev);  
> >
> > Is there anything in the datasheet that says we can't change the filters
> > whilst doing captures?  Might get crazy data but if the datasheet doesn't
> > want against a particular action, then we tend not to try and prevent it.
> >  
> 
> The datasheet, to my knowledge, explicitly doesn't mention any
> restriction on changing the filter settings during a capture.
> 
> Quoting the datasheet, "The group delay of the wideband filter option
> is less than 0.5ms" (pg. 11) - during an ongoing capture if one
> re-configures the filter, how do we address this?

To be honest, I'm not really sure what that means, so will if you think
it makes sense it is fine to keep protections to not allow a filter update
during buffered capture.

> 
> > > +     if (ret)
> > > +             return ret;
...
> > >  static int adxrs290_probe(struct spi_device *spi)
> > >  {
> > >       struct iio_dev *indio_dev;
> > > @@ -384,6 +609,7 @@ static int adxrs290_probe(struct spi_device *spi)
> > >       indio_dev->channels = adxrs290_channels;
> > >       indio_dev->num_channels = ARRAY_SIZE(adxrs290_channels);
> > >       indio_dev->info = &adxrs290_info;
> > > +     indio_dev->available_scan_masks = adxrs290_avail_scan_masks;
> > >
> > >       mutex_init(&st->lock);
> > >
> > > @@ -416,6 +642,12 @@ static int adxrs290_probe(struct spi_device *spi)
> > >       /* max transition time to measurement mode */
> > >       msleep(ADXRS290_MAX_TRANSITION_TIME_MS);
> > >
> > > +     ret = devm_add_action_or_reset(&spi->dev,
> > > +                                    adxrs290_chip_off_action,
> > > +                                    indio_dev);  
> >
> > What is this unwinding?  We should only be using devm to undo things
> > that have been 'done' in probe and the association should be clear.
> >  
> 
> This unwinding is for placing the device **back** to its default
> 'STANDBY' mode. The device, during the probe, was put to 'MEASUREMENT'
> mode during 'adxrs290_initial_setup()' a few lines back.
> 
> How else do you suggest I highlight the association?

Perhaps put it inside the call to initial_setup() so it is clearly
paired with the 'forwards' action.

Jonathan