Add support for pressure sensor Bosch BMP380

PATCH Superseded Angel Iglesias 4 participants 22 messages Aug 7, 2022 – Sep 15, 2022

Subsystems (2)

Device tree bindings IIO

lore

Share & tools

lore

Angel Iglesias

author

2022-08-07 11:54 UTC #0 lore raw

v5 cover Add support for pressure sensor Bosch BMP380

This patchset adds BMP380 variant to the already existing drivers for the Bosch BMP180/280 pressure sensors.

Patch 1 is a minor refactor simplifying driver initialization logic to facilitate the integration of the new sensor variant extending the information stored in the "chip_info" struct.
Patch 2 fixes DMA unsafe regmap_bulk_* calls reported by Jonathan Cameron <[email protected]>.
Patch 3 adds the basic logic to initialize and read measurements from the sensor.
Patch 4 adds references and sensor id to the devicetree bindings docs. Patch 5 adds advanced configurable features such as sampling frequency and IIR filter through the IIO sysfs ABI.

Changes in v5:
- Moved dt bindings patch (previously patch 2/5) to 4/5 to preserve tree coherence as suggested by Jonathan Cameron <[email protected]>
- Patch 1: Simplified initial oversampling values assignation.
- Patch 1: Updated codepaths for bmp180 and bmp280 to use FIELD_GET and FIELD_GET helpers. Migrated compatible masks to use GENMASK.
- Patch 2: Store DMA-safe buffers on the device data struct instead of using dynamic allocations for calibration buffers. Thanks Jonathan Cameron <[email protected]>.
- Patch 3: Fixed small typos
- Patch 3: Fixed incompatible division on 32-bit machines reported by Andy Shevchenko <[email protected]> and kernel test robot <[email protected]>.
- Patch 3: Fixed inconsistent use of "x" and "X" declaring constants.
- Patch 5: Dropped incorrect reported-by tag on changelog message.
- Patch 5: Fixed typos on various comments.

Changes in v4:
- Patch 4 and 2: Merged v3 patch 2 (Kconfig refs update) into this patch.
- Patch 3: Added patch fixing unsafe DMA regmap_bulk_* calls reported by Jonathan Cameron <[email protected]>.
- Patch 4: Fixed DMA unsafe buffers used on regmap_bulk_* calls reported by Jonathan Cameron <[email protected]>.

Changes in v3:
- Patch 2: Fixed incorrect abbreviation.
- Patch 3: use dev_err_probe helper to handle error initializing sensor.
- Patch 4: Fixed kernel test robot warning provoked by missing include.
- Patch 4: Fixed bug reported by Dan Carpenter <[email protected]>.
- Patch 5: Fixed formatting and typos on multiple comments.
- Patch 5: Fixed missing boolean initialization reported by Andy Shevchenko <[email protected]>.
- Patch 5: Replaced duplicated comments with a single comment containing a brief explantation in a shared location.
- Patch 5: Dropped incorrect use of unlikely macro.

Changes in v2:
- Added patch 2 updating Kconfig with references to new sensor.
- Patch 3 adds changes proposed by Jonathan Cameron <[email protected]> to declutter and unify configuration logic for the different sensors extending "chip_info" struct with default configuration parameters.
- Patch 4: store temperature and pressure adc values on 3 byte array instead of using the type __le32. Uses function get_unaligned_le24 to convert the little-endian encoded 3 byte value to an integer.
- Patch 4: drops custom macro le16_from_bytes and use get_unaligned_le16.
- Patch 4: generate masks using GENMASK macro.
- Patch 4: use FIELD_PREP to generate bitfields for registries.
- Patch 4: dropped stray formatting change.
- Patch 5: adds sanity checks in bmp280_read_raw for channel properties only available in the BMP380.
- Patch 5: on bmp280_write_* checks if a problem occurred committing new configuration and tries to restore previous working configuration to keep the sensor in a previous working state.
- Patch 5: refactored bmp380_chip_config to only check for configuration errors when a configuration change is detected.
- Patch 5: improved invalid configuration detection on BMP380 restarting measurement loop to force a new measurement after the configuration is updated.

Previous patch version available on:
https://lore.kernel.org/all/[email protected]/

Angel Iglesias (5):
iio: pressure: bmp280: simplify driver initialization logic
iio: pressure: bmp280: Fix alignment for DMA safety
iio: pressure: bmp280: Add support for BMP380 sensor family
dt-bindings: iio: pressure: bmp085: Add BMP380 compatible string
iio: pressure: bmp280: Add more tunable config parameters for BMP380

 .../bindings/iio/pressure/bmp085.yaml         |   4 +-
 drivers/iio/pressure/Kconfig                  |   6 +-
 drivers/iio/pressure/bmp280-core.c            | 866 +++++++++++++++---
 drivers/iio/pressure/bmp280-i2c.c             |   5 +
 drivers/iio/pressure/bmp280-regmap.c          |  55 ++
 drivers/iio/pressure/bmp280-spi.c             |   5 +
 drivers/iio/pressure/bmp280.h                 | 159 +++-
 7 files changed, 952 insertions(+), 148 deletions(-)

base-commit: 180c6cb6b9b79c55b79e8414f4c0208f2463af7d

Signature · 1 line

2.37.1

Angel Iglesias

author

2022-08-07 11:54 UTC #1 lore raw

PATCH v5 1/5 iio: pressure: bmp280: simplify driver initialization logic

Simplified common initialization logic of different sensor types unifying calibration and initial configuration recovery.

Default config param values of each sensor type are stored inside chip_info structure and used to initialize sensor data struct instance.

The helper functions for read each sensor type calibration are converted to a callback available on the chip_info struct.

Revised BMP180 and BMP280 definitions and code functions to use GENMASK and FIELD_PREP/FIELD_GET utilities to homogenize structure with more recent drivers, in preparation for the patches adding support for the BMP380 sensors.

Signed-off-by Angel Iglesias <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 120 ++++++++++++++++++-----------
 drivers/iio/pressure/bmp280.h      |  73 +++++++++---------
 2 files changed, 113 insertions(+), 80 deletions(-)

drivers/iio/pressure/bmp280-core.c +77 ~~-43~~

index fe7aa81e7cc9..a109c2609896 100644
@@ -16,6 +16,8 @@
 
 #define pr_fmt(fmt) "bmp280: " fmt
 
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/regmap.h>

@@ -107,19 +109,28 @@ struct bmp280_data {
 };
 
 struct bmp280_chip_info {
+	unsigned int id_reg;
+
+	int num_channels;
+	unsigned int start_up_time;
+
 	const int *oversampling_temp_avail;
 	int num_oversampling_temp_avail;
+	int oversampling_temp_default;
 
 	const int *oversampling_press_avail;
 	int num_oversampling_press_avail;
+	int oversampling_press_default;
 
 	const int *oversampling_humid_avail;
 	int num_oversampling_humid_avail;
+	int oversampling_humid_default;
 
 	int (*chip_config)(struct bmp280_data *);
 	int (*read_temp)(struct bmp280_data *, int *);
 	int (*read_press)(struct bmp280_data *, int *, int *);
 	int (*read_humid)(struct bmp280_data *, int *, int *);
+	int (*read_calib)(struct bmp280_data *, unsigned int);
 };
 
 /*

@@ -147,15 +158,14 @@ static const struct iio_chan_spec bmp280_channels[] = {
 	},
 };
 
-static int bmp280_read_calib(struct bmp280_data *data,
-			     struct bmp280_calib *calib,
-			     unsigned int chip)
+static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 {
 	int ret;
 	unsigned int tmp;
 	__le16 l16;
 	__be16 b16;
 	struct device *dev = data->dev;
+	struct bmp280_calib *calib = &data->calib.bmp280;
 	__le16 t_buf[BMP280_COMP_TEMP_REG_COUNT / 2];
 	__le16 p_buf[BMP280_COMP_PRESS_REG_COUNT / 2];

@@ -611,8 +621,8 @@ static const struct iio_info bmp280_info = {
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	int ret;
-	u8 osrs = BMP280_OSRS_TEMP_X(data->oversampling_temp + 1) |
-		  BMP280_OSRS_PRESS_X(data->oversampling_press + 1);
+	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
+		  FIELD_PREP(BMP280_OSRS_PRESS_MASK, data->oversampling_press + 1);
 
 	ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
 				 BMP280_OSRS_TEMP_MASK |

@@ -640,21 +650,38 @@ static int bmp280_chip_config(struct bmp280_data *data)
 static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
 
 static const struct bmp280_chip_info bmp280_chip_info = {
+	.id_reg = BMP280_REG_ID,
+	.start_up_time = 2000,
+	.num_channels = 2,
+
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	/*
+	 * Oversampling config values on BMx280 have one additional setting
+	 * that other generations of the family don't:
+	 * The value 0 means the measurement is bypassed instead of
+	 * oversampling set to x1.
+	 *
+	 * To account for this difference, and preserve the same common
+	 * config logic, this is handled later on chip_config callback
+	 * incrementing one unit the oversampling setting.
+	 */
+	.oversampling_temp_default = BMP280_OSRS_TEMP_2X - 1,
 
 	.oversampling_press_avail = bmp280_oversampling_avail,
 	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
 
 	.chip_config = bmp280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
