On Tue, 18 Feb 2025 23:29:46 +0100
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> Dear Jonathan, find my answer down below.
>
> On Sun, Feb 16, 2025 at 6:28 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Mon, 10 Feb 2025 11:01:12 +0000
> > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >
> > > Add the suppress bit feature to the double tap feature.
> > >
> > > Any tap event is defined by a rising signal edge above threshold, i.e.
> > > duration time starts counting; and the falling edge under threshold
> > > within duration time, i.e. then the tap event is issued. This means
> > > duration is used individually for each tap event.
> > >
> > > For double tap detection after a single tap, a latency time needs to be
> > > specified. Usually tap events, i.e. spikes above and returning below
> > > threshold will be ignored within latency. After latency, the window
> > > time starts counting for a second tap detection which has to happen
> > > within a duration time.
> > >
> > > If the suppress bit is not set, spikes within latency time are ignored.
> > > Setting the suppress bit will invalidate the double tap function. The
> > > sensor will thus be able to save the window time for double tap
> > > detection, and follow a more strict definition of what signal qualifies
> > > for a double tap.
> >
> > Silly question. Is there a reason this function would ever be
> > turned off? Seems like a sensible heuristic that would not stop
> > genuine double taps being detected. Maybe we just always leave it on?
> >
> > Sometimes the best ABI is the one that doesn't exist as userspace
> > can't use it wrong.
> >
> > Jonathan
> >
>
> hehehe.. you already mentioned this point, I guess. At least I tried
> to put my understanding of it into the lengthy comment of the patch.
> Well, patches with lengthy comments.... this seems to go into the same
> direction as the wisdom of better limiting userspace interfaces in
> general ;)
>
> TBH you have probably seen far more sensors than me, as I'm doing this
> just as hobbyist to learn and for fun. I only can provide my
> understanding of the particular datasheet.
> I think, to set or not to set this bit changes little. It influences a
> bit how restrictive the latency period is handled at detection.
> Doubletaps are detected with or without having the "suppress" bit set.
> If set, AFAIK it could be harder to detect doubletaps. So to speak,
> you could reduce "noise" in double tapping (?), or if one receives too
> many double taps...(?) perhaps, ..eh.. legal reasons?! Personally,
> I'd liked to play with this sensor a bit, and I found it then useful
> to have some kind of knob to change a bit and see what happens without
> really messing things up.
> As I'm not too familiar with the accelerometer scene and such kind of
> "power user settings". I'm unsure if there are typical usecases here.
> I would agree that usually one would leave that in one setting,
> turned on or off (unless he/she enters in obsession with double taps).
>
> Perhaps I'll change this patch so that it's always set or not set (to
> bring it initially into a defined state), but no sysfs is around.
> Let's see. If you think I'd better just drop it entirly, let me know
> then.
I think default to always set. We can revisit the ABI question later
if turns out to have be something people change in practice!
Jonathan
>
> Best,
> L
>
> >
> > >
> > > This brings in a new ABI functionality.
> > > ---
> > > Q: Perhaps there is already some IIO ABI for it? If not, please let me
> > > know which ABI documentation to extend. There will be a documentation
> > > patch also later in this series.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > > ---
> > > drivers/iio/accel/adxl345_core.c | 82 ++++++++++++++++++++++++++++++++
> > > 1 file changed, 82 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index cf35a8f9f432..b6966fee3e3d 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -34,6 +34,7 @@
> > > #define ADXL345_INT2 1
> > >
> > > #define ADXL345_REG_TAP_AXIS_MSK GENMASK(2, 0)
> > > +#define ADXL345_REG_TAP_SUPPRESS_MSK BIT(3)
> > >
> > > enum adxl345_axis {
> > > ADXL345_Z_EN = BIT(0),
> > > @@ -81,6 +82,7 @@ struct adxl345_state {
> > > u32 tap_duration_us;
> > > u32 tap_latent_us;
> > > u32 tap_window_us;
> > > + bool tap_suppressed;
> > >
> > > __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > > };
> > > @@ -243,6 +245,31 @@ static int adxl345_set_doubletap_en(struct adxl345_state *st, bool en)
> > > return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
> > > }
> > >
> > > +static int adxl345_is_suppressed_en(struct adxl345_state *st, bool *en)
> > > +{
> > > + *en = st->tap_suppressed;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int adxl345_set_suppressed_en(struct adxl345_state *st, bool en)
> > > +{
> > > + unsigned long regval = 0;
> > > + int ret;
> > > +
> > > + en ? __set_bit(ilog2(ADXL345_TAP_SUPPRESS), ®val)
> > > + : __clear_bit(ilog2(ADXL345_TAP_SUPPRESS), ®val);
> > > +
> > > + ret = regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
> > > + ADXL345_REG_TAP_SUPPRESS_MSK, regval);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + st->tap_suppressed = en;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int adxl345_set_tap_threshold(struct adxl345_state *st, u8 val)
> > > {
> > > int ret;
> > > @@ -616,6 +643,60 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
> > > }
> > > }
> > >
> > > +static ssize_t in_accel_gesture_doubletap_suppressed_en_show(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > + struct adxl345_state *st = iio_priv(indio_dev);
> > > + bool en;
> > > + int val, ret;
> > > +
> > > + ret = adxl345_is_suppressed_en(st, &en);
> > > + if (ret)
> > > + return ret;
> > > + val = en ? 1 : 0;
> > > +
> > > + return iio_format_value(buf, IIO_VAL_INT, 1, &val);
> > > +}
> > > +
> > > +static ssize_t in_accel_gesture_doubletap_suppressed_en_store(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t len)
> > > +{
> > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > + struct adxl345_state *st = iio_priv(indio_dev);
> > > + int val, ret;
> > > +
> > > + ret = kstrtoint(buf, 0, &val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = adxl345_set_measure_en(st, false);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = adxl345_set_suppressed_en(st, val > 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = adxl345_set_measure_en(st, true);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return len;
> > > +}
> > > +static IIO_DEVICE_ATTR_RW(in_accel_gesture_doubletap_suppressed_en, 0);
> > > +
> > > +static struct attribute *adxl345_event_attrs[] = {
> > > + &iio_dev_attr_in_accel_gesture_doubletap_suppressed_en.dev_attr.attr,
> > > + NULL
> > > +};
> > > +
> > > +static const struct attribute_group adxl345_event_attrs_group = {
> > > + .attrs = adxl345_event_attrs,
> > > +};
> > > +
> > > static void adxl345_powerdown(void *ptr)
> > > {
> > > struct adxl345_state *st = ptr;
> > > @@ -899,6 +980,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > >
> > > static const struct iio_info adxl345_info = {
> > > .attrs = &adxl345_attrs_group,
> > > + .event_attrs = &adxl345_event_attrs_group,
> > > .read_raw = adxl345_read_raw,
> > > .write_raw = adxl345_write_raw,
> > > .write_raw_get_fmt = adxl345_write_raw_get_fmt,
> >
>
