| Message ID | 20250318230843.76068-6-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: add interrupt based sensor events | expand |
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. > + > +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
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
On Mon, 31 Mar 2025 19:23:22 +0200 Lothar Rubusch <l.rubusch@gmail.com> wrote: > 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? 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
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c index 52daf46c4acd..76ee657e958a 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), + }, }; #define ADXL345_CHANNEL(index, reg, axis) { \ @@ -376,6 +385,64 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int, return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us); } +/* freefall */ + +static int adxl345_is_ff_en(struct adxl345_state *st, bool *en) +{ + int ret; + unsigned int regval; + + ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val); + if (ret) + return ret; + + *en = FIELD_GET(ADXL345_INT_FREE_FALL, regval) > 0; + + return 0; +} + +static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en) +{ + unsigned int regval, ff_threshold; + const unsigned int freefall_mask = 0x02; + 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); +} + +static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int, + u32 val_fract_us) +{ + unsigned int regval; + int val_ms; + + /* + * max value is 255 * 5000 us = 1.275000 seconds + * + * Note: the scaling is similar to the scaling in the ADXL380 + */ + if (1000000 * val_int + val_fract_us > 1275000) + return -EINVAL; + + val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000); + st->ff_time_ms = val_ms; + + regval = DIV_ROUND_CLOSEST(val_ms, 5); + + /* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */ + return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff)); +} + static int adxl345_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -488,6 +555,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_EV_TYPE_MAG: + ret = adxl345_is_ff_en(st, &int_en); + if (ret) + return ret; + return int_en; default: return -EINVAL; } @@ -511,6 +583,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_EV_TYPE_MAG: + return adxl345_set_ff_en(st, state); default: return -EINVAL; } @@ -525,6 +599,7 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev, { struct adxl345_state *st = iio_priv(indio_dev); unsigned int tap_threshold; + unsigned int ff_threshold; int ret; switch (type) { @@ -558,6 +633,22 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_EV_TYPE_MAG: + switch (info) { + case IIO_EV_INFO_VALUE: + ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, + &ff_threshold); + if (ret) + return ret; + *val = ff_threshold; + return IIO_VAL_INT; + case IIO_EV_INFO_PERIOD: + *val = st->ff_time_ms; + *val2 = 1000; + return IIO_VAL_FRACTIONAL; + default: + return -EINVAL; + } default: return -EINVAL; } @@ -605,6 +696,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev, return -EINVAL; } break; + case IIO_EV_TYPE_MAG: + switch (info) { + case IIO_EV_INFO_VALUE: + ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val); + if (ret) + return ret; + break; + case IIO_EV_INFO_PERIOD: + ret = adxl345_set_ff_time(st, val, val2); + if (ret) + return ret; + break; + default: + return -EINVAL; + } + break; default: return -EINVAL; } @@ -858,6 +965,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat, return ret; } + if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) { + ret = iio_push_event(indio_dev, + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, + IIO_MOD_X_OR_Y_OR_Z, + IIO_EV_TYPE_MAG, + IIO_EV_DIR_FALLING), + ts); + if (ret) + return ret; + } + if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) { samples = adxl345_get_samples(st); if (samples < 0) @@ -966,6 +1084,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, ADXL345_DATA_FORMAT_FULL_RES | ADXL345_DATA_FORMAT_SELF_TEST); unsigned int tap_threshold; + unsigned int ff_threshold; int ret; indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); @@ -985,6 +1104,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, st->tap_window_us = 64; /* 64 [0x40] -> .080 */ st->tap_latent_us = 16; /* 16 [0x10] -> .020 */ + ff_threshold = 8; /* 8 [0x08] */ + st->ff_time_ms = 32; /* 32 [0x20] -> 0.16 */ + indio_dev->name = st->info->name; indio_dev->info = &adxl345_info; indio_dev->modes = INDIO_DIRECT_MODE; @@ -1061,6 +1183,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, if (ret) return ret; + ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, ff_threshold); + if (ret) + return ret; + /* FIFO_STREAM mode is going to be activated later */ ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); if (ret)
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> --- drivers/iio/accel/adxl345_core.c | 126 +++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)