Message-ID: <20250406121825.3d4836b1@jic23-huawei>
Date: Sun, 6 Apr 2025 12:18:25 +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 v5 05/11] iio: accel: adxl345: add freefall feature

On Mon, 31 Mar 2025 19:23:22 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:

> Hi Jonathan & IIO Mailing List'ers
> 
> On Mon, Mar 31, 2025 at 12:28 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > On Tue, 18 Mar 2025 23:08:37 +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.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>  
> > Hi Lothar,
> >
> > Apologies for the slow review!  Just catching up after travel
> > and I did it reverse order.  
> 
> No problem a all, hope you had a great trip! I'm glad this goes for
> another version. In the meanwhile I was messing with the zephyr driver
> implementation for this sensor and had some findings and final
> thoughts about the ADXL345.
> 
> First, set_measure_en() I use to enable/disable the measurement by
> setting a bit in the POWER_CTL register using regmap_write(). This was
> ok until adding the act/inact feature. For adding power modes to
> inactivity, I'm going to set the link bit in the same POWER_CTL reg.
> So you already guess, yet another call  to set_measure_en() simply
> wipes this link bit out immediately. I'll probably replace
> regmap_write() using regmap_update_bits() still in this series.
> 
> Second, while playing with the zephyr driver and another setup I
> discovered, that probably the sensor is capable of mapping events to
> both interrupt lines in parallel. Currently, either all events to to
> INT1 or to INT2, not both. This affects actually 8 interrupt events:
> data ready, single tap, double tap, activity, inactivity, free fall,
> watermark, overrun. Actually they could individually be mapped either
> to INT1 or INT2.
> Initially I assumed they all need to go either to INT1 or INT2
> altogether. I appologize for this, I was wrong due to the breakout
> board I was using. That's a kind of crazy feature, and I think of
> implement it perhaps in a follow up series. Anyway, I was curisous
> about the approach, currently only can think of introducing 8x new DTS
> properties. Are you aware of sensors with similar features, what is
> usually the approach how to implement that? What is your oppinion on
> this?

It's not a board wiring thing if both are available (unless we are
dealing with the complexity of external hardware driven by the interrupts).
It is a policy thing for the driver.  So all DT should tell us is what
is wired.  Note this is very common on more complex sensors (take a look
at all the ADIS IMUs for instance). In practice it hasn't often proved
useful to route different interrupts to different pins so we haven't
bothered.  Linux drivers tend to always check what the interrupt was
anyway (to detect false interrupts, share lines etc) so once you are doing
that there is little point in splitting the handler in two. For RTOS
cases it may make more sense.


> 
> Third item, there are 4 FIFO modes: Bypass and Streaming are currently
> used. There is another FIFO mode and further a Trigger mode i.e. only
> when the sensor got triggered it fills up the FIFO with data (also
> this is mappable by the INT1 or INT2 line then).

ah. This tends to happen in devices that do things like impact detection.
We have never really supported that properly (and the one driver that
was in staging went away recently as it's now end of life and no
one seemed to care).

> What would be a way
> to configure such feature? I know many of the Analog accelerometers
> seem to have FIFO modes. Is this to be configured by DT properties?

This is definitely policy so doesn't belong in DT at all.

> What would be means to configure it? Also, this would be a separate
> patch set.

The closest we get to this is probably the complex stm32 triggering
but that is not necessarily something I'd base such a feature on as
it is really about interactions across a lot of different system
elements even though it incorporates a grab N samples on event Y element.

We'd need a way to add new richer description around the fifo + trigger.
It's kind of a mixture of both because a trigger causing a bunch of
scan's to be taken. In general that might be limited by the fifo or
it might just be always take 'N' samples.

Before spending too much time on this I'd consider whether there is
a use case to justify the work.  They exist on paper, but we haven't
yet had anyone actually implement it in a driver which makes me
wonder if people care!

Jonathan

> 
> Best,
> L
> 
> >  
> > > +
> > > +static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
> > > +{
> > > +     unsigned int regval, ff_threshold;
> > > +     const unsigned int freefall_mask = 0x02;  
> >
> > Where did this mask come from?   Feels like it should be a define
> > (just use ADXL345_INT_FREE_FALL probably)
> > or if not that at lest use BIT(1) to make it clear it's a bit rather
> > than the number 2.
> >  
> > > +     bool en;
> > > +     int ret;
> > > +
> > > +     ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
> > > +
> > > +     regval = en ? ADXL345_INT_FREE_FALL : 0x00;
> > > +
> > > +     return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > > +                               freefall_mask, regval);
> > > +}  
> >
> > Jonathan  

Powered by blists - more mailing lists