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

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

Just an update, there'll be a v6 but I currently don't have access to 
the board for testing.

On 04-04-2020 16:48, Jonathan Cameron wrote:
> On Mon, 30 Mar 2020 10:07:00 +0200
> Mike Looijmans <mike.looijmans@topic.nl> wrote:
>
>> 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.
> True, but the table is still there be it in string form  in order
> to provide the 'available' list.
>
> For reasons of simplicity in the IIO core, the read_avail callbacks
> deal with tables of numbers like was originally the case here.
>
>
>>> 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.
> Agreed.  Just add the depends on SPI.  I was just looking forwards to yourself
> or someone else adding I2C when they do have a platform to test it.
>
>
>>
>>> ...
>>>    
>>>> +
>>>> +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.
> That's a bold assumption to make about userspace doing what it should
> and checking _avail before writing crazy values.   A not entirely stupid
> optimization would be to write without checking _avail and only do the
> handling of that if the write fails (indicating that whatever value was
> provided by some user isn't fine).
>
>
>> 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".
> Given the way the IIO core works, it will be perfectly happy to present
> you with precise values.  If the val == 12, then check the val2 == 5
> For the rest just check if the val is correct and val2 == 0.
>
> If decimal part is not provide val2 will be equal to 0.
>
> I'm still falling on the side of thinking a table of values is simpler,
> less bug prone (because it's just a case of linear search for a match)
> and if you use read_avail callback can handle that case as well without
> needing to repeat values.  Memory cost of the
> table is tiny 8 * 4 * 2 = 64 bytes.
>
> It's not exactly a huge list of possibilities.
>
> Jonathan
>
>>>    
>>>> +
>>>> +	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