linux-kernel - Re: [PATCH v9 01/11] iio: accel: adxl345: apply scale factor to tap threshold

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] [day] [month] [year] [list]

Message-ID: <CAFXKEHbh=_A9WvEvkBaz9nNEGX5bxWu2sFvbMtqLM-Ag0cdY0A@mail.gmail.com>
Date: Mon, 16 Jun 2025 00:20:49 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, corbet@....net, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v9 01/11] iio: accel: adxl345: apply scale factor to tap threshold

On Sat, Jun 14, 2025 at 3:42 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Tue, 10 Jun 2025 21:59:23 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > The threshold for tap detection was still not scaled. The datasheet sets
> > a scale factor of 62.5mg/LSB. Remove commit about not scaled threshold
> > for tap detection, and apply scaling to it.
> >
>
> Given tap detection algorithms are not generally well defined and not a simple
> threshold (generally) what scaling should we be aiming for here?
> Even if it were a simple threshold, when a channel provides _raw the
> expectation is that event config is vs _raw, not the base units.
>
> So if this doesn't care about the current fullscale range (which the
> comment implied was the case) it would need to rescale when the
> IIO_INFO_SCALE changes.
>
> That comment is I think indicating we decided to gloss over the
> detail because it's going into a (potentially) non trivial algorithm anyway.
>
> Jonathan
>

Well, the tap threshold so far was around in "raw" LSB bits. At that
time I only left the comment that the value is not scaled. The current
patch is just putting now the scale factor and the sysfs handle then
will take values of 'g' and not just raw bits. This is like for the
other scaled values such as periods.

I think at the time I left the thresholds a bit out, because for me
it's clear what a time is. But I'm not sure, if actually the
thresholds are going so much by the unit values. So, in particular
what is missing here? Is it just about the commit message, or does it
need technical further adjustments?

Best,
L
>
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 7c093c0241de..d80efb68d113 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -697,17 +697,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> >               switch (info) {
> >               case IIO_EV_INFO_VALUE:
> >                       /*
> > -                      * The scale factor would be 62.5mg/LSB (i.e. 0xFF = 16g) but
> > -                      * not applied here. In context of this general purpose sensor,
> > -                      * what imports is rather signal intensity than the absolute
> > -                      * measured g value.
> > +                      * Scale factor is 62.5mg/LSB i.e. 0xff = 16g
> >                        */
> >                       ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP,
> >                                         &tap_threshold);
> >                       if (ret)
> >                               return ret;
> > -                     *val = sign_extend32(tap_threshold, 7);
> > -                     return IIO_VAL_INT;
> > +                     *val = 62500 * sign_extend32(tap_threshold, 7);
> > +                     *val2 = MICRO;
> > +                     return IIO_VAL_FRACTIONAL;
> >               case IIO_EV_INFO_TIMEOUT:
> >                       *val = st->tap_duration_us;
> >                       *val2 = 1000000;
> > @@ -746,6 +744,7 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> >       case IIO_EV_TYPE_GESTURE:
> >               switch (info) {
> >               case IIO_EV_INFO_VALUE:
> > +                     val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
> >                       ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
> >                                          min(val, 0xFF));
> >                       if (ret)
>