+	.read_calib = bmp280_read_calib,
 };
 
 static int bme280_chip_config(struct bmp280_data *data)
 {
 	int ret;
-	u8 osrs = BMP280_OSRS_HUMIDITIY_X(data->oversampling_humid + 1);
+	u8 osrs = FIELD_PREP(BMP280_OSRS_HUMIDITY_MASK, data->oversampling_humid + 1);
 
 	/*
 	 * Oversampling of humidity must be set before oversampling of

@@ -670,19 +697,27 @@ static int bme280_chip_config(struct bmp280_data *data)
 }
 
 static const struct bmp280_chip_info bme280_chip_info = {
+	.id_reg = BMP280_REG_ID,
+	.start_up_time = 2000,
+	.num_channels = 3,
+
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_temp_default = BMP280_OSRS_TEMP_2X - 1,
 
 	.oversampling_press_avail = bmp280_oversampling_avail,
 	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
 
 	.oversampling_humid_avail = bmp280_oversampling_avail,
 	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
 
 	.chip_config = bme280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
 	.read_humid = bmp280_read_humid,
+	.read_calib = bmp280_read_calib,
 };
 
 static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)

@@ -710,7 +745,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
 		if (!ret)
 			dev_err(data->dev, "timeout waiting for completion\n");
 	} else {
-		if (ctrl_meas == BMP180_MEAS_TEMP)
+		if (FIELD_GET(BMP180_MEAS_CTRL_MASK, ctrl_meas) == BMP180_MEAS_TEMP)
 			delay_us = 4500;
 		else
 			delay_us =

@@ -735,7 +770,9 @@ static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
 	__be16 tmp;
 	int ret;
 
-	ret = bmp180_measure(data, BMP180_MEAS_TEMP);
+	ret = bmp180_measure(data,
+			     FIELD_PREP(BMP180_MEAS_CTRL_MASK, BMP180_MEAS_TEMP) |
+			     BMP180_MEAS_SCO);
 	if (ret)
 		return ret;

@@ -748,11 +785,11 @@ static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
 	return 0;
 }
 
-static int bmp180_read_calib(struct bmp280_data *data,
-			     struct bmp180_calib *calib)
+static int bmp180_read_calib(struct bmp280_data *data, unsigned int chip)
 {
 	int ret;
 	int i;
+	struct bmp180_calib *calib = &data->calib.bmp180;
 	__be16 buf[BMP180_REG_CALIB_COUNT / 2];
 
 	ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,

@@ -832,7 +869,10 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
 	__be32 tmp = 0;
 	u8 oss = data->oversampling_press;
 
-	ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
+	ret = bmp180_measure(data,
+			     FIELD_PREP(BMP180_MEAS_CTRL_MASK, BMP180_MEAS_PRESS) |
+			     FIELD_PREP(BMP180_OSRS_PRESS_MASK, oss) |
+			     BMP180_MEAS_SCO);
 	if (ret)
 		return ret;

@@ -913,17 +953,24 @@ static const int bmp180_oversampling_temp_avail[] = { 1 };
 static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
 
 static const struct bmp280_chip_info bmp180_chip_info = {
+	.id_reg = BMP280_REG_ID,
+	.start_up_time = 2000,
+	.num_channels = 2,
+
 	.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,
 
 	.chip_config = bmp180_chip_config,
 	.read_temp = bmp180_read_temp,
 	.read_press = bmp180_read_press,
+	.read_calib = bmp180_read_calib,
 };
 
 static irqreturn_t bmp085_eoc_irq(int irq, void *d)

@@ -993,6 +1040,7 @@ int bmp280_common_probe(struct device *dev,
 	int ret;
 	struct iio_dev *indio_dev;
 	struct bmp280_data *data;
+	const struct bmp280_chip_info *chip_info;
 	unsigned int chip_id;
 	struct gpio_desc *gpiod;

@@ -1011,30 +1059,25 @@ int bmp280_common_probe(struct device *dev,
 
 	switch (chip) {
 	case BMP180_CHIP_ID:
-		indio_dev->num_channels = 2;
-		data->chip_info = &bmp180_chip_info;
-		data->oversampling_press = ilog2(8);
-		data->oversampling_temp = ilog2(1);
-		data->start_up_time = 10000;
+		chip_info = &bmp180_chip_info;
 		break;
 	case BMP280_CHIP_ID:
-		indio_dev->num_channels = 2;
-		data->chip_info = &bmp280_chip_info;
-		data->oversampling_press = ilog2(16);
-		data->oversampling_temp = ilog2(2);
-		data->start_up_time = 2000;
+		chip_info = &bmp280_chip_info;
 		break;
 	case BME280_CHIP_ID:
-		indio_dev->num_channels = 3;
-		data->chip_info = &bme280_chip_info;
-		data->oversampling_press = ilog2(16);
-		data->oversampling_humid = ilog2(16);
-		data->oversampling_temp = ilog2(2);
-		data->start_up_time = 2000;
+		chip_info = &bme280_chip_info;
 		break;
 	default:
 		return -EINVAL;
 	}
+	data->chip_info = chip_info;
+
+	/* apply initial values from chip info structure */
+	indio_dev->num_channels = chip_info->num_channels;
+	data->oversampling_press = chip_info->oversampling_press_default;
+	data->oversampling_humid = chip_info->oversampling_humid_default;
+	data->oversampling_temp = chip_info->oversampling_temp_default;
+	data->start_up_time = chip_info->start_up_time;
 
 	/* Bring up regulators */
 	regulator_bulk_set_supply_names(data->supplies,

@@ -1071,7 +1114,8 @@ int bmp280_common_probe(struct device *dev,
 	}
 
 	data->regmap = regmap;
-	ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
+
+	ret = regmap_read(regmap, data->chip_info->id_reg, &chip_id);
 	if (ret < 0)
 		return ret;
 	if (chip_id != chip) {

@@ -1091,21 +1135,11 @@ int bmp280_common_probe(struct device *dev,
 	 * non-volatile memory during production". Let's read them out at probe
 	 * time once. They will not change.
 	 */
-	if (chip_id  == BMP180_CHIP_ID) {
-		ret = bmp180_read_calib(data, &data->calib.bmp180);
-		if (ret < 0) {
-			dev_err(data->dev,
-				"failed to read calibration coefficients\n");
-			return ret;
-		}
-	} else if (chip_id == BMP280_CHIP_ID || chip_id == BME280_CHIP_ID) {
-		ret = bmp280_read_calib(data, &data->calib.bmp280, chip_id);
-		if (ret < 0) {
-			dev_err(data->dev,
-				"failed to read calibration coefficients\n");
-			return ret;
-		}
-	}
+
+	ret = data->chip_info->read_calib(data, chip_id);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret,
+				     "failed to read calibration coefficients\n");
 
 	/*
 	 * Attempt to grab an optional EOC IRQ - only the BMP085 has this

drivers/iio/pressure/bmp280.h +36 ~~-37~~

93 lines not on this page · raw message

Signature · 1 line

2.37.1

Andy Shevchenko

2022-08-08 08:19 UTC #2 lore raw

Re: [PATCH v5 1/5] iio: pressure: bmp280: simplify driver initialization logic

Quoted 14 lines · Angel Iglesias

Simplified common initialization logic of different sensor types unifying calibration and initial configuration recovery.
Default config param values of each sensor type are stored inside chip_info structure and used to initialize sensor data struct instance.
The helper functions for read each sensor type calibration are converted to a callback available on the chip_info struct.
Revised BMP180 and BMP280 definitions and code functions to use GENMASK and FIELD_PREP/FIELD_GET utilities to homogenize structure with more recent drivers, in preparation for the patches adding support for the BMP380 sensors.

...

Quoted 6 lines

        int ret;
        unsigned int tmp;
        __le16 l16;
        __be16 b16;
        struct device *dev = data->dev;

+ struct bmp280_calib *calib = &data->calib.bmp280;

When you add longer lines, try to put them to be first.

Quoted 2 lines

        __le16 t_buf[BMP280_COMP_TEMP_REG_COUNT / 2];
        __le16 p_buf[BMP280_COMP_PRESS_REG_COUNT / 2];

...

drivers/iio/pressure/bmp280-core.c static const struct iio_info bmp280_info = { · line 625

Quoted 5 lines

        int ret;

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

-       u8 osrs = BMP280_OSRS_TEMP_X(data->oversampling_temp + 1) |
-                 BMP280_OSRS_PRESS_X(data->oversampling_press + 1);
+       u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
+                 FIELD_PREP(BMP280_OSRS_PRESS_MASK, data->oversampling_press + 1);

Ditto, move it to be before int ret; at the same time. Same for other similar cases.

...

Quoted 1 line

+ /* apply initial values from chip info structure */

Make (one-line) comments consistent...

Quoted 1 line

        /* Bring up regulators */

...see the difference?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jonathan Cameron

maintainer

2022-08-14 13:33 UTC #3 lore raw

Re: [PATCH v5 1/5] iio: pressure: bmp280: simplify driver initialization logic

In reply to Angel Iglesias

Quoted 9 lines · On Sun, 7 Aug 2022 13:54:40 +0200 Angel Iglesias <[email protected]>…

Simplified common initialization logic of different sensor types unifying calibration and initial configuration recovery.
Default config param values of each sensor type are stored inside chip_info structure and used to initialize sensor data struct instance.
The helper functions for read each sensor type calibration are converted to a callback available on the chip_info struct.

Quoted 7 lines

Revised BMP180 and BMP280 definitions and code functions to use GENMASK and FIELD_PREP/FIELD_GET utilities to homogenize structure with more recent drivers, in preparation for the patches adding support for the BMP380 sensors.
Suggested-by Jonathan Cameron <[email protected]>
Signed-off-by Angel Iglesias <[email protected]>

Hi Angel, not sure if I commented on this before, but this patch is doing several different things, so in the ideal case it would be split into 1: FIELD_PREP/FIELD_GET/GENMASK changes.
2: Chipinfo related changes
3: Maybe the the reordering of defintions of local variables that Andy
indirectly referered to.

However, this is short enough to be fairly reviewable as it stands so from my point of view I'll accept leaving 1 and 2 combined if it is going to be a lot of work to separate them. Doesn't look like it will be hard but without trying I'm not totally sure!

One question inline on what the chip parameter to the calibration functions is there for as it appears unused.

Thanks,

Jonathan

Quoted 165 lines

---
 drivers/iio/pressure/bmp280-core.c | 120 ++++++++++++++++++-----------
 drivers/iio/pressure/bmp280.h      |  73 +++++++++---------
 2 files changed, 113 insertions(+), 80 deletions(-)

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

index fe7aa81e7cc9..a109c2609896 100644
@@ -16,6 +16,8 @@
 
 #define pr_fmt(fmt) "bmp280: " fmt
 
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/regmap.h>

@@ -107,19 +109,28 @@ struct bmp280_data {
 };
 
 struct bmp280_chip_info {
+	unsigned int id_reg;
+
+	int num_channels;
+	unsigned int start_up_time;
+
 	const int *oversampling_temp_avail;
 	int num_oversampling_temp_avail;
+	int oversampling_temp_default;
 
 	const int *oversampling_press_avail;
 	int num_oversampling_press_avail;
+	int oversampling_press_default;
 
 	const int *oversampling_humid_avail;
 	int num_oversampling_humid_avail;
+	int oversampling_humid_default;
 
 	int (*chip_config)(struct bmp280_data *);
 	int (*read_temp)(struct bmp280_data *, int *);
 	int (*read_press)(struct bmp280_data *, int *, int *);
 	int (*read_humid)(struct bmp280_data *, int *, int *);
+	int (*read_calib)(struct bmp280_data *, unsigned int);
 };
 
 /*

@@ -147,15 +158,14 @@ static const struct iio_chan_spec bmp280_channels[] = {
 	},
 };
 
-static int bmp280_read_calib(struct bmp280_data *data,
-			     struct bmp280_calib *calib,
-			     unsigned int chip)
+static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 {
 	int ret;
 	unsigned int tmp;
 	__le16 l16;
 	__be16 b16;
 	struct device *dev = data->dev;
+	struct bmp280_calib *calib = &data->calib.bmp280;
 	__le16 t_buf[BMP280_COMP_TEMP_REG_COUNT / 2];
 	__le16 p_buf[BMP280_COMP_PRESS_REG_COUNT / 2];

@@ -611,8 +621,8 @@ static const struct iio_info bmp280_info = {
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	int ret;
-	u8 osrs = BMP280_OSRS_TEMP_X(data->oversampling_temp + 1) |
-		  BMP280_OSRS_PRESS_X(data->oversampling_press + 1);
+	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
+		  FIELD_PREP(BMP280_OSRS_PRESS_MASK, data->oversampling_press + 1);
 
 	ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
 				 BMP280_OSRS_TEMP_MASK |

@@ -640,21 +650,38 @@ static int bmp280_chip_config(struct bmp280_data *data)
 static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
 
 static const struct bmp280_chip_info bmp280_chip_info = {
+	.id_reg = BMP280_REG_ID,
+	.start_up_time = 2000,
+	.num_channels = 2,
+
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	/*
+	 * Oversampling config values on BMx280 have one additional setting
+	 * that other generations of the family don't:
+	 * The value 0 means the measurement is bypassed instead of
+	 * oversampling set to x1.
+	 *
+	 * To account for this difference, and preserve the same common
+	 * config logic, this is handled later on chip_config callback
+	 * incrementing one unit the oversampling setting.
+	 */
+	.oversampling_temp_default = BMP280_OSRS_TEMP_2X - 1,
 
 	.oversampling_press_avail = bmp280_oversampling_avail,
 	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
 
 	.chip_config = bmp280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
+	.read_calib = bmp280_read_calib,
 };
 
 static int bme280_chip_config(struct bmp280_data *data)
 {
 	int ret;
-	u8 osrs = BMP280_OSRS_HUMIDITIY_X(data->oversampling_humid + 1);
+	u8 osrs = FIELD_PREP(BMP280_OSRS_HUMIDITY_MASK, data->oversampling_humid + 1);
 
 	/*
 	 * Oversampling of humidity must be set before oversampling of

@@ -670,19 +697,27 @@ static int bme280_chip_config(struct bmp280_data *data)
 }
 
 static const struct bmp280_chip_info bme280_chip_info = {
+	.id_reg = BMP280_REG_ID,
+	.start_up_time = 2000,
+	.num_channels = 3,
+
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_temp_default = BMP280_OSRS_TEMP_2X - 1,
 
 	.oversampling_press_avail = bmp280_oversampling_avail,
 	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
 
 	.oversampling_humid_avail = bmp280_oversampling_avail,
 	.num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+	.oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
 
 	.chip_config = bme280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
 	.read_humid = bmp280_read_humid,
+	.read_calib = bmp280_read_calib,
 };
 
 static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)

@@ -710,7 +745,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
 		if (!ret)
 			dev_err(data->dev, "timeout waiting for completion\n");
 	} else {
-		if (ctrl_meas == BMP180_MEAS_TEMP)
+		if (FIELD_GET(BMP180_MEAS_CTRL_MASK, ctrl_meas) == BMP180_MEAS_TEMP)
 			delay_us = 4500;
 		else
 			delay_us =

@@ -735,7 +770,9 @@ static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
 	__be16 tmp;
 	int ret;
 
-	ret = bmp180_measure(data, BMP180_MEAS_TEMP);
+	ret = bmp180_measure(data,
+			     FIELD_PREP(BMP180_MEAS_CTRL_MASK, BMP180_MEAS_TEMP) |
+			     BMP180_MEAS_SCO);
 	if (ret)
 		return ret;

drivers/iio/pressure/bmp280-core.c static int bmp180_read_adc_temp(struct bmp280_data *data, int *val) · line 788

Quoted 6 lines

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

@@ -748,11 +785,11 @@ static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
 	return 0;
 }
 
-static int bmp180_read_calib(struct bmp280_data *data,
-			     struct bmp180_calib *calib)
+static int bmp180_read_calib(struct bmp280_data *data, unsigned int chip)

What is chip used for? If there will be a need for it in later patches, then it's fine to add the additional parameter now but please say why in the patch description.

Quoted 19 lines

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

 {
 	int ret;
 	int i;
+	struct bmp180_calib *calib = &data->calib.bmp180;
 	__be16 buf[BMP180_REG_CALIB_COUNT / 2];
 
 	ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,

@@ -832,7 +869,10 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
 	__be32 tmp = 0;
 	u8 oss = data->oversampling_press;
 
-	ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
+	ret = bmp180_measure(data,
+			     FIELD_PREP(BMP180_MEAS_CTRL_MASK, BMP180_MEAS_PRESS) |
+			     FIELD_PREP(BMP180_OSRS_PRESS_MASK, oss) |
+			     BMP180_MEAS_SCO);
 	if (ret)
 		return ret;

Angel Iglesias

author

2022-08-14 14:26 UTC #4 lore raw

Re: [PATCH v5 1/5] iio: pressure: bmp280: simplify driver initialization logic

Quoted 26 lines · Jonathan Cameron

Quoted 9 lines · On Sun, 7 Aug 2022 13:54:40 +0200 Angel Iglesias <[email protected]>…
Simplified common initialization logic of different sensor types unifying calibration and initial configuration recovery.
Default config param values of each sensor type are stored inside chip_info structure and used to initialize sensor data struct instance.
The helper functions for read each sensor type calibration are converted to a callback available on the chip_info struct.
Quoted 7 lines
Revised BMP180 and BMP280 definitions and code functions to use GENMASK and FIELD_PREP/FIELD_GET utilities to homogenize structure with more recent drivers, in preparation for the patches adding support for the BMP380 sensors.
Suggested-by Jonathan Cameron <[email protected]>
Signed-off-by Angel Iglesias <[email protected]>
Hi Angel, not sure if I commented on this before, but this patch is doing several different things, so in the ideal case it would be split into 1: FIELD_PREP/FIELD_GET/GENMASK changes.
2: Chipinfo related changes
3: Maybe the the reordering of defintions of local variables that Andy indirectly referered to.

Quoted 4 lines

However, this is short enough to be fairly reviewable as it stands so from my point of view I'll accept leaving 1 and 2 combined if it is going to be a lot of work to separate them. Doesn't look like it will be hard but without trying I'm not totally sure!

Sure, splitting the patch and keeping each patch focused on one aspect makes total sense to me. Thanks as always for your feedback!

Quoted 193 lines

One question inline on what the chip parameter to the calibration functions is there for as it appears unused.
Thanks,
Jonathan
Quoted 185 lines
---
drivers/iio/pressure/bmp280-core.c | 120 ++++++++++++++++++----------- drivers/iio/pressure/bmp280.h | 73 +++++++++---------
 2 files changed, 113 insertions(+), 80 deletions(-)
drivers/iio/pressure/bmp280-core.c +0 -0
index fe7aa81e7cc9..a109c2609896 100644
@@ -16,6 +16,8 @@
#define pr_fmt(fmt) "bmp280: " fmt
drivers/iio/pressure/bmp280-core.c +2 -0
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/regmap.h>
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -107,19 +109,28 @@ struct bmp280_data {
  };
struct bmp280_chip_info {
+ unsigned int id_reg;
drivers/iio/pressure/bmp280-core.c +4 -0
+
+       int num_channels;
+       unsigned int start_up_time;
+
        const int *oversampling_temp_avail;
        int num_oversampling_temp_avail;
+       int oversampling_temp_default;

        const int *oversampling_press_avail;
        int num_oversampling_press_avail;
+       int oversampling_press_default;

        const int *oversampling_humid_avail;
        int num_oversampling_humid_avail;
+       int oversampling_humid_default;

        int (*chip_config)(struct bmp280_data *); int (*read_temp)(struct bmp280_data *, int *); int (*read_press)(struct bmp280_data *, int *, int *); int (*read_humid)(struct bmp280_data *, int *, int *); +       int (*read_calib)(struct bmp280_data *, unsigned int); };

/*
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -147,15 +158,14 @@ static const struct iio_chan_spec bmp280_channels[] =
 {
         },
  };
drivers/iio/pressure/bmp280-core.c +1 -3
-static int bmp280_read_calib(struct bmp280_data *data,
-                            struct bmp280_calib *calib,
-                            unsigned int chip)
+static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
{
        int ret;
        unsigned int tmp;
        __le16 l16;
        __be16 b16;
        struct device *dev = data->dev;
+       struct bmp280_calib *calib = &data->calib.bmp280; __le16 t_buf[BMP280_COMP_TEMP_REG_COUNT / 2]; __le16 p_buf[BMP280_COMP_PRESS_REG_COUNT / 2];
drivers/iio/pressure/bmp280-core.c +3 -2
@@ -611,8 +621,8 @@ static const struct iio_info bmp280_info = {
  static int bmp280_chip_config(struct bmp280_data *data)
  {
         int ret;
-       u8 osrs = BMP280_OSRS_TEMP_X(data->oversampling_temp + 1) |
-                 BMP280_OSRS_PRESS_X(data->oversampling_press + 1);
+       u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp
+ 1) |
+                 FIELD_PREP(BMP280_OSRS_PRESS_MASK, data-
 >oversampling_press + 1);
ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS, BMP280_OSRS_TEMP_MASK |
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -640,21 +650,38 @@ static int bmp280_chip_config(struct bmp280_data
 *data)
  static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
static const struct bmp280_chip_info bmp280_chip_info = {
+       .id_reg = BMP280_REG_ID,
+       .start_up_time = 2000,
+       .num_channels = 2,
+
        .oversampling_temp_avail = bmp280_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+       /*
+        * Oversampling config values on BMx280 have one additional setting +        * that other generations of the family don't:
+        * The value 0 means the measurement is bypassed instead of +        * oversampling set to x1.
+        *
+        * To account for this difference, and preserve the same common +        * config logic, this is handled later on chip_config callback +        * incrementing one unit the oversampling setting.
+        */
+       .oversampling_temp_default = BMP280_OSRS_TEMP_2X - 1,

        .oversampling_press_avail = bmp280_oversampling_avail, .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+       .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
        .chip_config = bmp280_chip_config,
        .read_temp = bmp280_read_temp,
        .read_press = bmp280_read_press,
+       .read_calib = bmp280_read_calib,
};

static int bme280_chip_config(struct bmp280_data *data)
{
        int ret;
-       u8 osrs = BMP280_OSRS_HUMIDITIY_X(data->oversampling_humid + 1); +       u8 osrs = FIELD_PREP(BMP280_OSRS_HUMIDITY_MASK, data-
Quoted 1 line
oversampling_humid + 1);

        /*
         * Oversampling of humidity must be set before oversampling of
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -670,19 +697,27 @@ static int bme280_chip_config(struct bmp280_data
 *data)
  }
static const struct bmp280_chip_info bme280_chip_info = { +       .id_reg = BMP280_REG_ID,
+       .start_up_time = 2000,
+       .num_channels = 3,
+
        .oversampling_temp_avail = bmp280_oversampling_avail, .num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+       .oversampling_temp_default = BMP280_OSRS_TEMP_2X - 1,
        .oversampling_press_avail = bmp280_oversampling_avail, .num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+       .oversampling_press_default = BMP280_OSRS_PRESS_16X - 1,
        .oversampling_humid_avail = bmp280_oversampling_avail, .num_oversampling_humid_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+       .oversampling_humid_default = BMP280_OSRS_HUMIDITY_16X - 1,
        .chip_config = bme280_chip_config,
        .read_temp = bmp280_read_temp,
        .read_press = bmp280_read_press,
        .read_humid = bmp280_read_humid,
+       .read_calib = bmp280_read_calib,
};

static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -710,7 +745,7 @@ static int bmp180_measure(struct bmp280_data *data, u8
 ctrl_meas)
                 if (!ret)
                        dev_err(data->dev, "timeout waiting for completion\n");
        } else {
-               if (ctrl_meas == BMP180_MEAS_TEMP)
+               if (FIELD_GET(BMP180_MEAS_CTRL_MASK, ctrl_meas) == BMP180_MEAS_TEMP)
                        delay_us = 4500;
                else
                        delay_us =
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -735,7 +770,9 @@ static int bmp180_read_adc_temp(struct bmp280_data
 *data, int *val)
         __be16 tmp;
         int ret;
-       ret = bmp180_measure(data, BMP180_MEAS_TEMP); +       ret = bmp180_measure(data,
+                            FIELD_PREP(BMP180_MEAS_CTRL_MASK, BMP180_MEAS_TEMP) |
+                            BMP180_MEAS_SCO);
        if (ret)
                return ret;
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -748,11 +785,11 @@ static int bmp180_read_adc_temp(struct bmp280_data
 *data, int *val)
         return 0;
  }

Quoted 8 lines

drivers/iio/pressure/bmp280-core.c static int bmp180_read_adc_temp(struct bmp280_data *data, int *val) · line 788
Quoted 4 lines
drivers/iio/pressure/bmp280-core.c +1 -2
-static int bmp180_read_calib(struct bmp280_data *data,
-                            struct bmp180_calib *calib)
+static int bmp180_read_calib(struct bmp280_data *data, unsigned int chip)
What is chip used for? If there will be a need for it in later patches, then it's fine to add the additional parameter now but please say why in the patch description.

In the codepath BMP/BME 280, chip contains the read chip id and is used to check if humidity calibration needs to be loaded when the sensor is a BME280. I took a conservative approach and introduced this field on bmp180 and bmp380 codepaths, assigning the same signature of the three calibration loading functions and use as callbacks.

Having in mind this parameter has no use in two cases out of three, I think I could have done better by splitting the BMP/BME280 callback in one shared part loading pressure and temperature params for both sensors, and a new callback for BME280 invoking first the common code and then loading humidity params. If it sounds good, I would include this change for next iteration.

Quoted 13 lines

Quoted 13 lines
{
        int ret;
        int i;
+       struct bmp180_calib *calib = &data->calib.bmp180; __be16 buf[BMP180_REG_CALIB_COUNT / 2];

        ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,
drivers/iio/pressure/bmp280-core.c +0 -0
@@ -832,7 +869,10 @@ static int bmp180_read_adc_press(struct bmp280_data
 *data, int *val)
         __be32 tmp = 0;
         u8 oss = data->oversampling_press;
- ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));

Quoted 8 lines

Quoted 8 lines
+       ret = bmp180_measure(data,
+                            FIELD_PREP(BMP180_MEAS_CTRL_MASK, BMP180_MEAS_PRESS) |
+                            FIELD_PREP(BMP180_OSRS_PRESS_MASK, oss) | +                            BMP180_MEAS_SCO);
        if (ret)
                return ret;

Kind regards,
Angel

Angel Iglesias

author

2022-08-07 11:55 UTC #5 lore raw

PATCH v5 2/5 iio: pressure: bmp280: Fix alignment for DMA safety

In reply to Angel Iglesias

Adds DMA-safe buffers to driver data struct to store raw data from sensors

The multiple buffers used thorough the driver share the same memory allocated as part of the device data instance. The union containing the buffers is aligned to allow safe usage with DMA operations, such as regmap bulk read calls.

Updated measurement and calibration reading functions to use the new, DMA-safe, buffers.

Signed-off-by Angel Iglesias <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 122 ++++++++++++++---------------
 drivers/iio/pressure/bmp280.h      |   3 +
 2 files changed, 64 insertions(+), 61 deletions(-)

drivers/iio/pressure/bmp280-core.c +61 ~~-61~~

index a109c2609896..4cd657dcbfed 100644
@@ -31,6 +31,7 @@
 #include <linux/completion.h>
 #include <linux/pm_runtime.h>
 #include <linux/random.h>
+#include <asm/unaligned.h>
 
 #include "bmp280.h"

@@ -106,6 +107,18 @@ struct bmp280_data {
 	 * calculation.
 	 */
 	s32 t_fine;
+
+	/*
+	 * DMA (thus cache coherency maintenance) may require the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	union {
+		/* sensor data buffer */
+		u8 data[3];
+		/* calibration data buffers */
+		__le16 bmp280_cal[BMP280_CONTIGUOUS_CALIB_REGS / 2];
+		__be16 bmp180_cal[BMP180_REG_CALIB_COUNT / 2];
+	} buf __aligned(IIO_DMA_MINALIGN);
 };
 
 struct bmp280_chip_info {

@@ -162,41 +175,29 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 {
 	int ret;
 	unsigned int tmp;
-	__le16 l16;
-	__be16 b16;
 	struct device *dev = data->dev;
+	__le16 *t_buf = data->buf.bmp280_cal;
+	__le16 *p_buf = &data->buf.bmp280_cal[T3+1];
 	struct bmp280_calib *calib = &data->calib.bmp280;
-	__le16 t_buf[BMP280_COMP_TEMP_REG_COUNT / 2];
-	__le16 p_buf[BMP280_COMP_PRESS_REG_COUNT / 2];
 
 	/* Read temperature calibration values. */
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
-			       t_buf, BMP280_COMP_TEMP_REG_COUNT);
+			       data->buf.bmp280_cal, sizeof(data->buf.bmp280_cal));
 	if (ret < 0) {
 		dev_err(data->dev,
 			"failed to read temperature calibration parameters\n");
 		return ret;
 	}
 
-	/* Toss the temperature calibration data into the entropy pool */
-	add_device_randomness(t_buf, sizeof(t_buf));
+	/* Toss the temperature and pressure calibration data into the entropy pool */
+	add_device_randomness(data->buf.bmp280_cal, sizeof(data->buf.bmp280_cal));
 
+	/* parse temperature calibration data */
 	calib->T1 = le16_to_cpu(t_buf[T1]);
 	calib->T2 = le16_to_cpu(t_buf[T2]);
 	calib->T3 = le16_to_cpu(t_buf[T3]);
 
-	/* Read pressure calibration values. */
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
-			       p_buf, BMP280_COMP_PRESS_REG_COUNT);
-	if (ret < 0) {
-		dev_err(data->dev,
-			"failed to read pressure calibration parameters\n");
-		return ret;
-	}
-
-	/* Toss the pressure calibration data into the entropy pool */
-	add_device_randomness(p_buf, sizeof(p_buf));
-
+	/* parse pressure calibration data */
 	calib->P1 = le16_to_cpu(p_buf[P1]);
 	calib->P2 = le16_to_cpu(p_buf[P2]);
 	calib->P3 = le16_to_cpu(p_buf[P3]);

