[PATCH v1 00/10] BMP280: Minor cleanup and interrupt support

[PATCH v1 00/10] BMP280: Minor cleanup and interrupt support

[Vasileios Amoiridis]

Based on iio testing branch.

Depends on this series [1].

This series aims to add hardware trigger support and extend the functionality
of the driver. Sensors BMP3xx and BMP5xx have an interrupt pin which can be
used in order to inform about a specific event in the sensor. For now, the
data ready event is used, and is added as a DRDY interrupt in the driver.

The interrupt is supported only in rising modes for now, and it doesn't support
latched mode.

Other interrupts such as, FIFO-FULL, FIFO-WATERMARK, Out of range values etc.
are not supported for the moment, and only the DRDY interrupt is supported.

While working on the trigger, FORCED MODE instead of NORMAL MODE was added to
the driver for use in the oneshot capture reads. There is no need for the
driver to continuously produce data, without using them and without properly
notifying the user when those data became available. This can produce high
incosistencies between the acquisition time and the readout of the sensor.
The data now, in the case of the .read_raw() function is using the FORCED MODE,
which samples and calculates the values at that moment.

Last commit, is just moving the interrupt interface of a very old sensor to be
consistent with the new ones, and no functional changes are intended.

[1]: https://lore.kernel.org/linux-iio/20240628171726.124852-1-vassilisamir@gmail.com/

Vasileios Amoiridis (10):
  iio: pressure: bmp280: Fix regmap for BMP280 device
  iio: pressure: bmp280: Fix waiting time for BMP3xx configuration
  iio: pressure: bmp280: Sort headers alphabetically
  iio: pressure: bmp280: Use bulk read for humidity calibration data
  iio: pressure: bmp280: Add support for bmp280 soft reset
  iio: pressure: bmp280: Remove config error check for IIR filter
    updates
  iio: pressure: bmp280: Use sleep and forced mode for oneshot captures
  dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and
    BMP5xx devices
  iio: pressure: bmp280: Add data ready trigger support
  iio: pressure bmp280: Move bmp085 interrupt to new configuration

 .../bindings/iio/pressure/bmp085.yaml         |  13 +-
 drivers/iio/pressure/bmp280-core.c            | 717 +++++++++++++++---
 drivers/iio/pressure/bmp280-i2c.c             |   6 +-
 drivers/iio/pressure/bmp280-regmap.c          |  47 +-
 drivers/iio/pressure/bmp280-spi.c             |   8 +-
 drivers/iio/pressure/bmp280.h                 |  47 +-
 6 files changed, 735 insertions(+), 103 deletions(-)


base-commit: 986da024b99a72e64f6bdb3f3f0e52af024b1f50
prerequisite-patch-id: ffe99ffe927e3309f0cf49ad0109818f58f4048d
prerequisite-patch-id: 5e534cafdd556c49ed1041a8d2e448713021a08e
prerequisite-patch-id: fe806efc601fc318d7240953d44491217de3535f
-- 
2.25.1

[PATCH v1 01/10] iio: pressure: bmp280: Fix regmap for BMP280 device

[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 <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c   |  2 +-
 drivers/iio/pressure/bmp280-regmap.c | 45 ++++++++++++++++++++++++++--
 drivers/iio/pressure/bmp280.h        |  1 +
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 2fc5724196e3..cc8553177977 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1186,7 +1186,7 @@ const struct bmp280_chip_info bme280_chip_info = {
 	.id_reg = BMP280_REG_ID,
 	.chip_id = bme280_chip_ids,
 	.num_chip_id = ARRAY_SIZE(bme280_chip_ids),
-	.regmap_config = &bmp280_regmap_config,
+	.regmap_config = &bme280_regmap_config,
 	.start_up_time = 2000,
 	.channels = bme280_channels,
 	.num_channels = ARRAY_SIZE(bme280_channels),
diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
index fa52839474b1..d27d68edd906 100644
--- a/drivers/iio/pressure/bmp280-regmap.c
+++ b/drivers/iio/pressure/bmp280-regmap.c
@@ -41,7 +41,7 @@ const struct regmap_config bmp180_regmap_config = {
 };
 EXPORT_SYMBOL_NS(bmp180_regmap_config, IIO_BMP280);
 
-static bool bmp280_is_writeable_reg(struct device *dev, unsigned int reg)
+static bool bme280_is_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case BMP280_REG_CONFIG:
@@ -54,7 +54,35 @@ static bool bmp280_is_writeable_reg(struct device *dev, unsigned int reg)
 	}
 }
 
+static bool bmp280_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMP280_REG_CONFIG:
+	case BMP280_REG_CTRL_MEAS:
+	case BMP280_REG_RESET:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static bool bmp280_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMP280_REG_TEMP_XLSB:
+	case BMP280_REG_TEMP_LSB:
+	case BMP280_REG_TEMP_MSB:
+	case BMP280_REG_PRESS_XLSB:
+	case BMP280_REG_PRESS_LSB:
+	case BMP280_REG_PRESS_MSB:
+	case BMP280_REG_STATUS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool bme280_is_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case BME280_REG_HUMIDITY_LSB:
@@ -71,7 +99,6 @@ static bool bmp280_is_volatile_reg(struct device *dev, unsigned int reg)
 		return false;
 	}
 }
-
 static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -167,7 +194,7 @@ const struct regmap_config bmp280_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 
-	.max_register = BME280_REG_HUMIDITY_LSB,
+	.max_register = BMP280_REG_TEMP_XLSB,
 	.cache_type = REGCACHE_RBTREE,
 
 	.writeable_reg = bmp280_is_writeable_reg,
@@ -175,6 +202,18 @@ const struct regmap_config bmp280_regmap_config = {
 };
 EXPORT_SYMBOL_NS(bmp280_regmap_config, IIO_BMP280);
 
+const struct regmap_config bme280_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = BME280_REG_HUMIDITY_LSB,
+	.cache_type = REGCACHE_RBTREE,
+
+	.writeable_reg = bme280_is_writeable_reg,
+	.volatile_reg = bme280_is_volatile_reg,
+};
+EXPORT_SYMBOL_NS(bme280_regmap_config, IIO_BMP280);
+
 const struct regmap_config bmp380_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 0933e411ae2c..4b0ebce001df 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -490,6 +490,7 @@ extern const struct bmp280_chip_info bmp580_chip_info;
 /* Regmap configurations */
 extern const struct regmap_config bmp180_regmap_config;
 extern const struct regmap_config bmp280_regmap_config;
