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

["Nuno Sá" <noname.nuno@gmail.com>]

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@analog.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Robert Budai <robert.budai@analog.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á