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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 10 Feb 2025 11:01:10 +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 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@gmail.com>
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, &regval);
> +		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;
> +	}
> +