BMP280: Minor cleanup and interrupt support

Up to now, the BMP280 device is using the regmap of the BME280 which has registers that exist only in the BME280 device.

Fixes 14e8015f8569 ("iio: pressure: bmp280: split driver in logical parts")

Signed-off-by Vasileios Amoiridis <[email protected]>

Quoted 5 lines · On Thu, 11 Jul 2024 23:15:49 +0200 Vasileios Amoiridis…

Up to now, the BMP280 device is using the regmap of the BME280 which has registers that exist only in the BME280 device.

Fixes 14e8015f8569 ("iio: pressure: bmp280: split driver in logical parts")

Signed-off-by Vasileios Amoiridis <[email protected]>

This fix is for an ancient issue (2016) so I'm not going to rush it in but will mark it for stable inclusion. Advantage of taking this through the main tree is we can move faster with the rest of the series. So applied to the testing branch of iio.git which will be rebase on rc1 when available and become togreg for this cycle.

Thanks,

Jonathan

According to the datasheet, both pressure and temperature can go up to oversampling x32. With this option, the maximum measurement time is not 80ms (this is for press x32 and temp x2), but it is 130ms nominal (calculated from table 3.9.2) and since most of the maximum values are around +15%, it is configured to 150ms.

Fixes 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")

Signed-off-by Vasileios Amoiridis <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iio/pressure/bmp280-core.c +4 -3

index cc8553177977..3deaa57bb3f5 100644
@@ -1581,10 +1581,11 @@ static int bmp380_chip_config(struct bmp280_data *data)
 		}
 		/*
 		 * Waits for measurement before checking configuration error
-		 * flag. Selected longest measure time indicated in
-		 * section 3.9.1 in the datasheet.
+		 * flag. Selected longest measurement time, calculated from
+		 * formula in datasheet section 3.9.2 with an offset of ~+15%
+		 * as it seen as well in table 3.9.1.
 		 */
-		msleep(80);
+		msleep(150);
 
 		/* Check config error flag */
 		ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);

Quoted 6 lines · On Thu, 11 Jul 2024 23:15:50 +0200 Vasileios Amoiridis…

According to the datasheet, both pressure and temperature can go up to oversampling x32. With this option, the maximum measurement time is not 80ms (this is for press x32 and temp x2), but it is 130ms nominal (calculated from table 3.9.2) and since most of the maximum values are around +15%, it is configured to 150ms.

Quoted 2 lines

Fixes 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")

Signed-off-by Vasileios Amoiridis <[email protected]>

Applied to what will be the togreg branch of iio.git and marked for stable.

If it needs to move quicker as we have reports of problems from users let me know.

Jonathan

Sort headers in alphabetical order

Signed-off-by Vasileios Amoiridis <[email protected]>

Convert individual reads to a bulk read for the humidity calibration data.

Signed-off-by Vasileios Amoiridis <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 57 +++++++++---------------------
 drivers/iio/pressure/bmp280.h      |  5 +++
 2 files changed, 21 insertions(+), 41 deletions(-)

drivers/iio/pressure/bmp280-core.c +0 -0

index 3deaa57bb3f5..9c32266403bd 100644

drivers/iio/pressure/bmp280-core.c +5 -38

+
 enum {
 	/* Temperature calib indexes */
 	BMP380_T1 = 0,

@@ -344,6 +346,7 @@ static int bme280_read_calib(struct bmp280_data *data)
 {
 	struct bmp280_calib *calib = &data->calib.bmp280;
 	struct device *dev = data->dev;
+	s16 h4_upper, h4_lower;
 	unsigned int tmp;
 	int ret;
 

@@ -352,14 +355,6 @@ static int bme280_read_calib(struct bmp280_data *data)
 	if (ret)
 		return ret;
 
-	/*
-	 * Read humidity calibration values.
-	 * Due to some odd register addressing we cannot just
-	 * do a big bulk read. Instead, we have to read each Hx
-	 * value separately and sometimes do some bit shifting...
-	 * Humidity data is only available on BME280.
-	 */
-
 	ret = regmap_read(data->regmap, BME280_REG_COMP_H1, &tmp);
 	if (ret) {
 		dev_err(dev, "failed to read H1 comp value\n");

@@ -368,43 +363,23 @@ static int bme280_read_calib(struct bmp280_data *data)
 	calib->H1 = tmp;
 
 	ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H2,
-			       &data->le16, sizeof(data->le16));
-	if (ret) {
-		dev_err(dev, "failed to read H2 comp value\n");
-		return ret;
-	}
-	calib->H2 = sign_extend32(le16_to_cpu(data->le16), 15);
-
-	ret = regmap_read(data->regmap, BME280_REG_COMP_H3, &tmp);
-	if (ret) {
-		dev_err(dev, "failed to read H3 comp value\n");
-		return ret;
-	}
-	calib->H3 = tmp;
-
-	ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H4,
-			       &data->be16, sizeof(data->be16));
-	if (ret) {
-		dev_err(dev, "failed to read H4 comp value\n");
-		return ret;
-	}
-	calib->H4 = sign_extend32(((be16_to_cpu(data->be16) >> 4) & 0xff0) |
-				  (be16_to_cpu(data->be16) & 0xf), 11);
-
-	ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H5,
-			       &data->le16, sizeof(data->le16));
+			       data->bme280_humid_cal_buf,
+			       sizeof(data->bme280_humid_cal_buf));
 	if (ret) {
-		dev_err(dev, "failed to read H5 comp value\n");
+		dev_err(dev, "failed to read humidity calibration values\n");
 		return ret;
 	}
-	calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, le16_to_cpu(data->le16)), 11);
 
-	ret = regmap_read(data->regmap, BME280_REG_COMP_H6, &tmp);
-	if (ret) {
-		dev_err(dev, "failed to read H6 comp value\n");

drivers/iio/pressure/bmp280-core.c +3 -0

+	calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK,
+				  get_unaligned_le16(&data->bme280_humid_cal_buf[H5])), 11);
+	calib->H6 = data->bme280_humid_cal_buf[H6];
 
 	return 0;
 }

drivers/iio/pressure/bmp280.h +5 -0

index ccacc67c1473..56c01f224728 100644
@@ -257,8 +257,12 @@
 #define BME280_REG_COMP_H5		0xE5
 #define BME280_REG_COMP_H6		0xE7
 
+#define BME280_COMP_H4_MASK_UP		GENMASK(15, 4)
+#define BME280_COMP_H4_MASK_LOW		GENMASK(3, 0)
 #define BME280_COMP_H5_MASK		GENMASK(15, 4)
 
+#define BME280_CONTIGUOUS_CALIB_REGS	7
+
 #define BME280_OSRS_HUMIDITY_MASK	GENMASK(2, 0)
 #define BME280_OSRS_HUMIDITY_SKIP	0
 #define BME280_OSRS_HUMIDITY_1X		1

