Messages in this thread | | | Subject | Re: [PATCH v5 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088 | From | Mike Looijmans <> | Date | Fri, 24 Apr 2020 07:54:22 +0200 |
| |
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
| |