@@ -224,12 +225,12 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 	}
 	calib->H1 = tmp;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H2, &l16, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H2, data->buf.data, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read H2 comp value\n");
 		return ret;
 	}
-	calib->H2 = sign_extend32(le16_to_cpu(l16), 15);
+	calib->H2 = get_unaligned_le16(data->buf.data);
 
 	ret = regmap_read(data->regmap, BMP280_REG_COMP_H3, &tmp);
 	if (ret < 0) {

@@ -238,20 +239,20 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 	}
 	calib->H3 = tmp;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H4, &b16, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H4, data->buf.data, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read H4 comp value\n");
 		return ret;
 	}
-	calib->H4 = sign_extend32(((be16_to_cpu(b16) >> 4) & 0xff0) |
-				  (be16_to_cpu(b16) & 0xf), 11);
+	calib->H4 = sign_extend32(((get_unaligned_be16(data->buf.data) >> 4) & 0xff0) |
+				  (get_unaligned_be16(data->buf.data) & 0xf), 11);
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H5, &l16, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H5, data->buf.data, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read H5 comp value\n");
 		return ret;
 	}
-	calib->H5 = sign_extend32(((le16_to_cpu(l16) >> 4) & 0xfff), 11);
+	calib->H5 = sign_extend32(((get_unaligned_le16(data->buf.data) >> 4) & 0xfff), 11);
 
 	ret = regmap_read(data->regmap, BMP280_REG_COMP_H6, &tmp);
 	if (ret < 0) {

@@ -346,16 +347,16 @@ static int bmp280_read_temp(struct bmp280_data *data,
 			    int *val)
 {
 	int ret;
-	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB, &tmp, 3);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
+			       data->buf.data, sizeof(data->buf.data));
 	if (ret < 0) {
 		dev_err(data->dev, "failed to read temperature\n");
 		return ret;
 	}
 
-	adc_temp = be32_to_cpu(tmp) >> 12;
+	adc_temp = get_unaligned_be24(data->buf.data) >> 4;
 	if (adc_temp == BMP280_TEMP_SKIPPED) {
 		/* reading was skipped */
 		dev_err(data->dev, "reading temperature skipped\n");

@@ -379,7 +380,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 			     int *val, int *val2)
 {
 	int ret;
-	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;

@@ -388,13 +388,14 @@ static int bmp280_read_press(struct bmp280_data *data,
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB, &tmp, 3);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
+			       data->buf.data, sizeof(data->buf.data));
 	if (ret < 0) {
 		dev_err(data->dev, "failed to read pressure\n");
 		return ret;
 	}
 
-	adc_press = be32_to_cpu(tmp) >> 12;
+	adc_press = get_unaligned_be24(data->buf.data) >> 4;
 	if (adc_press == BMP280_PRESS_SKIPPED) {
 		/* reading was skipped */
 		dev_err(data->dev, "reading pressure skipped\n");

@@ -410,7 +411,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 
 static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 {
-	__be16 tmp;
 	int ret;
 	s32 adc_humidity;
 	u32 comp_humidity;

@@ -420,13 +420,14 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, &tmp, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
+			       data->buf.data, 2);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to read humidity\n");
 		return ret;
 	}
 
-	adc_humidity = be16_to_cpu(tmp);
+	adc_humidity = get_unaligned_be16(data->buf.data);
 	if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
 		/* reading was skipped */
 		dev_err(data->dev, "reading humidity skipped\n");

@@ -767,7 +768,6 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
 
 static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
 {
-	__be16 tmp;
 	int ret;
 
 	ret = bmp180_measure(data,

@@ -776,48 +776,48 @@ static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 2);
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, data->buf.data, 2);
 	if (ret)
 		return ret;
 
-	*val = be16_to_cpu(tmp);
+	*val = get_unaligned_be16(data->buf.data);
 
 	return 0;
 }
 
 static int bmp180_read_calib(struct bmp280_data *data, unsigned int chip)
 {
+	struct bmp180_calib *calib = &data->calib.bmp180;
 	int ret;
 	int i;
-	struct bmp180_calib *calib = &data->calib.bmp180;
-	__be16 buf[BMP180_REG_CALIB_COUNT / 2];
 
-	ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,
-			       sizeof(buf));
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START,
+			       data->buf.bmp180_cal, sizeof(data->buf.bmp180_cal));
 
 	if (ret < 0)
 		return ret;
 
 	/* None of the words has the value 0 or 0xFFFF */
