Re: [PATCH v5 05/11] iio: accel: adxl345: add freefall feature

[Lothar Rubusch <l.rubusch@gmail.com>]

Hi Jonathan & IIO Mailing List'ers

On Mon, Mar 31, 2025 at 12:28 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 18 Mar 2025 23:08:37 +0000
> Lothar Rubusch <l.rubusch@gmail.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@gmail.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?

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). 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?
What would be means to configure it? Also, this would be a separate
patch set.

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