linux-kernel - Re: [RESEND PATCH v8 5/6] iio: imu: adis16550: add adis16550 support

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: <aee93ef96e71adf70a48ee5877bd75966d9c78c1.camel@gmail.com>
Date: Thu, 20 Feb 2025 08:21:56 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: Robert Budai <robert.budai@...log.com>, Lars-Peter Clausen
 <lars@...afoo.de>,  Michael Hennerich <Michael.Hennerich@...log.com>, Nuno
 Sa <nuno.sa@...log.com>, Ramona Gradinariu	 <ramona.gradinariu@...log.com>,
 Antoniu Miclaus <antoniu.miclaus@...log.com>,  Jonathan Cameron	
 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski	
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet	
 <corbet@....net>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [RESEND PATCH v8 5/6] iio: imu: adis16550: add adis16550 support

On Mon, 2025-02-17 at 12:57 +0200, Robert Budai wrote:
> The ADIS16550 is a complete inertial system that includes a triaxis
> gyroscope and a triaxis accelerometer. Each inertial sensor in the
> ADIS16550 combines industry leading MEMS only technology with signal
> conditioning that optimizes dynamic performance. The factory calibration
> characterizes each sensor for sensitivity, bias, and alignment. As a
> result, each sensor has its own dynamic compensation formulas that
> provide accurate sensor measurements.
> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Nuno Sá <nuno.sa@...log.com>
> Signed-off-by: Robert Budai <robert.budai@...log.com>
> ---
> 

I guess it would make sense a Co-developed-by: for Robert?

Anyways, all looks good except for one thing that I just spotted...

> v8:
> - removed __aligned from struct adis16550, as suggested
> - crc buffer extraction into the crc check function
> - passed buffer into crc validation as original, __be32 and performed check
> using be32_to_cpu conversion of the buffer
> - added trailing comma to line 993
> - removed trailing comma from line 877
> 
>  drivers/iio/imu/Kconfig     |   13 +
>  drivers/iio/imu/Makefile    |    1 +
>  drivers/iio/imu/adis16550.c | 1149 +++++++++++++++++++++++++++++++++++
>  3 files changed, 1163 insertions(+)
>  create mode 100644 drivers/iio/imu/adis16550.c
> 

...

> 
> +static int adis16550_set_freq_hz(struct adis16550 *st, u32 freq_hz)
> +{
> +	u16 dec;
> +	int ret;
> +	u32 sample_rate = st->clk_freq_hz;
> +	/*
> +	 * The optimal sample rate for the supported IMUs is between
> +	 * int_clk - 1000 and int_clk + 500.
> +	 */
> +	u32 max_sample_rate = st->info->int_clk * 1000 + 500000;
> +	u32 min_sample_rate = st->info->int_clk * 1000 - 1000000;
> +
> +	if (!freq_hz)
> +		return -EINVAL;
> +
> +	adis_dev_auto_lock(&st->adis);
> +
> +	if (st->sync_mode == ADIS16550_SYNC_MODE_SCALED) {
> +		unsigned long scaled_rate = lcm(st->clk_freq_hz, freq_hz);
> +		int sync_scale;
> +
> +		if (scaled_rate > max_sample_rate)
> +			scaled_rate = max_sample_rate / st->clk_freq_hz * st-
> >clk_freq_hz;
> +		else
> +			scaled_rate = max_sample_rate / scaled_rate *
> scaled_rate;
> +
> +		if (scaled_rate < min_sample_rate)
> +			scaled_rate = roundup(min_sample_rate, st-
> >clk_freq_hz);
> +

I would imagine the above is the same deal as in other devices [1] or do you
know for a fact this one is different? Maybe it's simple enough for Jonathan to
tweak while applying...

[1]: https://elixir.bootlin.com/linux/v6.13.3/source/drivers/iio/imu/adis16475.c#L364

- Nuno Sá