On Mon, 10 Feb 2025 11:01:10 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> Add the single tap feature with a threshold in 62.5mg/LSB points and a
> scaled duration in us. Keep singletap threshold and duration using means
> of IIO ABI. Extend the channels for single enable x/y/z axis of the
> feature but also initialize threshold and duration with reasonable
> content. When an interrupt is caught it will be pushed to the according
> IIO channel.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
Hi Lothar,
Various comments inline,
Thanks,
Jonathan
> ---
> drivers/iio/accel/adxl345_core.c | 369 ++++++++++++++++++++++++++++++-
> 1 file changed, 367 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 910ad21279ed..304147a4ca60 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -8,6 +8,7 @@
> */
> +};
> +
> +static const unsigned int adxl345_tap_int_reg[2] = {
Why force 2? Just use []
> + [ADXL345_SINGLE_TAP] = ADXL345_INT_SINGLE_TAP,
> +};
> +
> +enum adxl345_tap_time_type {
> + ADXL345_TAP_TIME_DUR,
> +};
> +
> +static const unsigned int adxl345_tap_time_reg[3] = {
Why force 3?
> + [ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
> +};
> +static int _adxl345_set_tap_int(struct adxl345_state *st,
> + enum adxl345_tap_type type, bool state)
> +{
> + bool axis_valid;
> + bool singletap_args_valid = false;
> + bool en = false;
> +
> + axis_valid = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
> +
> + /*
> + * Note: A value of 0 for threshold and/or dur may result in undesirable
> + * behavior if single tap/double tap interrupts are enabled.
> + */
> + singletap_args_valid = st->tap_threshold > 0 && st->tap_duration_us > 0;
> +
> + if (type == ADXL345_SINGLE_TAP)
> + en = axis_valid && singletap_args_valid;
> +
> + if (state && en)
> + __set_bit(ilog2(adxl345_tap_int_reg[type]),
> + (unsigned long *)&st->int_map);
That's not a good idea (I think I probably mislead you here). Casting
across integer pointer types isn't a good way to go.
> + else
> + __clear_bit(ilog2(adxl345_tap_int_reg[type]),
> + (unsigned long *)&st->int_map);
> +
> + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
As in previous, if we rely on the regmap cache then can just use
the regmap_set_bit() regmap_clear_bit() type helpers here.
> +}
>
> +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 (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:
> + axis_en = ADXL345_TAP_SUPPRESS;
Add a break here. Not strictly necessary but hardens against odd code
changes in future.
> + }
> +
> + switch (dir) {
> + case IIO_EV_DIR_SINGLETAP:
> + ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
> + if (ret)
> + return ret;
> + return int_en && axis_en;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adxl345_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)
My general preference in IIO is to keep to older 80 char line wrap limit unless
longer lines help readability. Even then try to keep them only a little longer.
So I'd prefer these parameters wrapped a bit more!
> +{
> -static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
> +static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat,
> + enum iio_modifier *act_tap_dir)
> {
> + unsigned int regval;
> + bool check_tap_stat;
> + int ret;
> +
> + *act_tap_dir = IIO_NO_MOD;
> + 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 */
> + ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> + if (ret)
> + return ret;
> +
This double bit pattern is odd enough that I think it needs a comment or
separate defines for the ACT_X source and TAP_X source
> + if ((FIELD_GET(ADXL345_Z_EN, regval >> 4)
> + | FIELD_GET(ADXL345_Z_EN, regval)) > 0)
> + *act_tap_dir = IIO_MOD_Z;
> + else if ((FIELD_GET(ADXL345_Y_EN, regval >> 4)
> + | FIELD_GET(ADXL345_Y_EN, regval)) > 0)
> + *act_tap_dir = IIO_MOD_Y;
> + else if ((FIELD_GET(ADXL345_X_EN, regval >> 4)
> + | FIELD_GET(ADXL345_X_EN, regval)) > 0)
> + *act_tap_dir = IIO_MOD_X;
> + }
> +