@@ -423,6 +427,7 @@ struct bmp280_data {
 		/* Calibration data buffers */
 		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
 		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
+		u8 bme280_humid_cal_buf[BME280_CONTIGUOUS_CALIB_REGS];
 		u8 bmp380_cal_buf[BMP380_CALIB_REG_COUNT];
 		/* Miscellaneous, endianness-aware data buffers */
 		__le16 le16;

Quoted 16 lines · On Thu, 11 Jul 2024 23:15:52 +0200 Vasileios Amoiridis…

Convert individual reads to a bulk read for the humidity calibration data.

Signed-off-by Vasileios Amoiridis <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 57 +++++++++---------------------
 drivers/iio/pressure/bmp280.h      |  5 +++
 2 files changed, 21 insertions(+), 41 deletions(-)

drivers/iio/pressure/bmp280-core.c +0 -0

index 3deaa57bb3f5..9c32266403bd 100644
@@ -118,6 +118,8 @@ enum bmp580_odr {
  */
 enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 

drivers/iio/pressure/bmp280-core.c enum bmp580_odr { · line 121

Quoted 1 line

drivers/iio/pressure/bmp280-core.c +1 -0

+enum { H2 = 0, H3 = 2, H4 = 3, H5 = 4, H6 = 6 };

Maybe add a comment to this that these are the locations where the field 'starts' and that some overlap such as H5 and H6.

Quoted 68 lines

drivers/iio/pressure/bmp280-core.c +5 -38

+
 enum {
 	/* Temperature calib indexes */
 	BMP380_T1 = 0,

@@ -344,6 +346,7 @@ static int bme280_read_calib(struct bmp280_data *data)
 {
 	struct bmp280_calib *calib = &data->calib.bmp280;
 	struct device *dev = data->dev;
+	s16 h4_upper, h4_lower;
 	unsigned int tmp;
 	int ret;
 

@@ -352,14 +355,6 @@ static int bme280_read_calib(struct bmp280_data *data)
 	if (ret)
 		return ret;
 
-	/*
-	 * Read humidity calibration values.
-	 * Due to some odd register addressing we cannot just
-	 * do a big bulk read. Instead, we have to read each Hx
-	 * value separately and sometimes do some bit shifting...
-	 * Humidity data is only available on BME280.
-	 */
-
 	ret = regmap_read(data->regmap, BME280_REG_COMP_H1, &tmp);
 	if (ret) {
 		dev_err(dev, "failed to read H1 comp value\n");

@@ -368,43 +363,23 @@ static int bme280_read_calib(struct bmp280_data *data)
 	calib->H1 = tmp;
 
 	ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H2,
-			       &data->le16, sizeof(data->le16));
-	if (ret) {
-		dev_err(dev, "failed to read H2 comp value\n");
-		return ret;
-	}
-	calib->H2 = sign_extend32(le16_to_cpu(data->le16), 15);
-
-	ret = regmap_read(data->regmap, BME280_REG_COMP_H3, &tmp);
-	if (ret) {
-		dev_err(dev, "failed to read H3 comp value\n");
-		return ret;
-	}
-	calib->H3 = tmp;
-
-	ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H4,
-			       &data->be16, sizeof(data->be16));
-	if (ret) {
-		dev_err(dev, "failed to read H4 comp value\n");
-		return ret;
-	}
-	calib->H4 = sign_extend32(((be16_to_cpu(data->be16) >> 4) & 0xff0) |
-				  (be16_to_cpu(data->be16) & 0xf), 11);
-
-	ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H5,
-			       &data->le16, sizeof(data->le16));
+			       data->bme280_humid_cal_buf,
+			       sizeof(data->bme280_humid_cal_buf));
 	if (ret) {
-		dev_err(dev, "failed to read H5 comp value\n");
+		dev_err(dev, "failed to read humidity calibration values\n");
 		return ret;
 	}
-	calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, le16_to_cpu(data->le16)), 11);
 
-	ret = regmap_read(data->regmap, BME280_REG_COMP_H6, &tmp);
-	if (ret) {
-		dev_err(dev, "failed to read H6 comp value\n");

drivers/iio/pressure/bmp280-core.c static int bme280_read_calib(struct bmp280_data *data) · line 379

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +7 -3

-		return ret;
-	}
-	calib->H6 = sign_extend32(tmp, 7);
+	calib->H2 = get_unaligned_le16(&data->bme280_humid_cal_buf[H2]);
+	calib->H3 = data->bme280_humid_cal_buf[H3];
+	h4_upper = FIELD_GET(BME280_COMP_H4_MASK_UP,
+			     get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
+	h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW,
+			     get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
+	calib->H4 = sign_extend32((h4_upper & ~BME280_COMP_H4_MASK_LOW) | h4_lower, 11);

This looks unusual. Why mask with MASK_LOW? The field_get above already drops the bottom 4 bits an this is dropping more. Should that H4_MASK_UP actually be GENMASK(15, 8) and then you shift it left 4 to make space for the lower part?

Original code was messing with values inline so there is less need for it to be explicit in how it does the masks. Here you imply a 12 bit field but only use 8 bits of it which isn't good.

Quoted 31 lines

drivers/iio/pressure/bmp280-core.c +3 -0

+	calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK,
+				  get_unaligned_le16(&data->bme280_humid_cal_buf[H5])), 11);
+	calib->H6 = data->bme280_humid_cal_buf[H6];
 
 	return 0;
 }

drivers/iio/pressure/bmp280.h +5 -0

index ccacc67c1473..56c01f224728 100644
@@ -257,8 +257,12 @@
 #define BME280_REG_COMP_H5		0xE5
 #define BME280_REG_COMP_H6		0xE7
 
+#define BME280_COMP_H4_MASK_UP		GENMASK(15, 4)
+#define BME280_COMP_H4_MASK_LOW		GENMASK(3, 0)
 #define BME280_COMP_H5_MASK		GENMASK(15, 4)
 
+#define BME280_CONTIGUOUS_CALIB_REGS	7
+
 #define BME280_OSRS_HUMIDITY_MASK	GENMASK(2, 0)
 #define BME280_OSRS_HUMIDITY_SKIP	0
 #define BME280_OSRS_HUMIDITY_1X		1

