Re: [PATCH v10 3/7] iio: accel: adxl345: add activity event feature

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sun, Jun 22, 2025 at 03:50:06PM +0000, Lothar Rubusch wrote:
> Enable the sensor to detect activity and trigger interrupts accordingly.
> Activity events are determined based on a threshold, which is initialized
> to a sensible default during probe. This default value is adopted from the
> legacy ADXL345 input driver to maintain consistent behavior.
> 
> The combination of activity detection, ODR configuration, and range
> settings lays the groundwork for the activity/inactivity hysteresis
> mechanism, which will be implemented in a subsequent patch. As such,
> portions of this patch prepare switch-case structures to support those
> upcoming changes.

...

> +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> +				    enum adxl345_activity_type type,
> +				    bool cmd_en)
> +{
> +	unsigned int axis_ctrl;
> +	unsigned int threshold;
> +	int ret;
> +
> +	if (cmd_en) {
> +		/* When turning on, check if threshold is valid */

> +		ret = regmap_read(st->regmap,
> +				  adxl345_act_thresh_reg[type],
> +				  &threshold);

Can occupy less LoCs.

> +		if (ret)
> +			return ret;
> +
> +		if (!threshold) /* Just ignore the command if threshold is 0 */
> +			return 0;
> +	}
> +
> +	/* Start modifying configuration registers */
> +	ret = adxl345_set_measure_en(st, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable axis according to the command */
> +	switch (type) {
> +	case ADXL345_ACTIVITY:

> +		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> +				ADXL345_ACT_Z_EN;

I think

		axis_ctrl =
			ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;

is slightly better to read.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_assign_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +				 axis_ctrl, cmd_en);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable the interrupt line, according to the command */
> +	ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> +				 adxl345_act_int_reg[type], cmd_en);
> +	if (ret)
> +		return ret;
> +
> +	return adxl345_set_measure_en(st, true);
> +}

...

> +	case IIO_EV_TYPE_MAG:
> +		return adxl345_read_mag_config(st, dir,
> +					       ADXL345_ACTIVITY);

It looks like you set the editor to wrap at 72 characters, but here the single
line less than 80! Note that the limit is *exactly* 80 character.

...

> +	case IIO_EV_TYPE_MAG:
> +		return adxl345_write_mag_config(st, dir,
> +						ADXL345_ACTIVITY,

Ditto.

...

> +		return adxl345_read_mag_value(st, dir, info,
> +					      ADXL345_ACTIVITY,
> +					      val, val2);

Ditto and so on...

-- 
With Best Regards,
Andy Shevchenko