Re: [PATCH v3 12/15] iio: accel: adxl345: add activity event feature

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 20 Feb 2025 10:42:31 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Make the sensor detect and issue interrupts at activity. Activity
> events are configured by a threshold stored in regmap cache.
> 
> Activity, together with ODR and range setting are preparing a setup
> together with inactivity coming in a follow up patch.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Some questions / comments inline.

This review is has been at random moments whilst travelling (hence
over several days!) so it may be less than thorough or consistent!
I should be back to normal sometime next week.

> @@ -258,6 +284,73 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
>  	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
>  }
>  
> +/* act/inact */
> +
> +static int adxl345_write_act_axis(struct adxl345_state *st,
> +				  enum adxl345_activity_type type, bool en)
> +{
> +	int ret;
> +
> +	/*
> +	 * The ADXL345 allows for individually enabling/disabling axis for
> +	 * activity and inactivity detection, respectively. Here both axis are
> +	 * kept in sync, i.e. an axis will be generally enabled or disabled for
> +	 * both equally, activity and inactivity detection.

Why?  Can definitely see people only being interested in one case
and not the other.  What advantage is there in always having both
or neither over separate controls?

> +	 */
> +	if (type == ADXL345_ACTIVITY) {
> +		st->act_axis_ctrl = en
> +			? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> +			: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> +
> +		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +					 adxl345_act_axis_msk[type],
> +					 st->act_axis_ctrl);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}


> +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> +				    enum adxl345_activity_type type, bool cmd_en)
> +{
> +	bool axis_en, en = false;
I'm not keen on mix of set and unset in a declaration line.  Better to 
use two lines here as it can be hard to spot when things are or aren't
initialized when that is not the intent.

	bool en = false;
	bool axis_en;

> +	unsigned int threshold;
> +	int ret;
> +
> +	ret = adxl345_write_act_axis(st, type, cmd_en);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> +	if (ret)
> +		return ret;
> +
> +	if (type == ADXL345_ACTIVITY) {
> +		axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> +		en = axis_en && threshold > 0;
> +	}
> +
> +	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> +				  adxl345_act_int_reg[type],
> +				  en ? adxl345_act_int_reg[type] : 0);
> +}
> +

> @@ -842,6 +972,23 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
>  		return ret;
>  
>  	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			switch (dir) {
> +			case IIO_EV_DIR_RISING:
> +				ret = regmap_write(st->regmap,
> +						   adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +						   val);
> +				break;
This collection of breaks and nested functions suggests maybe we can either
return directly (I've lost track of what happens after this) or that
we should factor out some of this code to allow direct returns in the
function we put that code in.  Chasing the breaks is not great if it
doesn't lead to anything interesting.
> +			default:
> +				ret = -EINVAL;
> +			}
> +			break;
> +		default:
> +			ret = -EINVAL;
> +		}
> +		break;
>  	case IIO_EV_TYPE_GESTURE:
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:
> @@ -1124,6 +1271,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
>  			return ret;
>  	}
>  
> +	if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
> +		ret = iio_push_event(indio_dev,
> +				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							act_tap_dir,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
>  		ret = iio_push_event(indio_dev,
>  				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
>  		if (ret)
>  			return ret;
> +		/* tap direction */

Belongs in earlier patch?

>  		if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
>  			act_tap_dir = IIO_MOD_Z;
>  		else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  			act_tap_dir = IIO_MOD_X;
>  	}
>  
> +	if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> +		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> +		if (ret)
> +			return ret;
> +		/* activity direction */
> +		if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)

I'm not following the shifts here.   That looks like we don't have
defines that we should but instead use the ones for the lower fields.

> +			act_tap_dir = IIO_MOD_Z;
> +		else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> +			act_tap_dir = IIO_MOD_Y;
> +		else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> +			act_tap_dir = IIO_MOD_X;
> +	}