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

[Lothar Rubusch]

On Mon, Mar 31, 2025 at 12:33 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 18 Mar 2025 23:08:38 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> 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@xxxxxxxxx>
> 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.

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);
> >  }