| Message ID | 20220704003337.208696-1-ang.iglesiasg@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for pressure sensor Bosch BMP380 | expand |
Hi Angel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 69cb6c6556ad89620547318439d6be8bb1629a5a]
url: https://github.com/intel-lab-lkp/linux/commits/Angel-Iglesias/Add-support-for-pressure-sensor-Bosch-BMP380/20220704-083456
base: 69cb6c6556ad89620547318439d6be8bb1629a5a
config: arm64-buildonly-randconfig-r001-20220703 (https://download.01.org/0day-ci/archive/20220704/202207041006.YJFp2Aj6-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 134363208b9272a967c911f7b56c255a72a6f0a0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/b9905383fbc9858f211da589e86db6675f82f528
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Angel-Iglesias/Add-support-for-pressure-sensor-Bosch-BMP380/20220704-083456
git checkout b9905383fbc9858f211da589e86db6675f82f528
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/iio/pressure/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/iio/pressure/bmp280-core.c:1230:11: warning: variable 'change' is uninitialized when used here [-Wuninitialized]
change = change || aux;
^~~~~~
drivers/iio/pressure/bmp280-core.c:1205:13: note: initialize the variable 'change' to silence this warning
bool change, aux;
^
= 0
1 warning generated.
vim +/change +1230 drivers/iio/pressure/bmp280-core.c
1199
1200 static int bmp380_chip_config(struct bmp280_data *data)
1201 {
1202 u8 osrs;
1203 unsigned int tmp;
1204 int ret;
1205 bool change, aux;
1206
1207 /* configure power control register */
1208 ret = regmap_update_bits(data->regmap, BMP380_REG_POWER_CONTROL,
1209 BMP380_CTRL_SENSORS_MASK,
1210 BMP380_CTRL_SENSORS_PRESS_EN |
1211 BMP380_CTRL_SENSORS_TEMP_EN);
1212 if (ret < 0) {
1213 dev_err(data->dev,
1214 "failed to write operation control register\n");
1215 return ret;
1216 }
1217
1218 /* configure oversampling */
1219 osrs = FIELD_PREP(BMP380_OSRS_TEMP_MASK, data->oversampling_temp) |
1220 FIELD_PREP(BMP380_OSRS_PRESS_MASK, data->oversampling_press);
1221
1222 ret = regmap_update_bits_check(data->regmap, BMP380_REG_OSR,
1223 BMP380_OSRS_TEMP_MASK |
1224 BMP380_OSRS_PRESS_MASK,
1225 osrs, &aux);
1226 if (ret < 0) {
1227 dev_err(data->dev, "failed to write oversampling register\n");
1228 return ret;
1229 }
> 1230 change = change || aux;
1231
1232 /* configure output data rate */
1233 ret = regmap_update_bits_check(data->regmap, BMP380_REG_ODR,
1234 BMP380_ODRS_MASK, data->sampling_freq,
1235 &aux);
1236 if (ret < 0) {
1237 dev_err(data->dev, "failed to write ODR selection register\n");
1238 return ret;
1239 }
1240 change = change || aux;
1241
1242 /* set filter data */
1243 ret = regmap_update_bits_check(data->regmap, BMP380_REG_CONFIG,
1244 BMP380_FILTER_MASK,
1245 FIELD_PREP(BMP380_FILTER_MASK, data->iir_filter_coeff),
1246 &aux);
1247 if (ret < 0) {
1248 dev_err(data->dev, "failed to write config register\n");
1249 return ret;
1250 }
1251 change = change || aux;
1252
1253 if (change) {
1254 /* cycle sensor state machine to reset any measurement in progress
1255 * configuration errors are detected in a measurment cycle.
1256 * If the sampling frequency is too low, it is faster to reset
1257 * measurement cycle and restart measurements
1258 */
1259 ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
1260 BMP380_MODE_MASK,
1261 FIELD_PREP(BMP380_MODE_MASK,
1262 BMP380_MODE_SLEEP));
1263 if (ret < 0) {
1264 dev_err(data->dev, "failed to set sleep mode\n");
1265 return ret;
1266 }
1267 usleep_range(2000, 2500);
1268 ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
1269 BMP380_MODE_MASK,
1270 FIELD_PREP(BMP380_MODE_MASK,
1271 BMP380_MODE_NORMAL));
1272 if (ret < 0) {
1273 dev_err(data->dev, "failed to set normal mode\n");
1274 return ret;
1275 }
1276 /* wait before checking the configuration error flag.
1277 * Worst case value for measure time indicated in the datashhet
1278 * in section 3.9.1 is used.
1279 */
1280 msleep(80);
1281
1282 /* check config error flag */
1283 ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);
1284 if (ret < 0) {
1285 dev_err(data->dev,
1286 "failed to read error register\n");
1287 return ret;
1288 }
1289 if (tmp & BMP380_ERR_CONF_MASK) {
1290 dev_warn(data->dev,
1291 "sensor flagged configuration as incompatible\n");
1292 return -EINVAL;
1293 }
1294 }
1295
1296 return 0;
1297 }
1298
On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@gmail.com> wrote: > > Allows to configure the IIR filter coefficient and the sampling frequency > The IIR filter coefficient is exposed using the sysfs attribute > "filter_low_pass_3db_frequency" In all your commit messages, please pay attention to English grammar. Here you forgot all the periods. ... > + BMP380_ODR_0_0015HZ Keep a comma here. ... > + /* BMP380 devices introduce sampling frequecy configuration. See frequency. > + * datasheet sections 3.3.3. and 4.3.19. > + * > + * BMx280 devices allowed indirect configuration of sampling frequency > + * changing the t_standby duration between measurements. See datasheet > + * section 3.6.3 > + */ /* * Multi-line comment style * example. Use it. */ ... > + if (unlikely(!data->chip_info->sampling_freq_avail)) { Why unlikely() ? How does this improve code generation / performance? ... > + if (unlikely(!data->chip_info->iir_filter_coeffs_avail)) { Ditto. ... > + /* > + * Error applying new configuration. Might be > + * an invalid configuration, will try to > + * restore previous value just to be sure Missed period. Please, check all your texts (commit messages, comments, etc) for proper English grammar. > + */ ... > + /* > + * Error applying new configuration. Might be > + * an invalid configuration, will try to > + * restore previous value just to be sure Ditto. > + */ ... > + /* > + * Error applying new configuration. Might be > + * an invalid configuration, will try to > + * restore previous value just to be sure Ditto. > + */ ... > + /* > + * Error applying new configuration. Might be > + * an invalid configuration, will try to > + * restore previous value just to be sure Ditto. > + */ ... > + /* > + * Error applying new configuration. Might be > + * an invalid configuration, will try to > + * restore previous value just to be sure Ditto. > + */ Why do you need to copy'n'paste dozens of the very same comment? Wouldn't it be enough to explain it somewhere at the top of the file or in the respective documentation (if it exists)? ... > u8 osrs; > unsigned int tmp; > int ret; > + bool change, aux; Move them up, and probably use reversed xmas tree ordering ("longest line first" rule). Also should be bool change = false, aux; ... > + change = change || aux; change ||= aux; And in other cases. ... > + /* cycle sensor state machine to reset any measurement in progress > + * configuration errors are detected in a measurment cycle. measurement > + * If the sampling frequency is too low, it is faster to reset > + * measurement cycle and restart measurements > + */ Completely wrong comment style. Be consistent and follow the common standards in the Linux kernel. ... > + /* wait before checking the configuration error flag. > + * Worst case value for measure time indicated in the datashhet datasheet > + * in section 3.9.1 is used. > + */ Ditto.
Thank you for your comments! On Mon, 2022-07-04 at 22:08 +0200, Andy Shevchenko wrote: > On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@gmail.com> wrote: > > > > Allows to configure the IIR filter coefficient and the sampling frequency > > The IIR filter coefficient is exposed using the sysfs attribute > > "filter_low_pass_3db_frequency" > > In all your commit messages, please pay attention to English grammar. > Here you forgot all the periods. > > ... > > > + BMP380_ODR_0_0015HZ > > Keep a comma here. > > ... > > > + /* BMP380 devices introduce sampling frequecy configuration. See > > frequency. > > > + * datasheet sections 3.3.3. and 4.3.19. > > + * > > + * BMx280 devices allowed indirect configuration of sampling > > frequency > > + * changing the t_standby duration between measurements. See > > datasheet > > + * section 3.6.3 > > + */ > > /* > * Multi-line comment style > * example. Use it. > */ > > ... > > > + if (unlikely(!data->chip_info->sampling_freq_avail)) { > > Why unlikely() ? How does this improve code generation / performance? > > ... As Jonathan Cameron sugested on a previous version of the patch, even thought this code should be safe (as if we are checking sampling frequency is because the sensor is a BMP380 and has that property), it would be better to have a sanity check just to be sure the property is really available. I used unlikely macro to take into account that the property would be almost always initialized. Now that you mention, probably this code won't be called too often to make the "unlikely" branching hint make a meaningful performance difference > > + if (unlikely(!data->chip_info->iir_filter_coeffs_avail)) { > > Ditto. > > ... > > > + /* > > + * Error applying new configuration. Might > > be > > + * an invalid configuration, will try to > > + * restore previous value just to be sure > > Missed period. Please, check all your texts (commit messages, > comments, etc) for proper English grammar. Apologies, I'll be more careful before sending the revised patches next time > > > + */ > > ... > > > + /* > > + * Error applying new configuration. Might > > be > > + * an invalid configuration, will try to > > + * restore previous value just to be sure > > Ditto. > > > + */ > > ... > > > + /* > > + * Error applying new configuration. Might > > be > > + * an invalid configuration, will try to > > + * restore previous value just to be sure > > Ditto. > > > + */ > > ... > > > + /* > > + * Error applying new configuration. Might > > be > > + * an invalid configuration, will try to > > + * restore previous value just to be sure > > Ditto. > > > + */ > > ... > > > + /* > > + * Error applying new configuration. Might > > be > > + * an invalid configuration, will try to > > + * restore previous value just to be sure > > Ditto. > > > + */ > > Why do you need to copy'n'paste dozens of the very same comment? > Wouldn't it be enough to explain it somewhere at the top of the file > or in the respective documentation (if it exists)? > > ... > > > u8 osrs; > > unsigned int tmp; > > int ret; > > + bool change, aux; > > Move them up, and probably use reversed xmas tree ordering ("longest > line first" rule). > > Also should be > bool change = false, aux; > > ... > > > + change = change || aux; > > change ||= aux; I think I'm missing something, do you mean to use '|='? > > And in other cases. > > ... > > > + /* cycle sensor state machine to reset any measurement in > > progress > > + * configuration errors are detected in a measurment cycle. > > measurement > > > + * If the sampling frequency is too low, it is faster to > > reset > > + * measurement cycle and restart measurements > > + */ > > Completely wrong comment style. Be consistent and follow the common > standards in the Linux kernel. > > ... > > > + /* wait before checking the configuration error flag. > > + * Worst case value for measure time indicated in the > > datashhet > > datasheet > > > + * in section 3.9.1 is used. > > + */ > > Ditto. >
On Wed, Jul 6, 2022 at 12:51 AM Angel Iglesias <ang.iglesiasg@gmail.com> wrote: > On Mon, 2022-07-04 at 22:08 +0200, Andy Shevchenko wrote: > > On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@gmail.com> wrote: ... > > > + if (unlikely(!data->chip_info->sampling_freq_avail)) { > > > > Why unlikely() ? How does this improve code generation / performance? > > As Jonathan Cameron sugested on a previous version of the patch, even thought > this code should be safe (as if we are checking sampling frequency is because > the sensor is a BMP380 and has that property), it would be better to have a > sanity check just to be sure the property is really available. I used unlikely > macro to take into account that the property would be almost always initialized. > > Now that you mention, probably this code won't be called too often to make the > "unlikely" branching hint make a meaningful performance difference > > > > + if (unlikely(!data->chip_info->iir_filter_coeffs_avail)) { > > > > Ditto. Is this really a performance-critical path? How did you check that unlikely() makes sense? More evidence, please! ... > > Why do you need to copy'n'paste dozens of the very same comment? > > Wouldn't it be enough to explain it somewhere at the top of the file > > or in the respective documentation (if it exists)? No answer?
On mié, 2022-07-06 at 14:42 +0200, Andy Shevchenko wrote: > On Wed, Jul 6, 2022 at 12:51 AM Angel Iglesias <ang.iglesiasg@gmail.com> > wrote: > > On Mon, 2022-07-04 at 22:08 +0200, Andy Shevchenko wrote: > > > On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@gmail.com> > > > wrote: > > ... > > > > > + if (unlikely(!data->chip_info->sampling_freq_avail)) { > > > > > > Why unlikely() ? How does this improve code generation / performance? > > > > As Jonathan Cameron sugested on a previous version of the patch, even > > thought > > this code should be safe (as if we are checking sampling frequency is > > because > > the sensor is a BMP380 and has that property), it would be better to have a > > sanity check just to be sure the property is really available. I used > > unlikely > > macro to take into account that the property would be almost always > > initialized. > > > > Now that you mention, probably this code won't be called too often to make > > the > > "unlikely" branching hint make a meaningful performance difference > > > > > > + if (unlikely(!data->chip_info->iir_filter_coeffs_avail)) > > > > { > > > > > > Ditto. > > Is this really a performance-critical path? How did you check that > unlikely() makes sense? > More evidence, please! You're right. This code will be invoked by userspace using the sysfs ABI, probably just once, to check sensor settings. The unlikely() is out place, I'll drop it in next patch iteration. > ... > > > > Why do you need to copy'n'paste dozens of the very same comment? > > > Wouldn't it be enough to explain it somewhere at the top of the file > > > or in the respective documentation (if it exists)? > > No answer? Apologies, I'll fix the duplicated comments. Would be a good place for the comment before the function "bmp280_write_raw" or at the start of the switch block?
On Wed, Jul 6, 2022 at 4:39 PM Angel Iglesias <ang.iglesiasg@gmail.com> wrote: > On mié, 2022-07-06 at 14:42 +0200, Andy Shevchenko wrote: > > On Wed, Jul 6, 2022 at 12:51 AM Angel Iglesias <ang.iglesiasg@gmail.com> > > wrote: > > > On Mon, 2022-07-04 at 22:08 +0200, Andy Shevchenko wrote: > > > > On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@gmail.com> > > > > wrote: ... > > > > Why do you need to copy'n'paste dozens of the very same comment? > > > > Wouldn't it be enough to explain it somewhere at the top of the file > > > > or in the respective documentation (if it exists)? > > > > No answer? > > Apologies, I'll fix the duplicated comments. Would be a good place for the > comment before the function "bmp280_write_raw" or at the start of the switch > block? I believe you may find the best place yourself. My point is to see no duplication, that's it.
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 0d2395a28df8..fb321419bff8 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -99,6 +99,27 @@ static const char *const bmp280_supply_names[] = { "vddd", "vdda" }; +enum bmp380_odr { + BMP380_ODR_200HZ, + BMP380_ODR_100HZ, + BMP380_ODR_50HZ, + BMP380_ODR_25HZ, + BMP380_ODR_12_5HZ, + BMP380_ODR_6_25HZ, + BMP380_ODR_3_125HZ, + BMP380_ODR_1_5625HZ, + BMP380_ODR_0_78HZ, + BMP380_ODR_0_39HZ, + BMP380_ODR_0_2HZ, + BMP380_ODR_0_1HZ, + BMP380_ODR_0_05HZ, + BMP380_ODR_0_02HZ, + BMP380_ODR_0_01HZ, + BMP380_ODR_0_006HZ, + BMP380_ODR_0_003HZ, + BMP380_ODR_0_0015HZ +}; + #define BMP280_NUM_SUPPLIES ARRAY_SIZE(bmp280_supply_names) struct bmp280_data { @@ -120,6 +141,16 @@ struct bmp280_data { u8 oversampling_press; u8 oversampling_temp; u8 oversampling_humid; + u8 iir_filter_coeff; + + /* BMP380 devices introduce sampling frequecy configuration. See + * datasheet sections 3.3.3. and 4.3.19. + * + * BMx280 devices allowed indirect configuration of sampling frequency + * changing the t_standby duration between measurements. See datasheet + * section 3.6.3 + */ + int sampling_freq; /* * Carryover value from temperature conversion, used in pressure @@ -131,6 +162,7 @@ struct bmp280_data { struct bmp280_chip_info { unsigned int id_reg; + const struct iio_chan_spec *channels; int num_channels; unsigned int start_up_time; @@ -146,6 +178,14 @@ struct bmp280_chip_info { int num_oversampling_humid_avail; int oversampling_humid_default; + const int *iir_filter_coeffs_avail; + int num_iir_filter_coeffs_avail; + int iir_filter_coeff_default; + + const int (*sampling_freq_avail)[2]; + int num_sampling_freq_avail; + int sampling_freq_default; + int (*chip_config)(struct bmp280_data *); int (*read_temp)(struct bmp280_data *, int *); int (*read_press)(struct bmp280_data *, int *, int *); @@ -197,6 +237,30 @@ static const struct iio_chan_spec bmp280_channels[] = { }, }; +static const struct iio_chan_spec bmp380_channels[] = { + { + .type = IIO_PRESSURE, + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), + }, + { + .type = IIO_TEMP, + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), + }, + { + .type = IIO_HUMIDITYRELATIVE, + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), + }, +}; + static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip) { int ret; @@ -524,6 +588,25 @@ static int bmp280_read_raw(struct iio_dev *indio_dev, break; } break; + case IIO_CHAN_INFO_SAMP_FREQ: + if (unlikely(!data->chip_info->sampling_freq_avail)) { + ret = -EINVAL; + break; + } + + *val = data->chip_info->sampling_freq_avail[data->sampling_freq][0]; + *val2 = data->chip_info->sampling_freq_avail[data->sampling_freq][1]; + ret = IIO_VAL_INT_PLUS_MICRO; + break; + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: + if (unlikely(!data->chip_info->iir_filter_coeffs_avail)) { + ret = -EINVAL; + break; + } + + *val = data->chip_info->iir_filter_coeffs_avail[data->iir_filter_coeff]; + ret = IIO_VAL_INT; + break; default: ret = -EINVAL; break; @@ -540,14 +623,27 @@ static int bmp280_write_oversampling_ratio_humid(struct bmp280_data *data, int val) { int i; + int ret, prev; const int *avail = data->chip_info->oversampling_humid_avail; const int n = data->chip_info->num_oversampling_humid_avail; for (i = 0; i < n; i++) { if (avail[i] == val) { + prev = data->oversampling_humid; data->oversampling_humid = ilog2(val); - return data->chip_info->chip_config(data); + ret = data->chip_info->chip_config(data); + if (ret) { + /* + * Error applying new configuration. Might be + * an invalid configuration, will try to + * restore previous value just to be sure + */ + data->oversampling_humid = prev; + data->chip_info->chip_config(data); + return ret; + } + return 0; } } return -EINVAL; @@ -557,14 +653,27 @@ static int bmp280_write_oversampling_ratio_temp(struct bmp280_data *data, int val) { int i; + int ret, prev; const int *avail = data->chip_info->oversampling_temp_avail; const int n = data->chip_info->num_oversampling_temp_avail; for (i = 0; i < n; i++) { if (avail[i] == val) { + prev = data->oversampling_temp; data->oversampling_temp = ilog2(val); - return data->chip_info->chip_config(data); + ret = data->chip_info->chip_config(data); + if (ret) { + /* + * Error applying new configuration. Might be + * an invalid configuration, will try to + * restore previous value just to be sure + */ + data->oversampling_temp = prev; + data->chip_info->chip_config(data); + return ret; + } + return 0; } } return -EINVAL; @@ -574,14 +683,87 @@ static int bmp280_write_oversampling_ratio_press(struct bmp280_data *data, int val) { int i; + int ret, prev; const int *avail = data->chip_info->oversampling_press_avail; const int n = data->chip_info->num_oversampling_press_avail; for (i = 0; i < n; i++) { if (avail[i] == val) { + prev = data->oversampling_press; data->oversampling_press = ilog2(val); - return data->chip_info->chip_config(data); + ret = data->chip_info->chip_config(data); + if (ret) { + /* + * Error applying new configuration. Might be + * an invalid configuration, will try to + * restore previous value just to be sure + */ + data->oversampling_press = prev; + data->chip_info->chip_config(data); + return ret; + } + return 0; + } + } + return -EINVAL; +} + +static int bmp280_write_sampling_frequency(struct bmp280_data *data, + int val, int val2) +{ + int i; + int ret, prev; + const int (*avail)[2] = data->chip_info->sampling_freq_avail; + const int n = data->chip_info->num_sampling_freq_avail; + + for (i = 0; i < n; i++) { + if (avail[i][0] == val && avail[i][1] == val2) { + prev = data->sampling_freq; + data->sampling_freq = i; + + ret = data->chip_info->chip_config(data); + if (ret) { + /* + * Error applying new configuration. Might be + * an invalid configuration, will try to + * restore previous value just to be sure + */ + data->sampling_freq = prev; + data->chip_info->chip_config(data); + return ret; + } + return 0; + } + } + return -EINVAL; +} + +static int bmp280_write_iir_filter_coeffs(struct bmp280_data *data, int val) +{ + int i; + int ret, prev; + const int *avail = data->chip_info->iir_filter_coeffs_avail; + const int n = data->chip_info->num_iir_filter_coeffs_avail; + + for (i = 0; i < n; i++) { + if (avail[i] == val) { + prev = data->iir_filter_coeff; + data->iir_filter_coeff = i; + + ret = data->chip_info->chip_config(data); + if (ret) { + /* + * Error applying new configuration. Might be + * an invalid configuration, will try to + * restore previous value just to be sure + */ + data->iir_filter_coeff = prev; + data->chip_info->chip_config(data); + return ret; + + } + return 0; } } return -EINVAL; @@ -616,6 +798,22 @@ static int bmp280_write_raw(struct iio_dev *indio_dev, pm_runtime_mark_last_busy(data->dev); pm_runtime_put_autosuspend(data->dev); break; + case IIO_CHAN_INFO_SAMP_FREQ: + pm_runtime_get_sync(data->dev); + mutex_lock(&data->lock); + ret = bmp280_write_sampling_frequency(data, val, val2); + mutex_unlock(&data->lock); + pm_runtime_mark_last_busy(data->dev); + pm_runtime_put_autosuspend(data->dev); + break; + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: + pm_runtime_get_sync(data->dev); + mutex_lock(&data->lock); + ret = bmp280_write_iir_filter_coeffs(data, val); + mutex_unlock(&data->lock); + pm_runtime_mark_last_busy(data->dev); + pm_runtime_put_autosuspend(data->dev); + break; default: return -EINVAL; } @@ -646,6 +844,17 @@ static int bmp280_read_avail(struct iio_dev *indio_dev, } *type = IIO_VAL_INT; return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_SAMP_FREQ: + *vals = (const int *)data->chip_info->sampling_freq_avail; + *type = IIO_VAL_INT_PLUS_MICRO; + /* Values are stored in a 2D matrix */ + *length = data->chip_info->num_sampling_freq_avail; + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: + *vals = data->chip_info->iir_filter_coeffs_avail; + *type = IIO_VAL_INT; + *length = data->chip_info->num_iir_filter_coeffs_avail; + return IIO_AVAIL_LIST; default: return -EINVAL; } @@ -691,6 +900,7 @@ static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 }; static const struct bmp280_chip_info bmp280_chip_info = { .id_reg = BMP280_REG_ID, .start_up_time = 2000, + .channels = bmp280_channels, .num_channels = 2, .oversampling_temp_avail = bmp280_oversampling_avail, @@ -728,6 +938,7 @@ static int bme280_chip_config(struct bmp280_data *data) static const struct bmp280_chip_info bme280_chip_info = { .id_reg = BMP280_REG_ID, .start_up_time = 2000, + .channels = bmp280_channels, .num_channels = 3, .oversampling_temp_avail = bmp280_oversampling_avail, @@ -965,18 +1176,39 @@ static int bmp380_read_calib(struct bmp280_data *data, unsigned int chip) return 0; } +static const int bmp380_odr_table[][2] = { + [BMP380_ODR_200HZ] = {200, 0}, + [BMP380_ODR_100HZ] = {100, 0}, + [BMP380_ODR_50HZ] = {50, 0}, + [BMP380_ODR_25HZ] = {25, 0}, + [BMP380_ODR_12_5HZ] = {12, 500000}, + [BMP380_ODR_6_25HZ] = {6, 250000}, + [BMP380_ODR_3_125HZ] = {3, 125000}, + [BMP380_ODR_1_5625HZ] = {1, 562500}, + [BMP380_ODR_0_78HZ] = {0, 781250}, + [BMP380_ODR_0_39HZ] = {0, 390625}, + [BMP380_ODR_0_2HZ] = {0, 195313}, + [BMP380_ODR_0_1HZ] = {0, 97656}, + [BMP380_ODR_0_05HZ] = {0, 48828}, + [BMP380_ODR_0_02HZ] = {0, 24414}, + [BMP380_ODR_0_01HZ] = {0, 12207}, + [BMP380_ODR_0_006HZ] = {0, 6104}, + [BMP380_ODR_0_003HZ] = {0, 3052}, + [BMP380_ODR_0_0015HZ] = {0, 1526}, +}; + static int bmp380_chip_config(struct bmp280_data *data) { u8 osrs; unsigned int tmp; int ret; + bool change, aux; /* configure power control register */ - ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL, - BMP380_CTRL_SENSORS_MASK | BMP380_MODE_MASK, - BMP380_CTRL_SENSORS_PRESS_EN | - BMP380_CTRL_SENSORS_TEMP_EN | - FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_NORMAL)); + ret = regmap_update_bits(data->regmap, BMP380_REG_POWER_CONTROL, + BMP380_CTRL_SENSORS_MASK, + BMP380_CTRL_SENSORS_PRESS_EN | + BMP380_CTRL_SENSORS_TEMP_EN); if (ret < 0) { dev_err(data->dev, "failed to write operation control register\n"); @@ -987,55 +1219,90 @@ static int bmp380_chip_config(struct bmp280_data *data) osrs = FIELD_PREP(BMP380_OSRS_TEMP_MASK, data->oversampling_temp) | FIELD_PREP(BMP380_OSRS_PRESS_MASK, data->oversampling_press); - ret = regmap_write_bits(data->regmap, BMP380_REG_OSR, - BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK, - osrs); + ret = regmap_update_bits_check(data->regmap, BMP380_REG_OSR, + BMP380_OSRS_TEMP_MASK | + BMP380_OSRS_PRESS_MASK, + osrs, &aux); if (ret < 0) { dev_err(data->dev, "failed to write oversampling register\n"); return ret; } + change = change || aux; /* configure output data rate */ - ret = regmap_write_bits(data->regmap, BMP380_REG_ODR, - BMP380_ODRS_MASK, BMP380_ODRS_50HZ); + ret = regmap_update_bits_check(data->regmap, BMP380_REG_ODR, + BMP380_ODRS_MASK, data->sampling_freq, + &aux); if (ret < 0) { dev_err(data->dev, "failed to write ODR selection register\n"); return ret; } + change = change || aux; /* set filter data */ - ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG, - BMP380_FILTER_MASK, - FIELD_PREP(BMP380_FILTER_MASK, BMP380_FILTER_3X)); + ret = regmap_update_bits_check(data->regmap, BMP380_REG_CONFIG, + BMP380_FILTER_MASK, + FIELD_PREP(BMP380_FILTER_MASK, data->iir_filter_coeff), + &aux); if (ret < 0) { dev_err(data->dev, "failed to write config register\n"); return ret; } + change = change || aux; - /* wait startup_time before verifying config changes */ - usleep_range(data->start_up_time, data->start_up_time + 100); + if (change) { + /* cycle sensor state machine to reset any measurement in progress + * configuration errors are detected in a measurment cycle. + * If the sampling frequency is too low, it is faster to reset + * measurement cycle and restart measurements + */ + ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL, + BMP380_MODE_MASK, + FIELD_PREP(BMP380_MODE_MASK, + BMP380_MODE_SLEEP)); + if (ret < 0) { + dev_err(data->dev, "failed to set sleep mode\n"); + return ret; + } + usleep_range(2000, 2500); + ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL, + BMP380_MODE_MASK, + FIELD_PREP(BMP380_MODE_MASK, + BMP380_MODE_NORMAL)); + if (ret < 0) { + dev_err(data->dev, "failed to set normal mode\n"); + return ret; + } + /* wait before checking the configuration error flag. + * Worst case value for measure time indicated in the datashhet + * in section 3.9.1 is used. + */ + msleep(80); - /* check config error flag */ - ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp); - if (ret < 0) { - dev_err(data->dev, - "failed to read error register\n"); - return ret; - } - if (tmp & BMP380_ERR_CONF_MASK) { - dev_warn(data->dev, - "sensor flagged configuration as incompatible\n"); - return -EINVAL; + /* check config error flag */ + ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp); + if (ret < 0) { + dev_err(data->dev, + "failed to read error register\n"); + return ret; + } + if (tmp & BMP380_ERR_CONF_MASK) { + dev_warn(data->dev, + "sensor flagged configuration as incompatible\n"); + return -EINVAL; + } } return 0; } static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 }; +static const int bmp380_iir_filter_coeffs_avail[] = { 0, 1, 3, 7, 15, 31, 63, 127 }; static const struct bmp280_chip_info bmp380_chip_info = { .id_reg = BMP380_REG_ID, .start_up_time = 2000, + .channels = bmp380_channels, .num_channels = 2, .oversampling_temp_avail = bmp380_oversampling_avail, @@ -1046,6 +1313,14 @@ static const struct bmp280_chip_info bmp380_chip_info = { .num_oversampling_press_avail = ARRAY_SIZE(bmp380_oversampling_avail), .oversampling_press_default = ilog2(4), + .sampling_freq_avail = bmp380_odr_table, + .num_sampling_freq_avail = ARRAY_SIZE(bmp380_odr_table) * 2, + .sampling_freq_default = BMP380_ODR_50HZ, + + .iir_filter_coeffs_avail = bmp380_iir_filter_coeffs_avail, + .num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail), + .iir_filter_coeff_default = 2, + .chip_config = bmp380_chip_config, .read_temp = bmp380_read_temp, .read_press = bmp380_read_press, @@ -1282,6 +1557,7 @@ static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 }; static const struct bmp280_chip_info bmp180_chip_info = { .id_reg = BMP280_REG_ID, .start_up_time = 2000, + .channels = bmp280_channels, .num_channels = 2, .oversampling_temp_avail = bmp180_oversampling_temp_avail, @@ -1380,7 +1656,6 @@ int bmp280_common_probe(struct device *dev, data->dev = dev; indio_dev->name = name; - indio_dev->channels = bmp280_channels; indio_dev->info = &bmp280_info; indio_dev->modes = INDIO_DIRECT_MODE; @@ -1403,10 +1678,13 @@ int bmp280_common_probe(struct device *dev, data->chip_info = chip_info; /* apply initial values from chip info structure */ + indio_dev->channels = chip_info->channels; indio_dev->num_channels = chip_info->num_channels; data->oversampling_press = chip_info->oversampling_press_default; data->oversampling_humid = chip_info->oversampling_humid_default; data->oversampling_temp = chip_info->oversampling_temp_default; + data->iir_filter_coeff = chip_info->iir_filter_coeff_default; + data->sampling_freq = chip_info->sampling_freq_default; data->start_up_time = chip_info->start_up_time; /* Bring up regulators */ diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index fd38906c889c..1314d5059c53 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -54,24 +54,6 @@ #define BMP380_OSRS_PRESS_MASK GENMASK(2, 0) #define BMP380_ODRS_MASK GENMASK(4, 0) -#define BMP380_ODRS_200HZ 0x00 -#define BMP380_ODRS_100HZ 0x01 -#define BMP380_ODRS_50HZ 0x02 -#define BMP380_ODRS_25HZ 0x03 -#define BMP380_ODRS_12_5HZ 0x04 -#define BMP380_ODRS_6_25HZ 0x05 -#define BMP380_ODRS_3_1HZ 0x06 -#define BMP380_ODRS_1_5HZ 0x07 -#define BMP380_ODRS_0_78HZ 0x08 -#define BMP380_ODRS_0_39HZ 0x09 -#define BMP380_ODRS_0_2HZ 0x0A -#define BMP380_ODRS_0_1HZ 0x0B -#define BMP380_ODRS_0_05HZ 0x0C -#define BMP380_ODRS_0_02HZ 0x0D -#define BMP380_ODRS_0_01HZ 0x0E -#define BMP380_ODRS_0_006HZ 0x0F -#define BMP380_ODRS_0_003HZ 0x10 -#define BMP380_ODRS_0_0015HZ 0x11 #define BMP380_CTRL_SENSORS_MASK GENMASK(1, 0) #define BMP380_CTRL_SENSORS_PRESS_EN BIT(0)
Allows to configure the IIR filter coefficient and the sampling frequency The IIR filter coefficient is exposed using the sysfs attribute "filter_low_pass_3db_frequency" Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 338 ++++++++++++++++++++++++++--- drivers/iio/pressure/bmp280.h | 18 -- 2 files changed, 308 insertions(+), 48 deletions(-)