@@ -423,6 +427,7 @@ struct bmp280_data {
 		/* Calibration data buffers */
 		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
 		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
+		u8 bme280_humid_cal_buf[BME280_CONTIGUOUS_CALIB_REGS];
 		u8 bmp380_cal_buf[BMP380_CALIB_REG_COUNT];
 		/* Miscellaneous, endianness-aware data buffers */
 		__le16 le16;

This commit adds forced mode support in sensors BMP28x, BME28x, BMP3xx and BMP58x. Sensors BMP18x and BMP085 are old and do not support this feature so their operation is not affected at all.

Essentially, up to now, the rest of the sensors were used in normal mode all the time. This means that they are continuously doing measurements even though these measurements are not used. Even though the sensor does provide PM support, to cover all the possible use cases, the sensor needs to go into sleep mode and wake up whenever necessary.

This commit, adds sleep and forced mode support. Essentially, the sensor sleeps all the time except for when a measurement is requested. When there is a request for a measurement, the sensor is put into forced mode, starts the measurement and after it is done we read the output and we put it again in sleep mode.

For really fast and more deterministic measurements, the triggered buffer interface can be used, since the sensor is still used in normal mode for that use case.

This commit does not add though support for DEEP STANDBY, Low Power NORMAL and CONTINUOUS modes, supported only by the BMP58x version.

Signed-off-by Vasileios Amoiridis <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 276 +++++++++++++++++++++++++++--
 drivers/iio/pressure/bmp280.h      |  12 ++
 2 files changed, 269 insertions(+), 19 deletions(-)

drivers/iio/pressure/bmp280-core.c +0 -0

index 9c99373d66ec..fc8d42880eb8 100644
@@ -145,6 +145,12 @@ enum bmp280_scan {
 	BME280_HUMID,
 };

drivers/iio/pressure/bmp280-core.c +21 -0

+		break;
+	case BMP280_FORCED:
+		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
+					BMP280_MODE_MASK, BMP280_MODE_FORCED);
+		break;
+	case BMP280_NORMAL:
+		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
+					BMP280_MODE_MASK, BMP280_MODE_NORMAL);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(data->dev, "failed to  write ctrl_meas register\n");
+		return ret;
+	}
+
+	return 0;
+}
+

drivers/iio/pressure/bmp280-core.c +28 -1

+
+	/* Pressure measurement time */
+	meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +
+		      BMP280_PRESS_HUMID_MEAS_OFFSET;
+
+	/* Temperature measurement time */
+	meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;
+
+	usleep_range(meas_time, meas_time * 12 / 10);
+
+	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
+	if (ret) {
+		dev_err(data->dev, "failed to read status register\n");
+		return ret;
+	}
+	if (reg & BMP280_REG_STATUS_MEAS_BIT) {
+		dev_err(data->dev, "Measurement cycle didn't complete\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |

@@ -994,7 +1078,7 @@ static int bmp280_chip_config(struct bmp280_data *data)
 				BMP280_OSRS_TEMP_MASK |
 				BMP280_OSRS_PRESS_MASK |
 				BMP280_MODE_MASK,
-				osrs | BMP280_MODE_NORMAL);
+				osrs | BMP280_MODE_SLEEP);
 	if (ret) {
 		dev_err(data->dev, "failed to write ctrl_meas register\n");
 		return ret;

@@ -1100,6 +1184,8 @@ const struct bmp280_chip_info bmp280_chip_info = {
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
 	.read_calib = bmp280_read_calib,
+	.set_mode = bmp280_set_mode,
+	.wait_conv = bmp280_wait_conv,
 	.preinit = bmp280_preinit,
 
 	.trigger_handler = bmp280_trigger_handler,

@@ -1218,6 +1304,8 @@ const struct bmp280_chip_info bme280_chip_info = {
 	.read_press = bmp280_read_press,
 	.read_humid = bme280_read_humid,
 	.read_calib = bme280_read_calib,
+	.set_mode = bmp280_set_mode,
+	.wait_conv = bmp280_wait_conv,
 	.preinit = bmp280_preinit,
 
 	.trigger_handler = bme280_trigger_handler,

@@ -1505,6 +1593,75 @@ static int bmp380_preinit(struct bmp280_data *data)
 	return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
 }
 

drivers/iio/pressure/bmp280-core.c +42 -0

+		break;
+	case BMP280_FORCED:
+		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+					BMP380_MODE_MASK,
+					FIELD_PREP(BMP380_MODE_MASK,
+						   BMP380_MODE_FORCED));
+		break;
+	case BMP280_NORMAL:
+		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+					BMP380_MODE_MASK,
+					FIELD_PREP(BMP380_MODE_MASK,
+						   BMP380_MODE_NORMAL));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(data->dev, "failed to  write power control register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmp380_wait_conv(struct bmp280_data *data)
+{
+	unsigned int reg;
+	int ret, meas_time;
+
+	/* Offset measurement time */
+	meas_time = BMP380_MEAS_OFFSET;
+
+	/* Pressure measurement time */
+	meas_time += (1 << data->oversampling_press) * BMP380_MEAS_DUR +
+		      BMP380_PRESS_MEAS_OFFSET;
+
+	/* Temperature measurement time */
+	meas_time += (1 << data->oversampling_temp) * BMP380_MEAS_DUR +
+		      BMP380_TEMP_MEAS_OFFSET;
+
+	usleep_range(meas_time, meas_time * 12 / 10);

drivers/iio/pressure/bmp280-core.c +30 -7

+		dev_err(data->dev, "Measurement cycle didn't complete\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 static int bmp380_chip_config(struct bmp280_data *data)
 {
 	bool change = false, aux;

@@ -1565,17 +1722,15 @@ static int bmp380_chip_config(struct bmp280_data *data)
 		 * Resets sensor measurement loop toggling between sleep and
 		 * normal operating modes.
 		 */
-		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
-					BMP380_MODE_MASK,
-					FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_SLEEP));
+		ret = bmp380_set_mode(data, BMP280_SLEEP);
 		if (ret) {
 			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));
+
+		usleep_range(data->start_up_time, data->start_up_time + 500);
+
+		ret = bmp380_set_mode(data, BMP280_NORMAL);
 		if (ret) {
 			dev_err(data->dev, "failed to set normal mode\n");
 			return ret;

@@ -1601,6 +1756,17 @@ static int bmp380_chip_config(struct bmp280_data *data)
 		}
 	}
 
+	/* Dummy read to empty data registers. */
+	ret = bmp380_read_press(data, &tmp);
+	if (ret)
+		return ret;
+
+	ret = bmp380_set_mode(data, BMP280_SLEEP);
+	if (ret) {
+		dev_err(data->dev, "failed to set sleep mode\n");
+		return ret;
+	}
+
 	return 0;
 }
 

@@ -1693,6 +1859,8 @@ const struct bmp280_chip_info bmp380_chip_info = {
 	.read_temp = bmp380_read_temp,
 	.read_press = bmp380_read_press,
 	.read_calib = bmp380_read_calib,
+	.set_mode = bmp380_set_mode,
+	.wait_conv = bmp380_wait_conv,
 	.preinit = bmp380_preinit,
 
 	.trigger_handler = bmp380_trigger_handler,

@@ -2080,6 +2248,65 @@ static int bmp580_preinit(struct bmp280_data *data)
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config));
 }
 
