Re: [PATCH v2 5/6] iio: imu: Add support for adis16475

[Lars-Peter Clausen <lars@metafoo.de>]

On 3/16/20 1:53 PM, Nuno Sá wrote:
> Support ADIS16475 and similar IMU devices. These devices are
> a precision, miniature MEMS inertial measurement unit (IMU) that
> includes a triaxial gyroscope and a triaxial accelerometer. Each
> inertial sensor combines with signal conditioning that optimizes
> dynamic performance.
>
> The driver adds support for the following devices:
>   * adis16470, adis16475, adis16477, adis16465, adis16467, adis16500,
>     adis16505, adis16507.
>
Looks very good, just a few comments and questions. I like the solution 
for the 32-bit registers.

> +static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
> +{
> +	u16 dec;
> +	int ret;
> +
> +	if (freq == 0 || freq > st->clk_freq)
> +		return -EINVAL;

Maybe round down if freq is larger the maximum rate. I believe we do 
that same for other drivers.

> +
> +	dec = DIV_ROUND_CLOSEST(st->clk_freq, freq);
> +
> +	if (dec)
> +		dec--;
> +
> +	if (dec > st->info->max_dec)
> +		dec = st->info->max_dec;
> +
> +	ret = adis_write_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, dec);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If decimation is used, then gyro and accel data will have meaningfull
Typo: "meaningful"
> +	 * bits on the LSB registers. This info is used on the trigger handler.
> +	 */
> +	if (!dec)
> +		clear_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);
> +	else
> +		set_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);
> +
> +	return 0;
> +}
> +
> +/* The values are approximated. */
> +static const u32 adis16475_3db_freqs[] = {
> +	[0] = 720, /* Filter disabled, full BW (~720Hz) */
> +	[1] = 360,
> +	[2] = 164,

164 is the only one that does not follow the pattern of 720 / (2**n). 
Not sure if that is on purpose. But either way where did you find the 
bandwidths for the different filter settings? I had a quick look at the 
datasheet and could not find anything.

Maybe it also makes sense to consider the clock rate when running in 
sync mode as the bandwidth will be a function of the sampling rate.

> +	[3] = 80,
> +	[4] = 40,
> +	[5] = 20,
> +	[6] = 10,
> +	[7] = 10, /* not a valid setting */
> +};
> +
> +static int adis16475_get_filter(struct adis16475 *st, u32 *filter)
> +{
> +	u16 filter_sz;
> +	int ret;
> +	const int mask = ADIS16475_FILT_CTRL_MASK;
> +
> +	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FILT_CTRL, &filter_sz);
> +	if (ret)
> +		return ret;
> +
> +	*filter = adis16475_3db_freqs[filter_sz & mask];
> +
> +	return 0;
> +}
> +
> +static int adis16475_set_filter(struct adis16475 *st, const u32 filter)
> +{
> +	int i;
> +	int ret;
> +
> +	for (i = ARRAY_SIZE(adis16475_3db_freqs) - 1; i >= 1; i--) {
> +		if (adis16475_3db_freqs[i] >= filter)
> +			break;
> +	}

If the filter frequency is less or equal to 10, this will pick 7 for i. 
But the comment above says that it is an invalid setting.

>
> +static u16 adis16475_validate_crc(const u8 *buffer, const u16 crc,
> +				  const bool burst32)
Return type is u16, but the return value is bool. Also validate kind of 
suggests that it returns true if valid, but right now it returns true if 
invalid.
> +{
> +	int i;
> +	u16 __crc = 0;

I find having both __crc and crc a bit confusing, maybe give them names 
which better describe them. Maybe computed_crc or expected_crc. Or as an 
alternative I think also just crc -= buffer[i] in the loop and crc != 0 
at the end should work.


> +	/* extra 6 elements for low gyro and accel */
> +	const u16 sz = burst32 ? ADIS16475_BURST32_MAX_DATA :
> +		ADIS16475_BURST_MAX_DATA;
> +
> +	for (i = 0; i < sz - 2; i++)
> +		__crc += buffer[i];
> +
> +	return (__crc != crc);
> +}
> +
> +static void adis16475_burst32_check(struct adis16475 *st)
> +{
> +	int ret;
> +	struct adis *adis = &st->adis;
> +
> +	if (!st->info->has_burst32)
> +		return;
> +
> +	if (st->lsb_flag && !st->burst32) {
> +		const u16 en = ADIS16500_BURST32(1);
> +
> +		ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> +					 ADIS16500_BURST32_MASK, en);
> +		if (ret < 0)
> +			return;
> +
> +		st->burst32 = true;
> +		/*
> +		 * In 32bit mode we need extra 2 bytes for all gyro
> +		 * and accel channels.
> +		 */
> +		adis->burst->extra_len += 6 * sizeof(u16);

I believe this breaks if you have more than one device instance as 
adis->burst points to a global struct that is shared between all 
instances. extra_len should probably be part of the adis struct itself 
and then make the burst field const.

> +		adis->xfer[1].len += 6 * sizeof(u16);
> +		dev_dbg(&adis->spi->dev, "Enable burst32 mode, xfer:%d",
> +			adis->xfer[1].len);
> +
> +	} else if (!st->lsb_flag && st->burst32) {
> +		const u16 en = ADIS16500_BURST32(0);
> +
> +		ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> +					 ADIS16500_BURST32_MASK, en);
> +		if (ret < 0)
> +			return;
> +
> +		st->burst32 = false;
> +		/* Remove the extra bits */
> +		adis->burst->extra_len -= 6 * sizeof(u16);
> +		adis->xfer[1].len -= 6 * sizeof(u16);
> +		dev_dbg(&adis->spi->dev, "Disable burst32 mode, xfer:%d\n",
> +			adis->xfer[1].len);
> +	}
> +}
> +