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

[Mike Looijmans <mike.looijmans@topic.nl>]

On 28-03-2020 17:20, Jonathan Cameron wrote:
> 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?

The frequency range is just a simple power-of-two formula, so my take 
was that a table would just be a waste of memory and resources.

A table lookup costs more resources and requires more code in this case.

> 
> 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.
> 

I'm okay with a depends on SPI. Adding the I2C support is not difficult, 
but I don't want to release something into the kernel that I cannot test.


> ...
> 
>> +
>> +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.

I went for simplicity. The driver reports an "avail" range, so users 
should not expect other values like "70" to actually work. The above is 
the shortest inversion of the get_sample_freq function.

Just wanted to make it so that obvious things would work, and I feared 
that a range would require one to spec "100.000" in decimal format just 
because of the existence of the 12.5 value. So the driver is a bit 
forgiving in that specifying "12" or "13" will also work.

For a more exact match I could also add something like:

if (val > 13 && (25 << (value - BMI088_ACCEL_MODE_ODR_25 ) != val)
    return -EINVAL;

this would return -EINVAL for values like "26" or "1599".

> 
>> +
>> +	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.

will remove

> 
>> +		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;
>> +	}
>> +}
>> +
> ...
> 


-- 
Mike Looijmans