+static int bmp580_set_mode(struct bmp280_data *data, u8 mode)
+{
+	int ret;
+
+	switch (mode) {

drivers/iio/pressure/bmp280-core.c +29 -0

+		break;
+	case BMP280_NORMAL:
+		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+					BMP580_MODE_MASK,
+					FIELD_PREP(BMP580_MODE_MASK,
+						   BMP580_MODE_NORMAL));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(data->dev, "failed to  write power control register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmp580_wait_conv(struct bmp280_data *data)
+{
+	/*
+	 * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
+	 * characteristics
+	 */
+	const int time_conv_press[] = { 0, 1050, 1785, 3045, 5670, 10920, 21420,
+					42420, 84420};
+	const int time_conv_temp[] = { 0, 1050, 1105, 1575, 2205, 3465, 6090,
+				       11340, 21840};

drivers/iio/pressure/bmp280-core.c +3 -11

+
 static int bmp580_chip_config(struct bmp280_data *data)
 {
 	bool change = false, aux;

@@ -2150,17 +2377,6 @@ static int bmp580_chip_config(struct bmp280_data *data)
 		return ret;
 	}
 
-	/* Restore sensor to normal operation mode */
-	ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
-				BMP580_MODE_MASK,
-				FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_NORMAL));
-	if (ret) {
-		dev_err(data->dev, "failed to set normal mode\n");
-		return ret;
-	}
-	/* From datasheet's table 4: electrical characteristics */
-	usleep_range(3000, 3500);
-
 	if (change) {
 		/*
 		 * Check if ODR and OSR settings are valid or we are

@@ -2256,6 +2472,8 @@ const struct bmp280_chip_info bmp580_chip_info = {
 	.chip_config = bmp580_chip_config,
 	.read_temp = bmp580_read_temp,
 	.read_press = bmp580_read_press,
+	.set_mode = bmp580_set_mode,
+	.wait_conv = bmp580_wait_conv,
 	.preinit = bmp580_preinit,
 
 	.trigger_handler = bmp580_trigger_handler,

drivers/iio/pressure/bmp280-core.c +7 -0

+}
+
+static int bmp180_wait_conv(struct bmp280_data *data)
+{
+	return 0;
+}
+
 

Quoted 23 lines · On Thu, 11 Jul 2024 23:15:55 +0200 Vasileios Amoiridis…

This commit adds forced mode support in sensors BMP28x, BME28x, BMP3xx and BMP58x. Sensors BMP18x and BMP085 are old and do not support this feature so their operation is not affected at all.

Essentially, up to now, the rest of the sensors were used in normal mode all the time. This means that they are continuously doing measurements even though these measurements are not used. Even though the sensor does provide PM support, to cover all the possible use cases, the sensor needs to go into sleep mode and wake up whenever necessary.

This commit, adds sleep and forced mode support. Essentially, the sensor sleeps all the time except for when a measurement is requested. When there is a request for a measurement, the sensor is put into forced mode, starts the measurement and after it is done we read the output and we put it again in sleep mode.

For really fast and more deterministic measurements, the triggered buffer interface can be used, since the sensor is still used in normal mode for that use case.

This commit does not add though support for DEEP STANDBY, Low Power NORMAL and CONTINUOUS modes, supported only by the BMP58x version.

Quoted 1 line

Signed-off-by Vasileios Amoiridis <[email protected]>

Various minor comments inline.

Thanks,

Jonathan

Quoted 12 lines

---
 drivers/iio/pressure/bmp280-core.c | 276 +++++++++++++++++++++++++++--
 drivers/iio/pressure/bmp280.h      |  12 ++
 2 files changed, 269 insertions(+), 19 deletions(-)

drivers/iio/pressure/bmp280-core.c +0 -0

index 9c99373d66ec..fc8d42880eb8 100644
@@ -145,6 +145,12 @@ enum bmp280_scan {
 	BME280_HUMID,
 };

drivers/iio/pressure/bmp280-core.c enum bmp280_scan { · line 156

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +8 -0

 }
 
+static int bmp280_set_mode(struct bmp280_data *data, u8 mode)
+{
+	int ret;
+
+	switch (mode) {
+	case BMP280_SLEEP:
+		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
+					BMP280_MODE_MASK, BMP280_MODE_SLEEP);

Use a local variable for the BMP280_MODE_* and then have the regmap_write_bits() after the switch statement.

Could even make it a const data look up given you are getting a value based on the enum.

Quoted 26 lines

drivers/iio/pressure/bmp280-core.c +26 -0

+		break;
+	case BMP280_FORCED:
+		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
+					BMP280_MODE_MASK, BMP280_MODE_FORCED);
+		break;
+	case BMP280_NORMAL:
+		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
+					BMP280_MODE_MASK, BMP280_MODE_NORMAL);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(data->dev, "failed to  write ctrl_meas register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmp280_wait_conv(struct bmp280_data *data)
+{
+	unsigned int reg;
+	int ret, meas_time;
+

drivers/iio/pressure/bmp280-core.c static int bmp280_preinit(struct bmp280_data *data) · line 1047

Quoted 5 lines

drivers/iio/pressure/bmp280-core.c +5 -0

+	meas_time = BMP280_MEAS_OFFSET;
+
+	if (data->oversampling_humid)
+		meas_time += (1 << data->oversampling_humid) * BMP280_MEAS_DUR +
+			       BMP280_PRESS_HUMID_MEAS_OFFSET;

Add a comment on why, if oversampling_humid is not set we end up with no time for measuring humidity. The MEAS_OFFSET is less than one MEAS_DUR so not it's not a case of that already incorporating the time.

Quoted 58 lines

drivers/iio/pressure/bmp280-core.c +28 -1

+
+	/* Pressure measurement time */
+	meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +
+		      BMP280_PRESS_HUMID_MEAS_OFFSET;
+
+	/* Temperature measurement time */
+	meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;
+
+	usleep_range(meas_time, meas_time * 12 / 10);
+
+	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
+	if (ret) {
+		dev_err(data->dev, "failed to read status register\n");
+		return ret;
+	}
+	if (reg & BMP280_REG_STATUS_MEAS_BIT) {
+		dev_err(data->dev, "Measurement cycle didn't complete\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |

@@ -994,7 +1078,7 @@ static int bmp280_chip_config(struct bmp280_data *data)
 				BMP280_OSRS_TEMP_MASK |
 				BMP280_OSRS_PRESS_MASK |
 				BMP280_MODE_MASK,
-				osrs | BMP280_MODE_NORMAL);
+				osrs | BMP280_MODE_SLEEP);
 	if (ret) {
 		dev_err(data->dev, "failed to write ctrl_meas register\n");
 		return ret;

@@ -1100,6 +1184,8 @@ const struct bmp280_chip_info bmp280_chip_info = {
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
 	.read_calib = bmp280_read_calib,
+	.set_mode = bmp280_set_mode,
+	.wait_conv = bmp280_wait_conv,
 	.preinit = bmp280_preinit,
 
 	.trigger_handler = bmp280_trigger_handler,

@@ -1218,6 +1304,8 @@ const struct bmp280_chip_info bme280_chip_info = {
 	.read_press = bmp280_read_press,
 	.read_humid = bme280_read_humid,
 	.read_calib = bme280_read_calib,
+	.set_mode = bmp280_set_mode,
+	.wait_conv = bmp280_wait_conv,
 	.preinit = bmp280_preinit,
 
 	.trigger_handler = bme280_trigger_handler,

@@ -1505,6 +1593,75 @@ static int bmp380_preinit(struct bmp280_data *data)
 	return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
 }
 

drivers/iio/pressure/bmp280-core.c static int bmp380_preinit(struct bmp280_data *data) · line 1605

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +10 -0

+static int bmp380_set_mode(struct bmp280_data *data, u8 mode)
+{
+	int ret;
+
+	switch (mode) {
+	case BMP280_SLEEP:
+		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+					BMP380_MODE_MASK,
+					FIELD_PREP(BMP380_MODE_MASK,
+						   BMP380_MODE_SLEEP));

As above. I'd use a local variable to stash the MODE* that you are going to write or just look it up in a const array.

Quoted 46 lines

drivers/iio/pressure/bmp280-core.c +46 -0

+		break;
+	case BMP280_FORCED:
+		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+					BMP380_MODE_MASK,
+					FIELD_PREP(BMP380_MODE_MASK,
+						   BMP380_MODE_FORCED));
+		break;
+	case BMP280_NORMAL:
+		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+					BMP380_MODE_MASK,
+					FIELD_PREP(BMP380_MODE_MASK,
+						   BMP380_MODE_NORMAL));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(data->dev, "failed to  write power control register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmp380_wait_conv(struct bmp280_data *data)
+{
+	unsigned int reg;
+	int ret, meas_time;
+
+	/* Offset measurement time */
+	meas_time = BMP380_MEAS_OFFSET;
+
+	/* Pressure measurement time */
+	meas_time += (1 << data->oversampling_press) * BMP380_MEAS_DUR +
+		      BMP380_PRESS_MEAS_OFFSET;
+
+	/* Temperature measurement time */
+	meas_time += (1 << data->oversampling_temp) * BMP380_MEAS_DUR +
+		      BMP380_TEMP_MEAS_OFFSET;
+
+	usleep_range(meas_time, meas_time * 12 / 10);
+
+	ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
+	if (ret) {
+		dev_err(data->dev, "failed to read status register\n");

drivers/iio/pressure/bmp280-core.c static int bmp380_preinit(struct bmp280_data *data) · line 1657

Quoted 6 lines

drivers/iio/pressure/bmp280-core.c +6 -0

+		return ret;
+	}
+
+	if (!(reg & BMP380_STATUS_DRDY_PRESS_MASK) ||
+	    !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
+		pr_info("Meas_time: %d\n", meas_time);

Why as pr_info? Seems like part of the dev_err.

Quoted 64 lines

drivers/iio/pressure/bmp280-core.c +25 -7

+		dev_err(data->dev, "Measurement cycle didn't complete\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 static int bmp380_chip_config(struct bmp280_data *data)
 {
 	bool change = false, aux;

@@ -1565,17 +1722,15 @@ static int bmp380_chip_config(struct bmp280_data *data)
 		 * Resets sensor measurement loop toggling between sleep and
 		 * normal operating modes.
 		 */
-		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
-					BMP380_MODE_MASK,
-					FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_SLEEP));
+		ret = bmp380_set_mode(data, BMP280_SLEEP);
 		if (ret) {
 			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));
+
+		usleep_range(data->start_up_time, data->start_up_time + 500);
+
+		ret = bmp380_set_mode(data, BMP280_NORMAL);
 		if (ret) {
 			dev_err(data->dev, "failed to set normal mode\n");
 			return ret;

@@ -1601,6 +1756,17 @@ static int bmp380_chip_config(struct bmp280_data *data)
 		}
 	}
 
+	/* Dummy read to empty data registers. */
+	ret = bmp380_read_press(data, &tmp);
+	if (ret)
+		return ret;
+
+	ret = bmp380_set_mode(data, BMP280_SLEEP);
+	if (ret) {
+		dev_err(data->dev, "failed to set sleep mode\n");
+		return ret;
+	}
+
 	return 0;
 }
 

@@ -1693,6 +1859,8 @@ const struct bmp280_chip_info bmp380_chip_info = {
 	.read_temp = bmp380_read_temp,
 	.read_press = bmp380_read_press,
 	.read_calib = bmp380_read_calib,
+	.set_mode = bmp380_set_mode,
+	.wait_conv = bmp380_wait_conv,
 	.preinit = bmp380_preinit,
 
 	.trigger_handler = bmp380_trigger_handler,

@@ -2080,6 +2248,65 @@ static int bmp580_preinit(struct bmp280_data *data)
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config));
 }
 

Quoted 5 lines

drivers/iio/pressure/bmp280-core.c +5 -0

+static int bmp580_set_mode(struct bmp280_data *data, u8 mode)
+{
+	int ret;
+
+	switch (mode) {

drivers/iio/pressure/bmp280-core.c static int bmp580_preinit(struct bmp280_data *data) · line 2265

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +10 -0

+	case BMP280_SLEEP:
+		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+					BMP580_MODE_MASK,
+					FIELD_PREP(BMP580_MODE_MASK,
+						   BMP580_MODE_SLEEP));
+		break;
+	case BMP280_FORCED:
+		ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
+				      BMP580_DSP_IIR_FORCED_FLUSH);
+

check that ret.

drivers/iio/pressure/bmp280-core.c static int bmp580_preinit(struct bmp280_data *data) · line 2269

Quoted 4 lines

drivers/iio/pressure/bmp280-core.c +4 -0

+		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+					BMP580_MODE_MASK,
+					FIELD_PREP(BMP580_MODE_MASK,
+						   BMP580_MODE_FORCED));

