Re: [PATCH v6 6/6] iio: imu: inv_icm42607: Add accelerometer calibbias support

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 10/09, Kanak Shilledar wrote:
> Expose IIO_CHAN_INFO_CALIBBIAS on the accelerometer channels. The
> registers are stored in MREG1. The calibration bias is written to
> OFFSET_USER4 to OFFSET_USER8 registers in MREG1. Reject the out of
> limit calibbias values instead of clamping it.
> 
> Datasheet: https://www.invensense.tdk.com/en-us/products/3-axis/icm-42370-p
> Datasheet: https://www.lcsc.com/product-detail/C5129967.html
> Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com>
> ---
...
> +static int inv_icm42607_accel_read_offset(struct inv_icm42607_state *st,
> +		struct iio_chan_spec const *chan, int *val, int *val2)
> +{
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int msb_reg, lsb_reg, nib, byte;
> +	int offset;
> +	s64 val64;
> +	s32 bias;
> +	int ret;
> +	u32 raw;
> +
> +	if (chan->type != IIO_ACCEL)
> +		return -EINVAL;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		msb_reg = INV_ICM42607_REG_OFFSET_USER4;
> +		lsb_reg = INV_ICM42607_REG_OFFSET_USER5;
> +		break;
> +	case IIO_MOD_Y:
> +		msb_reg = INV_ICM42607_REG_OFFSET_USER7;
> +		lsb_reg = INV_ICM42607_REG_OFFSET_USER6;
> +		break;
> +	case IIO_MOD_Z:
> +		msb_reg = INV_ICM42607_REG_OFFSET_USER7;
> +		lsb_reg = INV_ICM42607_REG_OFFSET_USER8;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_read(st->map, msb_reg, &nib);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->map, lsb_reg, &byte);
> +	if (ret)
> +		return ret;
> +
> +	/* 12 bits signed value */
> +	if (chan->channel2 == IIO_MOD_Y)
> +		raw = FIELD_PREP(INV_ICM42607_OFFSET_MSB,
> +				 FIELD_GET(INV_ICM42607_OFFSET_USER_NIBBLE_LO, nib));
> +	else
> +		raw = FIELD_PREP(INV_ICM42607_OFFSET_MSB,
> +				 FIELD_GET(INV_ICM42607_OFFSET_USER_NIBBLE_HI, nib));
> +
> +	raw |= FIELD_PREP(INV_ICM42607_OFFSET_LSB, byte);
> +	offset = sign_extend32(raw, 11);
> +
> +	/*
> +	 * Convert raw offset to g then to m/s²
> +	 * 12 bits signed raw step 0.5mg to g: 5 / 10000
> +	 * g to m/s²: 9.806650
> +	 * Result in micro (1000000)
> +	 * (offset * 5 * 9.806650 * 1000000) / 10000
> +	 */
Maybe add a comment here highlighting that this is an exception among IIO ABI
and provide an additional patch extending the note on in_accel_x_calibbias ABI
doc to include icm42370. Having to amend the ABI doc that way looks to have been
an unfortunate need (also according to commit 69c93a7df74b).

icm42370 seems to be similar to icm42600 so extending the exception might be
okay in this case. Creating new ABI is probably too much.

> +	val64 = (s64)offset * 5LL * 9806650LL;
> +	/* For rounding, add + or - divisor (10000) divided by 2 */
> +	bias = DIV_S64_ROUND_CLOSEST(val64, 10000L);
> +	*val = bias / (long)MEGA;
> +	*val2 = bias % (long)MEGA;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int inv_icm42607_accel_write_offset(struct iio_dev *indio_dev,
> +					   struct iio_chan_spec const *chan,
> +					   int val, int val2)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	s32 min, max;
> +	u8 msb, lsb;
> +	s16 offset;
> +	s64 val64;
> +	int ret;
> +
> +	if (chan->type != IIO_ACCEL)
> +		return -EINVAL;
> +
> +	/* inv_icm42607_accel_calibbias: min - step - max in micro */
> +	min = inv_icm42607_accel_calibbias[0] * (long)MEGA -
> +	      inv_icm42607_accel_calibbias[1];
> +	max = inv_icm42607_accel_calibbias[4] * (long)MEGA +
> +	      inv_icm42607_accel_calibbias[5];
> +
> +	val64 = val * (s64)MEGA;
> +	if (val >= 0)
> +		val64 += val2;
> +	else
> +		val64 -= val2;
> +
> +	if (val64 < min || val64 > max)
> +		return -EINVAL;
> +
> +	/*
> +	 * Convert m/s² to g then to raw value
> +	 * m/s² to g: 1 / 9.806650
> +	 * g to raw 12 bits signed, step 0.5mg: 10000 / 5
> +	 * val in micro (1000000)
> +	 * val * 10000 / (9.806650 * 1000000 * 5)
> +	 */
Unless Jonathan thinks it to be too verbose, we can also have an ABI comment here.

With the ABI exception somehow highlighted,
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

> +	val64 *= 10000LL;
> +	offset = DIV_S64_ROUND_CLOSEST(val64, 9806650 * 5);

With best regards,
Marcelo