+extern const struct regmap_config bme280_regmap_config;
 extern const struct regmap_config bmp380_regmap_config;
 extern const struct regmap_config bmp580_regmap_config;
 
-- 
2.25.1

[PATCH v1 02/10] iio: pressure: bmp280: Fix waiting time for BMP3xx configuration

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

Fixes: 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index cc8553177977..3deaa57bb3f5 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -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);
-- 
2.25.1

[PATCH v1 03/10] iio: pressure: bmp280: Sort headers alphabetically

[Vasileios Amoiridis]

Sort headers in alphabetical order

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-i2c.c | 2 +-
 drivers/iio/pressure/bmp280-spi.c | 4 ++--
 drivers/iio/pressure/bmp280.h     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
index 34e3bc758493..5c3a63b4327c 100644
--- a/drivers/iio/pressure/bmp280-i2c.c
+++ b/drivers/iio/pressure/bmp280-i2c.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
-#include <linux/module.h>
 #include <linux/i2c.h>
+#include <linux/module.h>
 #include <linux/regmap.h>
 
 #include "bmp280.h"
diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
index f855f56a1b06..d18549d9bb64 100644
--- a/drivers/iio/pressure/bmp280-spi.c
+++ b/drivers/iio/pressure/bmp280-spi.c
@@ -5,10 +5,10 @@
  * Inspired by the older BMP085 driver drivers/misc/bmp085-spi.c
  */
 #include <linux/bits.h>
-#include <linux/module.h>
-#include <linux/spi/spi.h>
 #include <linux/err.h>
+#include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/spi/spi.h>
 
 #include "bmp280.h"
 
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 4b0ebce001df..ccacc67c1473 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -1,10 +1,10 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <linux/bitops.h>
 #include <linux/device.h>
-#include <linux/iio/iio.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/iio/iio.h>
 
 /* BMP580 specific registers */
 #define BMP580_REG_CMD			0x7E
-- 
2.25.1

[PATCH v1 04/10] iio: pressure: bmp280: Use bulk read for humidity calibration data

[Vasileios Amoiridis]

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

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 57 +++++++++---------------------
 drivers/iio/pressure/bmp280.h      |  5 +++
 2 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 3deaa57bb3f5..9c32266403bd 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -118,6 +118,8 @@ enum bmp580_odr {
  */
 enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 
+enum { H2 = 0, H3 = 2, H4 = 3, H5 = 4, H6 = 6 };
+
 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");
-		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);
+	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;
 }
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index ccacc67c1473..56c01f224728 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -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;
-- 
2.25.1

[PATCH v1 05/10] iio: pressure: bmp280: Add support for bmp280 soft reset

[Vasileios Amoiridis]

The BM(P/E)28x devices have an option for soft reset which is also
recommended by the Bosch Sensortech BME2 Sensor API to be used before the
initial configuration of the device.

Link: https://github.com/boschsensortec/BME280_SensorAPI/blob/bme280_v3.5.1/bme280.c#L429
Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 28 ++++++++++++++++++++++++++++
 drivers/iio/pressure/bmp280.h      |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 9c32266403bd..caee586b2268 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -958,6 +958,32 @@ static const unsigned long bme280_avail_scan_masks[] = {
 	0
 };
 
+static int bmp280_preinit(struct bmp280_data *data)
+{
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_write(data->regmap, BMP280_REG_RESET, BMP280_RST_SOFT_CMD);
+	if (ret) {
+		dev_err(data->dev, "Failed to reset device.\n");
+		return ret;
+	}
+
+	usleep_range(data->start_up_time, data->start_up_time + 500);
+
+	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_IM_UPDATE) {
+		dev_err(data->dev, "Failed to copy NVM contents.\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
@@ -1074,6 +1100,7 @@ const struct bmp280_chip_info bmp280_chip_info = {
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
 	.read_calib = bmp280_read_calib,
+	.preinit = bmp280_preinit,
 
 	.trigger_handler = bmp280_trigger_handler,
 };
@@ -1191,6 +1218,7 @@ const struct bmp280_chip_info bme280_chip_info = {
 	.read_press = bmp280_read_press,
 	.read_humid = bme280_read_humid,
 	.read_calib = bme280_read_calib,
+	.preinit = bmp280_preinit,
 
 	.trigger_handler = bme280_trigger_handler,
 };
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 56c01f224728..02647454bd02 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -205,6 +205,9 @@
 #define BMP280_REG_CONFIG		0xF5
 #define BMP280_REG_CTRL_MEAS		0xF4
 #define BMP280_REG_STATUS		0xF3
+#define BMP280_REG_STATUS_IM_UPDATE	BIT(0)
+#define BMP280_REG_RESET		0xE0
+#define BMP280_RST_SOFT_CMD		0xB6
 
 #define BMP280_REG_COMP_TEMP_START	0x88
 #define BMP280_COMP_TEMP_REG_COUNT	6
-- 
2.25.1

[PATCH v1 06/10] iio: pressure: bmp280: Remove config error check for IIR filter updates

[Vasileios Amoiridis]

When there is a change in the configuration of the BMP3xx device, several
steps take place. These steps include:

1) Update the OSR settings and check if there was an update
2) Update the ODR settings and check if there was an update
3) Update the IIR settings and check if there was an update
4) Check if there was an update with the following procedure:
	a) Set sensor to SLEEP mode and after to NORMAL mode to trigger
	   a new measurement.
	b) Wait the maximum amount possible depending on the OSR settings
	c) Check the configuration error register if there was an error
	   during the configuration of the sensor.

This check is necessary, because there could be a case where the OSR is
too high for the requested ODR so either the ODR needs to be slower or the
OSR needs to be less. This is something that is checked internally by the
sensor when it runs in NORMAL mode.

In the BMP58x devices the previous steps are done internally by the sensor.

The IIR filter settings do not depend on the OSR or ODR settings, and there
is no need to run a check in case they change.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index caee586b2268..9c99373d66ec 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1548,14 +1548,12 @@ static int bmp380_chip_config(struct bmp280_data *data)
 	change = change || aux;
 
 	/* Set filter data */
