Re: [PATCH v10 4/7] iio: accel: adxl345: add inactivity feature

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

On Sun, Jun 22, 2025 at 03:50:07PM +0000, Lothar Rubusch wrote:
> Add support for the sensor’s inactivity feature in the driver. When both
> activity and inactivity detection are enabled, the sensor sets a link bit
> that ties the two functions together. This also enables auto-sleep mode,
> allowing the sensor to automatically enter sleep state upon detecting
> inactivity.
> 
> Inactivity detection relies on a configurable threshold and a specified
> time period. If sensor measurements remain below the threshold for the
> defined duration, the sensor transitions to the inactivity state.
> 
> When an Output Data Rate (ODR) is set, the inactivity time period is
> automatically adjusted to a sensible default. Higher ODRs result in shorter
> inactivity timeouts, while lower ODRs allow longer durations-within
> reasonable upper and lower bounds. This is important because features like
> auto-sleep operate effectively only between 12.5 Hz and 400 Hz. These
> defaults are applied when the sample rate is modified, but users can
> override them by explicitly setting a custom inactivity timeout.
> 
> Similarly, configuring the g-range provides default threshold values for
> both activity and inactivity detection. These are implicit defaults meant
> to simplify configuration, but they can also be manually overridden as
> needed.

...

> +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> +{
> +	int max_boundary = U8_MAX;
> +	int min_boundary = 10;
> +	unsigned int val = min(val_s, U8_MAX);

Wondering if it's possible to refer here to max_boundary?
In any case, split this assignment since it will be easier
to maintain.

> +	enum adxl345_odr odr;
> +	unsigned int regval;
> +	int ret;

	val = min(val_s, max_boundary);

> +	if (val == 0) {
> +		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> +		if (ret)
> +			return ret;
> +
> +		odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> +		val = clamp(max_boundary - adxl345_odr_tbl[odr][0],
> +			    min_boundary, max_boundary);
> +	}
> +
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +}

...

> +	case ADXL345_INACTIVITY:
> +		en = FIELD_GET(ADXL345_INACT_X_EN, axis_ctrl) |
> +			FIELD_GET(ADXL345_INACT_Y_EN, axis_ctrl) |
> +			FIELD_GET(ADXL345_INACT_Z_EN, axis_ctrl);

As I pointed out earlier. the indentation is supposed to be on the same colomn
for 'F' letters.

> +		if (!en)
> +			return false;
> +		break;

...

> +			ret = regmap_read(st->regmap,
> +					  ADXL345_REG_TIME_INACT,
> +					  &period);

There is plenty of room on the previous lines. Depending on the next
changes (which I believe unlikely touch this) it may be packed to two
lines with a logical split, like

			ret = regmap_read(st->regmap,
					  ADXL345_REG_TIME_INACT, &period);

It again seems the byproduct of the too strict settings of the wrap limit in
your editor.

...

> +	case ADXL345_INACTIVITY:
> +		axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> +				ADXL345_INACT_Z_EN;

Consider
		axis_ctrl =
			ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;

(yes, I see that it's longer than 80, but it might worth doing it for the sake of
 consistency with the previous suggestion).


...

>  static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
>  {
> -	return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,

> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
>  				 ADXL345_DATA_FORMAT_RANGE,
>  				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> +	if (ret)
> +		return ret;

If it's a code from the previous patch, it might make sense to introduce ret
there.

>  }

...

> +	case IIO_EV_INFO_PERIOD:
> +		ret = regmap_read(st->regmap,
> +				  ADXL345_REG_TIME_INACT,
> +				  &period);

Too short lines.

-- 
With Best Regards,
Andy Shevchenko