linux-kernel - Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aErE0xmlm4qBHg03@smile.fi.intel.com>
Date: Thu, 12 Jun 2025 15:15:15 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
	corbet@....net, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	eraretuya@...il.com
Subject: Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

On Tue, Jun 10, 2025 at 09:59:30PM +0000, Lothar Rubusch wrote:
> Add the inactivity feature of the sensor to the driver. When activity
> and inactivity are enabled, a link bit will be set linking activity and
> inactivity handling. Additionally, the auto-sleep mode will be enabled.
> Due to the link bit the sensor is going to auto-sleep when inactivity
> was detected.
> 
> Inactivity detection needs a threshold to be configured and a period of
> time in seconds. After, it will transition to inactivity state, if
> measurements stay below inactivity threshold.
> 
> When a ODR is configured the period for inactivity is adjusted with a
> corresponding reasonable default value, in order to have higher
> frequencies, lower inactivity times, and lower sample frequency but
> give more time until inactivity. Both with reasonable upper and lower
> boundaries, since many of the sensor's features (e.g. auto-sleep) will
> need to operate between 12.5 Hz and 400 Hz. This is a default setting
> when actively changing sample frequency, explicitly setting the time
> until inactivity will overwrite the default.
> 
> Similarly, setting the g-range will provide a default value for the
> activity and inactivity thresholds. Both are implicit defaults, but
> equally can be overwritten to be explicitly configured.

...

> +static const struct iio_event_spec adxl345_fake_chan_events[] = {
> +	{
> +		/* inactivity */
> +		.type = IIO_EV_TYPE_MAG,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> +			BIT(IIO_EV_INFO_PERIOD),

Slightly better

		.mask_shared_by_type =
			BIT(IIO_EV_INFO_VALUE) |
			BIT(IIO_EV_INFO_PERIOD),

> +	},
> +};

And the same for other similar cases.

...

> +/**
> + * adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
> + * @st: The sensor state instance.
> + * @val_s: A desired time value, between 0 and 255.
> + *
> + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> + * is configured by a user, then a default inactivity time will be computed.
> + *
> + * In such case, it should take power consumption into consideration. Thus it
> + * shall be shorter for higher frequencies and longer for lower frequencies.
> + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> + * 10 Hz shall result in 255 s to detect inactivity.
> + *
> + * The approach simply subtracts the pre-decimal figure of the configured
> + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> + * ignored in this estimation. The recommended ODRs for various features
> + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> + * 400 Hz, thus higher or lower frequencies will result in the boundary
> + * defaults or need to be explicitly specified via val_s.
> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> +{
> +	unsigned int max_boundary = 255;
> +	unsigned int min_boundary = 10;
> +	unsigned int val = min(val_s, max_boundary);
> +	enum adxl345_odr odr;
> +	unsigned int regval;
> +	int ret;
> +
> +	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 = (adxl345_odr_tbl[odr][0] > max_boundary)
> +			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];

clamp() ?

> +	}
> +
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +}

...

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

Now this can be as simple as

	axis_ctrl = ADXL345_ACT_X_EN;
	if (type == ADXL345_ACTIVITY)
		axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
	else
		axis_ctrl |= ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;

Yeah, I don't know how to make the diff better (it gets worse), but the end
result is better.

One way, which I don't like much is to previously have this conditional written as:

	axis_ctrl = ADXL345_ACT_X_EN;
	if (type == ADXL345_ACTIVITY)
		axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
	else
		axis_ctrl = 0;

...

> +	ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> +				 (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),

Unneeded parentheses.

> +				 en);
>  	if (ret)
>  		return ret;

...

>  static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
>  {
> -	return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
>  				 ADXL345_BW_RATE_MSK,
>  				 FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
> +	if (ret)
> +		return ret;
> +
> +	/* update inactivity time by ODR */
> +	return adxl345_set_inact_time(st, 0);

Okay, in this case the initial form of

	int ret;

	ret = ...
	if (ret)
		return ret;

	return 0;


will be better with the respectful comment (as Jonathan suggested) in that
change that this is not optimal as standalone change, but it will help reduce
churn in the next change(s).

>  }

...

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

Same here.

> +	unsigned int act_threshold, inact_threshold;
> +	unsigned int range_old;
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
> +	if (ret)
> +		return ret;
> +	range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
> +
> +	ret = regmap_read(st->regmap,
> +			  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +			  &act_threshold);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap,
> +			  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +			  &inact_threshold);
> +	if (ret)
> +		return 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;
> +
> +	act_threshold = act_threshold * adxl345_range_factor_tbl[range_old]
> +		/ adxl345_range_factor_tbl[range];
> +	act_threshold = min(U8_MAX, max(1, act_threshold));
> +
> +	inact_threshold = inact_threshold * adxl345_range_factor_tbl[range_old]
> +		/ adxl345_range_factor_tbl[range];
> +	inact_threshold = min(U8_MAX, max(1, inact_threshold));
> +
> +	ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +			   act_threshold);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +			   inact_threshold);
>  }

-- 
With Best Regards,
Andy Shevchenko