Re: [PATCH v5 06/11] iio: accel: adxl345: extend sample frequency adjustments

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Apr 2025 13:41:20 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Mon, Mar 31, 2025 at 12:33 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 18 Mar 2025 23:08:38 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Introduce enums and functions to work with the sample frequency
> > > adjustments. Let the sample frequency adjust via IIO and configure
> > > a reasonable default.
> > >
> > > Replace the old static sample frequency handling. During adjustment of
> > > bw registers, measuring is disabled and afterwards enabled again.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > One minor thing inline.
> >
> >  
> > >       return -EINVAL;
> > > @@ -504,7 +581,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> > >                            int val, int val2, long mask)
> > >  {
> > >       struct adxl345_state *st = iio_priv(indio_dev);
> > > -     s64 n;
> > > +     enum adxl345_odr odr;
> > > +     int ret;
> > > +
> > > +     ret = adxl345_set_measure_en(st, false);  
> >
> > Why is this necessary but wasn't before?
> > If it should always have been done for existing calibbias etc,
> > perhaps a separate precursor patch is appropriate?
> >  
> 
> On the one side the datasheet recommends to have measurement disabled,
> when adjusting certain sensor registers, mostly related to interrupt
> events. Before the sensor was operated in FIFO_BYPASS mode only
> without using the sensor events. With interrupt based events, it will
> operate in FIFO_STREAM or similar. Then it seems to me to be a better
> approach to put it generally in standby mode when configuring
> registers to avoid ending up e.g. in FIFO overrun or the like. On the
> other side, I saw similar approaches in the sources of Analog sensors.
> Enable/disable measurement was done there in the particular functions.
> In the particular case of adxl345_write_raw, odr and range values are
> going to be set and I implement enable/disable measurement directly in
> the write_raw. In comparison to the ADXL380 (different sensor!) where
> this is done, too, but down in the specific setter functions. I can
> see a bit of an advantage, if something fails, the sensor generally
> stays turned off. I'll keep this in v6 of the patches.

Thanks for the explanation.  That answered my question nicely and
this is fine.

> 
> Pls,  note, I did not observe faulty behavior due to that or analyzed
> it thoroughly if and where it is probably better to have measurement
> turned off. At best, it won't make any difference and is probably
> rather kind of "best practice". If not, I would expect rather sporadic
> minor issues.
> 
> As always, pls consider my patch(es) as a proposal, sometimes with an
> invisible question mark ;) If you have a contrary opinion and/or
> experience, please let me know.
> 
> >  
> > > +     if (ret)
> > > +             return ret;
> > >
> > >       switch (mask) {
> > >       case IIO_CHAN_INFO_CALIBBIAS:
> > > @@ -512,20 +594,26 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> > >                * 8-bit resolution at +/- 2g, that is 4x accel data scale
> > >                * factor
> > >                */
> > > -             return regmap_write(st->regmap,
> > > -                                 ADXL345_REG_OFS_AXIS(chan->address),
> > > -                                 val / 4);
> > > +             ret = regmap_write(st->regmap,
> > > +                                ADXL345_REG_OFS_AXIS(chan->address),
> > > +                                val / 4);
> > > +             if (ret)
> > > +                     return ret;
> > > +             break;
> > >       case IIO_CHAN_INFO_SAMP_FREQ:
> > > -             n = div_s64(val * NANOHZ_PER_HZ + val2,
> > > -                         ADXL345_BASE_RATE_NANO_HZ);
> > > +             ret = adxl345_find_odr(st, val, val2, &odr);
> > > +             if (ret)
> > > +                     return ret;
> > >
> > > -             return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> > > -                                       ADXL345_BW_RATE,
> > > -                                       clamp_val(ilog2(n), 0,
> > > -                                                 ADXL345_BW_RATE));
> > > +             ret = adxl345_set_odr(st, odr);
> > > +             if (ret)
> > > +                     return ret;
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> > >       }
> > >
> > > -     return -EINVAL;
> > > +     return adxl345_set_measure_en(st, true);
> > >  }