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

[Budai, Robert]

> -----Original Message-----
> From: Nuno Sá <noname.nuno@xxxxxxxxx>
> Sent: Thursday, February 20, 2025 10:22 AM
> To: Budai, Robert <Robert.Budai@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Gradinariu, Ramona
> <Ramona.Gradinariu@xxxxxxxxxx>; Miclaus, Antoniu
> <Antoniu.Miclaus@xxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>; Rob
> Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor
> Dooley <conor+dt@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-doc@xxxxxxxxxxxxxxx
> Subject: Re: [RESEND PATCH v8 5/6] iio: imu: adis16550: add adis16550
> support
> 
> [External]
> 
> 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@xxxxxxxxxx>
> > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@xxxxxxxxxx>
> > Co-developed-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > Signed-off-by: Robert Budai <robert.budai@xxxxxxxxxx>
> > ---
> >
> 
> 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.13.3/source
> /drivers/iio/imu/adis16475.c*L364__;Iw!!A3Ni8CS0y2Y!7Y71yPaQAxVzNRd
> O_jT7wEz4k-
> s6z4tJHOcES84HYkq8qNGsgJH7zxwjfPNjLF3OEGVInSolo1ennLU_mwpmEbo$
> 
> - Nuno Sá

[Robert Budai] 
No differences were found in the scaled_sync behavior of the ADIS16475 and
ADIS16550. It is safe to add from my side.

Best regards,
Robert B