Re: [PATCH v9 07/11] iio: accel: adxl345: add activity event feature

[Andy Shevchenko]

On Tue, Jun 10, 2025 at 09:59:29PM +0000, Lothar Rubusch wrote:
> Make the sensor detect and issue interrupts at activity. Activity
> events are configured by a threshold. Initialize the activity threshold
> register to a reasonable default value in probe. The value is taken from
> the older ADXL345 input driver, to provide a similar behavior.
> 
> Activity, ODR configuration together with the range setting prepare the
> activity/inactivity hysteresis setup, implemented in a follow up patch.
> Thus parts of this patch prepare switch/case setups for the follow up
> patches.

...

> +/* act/inact */

Useless comment. If you want it to stay, decrypt it and make it useful.

...

> +static int adxl345_is_act_inact_en(struct adxl345_state *st,
> +				   enum adxl345_activity_type type)
> +{
> +	unsigned int regval;
> +	u32 axis_ctrl;
> +	bool en;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case ADXL345_ACTIVITY:
> +		en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
> +			FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
> +			FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);

Something happened to the indentation.
Ditto for several places in the code (upper and lower from this).

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!en)
> +		return en;
> +
> +	/* Check if corresponding interrupts are enabled */
> +	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> +	if (ret)
> +		return ret;
> +
> +	return adxl345_act_int_reg[type] & regval;
> +}

...

> +	if (type == ADXL345_ACTIVITY) {
> +		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> +				ADXL345_ACT_Z_EN;
> +	} else {
> +		axis_ctrl = 0x00;
> +	}

Besides an indentation issue, {} are redundant.

...

> +	en = false;

This line makes no sense. When it will, it should be there, not in this change.

> +	switch (type) {
> +	case ADXL345_ACTIVITY:
> +		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
> +			threshold;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

...

>  	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			switch (dir) {
> +			case IIO_EV_DIR_RISING:
> +				ret = regmap_read(st->regmap,
> +						  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +						  &act_threshold);
> +				if (ret)
> +					return ret;
> +				*val = 62500 * act_threshold;
> +				*val2 = MICRO;
> +				return IIO_VAL_FRACTIONAL;
> +			default:
> +				return -EINVAL;
> +			}
> +		default:
> +			return -EINVAL;
> +		}

As I mentioned before, try to avoid nested switch cases like this. Use helpers
to make this gone to 1 level or so.

>  	case IIO_EV_TYPE_GESTURE:
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:

Ditto for other similar cases.

...

>  static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> -			      enum iio_modifier tap_dir)
> +			      enum iio_modifier tap_dir,
> +			      enum iio_modifier act_dir)

If the order of parameters is not so important, I would squeeze new one to be
before the last argument.

-- 
With Best Regards,
Andy Shevchenko