-	ret = regmap_update_bits_check(data->regmap, BMP380_REG_CONFIG, BMP380_FILTER_MASK,
-				       FIELD_PREP(BMP380_FILTER_MASK, data->iir_filter_coeff),
-				       &aux);
+	ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG, BMP380_FILTER_MASK,
+				 FIELD_PREP(BMP380_FILTER_MASK, data->iir_filter_coeff));
 	if (ret) {
 		dev_err(data->dev, "failed to write config register\n");
 		return ret;
 	}
-	change = change || aux;
 
 	if (change) {
 		/*
@@ -2144,15 +2142,13 @@ static int bmp580_chip_config(struct bmp280_data *data)
 	reg_val = FIELD_PREP(BMP580_DSP_IIR_PRESS_MASK, data->iir_filter_coeff) |
 		  FIELD_PREP(BMP580_DSP_IIR_TEMP_MASK, data->iir_filter_coeff);
 
-	ret = regmap_update_bits_check(data->regmap, BMP580_REG_DSP_IIR,
-				       BMP580_DSP_IIR_PRESS_MASK |
-				       BMP580_DSP_IIR_TEMP_MASK,
-				       reg_val, &aux);
+	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_IIR,
+				 BMP580_DSP_IIR_PRESS_MASK |
+				 BMP580_DSP_IIR_TEMP_MASK, reg_val);
 	if (ret) {
 		dev_err(data->dev, "failed to write config register\n");
 		return ret;
 	}
-	change = change || aux;
 
 	/* Restore sensor to normal operation mode */
 	ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
-- 
2.25.1

[PATCH v1 07/10] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

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

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 276 +++++++++++++++++++++++++++--
 drivers/iio/pressure/bmp280.h      |  12 ++
 2 files changed, 269 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 9c99373d66ec..fc8d42880eb8 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -145,6 +145,12 @@ enum bmp280_scan {
 	BME280_HUMID,
 };
 
+enum bmp280_power_modes {
+	BMP280_SLEEP,
+	BMP280_NORMAL,
+	BMP280_FORCED,
+};
+
 static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,
@@ -610,6 +616,14 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_PROCESSED:
+		ret = data->chip_info->set_mode(data, BMP280_FORCED);
+		if (ret)
+			return ret;
+
+		ret = data->chip_info->wait_conv(data);
+		if (ret)
+			return ret;
+
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
 			ret = data->chip_info->read_humid(data, &chan_value);
@@ -639,6 +653,14 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 	case IIO_CHAN_INFO_RAW:
+		ret = data->chip_info->set_mode(data, BMP280_FORCED);
+		if (ret)
+			return ret;
+
+		ret = data->chip_info->wait_conv(data);
+		if (ret)
+			return ret;
+
 		switch (chan->type) {
 		case IIO_HUMIDITYRELATIVE:
 			ret = data->chip_info->read_humid(data, &chan_value);
@@ -984,6 +1006,68 @@ static int bmp280_preinit(struct bmp280_data *data)
 	return 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);
+		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;
+
+	meas_time = BMP280_MEAS_OFFSET;
+
+	if (data->oversampling_humid)
+		meas_time += (1 << data->oversampling_humid) * BMP280_MEAS_DUR +
+			       BMP280_PRESS_HUMID_MEAS_OFFSET;
+
+	/* 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);
 }
 
+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));
+		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");
+		return ret;
+	}
+
+	if (!(reg & BMP380_STATUS_DRDY_PRESS_MASK) ||
+	    !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
+		pr_info("Meas_time: %d\n", meas_time);
+		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) {
+	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);
+
+		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+					BMP580_MODE_MASK,
+					FIELD_PREP(BMP580_MODE_MASK,
+						   BMP580_MODE_FORCED));
+		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] +
+			   time_conv_press[data->oversampling_press];
+
+	usleep_range(meas_time, meas_time * 12 / 10);
+
+	return 0;
+}
+
+
 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;
 }
 
+static int bmp180_set_mode(struct bmp280_data *data, u8 mode)
+{
+	return 0;
+}
+
+static int bmp180_wait_conv(struct bmp280_data *data)
+{
+	return 0;
+}
+
 static int bmp180_chip_config(struct bmp280_data *data)
 {
 	return 0;
@@ -2573,6 +2801,8 @@ const struct bmp280_chip_info bmp180_chip_info = {
 	.read_temp = bmp180_read_temp,
 	.read_press = bmp180_read_press,
 	.read_calib = bmp180_read_calib,
+	.set_mode = bmp180_set_mode,
+	.wait_conv = bmp180_wait_conv,
 
 	.trigger_handler = bmp180_trigger_handler,
 };
@@ -2625,6 +2855,7 @@ static int bmp280_buffer_preenable(struct iio_dev *indio_dev)
 	struct bmp280_data *data = iio_priv(indio_dev);
 
 	pm_runtime_get_sync(data->dev);
+	data->chip_info->set_mode(data, BMP280_NORMAL);
 
 	return 0;
 }
@@ -2795,6 +3026,10 @@ int bmp280_common_probe(struct device *dev,
 			return ret;
 	}
 
+	ret = data->chip_info->set_mode(data, BMP280_SLEEP);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set sleep mode\n");
+
 	/* Enable runtime PM */
 	pm_runtime_get_noresume(dev);
 	pm_runtime_set_active(dev);
@@ -2820,6 +3055,9 @@ static int bmp280_runtime_suspend(struct device *dev)
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct bmp280_data *data = iio_priv(indio_dev);
 
+	data->chip_info->set_mode(data, BMP280_SLEEP);
+
+	usleep_range(2500, 3000);
 	return regulator_bulk_disable(BMP280_NUM_SUPPLIES, data->supplies);
 }
 
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 02647454bd02..93c006c33552 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -170,6 +170,11 @@
 #define BMP380_MODE_FORCED		1
 #define BMP380_MODE_NORMAL		3
 
+#define BMP380_MEAS_OFFSET		234
+#define BMP380_MEAS_DUR			2020
+#define BMP380_TEMP_MEAS_OFFSET		163
+#define BMP380_PRESS_MEAS_OFFSET	392
+
 #define BMP380_MIN_TEMP			-4000
 #define BMP380_MAX_TEMP			8500
 #define BMP380_MIN_PRES			3000000
@@ -206,6 +211,7 @@
 #define BMP280_REG_CTRL_MEAS		0xF4
 #define BMP280_REG_STATUS		0xF3
 #define BMP280_REG_STATUS_IM_UPDATE	BIT(0)
+#define BMP280_REG_STATUS_MEAS_BIT	BIT(3)
 #define BMP280_REG_RESET		0xE0
 #define BMP280_RST_SOFT_CMD		0xB6
 
@@ -246,6 +252,10 @@
 #define BMP280_MODE_FORCED		1
 #define BMP280_MODE_NORMAL		3
 
+#define BMP280_MEAS_OFFSET		1250
+#define BMP280_MEAS_DUR			2300
+#define BMP280_PRESS_HUMID_MEAS_OFFSET	575
+
 /* BME280 specific registers */
 #define BME280_REG_HUMIDITY_LSB		0xFE
 #define BME280_REG_HUMIDITY_MSB		0xFD
@@ -484,6 +494,8 @@ struct bmp280_chip_info {
 	int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity);
 	int (*read_calib)(struct bmp280_data *data);
 	int (*preinit)(struct bmp280_data *data);
+	int (*set_mode)(struct bmp280_data *data, u8 mode);
+	int (*wait_conv)(struct bmp280_data *data);
 
 	irqreturn_t (*trigger_handler)(int irq, void *p);
 };
-- 
2.25.1

[PATCH v1 08/10] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Vasileios Amoiridis]

Add interrupt options for BMP3xx and BMP5xx devices as well.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 .../devicetree/bindings/iio/pressure/bmp085.yaml    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
index 6fda887ee9d4..f06f119963bc 100644
--- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
@@ -48,9 +48,20 @@ properties:
 
   interrupts:
     description:
-      interrupt mapping for IRQ (BMP085 only)
+      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
     maxItems: 1
 
+  interrupt-names:
+    maxItems: 1
+    items:
+      enum:
+        - DRDY
+
+  int-open-drain:
+    desription:
+      set if the interrupt pin should be configured as open drain.
+      If not set, defaults to push-pull configuration.
+
 required:
   - compatible
   - vddd-supply
-- 
2.25.1

[PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

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

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 <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c   | 271 ++++++++++++++++++++++++++-
 drivers/iio/pressure/bmp280-regmap.c |   2 +-
 drivers/iio/pressure/bmp280.h        |  23 ++-
 3 files changed, 290 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index fc8d42880eb8..ee9b9676ad10 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -37,12 +37,14 @@
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/random.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
@@ -1770,6 +1772,129 @@ static int bmp380_chip_config(struct bmp280_data *data)
 	return 0;
 }
 
+static void bmp380_trigger_reenable(struct iio_trigger *trig)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	unsigned int tmp;
+	int ret;
+
+	ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &tmp);
+	if (ret)
+		dev_err(data->dev, "Failed to reset interrupt\n");
+}
+
+static int bmp380_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool state)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CTRL,
+				 BMP380_INT_CTRL_DRDY_EN,
+				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
+					    state ? 1 : 0));
+	if (ret) {
+		dev_err(data->dev, "Could not enable/disable interrupt\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct iio_trigger_ops bmp380_trigger_ops = {
+	.set_trigger_state = &bmp380_data_rdy_trigger_set_state,
+	.reenable = &bmp380_trigger_reenable,
+};
+
+static int bmp380_int_config(struct bmp280_data *data)
+{
+	int ret, int_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN,
+				      data->trig_open_drain) |
+			   FIELD_PREP(BMP380_INT_CTRL_LEVEL,
+				      data->trig_active_high);
+
+	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CTRL,
+				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
+	if (ret) {
+		dev_err(data->dev, "Could not set interrupt settings\n");
+		return ret;
+	}
+
+	return 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;
+	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;
+	}
+
+	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) {
+		dev_err(data->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}
+
+
 static irqreturn_t bmp380_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1863,6 +1988,7 @@ const struct bmp280_chip_info bmp380_chip_info = {
 	.wait_conv = bmp380_wait_conv,
 	.preinit = bmp380_preinit,
 
+	.trigger_probe = bmp380_trigger_probe,
 	.trigger_handler = bmp380_trigger_handler,
 };
 EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
@@ -2400,6 +2526,135 @@ static int bmp580_chip_config(struct bmp280_data *data)
 	return 0;
 }
 
+static void bmp580_trigger_reenable(struct iio_trigger *trig)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	unsigned int tmp;
+	int ret;
+
+	ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &tmp);
+	if (ret)
+		dev_err(data->dev, "Failed to reset interrupt\n");
+}
+
+static int bmp580_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool state)
+{
+	struct bmp280_data *data = iio_trigger_get_drvdata(trig);
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG,
+				 BMP580_INT_CONFIG_INT_EN,
+				 FIELD_PREP(BMP580_INT_CONFIG_INT_EN,
+					    state ? 1 : 0));
+	if (ret) {
+		dev_err(data->dev, "Could not enable/disable interrupt\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct iio_trigger_ops bmp580_trigger_ops = {
+	.set_trigger_state = &bmp580_data_rdy_trigger_set_state,
+	.reenable = &bmp580_trigger_reenable,
+};
+
+static int bmp580_int_config(struct bmp280_data *data)
+{
+	int ret, int_cfg = FIELD_PREP(BMP580_INT_CONFIG_OPEN_DRAIN,
+				      data->trig_open_drain) |
+			   FIELD_PREP(BMP580_INT_CONFIG_LEVEL,
+				      data->trig_active_high);
+
+	ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG,
+				 BMP580_INT_CONFIG_MASK, int_cfg);
+	if (ret) {
+		dev_err(data->dev, "Could not set interrupt settings\n");
+		return ret;
+	}
+
+	ret = regmap_set_bits(data->regmap, BMP580_REG_INT_SOURCE,
+			      BMP580_INT_SOURCE_DRDY);
+	if (ret) {
+		dev_err(data->dev, "Could not set interrupt source\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bmp580_trigger_probe(struct iio_dev *indio_dev)
+{
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;
+	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");
+		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;
+}
+
 static irqreturn_t bmp580_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -2476,6 +2731,7 @@ const struct bmp280_chip_info bmp580_chip_info = {
 	.wait_conv = bmp580_wait_conv,
 	.preinit = bmp580_preinit,
 
+	.trigger_probe = bmp580_trigger_probe,
 	.trigger_handler = bmp580_trigger_handler,
 };
 EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280);
@@ -3020,10 +3276,17 @@ int bmp280_common_probe(struct device *dev,
 	 * however as it happens, the BMP085 shares the chip ID of BMP180
 	 * so we look for an IRQ if we have that.
 	 */
-	if (irq > 0 && (chip_id  == BMP180_CHIP_ID)) {
-		ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
-		if (ret)
-			return ret;
+	if (irq > 0) {
+		if (chip_id == BMP180_CHIP_ID) {
+			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
+			if (ret)
+				return ret;
+		}
+		if (data->chip_info->trigger_probe) {
+			ret = data->chip_info->trigger_probe(indio_dev);
+			if (ret)
+				return ret;
+		}
 	}
 
 	ret = data->chip_info->set_mode(data, BMP280_SLEEP);
diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
index d27d68edd906..cccdf8fc6c09 100644
--- a/drivers/iio/pressure/bmp280-regmap.c
+++ b/drivers/iio/pressure/bmp280-regmap.c
@@ -109,7 +109,7 @@ static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
 	case BMP380_REG_FIFO_WATERMARK_LSB:
 	case BMP380_REG_FIFO_WATERMARK_MSB:
 	case BMP380_REG_POWER_CONTROL:
-	case BMP380_REG_INT_CONTROL:
+	case BMP380_REG_INT_CTRL:
 	case BMP380_REG_IF_CONFIG:
 	case BMP380_REG_ODR:
 	case BMP380_REG_OSR:
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 93c006c33552..73d0d25ae0f4 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -55,8 +55,17 @@
 #define BMP580_CMD_NVM_WRITE_SEQ_1	0xA0
 #define BMP580_CMD_SOFT_RESET		0xB6
 
+#define BMP580_INT_STATUS_DRDY_MASK	BIT(0)
 #define BMP580_INT_STATUS_POR_MASK	BIT(4)
 
+#define BMP580_INT_SOURCE_DRDY		BIT(0)
+
+#define BMP580_INT_CONFIG_MASK		GENMASK(3, 0)
+#define BMP580_INT_CONFIG_LATCH		BIT(0)
+#define BMP580_INT_CONFIG_LEVEL		BIT(1)
+#define BMP580_INT_CONFIG_OPEN_DRAIN	BIT(2)
+#define BMP580_INT_CONFIG_INT_EN	BIT(3)
+
 #define BMP580_STATUS_CORE_RDY_MASK	BIT(0)
 #define BMP580_STATUS_NVM_RDY_MASK	BIT(1)
 #define BMP580_STATUS_NVM_ERR_MASK	BIT(2)
@@ -117,7 +126,7 @@
 #define BMP380_REG_OSR			0x1C
 #define BMP380_REG_POWER_CONTROL	0x1B
 #define BMP380_REG_IF_CONFIG		0x1A
-#define BMP380_REG_INT_CONTROL		0x19
+#define BMP380_REG_INT_CTRL		0x19
 #define BMP380_REG_INT_STATUS		0x11
 #define BMP380_REG_EVENT		0x10
 #define BMP380_REG_STATUS		0x03
@@ -175,6 +184,14 @@
 #define BMP380_TEMP_MEAS_OFFSET		163
 #define BMP380_PRESS_MEAS_OFFSET	392
 
+#define BMP380_INT_STATUS_DRDY		BIT(3)
+
+#define BMP380_INT_CTRL_SETTINGS_MASK	GENMASK(2, 0)
+#define BMP380_INT_CTRL_OPEN_DRAIN	BIT(0)
+#define BMP380_INT_CTRL_LEVEL		BIT(1)
+#define BMP380_INT_CTRL_LATCH		BIT(2)
+#define BMP380_INT_CTRL_DRDY_EN		BIT(6)
+
 #define BMP380_MIN_TEMP			-4000
 #define BMP380_MAX_TEMP			8500
 #define BMP380_MIN_PRES			3000000
@@ -399,6 +416,9 @@ struct bmp280_data {
 	struct regmap *regmap;
 	struct completion done;
 	bool use_eoc;
+	bool trig_open_drain;
+	bool trig_active_high;
+	struct iio_trigger *trig;
 	const struct bmp280_chip_info *chip_info;
 	union {
 		struct bmp180_calib bmp180;
@@ -497,6 +517,7 @@ struct bmp280_chip_info {
 	int (*set_mode)(struct bmp280_data *data, u8 mode);
 	int (*wait_conv)(struct bmp280_data *data);
 
+	int (*trigger_probe)(struct iio_dev *indio_dev);
 	irqreturn_t (*trigger_handler)(int irq, void *p);
 };
 
-- 
2.25.1

[PATCH v1 10/10] iio: pressure bmp280: Move bmp085 interrupt to new configuration

[Vasileios Amoiridis]

This commit intends to add the old BMP085 sensor to the new IRQ interface
of the sensor consistence. No functional changes intended.

The BMP085 sensor is equivalent with the BMP180 with the only difference of
BMP085 having an extra interrupt pin to inform about an End of Conversion.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 72 +++++++++++++++++++++++-------
 drivers/iio/pressure/bmp280-i2c.c  |  4 +-
 drivers/iio/pressure/bmp280-spi.c  |  4 +-
 drivers/iio/pressure/bmp280.h      |  1 +
 4 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index ee9b9676ad10..f0443016a2f7 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -3073,13 +3073,19 @@ static irqreturn_t bmp085_eoc_irq(int irq, void *d)
 	return IRQ_HANDLED;
 }
 
-static int bmp085_fetch_eoc_irq(struct device *dev,
-				const char *name,
-				int irq,
-				struct bmp280_data *data)
+static int bmp085_trigger_probe(struct iio_dev *indio_dev)
 {
+	struct bmp280_data *data = iio_priv(indio_dev);
+	struct device *dev = data->dev;
+	struct fwnode_handle *fwnode;
 	unsigned long irq_trig;
-	int ret;
+	int ret, irq;
+
+	fwnode = dev_fwnode(data->dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get(fwnode, 0);
 
 	irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
 	if (irq_trig != IRQF_TRIGGER_RISING) {
@@ -3089,13 +3095,12 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
 
 	init_completion(&data->done);
 
-	ret = devm_request_threaded_irq(dev,
-			irq,
-			bmp085_eoc_irq,
-			NULL,
-			irq_trig,
-			name,
-			data);
+	ret = devm_request_irq(dev,
+			       irq,
+			       bmp085_eoc_irq,
+			       irq_trig,
+			       indio_dev->name,
+			       data);
 	if (ret) {
 		/* Bail out without IRQ but keep the driver in place */
 		dev_err(dev, "unable to request DRDY IRQ\n");
@@ -3106,6 +3111,44 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
 	return 0;
 }
 
+const struct bmp280_chip_info bmp085_chip_info = {
+	.id_reg = bmp180_chip_info.id_reg,
+	.chip_id = bmp180_chip_info.chip_id,
+	.num_chip_id = bmp180_chip_info.num_chip_id,
+	.regmap_config = bmp180_chip_info.regmap_config,
+	.start_up_time = bmp180_chip_info.start_up_time,
+	.channels = bmp180_chip_info.channels,
+	.num_channels = bmp180_chip_info.num_channels,
+	.avail_scan_masks = bmp180_chip_info.avail_scan_masks,
+
+	.oversampling_temp_avail = bmp180_chip_info.oversampling_temp_avail,
+	.num_oversampling_temp_avail =
+		bmp180_chip_info.num_oversampling_temp_avail,
+	.oversampling_temp_default = bmp180_chip_info.oversampling_temp_default,
+
+	.oversampling_press_avail = bmp180_chip_info.oversampling_press_avail,
+	.num_oversampling_press_avail =
+		bmp180_chip_info.num_oversampling_press_avail,
+	.oversampling_press_default =
+		bmp180_chip_info.oversampling_press_default,
+
+	.temp_coeffs = bmp180_chip_info.temp_coeffs,
+	.temp_coeffs_type = bmp180_chip_info.temp_coeffs_type,
+	.press_coeffs = bmp180_chip_info.press_coeffs,
+	.press_coeffs_type = bmp180_chip_info.press_coeffs_type,
+
+	.chip_config = bmp180_chip_info.chip_config,
+	.read_temp = bmp180_chip_info.read_temp,
+	.read_press = bmp180_chip_info.read_press,
+	.read_calib = bmp180_chip_info.read_calib,
+	.set_mode = bmp180_chip_info.set_mode,
+	.wait_conv = bmp180_chip_info.wait_conv,
+
+	.trigger_probe = bmp085_trigger_probe,
+	.trigger_handler = bmp180_trigger_handler,
+};
+EXPORT_SYMBOL_NS(bmp085_chip_info, IIO_BMP280);
+
 static int bmp280_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct bmp280_data *data = iio_priv(indio_dev);
@@ -3277,11 +3320,6 @@ int bmp280_common_probe(struct device *dev,
 	 * so we look for an IRQ if we have that.
 	 */
 	if (irq > 0) {
-		if (chip_id == BMP180_CHIP_ID) {
-			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
-			if (ret)
-				return ret;
-		}
 		if (data->chip_info->trigger_probe) {
 			ret = data->chip_info->trigger_probe(indio_dev);
 			if (ret)
diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
index 5c3a63b4327c..2f7b25984c7b 100644
--- a/drivers/iio/pressure/bmp280-i2c.c
+++ b/drivers/iio/pressure/bmp280-i2c.c
@@ -27,7 +27,7 @@ static int bmp280_i2c_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id bmp280_of_i2c_match[] = {
-	{ .compatible = "bosch,bmp085", .data = &bmp180_chip_info },
+	{ .compatible = "bosch,bmp085", .data = &bmp085_chip_info },
 	{ .compatible = "bosch,bmp180", .data = &bmp180_chip_info },
 	{ .compatible = "bosch,bmp280", .data = &bmp280_chip_info },
 	{ .compatible = "bosch,bme280", .data = &bme280_chip_info },
@@ -38,7 +38,7 @@ static const struct of_device_id bmp280_of_i2c_match[] = {
 MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
 
 static const struct i2c_device_id bmp280_i2c_id[] = {
-	{"bmp085", (kernel_ulong_t)&bmp180_chip_info },
+	{"bmp085", (kernel_ulong_t)&bmp085_chip_info },
 	{"bmp180", (kernel_ulong_t)&bmp180_chip_info },
 	{"bmp280", (kernel_ulong_t)&bmp280_chip_info },
 	{"bme280", (kernel_ulong_t)&bme280_chip_info },
diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
index d18549d9bb64..49aa8c2cd85b 100644
--- a/drivers/iio/pressure/bmp280-spi.c
+++ b/drivers/iio/pressure/bmp280-spi.c
@@ -114,7 +114,7 @@ static int bmp280_spi_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id bmp280_of_spi_match[] = {
-	{ .compatible = "bosch,bmp085", .data = &bmp180_chip_info },
+	{ .compatible = "bosch,bmp085", .data = &bmp085_chip_info },
 	{ .compatible = "bosch,bmp180", .data = &bmp180_chip_info },
 	{ .compatible = "bosch,bmp181", .data = &bmp180_chip_info },
 	{ .compatible = "bosch,bmp280", .data = &bmp280_chip_info },
@@ -126,7 +126,7 @@ static const struct of_device_id bmp280_of_spi_match[] = {
 MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
 
 static const struct spi_device_id bmp280_spi_id[] = {
-	{ "bmp085", (kernel_ulong_t)&bmp180_chip_info },
+	{ "bmp085", (kernel_ulong_t)&bmp085_chip_info },
 	{ "bmp180", (kernel_ulong_t)&bmp180_chip_info },
 	{ "bmp181", (kernel_ulong_t)&bmp180_chip_info },
 	{ "bmp280", (kernel_ulong_t)&bmp280_chip_info },
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 73d0d25ae0f4..1c1e3001efc9 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -522,6 +522,7 @@ struct bmp280_chip_info {
 };
 
 /* Chip infos for each variant */
+extern const struct bmp280_chip_info bmp085_chip_info;
 extern const struct bmp280_chip_info bmp180_chip_info;
 extern const struct bmp280_chip_info bmp280_chip_info;
 extern const struct bmp280_chip_info bme280_chip_info;
-- 
2.25.1

Re: [PATCH v1 08/10] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Rob Herring (Arm)]

On Thu, 11 Jul 2024 23:15:56 +0200, Vasileios Amoiridis wrote:
> Add interrupt options for BMP3xx and BMP5xx devices as well.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  .../devicetree/bindings/iio/pressure/bmp085.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml: properties:int-open-drain: 'anyOf' conditional failed, one must be fixed:
	'desription' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'dependentRequired', 'dependentSchemas', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'typeSize', 'unevaluatedProperties', 'uniqueItems']
	'type' was expected
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml: properties:interrupt-names:items: {'enum': ['DRDY']} is not of type 'array'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml: int-open-drain: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240711211558.106327-9-vassilisamir@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 08/10] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Rob Herring]

On Thu, Jul 11, 2024 at 11:15:56PM +0200, Vasileios Amoiridis wrote:
> Add interrupt options for BMP3xx and BMP5xx devices as well.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  .../devicetree/bindings/iio/pressure/bmp085.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> index 6fda887ee9d4..f06f119963bc 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> @@ -48,9 +48,20 @@ properties:
>  
>    interrupts:
>      description:
> -      interrupt mapping for IRQ (BMP085 only)
> +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
>      maxItems: 1
>  
> +  interrupt-names:
> +    maxItems: 1
> +    items:
> +      enum:
> +        - DRDY
> +
> +  int-open-drain:

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

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

Re: [PATCH v1 01/10] iio: pressure: bmp280: Fix regmap for BMP280 device

[Jonathan Cameron]

On Thu, 11 Jul 2024 23:15:49 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> 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 <vassilisamir@gmail.com>
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

Re: [PATCH v1 02/10] iio: pressure: bmp280: Fix waiting time for BMP3xx configuration

[Jonathan Cameron]

On Thu, 11 Jul 2024 23:15:50 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> 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 <vassilisamir@gmail.com>
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

> ---
>  drivers/iio/pressure/bmp280-core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index cc8553177977..3deaa57bb3f5 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -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);

Re: [PATCH v1 03/10] iio: pressure: bmp280: Sort headers alphabetically

[Jonathan Cameron]

On Thu, 11 Jul 2024 23:15:51 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Sort headers in alphabetical order
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
Applied.

Re: [PATCH v1 04/10] iio: pressure: bmp280: Use bulk read for humidity calibration data

[Jonathan Cameron]

On Thu, 11 Jul 2024 23:15:52 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Convert individual reads to a bulk read for the humidity calibration data.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-core.c | 57 +++++++++---------------------
>  drivers/iio/pressure/bmp280.h      |  5 +++
>  2 files changed, 21 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 3deaa57bb3f5..9c32266403bd 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -118,6 +118,8 @@ enum bmp580_odr {
>   */
>  enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 };
>  
> +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.

> +
>  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");
> -		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.


> +	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;
>  }
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index ccacc67c1473..56c01f224728 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -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;

Re: [PATCH v1 07/10] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Jonathan Cameron]

On Thu, 11 Jul 2024 23:15:55 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> 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 <vassilisamir@gmail.com>
Various minor comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/pressure/bmp280-core.c | 276 +++++++++++++++++++++++++++--
>  drivers/iio/pressure/bmp280.h      |  12 ++
>  2 files changed, 269 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 9c99373d66ec..fc8d42880eb8 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -145,6 +145,12 @@ enum bmp280_scan {
>  	BME280_HUMID,
>  };
>  }
>  
> +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.

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


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

> +		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");
> +		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.

> +		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) {
> +	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.

> +		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.
> +		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] +
> +			   time_conv_press[data->oversampling_press];
> +
> +	usleep_range(meas_time, meas_time * 12 / 10);
> +
> +	return 0;
> +}
> +
one blank line only.
> +
>  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;
>  }
>  
> +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.

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

Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

[Jonathan Cameron]

On Thu, 11 Jul 2024 23:15:57 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> 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 <vassilisamir@gmail.com>

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.

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

> +	}
> +
> +	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) {
> +		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.

>  static irqreturn_t bmp380_trigger_handler(int irq, void *p)


> +
> +static int bmp580_trigger_probe(struct iio_dev *indio_dev)
> +{
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	struct fwnode_handle *fwnode;
> +	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.

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

Re: [PATCH v1 01/10] iio: pressure: bmp280: Fix regmap for BMP280 device

[Vasileios Amoiridis]

On Sat, Jul 20, 2024 at 12:04:29PM +0100, Jonathan Cameron wrote:
> On Thu, 11 Jul 2024 23:15:49 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > 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 <vassilisamir@gmail.com>
> 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
> 
> 

Hi Jonathan,

No problem at all! Yes indeed it's an old fix and I am in no rush at all
so th simpler the better :)

Cheers,
Vasilis

Re: [PATCH v1 02/10] iio: pressure: bmp280: Fix waiting time for BMP3xx configuration

[Vasileios Amoiridis]

On Sat, Jul 20, 2024 at 12:06:36PM +0100, Jonathan Cameron wrote:
> On Thu, 11 Jul 2024 23:15:50 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > 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 <vassilisamir@gmail.com>
> 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
> 

Hi Jonathan,

Probably no one had this configuration up to now so I don't think there is a need
to rush it :)

Cheers,
Vasilis

Re: [PATCH v1 04/10] iio: pressure: bmp280: Use bulk read for humidity calibration data

[Vasileios Amoiridis]

On Sat, Jul 20, 2024 at 12:16:04PM +0100, Jonathan Cameron wrote:
> On Thu, 11 Jul 2024 23:15:52 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > Convert individual reads to a bulk read for the humidity calibration data.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 57 +++++++++---------------------
> >  drivers/iio/pressure/bmp280.h      |  5 +++
> >  2 files changed, 21 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 3deaa57bb3f5..9c32266403bd 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -118,6 +118,8 @@ enum bmp580_odr {
> >   */
> >  enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 };
> >  
> > +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.
> 

True, thanks!

> > +
> >  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");
> > -		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.
> 
> 

You are right it is a bit confusing. This endianness "fun" made me probably
write much more complex code. Indeed, it doesn't look good what I do even
though it works.

As you said, that H4_MASK_UP should be GENMASK(15,8) and then I should find
a better way.

Cheers,
Vasilis

> > +	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;
> >  }
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index ccacc67c1473..56c01f224728 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -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;
> 

Re: [PATCH v1 07/10] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Vasileios Amoiridis]

On Sat, Jul 20, 2024 at 12:28:02PM +0100, Jonathan Cameron wrote:
> On Thu, 11 Jul 2024 23:15:55 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > 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 <vassilisamir@gmail.com>
> Various minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 

Hi Jonathan,

Thanks again for the amazing feedback!

My answers inline.

Cheers,
Vasilis
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 276 +++++++++++++++++++++++++++--
> >  drivers/iio/pressure/bmp280.h      |  12 ++
> >  2 files changed, 269 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 9c99373d66ec..fc8d42880eb8 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -145,6 +145,12 @@ enum bmp280_scan {
> >  	BME280_HUMID,
> >  };
> >  }
> >  
> > +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.
> 

I like a lot both approaches, I feel like the const array one I like it more.

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

This is a check for if we use the BME280 or the BMP280 since humidity is a
feature of only the BME280 sensor. I should add it, thanks.

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

Ack.

> > +		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");
> > +		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.
> 

This is one of my forgotten "debug" messages...

> > +		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) {
> > +	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.
> 

True, will do.

> > +		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.
> > +		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] +
> > +			   time_conv_press[data->oversampling_press];
> > +
> > +	usleep_range(meas_time, meas_time * 12 / 10);
> > +
> > +	return 0;
> > +}
> > +
> one blank line only.

True, will do.
> > +
> >  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;
> >  }
> >  
> > +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?

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

Re: [PATCH v1 08/10] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Vasileios Amoiridis]

On Fri, Jul 12, 2024 at 06:48:05AM -0600, Rob Herring wrote:
> On Thu, Jul 11, 2024 at 11:15:56PM +0200, Vasileios Amoiridis wrote:
> > Add interrupt options for BMP3xx and BMP5xx devices as well.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  .../devicetree/bindings/iio/pressure/bmp085.yaml    | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > index 6fda887ee9d4..f06f119963bc 100644
> > --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > @@ -48,9 +48,20 @@ properties:
> >  
> >    interrupts:
> >      description:
> > -      interrupt mapping for IRQ (BMP085 only)
> > +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
> >      maxItems: 1
> >  
> > +  interrupt-names:
> > +    maxItems: 1
> > +    items:
> > +      enum:
> > +        - DRDY
> > +
> > +  int-open-drain:
> 
> Use the existing 'drive-open-drain' property.
> 

Hi Rob,

Thanks for the feedback. I will also use the tools from your automated reply.

Cheers,
Vasilis

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

Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

[Vasileios Amoiridis]

On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote:
> On Thu, 11 Jul 2024 23:15:57 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > 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 <vassilisamir@gmail.com>
> 
> 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.
> 

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

> > +
> > +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;
> > +	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()
> 
> > +	}
> > +
> > +	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) {
> > +		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.
> 
> >  static irqreturn_t bmp380_trigger_handler(int irq, void *p)
> 
> 
> > +
> > +static int bmp580_trigger_probe(struct iio_dev *indio_dev)
> > +{
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	struct fwnode_handle *fwnode;
> > +	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.
> 
> > +		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;
> > +}
> >
> >  
> 

Re: [PATCH v1 07/10] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Jonathan Cameron]

> > >  
> > >  	.trigger_handler = bmp580_trigger_handler,
> > > @@ -2503,6 +2721,16 @@ static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press)
> > >  	return 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?
Hmm. Maybe? :)

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

Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

[Jonathan Cameron]

On Mon, 22 Jul 2024 01:51:13 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote:
> > On Thu, 11 Jul 2024 23:15:57 +0200
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > 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 <vassilisamir@gmail.com>  
> > 
> > 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.
> >   
> 
> 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

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.

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

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



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

Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

[Vasileios Amoiridis]

On Mon, Jul 22, 2024 at 08:31:38PM +0100, Jonathan Cameron wrote:
> On Mon, 22 Jul 2024 01:51:13 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote:
> > > On Thu, 11 Jul 2024 23:15:57 +0200
> > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > >   
> > > > 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 <vassilisamir@gmail.com>  
> > > 
> > > 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.
> > >   
> > 
> > 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
> 
> 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.

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

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

> Jonathan
> 
> 

Thanks for the feedback :)

Cheers,
Vasilis

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

Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

[Jonathan Cameron]

On Mon, 22 Jul 2024 22:38:34 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Mon, Jul 22, 2024 at 08:31:38PM +0100, Jonathan Cameron wrote:
> > On Mon, 22 Jul 2024 01:51:13 +0200
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote:  
> > > > On Thu, 11 Jul 2024 23:15:57 +0200
> > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > > >     
> > > > > 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 <vassilisamir@gmail.com>    
> > > > 
> > > > 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.
> > > >     
> > > 
> > > 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  
> > 
> > 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.

Often the right thing to do for a fifo is to not use a trigger at all.
So instead it becomes the main interrupt handler that runs that path.
The reason being that triggers are typically one per 'scan' i.e. set of
channels and fifo interrupts are much less common.  That makes a mess
of things like timestamps etc that means different handling is necessary.
So for fifo paths we often just don't use a trigger.


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

No. This is a policy question for the driver, not something to do
with wiring (which is what belongs in device tree).
You choose how it is used based on what userspace configures the
device to do.

DRDY if it uses that trigger.  FIFO typically if the buffer is enabled
without a trigger (though we have a few cases where a dummy trigger
is used for this).  


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

You don't have to use a trigger.  Just look for drivers that handle
a fifo. This is pretty common situation and there are a couple of
different solutions but often the cleanest is to not use the
trigger infrastructure at all if the fifo is in use.

Jonathan

> 
> > Jonathan
> > 
> >   
> 
> Thanks for the feedback :)
> 
> Cheers,
> Vasilis
> 
> >   
> > > 
> > > 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
> > >   
> >