Message-ID: <20250421111641.1cb83848@jic23-huawei>
Date: Mon, 21 Apr 2025 11:16:41 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 eraretuya@...il.com
Subject: Re: [PATCH v6 05/11] iio: accel: adxl345: add freefall feature

On Sun, 20 Apr 2025 23:26:47 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:

> Happy Easter!
> 
> On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > On Mon, 14 Apr 2025 18:42:39 +0000
> > Lothar Rubusch <l.rubusch@...il.com> wrote:
> >  
> > > Add the freefall detection of the sensor together with a threshold and
> > > time parameter. A freefall event is detected if the measuring signal
> > > falls below the threshold.
> > >
> > > Introduce a freefall threshold stored in regmap cache, and a freefall
> > > time, having the scaled time value stored as a member variable in the
> > > state instance.
> > >  
> > Reading this I wondered whether we had the event code consistent for
> > freefall detectors... Or indeed inactivity ones (which are kind of similarish)
> >
> > :( We don't it seems.  The issue is that
> > freefall is actually that all channels are simultaneously under the the magnitude
> > threshold, not one of them.  So it should I think be
> > X_AND_Y_AND_Z not X_OR_Y_OR_Z
> >  
> 
> I change to X_AND_Y_AND_Z.
> 
> > This is as opposed to activity detectors which tend to be any axis shows
> > activity and X_OR_Y_OR_Z applies.
> >
> > Anyhow upshot is I think I lead you astray on this and we should make this
> > one IIO_MOD_X_AND_Y_AND_Z
> >
> > A few other things inline.
> >
> > Unfortunately we don't deal with these events that often so I forget
> > what we did before :(
> >  
> > > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > >  1 file changed, 125 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index c464c87033fb..ae02826e552b 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -75,6 +75,7 @@ struct adxl345_state {
> > >       u32 tap_duration_us;
> > >       u32 tap_latent_us;
> > >       u32 tap_window_us;
> > > +     u32 ff_time_ms;
> > >
> > >       __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > >  };
> > > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > >                       BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > >                       BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > >       },
> > > +     {
> > > +             /* free fall */
> > > +             .type = IIO_EV_TYPE_MAG,
> > > +             .dir = IIO_EV_DIR_FALLING,
> > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > +                     BIT(IIO_EV_INFO_VALUE) |
> > > +                     BIT(IIO_EV_INFO_PERIOD),
> > > +     },  
> > This is creating separate per axis enables, values and period. Does that make
> > sense?  If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> > and add the events to it.
> >
> > See how the sca3000 does it for example.  
> 
> Hum, I'm not sure if I understand you correctly. In my driver, I'm
> using .mask_shared_by_type, and I verified there appears only one
> enable, one value and one period handle.
> # ls -l /sys/bus/iio/devices/iio:device0/events/
> total 0
> ...
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
> ...
> 
> In the sources of sca3000.c I saw this setup with .mask_separate. So,
> there I'd expect to see separate enables per axis, or am I wrong? In
> the case of the ADXL345, there should only be one freefall enable (in
> my driver) and not per axis. So, isn't this what is currently there?
> 
> So far I only adjust the or'ing to and'ing the axis for freefall.

So this is a messy corner of the ABI (because these are tricky to describe).
Shared by type usually means there is one attribute applying to all the
axis, but that they are reported separately, or potentially multiple events
/ _OR_ form used if we can distinguish exactly what the event is.

In this case there is no way for userspace to anticipate that the event
that might be generate is X_AND_Y_AND_Z.  So for this
the ABI solution we came up with is that virtual channel and separate.

So you get something along the lines of
in_accel_x&y&z_mag_falling_en
in_accel_x&y&z_mag_falling_period
etc

The tricky remaining corner is this only makes sense if we always enable
all axis (which is typical for a freefall detector). If we get a device
that oddly has per axis free fall enables, then it would be hard and I
might argue nonsense to enable them separately anyway.  Not true
here though I think.

Note that we may well have drivers using the ABI slightly differently for
freefall events which will be at least partly because I'd forgotten how
we resolved all this complexity long ago (that sca3000 driver is ancient!)
ABI like this is tricky to fix up, but we might want to consider some duplication
in those drivers so we standardize on one form for freefall (even if we have some
stray ABI from other possible solutions).

What we should definitely do is pull together some documentation on multi channel
event handling as the ABI docs are probably not enough.

Jonathan

Powered by blists - more mailing lists