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

[Jonathan Cameron <jic23@kernel.org>]

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