Message-ID: <20250621175540.4520a6b5@jic23-huawei>
Date: Sat, 21 Jun 2025 17:55:40 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
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 Mon, 16 Jun 2025 00:20:49 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:

> 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.

Tricky corner because tap isn't a simple threshold - it it were I'd have
a cleaner argument.

If we were doing this it would need to be scalling to m/s^2 not g but
that's not important for this discussion.

Huh. For thresholds I thought we had this clear in the ABI docs, but we don't.
The ABI doc refers to having _raw_ in the name which I'm not sure has been true 
in a very long time.  The convention is intended to be if the channel
has _raw the thresholds are in that unit (i.e. ADC counts) and if not
they are in the processed value units.

It has to be this way because of non linear sensors.  We have cases
where there isn't a transform we can sensibly convert in the kernel
to set a 'raw' threshold.   (involves cube roots for instance).
As a side note, those sensors are one of the few cases where we have
both _RAW and _PROCESSED because the thresholds have to relate to _RAW
but we need _PROCESSED to give standard units.
 
Now for this case where it's kind of tangentially connected by the
particular algorithm to the raw reading things are non obvious.
The tap detector could just as easily be a threshold on jerk -
rate of change of acceleration or some 'score' calculated from
a bunch of inputs in which case we couldn't apply a scaling.

> 
> 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?

I don't think the patch is needed. For this particular parameter there
isn't a clear concept of scale (putting aside that for this particular
sensor there is one).  Thus it's a twiddle control. No need to connect
it to real world units at all.  Also change this is an ABI change
so we should do it only if we are considering the change to be fixing
a bug.

Jonathan

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

Powered by blists - more mailing lists