-	for (i = 0; i < ARRAY_SIZE(buf); i++) {
-		if (buf[i] == cpu_to_be16(0) || buf[i] == cpu_to_be16(0xffff))
+	for (i = 0; i < ARRAY_SIZE(data->buf.bmp180_cal); i++) {
+		if (data->buf.bmp180_cal[i] == cpu_to_be16(0) ||
+		    data->buf.bmp180_cal[i] == cpu_to_be16(0xffff))
 			return -EIO;
 	}
 
 	/* Toss the calibration data into the entropy pool */
-	add_device_randomness(buf, sizeof(buf));
-
-	calib->AC1 = be16_to_cpu(buf[AC1]);
-	calib->AC2 = be16_to_cpu(buf[AC2]);
-	calib->AC3 = be16_to_cpu(buf[AC3]);
-	calib->AC4 = be16_to_cpu(buf[AC4]);
-	calib->AC5 = be16_to_cpu(buf[AC5]);
-	calib->AC6 = be16_to_cpu(buf[AC6]);
-	calib->B1 = be16_to_cpu(buf[B1]);
-	calib->B2 = be16_to_cpu(buf[B2]);
-	calib->MB = be16_to_cpu(buf[MB]);
-	calib->MC = be16_to_cpu(buf[MC]);
-	calib->MD = be16_to_cpu(buf[MD]);
+	add_device_randomness(data->buf.bmp180_cal, sizeof(data->buf.bmp180_cal));
+
+	calib->AC1 = be16_to_cpu(data->buf.bmp180_cal[AC1]);
+	calib->AC2 = be16_to_cpu(data->buf.bmp180_cal[AC2]);
+	calib->AC3 = be16_to_cpu(data->buf.bmp180_cal[AC3]);
+	calib->AC4 = be16_to_cpu(data->buf.bmp180_cal[AC4]);
+	calib->AC5 = be16_to_cpu(data->buf.bmp180_cal[AC5]);
+	calib->AC6 = be16_to_cpu(data->buf.bmp180_cal[AC6]);
+	calib->B1 = be16_to_cpu(data->buf.bmp180_cal[B1]);
+	calib->B2 = be16_to_cpu(data->buf.bmp180_cal[B2]);
+	calib->MB = be16_to_cpu(data->buf.bmp180_cal[MB]);
+	calib->MC = be16_to_cpu(data->buf.bmp180_cal[MC]);
+	calib->MD = be16_to_cpu(data->buf.bmp180_cal[MD]);
 
 	return 0;
 }

@@ -865,9 +865,8 @@ static int bmp180_read_temp(struct bmp280_data *data, int *val)
 
 static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
 {
-	int ret;
-	__be32 tmp = 0;
 	u8 oss = data->oversampling_press;
+	int ret;
 
 	ret = bmp180_measure(data,
 			     FIELD_PREP(BMP180_MEAS_CTRL_MASK, BMP180_MEAS_PRESS) |

@@ -876,11 +875,12 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, &tmp, 3);
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB,
+			       data->buf.data, sizeof(data->buf.data));
 	if (ret)
 		return ret;
 
-	*val = (be32_to_cpu(tmp) >> 8) >> (8 - oss);
+	*val = get_unaligned_be24(data->buf.data) >> (8 - oss);
 
 	return 0;
 }

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

index 5a19c7d04804..c036c7835004 100644
@@ -32,6 +32,9 @@
 #define BMP280_REG_COMP_PRESS_START	0x8E
 #define BMP280_COMP_PRESS_REG_COUNT	18
 
+#define BMP280_CONTIGUOUS_CALIB_REGS	(BMP280_COMP_TEMP_REG_COUNT \
+					 + BMP280_COMP_PRESS_REG_COUNT)
+
 #define BMP280_FILTER_MASK		GENMASK(4, 2)
 #define BMP280_FILTER_OFF		0
 #define BMP280_FILTER_2X		1

Signature · 1 line

2.37.1

Andy Shevchenko

2022-08-08 08:53 UTC #6 lore raw

Re: [PATCH v5 2/5] iio: pressure: bmp280: Fix alignment for DMA safety

Quoted 7 lines · Angel Iglesias

Adds DMA-safe buffers to driver data struct to store raw data from sensors
The multiple buffers used thorough the driver share the same memory allocated as part of the device data instance. The union containing the buffers is aligned to allow safe usage with DMA operations, such as regmap bulk read calls.

...

Quoted 3 lines

#include <linux/completion.h>
#include <linux/pm_runtime.h>
#include <linux/random.h>

+ Blank line.

Quoted 1 line

+#include <asm/unaligned.h>

...

drivers/iio/pressure/bmp280-core.c struct bmp280_data { · line 121

Quoted 7 lines

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

+       union {
+               /* sensor data buffer */
+               u8 data[3];
+               /* calibration data buffers */
+               __le16 bmp280_cal[BMP280_CONTIGUOUS_CALIB_REGS / 2];
+               __be16 bmp180_cal[BMP180_REG_CALIB_COUNT / 2];
+       } buf __aligned(IIO_DMA_MINALIGN);

Hmm... And which field in the struct defines which of the buffers is being used? Also, do you need a non-anonymous union?

Quoted 1 line

};

...

Quoted 1 line

+ /* parse temperature calibration data */

Be consistent! Check all your patches for the consistency (comments, other code style, etc).

...

Quoted 1 line

+ calib->H5 = sign_extend32(((get_unaligned_le16(data->buf.data) >> 4) & 0xfff), 11);

(It's not directly related to this change, but good to ask) Are you going to change all those masks to use GENMASK()?

...

drivers/iio/pressure/bmp280-core.c static int bmp180_read_adc_temp(struct bmp280_data *data, int *val) · line 792

Quoted 4 lines

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

+       struct bmp180_calib *calib = &data->calib.bmp180;
        int ret;
        int i;
-       struct bmp180_calib *calib = &data->calib.bmp180;

Exactly my point given the previous patch, now you have a ping-pong style of changes: the introduced line in the series is being touched again in the very same series without any need.

...

Quoted 2 lines

        u8 oss = data->oversampling_press;

+ int ret;

Ditto.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Angel Iglesias

author

2022-08-12 10:00 UTC #7 lore raw

Re: [PATCH v5 2/5] iio: pressure: bmp280: Fix alignment for DMA safety

Quoted 29 lines · Andy Shevchenko

Quoted 7 lines · Angel Iglesias
Adds DMA-safe buffers to driver data struct to store raw data from sensors
The multiple buffers used thorough the driver share the same memory allocated as part of the device data instance. The union containing the buffers is aligned to allow safe usage with DMA operations, such as regmap bulk read calls.
...
Quoted 3 lines
#include <linux/completion.h>
#include <linux/pm_runtime.h>
#include <linux/random.h>
+ Blank line.
Quoted 1 line
+#include <asm/unaligned.h>
...
Quoted 7 lines
+       union {
+               /* sensor data buffer */
+               u8 data[3];
+               /* calibration data buffers */
+               __le16 bmp280_cal[BMP280_CONTIGUOUS_CALIB_REGS / 2]; +               __be16 bmp180_cal[BMP180_REG_CALIB_COUNT / 2]; +       } buf __aligned(IIO_DMA_MINALIGN);

Quoted 2 lines

Hmm... And which field in the struct defines which of the buffers is being used?

There's no concurrent use of the buffers. Calibration data is read during the initialization of the sensor. The data buffer is then used to store the raw data read from the measurement regs, and is also used a few times to read a the humid calibration on BME280, but again, in a sequential, non concurrent manner.

Regarding which calibration buffer is used, is the same situation as the calibration data union, helper functions and callback for the sensor use the buffer reserved for the sensor. I don't know if this is the best approach, I just followed what I saw previously on this drivers and others from IIO subsystem.

Quoted 1 line

Also, do you need a non-anonymous union?

No I could use an anonymous function. Should I change it to an anonymous union?

Quoted 9 lines

Quoted 1 line
};
...
Quoted 1 line
+ /* parse temperature calibration data */
Be consistent! Check all your patches for the consistency (comments, other code style, etc).

Quoted 7 lines

...
Quoted 2 lines
+ calib->H5 = sign_extend32(((get_unaligned_le16(data->buf.data) >> 4) & 0xfff), 11);
(It's not directly related to this change, but good to ask) Are you going to change all those masks to use GENMASK()?

I thought I made sense refresh previous code on the driver to use GENMASK() and FIELD_PREP and FIELD_GET helpers to use the same standards on the BMP380 codepath. Having in mind other feedback you gave me on this iteration, this GENMASK() and FIELD_PREP/FIELD_GET changes make more sense in a prerequisite patch and not as part of patch 1.

Quoted 10 lines

...
Quoted 4 lines
+       struct bmp180_calib *calib = &data->calib.bmp180; int ret;
        int i;
-       struct bmp180_calib *calib = &data->calib.bmp180;
Exactly my point given the previous patch, now you have a ping-pong style of changes: the introduced line in the series is being touched again in the very same series without any need.

Yup, apologies. I'll be more careful

Quoted 7 lines

...
Quoted 2 lines
u8 oss = data->oversampling_press;
+ int ret;
Ditto.

Jonathan Cameron

maintainer

2022-08-14 13:42 UTC #8 lore raw

Re: [PATCH v5 2/5] iio: pressure: bmp280: Fix alignment for DMA safety

Quoted 30 lines · On Fri, 12 Aug 2022 11:59:50 +0200 Angel Iglesias <[email protected]>…

Quoted 29 lines · Andy Shevchenko
Quoted 7 lines · Angel Iglesias
Adds DMA-safe buffers to driver data struct to store raw data from sensors
The multiple buffers used thorough the driver share the same memory allocated as part of the device data instance. The union containing the buffers is aligned to allow safe usage with DMA operations, such as regmap bulk read calls.
...
Quoted 3 lines
#include <linux/completion.h>
#include <linux/pm_runtime.h>
#include <linux/random.h>
+ Blank line.
Quoted 1 line
+#include <asm/unaligned.h>
...
Quoted 7 lines
+       union {
+               /* sensor data buffer */
+               u8 data[3];
+               /* calibration data buffers */
+               __le16 bmp280_cal[BMP280_CONTIGUOUS_CALIB_REGS / 2]; +               __be16 bmp180_cal[BMP180_REG_CALIB_COUNT / 2]; +       } buf __aligned(IIO_DMA_MINALIGN);

Quoted 2 lines

Quoted 2 lines
Hmm... And which field in the struct defines which of the buffers is being used?

I think the answer to this is which callback is set in the chip_data structure defines which one of the calibration buffers is in use for that purpose. The one used for everything else is defined by the code path, not an explicit variable.

There might be some (I haven't looked) but lock protection is unnecessary as _cal buffers used before we register the device so there is no concurrency yet.

Quoted 12 lines

There's no concurrent use of the buffers. Calibration data is read during the initialization of the sensor. The data buffer is then used to store the raw data read from the measurement regs, and is also used a few times to read a the humid calibration on BME280, but again, in a sequential, non concurrent manner.
Regarding which calibration buffer is used, is the same situation as the calibration data union, helper functions and callback for the sensor use the buffer reserved for the sensor. I don't know if this is the best approach, I just followed what I saw previously on this drivers and others from IIO subsystem.

Quoted 3 lines

Quoted 1 line
Also, do you need a non-anonymous union?
No I could use an anonymous function. Should I change it to an anonymous union?

yes. That seems a good idea.

It's worth giving unions a name if you are using them such that you write via one element and read via another (e.g. for type conversion) but where it's really just a case of potential space saving like this, nice to use an anonymous union and shorten all the lines accessing the elements.

Quoted 15 lines

Quoted 15 lines
Quoted 2 lines
};
...
Quoted 1 line
+ /* parse temperature calibration data */
Be consistent! Check all your patches for the consistency (comments, other code style, etc).
...
Quoted 2 lines
+ calib->H5 = sign_extend32(((get_unaligned_le16(data->buf.data) >> 4) & 0xfff), 11);

Quoted 8 lines

Quoted 2 lines
(It's not directly related to this change, but good to ask) Are you going to change all those masks to use GENMASK()?
I thought I made sense refresh previous code on the driver to use GENMASK() and FIELD_PREP and FIELD_GET helpers to use the same standards on the BMP380 codepath. Having in mind other feedback you gave me on this iteration, this GENMASK() and FIELD_PREP/FIELD_GET changes make more sense in a prerequisite patch and not as part of patch 1.

Agreed. I was thinking the same thing. Pulling out that conversion as a precursor nop cleanup will make all the subsequent changes more readable. Great!

Quoted 8 lines

Quoted 8 lines
...
Quoted 4 lines
+       struct bmp180_calib *calib = &data->calib.bmp180; int ret;
        int i;
-       struct bmp180_calib *calib = &data->calib.bmp180;

Quoted 5 lines

Quoted 3 lines
Exactly my point given the previous patch, now you have a ping-pong style of changes: the introduced line in the series is being touched again in the very same series without any need.
Yup, apologies. I'll be more careful

I'm not particularly fussy about reverse xmas tree so wouldn't normally advocate a cleanup patch just to reorder local variable definitions. However, I think in this case it would be good to have such a precursor patch so as to make the ordering more logical for the additions made by the rest of the series.

Thanks,

Jonathan

Quoted 9 lines

Quoted 8 lines
...
Quoted 2 lines
u8 oss = data->oversampling_press;
+ int ret;
Ditto.

Jonathan Cameron

2022-08-14 13:55 UTC #9 lore raw

Re: [PATCH v5 2/5] iio: pressure: bmp280: Fix alignment for DMA safety

In reply to Angel Iglesias

Quoted 7 lines · On Sun, 7 Aug 2022 13:55:15 +0200 Angel Iglesias <[email protected]>…

Adds DMA-safe buffers to driver data struct to store raw data from sensors
The multiple buffers used thorough the driver share the same memory allocated as part of the device data instance. The union containing the buffers is aligned to allow safe usage with DMA operations, such as regmap bulk read calls.

Quoted 5 lines

Updated measurement and calibration reading functions to use the new, DMA-safe, buffers.
Suggested-by Jonathan Cameron <[email protected]>
Signed-off-by Angel Iglesias <[email protected]>

Hi Angel,

As you are doing a v6 anyway to address Andy's review, I took another look and have a few additional comments below.

Quoted 19 lines

---
 drivers/iio/pressure/bmp280-core.c | 122 ++++++++++++++---------------
 drivers/iio/pressure/bmp280.h      |   3 +
 2 files changed, 64 insertions(+), 61 deletions(-)

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

index a109c2609896..4cd657dcbfed 100644
@@ -31,6 +31,7 @@
 #include <linux/completion.h>
 #include <linux/pm_runtime.h>
 #include <linux/random.h>
+#include <asm/unaligned.h>
 
 #include "bmp280.h"

@@ -106,6 +107,18 @@ struct bmp280_data {
 	 * calculation.

drivers/iio/pressure/bmp280-core.c struct bmp280_data { · line 117

Quoted 10 lines

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

 	 */
 	s32 t_fine;
+
+	/*
+	 * DMA (thus cache coherency maintenance) may require the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	union {
+		/* sensor data buffer */
+		u8 data[3];

__le16 le16;
__be16 be16;

added here would simplify some of the other changes below.

Quoted 8 lines

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

+		/* calibration data buffers */
+		__le16 bmp280_cal[BMP280_CONTIGUOUS_CALIB_REGS / 2];
+		__be16 bmp180_cal[BMP180_REG_CALIB_COUNT / 2];
+	} buf __aligned(IIO_DMA_MINALIGN);
 };
 
 struct bmp280_chip_info {

drivers/iio/pressure/bmp280-core.c static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip) · line 180

Quoted 8 lines

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

@@ -162,41 +175,29 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 {
 	int ret;
 	unsigned int tmp;
-	__le16 l16;
-	__be16 b16;
 	struct device *dev = data->dev;
+	__le16 *t_buf = data->buf.bmp280_cal;
+	__le16 *p_buf = &data->buf.bmp280_cal[T3+1];

spaces around +

drivers/iio/pressure/bmp280-core.c static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip) · line 185

Quoted 8 lines

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

 	struct bmp280_calib *calib = &data->calib.bmp280;
-	__le16 t_buf[BMP280_COMP_TEMP_REG_COUNT / 2];
-	__le16 p_buf[BMP280_COMP_PRESS_REG_COUNT / 2];
 
 	/* Read temperature calibration values. */
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
-			       t_buf, BMP280_COMP_TEMP_REG_COUNT);
+			       data->buf.bmp280_cal, sizeof(data->buf.bmp280_cal));

