[PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[Angel Iglesias]

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(-)

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)
-- 
2.36.1

Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[Angel Iglesias]

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

Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[Angel Iglesias]

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?

Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko