| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251018181632.76851d4e@jic23-huawei>
Date: Sat, 18 Oct 2025 18:16:32 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, Jagath Jog J
<jagathjog1996@...il.com>
Subject: Re: [PATCH 3/6] iio: accel: bma220: add tap detection
On Tue, 14 Oct 2025 19:42:59 +0300
Petre Rodan <petre.rodan@...dimension.ro> wrote:
> Add support for tap events.
>
> Signed-off-by: Petre Rodan <petre.rodan@...dimension.ro>
> ---
> Please advise on where should I should be using the mutex guard()
> (single regmap reads vs single writes vs more complex read/write combo)
It usually depends not so much on what is being accessed directly under the lock
but more whether we need to prevent interruption of sequences that might
be running from other paths. That or protecting a buffer that might be
hit from another path.
The regmap calls themselves are effectively atomic due to internal locking
so we shouldn't need to add to that if there are no issues with more complex
sequences that need protecting.
In general tap detection is one of my least favourite things to enable because
every device seems to have a different set of control parameters.
I've +CC Jagath who looked at this a lot a while back and defined much of
the existing ABI.
> ---
> drivers/iio/accel/bma220_core.c | 296 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 295 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> index 871342d21456..c4bebf3e5548 100644
> --- a/drivers/iio/accel/bma220_core.c
> +++ b/drivers/iio/accel/bma220_core.c
> @@ -21,6 +21,7 @@
> @@ -231,6 +254,13 @@ static const struct iio_trigger_ops bma220_trigger_ops = {
> .validate_device = &iio_trigger_validate_own_device,
> };
>
> +static int bma220_reset_int(struct bma220_data *data)
> +{
> + return regmap_update_bits(data->regmap, BMA220_REG_IE1,
> + BMA220_INT_RST_MSK,
> + FIELD_PREP(BMA220_INT_RST_MSK, 1));
regmap_set_bits()
> +}
> +static int bma220_set_tap_en(struct bma220_data *data,
> + enum iio_modifier axis,
> + int type,
> + bool en)
> +{
> + unsigned int flags = BMA220_TAP_TYPE_DOUBLE;
> + int ret;
> +
> + switch (axis) {
> + case IIO_MOD_X:
> + ret = regmap_update_bits(data->regmap, BMA220_REG_IE0,
> + BMA220_INT_EN_TAP_X_MSK,
> + FIELD_PREP(BMA220_INT_EN_TAP_X_MSK, en));
regmap_assign_bits() comes in handy when you are setting just one bit.
> + break;
> + case IIO_MOD_Y:
> + ret = regmap_update_bits(data->regmap, BMA220_REG_IE0,
> + BMA220_INT_EN_TAP_Y_MSK,
> + FIELD_PREP(BMA220_INT_EN_TAP_Y_MSK, en));
> + break;
> + case IIO_MOD_Z:
> + ret = regmap_update_bits(data->regmap, BMA220_REG_IE0,
> + BMA220_INT_EN_TAP_Z_MSK,
> + FIELD_PREP(BMA220_INT_EN_TAP_Z_MSK, en));
> + break;
> + default:
> + return -EINVAL;
> + }
> + if (ret)
> + return ret;
> +
> + /* tip_en must be 1 to select singletap detection */
> + if (type == IIO_EV_DIR_SINGLETAP)
> + flags = BMA220_TAP_TYPE_SINGLE;
> +
> + ret = regmap_update_bits(data->regmap, BMA220_REG_CONF5,
> + BMA220_TIP_EN_MSK,
> + FIELD_PREP(BMA220_TIP_EN_MSK, flags));
> + if (ret)
> + return ret;
> +
> + data->tap_type = flags;
> +
> + return 0;
> +}
> +
> +static int bma220_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct bma220_data *data = iio_priv(indio_dev);
> + unsigned int reg_val;
> + int ret;
> +
> + guard(mutex)(&data->lock);
> +
> + switch (type) {
> + case IIO_EV_TYPE_GESTURE:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + ret = regmap_read(data->regmap, BMA220_REG_CONF3, ®_val);
> + if (ret)
> + return ret;
> + *val = FIELD_GET(BMA220_TT_TH_MSK, reg_val);
> + return IIO_VAL_INT;
> + case IIO_EV_INFO_PERIOD:
Period for a tap detector, particularly one that can do double tap is unusual.
Period is normally a case of how long a condition must be true before it causes
an interrupt.
I haven't figured out if this maps to one of the rather rich set of double tap
attributes in the ABI. See Documentation/ABI/testing/sysfs-bus-iio *doubletap*
Related stuff are
in_accel_gesture_doubletap_tap2_min_delay - I think that is tt_quiet in this case.
in_accel_gesture_doubletap_reset_timeout - I think this is at least approximately
tt_quiet + tt_dur. It gives the time at which we give up on this being a potential
double tap. It's not quite right though as the actual reset is a bit after that.
+CC Jagath who defined most of the tap ABI and might be able to remember how this
all works better than me.
We may have a gap here that needs yet more ABI.
> + ret = regmap_read(data->regmap, BMA220_REG_CONF3, ®_val);
> + if (ret)
> + return ret;
> + *val = FIELD_GET(BMA220_TT_DUR_MSK, reg_val);
This needs to be in second if you are using duration. Is the register really in seconds?
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
>
> @@ -506,13 +777,36 @@ static irqreturn_t bma220_irq_handler(int irq, void *private)
> struct bma220_data *data = iio_priv(indio_dev);
> int ret;
> unsigned int bma220_reg_if1;
> + s64 timestamp = iio_get_time_ns(indio_dev);
> +
> + guard(mutex)(&data->lock);
>
> ret = regmap_read(data->regmap, BMA220_REG_IF1, &bma220_reg_if1);
> if (ret)
> return IRQ_NONE;
>
> - if (FIELD_GET(BMA220_IF_DRDY, bma220_reg_if1))
> + if (FIELD_GET(BMA220_IF_DRDY, bma220_reg_if1)) {
Is it an either / or case? I.e. we can only have buffered reads with
the data ready interrupt or events? That does happen in some devices
but is fairly unusual.
> iio_trigger_poll_nested(data->trig);
> + return IRQ_HANDLED;
> + }
> +
> + if (FIELD_GET(BMA220_IF_TT, bma220_reg_if1)) {
> +
> + if (data->tap_type == BMA220_TAP_TYPE_SINGLE)
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> + IIO_MOD_X_OR_Y_OR_Z,
> + IIO_EV_TYPE_GESTURE,
> + IIO_EV_DIR_SINGLETAP),
> + timestamp);
> + else
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> + IIO_MOD_X_OR_Y_OR_Z,
> + IIO_EV_TYPE_GESTURE,
> + IIO_EV_DIR_DOUBLETAP),
> + timestamp);
> + }
>
> return IRQ_HANDLED;
> }
>
Powered by blists - more mailing lists