| 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: <20250201170225.35d06934@jic23-huawei>
Date: Sat, 1 Feb 2025 17:02:25 +0000
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 v1 06/12] iio: accel: adxl345: add single tap feature
On Tue, 28 Jan 2025 12:00:54 +0000
Lothar Rubusch <l.rubusch@...il.com> wrote:
> Add the single tap feature with a threshold in 62.5mg/LSB points and a
> scaled duration in us. Keep singletap threshold by means of IIO but add
> sysfs entry for the duration. Using a sysfs entry allow for a clearer
> naming of the handle to improve usage. Extend the channels for single
> enable x/y/z axis of the feature but also check if threshold (a.k.a
> "value") and duration have reasonable content. When an interrupt is
> caught it will be pushed to the according IIO channel.
>
> The function call structure is in preparation to be extended for an
> upcoming doubletap feature in the follow up patches.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
The duration ABI isn't standard, so it should come with ABI docs and
some explanation of why we can't use existing ABI. The postfix of _us
whilst it seems sensible is not inline with existing IIO ABI. Times
are always in seconds.
> +static inline void adxl345_intmap_switch_bit(struct adxl345_state *st,
> + bool condition, u8 bit)
> +{
> + st->int_map = condition ? st->int_map | bit : st->int_map & ~bit;
I'm not convinced the wrapper is that useful.
Maybe can use __clear_bit() and __set_bit()
> +}
> +
> +static inline int adxl345_read_interrupts(struct adxl345_state *st,
> + unsigned int *interrupts)
> +{
> + return regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, interrupts);
I don't see an advantage in this wrapper. Why not just call regmap
directly. It's pretty self documenting!
> +}
> +
> static inline int adxl345_write_interrupts(struct adxl345_state *st)
> {
> return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
> }
> static int adxl345_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> @@ -275,6 +413,141 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> ADXL345_BW_RATE,
> clamp_val(ilog2(n), 0,
> ADXL345_BW_RATE));
> + default:
> + return -EINVAL;
> + }
> +
Doubt we can get here.
> + return -EINVAL;
> +}
> +
> +static int adxl345_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct adxl345_state *st = iio_priv(indio_dev);
> + bool int_en;
> + bool axis_en;
> + int ret = -EFAULT;
> +
> + switch (type) {
> + case IIO_EV_TYPE_GESTURE:
> + switch (dir) {
> + case IIO_EV_DIR_SINGLETAP:
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
> + break;
> + case IIO_MOD_Y:
> + axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
> + break;
> + case IIO_MOD_Z:
> + axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = adxl345_is_tap_en(st, &int_en);
> + if (ret)
> + return ret;
> + return int_en && axis_en;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +
> + return ret;
Can't get here I think.
> +}
> +#define ADXL345_generate_iio_dev_attr_FRACTIONAL(A, B, C, D, E) \
> + static ssize_t in_accel_##A##_##C##_##E##_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); \
> + int vals[2]; \
> + \
> + vals[0] = st->B##_##C##_##E; \
> + vals[1] = D; \
> + \
> + return iio_format_value(buf, IIO_VAL_FRACTIONAL, 2, vals); \
> + } \
> + \
> + static ssize_t in_accel_##A##_##C##_##E##_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_int, val_fract_us, ret; \
> + \
> + ret = iio_str_to_fixpoint(buf, 100000, &val_int, &val_fract_us); \
> + if (ret) \
> + return ret; \
> + \
> + ret = adxl345_set_measure_en(st, false); \
> + if (ret) \
> + return ret; \
> + \
> + adxl345_set_##B##_##C(st, val_int, val_fract_us); \
> + \
> + ret = adxl345_set_measure_en(st, true); \
> + if (ret) \
> + return ret; \
> + \
> + return len; \
> + } \
> + static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
> +
> +ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
> +
> +static struct attribute *adxl345_event_attrs[] = {
> + &iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
New ABI so I should be seeing ABI docs.
Also durations (anything time related) in IIO ABI is in seconds.
Does this not map to existing documented ABI of
in_accel_gesture_tap_wait_dur?
> + NULL
> +};
> +
> +static const struct attribute_group adxl345_event_attrs_group = {
> + .attrs = adxl345_event_attrs,
> +};
> +
> static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
> );
> @@ -477,6 +802,17 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>
> static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
> {
> + unsigned int regval;
> + bool check_tap_stat;
> +
> + check_tap_stat = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
> +
> + if (check_tap_stat) {
> + /* ACT_TAP_STATUS should be read before clearing the interrupt */
> + if (regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val))
> + return -EINVAL;
Don't eat the return value. It might be useful.
ret = regmap_read()
if (ret)
return ret;
> + }
> +
> return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);
> }
>
> @@ -499,6 +835,25 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
> return 0;
> }
> /**
> * adxl345_irq_handler() - Handle irqs of the ADXL345.
> * @irq: The irq being handled.
> @@ -516,6 +871,9 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> if (adxl345_get_status(st, &int_stat))
> return IRQ_NONE;
>
> + if (adxl345_push_event(indio_dev, int_stat) == 0)
> + return IRQ_HANDLED;
Can have multiple interrupts at once. Say an event that happens to
occur very close to watermark. So I'd normally expect to
carry on looking for interrupts.
> +
> if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
> samples = adxl345_get_samples(st);
> if (samples < 0)
> @@ -538,9 +896,14 @@ 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,
> + .read_event_config = adxl345_read_event_config,
> + .write_event_config = adxl345_write_event_config,
> + .read_event_value = adxl345_read_event_value,
> + .write_event_value = adxl345_write_event_value,
> .debugfs_reg_access = &adxl345_reg_access,
> .hwfifo_set_watermark = adxl345_set_watermark,
> };
> @@ -588,6 +951,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>
> st->int_map = 0x00; /* reset interrupts */
>
> + /* Init with reasonable values */
> + st->tap_threshold = 35; /* 35 [0x23] */
> + st->tap_duration_us = 3; /* 3 [0x03] -> .001875 */
> +
> indio_dev->name = st->info->name;
> indio_dev->info = &adxl345_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
Powered by blists - more mailing lists