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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFXKEHau6n2o_MPimiEkYRNvE3TO9f5j_tDH0FwJYsk6V5B9WA@mail.gmail.com>
Date: Mon, 14 Apr 2025 13:41:20 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v5 06/11] iio: accel: adxl345: extend sample frequency adjustments

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

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