Given we don't use the local variable until later (as can't get the size from it, I would suggest only setting it further down.

drivers/iio/pressure/bmp280-core.c static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip) · line 194

Quoted 11 lines

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

 	if (ret < 0) {
 		dev_err(data->dev,
 			"failed to read temperature calibration parameters\n");
 		return ret;
 	}
 
-	/* Toss the temperature calibration data into the entropy pool */
-	add_device_randomness(t_buf, sizeof(t_buf));
+	/* Toss the temperature and pressure calibration data into the entropy pool */
+	add_device_randomness(data->buf.bmp280_cal, sizeof(data->buf.bmp280_cal));

like here.

	t_buf = data->bmp280_cal;

or just don't bother with those at all as they don't greatly help readability over

	calib->T1 = le16_to_cpu(data->bmp280_cal[T1]);

similar for p_buf.
Note I haven't looked at the existing code to see if there are other reasons to use t_buf etc, so may be wrong on this!
It will as make this patch more complex even if the code ends up simpler as a result.

Quoted 22 lines

drivers/iio/pressure/bmp280-core.c +2 ~~-12~~

+	/* parse temperature calibration data */
 	calib->T1 = le16_to_cpu(t_buf[T1]);
 	calib->T2 = le16_to_cpu(t_buf[T2]);
 	calib->T3 = le16_to_cpu(t_buf[T3]);
 
-	/* Read pressure calibration values. */
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
-			       p_buf, BMP280_COMP_PRESS_REG_COUNT);
-	if (ret < 0) {
-		dev_err(data->dev,
-			"failed to read pressure calibration parameters\n");
-		return ret;
-	}
-
-	/* Toss the pressure calibration data into the entropy pool */
-	add_device_randomness(p_buf, sizeof(p_buf));
-
+	/* parse pressure calibration data */
 	calib->P1 = le16_to_cpu(p_buf[P1]);
 	calib->P2 = le16_to_cpu(p_buf[P2]);
 	calib->P3 = le16_to_cpu(p_buf[P3]);

drivers/iio/pressure/bmp280-core.c static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip) · line 228

Quoted 5 lines

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

@@ -224,12 +225,12 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 	}
 	calib->H1 = tmp;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H2, &l16, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H2, data->buf.data, 2);

Bit nasty to read little endian into buf.data which I think is a u8 array. Maybe we need another entry in that union for each of le16 and be16 similar to the local variables previously in this function?

Quoted 35 lines

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

 	if (ret < 0) {
 		dev_err(dev, "failed to read H2 comp value\n");
 		return ret;
 	}
-	calib->H2 = sign_extend32(le16_to_cpu(l16), 15);
+	calib->H2 = get_unaligned_le16(data->buf.data);
 
 	ret = regmap_read(data->regmap, BMP280_REG_COMP_H3, &tmp);
 	if (ret < 0) {

@@ -238,20 +239,20 @@ static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 	}
 	calib->H3 = tmp;
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H4, &b16, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H4, data->buf.data, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read H4 comp value\n");
 		return ret;
 	}
-	calib->H4 = sign_extend32(((be16_to_cpu(b16) >> 4) & 0xff0) |
-				  (be16_to_cpu(b16) & 0xf), 11);
+	calib->H4 = sign_extend32(((get_unaligned_be16(data->buf.data) >> 4) & 0xff0) |
+				  (get_unaligned_be16(data->buf.data) & 0xf), 11);
 
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H5, &l16, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H5, data->buf.data, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read H5 comp value\n");
 		return ret;
 	}
-	calib->H5 = sign_extend32(((le16_to_cpu(l16) >> 4) & 0xfff), 11);
+	calib->H5 = sign_extend32(((get_unaligned_le16(data->buf.data) >> 4) & 0xfff), 11);
 
 	ret = regmap_read(data->regmap, BMP280_REG_COMP_H6, &tmp);
 	if (ret < 0) {

...

Quoted 10 lines

static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2) {
drivers/iio/pressure/bmp280-core.c +0 -1
-	__be16 tmp;
 	int ret;
 	s32 adc_humidity;
 	u32 comp_humidity;
@@ -420,13 +420,14 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 	if (ret < 0)
 		return ret;
 

drivers/iio/pressure/bmp280-core.c static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2) · line 430

Quoted 10 lines

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

-	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB, &tmp, 2);
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
+			       data->buf.data, 2);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to read humidity\n");
 		return ret;
 	}
 
-	adc_humidity = be16_to_cpu(tmp);
+	adc_humidity = get_unaligned_be16(data->buf.data);

We are only unaligned because data is u8 and a dumb compiler might not realize the alignment is forced. Use the suggestion above of a couple more entries in the union and we can switch back to be16_to_cpu()

Quoted 5 lines

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

 	if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
 		/* reading was skipped */
 		dev_err(data->dev, "reading humidity skipped\n");

@@ -767,7 +768,6 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)

Angel Iglesias

author

2022-08-07 11:56 UTC #10 lore raw

PATCH v5 3/5 iio: pressure: bmp280: Add support for BMP380 sensor family

In reply to Angel Iglesias

Adds compatibility with the new generation of this sensor, the BMP380

Includes basic sensor initialization to do pressure and temp measurements and allows tuning oversampling settings for each channel.

The compensation algorithms are adapted from the device datasheet and the repository https://github.com/BoschSensortec/BMP3-Sensor-API

Signed-off-by Angel Iglesias <[email protected]>

---
 drivers/iio/pressure/Kconfig         |   6 +-
 drivers/iio/pressure/bmp280-core.c   | 354 +++++++++++++++++++++++++++
 drivers/iio/pressure/bmp280-i2c.c    |   5 +
 drivers/iio/pressure/bmp280-regmap.c |  55 +++++
 drivers/iio/pressure/bmp280-spi.c    |   5 +
 drivers/iio/pressure/bmp280.h        | 101 ++++++++
 6 files changed, 523 insertions(+), 3 deletions(-)

drivers/iio/pressure/Kconfig +3 -3

index 0ff756cea63a..c9453389e4f7 100644
@@ -17,14 +17,14 @@ config ABP060MG
 	  will be called abp060mg.
 
 config BMP280
-	tristate "Bosch Sensortec BMP180/BMP280 pressure sensor I2C driver"
+	tristate "Bosch Sensortec BMP180/BMP280/BMP380 pressure sensor I2C driver"
 	depends on (I2C || SPI_MASTER)
 	select REGMAP
 	select BMP280_I2C if (I2C)
 	select BMP280_SPI if (SPI_MASTER)
 	help
-	  Say yes here to build support for Bosch Sensortec BMP180 and BMP280
-	  pressure and temperature sensors. Also supports the BME280 with
+	  Say yes here to build support for Bosch Sensortec BMP180, BMP280 and
+	  BMP380 pressure and temperature sensors. Also supports the BME280 with
 	  an additional humidity sensor channel.
 
 	  To compile this driver as a module, choose M here: the core module

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

402 lines not on this page · raw message

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

index bf4a7a617537..744442dbe5f9 100644
@@ -19,6 +19,9 @@ static int bmp280_i2c_probe(struct i2c_client *client,
 	case BME280_CHIP_ID:
 		regmap_config = &bmp280_regmap_config;
 		break;
+	case BMP380_CHIP_ID:
+		regmap_config = &bmp380_regmap_config;
+		break;
 	default:
 		return -EINVAL;
 	}