This one is more complex so a switch statement makes sense here.

Quoted 32 lines

drivers/iio/pressure/bmp280-core.c +32 -0

+		break;
+	case BMP280_NORMAL:
+		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+					BMP580_MODE_MASK,
+					FIELD_PREP(BMP580_MODE_MASK,
+						   BMP580_MODE_NORMAL));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(data->dev, "failed to  write power control register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmp580_wait_conv(struct bmp280_data *data)
+{
+	/*
+	 * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
+	 * characteristics
+	 */
+	const int time_conv_press[] = { 0, 1050, 1785, 3045, 5670, 10920, 21420,
+					42420, 84420};
+	const int time_conv_temp[] = { 0, 1050, 1105, 1575, 2205, 3465, 6090,
+				       11340, 21840};
+	int meas_time;
+
+	meas_time = 4000 + time_conv_temp[data->oversampling_temp] +

drivers/iio/pressure/bmp280-core.c static int bmp580_preinit(struct bmp280_data *data) · line 2308

Quoted 7 lines

drivers/iio/pressure/bmp280-core.c +7 -0

+			   time_conv_press[data->oversampling_press];
+
+	usleep_range(meas_time, meas_time * 12 / 10);
+
+	return 0;
+}
+

one blank line only.

Quoted 35 lines

drivers/iio/pressure/bmp280-core.c +3 -11

+
 static int bmp580_chip_config(struct bmp280_data *data)
 {
 	bool change = false, aux;

@@ -2150,17 +2377,6 @@ static int bmp580_chip_config(struct bmp280_data *data)
 		return ret;
 	}
 
-	/* Restore sensor to normal operation mode */
-	ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
-				BMP580_MODE_MASK,
-				FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_NORMAL));
-	if (ret) {
-		dev_err(data->dev, "failed to set normal mode\n");
-		return ret;
-	}
-	/* From datasheet's table 4: electrical characteristics */
-	usleep_range(3000, 3500);
-
 	if (change) {
 		/*
 		 * Check if ODR and OSR settings are valid or we are

@@ -2256,6 +2472,8 @@ const struct bmp280_chip_info bmp580_chip_info = {
 	.chip_config = bmp580_chip_config,
 	.read_temp = bmp580_read_temp,
 	.read_press = bmp580_read_press,
+	.set_mode = bmp580_set_mode,
+	.wait_conv = bmp580_wait_conv,
 	.preinit = bmp580_preinit,
 
 	.trigger_handler = bmp580_trigger_handler,

@@ -2503,6 +2721,16 @@ static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press)
 	return 0;
 }
 

drivers/iio/pressure/bmp280-core.c static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press) · line 2726

Quoted 3 lines

drivers/iio/pressure/bmp280-core.c +3 -0

+static int bmp180_set_mode(struct bmp280_data *data, u8 mode)
+{
+	return 0;

Add a comment on why these are stubs. It's in the patch description, but better to have it recorded in the code.

Quoted 8 lines

drivers/iio/pressure/bmp280-core.c +7 -0

+}
+
+static int bmp180_wait_conv(struct bmp280_data *data)
+{
+	return 0;
+}
+
 

Quoted 7 lines

Quoted 7 lines

drivers/iio/pressure/bmp280-core.c +0 -0

 	.trigger_handler = bmp580_trigger_handler,

@@ -2503,6 +2721,16 @@ static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press)
 	return 0;
 }
 

Quoted 6 lines

drivers/iio/pressure/bmp280-core.c static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press) · line 2726

Quoted 3 lines

drivers/iio/pressure/bmp280-core.c +3 -0

+static int bmp180_set_mode(struct bmp280_data *data, u8 mode)
+{
+	return 0;  

Add a comment on why these are stubs. It's in the patch description, but better to have it recorded in the code.

I didn't add because as you can see below there is another function exactly like this one that doesn't have one. Should I add also to the other one?

Quoted 9 lines

Quoted 9 lines

drivers/iio/pressure/bmp280-core.c +7 -0

+}
+
+static int bmp180_wait_conv(struct bmp280_data *data)
+{
+	return 0;
+}
+
 

Add interrupt options for BMP3xx and BMP5xx devices as well.

Signed-off-by Vasileios Amoiridis <[email protected]>

---
 .../devicetree/bindings/iio/pressure/bmp085.yaml    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Add interrupt options for BMP3xx and BMP5xx devices as well.

Signed-off-by Vasileios Amoiridis <[email protected]>

---
 .../devicetree/bindings/iio/pressure/bmp085.yaml    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/iio/pressure/bmp085.yaml +0 -1

index 6fda887ee9d4..f06f119963bc 100644
@@ -48,9 +48,20 @@ properties:
 
   interrupts:
     description:
-      interrupt mapping for IRQ (BMP085 only)

Documentation/devicetree/bindings/iio/pressure/bmp085.yaml +4 -0

+    desription:
+      set if the interrupt pin should be configured as open drain.
+      If not set, defaults to push-pull configuration.
+
 required:
   - compatible
   - vddd-supply

Signature · 1 line

2.25.1

Quoted 19 lines · Vasileios Amoiridis

Add interrupt options for BMP3xx and BMP5xx devices as well.

Signed-off-by Vasileios Amoiridis <[email protected]>

---
 .../devicetree/bindings/iio/pressure/bmp085.yaml    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/iio/pressure/bmp085.yaml +1 -1

index 6fda887ee9d4..f06f119963bc 100644
@@ -48,9 +48,20 @@ properties:
 
   interrupts:
     description:
-      interrupt mapping for IRQ (BMP085 only)
+      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
     maxItems: 1
 

Documentation/devicetree/bindings/iio/pressure/bmp085.yaml properties: · line 60

Quoted 7 lines

Documentation/devicetree/bindings/iio/pressure/bmp085.yaml +7 -0

+  interrupt-names:
+    maxItems: 1
+    items:
+      enum:
+        - DRDY
+
+  int-open-drain:

Use the existing 'drive-open-drain' property.

Quoted 10 lines

Documentation/devicetree/bindings/iio/pressure/bmp085.yaml +4 -0

+    desription:
+      set if the interrupt pin should be configured as open drain.
+      If not set, defaults to push-pull configuration.
+
 required:
   - compatible
   - vddd-supply

Signature · 1 line

2.25.1

The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as a trigger for when there are data ready in the sensor for pick up.

This use case is used along with NORMAL_MODE in the sensor, which allows the sensor to do consecutive measurements depending on the ODR rate value.

The trigger pin can be configured to be open-drain or push-pull and either rising or falling edge.

No support is added yet for interrupts for FIFO, WATERMARK and out of range values.

Signed-off-by Vasileios Amoiridis <[email protected]>

drivers/iio/pressure/bmp280-core.c +6 -0

+
+static int bmp380_trigger_probe(struct iio_dev *indio_dev)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;
+	int ret, irq, irq_type;

drivers/iio/pressure/bmp280-core.c +45 -0

+	}
+
+	desc = irq_get_irq_data(irq);
+	if (!desc)
+		return -EINVAL;
+
+	irq_type = irqd_get_trigger_type(desc);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		data->trig_active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		data->trig_active_high = false;
+		break;
+	default:
+		dev_err(data->dev, "Invalid interrupt type specified\n");
+		return -EINVAL;
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = bmp380_int_config(data);
+	if (ret)
+		return ret;
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
+					    indio_dev->name,
+					    iio_device_id(indio_dev));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bmp380_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_irq(data->dev, irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_ONESHOT, indio_dev->name, data->trig);
+	if (ret) {
+		dev_err(data->dev, "request irq failed\n");
+		return ret;
+	}
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret) {

static irqreturn_t bmp380_trigger_handler(int irq, void *p)

drivers/iio/pressure/bmp280-core.c +5 -0

+
+static int bmp580_trigger_probe(struct iio_dev *indio_dev)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;

drivers/iio/pressure/bmp280-core.c +54 -0

+		return -ENODEV;
+	}
+
+	desc = irq_get_irq_data(irq);
+	if (!desc)
+		return -EINVAL;
+
+	irq_type = irqd_get_trigger_type(desc);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		data->trig_active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		data->trig_active_high = false;
+		break;
+	default:
+		dev_err(data->dev, "Invalid interrupt type specified\n");
+		return -EINVAL;
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = bmp580_int_config(data);
+	if (ret)
+		return ret;
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
+					    indio_dev->name,
+					    iio_device_id(indio_dev));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bmp580_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_irq(data->dev, irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_ONESHOT, indio_dev->name, data->trig);
+	if (ret) {
+		dev_err(data->dev, "request irq failed\n");
+		return ret;
+	}
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret) {
+		dev_err(data->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}

Quoted 6 lines · On Thu, 11 Jul 2024 23:15:57 +0200 Vasileios Amoiridis…

The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as a trigger for when there are data ready in the sensor for pick up.

This use case is used along with NORMAL_MODE in the sensor, which allows the sensor to do consecutive measurements depending on the ODR rate value.

Quoted 7 lines

The trigger pin can be configured to be open-drain or push-pull and either rising or falling edge.

No support is added yet for interrupts for FIFO, WATERMARK and out of range values.

Signed-off-by Vasileios Amoiridis <[email protected]>

A few minor things inline.

It might be worth thinking a bit about future fifo support as that can get a little messy in a driver that already supports a dataready trigger. We end up with no trigger being set meaning use the fifo. Sometimes it makes more sense to not support triggers at all.

What you have here is fine though as we have a bunch of drivers that grew dataready trigger support before adding fifos later particularly as often it's a 'new chip' that brings the fifo support but maintains backwards compatibility if you don't use it.

Quoted 6 lines

drivers/iio/pressure/bmp280-core.c +6 -0

+
+static int bmp380_trigger_probe(struct iio_dev *indio_dev)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;
+	int ret, irq, irq_type;

drivers/iio/pressure/bmp280-core.c static int bmp380_chip_config(struct bmp280_data *data) · line 1842

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +10 -0

+	struct irq_data *desc;
+
+	fwnode = dev_fwnode(data->dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get_byname(fwnode, "DRDY");
+	if (!irq) {
+		dev_err(data->dev, "No DRDY interrupt found\n");
+		return -ENODEV;

As below. return dev_err_probe() for anything that is only called from probe()

Quoted 45 lines

drivers/iio/pressure/bmp280-core.c +45 -0

+	}
+
+	desc = irq_get_irq_data(irq);
+	if (!desc)
+		return -EINVAL;
+
+	irq_type = irqd_get_trigger_type(desc);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		data->trig_active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		data->trig_active_high = false;
+		break;
+	default:
+		dev_err(data->dev, "Invalid interrupt type specified\n");
+		return -EINVAL;
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = bmp380_int_config(data);
+	if (ret)
+		return ret;
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
+					    indio_dev->name,
+					    iio_device_id(indio_dev));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bmp380_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_irq(data->dev, irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_ONESHOT, indio_dev->name, data->trig);
+	if (ret) {
+		dev_err(data->dev, "request irq failed\n");
+		return ret;
+	}
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret) {

drivers/iio/pressure/bmp280-core.c static int bmp380_chip_config(struct bmp280_data *data) · line 1897

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +10 -0

+		dev_err(data->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}
+
+

One blank line only.

Quoted 8 lines

static irqreturn_t bmp380_trigger_handler(int irq, void *p)

drivers/iio/pressure/bmp280-core.c +5 -0

+
+static int bmp580_trigger_probe(struct iio_dev *indio_dev)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;

drivers/iio/pressure/bmp280-core.c static int bmp580_chip_config(struct bmp280_data *data) · line 2602

Quoted 10 lines

drivers/iio/pressure/bmp280-core.c +10 -0

+	int ret, irq, irq_type;
+	struct irq_data *desc;
+
+	fwnode = dev_fwnode(data->dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get_byname(fwnode, "DRDY");
+	if (!irq) {
+		dev_err(data->dev, "No DRDY interrupt found\n");

As this only gets called from probe(), use return dev_err_probe() throughout.

Quoted 56 lines

drivers/iio/pressure/bmp280-core.c +54 -0

+		return -ENODEV;
+	}
+
+	desc = irq_get_irq_data(irq);
+	if (!desc)
+		return -EINVAL;
+
+	irq_type = irqd_get_trigger_type(desc);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		data->trig_active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		data->trig_active_high = false;
+		break;
+	default:
+		dev_err(data->dev, "Invalid interrupt type specified\n");
+		return -EINVAL;
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = bmp580_int_config(data);
+	if (ret)
+		return ret;
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
+					    indio_dev->name,
+					    iio_device_id(indio_dev));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bmp580_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_irq(data->dev, irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_ONESHOT, indio_dev->name, data->trig);
+	if (ret) {
+		dev_err(data->dev, "request irq failed\n");
+		return ret;
+	}
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret) {
+		dev_err(data->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}

Quoted 24 lines · Jonathan Cameron

Quoted 6 lines · On Thu, 11 Jul 2024 23:15:57 +0200 Vasileios Amoiridis…

The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as a trigger for when there are data ready in the sensor for pick up.

This use case is used along with NORMAL_MODE in the sensor, which allows the sensor to do consecutive measurements depending on the ODR rate value.

Quoted 7 lines

The trigger pin can be configured to be open-drain or push-pull and either rising or falling edge.

No support is added yet for interrupts for FIFO, WATERMARK and out of range values.

Signed-off-by Vasileios Amoiridis <[email protected]>

A few minor things inline.

It might be worth thinking a bit about future fifo support as that can get a little messy in a driver that already supports a dataready trigger. We end up with no trigger being set meaning use the fifo. Sometimes it makes more sense to not support triggers at all.

Quoted 5 lines

What you have here is fine though as we have a bunch of drivers that grew dataready trigger support before adding fifos later particularly as often it's a 'new chip' that brings the fifo support but maintains backwards compatibility if you don't use it.

Hi Jonathan,

Thank you very much for your thorough review again!

What I could do to make the code even better to be able to accept FIFO irq support are the following:

1) in the bmp{380/580}_trigger_handler() currently, the data registers are being read. What I could do is to move the reading of registers to a separe function like bmpxxx_drdy_trigger_handler() and calling it inside the bmp{380/580}_trigger_handler() when I have DRDY or sysfs irq. In order to check the enabled irqs I propose also no.2

2) in the following bmp{380/580}_trigger_probe() functions instead of just doing:

       irq = fwnode_irq_get_byname(fwnode, "DRDY");
       if (!irq) {
               dev_err(data->dev, "No DRDY interrupt found\n");
               return -ENODEV;
       }

I could also use some type of variable like we do for the active channels in order to track "active/existing irqs".

Like this it would be easier to track the active irqs of the sensor.

Let me know what you think, or if I manage to have time I might send a v2 with those changes even earlier :)

Cheers,
Vasilis

Quoted 35 lines · On Mon, 22 Jul 2024 01:51:13 +0200 Vasileios Amoiridis…

Quoted 24 lines · Jonathan Cameron

Quoted 6 lines · On Thu, 11 Jul 2024 23:15:57 +0200 Vasileios Amoiridis…

The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as a trigger for when there are data ready in the sensor for pick up.

This use case is used along with NORMAL_MODE in the sensor, which allows the sensor to do consecutive measurements depending on the ODR rate value.

Quoted 7 lines

The trigger pin can be configured to be open-drain or push-pull and either rising or falling edge.

No support is added yet for interrupts for FIFO, WATERMARK and out of range values.

Signed-off-by Vasileios Amoiridis <[email protected]>

A few minor things inline.

It might be worth thinking a bit about future fifo support as that can get a little messy in a driver that already supports a dataready trigger. We end up with no trigger being set meaning use the fifo. Sometimes it makes more sense to not support triggers at all.

Quoted 5 lines

What you have here is fine though as we have a bunch of drivers that grew dataready trigger support before adding fifos later particularly as often it's a 'new chip' that brings the fifo support but maintains backwards compatibility if you don't use it.

Hi Jonathan,

Thank you very much for your thorough review again!

Quoted 8 lines

What I could do to make the code even better to be able to accept FIFO irq support are the following:

1) in the bmp{380/580}_trigger_handler() currently, the data registers are being read. What I could do is to move the reading of registers to a separe function like bmpxxx_drdy_trigger_handler() and calling it inside the bmp{380/580}_trigger_handler() when I have DRDY or sysfs irq. In order to check the enabled irqs I propose also no.2

You shouldn't get to the trigger_handler by other paths. But sure a bit of code reuse might make sense if fifo read out path is same as for other data reads. Superficially it looks totally different on the bmp380 though as there is a separate fifo register.

Quoted 4 lines

2) in the following bmp{380/580}_trigger_probe() functions instead of just doing:

Quoted 8 lines

       irq = fwnode_irq_get_byname(fwnode, "DRDY");
       if (!irq) {
               dev_err(data->dev, "No DRDY interrupt found\n");
               return -ENODEV;
       }

I could also use some type of variable like we do for the active channels in order to track "active/existing irqs".

I think there is only one IRQ on the 380 at least. So you should only request it once for this driver. Then software gets to configure what it is for.

However it shouldn't be called DRDY for these parts at least. It's just INT on the datasheet.
The interrupt control register value will tell you what is enabled. No need to track it separately.

If you mean track the one from the poll function registered for handling triggers - that's an internal detail but you would indeed need to track in your data structures whether that's the trigger currently being used or not (easy to do by comparing iio_dev->trig with a pointer for each trigger in iio_priv() data - so should be no need for separate tracking.

Jonathan

Quoted 10 lines

Like this it would be easier to track the active irqs of the sensor.

Let me know what you think, or if I manage to have time I might send a v2 with those changes even earlier :)

Cheers,
Vasilis

Quoted 47 lines · Jonathan Cameron

Quoted 35 lines · On Mon, 22 Jul 2024 01:51:13 +0200 Vasileios Amoiridis…

Quoted 24 lines · Jonathan Cameron

Quoted 6 lines · On Thu, 11 Jul 2024 23:15:57 +0200 Vasileios Amoiridis…

The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as a trigger for when there are data ready in the sensor for pick up.

This use case is used along with NORMAL_MODE in the sensor, which allows the sensor to do consecutive measurements depending on the ODR rate value.

Quoted 7 lines

The trigger pin can be configured to be open-drain or push-pull and either rising or falling edge.

No support is added yet for interrupts for FIFO, WATERMARK and out of range values.

Signed-off-by Vasileios Amoiridis <[email protected]>

A few minor things inline.

It might be worth thinking a bit about future fifo support as that can get a little messy in a driver that already supports a dataready trigger. We end up with no trigger being set meaning use the fifo. Sometimes it makes more sense to not support triggers at all.

Quoted 5 lines

What you have here is fine though as we have a bunch of drivers that grew dataready trigger support before adding fifos later particularly as often it's a 'new chip' that brings the fifo support but maintains backwards compatibility if you don't use it.

Hi Jonathan,

Thank you very much for your thorough review again!

Quoted 8 lines

What I could do to make the code even better to be able to accept FIFO irq support are the following:

1) in the bmp{380/580}_trigger_handler() currently, the data registers are being read. What I could do is to move the reading of registers to a separe function like bmpxxx_drdy_trigger_handler() and calling it inside the bmp{380/580}_trigger_handler() when I have DRDY or sysfs irq. In order to check the enabled irqs I propose also no.2

Quoted 5 lines

You shouldn't get to the trigger_handler by other paths. But sure a bit of code reuse might make sense if fifo read out path is same as for other data reads. Superficially it looks totally different on the bmp380 though as there is a separate fifo register.

So, I don't mean getting into the trigger_handler by other paths. I will always end up in the trigger_handler and then, depending on the interrupt that was triggered (DRDY, FIFO, etc...) I choose different actions.

Quoted 14 lines

Quoted 5 lines

2) in the following bmp{380/580}_trigger_probe() functions instead of just doing:

Quoted 8 lines

       irq = fwnode_irq_get_byname(fwnode, "DRDY");
       if (!irq) {
               dev_err(data->dev, "No DRDY interrupt found\n");
               return -ENODEV;
       }

I could also use some type of variable like we do for the active channels in order to track "active/existing irqs".

Quoted 9 lines

I think there is only one IRQ on the 380 at least. So you should only request it once for this driver. Then software gets to configure what it is for.

However it shouldn't be called DRDY for these parts at least. It's just INT on the datasheet.
The interrupt control register value will tell you what is enabled. No need to track it separately.

So I am a bit confused. Indeed, BMP380 has only irq line. So I understand why I should call it INT. The actual IRQ inside the chip that will be triggered needs to be configured by software. I do it through the bmp{3/5}80_int_config() function. How am I supposed to know which IRQ to enable? Options are:

a) DRDY only
b) FIFO only
c) both

How can I inform the driver about which to enable? Shouldn't this go through the device-tree?

Quoted 8 lines

If you mean track the one from the poll function registered for handling triggers - that's an internal detail but you would indeed need to track in your data structures whether that's the trigger currently being used or not (easy to do by comparing iio_dev->trig with a pointer for each trigger in iio_priv() data - so should be no need for separate tracking.

My idea is that there is one trigger_handler which inside you check which interrupt triggered and you choose which path to choose. If that's wrong, do you know any specific driver that implements it in the correct way? Because in my mind, the iio_dev->trig is one and just inside the handler, different functions are called.

Quoted 4 lines

Jonathan

Thanks for the feedback :)

Cheers,
Vasilis

Quoted 11 lines

Quoted 10 lines

Like this it would be easier to track the active irqs of the sensor.

Let me know what you think, or if I manage to have time I might send a v2 with those changes even earlier :)

Cheers,
Vasilis