linux-kernel - Re: [PATCH v3 1/2] iio: imu: inv

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] [day] [month] [year] [list]
Message-ID: <aAPDovuee7hoY1PS@smile.fi.intel.com>
Date: Sat, 19 Apr 2025 18:39:14 +0300
From: Andy Shevchenko <andy@...nel.org>
To: jean-baptiste.maneyrol@....com
Cc: Jonathan Cameron <jic23@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] iio: imu: inv_icm42600: add WoM support

On Fri, Apr 18, 2025 at 06:19:02PM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
> 
> Add WoM as accel roc rising x|y|z event.

...

> +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,
> +							  int accel_hz, int accel_uhz)
> +{
> +	/* 1000/256mg per LSB converted in µm/s² */
> +	const unsigned int convert = (1000U * 9807U) / 256U;

Wondering if KILO (or MILLI?) is a good suit here...

> +	uint64_t value;
> +	uint64_t freq_uhz;
> +
> +	/* return 0 only if roc is 0 */
> +	if (roc == 0)
> +		return 0;
> +
> +	freq_uhz = (uint64_t)accel_hz * MICRO + (uint64_t)accel_uhz;
> +	value = div64_u64(roc * MICRO, freq_uhz * (uint64_t)convert);
> +
> +	/* limit value to 8 bits and prevent 0 */
> +	return clamp(value, 1, 255);
> +}
> +
> +static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
> +						      int accel_hz, int accel_uhz)
> +{
> +	/* 1000/256mg per LSB converted in µm/s² */
> +	const unsigned int convert = (1000U * 9807U) / 256U;

Ditto.

> +	uint64_t value;
> +	uint64_t freq_uhz;
> +
> +	value = threshold * convert;
> +	freq_uhz = (uint64_t)accel_hz * MICRO + (uint64_t)accel_uhz;
> +
> +	/* compute the differential by multiplying by the frequency */
> +	return div_u64(value * freq_uhz, MICRO);
> +}

...

> +static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
> +	struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> +	unsigned int sleep_ms = 0;
> +	int ret;
> +
> +	scoped_guard(mutex, &st->lock) {

> +		st->apex.wom.enable = false;
> +		st->apex.on--;

Hmm... Even if the below fails we consider it successful? Why?

> +		ret = inv_icm42600_disable_wom(st);
> +		if (ret)
> +			break;
> +		/* turn off accel sensor if not used */
> +		if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
> +			conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
> +			ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> +			if (ret)
> +				break;
> +		}
> +	}
> +
> +	if (sleep_ms)
> +		msleep(sleep_ms);
> +	pm_runtime_mark_last_busy(pdev);
> +	pm_runtime_put_autosuspend(pdev);
> +
> +	return ret;
> +}

...

> +static int inv_icm42600_accel_read_event_config(struct iio_dev *indio_dev,
> +						const struct iio_chan_spec *chan,
> +						enum iio_event_type type,
> +						enum iio_event_direction dir)
> +{
> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* handle WoM (roc rising) event */
> +	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING)
> +		return st->apex.wom.enable ? 1 : 0;

Invert conditional as below?

> +	return -EINVAL;
> +}
> +
> +static int inv_icm42600_accel_write_event_config(struct iio_dev *indio_dev,
> +						 const struct iio_chan_spec *chan,
> +						 enum iio_event_type type,
> +						 enum iio_event_direction dir,
> +						 bool state)
> +{
> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +
> +	/* handle only WoM (roc rising) event */
> +	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &st->lock) {
> +		if (st->apex.wom.enable == state)
> +			return 0;
> +	}
> +
> +	if (state)
> +		return inv_icm42600_accel_enable_wom(indio_dev);
> +	else
> +		return inv_icm42600_accel_disable_wom(indio_dev);
> +}

...

> +static int inv_icm42600_accel_write_event_value(struct iio_dev *indio_dev,
> +						const struct iio_chan_spec *chan,
> +						enum iio_event_type type,
> +						enum iio_event_direction dir,
> +						enum iio_event_info info,
> +						int val, int val2)
> +{
> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	uint64_t value;
> +	unsigned int accel_hz, accel_uhz;
> +	int ret;
> +
> +	/* handle only WoM (roc rising) event value */
> +	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING || val < 0 || val2 < 0)
> +		return -EINVAL;

Hmm... I think that splitting this will be logically better, as we see the
type/dir conditionals in many functions, and values checks only
exceptionally.

	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
		return -EINVAL;

	if (val < 0 || val2 < 0)
		return -EINVAL;

> +	value = (uint64_t)val * MICRO + (uint64_t)val2;
> +	pm_runtime_get_sync(dev);
> +	scoped_guard(mutex, &st->lock) {
> +		ret = inv_icm42600_accel_read_odr(st, &accel_hz, &accel_uhz);
> +		if (ret >= 0)
> +			ret = inv_icm42600_accel_set_wom_threshold(st, value,
> +								   accel_hz, accel_uhz);
> +	}
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko