[PATCH v3 0/7] pressure: bmp280: Minor cleanup and interrupt support

[PATCH v3 0/7] pressure: bmp280: Minor cleanup and interrupt support

[Vasileios Amoiridis]

Depends on this: https://lore.kernel.org/linux-iio/20240823172017.9028-1-vassilisamir@gmail.com

Changes in v3:

[PATCH v3 1/7]:
	- Moved the indexing enum on top of the calibration buffer and added a
	  comment that the reason for the complex accesses can be found in the
	  datasheet.

[PATCH v3 2/7]:
	- Use dev_err_probe() instead of dev_err() since the .preinit function
	  is called only from the probe.

[PATCH v3 4/7]:
	- Made static the const int arrays.
	- Changed the comment from "future" versions to "newer" versions.
	- The current state of the board is saved into a variable in order to be
	  able to be recovered after a suspend/resume operation.

[PATCH v3 5/7]:
	- Corrected syntax error.

[PATCH v3 6/7]:
	- Merged the bmp{3,5}80_trigger_probe() functions to one by using a
	  general __bmp280_trigger_probe() which takes the sensor specific
	  functions as interrupts.

[PATCH v3 7/7]:
	- Fixed the bmp085_chip_info array by duplicating the bmp180_chip_info
	  and just adding the extra variable for the bmp085 interrupt.

v2: https://lore.kernel.org/linux-iio/20240725231039.614536-1-vassilisamir@gmail.com

Vasileios Amoiridis (7):
  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         |   7 +-
 drivers/iio/pressure/bmp280-core.c            | 691 +++++++++++++++---
 drivers/iio/pressure/bmp280-i2c.c             |   4 +-
 drivers/iio/pressure/bmp280-spi.c             |   4 +-
 drivers/iio/pressure/bmp280.h                 |  52 ++
 5 files changed, 667 insertions(+), 91 deletions(-)


base-commit: 0f718e10da81446df0909c9939dff2b77e3b4e95
prerequisite-patch-id: e4f81f31f4fbb2aa872c0c74ed4511893eee0c9a
-- 
2.25.1

[PATCH v3 1/7] 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 | 64 +++++++++++-------------------
 drivers/iio/pressure/bmp280.h      |  6 +++
 2 files changed, 29 insertions(+), 41 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 8383d1a73cf9..c23515048081 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -340,10 +340,19 @@ static int bmp280_read_calib(struct bmp280_data *data)
 	return 0;
 }
 
+/*
+ * These enums are used for indexing into the array of humidity parameters
+ * for BME280. Due to some weird indexing, unaligned BE/LE accesses co-exist in
+ * order to prepare the FIELD_{GET/PREP}() fields. Table 16 in Section 4.2.2 of
+ * the datasheet.
+ */
+enum { H2 = 0, H3 = 2, H4 = 3, H5 = 4, H6 = 6 };
+
 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 +361,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 +369,24 @@ 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));
+			       data->bme280_humid_cal_buf,
+			       sizeof(data->bme280_humid_cal_buf));
 	if (ret) {
-		dev_err(dev, "failed to read H4 comp value\n");
+		dev_err(dev, "failed to read humidity calibration values\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));
-	if (ret) {
-		dev_err(dev, "failed to read H5 comp value\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_GET_MASK_UP,
+			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
+	h4_upper = FIELD_PREP(BME280_COMP_H4_PREP_MASK_UP, h4_upper);
+	h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW,
+			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
+	calib->H4 = sign_extend32(h4_upper | 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 a853b6d5bdfa..4e675401d61b 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -257,8 +257,13 @@
 #define BME280_REG_COMP_H5		0xE5
 #define BME280_REG_COMP_H6		0xE7
 
+#define BME280_COMP_H4_GET_MASK_UP	GENMASK(15, 8)
+#define BME280_COMP_H4_PREP_MASK_UP	GENMASK(11, 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
@@ -426,6 +431,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

Re: [PATCH v3 1/7] iio: pressure: bmp280: Use bulk read for humidity calibration data

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 08:17:08PM +0200, Vasileios Amoiridis wrote:
> Convert individual reads to a bulk read for the humidity calibration data.

...

> +	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_GET_MASK_UP,
> +			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
> +	h4_upper = FIELD_PREP(BME280_COMP_H4_PREP_MASK_UP, h4_upper);

This is a bit confusing. I would add a tmp variable and have this as

	tmp = FIELD_GET(BME280_COMP_H4_GET_MASK_UP,
			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
	h4_upper = FIELD_PREP(BME280_COMP_H4_PREP_MASK_UP, tmp);

Also note indentation issues.

> +	h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW,
> +			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
> +	calib->H4 = sign_extend32(h4_upper | 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];

...

>  		/* Calibration data buffers */
>  		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
>  		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];

Side note: I would see rather sizeof(__Xe16) than 2:s in the above definitions.

> +		u8 bme280_humid_cal_buf[BME280_CONTIGUOUS_CALIB_REGS];

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/7] iio: pressure: bmp280: Use bulk read for humidity calibration data

[Vasileios Amoiridis]

On Fri, Aug 23, 2024 at 09:47:22PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 08:17:08PM +0200, Vasileios Amoiridis wrote:
> > Convert individual reads to a bulk read for the humidity calibration data.
> 
> ...
> 
> > +	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_GET_MASK_UP,
> > +			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
> > +	h4_upper = FIELD_PREP(BME280_COMP_H4_PREP_MASK_UP, h4_upper);
> 
> This is a bit confusing. I would add a tmp variable and have this as
> 
> 	tmp = FIELD_GET(BME280_COMP_H4_GET_MASK_UP,
> 			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
> 	h4_upper = FIELD_PREP(BME280_COMP_H4_PREP_MASK_UP, tmp);
> 
> Also note indentation issues.
> 

Hi Andy,

It's been a long time since you last reviewed code of mine, thanks for
finding the time to do this, I appreciate it a lot!

You are right, it looks confusing, I can do what you proposed. I don't
know how I didn't see it before.

Cheers,
Vasilis

> > +	h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW,
> > +			get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
> > +	calib->H4 = sign_extend32(h4_upper | 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];
> 
> ...
> 
> >  		/* Calibration data buffers */
> >  		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> >  		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
> 
> Side note: I would see rather sizeof(__Xe16) than 2:s in the above definitions.
> 
> > +		u8 bme280_humid_cal_buf[BME280_CONTIGUOUS_CALIB_REGS];
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

[PATCH v3 2/7] 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 | 26 ++++++++++++++++++++++++++
 drivers/iio/pressure/bmp280.h      |  3 +++
 2 files changed, 29 insertions(+)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index c23515048081..e01c9369bd67 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -965,6 +965,30 @@ 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)
+		return dev_err_probe(data->dev, ret,
+				     "Failed to reset device.\n");
+
+	usleep_range(data->start_up_time, data->start_up_time + 500);
+
+	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Failed to read status register.\n");
+
+	if (reg & BMP280_REG_STATUS_IM_UPDATE)
+		return dev_err_probe(data->dev, ret,
+				     "Failed to copy NVM contents.\n");
+
+	return 0;
+}
+
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
@@ -1082,6 +1106,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,
 };
@@ -1202,6 +1227,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 4e675401d61b..73516878d020 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

Re: [PATCH v3 2/7] iio: pressure: bmp280: Add support for bmp280 soft reset

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 08:17:09PM +0200, Vasileios Amoiridis wrote:
> 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.

...

> +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)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to reset device.\n");

> +	usleep_range(data->start_up_time, data->start_up_time + 500);

Seems long enough to warrant the comment. Also, why not fsleep()?

> +	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to read status register.\n");
> +
> +	if (reg & BMP280_REG_STATUS_IM_UPDATE)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to copy NVM contents.\n");
> +
> +	return 0;
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/7] iio: pressure: bmp280: Add support for bmp280 soft reset

[Vasileios Amoiridis]

On Fri, Aug 23, 2024 at 10:13:57PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 08:17:09PM +0200, Vasileios Amoiridis wrote:
> > 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.
> 
> ...
> 
> > +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)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "Failed to reset device.\n");
> 
> > +	usleep_range(data->start_up_time, data->start_up_time + 500);
> 
> Seems long enough to warrant the comment. Also, why not fsleep()?

Hi Andy,

The datasheet of the sensor, and the published API from Bosch [1] 
require the startup_time for this procedure, it's not something that
came up from my mind. That's why I didn't add any comment.

I was not aware of fsleep(), I think that it could be fine to use it.

Cheers,
Vasilis

[1]: https://github.com/boschsensortec/BME280_SensorAPI/blob/master/bme280.c#L677

> 
> > +	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
> > +	if (ret)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "Failed to read status register.\n");
> > +
> > +	if (reg & BMP280_REG_STATUS_IM_UPDATE)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "Failed to copy NVM contents.\n");
> > +
> > +	return 0;
> > +}
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v3 2/7] iio: pressure: bmp280: Add support for bmp280 soft reset

[Andy Shevchenko]

On Sat, Aug 24, 2024 at 01:16:14PM +0200, Vasileios Amoiridis wrote:
> On Fri, Aug 23, 2024 at 10:13:57PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:09PM +0200, Vasileios Amoiridis wrote:

...

> > > +	usleep_range(data->start_up_time, data->start_up_time + 500);
> > 
> > Seems long enough to warrant the comment. Also, why not fsleep()?
> 
> The datasheet of the sensor, and the published API from Bosch [1] 
> require the startup_time for this procedure, it's not something that
> came up from my mind. That's why I didn't add any comment.

The comment usually needed on the basis of how long we have to sleep.
To me ~1ms warrants that as it's long enough timeout on modern (GHz
frequency range) CPUs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/7] iio: pressure: bmp280: Add support for bmp280 soft reset

[Christophe JAILLET]

Le 23/08/2024 à 20:17, Vasileios Amoiridis a écrit :
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/iio/pressure/bmp280-core.c | 26 ++++++++++++++++++++++++++
>   drivers/iio/pressure/bmp280.h      |  3 +++
>   2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index c23515048081..e01c9369bd67 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -965,6 +965,30 @@ 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)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to reset device.\n");
> +
> +	usleep_range(data->start_up_time, data->start_up_time + 500);
> +
> +	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to read status register.\n");
> +
> +	if (reg & BMP280_REG_STATUS_IM_UPDATE)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to copy NVM contents.\n");

ret is 0 at this point.
Should a -E<something> be used instead?

CJ

> +
> +	return 0;
> +}
> +
>   static int bmp280_chip_config(struct bmp280_data *data)
>   {
>   	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
> @@ -1082,6 +1106,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,
>   };
> @@ -1202,6 +1227,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 4e675401d61b..73516878d020 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

Re: [PATCH v3 2/7] iio: pressure: bmp280: Add support for bmp280 soft reset

[Vasileios Amoiridis]

On Sun, Aug 25, 2024 at 09:04:16AM +0200, Christophe JAILLET wrote:
> Le 23/08/2024 à 20:17, Vasileios Amoiridis a écrit :
> > 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >   drivers/iio/pressure/bmp280-core.c | 26 ++++++++++++++++++++++++++
> >   drivers/iio/pressure/bmp280.h      |  3 +++
> >   2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index c23515048081..e01c9369bd67 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -965,6 +965,30 @@ 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)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "Failed to reset device.\n");
> > +
> > +	usleep_range(data->start_up_time, data->start_up_time + 500);
> > +
> > +	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
> > +	if (ret)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "Failed to read status register.\n");
> > +
> > +	if (reg & BMP280_REG_STATUS_IM_UPDATE)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "Failed to copy NVM contents.\n");
> 
> ret is 0 at this point.
> Should a -E<something> be used instead?
> 
> CJ
> 

Hi Cristophe,

Yes, actually this could be an I/O error since we were not able to
copy the values back from the NVM device to the sensor.

Cheers,
Vasilis

> > +
> > +	return 0;
> > +}
> > +
> >   static int bmp280_chip_config(struct bmp280_data *data)
> >   {
> >   	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
> > @@ -1082,6 +1106,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,
> >   };
> > @@ -1202,6 +1227,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 4e675401d61b..73516878d020 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
> 

[PATCH v3 3/7] 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 e01c9369bd67..736a1f4fd5dc 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1557,14 +1557,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) {
 		/*
@@ -2154,15 +2152,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

Re: [PATCH v3 3/7] iio: pressure: bmp280: Remove config error check for IIR filter updates

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 08:17:10PM +0200, Vasileios Amoiridis wrote:
> 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.

...

> +	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_IIR,
> +				 BMP580_DSP_IIR_PRESS_MASK |
> +				 BMP580_DSP_IIR_TEMP_MASK, reg_val);

Better to split on logical bounds

	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_IIR,
				 BMP580_DSP_IIR_PRESS_MASK | BMP580_DSP_IIR_TEMP_MASK,
				 reg_val);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/7] iio: pressure: bmp280: Remove config error check for IIR filter updates

[Vasileios Amoiridis]

On Fri, Aug 23, 2024 at 10:15:29PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 08:17:10PM +0200, Vasileios Amoiridis wrote:
> > 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.
> 
> ...
> 
> > +	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_IIR,
> > +				 BMP580_DSP_IIR_PRESS_MASK |
> > +				 BMP580_DSP_IIR_TEMP_MASK, reg_val);
> 
> Better to split on logical bounds
> 
> 	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_IIR,
> 				 BMP580_DSP_IIR_PRESS_MASK | BMP580_DSP_IIR_TEMP_MASK,
> 				 reg_val);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 


This goes beyond the 80 char limit. I know that there is the relaxed
limit of 100 chars but I didn't feel it was more readable like this.
I could definitely use it though, thanks!

Cheers,
Vasilis

Re: [PATCH v3 3/7] iio: pressure: bmp280: Remove config error check for IIR filter updates

[Andy Shevchenko]

On Sat, Aug 24, 2024 at 01:18:06PM +0200, Vasileios Amoiridis wrote:
> On Fri, Aug 23, 2024 at 10:15:29PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:10PM +0200, Vasileios Amoiridis wrote:

...

> > > +	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_IIR,
> > > +				 BMP580_DSP_IIR_PRESS_MASK |
> > > +				 BMP580_DSP_IIR_TEMP_MASK, reg_val);
> > 
> > Better to split on logical bounds
> > 
> > 	ret = regmap_update_bits(data->regmap, BMP580_REG_DSP_IIR,
> > 				 BMP580_DSP_IIR_PRESS_MASK | BMP580_DSP_IIR_TEMP_MASK,
> > 				 reg_val);
> 
> This goes beyond the 80 char limit. I know that there is the relaxed
> limit of 100 chars but I didn't feel it was more readable like this.
> I could definitely use it though, thanks!

The readability has a priority over that limit. That's even mentioned in
the documentation besides the relaxed limit.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/7] 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 | 277 ++++++++++++++++++++++++++---
 drivers/iio/pressure/bmp280.h      |  21 +++
 2 files changed, 278 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 736a1f4fd5dc..e1336aeceec0 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -617,6 +617,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);
@@ -646,6 +654,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);
@@ -989,6 +1005,69 @@ static int bmp280_preinit(struct bmp280_data *data)
 	return 0;
 }
 
+static const u8 bmp280_operation_mode[] = { BMP280_MODE_SLEEP,
+					    BMP280_MODE_FORCED,
+					    BMP280_MODE_NORMAL };
+
+static int bmp280_set_mode(struct bmp280_data *data, enum bmp280_op_mode mode)
+{
+	int ret;
+
+	switch (mode) {
+	case BMP280_SLEEP:
+	case BMP280_FORCED:
+	case BMP280_NORMAL:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
+				BMP280_MODE_MASK, bmp280_operation_mode[mode]);
+	if (ret) {
+		dev_err(data->dev, "failed to  write ctrl_meas register\n");
+		return ret;
+	}
+
+	data->op_mode = mode;
+
+	return 0;
+}
+
+static int bmp280_wait_conv(struct bmp280_data *data)
+{
+	unsigned int reg;
+	int ret, meas_time;
+
+	meas_time = BMP280_MEAS_OFFSET;
+
+	/* Check if we are using a BME280 device */
+	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) |
@@ -999,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;
@@ -1106,6 +1185,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,
@@ -1227,6 +1308,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,
@@ -1514,6 +1597,70 @@ static int bmp380_preinit(struct bmp280_data *data)
 	return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
 }
 
+static const u8 bmp380_operation_mode[] = { BMP380_MODE_SLEEP,
+					    BMP380_MODE_FORCED,
+					    BMP380_MODE_NORMAL };
+
+static int bmp380_set_mode(struct bmp280_data *data, enum bmp280_op_mode mode)
+{
+	int ret;
+
+	switch (mode) {
+	case BMP280_SLEEP:
+	case BMP280_FORCED:
+	case BMP280_NORMAL:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+				BMP380_MODE_MASK,
+				FIELD_PREP(BMP380_MODE_MASK,
+					   bmp380_operation_mode[mode]));
+	if (ret) {
+		dev_err(data->dev, "failed to  write power control register\n");
+		return ret;
+	}
+
+	data->op_mode = mode;
+
+	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)) {
+		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;
@@ -1574,17 +1721,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;
@@ -1610,6 +1755,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;
 }
 
@@ -1703,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,
@@ -2090,6 +2248,66 @@ static int bmp580_preinit(struct bmp280_data *data)
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config));
 }
 
+static const u8 bmp580_operation_mode[] = { BMP580_MODE_SLEEP,
+					    BMP580_MODE_FORCED,
+					    BMP580_MODE_NORMAL };
+
+static int bmp580_set_mode(struct bmp280_data *data, enum bmp280_op_mode mode)
+{
+	int ret;
+
+	switch (mode) {
+	case BMP280_SLEEP:
+		break;
+	case BMP280_FORCED:
+		ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
+				      BMP580_DSP_IIR_FORCED_FLUSH);
+		if (ret) {
+			dev_err(data->dev,
+				"Could not flush IIR filter constants.\n");
+			return ret;
+		}
+		break;
+	case BMP280_NORMAL:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
+				BMP580_MODE_MASK,
+				FIELD_PREP(BMP580_MODE_MASK,
+					   bmp580_operation_mode[mode]));
+	if (ret) {
+		dev_err(data->dev, "failed to  write power control register\n");
+		return ret;
+	}
+
+	data->op_mode = mode;
+
+	return 0;
+}
+
+static int bmp580_wait_conv(struct bmp280_data *data)
+{
+	/*
+	 * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
+	 * characteristics
+	 */
+	static const int time_conv_press[] = { 0, 1050, 1785, 3045, 5670, 10920, 21420,
+					42420, 84420};
+	static 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;
@@ -2160,17 +2378,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
@@ -2266,6 +2473,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,
@@ -2513,6 +2722,19 @@ static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press)
 	return 0;
 }
 
+/* Keep compatibility with newer generations of the sensor */
+static int bmp180_set_mode(struct bmp280_data *data, enum bmp280_op_mode mode)
+{
+	return 0;
+}
+
+/* Keep compatibility with newer generations of the sensor */
+static int bmp180_wait_conv(struct bmp280_data *data)
+{
+	return 0;
+}
+
+/* Keep compatibility with newer generations of the sensor */
 static int bmp180_chip_config(struct bmp280_data *data)
 {
 	return 0;
@@ -2583,6 +2805,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,
 };
@@ -2635,6 +2859,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;
 }
@@ -2805,6 +3030,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);
@@ -2830,6 +3059,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);
 }
 
@@ -2844,7 +3076,12 @@ static int bmp280_runtime_resume(struct device *dev)
 		return ret;
 
 	usleep_range(data->start_up_time, data->start_up_time + 100);
-	return data->chip_info->chip_config(data);
+
+	ret = data->chip_info->chip_config(data);
+	if (ret)
+		return ret;
+
+	return data->chip_info->set_mode(data, data->op_mode);
 }
 
 EXPORT_RUNTIME_DEV_PM_OPS(bmp280_dev_pm_ops, bmp280_runtime_suspend,
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 73516878d020..c9840b8d58b0 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
@@ -384,6 +394,12 @@ struct bmp380_calib {
 	s8  P11;
 };
 
+enum bmp280_op_mode {
+	BMP280_SLEEP,
+	BMP280_FORCED,
+	BMP280_NORMAL,
+};
+
 struct bmp280_data {
 	struct device *dev;
 	struct mutex lock;
@@ -424,6 +440,9 @@ struct bmp280_data {
 		s64 ts __aligned(8);
 	} buffer;
 
+	/* Value to hold the current operation mode of the device */
+	enum bmp280_op_mode op_mode;
+
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
 	 * transfer buffers to live in their own cache lines.
@@ -488,6 +507,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, enum bmp280_op_mode mode);
+	int (*wait_conv)(struct bmp280_data *data);
 
 	irqreturn_t (*trigger_handler)(int irq, void *p);
 };
-- 
2.25.1

Re: [PATCH v3 4/7] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 08:17:11PM +0200, Vasileios Amoiridis 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.

...

> +static const u8 bmp280_operation_mode[] = { BMP280_MODE_SLEEP,
> +					    BMP280_MODE_FORCED,
> +					    BMP280_MODE_NORMAL };

Better style is

static const u8 bmp280_operation_mode[] = {
	BMP280_MODE_SLEEP, BMP280_MODE_FORCED, BMP280_MODE_NORMAL,
};

Also note comma at the end.

...

> +static int bmp280_wait_conv(struct bmp280_data *data)
> +{
> +	unsigned int reg;
> +	int ret, meas_time;
> +
> +	meas_time = BMP280_MEAS_OFFSET;
> +
> +	/* Check if we are using a BME280 device */
> +	if (data->oversampling_humid)
> +		meas_time += (1 << data->oversampling_humid) * BMP280_MEAS_DUR +

		BIT(data->oversampling_humid)

> +			       BMP280_PRESS_HUMID_MEAS_OFFSET;

> +	/* Pressure measurement time */
> +	meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +

Ditto.

> +		      BMP280_PRESS_HUMID_MEAS_OFFSET;

> +	/* Temperature measurement time */
> +	meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;

Ditto.

> +	usleep_range(meas_time, meas_time * 12 / 10);

fsleep() ?

> +	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 const u8 bmp380_operation_mode[] = { BMP380_MODE_SLEEP,
> +					    BMP380_MODE_FORCED,
> +					    BMP380_MODE_NORMAL };

As per above.

...

> +static int bmp380_wait_conv(struct bmp280_data *data)
> +{

As per above comments against bmp280_wait_conv().

> +	ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
> +	if (ret) {
> +		dev_err(data->dev, "failed to read status register\n");
> +		return ret;
> +	}

> +

Choose one style (with or without blank line), as in the above you have no
blank line in the similar situation.

> +	if (!(reg & BMP380_STATUS_DRDY_PRESS_MASK) ||
> +	    !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
> +		dev_err(data->dev, "Measurement cycle didn't complete.\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}

...

> +		usleep_range(data->start_up_time, data->start_up_time + 500);

fsleep() ? Comment?

...

> +static const u8 bmp580_operation_mode[] = { BMP580_MODE_SLEEP,
> +					    BMP580_MODE_FORCED,
> +					    BMP580_MODE_NORMAL };

As per above.

...

> +	switch (mode) {
> +	case BMP280_SLEEP:
> +		break;
> +	case BMP280_FORCED:
> +		ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
> +				      BMP580_DSP_IIR_FORCED_FLUSH);
> +		if (ret) {
> +			dev_err(data->dev,
> +				"Could not flush IIR filter constants.\n");
> +			return ret;
> +		}
> +		break;
> +	case BMP280_NORMAL:
> +		break;

Can be unified with _SLEEP case.

> +	default:
> +		return -EINVAL;
> +	}

...

> +static int bmp580_wait_conv(struct bmp280_data *data)
> +{
> +	/*
> +	 * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
> +	 * characteristics

Missing period.

> +	 */
> +	static const int time_conv_press[] = { 0, 1050, 1785, 3045, 5670, 10920, 21420,
> +					42420, 84420};
> +	static const int time_conv_temp[] = { 0, 1050, 1105, 1575, 2205, 3465, 6090,
> +				       11340, 21840};

Please, start values on the next line after {. Also make }; to be on a separate line.

> +	int meas_time;
> +
> +	meas_time = 4000 + time_conv_temp[data->oversampling_temp] +
> +			   time_conv_press[data->oversampling_press];

4 * USEC_PER_MSEC ?

> +	usleep_range(meas_time, meas_time * 12 / 10);

Comment? fsleep() ?

> +	return 0;
> +}

...

> +	usleep_range(2500, 3000);

fsleep() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Vasileios Amoiridis]

On Fri, Aug 23, 2024 at 10:25:01PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 08:17:11PM +0200, Vasileios Amoiridis 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.
> 
> ...
> 
> > +static const u8 bmp280_operation_mode[] = { BMP280_MODE_SLEEP,
> > +					    BMP280_MODE_FORCED,
> > +					    BMP280_MODE_NORMAL };
> 
> Better style is
> 
> static const u8 bmp280_operation_mode[] = {
> 	BMP280_MODE_SLEEP, BMP280_MODE_FORCED, BMP280_MODE_NORMAL,
> };
> 
> Also note comma at the end.
> 

Looks much better indeed, thanks!

> ...
> 
> > +static int bmp280_wait_conv(struct bmp280_data *data)
> > +{
> > +	unsigned int reg;
> > +	int ret, meas_time;
> > +
> > +	meas_time = BMP280_MEAS_OFFSET;
> > +
> > +	/* Check if we are using a BME280 device */
> > +	if (data->oversampling_humid)
> > +		meas_time += (1 << data->oversampling_humid) * BMP280_MEAS_DUR +
> 
> 		BIT(data->oversampling_humid)

ACK.

> 
> > +			       BMP280_PRESS_HUMID_MEAS_OFFSET;
> 
> > +	/* Pressure measurement time */
> > +	meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +
> 
> Ditto.

ACK.

> 
> > +		      BMP280_PRESS_HUMID_MEAS_OFFSET;
> 
> > +	/* Temperature measurement time */
> > +	meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;
> 
> Ditto.
> 

ACK.

> > +	usleep_range(meas_time, meas_time * 12 / 10);
> 
> fsleep() ?

Could be used indeed. My concern is that fsleep uses a sleep range
between x and 2x but I don't think it is a problem since these are
oneshot captures.

> 
> > +	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 const u8 bmp380_operation_mode[] = { BMP380_MODE_SLEEP,
> > +					    BMP380_MODE_FORCED,
> > +					    BMP380_MODE_NORMAL };
> 
> As per above.
> 

ACK.

> ...
> 
> > +static int bmp380_wait_conv(struct bmp280_data *data)
> > +{
> 
> As per above comments against bmp280_wait_conv().
> 

ACK.

> > +	ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
> > +	if (ret) {
> > +		dev_err(data->dev, "failed to read status register\n");
> > +		return ret;
> > +	}
> 
> > +
> 
> Choose one style (with or without blank line), as in the above you have no
> blank line in the similar situation.
> 

I didn't even notice it, you are right.

> > +	if (!(reg & BMP380_STATUS_DRDY_PRESS_MASK) ||
> > +	    !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
> > +		dev_err(data->dev, "Measurement cycle didn't complete.\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +		usleep_range(data->start_up_time, data->start_up_time + 500);
> 
> fsleep() ? Comment?
> 

I could use fsleep(). I didn't add a comment because also before it was
also like this. The code just used hardcoded (2000,2500) while I used
the data->start_up_time. It is mentioned in the datasheet, I could add it.

> ...
> 
> > +static const u8 bmp580_operation_mode[] = { BMP580_MODE_SLEEP,
> > +					    BMP580_MODE_FORCED,
> > +					    BMP580_MODE_NORMAL };
> 
> As per above.
> 

ACK.

> ...
> 
> > +	switch (mode) {
> > +	case BMP280_SLEEP:
> > +		break;
> > +	case BMP280_FORCED:
> > +		ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
> > +				      BMP580_DSP_IIR_FORCED_FLUSH);
> > +		if (ret) {
> > +			dev_err(data->dev,
> > +				"Could not flush IIR filter constants.\n");
> > +			return ret;
> > +		}
> > +		break;
> > +	case BMP280_NORMAL:
> > +		break;
> 
> Can be unified with _SLEEP case.
> 

ACK.

> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> ...
> 
> > +static int bmp580_wait_conv(struct bmp280_data *data)
> > +{
> > +	/*
> > +	 * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
> > +	 * characteristics
> 
> Missing period.

ACK.

> 
> > +	 */
> > +	static const int time_conv_press[] = { 0, 1050, 1785, 3045, 5670, 10920, 21420,
> > +					42420, 84420};
> > +	static const int time_conv_temp[] = { 0, 1050, 1105, 1575, 2205, 3465, 6090,
> > +				       11340, 21840};
> 
> Please, start values on the next line after {. Also make }; to be on a separate line.
> 

ACK.

> > +	int meas_time;
> > +
> > +	meas_time = 4000 + time_conv_temp[data->oversampling_temp] +
> > +			   time_conv_press[data->oversampling_press];
> 
> 4 * USEC_PER_MSEC ?

Since the previous values in the arrays are all in thousands, why should
I make this different?

> 
> > +	usleep_range(meas_time, meas_time * 12 / 10);
> 
> Comment? fsleep() ?
> 

The usleep here is for waiting for the sensor to make the conversion,
as the function name points out as well? Should I put it as a comment?

In general, is it considered good practice to add comments above all
sleep functions? I don't think it's a bad idea, I just didn't notice
it somewhere.

> > +	return 0;
> > +}
> 
> ...
> 
> > +	usleep_range(2500, 3000);
> 
> fsleep() ?
> 

ACK.

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Cheers,
Vasilis

Re: [PATCH v3 4/7] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Andy Shevchenko]

On Sat, Aug 24, 2024 at 01:29:24PM +0200, Vasileios Amoiridis wrote:
> On Fri, Aug 23, 2024 at 10:25:01PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:11PM +0200, Vasileios Amoiridis wrote:

...

> > > +	meas_time = 4000 + time_conv_temp[data->oversampling_temp] +
> > > +			   time_conv_press[data->oversampling_press];
> > 
> > 4 * USEC_PER_MSEC ?
> 
> Since the previous values in the arrays are all in thousands, why should
> I make this different?

When I read the code (and mind that we write code for humans), I don't
have a clue about the order of the values in use. Also it's hard to get from
the line the meaning of both sides of the formula. Using named definitions
helps a lot in understanding this line without reading and analysing code in
full.

...

> > > +	usleep_range(meas_time, meas_time * 12 / 10);
> > 
> > Comment? fsleep() ?
> 
> The usleep here is for waiting for the sensor to make the conversion,
> as the function name points out as well? Should I put it as a comment?
> 
> In general, is it considered good practice to add comments above all
> sleep functions?

Yes, it's even a requirement (not sure if it's documented anywhere) to comment
over long enough delays.

> I don't think it's a bad idea, I just didn't notice
> it somewhere.
> 
> > > +	return 0;
> > > +}

...

> > > +	usleep_range(2500, 3000);
> > 
> > fsleep() ?
> > 
> 
> ACK.

Also a comment, since it's milliseconds range which might be considered long
enough.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/7] 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>
---
 Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
index 6fda887ee9d4..eb1e1ab3dd18 100644
--- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
@@ -48,9 +48,14 @@ properties:
 
   interrupts:
     description:
-      interrupt mapping for IRQ (BMP085 only)
+      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
     maxItems: 1
 
+  drive-open-drain:
+    description:
+      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 v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Biju Das]

Hi Vasileios Amoiridis,

Thanks for the patch.

> -----Original Message-----
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> Sent: Friday, August 23, 2024 7:17 PM
> Subject: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx
> devices
> 
> Add interrupt options for BMP3xx and BMP5xx devices as well.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> index 6fda887ee9d4..eb1e1ab3dd18 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> @@ -48,9 +48,14 @@ properties:
> 
>    interrupts:
>      description:
> -      interrupt mapping for IRQ (BMP085 only)
> +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx

Since you have updated the description. It is better to enforce the same in
conditional schema?? So that dt binding check throws error if interrupt
used in bmp{180,280 and bme280}.

Cheers,
Biju

>      maxItems: 1
> 
> +  drive-open-drain:
> +    description:
> +      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 v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Vasileios Amoiridis]

On Fri, Aug 23, 2024 at 06:51:55PM +0000, Biju Das wrote:
> Hi Vasileios Amoiridis,
> 
> Thanks for the patch.
> 
> > -----Original Message-----
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > Sent: Friday, August 23, 2024 7:17 PM
> > Subject: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx
> > devices
> > 
> > Add interrupt options for BMP3xx and BMP5xx devices as well.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > index 6fda887ee9d4..eb1e1ab3dd18 100644
> > --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > @@ -48,9 +48,14 @@ properties:
> > 
> >    interrupts:
> >      description:
> > -      interrupt mapping for IRQ (BMP085 only)
> > +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
> 
> Since you have updated the description. It is better to enforce the same in
> conditional schema?? So that dt binding check throws error if interrupt
> used in bmp{180,280 and bme280}.
> 
> Cheers,
> Biju
> 

Hi Biju,

Thanks for the feedback! It is true that it would be good to throw an
error in case the IRQ is used in a not supported sensor. If you could
point me to an example of another sensor implementing it, it would be
even more helpful, but I am sure I will find something :)

Cheers,
Vasilis

> >      maxItems: 1
> > 
> > +  drive-open-drain:
> > +    description:
> > +      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 v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Biju Das]

Hi Vasileios Amoiridis,

> -----Original Message-----
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> Sent: Saturday, August 24, 2024 12:31 PM
> Subject: Re: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx
> devices
> 
> On Fri, Aug 23, 2024 at 06:51:55PM +0000, Biju Das wrote:
> > Hi Vasileios Amoiridis,
> >
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > Sent: Friday, August 23, 2024 7:17 PM
> > > Subject: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add
> > > interrupts for BMP3xx and BMP5xx devices
> > >
> > > Add interrupt options for BMP3xx and BMP5xx devices as well.
> > >
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > ---
> > >  Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7
> > > ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > index 6fda887ee9d4..eb1e1ab3dd18 100644
> > > --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > @@ -48,9 +48,14 @@ properties:
> > >
> > >    interrupts:
> > >      description:
> > > -      interrupt mapping for IRQ (BMP085 only)
> > > +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx,
> > > + BMP5xx
> >
> > Since you have updated the description. It is better to enforce the
> > same in conditional schema?? So that dt binding check throws error if
> > interrupt used in bmp{180,280 and bme280}.
> >
> > Cheers,
> > Biju
> >
> 
> Hi Biju,
> 
> Thanks for the feedback! It is true that it would be good to throw an error in case the IRQ is used in
> a not supported sensor. If you could point me to an example of another sensor implementing it, it
> would be even more helpful, but I am sure I will find something :)

As Krzysztof Kozlowski mentioned it depends upon driver(s/w) or device(H/W).

if it is driver(s/w), then you don't need conditional check as bindings describes
hardware.

There are plenty of examples for allOf:if:then: 

Cheers,
Biju

> 
> Cheers,
> Vasilis
> 
> > >      maxItems: 1
> > >
> > > +  drive-open-drain:
> > > +    description:
> > > +      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 v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Vasileios Amoiridis]

On Sat, Aug 24, 2024 at 11:41:26AM +0000, Biju Das wrote:
> Hi Vasileios Amoiridis,
> 
> > -----Original Message-----
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > Sent: Saturday, August 24, 2024 12:31 PM
> > Subject: Re: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx
> > devices
> > 
> > On Fri, Aug 23, 2024 at 06:51:55PM +0000, Biju Das wrote:
> > > Hi Vasileios Amoiridis,
> > >
> > > Thanks for the patch.
> > >
> > > > -----Original Message-----
> > > > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > Sent: Friday, August 23, 2024 7:17 PM
> > > > Subject: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add
> > > > interrupts for BMP3xx and BMP5xx devices
> > > >
> > > > Add interrupt options for BMP3xx and BMP5xx devices as well.
> > > >
> > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7
> > > > ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > > b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > > index 6fda887ee9d4..eb1e1ab3dd18 100644
> > > > --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > > > @@ -48,9 +48,14 @@ properties:
> > > >
> > > >    interrupts:
> > > >      description:
> > > > -      interrupt mapping for IRQ (BMP085 only)
> > > > +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx,
> > > > + BMP5xx
> > >
> > > Since you have updated the description. It is better to enforce the
> > > same in conditional schema?? So that dt binding check throws error if
> > > interrupt used in bmp{180,280 and bme280}.
> > >
> > > Cheers,
> > > Biju
> > >
> > 
> > Hi Biju,
> > 
> > Thanks for the feedback! It is true that it would be good to throw an error in case the IRQ is used in
> > a not supported sensor. If you could point me to an example of another sensor implementing it, it
> > would be even more helpful, but I am sure I will find something :)
> 
> As Krzysztof Kozlowski mentioned it depends upon driver(s/w) or device(H/W).
> 
> if it is driver(s/w), then you don't need conditional check as bindings describes
> hardware.
> 
> There are plenty of examples for allOf:if:then: 
> 
> Cheers,
> Biju
> 

Hi Biju,

Indeed, I checked Krzysztof's mail later. I will implement it like this
since it is a device(H/W) issue.

Cheers,
Vasilis

> > 
> > Cheers,
> > Vasilis
> > 
> > > >      maxItems: 1
> > > >
> > > > +  drive-open-drain:
> > > > +    description:
> > > > +      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 v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Krzysztof Kozlowski]

On Fri, Aug 23, 2024 at 08:17:12PM +0200, Vasileios Amoiridis wrote:
> Add interrupt options for BMP3xx and BMP5xx devices as well.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> index 6fda887ee9d4..eb1e1ab3dd18 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> @@ -48,9 +48,14 @@ properties:
>  
>    interrupts:
>      description:
> -      interrupt mapping for IRQ (BMP085 only)
> +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx

Supported by driver or device?
If the latter, this should be constrained per device variant in
allOf:if:then:.


>      maxItems: 1
>  
> +  drive-open-drain:

Missing type, unless some other core schema defined it? But then I
actually wonder if we need it.  Maybe this should be interrupt flag?
Just like GPIO has such.

Best regards,
Krzysztof

Re: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Vasileios Amoiridis]

On Sat, Aug 24, 2024 at 09:45:43AM +0200, Krzysztof Kozlowski wrote:
> On Fri, Aug 23, 2024 at 08:17:12PM +0200, Vasileios Amoiridis wrote:
> > Add interrupt options for BMP3xx and BMP5xx devices as well.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > index 6fda887ee9d4..eb1e1ab3dd18 100644
> > --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
> > @@ -48,9 +48,14 @@ properties:
> >  
> >    interrupts:
> >      description:
> > -      interrupt mapping for IRQ (BMP085 only)
> > +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
> 
> Supported by driver or device?
> If the latter, this should be constrained per device variant in
> allOf:if:then:.
> 

Hi Krzysztof,

Supported by some devices controlled by the same (just 1) driver.
Thanks for the hint, I will take a look how other drivers do it :)

> 
> >      maxItems: 1
> >  
> > +  drive-open-drain:
> 
> Missing type, unless some other core schema defined it? But then I
> actually wonder if we need it.  Maybe this should be interrupt flag?
> Just like GPIO has such.

I took it from the bindings/iio/imu/bosch,bmi323.yaml example which is
the same. You think something needs to change?

Cheers,
Vasilis

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 5/7] dt-bindings: iio: pressure: bmp085: Add interrupts for BMP3xx and BMP5xx devices

[Krzysztof Kozlowski]

On 24/08/2024 13:35, Vasileios Amoiridis wrote:
> On Sat, Aug 24, 2024 at 09:45:43AM +0200, Krzysztof Kozlowski wrote:
>> On Fri, Aug 23, 2024 at 08:17:12PM +0200, Vasileios Amoiridis wrote:
>>> Add interrupt options for BMP3xx and BMP5xx devices as well.
>>>
>>> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
>>> index 6fda887ee9d4..eb1e1ab3dd18 100644
>>> --- a/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/pressure/bmp085.yaml
>>> @@ -48,9 +48,14 @@ properties:
>>>  
>>>    interrupts:
>>>      description:
>>> -      interrupt mapping for IRQ (BMP085 only)
>>> +      interrupt mapping for IRQ. Supported in BMP085, BMP3xx, BMP5xx
>>
>> Supported by driver or device?
>> If the latter, this should be constrained per device variant in
>> allOf:if:then:.
>>
> 
> Hi Krzysztof,
> 
> Supported by some devices controlled by the same (just 1) driver.
> Thanks for the hint, I will take a look how other drivers do it :)
> 
>>
>>>      maxItems: 1
>>>  
>>> +  drive-open-drain:
>>
>> Missing type, unless some other core schema defined it? But then I
>> actually wonder if we need it.  Maybe this should be interrupt flag?
>> Just like GPIO has such.
> 
> I took it from the bindings/iio/imu/bosch,bmi323.yaml example which is
> the same. You think something needs to change?
> 

You need type: boolean.

Best regards,
Krzysztof

[PATCH v3 6/7] 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 | 252 ++++++++++++++++++++++++++++-
 drivers/iio/pressure/bmp280.h      |  21 +++
 2 files changed, 269 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index e1336aeceec0..f5268a13bfdb 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>
 
@@ -1271,6 +1273,74 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
+				  const struct iio_trigger_ops *trigger_ops,
+				  int (*int_config)(struct bmp280_data *data),
+				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
+{
+	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(fwnode, 0);
+	if (!irq)
+		return dev_err_probe(data->dev, -ENODEV,
+				     "No interrupt found.\n");
+
+	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:
+		return dev_err_probe(data->dev, -EINVAL,
+				     "Invalid interrupt type specified.\n");
+	}
+
+	data->trig_open_drain = fwnode_property_read_bool(fwnode,
+							  "int-open-drain");
+
+	ret = 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 = trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_threaded_irq(data->dev, irq, NULL,
+					irq_thread_handler, IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "request irq failed.\n");
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio trigger register failed.\n");
+
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	return 0;
+}
+
 static const u8 bme280_chip_ids[] = { BME280_CHIP_ID };
 static const int bme280_humid_coeffs[] = { 1000, 1024 };
 
@@ -1769,6 +1839,85 @@ 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_CONTROL,
+				 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_CONTROL,
+				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
+	if (ret) {
+		dev_err(data->dev, "Could not set interrupt settings\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static irqreturn_t bmp380_irq_thread_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct bmp280_data *data = iio_priv(indio_dev);
+	unsigned int int_ctrl;
+	int ret;
+
+	scoped_guard(mutex, &data->lock) {
+		ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &int_ctrl);
+		if (ret)
+			return IRQ_NONE;
+	}
+
+	if (FIELD_GET(BMP380_INT_STATUS_DRDY, int_ctrl))
+		iio_trigger_poll_nested(data->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int bmp380_trigger_probe(struct iio_dev *indio_dev)
+{
+	return __bmp280_trigger_probe(indio_dev, &bmp380_trigger_ops,
+				      bmp380_int_config,
+				      bmp380_irq_thread_handler);
+}
+
 static irqreturn_t bmp380_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1863,6 +2012,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);
@@ -2401,6 +2551,92 @@ 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 irqreturn_t bmp580_irq_thread_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct bmp280_data *data = iio_priv(indio_dev);
+	unsigned int int_ctrl;
+	int ret;
+
+	scoped_guard(mutex, &data->lock) {
+		ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
+		if (ret)
+			return IRQ_NONE;
+	}
+
+	if (FIELD_GET(BMP580_INT_STATUS_DRDY_MASK, int_ctrl))
+		iio_trigger_poll_nested(data->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int bmp580_trigger_probe(struct iio_dev *indio_dev)
+{
+	return __bmp280_trigger_probe(indio_dev, &bmp580_trigger_ops,
+				      bmp580_int_config,
+				      bmp580_irq_thread_handler);
+}
+
 static irqreturn_t bmp580_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -2477,6 +2713,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);
@@ -3024,10 +3261,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.h b/drivers/iio/pressure/bmp280.h
index c9840b8d58b0..0c32b6430677 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)
@@ -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
@@ -406,6 +423,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;
@@ -510,6 +530,7 @@ struct bmp280_chip_info {
 	int (*set_mode)(struct bmp280_data *data, enum bmp280_op_mode 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

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis 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.

...

> +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> +				  const struct iio_trigger_ops *trigger_ops,
> +				  int (*int_config)(struct bmp280_data *data),

> +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))

irq_handler_t

...

> +	fwnode = dev_fwnode(data->dev);
> +	if (!fwnode)
> +		return -ENODEV;

Why do you need this? The below will fail anyway.

> +	irq = fwnode_irq_get(fwnode, 0);
> +	if (!irq)

Are you sure this is correct check?

> +		return dev_err_probe(data->dev, -ENODEV,

Shadowed error code.

> +				     "No interrupt found.\n");

> +	desc = irq_get_irq_data(irq);
> +	if (!desc)
> +		return -EINVAL;

When may this fail?

> +	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:
> +		return dev_err_probe(data->dev, -EINVAL,
> +				     "Invalid interrupt type specified.\n");
> +	}

> +	data->trig_open_drain = fwnode_property_read_bool(fwnode,
> +							  "int-open-drain");

Better

	data->trig_open_drain =
		fwnode_property_read_bool(fwnode, "int-open-drain");

...

> +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_CONTROL,
> +				 BMP380_INT_CTRL_DRDY_EN,
> +				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
> +					    state ? 1 : 0));

				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, !!state));

? ( Even <= 80 characters)

> +	if (ret) {
> +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> +		return ret;
> +	}
> +
> +	return 0;

	if (ret)
		dev_err(data->dev, "Could not enable/disable interrupt\n");

	return ret;

?

> +}

...

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

Split these two variables and make the indentation better for int_cfg.

> +	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL,
> +				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
> +	if (ret) {
> +		dev_err(data->dev, "Could not set interrupt settings\n");

> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

?

> +}

...

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

!!state ?

> +	if (ret) {
> +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

?

> +}

...

> +static int bmp580_int_config(struct bmp280_data *data)

Same comments as per above.

...

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

Can be

	if (irq > 0) {
		if (chip_id == BMP180_CHIP_ID)
			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
		if (data->chip_info->trigger_probe)
			ret = data->chip_info->trigger_probe(indio_dev);
		if (ret)
			return ret;
	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Vasileios Amoiridis]

On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis 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.
> 
> ...
> 
> > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > +				  const struct iio_trigger_ops *trigger_ops,
> > +				  int (*int_config)(struct bmp280_data *data),
> 
> > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
> 
> irq_handler_t
> 

But the function returns an irqreturn_t type, no?

> ...
> 
> > +	fwnode = dev_fwnode(data->dev);
> > +	if (!fwnode)
> > +		return -ENODEV;
> 
> Why do you need this? The below will fail anyway.

Because If I don't make this check then fwnode might be garbage and I will
pass garbage to the fwnode_irq_get() function. Or do I miss something?

> 
> > +	irq = fwnode_irq_get(fwnode, 0);
> > +	if (!irq)
> 
> Are you sure this is correct check?
> 
Well, I think yes, because the function return either the Linux IRQ number
on success or a negative errno on failure.

https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987

> > +		return dev_err_probe(data->dev, -ENODEV,
> 
> Shadowed error code.
> 

I am not sure I understand what you mean here. You mean that there is no
chance that the first one will pass and this one will fail?

> > +				     "No interrupt found.\n");
> 
> > +	desc = irq_get_irq_data(irq);
> > +	if (!desc)
> > +		return -EINVAL;
> 
> When may this fail?
> 

I think that this will fail when Linux were not able to actually
register that interrupt.

> > +	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:
> > +		return dev_err_probe(data->dev, -EINVAL,
> > +				     "Invalid interrupt type specified.\n");
> > +	}
> 
> > +	data->trig_open_drain = fwnode_property_read_bool(fwnode,
> > +							  "int-open-drain");
> 
> Better
> 
> 	data->trig_open_drain =
> 		fwnode_property_read_bool(fwnode, "int-open-drain");
> 

Indeed, thanks!

> ...
> 
> > +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_CONTROL,
> > +				 BMP380_INT_CTRL_DRDY_EN,
> > +				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
> > +					    state ? 1 : 0));
> 
> 				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, !!state));
> 
> ? ( Even <= 80 characters)

Well, that's true.

> 
> > +	if (ret) {
> > +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> 
> 	if (ret)
> 		dev_err(data->dev, "Could not enable/disable interrupt\n");
> 
> 	return ret;
> 
> ?

All the other if statements follow the style that I typed. If I
follow yours, will make it different just for this one, does it
make sense?

Cheers,
Vasilis
> 
> > +}
> 
> ...
> 
> > +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);
> 
> Split these two variables and make the indentation better for int_cfg.
> 

True, makes sense.

> > +	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL,
> > +				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
> > +	if (ret) {
> > +		dev_err(data->dev, "Could not set interrupt settings\n");
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> 
> 	return ret;
> 
> ?

Yes, you are right.

> 
> > +}
> 
> ...
> 
> > +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));
> 
> !!state ?
> 

ACK.

> > +	if (ret) {
> > +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> 
> 	return ret;
> 
> ?
> 
> > +}
> 
> ...
> 
> > +static int bmp580_int_config(struct bmp280_data *data)
> 
> Same comments as per above.
> 
> ...
> 
> > +	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;
> > +		}
> >  	}
> 
> Can be
> 
> 	if (irq > 0) {
> 		if (chip_id == BMP180_CHIP_ID)
> 			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
> 		if (data->chip_info->trigger_probe)
> 			ret = data->chip_info->trigger_probe(indio_dev);
> 		if (ret)
> 			return ret;
> 	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Well, it looks much more beautiful indeed. Thanks again for the feedback
Andy, I really appreciate it a lot!

Cheers,
Vasilis

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Jonathan Cameron]

On Sat, 24 Aug 2024 14:02:22 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis 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.  
> > 
> > ...
> >   
> > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > > +				  const struct iio_trigger_ops *trigger_ops,
> > > +				  int (*int_config)(struct bmp280_data *data),  
> >   
> > > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))  
> > 
> > irq_handler_t
> >   
> 
> But the function returns an irqreturn_t type, no?
irq_handler_t is a typdef for the full function signature.
It will still return irqreturn_t 
> 
> > ...
> >   
> > > +	fwnode = dev_fwnode(data->dev);
> > > +	if (!fwnode)
> > > +		return -ENODEV;  
> > 
> > Why do you need this? The below will fail anyway.  
> 
> Because If I don't make this check then fwnode might be garbage and I will
> pass garbage to the fwnode_irq_get() function. Or do I miss something?
It checks for NULL which is all it can actually be and returns a suitable
error code if it is.

> 
> >   
> > > +	irq = fwnode_irq_get(fwnode, 0);
> > > +	if (!irq)  
> > 
> > Are you sure this is correct check?
> >   
> Well, I think yes, because the function return either the Linux IRQ number
> on success or a negative errno on failure.
> 
> https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987

Indeed, so if (irq < 0)
		return dev_err_probe(data->dev, irq, ...)

	carry on as valid irq.
your error check if returning only if irq == 0 which never
happens (due to catch for that in the code you link).
Negative values are true, so !-EINVAL == false
for example.
> 

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Andy Shevchenko]

On Mon, Aug 26, 2024 at 11:01:50AM +0100, Jonathan Cameron wrote:
> On Sat, 24 Aug 2024 14:02:22 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:  

...

> > > > +	fwnode = dev_fwnode(data->dev);
> > > > +	if (!fwnode)
> > > > +		return -ENODEV;  
> > > 
> > > Why do you need this? The below will fail anyway.  
> > 
> > Because If I don't make this check then fwnode might be garbage and I will
> > pass garbage to the fwnode_irq_get() function. Or do I miss something?
> It checks for NULL which is all it can actually be and returns a suitable
> error code if it is.

Actually not. It may be NULL, error pointer, or valid. So, for a bare minimum
this check is not full (and again, fwnode APIs should validate fwnode before
accessing them where it makes sense; if fwnode_irq_get() does not do that or
misses the case(s), it has to be improved).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Andy Shevchenko]

On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:

...

> > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > > +				  const struct iio_trigger_ops *trigger_ops,
> > > +				  int (*int_config)(struct bmp280_data *data),
> > 
> > > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
> > 
> > irq_handler_t
> 
> But the function returns an irqreturn_t type, no?

The type of the last parameter is irq_handler_t, no need to open code it.

...

> > > +	fwnode = dev_fwnode(data->dev);
> > > +	if (!fwnode)
> > > +		return -ENODEV;
> > 
> > Why do you need this? The below will fail anyway.
> 
> Because If I don't make this check then fwnode might be garbage and I will
> pass garbage to the fwnode_irq_get() function. Or do I miss something?

Yes, the function validates fwnode before use. So, please drop unneeded (or
even duplicate) check.

...

> > > +	irq = fwnode_irq_get(fwnode, 0);
> > > +	if (!irq)
> > 
> > Are you sure this is correct check?
> > 
> Well, I think yes, because the function return either the Linux IRQ number
> on success or a negative errno on failure.

Where is 0 mentioned in this?

> https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> 
> > > +		return dev_err_probe(data->dev, -ENODEV,
> > 
> > Shadowed error code.
> 
> I am not sure I understand what you mean here. You mean that there is no
> chance that the first one will pass and this one will fail?

-ENODEV is not what fwnode_irq_get() returns on error.

> > > +				     "No interrupt found.\n");

...

> > > +	desc = irq_get_irq_data(irq);
> > > +	if (!desc)
> > > +		return -EINVAL;
> > 
> > When may this fail?
> 
> I think that this will fail when Linux were not able to actually
> register that interrupt.

Wouldn't fwnode_irq_get() fail already?

...

> > 	if (ret)
> > 		dev_err(data->dev, "Could not enable/disable interrupt\n");

Btw you may use str_enable_disable() here.

> > 	return ret;
> > 
> > ?
> 
> All the other if statements follow the style that I typed. If I
> follow yours, will make it different just for this one, does it
> make sense?

When a comment is given, it's assumed that the _full_ patch (or patch series)
should be revisited for it. Or should I add to every comment something like
this:

"Please, check the entire code for the same or similar case and amend
accordingly."

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Vasileios Amoiridis]

On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:
> 
> ...
> 
> > > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > > > +				  const struct iio_trigger_ops *trigger_ops,
> > > > +				  int (*int_config)(struct bmp280_data *data),
> > > 
> > > > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))
> > > 
> > > irq_handler_t
> > 
> > But the function returns an irqreturn_t type, no?
> 
> The type of the last parameter is irq_handler_t, no need to open code it.
> 
> ...
> 
> > > > +	fwnode = dev_fwnode(data->dev);
> > > > +	if (!fwnode)
> > > > +		return -ENODEV;
> > > 
> > > Why do you need this? The below will fail anyway.
> > 
> > Because If I don't make this check then fwnode might be garbage and I will
> > pass garbage to the fwnode_irq_get() function. Or do I miss something?
> 
> Yes, the function validates fwnode before use. So, please drop unneeded (or
> even duplicate) check.
> 
> ...
> 
> > > > +	irq = fwnode_irq_get(fwnode, 0);
> > > > +	if (!irq)
> > > 
> > > Are you sure this is correct check?
> > > 
> > Well, I think yes, because the function return either the Linux IRQ number
> > on success or a negative errno on failure.
> 
> Where is 0 mentioned in this?
> 
> > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> > 
> > > > +		return dev_err_probe(data->dev, -ENODEV,
> > > 
> > > Shadowed error code.
> > 
> > I am not sure I understand what you mean here. You mean that there is no
> > chance that the first one will pass and this one will fail?
> 
> -ENODEV is not what fwnode_irq_get() returns on error.
> 
> > > > +				     "No interrupt found.\n");
> 
> ...
> 
> > > > +	desc = irq_get_irq_data(irq);
> > > > +	if (!desc)
> > > > +		return -EINVAL;
> > > 
> > > When may this fail?
> > 
> > I think that this will fail when Linux were not able to actually
> > register that interrupt.
> 
> Wouldn't fwnode_irq_get() fail already?
> 

Hi Andy,

By looking at it again, I didn't reply correct here. This function
internally calls the irq_to_desc() which basically returns the
irq desctiptor for this irq. This function can return NULL in
case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ)
or in case the interrupt number is bigger than the NR_IRQs which
the irq controller can handle (!CONFIG_SPARSE_IRQ).

So in my opinion, it makes sense to keep this check.

Cheers,
Vasilis

https://elixir.bootlin.com/linux/v6.10.6/source/kernel/irq/chip.c#L155

> ...
> 
> > > 	if (ret)
> > > 		dev_err(data->dev, "Could not enable/disable interrupt\n");
> 
> Btw you may use str_enable_disable() here.
> 
> > > 	return ret;
> > > 
> > > ?
> > 
> > All the other if statements follow the style that I typed. If I
> > follow yours, will make it different just for this one, does it
> > make sense?
> 
> When a comment is given, it's assumed that the _full_ patch (or patch series)
> should be revisited for it. Or should I add to every comment something like
> this:
> 
> "Please, check the entire code for the same or similar case and amend
> accordingly."
> 
> ?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Andy Shevchenko]

On Wed, Aug 28, 2024 at 04:01:19PM +0200, Vasileios Amoiridis wrote:
> On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote:
> > On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> > > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:

...