@@ -37,6 +40,7 @@ static int bmp280_i2c_probe(struct i2c_client *client,
 }
 
 static const struct of_device_id bmp280_of_i2c_match[] = {
+	{ .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID },
 	{ .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID },
 	{ .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID },
 	{ .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },

@@ -46,6 +50,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[] = {
+	{"bmp380", BMP380_CHIP_ID },
 	{"bmp280", BMP280_CHIP_ID },
 	{"bmp180", BMP180_CHIP_ID },
 	{"bmp085", BMP180_CHIP_ID },

drivers/iio/pressure/bmp280-regmap.c +55 -0

64 lines not on this page · raw message

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

index 4cfaf3e869b8..011c68e07ebf 100644
@@ -66,6 +66,9 @@ static int bmp280_spi_probe(struct spi_device *spi)
 	case BME280_CHIP_ID:
 		regmap_config = &bmp280_regmap_config;
 		break;
+	case BMP380_CHIP_ID:
+		regmap_config = &bmp380_regmap_config;
+		break;
 	default:
 		return -EINVAL;
 	}

@@ -92,6 +95,7 @@ static const struct of_device_id bmp280_of_spi_match[] = {
 	{ .compatible = "bosch,bmp181", },
 	{ .compatible = "bosch,bmp280", },
 	{ .compatible = "bosch,bme280", },
+	{ .compatible = "bosch,bmp380", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);

@@ -101,6 +105,7 @@ static const struct spi_device_id bmp280_spi_id[] = {
 	{ "bmp181", BMP180_CHIP_ID },
 	{ "bmp280", BMP280_CHIP_ID },
 	{ "bme280", BME280_CHIP_ID },
+	{ "bmp380", BMP380_CHIP_ID },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, bmp280_spi_id);

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

119 lines not on this page · raw message

Signature · 1 line

2.37.1

Andy Shevchenko

2022-08-08 09:09 UTC #11 lore raw

Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family

Quoted 8 lines · Angel Iglesias

Adds compatibility with the new generation of this sensor, the BMP380
Includes basic sensor initialization to do pressure and temp measurements and allows tuning oversampling settings for each channel.
The compensation algorithms are adapted from the device datasheet and the repository https://github.com/BoschSensortec/BMP3-Sensor-API

Missed period.

...

Quoted 3 lines

* https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP180-DS000-121.pdf
* https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP280-DS001-12.pdf
* https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME280_DS001-11.pdf

Seems like the above links are unresponsive now? Perhaps you may fix them as well in a separate patch?

Quoted 1 line

+ * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp388-ds001.pdf

...

Quoted 1 line

+/* See datasheet Section 3.11.1. */

Again, a bit of consistency in (one-line) comments, please.

Quoted 6 lines

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

+struct bmp380_calib {
+       u16 T1;
+       u16 T2;
+       s8  T3;
+       s16 P1;
+       s16 P2;

drivers/iio/pressure/bmp280-core.c struct bmp280_calib { · line 97

Quoted 10 lines

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

+       s8  P3;
+       s8  P4;
+       u16 P5;
+       u16 P6;
+       s8  P7;
+       s8  P8;
+       s16 P9;
+       s8  P10;
+       s8  P11;
+};

__packed ?

...

drivers/iio/pressure/bmp280-core.c static const struct bmp280_chip_info bme280_chip_info = { · line 789

Quoted 6 lines

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

+/* Send a command to BMP3XX sensors */
 
+       /* check if device is ready to process a command */
+       /* send command to process */
+       /* wait for 2ms for command to be processed */
+       /* check for command processing error */

Consistency, please!

...

Quoted 1 line

+ dev_dbg(data->dev, "Command 0x%X processed successfully\n", cmd);

Useless.

...

Quoted 2 lines

+/*
+ * Returns temperature in DegC, resolution is 0.01 DegC. Output value of

DegC?! Perhaps "Celsius degrees"?

Quoted 5 lines

+ * "5123" equals 51.23 DegC. t_fine carries fine temperature as global
+ * value.
+ *
+ * Taken from datasheet, Section Appendix 9, "Compensation formula" and repo
+ * https://github.com/BoschSensortec/BMP3-Sensor-API

Missed period.

Quoted 1 line

+ */

...

Quoted 1 line

+ return (s32) comp_temp;

Do you need casting?

...

Quoted 2 lines

+/*
+ * Returns pressure in Pa as unsigned 32 bit integer in fractional Pascal.

an unsigned
32-bit

Quoted 4 lines

+ * Output value of "9528709" represents 9528709/100 = 95287.09 Pa = 952.8709 hPa
+ *
+ * Taken from datasheet, Section 9.3. "Pressure compensation" and repository
+ * https://github.com/BoschSensortec/BMP3-Sensor-API

Missed period.

Quoted 1 line

+ */

...

Quoted 1 line

+ var1 = ((s64)data->t_fine) * ((s64)data->t_fine);

Too many parentheses?

...

drivers/iio/pressure/bmp280-core.c static const struct bmp280_chip_info bme280_chip_info = { · line 863

Quoted 3 lines

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

+       /*
+        * Dividing by 10 followed by multiplying by 10 to avoid
+        * possible overflow caused by (uncomp_data->pressure * partial_data4)

Missed period.

Quoted 1 line

+ */

...

Quoted 1 line

+ return (u32)comp_press;

Do you need casting?

...

Quoted 1 line

+ /* reading was skipped */

The useless comment.

Quoted 1 line

+ dev_err(data->dev, "reading pressure skipped\n");

...

drivers/iio/pressure/bmp280-core.c static const struct bmp280_chip_info bme280_chip_info = { · line 939

Quoted 2 lines

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

+       /* Compensated pressure is in cPa (centipascals) */
+       *val2 = 100000;

Anything to use from units.h?

...

drivers/iio/pressure/bmp280-core.c static const struct bmp280_chip_info bme280_chip_info = { · line 1006

Quoted 4 lines

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

+       ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
+                               BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK,
+                               osrs);
+       if (ret < 0) {

Do all these ' < 0' parts make any sense?

Quoted 1 line

+ }

...

Quoted 4 lines

+ { .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID },

        { .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID },
        { .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID },
        { .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },

See below.

...

Quoted 4 lines

+ {"bmp380", BMP380_CHIP_ID },

        {"bmp280", BMP280_CHIP_ID },
        {"bmp180", BMP180_CHIP_ID },
        {"bmp085", BMP180_CHIP_ID },

Can we actually keep them forward-ordered? (Add 380 after 280 here and in a separate patch sort the rest, or other way around, sort as a prerequisite patch)

...

drivers/iio/pressure/bmp280.h line 87

Quoted 4 lines

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

+#define BMP380_MIN_TEMP                        -4000
+#define BMP380_MAX_TEMP                        8500
+#define BMP380_MIN_PRES                        3000000
+#define BMP380_MAX_PRES                        12500000

Units?

--
With Best Regards,
Andy Shevchenko

Angel Iglesias

author

2022-08-12 10:47 UTC #12 lore raw

Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family

Quoted 21 lines · Andy Shevchenko

Quoted 8 lines · Angel Iglesias
Adds compatibility with the new generation of this sensor, the BMP380
Includes basic sensor initialization to do pressure and temp measurements and allows tuning oversampling settings for each channel.
The compensation algorithms are adapted from the device datasheet and the repository https://github.com/BoschSensortec/BMP3-Sensor-API
Missed period.
...
Quoted 6 lines
*
https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP180-DS000-121.pdf
*
https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP280-DS001-12.pdf
*
https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME280_DS001-11.pdf

Quoted 2 lines

Seems like the above links are unresponsive now? Perhaps you may fix them as well in a separate patch?

Sure. Should I include this patch in this next version of this series, or should I send the patch as a standalone change?

Quoted 86 lines

Quoted 2 lines
+ *
https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp388-ds001.pdf
...
Quoted 1 line
+/* See datasheet Section 3.11.1. */
Again, a bit of consistency in (one-line) comments, please.
Quoted 6 lines
drivers/iio/pressure/bmp280-core.c +6 -0
+struct bmp380_calib {
+       u16 T1;
+       u16 T2;
+       s8  T3;
+       s16 P1;
+       s16 P2;
drivers/iio/pressure/bmp280-core.c struct bmp280_calib { · line 97
Quoted 10 lines
drivers/iio/pressure/bmp280-core.c +10 -0
+       s8  P3;
+       s8  P4;
+       u16 P5;
+       u16 P6;
+       s8  P7;
+       s8  P8;
+       s16 P9;
+       s8  P10;
+       s8  P11;
+};
__packed ?
...
Quoted 6 lines
+/* Send a command to BMP3XX sensors */
+       /* check if device is ready to process a command */ +       /* send command to process */
+       /* wait for 2ms for command to be processed */ +       /* check for command processing error */
Consistency, please!
...
Quoted 1 line
+       dev_dbg(data->dev, "Command 0x%X processed successfully\n", cmd);
Useless.
...
Quoted 2 lines
+/*
+ * Returns temperature in DegC, resolution is 0.01 DegC. Output value of
DegC?! Perhaps "Celsius degrees"?
Quoted 6 lines
+ * "5123" equals 51.23 DegC. t_fine carries fine temperature as global
+ * value.
+ *
+ * Taken from datasheet, Section Appendix 9, "Compensation formula" and repo
+ * https://github.com/BoschSensortec/BMP3-Sensor-API
Missed period.
Quoted 1 line
+ */
...
Quoted 1 line
+       return (s32) comp_temp;
Do you need casting?
...
Quoted 2 lines
+/*
+ * Returns pressure in Pa as unsigned 32 bit integer in fractional Pascal.
an unsigned
32-bit
Quoted 6 lines
+ * Output value of "9528709" represents 9528709/100 = 95287.09 Pa = 952.8709 hPa
+ *
+ * Taken from datasheet, Section 9.3. "Pressure compensation" and repository
+ * https://github.com/BoschSensortec/BMP3-Sensor-API
Missed period.

Quoted 7 lines

Quoted 1 line
+ */
...
Quoted 1 line
+ var1 = ((s64)data->t_fine) * ((s64)data->t_fine);
Too many parentheses?

Yup, operator precedence of typecast precedes multiplication.

Quoted 9 lines

...
Quoted 4 lines
+ /*
+ * Dividing by 10 followed by multiplying by 10 to avoid + * possible overflow caused by (uncomp_data->pressure * partial_data4)
Missed period.

Quoted 7 lines

Quoted 1 line
+ */
...
Quoted 1 line
+ return (u32)comp_press;
Do you need casting?

Not really, should fall on the implicit conversion rules of the compiler as it works like an assignment, right?. Also, after shifting 40 bits to the right, there's no risk of overflow, comp_press can be a u32 from the start instead of a u64.

Quoted 17 lines

...
Quoted 1 line
+               /* reading was skipped */
The useless comment.
Quoted 1 line
+               dev_err(data->dev, "reading pressure skipped\n");
...
Quoted 2 lines
+       /* Compensated pressure is in cPa (centipascals) */ +       *val2 = 100000;
Anything to use from units.h?
...

Quoted 7 lines

Quoted 5 lines
+       ret = regmap_write_bits(data->regmap, BMP380_REG_OSR, +                               BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK,
+                               osrs);
+       if (ret < 0) {
Do all these ' < 0' parts make any sense?

I've checked regmap calls and return 0 when call is successful, so the answer is no

Quoted 13 lines

Quoted 1 line
+ }
...
Quoted 4 lines
+ { .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID }, { .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID }, { .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID }, { .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },
See below.
...

Quoted 8 lines

Quoted 4 lines
+       {"bmp380", BMP380_CHIP_ID },
        {"bmp280", BMP280_CHIP_ID },
        {"bmp180", BMP180_CHIP_ID },
        {"bmp085", BMP180_CHIP_ID },
Can we actually keep them forward-ordered? (Add 380 after 280 here and in a separate patch sort the rest, or other way around, sort as a prerequisite patch)

Sure!

Quoted 8 lines

...

drivers/iio/pressure/bmp280.h line 87

Quoted 4 lines

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

+#define BMP380_MIN_TEMP                        -4000
+#define BMP380_MAX_TEMP                        8500
+#define BMP380_MIN_PRES                        3000000
+#define BMP380_MAX_PRES                        12500000

Units?

Units as in I should use units.h constants or also comment in which units are these constants?

Quoted 4 lines

--
With Best Regards,
Andy Shevchenko

Kind regards,
Angel

Jonathan Cameron

maintainer

2022-08-14 14:00 UTC #13 lore raw

Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family

Quoted 22 lines · On Fri, 12 Aug 2022 12:47:20 +0200 Angel Iglesias <[email protected]>…

Quoted 21 lines · Andy Shevchenko
Quoted 8 lines · Angel Iglesias
Adds compatibility with the new generation of this sensor, the BMP380
Includes basic sensor initialization to do pressure and temp measurements and allows tuning oversampling settings for each channel.
The compensation algorithms are adapted from the device datasheet and the repository https://github.com/BoschSensortec/BMP3-Sensor-API
Missed period.
...
Quoted 6 lines
*
https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP180-DS000-121.pdf
*
https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP280-DS001-12.pdf
*
https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME280_DS001-11.pdf

Quoted 5 lines

Quoted 2 lines
Seems like the above links are unresponsive now? Perhaps you may fix them as well in a separate patch?
Sure. Should I include this patch in this next version of this series, or should I send the patch as a standalone change?

Either way is fine. Thanks for tidying this up!

Quoted 20 lines

Quoted 20 lines
Quoted 3 lines
+ *
https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp388-ds001.pdf
...
Quoted 1 line
+/* See datasheet Section 3.11.1. */
Again, a bit of consistency in (one-line) comments, please.
Quoted 10 lines
+struct bmp380_calib {
+       u16 T1;
+       u16 T2;
+       s8 T3;
+       s16 P1;
+       s16 P2;
+       s8 P3;
+       s8 P4;
+       u16 P5;
+       u16 P6;

Quoted 8 lines

Quoted 8 lines
Quoted 6 lines
+       s8 P7;
+       s8 P8;
+       s16 P9;
+       s8 P10;
+       s8 P11;
+};
__packed ?

Nope - the unpacking is done on filling this array.

Could reorder to reduce size a little but given the naming vs datasheet that would look very odd. I'd leave this as it is.

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
+ /* Compensated pressure is in cPa (centipascals) */ + *val2 = 100000;
Anything to use from units.h?

Not sure we want to add centi to killo conversion! That combination is rather rare.

Jonathan

Jonathan Cameron

maintainer

2022-08-14 14:04 UTC #14 lore raw

Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family

In reply to Angel Iglesias

Quoted 9 lines · On Sun, 7 Aug 2022 13:55:52 +0200 Angel Iglesias <[email protected]>…

Adds compatibility with the new generation of this sensor, the BMP380
Includes basic sensor initialization to do pressure and temp measurements and allows tuning oversampling settings for each channel.
The compensation algorithms are adapted from the device datasheet and the repository https://github.com/BoschSensortec/BMP3-Sensor-API
Signed-off-by Angel Iglesias <[email protected]>

One additional comment from me inline.

Thanks,

Jonathan

drivers/iio/pressure/bmp280-core.c int bmp280_common_probe(struct device *dev, · line 1475

Quoted 7 lines

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

@@ -1124,6 +1471,13 @@ int bmp280_common_probe(struct device *dev,
 		return -EINVAL;
 	}
 
+	/* BMP3xx requires soft-reset as part of initialization */
+	if (chip_id == BMP380_CHIP_ID) {

I'd prefer this to be based on a flag in chip_info so that we can trivially add it to future devices by just setting that flag for the chip_info added for the new device.

Quoted 8 lines

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

+		ret = bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = data->chip_info->chip_config(data);
 	if (ret < 0)
 		return ret;

Angel Iglesias

author

2022-08-14 14:38 UTC #15 lore raw

Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family

Quoted 20 lines · Jonathan Cameron

Quoted 9 lines · On Sun, 7 Aug 2022 13:55:52 +0200 Angel Iglesias <[email protected]>…
Adds compatibility with the new generation of this sensor, the BMP380
Includes basic sensor initialization to do pressure and temp measurements and allows tuning oversampling settings for each channel.
The compensation algorithms are adapted from the device datasheet and the repository https://github.com/BoschSensortec/BMP3-Sensor-API
Signed-off-by Angel Iglesias <[email protected]>
One additional comment from me inline.
Thanks,
Jonathan

Quoted 11 lines

Quoted 7 lines
}
(quoted diff) +0 -0
@@ -1124,6 +1471,13 @@ int bmp280_common_probe(struct device *dev,
                 return -EINVAL;
         }
+ /* BMP3xx requires soft-reset as part of initialization */ + if (chip_id == BMP380_CHIP_ID) {
I'd prefer this to be based on a flag in chip_info so that we can trivially add it to future devices by just setting that flag for the chip_info added for the new device.

How a new init or preinit callback? For now only BMP380 chip would use it, but I would like to get my hands on a BMP390 and the new BMP580 and extend the driver to support for them.

Quoted 9 lines

Quoted 8 lines
+               ret = bmp380_cmd(data, BMP380_CMD_SOFT_RESET); +               if (ret < 0)
+                       return ret;
+       }
+
        ret = data->chip_info->chip_config(data);
        if (ret < 0)
                return ret;

Kind regards,
Angel

Jonathan Cameron

maintainer

2022-08-14 16:57 UTC #16 lore raw

Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family

Quoted 33 lines · On Sun, 14 Aug 2022 16:37:58 +0200 Angel Iglesias <[email protected]>…

Quoted 20 lines · Jonathan Cameron
Quoted 9 lines · On Sun, 7 Aug 2022 13:55:52 +0200 Angel Iglesias <[email protected]>…
Adds compatibility with the new generation of this sensor, the BMP380
Includes basic sensor initialization to do pressure and temp measurements and allows tuning oversampling settings for each channel.
The compensation algorithms are adapted from the device datasheet and the repository https://github.com/BoschSensortec/BMP3-Sensor-API
Signed-off-by Angel Iglesias <[email protected]>
One additional comment from me inline.
Thanks,
Jonathan
Quoted 11 lines
Quoted 7 lines
}
(quoted diff) +0 -0
@@ -1124,6 +1471,13 @@ int bmp280_common_probe(struct device *dev,
                 return -EINVAL;
         }
+ /* BMP3xx requires soft-reset as part of initialization */ + if (chip_id == BMP380_CHIP_ID) {
I'd prefer this to be based on a flag in chip_info so that we can trivially add it to future devices by just setting that flag for the chip_info added for the new device.

Quoted 3 lines

How a new init or preinit callback? For now only BMP380 chip would use it, but I would like to get my hands on a BMP390 and the new BMP580 and extend the driver to support for them.

Hmm. Good point, I'd failed to noticed the register is bmp380 specific currently. Maybe one to leave for a future refactor if you get those newer parts.

Jonathan

Quoted 13 lines

Quoted 10 lines
Quoted 9 lines
+               ret = bmp380_cmd(data, BMP380_CMD_SOFT_RESET); +               if (ret < 0)
+                       return ret;
+       }
+
        ret = data->chip_info->chip_config(data);
        if (ret < 0)
                return ret;
Kind regards,
Angel

Angel Iglesias

author

2022-08-07 11:56 UTC #17 lore raw

PATCH v5 4/5 dt-bindings: iio: pressure: bmp085: Add BMP380 compatible string

In reply to Angel Iglesias

Add bosch,bmp380 compatible string for the new family of sensors. This family includes the BMP380, BMP384 and BMP388. The register map in this family changes substantially and introduces new features but core concepts and operations carryover from the previous iterations

Signed-off-by Angel Iglesias <[email protected]>

Acked-by Krzysztof Kozlowski <[email protected]>

---
 Documentation/devicetree/bindings/iio/pressure/bmp085.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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

index 49257f9251e8..72cd2c2d3f17 100644
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/iio/pressure/bmp085.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: BMP085/BMP180/BMP280/BME280 pressure iio sensors
+title: BMP085/BMP180/BMP280/BME280/BMP380 pressure iio sensors
 
 maintainers:
   - Andreas Klinger <[email protected]>

@@ -16,6 +16,7 @@ description: |
     https://www.bosch-sensortec.com/bst/products/all_products/bmp180
     https://www.bosch-sensortec.com/bst/products/all_products/bmp280
     https://www.bosch-sensortec.com/bst/products/all_products/bme280
+    https://www.bosch-sensortec.com/bst/products/all_products/bmp380
 
 properties:
   compatible:

@@ -24,6 +25,7 @@ properties:
       - bosch,bmp180
       - bosch,bmp280
       - bosch,bme280
+      - bosch,bmp380
 
   reg:
     maxItems: 1

Signature · 1 line

2.37.1

Angel Iglesias

author

2022-08-07 11:57 UTC #18 lore raw

PATCH v5 5/5 iio: pressure: bmp280: Add more tunable config parameters for BMP380

In reply to Angel Iglesias

Allows sampling frequency and IIR filter coefficients configuration using sysfs ABI.

The IIR filter coefficient is configurable using the sysfs attribute "filter_low_pass_3db_frequency".

Signed-off-by Angel Iglesias <[email protected]>

---
 drivers/iio/pressure/bmp280-core.c | 324 ++++++++++++++++++++++++++---
 drivers/iio/pressure/bmp280.h      |  18 --
 2 files changed, 294 insertions(+), 48 deletions(-)

drivers/iio/pressure/bmp280-core.c +294 ~~-30~~

index fe171908a89c..39406f0f1e76 100644
@@ -100,6 +100,27 @@ static const char *const bmp280_supply_names[] = {
 	"vddd", "vdda"
 };
 
+enum bmp380_odr {
+	BMP380_ODR_200HZ,
+	BMP380_ODR_100HZ,
+	BMP380_ODR_50HZ,
+	BMP380_ODR_25HZ,
+	BMP380_ODR_12_5HZ,
+	BMP380_ODR_6_25HZ,
+	BMP380_ODR_3_125HZ,
+	BMP380_ODR_1_5625HZ,
+	BMP380_ODR_0_78HZ,
+	BMP380_ODR_0_39HZ,
+	BMP380_ODR_0_2HZ,
+	BMP380_ODR_0_1HZ,
+	BMP380_ODR_0_05HZ,
+	BMP380_ODR_0_02HZ,
+	BMP380_ODR_0_01HZ,
+	BMP380_ODR_0_006HZ,
+	BMP380_ODR_0_003HZ,
+	BMP380_ODR_0_0015HZ,
+};
+
 #define BMP280_NUM_SUPPLIES ARRAY_SIZE(bmp280_supply_names)
 
 struct bmp280_data {

@@ -121,6 +142,17 @@ struct bmp280_data {
 	u8 oversampling_press;
 	u8 oversampling_temp;
 	u8 oversampling_humid;
+	u8 iir_filter_coeff;
+
+	/*
+	 * BMP380 devices introduce sampling frequency configuration. See
+	 * datasheet sections 3.3.3. and 4.3.19 for more details.
+	 *
+	 * BMx280 devices allowed indirect configuration of sampling frequency
+	 * changing the t_standby duration between measurements, as detailed on
+	 * section 3.6.3 of the datasheet.
+	 */
+	int sampling_freq;
 
 	/*
 	 * Carryover value from temperature conversion, used in pressure

@@ -145,6 +177,7 @@ struct bmp280_data {
 struct bmp280_chip_info {
 	unsigned int id_reg;
 
+	const struct iio_chan_spec *channels;
 	int num_channels;
 	unsigned int start_up_time;

@@ -160,6 +193,14 @@ struct bmp280_chip_info {
 	int num_oversampling_humid_avail;
 	int oversampling_humid_default;
 
+	const int *iir_filter_coeffs_avail;
+	int num_iir_filter_coeffs_avail;
+	int iir_filter_coeff_default;
+
+	const int (*sampling_freq_avail)[2];
+	int num_sampling_freq_avail;
+	int sampling_freq_default;
+
 	int (*chip_config)(struct bmp280_data *);
 	int (*read_temp)(struct bmp280_data *, int *);
 	int (*read_press)(struct bmp280_data *, int *, int *);

@@ -211,6 +252,30 @@ static const struct iio_chan_spec bmp280_channels[] = {
 	},
 };
 
+static const struct iio_chan_spec bmp380_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+	},
+	{
+		.type = IIO_HUMIDITYRELATIVE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+	},
+};
+
 static int bmp280_read_calib(struct bmp280_data *data, unsigned int chip)
 {
 	int ret;

@@ -526,6 +591,25 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 			break;
 		}
 		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (!data->chip_info->sampling_freq_avail) {
+			ret = -EINVAL;
+			break;
+		}
+
+		*val = data->chip_info->sampling_freq_avail[data->sampling_freq][0];
+		*val2 = data->chip_info->sampling_freq_avail[data->sampling_freq][1];
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		if (!data->chip_info->iir_filter_coeffs_avail) {
+			ret = -EINVAL;
+			break;
+		}
+
+		*val = data->chip_info->iir_filter_coeffs_avail[data->iir_filter_coeff];
+		ret = IIO_VAL_INT;
+		break;
 	default:
 		ret = -EINVAL;
 		break;

@@ -542,14 +626,22 @@ static int bmp280_write_oversampling_ratio_humid(struct bmp280_data *data,
 					       int val)
 {
 	int i;
+	int ret, prev;
 	const int *avail = data->chip_info->oversampling_humid_avail;
 	const int n = data->chip_info->num_oversampling_humid_avail;
 
 	for (i = 0; i < n; i++) {
 		if (avail[i] == val) {
+			prev = data->oversampling_humid;
 			data->oversampling_humid = ilog2(val);
 
-			return data->chip_info->chip_config(data);
+			ret = data->chip_info->chip_config(data);
+			if (ret) {
+				data->oversampling_humid = prev;
+				data->chip_info->chip_config(data);
+				return ret;
+			}
+			return 0;
 		}
 	}
 	return -EINVAL;

@@ -559,14 +651,22 @@ static int bmp280_write_oversampling_ratio_temp(struct bmp280_data *data,
 					       int val)
 {
 	int i;
+	int ret, prev;
 	const int *avail = data->chip_info->oversampling_temp_avail;
 	const int n = data->chip_info->num_oversampling_temp_avail;
 
 	for (i = 0; i < n; i++) {
 		if (avail[i] == val) {
+			prev = data->oversampling_temp;
 			data->oversampling_temp = ilog2(val);
 
-			return data->chip_info->chip_config(data);
+			ret = data->chip_info->chip_config(data);
+			if (ret) {
+				data->oversampling_temp = prev;
+				data->chip_info->chip_config(data);
+				return ret;
+			}
+			return 0;
 		}
 	}
 	return -EINVAL;

@@ -576,14 +676,72 @@ static int bmp280_write_oversampling_ratio_press(struct bmp280_data *data,
 					       int val)
 {
 	int i;
+	int ret, prev;
 	const int *avail = data->chip_info->oversampling_press_avail;
 	const int n = data->chip_info->num_oversampling_press_avail;
 
 	for (i = 0; i < n; i++) {
 		if (avail[i] == val) {
+			prev = data->oversampling_press;
 			data->oversampling_press = ilog2(val);
 
-			return data->chip_info->chip_config(data);
+			ret = data->chip_info->chip_config(data);
+			if (ret) {
+				data->oversampling_press = prev;
+				data->chip_info->chip_config(data);
+				return ret;
+			}
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+static int bmp280_write_sampling_frequency(struct bmp280_data *data,
+					   int val, int val2)
+{
+	int i;
+	int ret, prev;
+	const int (*avail)[2] = data->chip_info->sampling_freq_avail;
+	const int n = data->chip_info->num_sampling_freq_avail;
+
+	for (i = 0; i < n; i++) {
+		if (avail[i][0] == val && avail[i][1] == val2) {
+			prev = data->sampling_freq;
+			data->sampling_freq = i;
+
+			ret = data->chip_info->chip_config(data);
+			if (ret) {
+				data->sampling_freq = prev;
+				data->chip_info->chip_config(data);
+				return ret;
+			}
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+static int bmp280_write_iir_filter_coeffs(struct bmp280_data *data, int val)
+{
+	int i;
+	int ret, prev;
+	const int *avail = data->chip_info->iir_filter_coeffs_avail;
+	const int n = data->chip_info->num_iir_filter_coeffs_avail;
+
+	for (i = 0; i < n; i++) {
+		if (avail[i] == val) {
+			prev = data->iir_filter_coeff;
+			data->iir_filter_coeff = i;
+
+			ret = data->chip_info->chip_config(data);
+			if (ret) {
+				data->iir_filter_coeff = prev;
+				data->chip_info->chip_config(data);
+				return ret;
+
+			}
+			return 0;
 		}
 	}
 	return -EINVAL;

@@ -596,6 +754,12 @@ static int bmp280_write_raw(struct iio_dev *indio_dev,
 	int ret = 0;
 	struct bmp280_data *data = iio_priv(indio_dev);
 
+	/*
+	 * Helper functions to update sensor running configuration.
+	 * If an error happens applying new settings, will try restore
+	 * previous parameters to ensure the sensor is left in a known
+	 * working configuration.
+	 */
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		pm_runtime_get_sync(data->dev);

@@ -618,6 +782,22 @@ static int bmp280_write_raw(struct iio_dev *indio_dev,
 		pm_runtime_mark_last_busy(data->dev);
 		pm_runtime_put_autosuspend(data->dev);
 		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		pm_runtime_get_sync(data->dev);
+		mutex_lock(&data->lock);
+		ret = bmp280_write_sampling_frequency(data, val, val2);
+		mutex_unlock(&data->lock);
+		pm_runtime_mark_last_busy(data->dev);
+		pm_runtime_put_autosuspend(data->dev);
+		break;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		pm_runtime_get_sync(data->dev);
+		mutex_lock(&data->lock);
+		ret = bmp280_write_iir_filter_coeffs(data, val);
+		mutex_unlock(&data->lock);
+		pm_runtime_mark_last_busy(data->dev);
+		pm_runtime_put_autosuspend(data->dev);
+		break;
 	default:
 		return -EINVAL;
 	}

@@ -648,6 +828,17 @@ static int bmp280_read_avail(struct iio_dev *indio_dev,
 		}
 		*type = IIO_VAL_INT;
 		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (const int *)data->chip_info->sampling_freq_avail;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = data->chip_info->num_sampling_freq_avail;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*vals = data->chip_info->iir_filter_coeffs_avail;
+		*type = IIO_VAL_INT;
+		*length = data->chip_info->num_iir_filter_coeffs_avail;
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}

@@ -693,6 +884,7 @@ static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
 static const struct bmp280_chip_info bmp280_chip_info = {
 	.id_reg = BMP280_REG_ID,
 	.start_up_time = 2000,
+	.channels = bmp280_channels,
 	.num_channels = 2,
 
 	.oversampling_temp_avail = bmp280_oversampling_avail,

@@ -740,6 +932,7 @@ static int bme280_chip_config(struct bmp280_data *data)
 static const struct bmp280_chip_info bme280_chip_info = {
 	.id_reg = BMP280_REG_ID,
 	.start_up_time = 2000,
+	.channels = bmp280_channels,
 	.num_channels = 3,
 
 	.oversampling_temp_avail = bmp280_oversampling_avail,

@@ -978,18 +1171,39 @@ static int bmp380_read_calib(struct bmp280_data *data, unsigned int chip)
 	return 0;
 }
 
+static const int bmp380_odr_table[][2] = {
+	[BMP380_ODR_200HZ]	= {200, 0},
+	[BMP380_ODR_100HZ]	= {100, 0},
+	[BMP380_ODR_50HZ]	= {50, 0},
+	[BMP380_ODR_25HZ]	= {25, 0},
+	[BMP380_ODR_12_5HZ]	= {12, 500000},
+	[BMP380_ODR_6_25HZ]	= {6, 250000},
+	[BMP380_ODR_3_125HZ]	= {3, 125000},
+	[BMP380_ODR_1_5625HZ]	= {1, 562500},
+	[BMP380_ODR_0_78HZ]	= {0, 781250},
+	[BMP380_ODR_0_39HZ]	= {0, 390625},
+	[BMP380_ODR_0_2HZ]	= {0, 195313},
+	[BMP380_ODR_0_1HZ]	= {0, 97656},
+	[BMP380_ODR_0_05HZ]	= {0, 48828},
+	[BMP380_ODR_0_02HZ]	= {0, 24414},
+	[BMP380_ODR_0_01HZ]	= {0, 12207},
+	[BMP380_ODR_0_006HZ]	= {0, 6104},
+	[BMP380_ODR_0_003HZ]	= {0, 3052},
+	[BMP380_ODR_0_0015HZ]	= {0, 1526},
+};
+
 static int bmp380_chip_config(struct bmp280_data *data)
 {
+	bool change = false, aux;
 	unsigned int tmp;
 	u8 osrs;
 	int ret;
 
 	/* configure power control register */
-	ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
-				BMP380_CTRL_SENSORS_MASK | BMP380_MODE_MASK,
-				BMP380_CTRL_SENSORS_PRESS_EN |
-				BMP380_CTRL_SENSORS_TEMP_EN |
-				FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_NORMAL));
+	ret = regmap_update_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+				 BMP380_CTRL_SENSORS_MASK,
+				 BMP380_CTRL_SENSORS_PRESS_EN |
+				 BMP380_CTRL_SENSORS_TEMP_EN);
 	if (ret < 0) {
 		dev_err(data->dev,
 			"failed to write operation control register\n");

@@ -1000,55 +1214,94 @@ static int bmp380_chip_config(struct bmp280_data *data)
 	osrs = FIELD_PREP(BMP380_OSRS_TEMP_MASK, data->oversampling_temp) |
 	       FIELD_PREP(BMP380_OSRS_PRESS_MASK, data->oversampling_press);
 
-	ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
-				BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK,
-				osrs);
+	ret = regmap_update_bits_check(data->regmap, BMP380_REG_OSR,
+				       BMP380_OSRS_TEMP_MASK |
+				       BMP380_OSRS_PRESS_MASK,
+				       osrs, &aux);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to write oversampling register\n");
 		return ret;
 	}
+	change = change || aux;
 
 	/* configure output data rate */
-	ret = regmap_write_bits(data->regmap, BMP380_REG_ODR,
-				BMP380_ODRS_MASK, BMP380_ODRS_50HZ);
+	ret = regmap_update_bits_check(data->regmap, BMP380_REG_ODR,
+				       BMP380_ODRS_MASK, data->sampling_freq,
+				       &aux);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to write ODR selection register\n");
 		return ret;
 	}
+	change = change || aux;
 
 	/* set filter data */
-	ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG,
-				 BMP380_FILTER_MASK,
-				 FIELD_PREP(BMP380_FILTER_MASK, BMP380_FILTER_3X));
+	ret = regmap_update_bits_check(data->regmap, BMP380_REG_CONFIG,
+				BMP380_FILTER_MASK,
+				FIELD_PREP(BMP380_FILTER_MASK, data->iir_filter_coeff),
+				&aux);
 	if (ret < 0) {
 		dev_err(data->dev, "failed to write config register\n");
 		return ret;
 	}
+	change = change || aux;
 
-	/* wait startup_time before verifying config changes */
-	usleep_range(data->start_up_time, data->start_up_time + 100);
+	if (change) {
+		/*
+		 * The configurations errors are detected on the fly during a measurement
+		 * cycle. If the sampling frequency is too low, it's faster to reset
+		 * the measurement loop than wait until the next measurement is due.
+		 *
+		 * 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));
+		if (ret < 0) {
+			dev_err(data->dev, "failed to set sleep mode\n");
+			return ret;
+		}
+		usleep_range(2000, 2500);
+		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+					BMP380_MODE_MASK,
+					FIELD_PREP(BMP380_MODE_MASK,
+						   BMP380_MODE_NORMAL));
+		if (ret < 0) {
+			dev_err(data->dev, "failed to set normal mode\n");
+			return ret;
+		}
+		/*
+		 * Waits for measurement before checking configuration error flag.
+		 * Selected longest measure time indicated in section 3.9.1
+		 * in the datasheet.
+		 */
+		msleep(80);
 
-	/* check config error flag */
-	ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);
-	if (ret < 0) {
-		dev_err(data->dev,
-			"failed to read error register\n");
-		return ret;
-	}
-	if (tmp & BMP380_ERR_CONF_MASK) {
-		dev_warn(data->dev,
-			 "sensor flagged configuration as incompatible\n");
-		return -EINVAL;
+		/* check config error flag */
+		ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);
+		if (ret < 0) {
+			dev_err(data->dev,
+				"failed to read error register\n");
+			return ret;
+		}
+		if (tmp & BMP380_ERR_CONF_MASK) {
+			dev_warn(data->dev,
+				"sensor flagged configuration as incompatible\n");
+			return -EINVAL;
+		}
 	}
 
 	return 0;
 }
 
 static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
+static const int bmp380_iir_filter_coeffs_avail[] = { 0, 1, 3, 7, 15, 31, 63, 127 };
 
 static const struct bmp280_chip_info bmp380_chip_info = {
 	.id_reg = BMP380_REG_ID,
 	.start_up_time = 2000,
+	.channels = bmp380_channels,
 	.num_channels = 2,
 
 	.oversampling_temp_avail = bmp380_oversampling_avail,

@@ -1059,6 +1312,14 @@ static const struct bmp280_chip_info bmp380_chip_info = {
 	.num_oversampling_press_avail = ARRAY_SIZE(bmp380_oversampling_avail),
 	.oversampling_press_default = ilog2(4),
 
+	.sampling_freq_avail = bmp380_odr_table,
+	.num_sampling_freq_avail = ARRAY_SIZE(bmp380_odr_table) * 2,
+	.sampling_freq_default = BMP380_ODR_50HZ,
+
+	.iir_filter_coeffs_avail = bmp380_iir_filter_coeffs_avail,
+	.num_iir_filter_coeffs_avail = ARRAY_SIZE(bmp380_iir_filter_coeffs_avail),
+	.iir_filter_coeff_default = 2,
+
 	.chip_config = bmp380_chip_config,
 	.read_temp = bmp380_read_temp,
 	.read_press = bmp380_read_press,

@@ -1299,6 +1560,7 @@ static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
 static const struct bmp280_chip_info bmp180_chip_info = {
 	.id_reg = BMP280_REG_ID,
 	.start_up_time = 2000,
+	.channels = bmp280_channels,
 	.num_channels = 2,
 
 	.oversampling_temp_avail = bmp180_oversampling_temp_avail,

@@ -1397,7 +1659,6 @@ int bmp280_common_probe(struct device *dev,
 	data->dev = dev;
 
 	indio_dev->name = name;
-	indio_dev->channels = bmp280_channels;
 	indio_dev->info = &bmp280_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;

@@ -1420,10 +1681,13 @@ int bmp280_common_probe(struct device *dev,
 	data->chip_info = chip_info;
 
 	/* apply initial values from chip info structure */
+	indio_dev->channels = chip_info->channels;
 	indio_dev->num_channels = chip_info->num_channels;
 	data->oversampling_press = chip_info->oversampling_press_default;
 	data->oversampling_humid = chip_info->oversampling_humid_default;
 	data->oversampling_temp = chip_info->oversampling_temp_default;
+	data->iir_filter_coeff = chip_info->iir_filter_coeff_default;
+	data->sampling_freq = chip_info->sampling_freq_default;
 	data->start_up_time = chip_info->start_up_time;
 
 	/* Bring up regulators */

drivers/iio/pressure/bmp280.h +0 ~~-18~~

index 827bc6b7ca07..15ae3c32f168 100644
@@ -54,24 +54,6 @@
 #define BMP380_OSRS_PRESS_MASK		GENMASK(2, 0)
 
 #define BMP380_ODRS_MASK		GENMASK(4, 0)
-#define BMP380_ODRS_200HZ		0x00
-#define BMP380_ODRS_100HZ		0x01
-#define BMP380_ODRS_50HZ		0x02
-#define BMP380_ODRS_25HZ		0x03
-#define BMP380_ODRS_12_5HZ		0x04
-#define BMP380_ODRS_6_25HZ		0x05
-#define BMP380_ODRS_3_1HZ		0x06
-#define BMP380_ODRS_1_5HZ		0x07
-#define BMP380_ODRS_0_78HZ		0x08
-#define BMP380_ODRS_0_39HZ		0x09
-#define BMP380_ODRS_0_2HZ		0x0A
-#define BMP380_ODRS_0_1HZ		0x0B
-#define BMP380_ODRS_0_05HZ		0x0C
-#define BMP380_ODRS_0_02HZ		0x0D
-#define BMP380_ODRS_0_01HZ		0x0E
-#define BMP380_ODRS_0_006HZ		0x0F
-#define BMP380_ODRS_0_003HZ		0x10
-#define BMP380_ODRS_0_0015HZ		0x11
 
 #define BMP380_CTRL_SENSORS_MASK	GENMASK(1, 0)
 #define BMP380_CTRL_SENSORS_PRESS_EN	BIT(0)

Signature · 1 line

2.37.1

Andy Shevchenko

2022-08-08 09:14 UTC #19 lore raw

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

Quoted 6 lines · Angel Iglesias

Allows sampling frequency and IIR filter coefficients configuration using sysfs ABI.
The IIR filter coefficient is configurable using the sysfs attribute "filter_low_pass_3db_frequency".

...

Quoted 1 line

+static const int bmp380_odr_table[][2] = {

s32_fract ?

Quoted 19 lines

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

+       [BMP380_ODR_200HZ]      = {200, 0},
+       [BMP380_ODR_100HZ]      = {100, 0},
+       [BMP380_ODR_50HZ]       = {50, 0},
+       [BMP380_ODR_25HZ]       = {25, 0},
+       [BMP380_ODR_12_5HZ]     = {12, 500000},
+       [BMP380_ODR_6_25HZ]     = {6, 250000},
+       [BMP380_ODR_3_125HZ]    = {3, 125000},
+       [BMP380_ODR_1_5625HZ]   = {1, 562500},
+       [BMP380_ODR_0_78HZ]     = {0, 781250},
+       [BMP380_ODR_0_39HZ]     = {0, 390625},
+       [BMP380_ODR_0_2HZ]      = {0, 195313},
+       [BMP380_ODR_0_1HZ]      = {0, 97656},
+       [BMP380_ODR_0_05HZ]     = {0, 48828},
+       [BMP380_ODR_0_02HZ]     = {0, 24414},
+       [BMP380_ODR_0_01HZ]     = {0, 12207},
+       [BMP380_ODR_0_006HZ]    = {0, 6104},
+       [BMP380_ODR_0_003HZ]    = {0, 3052},
+       [BMP380_ODR_0_0015HZ]   = {0, 1526},
+};

...

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

Quoted 5 lines

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

+               ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+                                       BMP380_MODE_MASK,
 
+                                       FIELD_PREP(BMP380_MODE_MASK,
+                                                  BMP380_MODE_SLEEP));

One line?

...

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

Quoted 5 lines

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

+               ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+                                       BMP380_MODE_MASK,
 
+                                       FIELD_PREP(BMP380_MODE_MASK,
+                                                  BMP380_MODE_NORMAL));

Ditto.

...

Quoted 1 line

+static const int bmp380_iir_filter_coeffs_avail[] = { 0, 1, 3, 7, 15, 31, 63, 127 };

This seems like a power of two - 1, can it be replaced by a formula in the code?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Angel Iglesias

author

2022-09-12 00:53 UTC #20 lore raw

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

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Angel Iglesias
Allows sampling frequency and IIR filter coefficients configuration using sysfs ABI.

Quoted 8 lines

Quoted 2 lines
The IIR filter coefficient is configurable using the sysfs attribute "filter_low_pass_3db_frequency".
...
Quoted 1 line
+static const int bmp380_odr_table[][2] = {
s32_fract ?

I modeled this bit and other ODR representations after the adxl355 driver. I see that s32_fract would be a bit cleaner than having arrays inside arrays, but I'm failing to see which additional advantages would provide.
Also, technically, these are precomputed frequencies, the first index is the integer part and the second is the fractional part. The fractions would be 200/1, 200/2, 200/4 ... 200/131072

Quoted 50 lines

Quoted 19 lines
+       [BMP380_ODR_200HZ]      = {200, 0},
+       [BMP380_ODR_100HZ]      = {100, 0},
+       [BMP380_ODR_50HZ]       = {50, 0},
+       [BMP380_ODR_25HZ]       = {25, 0},
+       [BMP380_ODR_12_5HZ]     = {12, 500000},
+       [BMP380_ODR_6_25HZ]     = {6, 250000},
+       [BMP380_ODR_3_125HZ]    = {3, 125000},
+       [BMP380_ODR_1_5625HZ]   = {1, 562500},
+       [BMP380_ODR_0_78HZ]     = {0, 781250},
+       [BMP380_ODR_0_39HZ]     = {0, 390625},
+       [BMP380_ODR_0_2HZ]      = {0, 195313},
+       [BMP380_ODR_0_1HZ]      = {0, 97656},
+       [BMP380_ODR_0_05HZ]     = {0, 48828},
+       [BMP380_ODR_0_02HZ]     = {0, 24414},
+       [BMP380_ODR_0_01HZ]     = {0, 12207},
+       [BMP380_ODR_0_006HZ]    = {0, 6104},
+       [BMP380_ODR_0_003HZ]    = {0, 3052},
+       [BMP380_ODR_0_0015HZ]   = {0, 1526},
+};
...
Quoted 6 lines
+               ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+                                       BMP380_MODE_MASK,
+                                       FIELD_PREP(BMP380_MODE_MASK, +                                                  BMP380_MODE_SLEEP));
One line?
...
Quoted 6 lines
+               ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+                                       BMP380_MODE_MASK,
+                                       FIELD_PREP(BMP380_MODE_MASK, +                                                  BMP380_MODE_NORMAL));
Ditto.
...
Quoted 2 lines
+static const int bmp380_iir_filter_coeffs_avail[] = { 0, 1, 3, 7, 15, 31, 63, 127 };
This seems like a power of two - 1, can it be replaced by a formula in the code?

Jonathan Cameron

maintainer

2022-09-15 13:33 UTC #21 lore raw

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

Quoted 13 lines · On Mon, 12 Sep 2022 02:53:06 +0200 Angel Iglesias <[email protected]>…

Quoted 12 lines · Andy Shevchenko
Quoted 6 lines · Angel Iglesias
Allows sampling frequency and IIR filter coefficients configuration using sysfs ABI.
The IIR filter coefficient is configurable using the sysfs attribute "filter_low_pass_3db_frequency".
...
Quoted 1 line
+static const int bmp380_odr_table[][2] = {

Quoted 9 lines

Quoted 1 line
s32_fract ?
I modeled this bit and other ODR representations after the adxl355 driver. I see that s32_fract would be a bit cleaner than having arrays inside arrays, but I'm failing to see which additional advantages would provide.
Also, technically, these are precomputed frequencies, the first index is the integer part and the second is the fractional part. The fractions would be 200/1, 200/2, 200/4 ... 200/131072

Agreed. Don't use s32_fract for this. It would be misleading.

Jonathan

Quoted 51 lines

Quoted 50 lines
Quoted 19 lines
+       [BMP380_ODR_200HZ]      = {200, 0},
+       [BMP380_ODR_100HZ]      = {100, 0},
+       [BMP380_ODR_50HZ]       = {50, 0},
+       [BMP380_ODR_25HZ]       = {25, 0},
+       [BMP380_ODR_12_5HZ]     = {12, 500000},
+       [BMP380_ODR_6_25HZ]     = {6, 250000},
+       [BMP380_ODR_3_125HZ]    = {3, 125000},
+       [BMP380_ODR_1_5625HZ]   = {1, 562500},
+       [BMP380_ODR_0_78HZ]     = {0, 781250},
+       [BMP380_ODR_0_39HZ]     = {0, 390625},
+       [BMP380_ODR_0_2HZ]      = {0, 195313},
+       [BMP380_ODR_0_1HZ]      = {0, 97656},
+       [BMP380_ODR_0_05HZ]     = {0, 48828},
+       [BMP380_ODR_0_02HZ]     = {0, 24414},
+       [BMP380_ODR_0_01HZ]     = {0, 12207},
+       [BMP380_ODR_0_006HZ]    = {0, 6104},
+       [BMP380_ODR_0_003HZ]    = {0, 3052},
+       [BMP380_ODR_0_0015HZ]   = {0, 1526},
+};
...
Quoted 6 lines
+               ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+                                       BMP380_MODE_MASK,
+                                       FIELD_PREP(BMP380_MODE_MASK, +                                                  BMP380_MODE_SLEEP));
One line?
...
Quoted 6 lines
+               ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
+                                       BMP380_MODE_MASK,
+                                       FIELD_PREP(BMP380_MODE_MASK, +                                                  BMP380_MODE_NORMAL));
Ditto.
...
Quoted 2 lines
+static const int bmp380_iir_filter_coeffs_avail[] = { 0, 1, 3, 7, 15, 31, 63, 127 };
This seems like a power of two - 1, can it be replaced by a formula in the code?