linux-kernel - Re: [PATCH v10 3/7] iio: accel: adxl345: add activity event 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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFkfjAekGJTU5o71@smile.fi.intel.com>
Date: Mon, 23 Jun 2025 12:34:04 +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 v10 3/7] iio: accel: adxl345: add activity event feature

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