Message-ID: <20250421145152.22f42bc6@jic23-huawei>
Date: Mon, 21 Apr 2025 14:51:52 +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 Mon, 21 Apr 2025 15:31:19 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:

> On Mon, Apr 21, 2025 at 12:16 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > 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.
> >  
> 
> As I (begin to) understand now, in case of the sca3000, the virtual
> channel is literally an extra channel. That means, we're talking
> probably about this here down below, right?
> ...
yes. That is what I was referring to.

>  512 static const struct iio_chan_spec sca3000_channels[] = {
>  513         SCA3000_CHAN(0, IIO_MOD_X),
>  514         SCA3000_CHAN(1, IIO_MOD_Y),
>  515         SCA3000_CHAN(2, IIO_MOD_Z),
>  516         {
>  517                 .type = IIO_ACCEL,
>  518                 .modified = 1,
>  519                 .channel2 = IIO_MOD_X_AND_Y_AND_Z,
>  520                 .scan_index = -1, /* Fake channel */
>  521                 .event_spec = &sca3000_freefall_event_spec,
>  522                 .num_event_specs = 1,
>  523         },
>  524 };
> ...
> <taken from sca3000.c>
> 
> What's now missing for freefall and the ADXL345 and v7 in particular?
> - I need to provide a similar channel setup as in  the sca3000 snippet
> above, the virtual channel
> - I need to AND the axis for this channel
> - Now, with the virtual channel usage will be "separate" instead of
> "shared", which will result in a single enable handle in sysfs
> 
> Is this correct?

Yes.  I think that's all correct.

> 
> Sry, I needed to re-read your answer several times. What confuses me
> is still a bit the "virtual extra-channel". 

I made up that term for it as there is no actual traditional channel
there.  I should have given a pointer to the actual iio_chan_spec entry.
Sorry about that!

>Probably, I need to build
> up a bit more practical experience with the channels. Providing just a
> single enable handle already looks good to me. I still don't grok
> entirely where this can be / is problematic in such case, but no need
> to explain it now in full detail. If the TODOs match at least with my
> understanding I will try to implement the above points.

These events are just a bit weird when aligned with the simpler ones
we tend to get on ADCs etc.  Giving userspace the best visibility we
can is always good.  Here it is possible to get that, but in some other
cases we haven't yet come up with a good generic ABI :(

Jonathan


> 
> > Jonathan
> >  

Powered by blists - more mailing lists