Re: [PATCH v5 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 24 Mar 2020 09:03:08 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> The BMI088 is a combined module with both accelerometer and gyroscope.
> This adds the accelerometer driver support for the SPI interface.
> The gyroscope part is already supported by the BMG160 driver.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Hi Mike, 

A few tiny things from me.

For the sampling frequency, I wonder if we are better off going back
to the list of values, but then also using the read_avail infrastructure
to avoid having to carry them as a string as well?

Thanks,

Jonathan

> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 5d91a6dda894..7ed9c82b731b 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -151,6 +151,23 @@ config BMC150_ACCEL_SPI
>  	tristate
>  	select REGMAP_SPI
>  
> +config BMI088_ACCEL
> +	tristate "Bosch BMI088 Accelerometer Driver"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select REGMAP
> +	select BMI088_ACCEL_SPI
> +	help
> +	  Say yes here to build support for the Bosch BMI088 accelerometer.
> +
> +	  This is a combo module with both accelerometer and gyroscope.
> +	  This driver is only implementing accelerometer part, which has
> +	  its own address and register map.
> +
> +config BMI088_ACCEL_SPI
> +	tristate
> +	select REGMAP_SPI
> +

Hmm. So we list this driver even if SPI is disabled.  Generally we try
not to do that, as it makes for lots of things to pick on devices that
don't actually have an SPI bus.

Hence, please add a depends to the first Kconfig symbol so it's hidden
if no SPI.  When I2C is added it can become at least one of the two.

...

> +
> +static int bmi088_accel_get_sample_freq(struct bmi088_accel_data *data,
> +					int *val, int *val2)
> +{
> +	unsigned int value;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
> +			  &value);
> +	if (ret)
> +		return ret;
> +
> +	value &= BMI088_ACCEL_MODE_ODR_MASK;
> +	if (value == BMI088_ACCEL_MODE_ODR_12_5) {
> +		*val = 12;
> +		*val2 = 500000;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +	} else {
> +		*val = 25 << (value - BMI088_ACCEL_MODE_ODR_25);
> +		*val2 = 0;
> +		ret = IIO_VAL_INT;
> +	}
> +
> +	return ret;
> +}
> +
> +static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
> +{
> +	unsigned int value;
> +
> +	if (val < 12 || val > 1600)
> +		return -EINVAL;
> +
> +	value = fls(val) + 1;

This leads to some 'novel' rounding to my mind.

(12,16] = 12.5
(16,32] = 25
(32,64] = 50
(64,128] = 100

Generally we want to go faster if anything when talking about sampling frequencies,
so I'd either like to see round up or precise value matching only.

> +
> +	return regmap_update_bits(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
> +				  BMI088_ACCEL_MODE_ODR_MASK, value);
> +}
> +

...

> +
> +static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int *val, int *val2, long mask)
> +{
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			return bmi088_accel_get_temp(data, val);
> +		case IIO_ACCEL:
> +			ret = iio_device_claim_direct_mode(indio_dev);
> +			if (ret)
> +				return ret;
> +
> +			ret = bmi088_accel_get_axis(data, chan, val);
> +			iio_device_release_direct_mode(indio_dev);
> +			if (ret)
> +				return ret;
> +
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			/* Offset applies before scale */
> +			*val = BMI088_ACCEL_TEMP_OFFSET/BMI088_ACCEL_TEMP_UNIT;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;

Why?  In error paths it's not used, and it's set in the other two paths.

> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			/* 0.125 degrees per LSB */
> +			*val = BMI088_ACCEL_TEMP_UNIT;
> +			return IIO_VAL_INT;
> +		case IIO_ACCEL:
> +			ret = regmap_read(data->regmap,
> +					  BMI088_ACCEL_REG_ACC_RANGE, val);
> +			if (ret)
> +				return ret;
> +
> +			*val2 = 15 - (*val & 0x3);
> +			*val = 3 * 980;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&data->mutex);
> +		ret = bmi088_accel_get_sample_freq(data, val, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
...