Re: [PATCH v4 05/14] iio: accel: adxl345: add single tap feature

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 13 Mar 2025 16:50:40 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the single tap feature with a threshold in 62.5mg/LSB points and a
> scaled duration in us. Keep singletap threshold in regmap cache but
> the scaled value of duration in us as member variable.
> 
> Both use IIO channels for individual enable of the x/y/z axis. Initializes
> 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@gmail.com>

Hi Lothar,

A few things in here are from the discussion that was continuing
on v3 so I may have said more replying to that.

Anyhow, for now I'll hold off on applying from this point on as
a few more things to respond to inline.

Jonathan

>  
>  #include "adxl345.h"
> @@ -31,6 +33,33 @@
>  #define ADXL345_INT1			0
>  #define ADXL345_INT2			1
>  
> +#define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
> +
> +enum adxl345_axis {
> +	ADXL345_Z_EN = BIT(0),
> +	ADXL345_Y_EN = BIT(1),
> +	ADXL345_X_EN = BIT(2),
> +	/* Suppress double tap detection if value > tap threshold */
> +	ADXL345_TAP_SUPPRESS = BIT(3),
> +};
As per feedback (after you sent this!) on v3, I'd drop
the last value out of the enum, or just use defines and a u8 for
the one place this is used for local variable storage.


> @@ -198,6 +387,132 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
>  	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;
> +	int ret = -EFAULT;
Not used?

> +
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (dir) {
> +		case IIO_EV_DIR_SINGLETAP:
> +			ret = adxl345_is_tap_en(st, chan->channel2,
> +						ADXL345_SINGLE_TAP, &int_en);
> +			if (ret)
> +				return ret;
> +			return int_en;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int adxl345_write_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 adxl345_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = adxl345_set_measure_en(st, false);
> +	if (ret)
> +		return ret;
> +
So in my brief reply to the v3 discussion I suggested perhaps
factoring out everything from here...
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
> +					   min(val, 0xFF));
> +			break;
> +		case IIO_EV_INFO_TIMEOUT:
> +			ret = adxl345_set_tap_duration(st, val, val2);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
to here, so as to allow simple direct returns.

I think that will make the code more readable given the need to reenable
measurements and that you want to leave it off on error.

> +
> +	if (ret)
> +		return ret; /* measurement stays off */
> +
> +	return adxl345_set_measure_en(st, true);
> +}