> > > > > +	irq = fwnode_irq_get(fwnode, 0);
> > > > > +	if (!irq)
> > > > 
> > > > Are you sure this is correct check?
> > > > 
> > > Well, I think yes, because the function return either the Linux IRQ number
> > > on success or a negative errno on failure.
> > 
> > Where is 0 mentioned in this?
> > 
> > > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> > > 
> > > > > +		return dev_err_probe(data->dev, -ENODEV,
> > > > 
> > > > Shadowed error code.
> > > 
> > > I am not sure I understand what you mean here. You mean that there is no
> > > chance that the first one will pass and this one will fail?
> > 
> > -ENODEV is not what fwnode_irq_get() returns on error.
> > 
> > > > > +				     "No interrupt found.\n");

...

> > > > > +	desc = irq_get_irq_data(irq);
> > > > > +	if (!desc)
> > > > > +		return -EINVAL;
> > > > 
> > > > When may this fail?
> > > 
> > > I think that this will fail when Linux were not able to actually
> > > register that interrupt.
> > 
> > Wouldn't fwnode_irq_get() fail already?
> 
> By looking at it again, I didn't reply correct here. This function
> internally calls the irq_to_desc() which basically returns the
> irq desctiptor for this irq. This function can return NULL in
> case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ)
> or in case the interrupt number is bigger than the NR_IRQs which
> the irq controller can handle (!CONFIG_SPARSE_IRQ).
> 
> So in my opinion, it makes sense to keep this check.

So, you mean that if fwnode_irq_get() succeeded there is a chance that returned
Linux IRQ number is invalid?! If it's so, it's something new to me. I would like
to see the details, please!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

[Vasileios Amoiridis]

On Wed, Aug 28, 2024 at 05:17:40PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 28, 2024 at 04:01:19PM +0200, Vasileios Amoiridis wrote:
> > On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote:
> > > On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote:
> > > > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:
> 
> ...
> 
> > > > > > +	irq = fwnode_irq_get(fwnode, 0);
> > > > > > +	if (!irq)
> > > > > 
> > > > > Are you sure this is correct check?
> > > > > 
> > > > Well, I think yes, because the function return either the Linux IRQ number
> > > > on success or a negative errno on failure.
> > > 
> > > Where is 0 mentioned in this?
> > > 
> > > > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987
> > > > 
> > > > > > +		return dev_err_probe(data->dev, -ENODEV,
> > > > > 
> > > > > Shadowed error code.
> > > > 
> > > > I am not sure I understand what you mean here. You mean that there is no
> > > > chance that the first one will pass and this one will fail?
> > > 
> > > -ENODEV is not what fwnode_irq_get() returns on error.
> > > 
> > > > > > +				     "No interrupt found.\n");
> 
> ...
> 
> > > > > > +	desc = irq_get_irq_data(irq);
> > > > > > +	if (!desc)
> > > > > > +		return -EINVAL;
> > > > > 
> > > > > When may this fail?
> > > > 
> > > > I think that this will fail when Linux were not able to actually
> > > > register that interrupt.
> > > 
> > > Wouldn't fwnode_irq_get() fail already?
> > 
> > By looking at it again, I didn't reply correct here. This function
> > internally calls the irq_to_desc() which basically returns the
> > irq desctiptor for this irq. This function can return NULL in
> > case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ)
> > or in case the interrupt number is bigger than the NR_IRQs which
> > the irq controller can handle (!CONFIG_SPARSE_IRQ).
> > 
> > So in my opinion, it makes sense to keep this check.
> 
> So, you mean that if fwnode_irq_get() succeeded there is a chance that returned
> Linux IRQ number is invalid?! If it's so, it's something new to me. I would like
> to see the details, please!
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Hi Andy,

I did some more digging, and from my understanding, fwnode_irq_get()
directly returns the Linux IRQ which means that there should already
exist the irq_desc structure that is returned by the irq_to_desc().

So as you already said, I cannot see how this function can fail, if
the fwnode_irq_get() has succeeded. Thanks for asking the right
questions, I am getting to know things better.

Cheers,
Vasilis

[PATCH v3 7/7] 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 | 68 ++++++++++++++++++++++--------
 drivers/iio/pressure/bmp280-i2c.c  |  4 +-
 drivers/iio/pressure/bmp280-spi.c  |  4 +-
 drivers/iio/pressure/bmp280.h      |  1 +
 4 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index f5268a13bfdb..1d27777d8a2c 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -3058,13 +3058,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) {
@@ -3074,13 +3080,8 @@ 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");
@@ -3091,6 +3092,44 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
 	return 0;
 }
 
+/* Identical to bmp180_chip_info + bmp085_trigger_probe */
+const struct bmp280_chip_info bmp085_chip_info = {
+	.id_reg = BMP280_REG_ID,
+	.chip_id = bmp180_chip_ids,
+	.num_chip_id = ARRAY_SIZE(bmp180_chip_ids),
+	.regmap_config = &bmp180_regmap_config,
+	.start_up_time = 2000,
+	.channels = bmp280_channels,
+	.num_channels = ARRAY_SIZE(bmp280_channels),
+	.avail_scan_masks = bmp280_avail_scan_masks,
+
+	.oversampling_temp_avail = bmp180_oversampling_temp_avail,
+	.num_oversampling_temp_avail =
+		ARRAY_SIZE(bmp180_oversampling_temp_avail),
+	.oversampling_temp_default = 0,
+
+	.oversampling_press_avail = bmp180_oversampling_press_avail,
+	.num_oversampling_press_avail =
+		ARRAY_SIZE(bmp180_oversampling_press_avail),
+	.oversampling_press_default = BMP180_MEAS_PRESS_8X,
+
+	.temp_coeffs = bmp180_temp_coeffs,
+	.temp_coeffs_type = IIO_VAL_FRACTIONAL,
+	.press_coeffs = bmp180_press_coeffs,
+	.press_coeffs_type = IIO_VAL_FRACTIONAL,
+
+	.chip_config = bmp180_chip_config,
+	.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_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);
@@ -3262,11 +3301,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 0c32b6430677..e0e47bf22472 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -535,6 +535,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 v3 7/7] iio: pressure: bmp280: Move bmp085 interrupt to new configuration

[Jonathan Cameron]

On Fri, 23 Aug 2024 20:17:14 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

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

interface of the driver for consistence

> 
> 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 | 68 ++++++++++++++++++++++--------
>  drivers/iio/pressure/bmp280-i2c.c  |  4 +-
>  drivers/iio/pressure/bmp280-spi.c  |  4 +-
>  drivers/iio/pressure/bmp280.h      |  1 +
>  4 files changed, 56 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index f5268a13bfdb..1d27777d8a2c 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -3058,13 +3058,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);

Should check this + as Andy pointed out in earlier review this
call will return an error if fwnode not present anyway so can skip
earlier check.
see fwnode_call_int_op() definition.

Otherwise, Andy has given some detailed review. I wouldn't suggest
applying the style cleanup to existing code but he's entirely
correct that we can make the stuff being touched anyway easier
to read.

The more functional stuff maybe needs to be looked at in other
drivers though.

Jonathan

Re: [PATCH v3 7/7] iio: pressure: bmp280: Move bmp085 interrupt to new configuration

[Vasileios Amoiridis]

On Sat, Aug 24, 2024 at 11:02:42AM +0100, Jonathan Cameron wrote:
> On Fri, 23 Aug 2024 20:17:14 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > This commit intends to add the old BMP085 sensor to the new IRQ interface
> > of the sensor consistence. No functional changes intended.
> 
> interface of the driver for consistence
>
 
ACK.

> > 
> > 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 | 68 ++++++++++++++++++++++--------
> >  drivers/iio/pressure/bmp280-i2c.c  |  4 +-
> >  drivers/iio/pressure/bmp280-spi.c  |  4 +-
> >  drivers/iio/pressure/bmp280.h      |  1 +
> >  4 files changed, 56 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f5268a13bfdb..1d27777d8a2c 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -3058,13 +3058,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);
> 
> Should check this + as Andy pointed out in earlier review this
> call will return an error if fwnode not present anyway so can skip
> earlier check.
> see fwnode_call_int_op() definition.
> 
> Otherwise, Andy has given some detailed review. I wouldn't suggest
> applying the style cleanup to existing code but he's entirely
> correct that we can make the stuff being touched anyway easier
> to read.
> 
> The more functional stuff maybe needs to be looked at in other
> drivers though.
> 
> Jonathan


Hi Jonathan,

Thanks for the feedback. Indeed Andy has given a very detailed
review feedback, I will also check what you said.

Cheers,
Vasilis