LinuxLists.cc - [PATCH 2/3] iio: pressure: bmp280: Add support for BMP380 sensor family

2022-06-25 15:25:00

Subject: [PATCH 2/3] iio: pressure: bmp280: Add support for BMP380 sensor family

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

Included 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/bmp280-core.c | 378 ++++++++++++++++++++++++++-
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 | 118 +++++++++
5 files changed, 558 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index bf8167f43c56..3887475a9059 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -74,6 +74,24 @@ struct bmp280_calib {
s8 H6;
};

+/* See datasheet Section 3.11.1. */
+struct bmp380_calib {
+ u16 T1;
+ u16 T2;
+ s8 T3;
+ s16 P1;
+ s16 P2;
+ s8 P3;
+ s8 P4;
+ u16 P5;
+ u16 P6;
+ s8 P7;
+ s8 P8;
+ s16 P9;
+ s8 P10;
+ s8 P11;
+};
+
static const char *const bmp280_supply_names[] = {
"vddd", "vdda"
};
@@ -90,6 +108,7 @@ struct bmp280_data {
union {
struct bmp180_calib bmp180;
struct bmp280_calib bmp280;
+ struct bmp380_calib bmp380;
} calib;
struct regulator_bulk_data supplies[BMP280_NUM_SUPPLIES];
unsigned int start_up_time; /* in microseconds */
@@ -129,6 +148,25 @@ struct bmp280_chip_info {
enum { T1, T2, T3 };
enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };

+enum {
+ /* Temperature calib indexes */
+ BMP380_T1 = 0,
+ BMP380_T2 = 2,
+ BMP380_T3 = 4,
+ /* Pressure calib indexes */
+ BMP380_P1 = 5,
+ BMP380_P2 = 7,
+ BMP380_P3 = 9,
+ BMP380_P4 = 10,
+ BMP380_P5 = 11,
+ BMP380_P6 = 13,
+ BMP380_P7 = 15,
+ BMP380_P8 = 16,
+ BMP380_P9 = 17,
+ BMP380_P10 = 19,
+ BMP380_P11 = 20
+};
+
static const struct iio_chan_spec bmp280_channels[] = {
{
.type = IIO_PRESSURE,
@@ -252,6 +290,7 @@ static int bmp280_read_calib(struct bmp280_data *data,

return 0;
}
+
/*
* Returns humidity in percent, resolution is 0.01 percent. Output value of
* "47445" represents 47445/1024 = 46.333 %RH.
@@ -685,6 +724,302 @@ static const struct bmp280_chip_info bme280_chip_info = {
.read_humid = bmp280_read_humid,
};

+/* Send a command to BMP3XX sensors */
+static int bmp380_cmd(struct bmp280_data *data, u8 cmd)
+{
+ int ret;
+ unsigned int reg;
+
+ /* check if device is ready to process a command */
+ ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
+ if (ret) {
+ dev_err(data->dev, "failed to read error register\n");
+ goto failure;
+ }
+ if (!(cmd & BMP380_STATUS_CMD_RDY_MASK)) {
+ dev_err(data->dev, "device is not ready to accept commands\n");
+ ret = -EBUSY;
+ goto failure;
+ }
+
+ /* send command to process */
+ ret = regmap_write(data->regmap, BMP380_REG_CMD, cmd);
+ if (ret) {
+ dev_err(data->dev, "failed to send command to device\n");
+ goto failure;
+ }
+ /* wait for 2ms for command to be proccessed */
+ usleep_range(2000, 2500);
+ /* check for command processing error */
+ ret = regmap_read(data->regmap, BMP380_REG_ERROR, &reg);
+ if (ret) {
+ dev_err(data->dev, "error reading ERROR reg\n");
+ goto failure;
+ }
+ if (reg & BMP380_ERR_CMD_MASK) {
+ dev_err(data->dev, "error processing command 0x%X\n", cmd);
+ ret = -EINVAL;
+ goto failure;
+ }
+ dev_dbg(data->dev, "Command 0x%X proccessed successfully\n", cmd);
+
+failure:
+ return ret;
+}
+
+/*
+ * Returns temperature in DegC, resolution is 0.01 DegC. Output value of
+ * "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
+ */
+static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp)
+{
+ s64 var1, var2, var3, var4, var5, var6, comp_temp;
+ struct bmp380_calib *calib = &data->calib.bmp380;
+
+ var1 = ((s64) adc_temp) - (((s64) calib->T1) << 8);
+ var2 = var1 * ((s64) calib->T2);
+ var3 = var1 * var1;
+ var4 = var3 * ((s64) calib->T3);
+ var5 = (var2 << 18) + var4;
+ var6 = var5 >> 32;
+ data->t_fine = (s32) var6;
+ comp_temp = (var6 * 25) >> 14;
+
+ comp_temp = clamp_val(comp_temp, BMP380_MIN_TEMP, BMP380_MAX_TEMP);
+ return (s32) comp_temp;
+}
+
+/*
+ * Returns pressure in Pa as unsigned 32 bit integer in fractional Pascal.
+ * 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
+ */
+static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
+{
+ s64 var1, var2, var3, var4, var5, var6, offset, sensitivity;
+ u64 comp_press;
+ struct bmp380_calib *calib = &data->calib.bmp380;
+
+ var1 = ((s64)data->t_fine) * ((s64)data->t_fine);
+ var2 = var1 >> 6;
+ var3 = (var2 * ((s64) data->t_fine)) >> 8;
+ var4 = (((s64)calib->P8) * var3) >> 5;
+ var5 = (((s64) calib->P7) * var1) << 4;
+ var6 = (((s64) calib->P6) * ((s64)data->t_fine)) << 22;
+ offset = (((s64)calib->P5) << 47) + var4 + var5 + var6;
+ var2 = (((s64)calib->P4) * var3) >> 5;
+ var4 = (((s64) calib->P3) * var1) << 2;
+ var5 = (((s64) calib->P2) - ((s64) 1<<14)) *
+ (((s64)data->t_fine) << 21);
+ sensitivity = ((((s64) calib->P1) - ((s64) 1 << 14)) << 46) +
+ var2 + var4 + var5;
+ var1 = (sensitivity >> 24) * ((s64)adc_press);
+ var2 = ((s64)calib->P10) * ((s64) data->t_fine);
+ var3 = var2 + (((s64) calib->P9) << 16);
+ var4 = (var3 * ((s64)adc_press)) >> 13;
+
+ /* dividing by 10 followed by multiplying by 10
+ * To avoid overflow caused by (uncomp_data->pressure * partial_data4)
+ */
+ var5 = (((s64)adc_press) * (var4 / 10)) >> 9;
+ var5 *= 10;
+ var6 = ((s64)adc_press) * ((s64)adc_press);
+ var2 = (((s64)calib->P11) * var6) >> 16;
+ var3 = (var2 * ((s64)adc_press)) >> 7;
+ var4 = (offset >> 2) + var1 + var5 + var3;
+ comp_press = ((u64)var4 * 25) >> 40;
+
+ comp_press = clamp_val(comp_press, BMP380_MIN_PRES, BMP380_MAX_PRES);
+ return (u32)comp_press;
+}
+
+static int bmp380_read_temp(struct bmp280_data *data, int *val)
+{
+ int ret;
+ __le32 tmp = 0;
+ u32 adc_temp;
+ s32 comp_temp;
+
+ ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, &tmp, 3);
+ if (ret < 0) {
+ dev_err(data->dev, "failed to read temperature\n");
+ return ret;
+ }
+
+ adc_temp = le32_to_cpu(tmp);
+ if (adc_temp == BMP380_TEMP_SKIPPED) {
+ /* reading was skipped */
+ dev_err(data->dev, "reading temperature skipped\n");
+ return -EIO;
+ }
+ comp_temp = bmp380_compensate_temp(data, adc_temp);
+
+ /*
+ * val might be NULL if we're called by the read_press routine,
+ * who only cares about the carry over t_fine value.
+ */
+ if (val) {
+ /* IIO reports temperatures in mC */
+ *val = comp_temp * 10;
+ return IIO_VAL_INT;
+ }
+
+ return 0;
+}
+
+static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
+{
+ int ret;
+ __le32 tmp = 0;
+ u32 adc_press;
+ s32 comp_press;
+
+ /* Read and compensate temperature so we get a reading of t_fine. */
+ ret = bmp380_read_temp(data, NULL);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, &tmp, 3);
+ if (ret < 0) {
+ dev_err(data->dev, "failed to read pressure\n");
+ return ret;
+ }
+
+ adc_press = le32_to_cpu(tmp);
+ if (adc_press == BMP380_PRESS_SKIPPED) {
+ /* reading was skipped */
+ dev_err(data->dev, "reading pressure skipped\n");
+ return -EIO;
+ }
+ comp_press = bmp380_compensate_press(data, adc_press);
+
+ *val = comp_press;
+ /* Compensated pressure is in cPa (centipascals) */
+ *val2 = 100000;
+
+ return IIO_VAL_FRACTIONAL;
+}
+
+static int bmp380_read_calib(struct bmp280_data *data,
+ struct bmp380_calib *calib, unsigned int chip)
+{
+ int ret;
+ u8 buf[BMP380_CALIB_REG_COUNT];
+
+ /* Read temperature calibration values. */
+ ret = regmap_bulk_read(data->regmap, BMP380_REG_CALIB_TEMP_START, buf,
+ BMP380_CALIB_REG_COUNT);
+ 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(buf, sizeof(buf));
+
+ /* Parse calibration data */
+ calib->T1 = le16_from_bytes(buf[BMP380_T1], buf[BMP380_T1 + 1]);
+ calib->T2 = le16_from_bytes(buf[BMP380_T2], buf[BMP380_T2 + 1]);
+ calib->T3 = buf[BMP380_T3];
+ calib->P1 = le16_from_bytes(buf[BMP380_P1], buf[BMP380_P1 + 1]);
+ calib->P2 = le16_from_bytes(buf[BMP380_P2], buf[BMP380_P2 + 1]);
+ calib->P3 = buf[BMP380_P3];
+ calib->P4 = buf[BMP380_P4];
+ calib->P5 = le16_from_bytes(buf[BMP380_P5], buf[BMP380_P5 + 1]);
+ calib->P6 = le16_from_bytes(buf[BMP380_P6], buf[BMP380_P6 + 1]);
+ calib->P7 = buf[BMP380_P7];
+ calib->P8 = buf[BMP380_P8];
+ calib->P9 = le16_from_bytes(buf[BMP380_P9], buf[BMP380_P9 + 1]);
+ calib->P10 = buf[BMP380_P10];
+ calib->P11 = buf[BMP380_P11];
+
+ return 0;
+}
+
+static int bmp380_chip_config(struct bmp280_data *data)
+{
+ u8 osrs;
+ unsigned int tmp;
+ 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 |
+ BMP380_MODE_NORMAL);
+ if (ret < 0) {
+ dev_err(data->dev,
+ "failed to write operation control register\n");
+ return ret;
+ }
+
+ /* configure oversampling */
+ osrs = BMP380_OSRS_TEMP_X(data->oversampling_temp) |
+ BMP380_OSRS_PRESS_X(data->oversampling_press);
+
+ ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
+ BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK,
+ osrs);
+ if (ret < 0) {
+ dev_err(data->dev, "failed to write oversampling register\n");
+ return ret;
+ }
+
+ /* configure output data rate */
+ ret = regmap_write_bits(data->regmap, BMP380_REG_ODR,
+ BMP380_ODRS_MASK, BMP380_ODRS_50HZ);
+ if (ret < 0) {
+ dev_err(data->dev, "failed to write ODR selection register\n");
+ return ret;
+ }
+
+ /* set filter data */
+ ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG,
+ BMP380_FILTER_MASK, BMP380_FILTER_3X);
+ if (ret < 0) {
+ dev_err(data->dev, "failed to write config register\n");
+ return ret;
+ }
+
+ /* 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");
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
+
+static const struct bmp280_chip_info bmp380_chip_info = {
+ .oversampling_temp_avail = bmp380_oversampling_avail,
+ .num_oversampling_temp_avail = ARRAY_SIZE(bmp380_oversampling_avail),
+
+ .oversampling_press_avail = bmp380_oversampling_avail,
+ .num_oversampling_press_avail = ARRAY_SIZE(bmp380_oversampling_avail),
+
+ .chip_config = bmp380_chip_config,
+ .read_temp = bmp380_read_temp,
+ .read_press = bmp380_read_press,
+};
+
static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
{
int ret;
@@ -1032,6 +1367,13 @@ int bmp280_common_probe(struct device *dev,
data->oversampling_temp = ilog2(2);
data->start_up_time = 2000;
break;
+ case BMP380_CHIP_ID:
+ indio_dev->num_channels = 2;
+ data->chip_info = &bmp380_chip_info;
+ data->oversampling_press = ilog2(4);
+ data->oversampling_temp = ilog2(1);
+ data->start_up_time = 2000;
+ break;
default:
return -EINVAL;
}
@@ -1071,7 +1413,18 @@ int bmp280_common_probe(struct device *dev,
}

data->regmap = regmap;
- ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
+ switch (chip) {
+ case BMP180_CHIP_ID:
+ case BMP280_CHIP_ID:
+ case BME280_CHIP_ID:
+ ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
+ break;
+ case BMP380_CHIP_ID:
+ ret = regmap_read(regmap, BMP380_REG_ID, &chip_id);
+ break;
+ default:
+ ret = -EINVAL;
+ }
if (ret < 0)
return ret;
if (chip_id != chip) {
@@ -1080,6 +1433,13 @@ int bmp280_common_probe(struct device *dev,
return -EINVAL;
}

+ /* BMP3xx requires soft-reset as part of initialization */
+ if (chip_id == BMP380_CHIP_ID) {
+ 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;
@@ -1091,20 +1451,32 @@ 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) {
+ switch (chip_id) {
+ case 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) {
+ break;
+ case BMP280_CHIP_ID:
+ case 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;
}
+ break;
+ case BMP380_CHIP_ID:
+ ret = bmp380_read_calib(data, &data->calib.bmp380, chip_id);
+ if (ret < 0) {
+ dev_err(data->dev,
+ "failed to read calibration coefficients\n");
+ return ret;
+ }
+ break;
}

/*
diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
index 35045bd92846..31a8a0daa39a 100644
--- a/drivers/iio/pressure/bmp280-i2c.c
+++ b/drivers/iio/pressure/bmp280-i2c.c
@@ -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 },
diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
index da136dbadc8f..b440fa41bf12 100644
--- a/drivers/iio/pressure/bmp280-regmap.c
+++ b/drivers/iio/pressure/bmp280-regmap.c
@@ -72,6 +72,49 @@ static bool bmp280_is_volatile_reg(struct device *dev, unsigned int reg)
}
}

+static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case BMP380_REG_CMD:
+ case BMP380_REG_CONFIG:
+ case BMP380_REG_FIFO_CONFIG_1:
+ case BMP380_REG_FIFO_CONFIG_2:
+ case BMP380_REG_FIFO_WATERMARK_LSB:
+ case BMP380_REG_FIFO_WATERMARK_MSB:
+ case BMP380_REG_POWER_CONTROL:
+ case BMP380_REG_INT_CONTROL:
+ case BMP380_REG_IF_CONFIG:
+ case BMP380_REG_ODR:
+ case BMP380_REG_OSR:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool bmp380_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case BMP380_REG_TEMP_XLSB:
+ case BMP380_REG_TEMP_LSB:
+ case BMP380_REG_TEMP_MSB:
+ case BMP380_REG_PRESS_XLSB:
+ case BMP380_REG_PRESS_LSB:
+ case BMP380_REG_PRESS_MSB:
+ case BMP380_REG_SENSOR_TIME_XLSB:
+ case BMP380_REG_SENSOR_TIME_LSB:
+ case BMP380_REG_SENSOR_TIME_MSB:
+ case BMP380_REG_INT_STATUS:
+ case BMP380_REG_FIFO_DATA:
+ case BMP380_REG_STATUS:
+ case BMP380_REG_ERROR:
+ case BMP380_REG_EVENT:
+ return true;
+ default:
+ return false;
+ }
+}
+
const struct regmap_config bmp280_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
@@ -83,3 +126,15 @@ const struct regmap_config bmp280_regmap_config = {
.volatile_reg = bmp280_is_volatile_reg,
};
EXPORT_SYMBOL(bmp280_regmap_config);
+
+const struct regmap_config bmp380_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = BMP380_REG_CMD,
+ .cache_type = REGCACHE_RBTREE,
+
+ .writeable_reg = bmp380_is_writeable_reg,
+ .volatile_reg = bmp380_is_volatile_reg,
+};
+EXPORT_SYMBOL(bmp380_regmap_config);
diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
index 41f6cc56d229..303c41130343 100644
--- a/drivers/iio/pressure/bmp280-spi.c
+++ b/drivers/iio/pressure/bmp280-spi.c
@@ -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);
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 57ba0e85db91..b4c122525ffe 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -3,6 +3,122 @@
#include <linux/device.h>
#include <linux/regmap.h>

+/* BMP380 specific registers */
+#define BMP380_REG_CMD 0x7E
+#define BMP380_REG_CONFIG 0x1F
+#define BMP380_REG_ODR 0X1D
+#define BMP380_REG_OSR 0X1C
+#define BMP380_REG_POWER_CONTROL 0X1B
+#define BMP380_REG_IF_CONFIG 0X1A
+#define BMP380_REG_INT_CONTROL 0X19
+#define BMP380_REG_INT_STATUS 0X11
+#define BMP380_REG_EVENT 0X10
+#define BMP380_REG_STATUS 0X03
+#define BMP380_REG_ERROR 0X02
+#define BMP380_REG_ID 0X00
+
+#define BMP380_REG_FIFO_CONFIG_1 0X18
+#define BMP380_REG_FIFO_CONFIG_2 0X17
+#define BMP380_REG_FIFO_WATERMARK_MSB 0X16
+#define BMP380_REG_FIFO_WATERMARK_LSB 0X15
+#define BMP380_REG_FIFO_DATA 0X14
+#define BMP380_REG_FIFO_LENGTH_MSB 0X13
+#define BMP380_REG_FIFO_LENGTH_LSB 0X12
+
+#define BMP380_REG_SENSOR_TIME_MSB 0X0E
+#define BMP380_REG_SENSOR_TIME_LSB 0X0D
+#define BMP380_REG_SENSOR_TIME_XLSB 0X0C
+
+#define BMP380_REG_TEMP_MSB 0X09
+#define BMP380_REG_TEMP_LSB 0X08
+#define BMP380_REG_TEMP_XLSB 0X07
+
+#define BMP380_REG_PRESS_MSB 0X06
+#define BMP380_REG_PRESS_LSB 0X05
+#define BMP380_REG_PRESS_XLSB 0X04
+
+#define BMP380_REG_CALIB_TEMP_START 0x31
+#define BMP380_CALIB_REG_COUNT 21
+#define le16_from_bytes(lsb, msb) (le16_to_cpu(((((__le16) msb) << 8) | \
+ (__le16) lsb)))
+
+#define BMP380_FILTER_MASK GENMASK(3, 1)
+#define BMP380_FILTER_OFF 0
+#define BMP380_FILTER_1X BIT(1)
+#define BMP380_FILTER_3X BIT(2)
+#define BMP380_FILTER_7X (BIT(2) | BIT(1))
+#define BMP380_FILTER_15X BIT(3)
+#define BMP380_FILTER_31X (BIT(3) | BIT(1))
+#define BMP380_FILTER_63X (BIT(3) | BIT(2))
+#define BMP380_FILTER_127X (BIT(3) | BIT(2) | BIT(1))
+
+#define BMP380_OSRS_TEMP_MASK GENMASK(5, 3)
+#define BMP380_OSRS_TEMP_X(osrs_t) ((osrs_t) << 3)
+#define BMP380_OSRS_TEMP_1X BMP380_OSRS_TEMP_X(0)
+#define BMP380_OSRS_TEMP_2X BMP380_OSRS_TEMP_X(1)
+#define BMP380_OSRS_TEMP_4X BMP380_OSRS_TEMP_X(2)
+#define BMP380_OSRS_TEMP_8X BMP380_OSRS_TEMP_X(3)
+#define BMP380_OSRS_TEMP_16X BMP380_OSRS_TEMP_X(4)
+#define BMP380_OSRS_TEMP_32X BMP380_OSRS_TEMP_X(5)
+
+#define BMP380_OSRS_PRESS_MASK (BIT(2) | BIT(1) | BIT(0))
+#define BMP380_OSRS_PRESS_X(osrs_p) ((osrs_p) << 0)
+#define BMP380_OSRS_PRESS_1X BMP380_OSRS_PRESS_X(0)
+#define BMP380_OSRS_PRESS_2X BMP380_OSRS_PRESS_X(1)
+#define BMP380_OSRS_PRESS_4X BMP380_OSRS_PRESS_X(2)
+#define BMP380_OSRS_PRESS_8X BMP380_OSRS_PRESS_X(3)
+#define BMP380_OSRS_PRESS_16X BMP380_OSRS_PRESS_X(4)
+#define BMP380_OSRS_PRESS_32X BMP380_OSRS_PRESS_X(5)
+
+#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)
+#define BMP380_CTRL_SENSORS_TEMP_EN BIT(1)
+#define BMP380_MODE_MASK GENMASK(5, 4)
+#define BMP380_MODE_SLEEP 0
+#define BMP380_MODE_FORCED BIT(4)
+#define BMP380_MODE_NORMAL (BIT(5) | BIT(4))
+
+#define BMP380_MIN_TEMP -4000
+#define BMP380_MAX_TEMP 8500
+#define BMP380_MIN_PRES 3000000
+#define BMP380_MAX_PRES 12500000
+
+#define BMP380_CMD_NOOP 0X00
+#define BMP380_CMD_EXTMODE_EN_MID 0x34
+#define BMP380_CMD_FIFO_FLUSH 0XB0
+#define BMP380_CMD_SOFT_RESET 0xB6
+
+#define BMP380_STATUS_CMD_RDY_MASK BIT(4)
+#define BMP380_STATUS_DRDY_PRESS_MASK BIT(5)
+#define BMP380_STATUS_DRDY_TEMP_MASK BIT(6)
+
+#define BMP380_ERR_FATAL_MASK BIT(0)
+#define BMP380_ERR_CMD_MASK BIT(1)
+#define BMP380_ERR_CONF_MASK BIT(2)
+
+#define BMP380_TEMP_SKIPPED 0x800000
+#define BMP380_PRESS_SKIPPED 0x800000
+
/* BMP280 specific registers */
#define BMP280_REG_HUMIDITY_LSB 0xFE
#define BMP280_REG_HUMIDITY_MSB 0xFD
@@ -92,6 +208,7 @@
#define BMP280_REG_RESET 0xE0
#define BMP280_REG_ID 0xD0

+#define BMP380_CHIP_ID 0x50
#define BMP180_CHIP_ID 0x55
#define BMP280_CHIP_ID 0x58
#define BME280_CHIP_ID 0x60
@@ -105,6 +222,7 @@
/* Regmap configurations */
extern const struct regmap_config bmp180_regmap_config;
extern const struct regmap_config bmp280_regmap_config;
+extern const struct regmap_config bmp380_regmap_config;

/* Probe called from different transports */
int bmp280_common_probe(struct device *dev,
--
2.25.1

2022-06-25 16:00:02

by Jonathan Cameron

[permalink] [raw]

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

On Sat, 25 Jun 2022 17:09:12 +0200
Angel Iglesias <[email protected]> wrote:

> Adds compatibility with the new generation of this sensor, the BMP380
>
> Included 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]>

Hi Angel,

A few comments inline, but mostly looks good to me.

Jonathan

> ---
> drivers/iio/pressure/bmp280-core.c | 378 ++++++++++++++++++++++++++-
> 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 | 118 +++++++++
> 5 files changed, 558 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index bf8167f43c56..3887475a9059 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -74,6 +74,24 @@ struct bmp280_calib {
> s8 H6;
> };
>
> +/* See datasheet Section 3.11.1. */
> +struct bmp380_calib {
> + u16 T1;
> + u16 T2;
> + s8 T3;
> + s16 P1;
> + s16 P2;
> + s8 P3;
> + s8 P4;
> + u16 P5;
> + u16 P6;
> + s8 P7;
> + s8 P8;
> + s16 P9;
> + s8 P10;
> + s8 P11;
> +};
> +
> static const char *const bmp280_supply_names[] = {
> "vddd", "vdda"
> };
> @@ -90,6 +108,7 @@ struct bmp280_data {
> union {
> struct bmp180_calib bmp180;
> struct bmp280_calib bmp280;
> + struct bmp380_calib bmp380;
> } calib;
> struct regulator_bulk_data supplies[BMP280_NUM_SUPPLIES];
> unsigned int start_up_time; /* in microseconds */
> @@ -129,6 +148,25 @@ struct bmp280_chip_info {
> enum { T1, T2, T3 };
> enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
>
> +enum {
> + /* Temperature calib indexes */
> + BMP380_T1 = 0,
> + BMP380_T2 = 2,
> + BMP380_T3 = 4,
> + /* Pressure calib indexes */
> + BMP380_P1 = 5,
> + BMP380_P2 = 7,
> + BMP380_P3 = 9,
> + BMP380_P4 = 10,
> + BMP380_P5 = 11,
> + BMP380_P6 = 13,
> + BMP380_P7 = 15,
> + BMP380_P8 = 16,
> + BMP380_P9 = 17,
> + BMP380_P10 = 19,
> + BMP380_P11 = 20
> +};
> +
> static const struct iio_chan_spec bmp280_channels[] = {
> {
> .type = IIO_PRESSURE,
> @@ -252,6 +290,7 @@ static int bmp280_read_calib(struct bmp280_data *data,
>
> return 0;
> }
> +
Stray change. It's a good one, but doesn't belong in this patch. Feel free to
do another patch tidying up whitespace in the driver.

> /*
> * Returns humidity in percent, resolution is 0.01 percent. Output value of
> * "47445" represents 47445/1024 = 46.333 %RH.
> @@ -685,6 +724,302 @@ static const struct bmp280_chip_info bme280_chip_info = {
> .read_humid = bmp280_read_humid,
> };
>
> +/* Send a command to BMP3XX sensors */
> +static int bmp380_cmd(struct bmp280_data *data, u8 cmd)
> +{
> + int ret;
> + unsigned int reg;
> +
> + /* check if device is ready to process a command */
> + ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
> + if (ret) {
> + dev_err(data->dev, "failed to read error register\n");
> + goto failure;
> + }
> + if (!(cmd & BMP380_STATUS_CMD_RDY_MASK)) {
> + dev_err(data->dev, "device is not ready to accept commands\n");
> + ret = -EBUSY;
> + goto failure;
> + }
> +
> + /* send command to process */
> + ret = regmap_write(data->regmap, BMP380_REG_CMD, cmd);
> + if (ret) {
> + dev_err(data->dev, "failed to send command to device\n");
> + goto failure;
> + }
> + /* wait for 2ms for command to be proccessed */
> + usleep_range(2000, 2500);
> + /* check for command processing error */
> + ret = regmap_read(data->regmap, BMP380_REG_ERROR, &reg);
> + if (ret) {
> + dev_err(data->dev, "error reading ERROR reg\n");
> + goto failure;
> + }
> + if (reg & BMP380_ERR_CMD_MASK) {
> + dev_err(data->dev, "error processing command 0x%X\n", cmd);
> + ret = -EINVAL;

as nothing to do in failure path, direct return from here preferred.
return -EINVAL;
Same for all similar cases.

> + goto failure;
> + }
> + dev_dbg(data->dev, "Command 0x%X proccessed successfully\n", cmd);
> +
> +failure:
> + return ret;
> +}
> +
> +/*
> + * Returns temperature in DegC, resolution is 0.01 DegC. Output value of
> + * "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
> + */
> +static s32 bmp380_compensate_temp(struct bmp280_data *data, u32 adc_temp)
> +{
> + s64 var1, var2, var3, var4, var5, var6, comp_temp;
> + struct bmp380_calib *calib = &data->calib.bmp380;
> +
> + var1 = ((s64) adc_temp) - (((s64) calib->T1) << 8);
> + var2 = var1 * ((s64) calib->T2);
> + var3 = var1 * var1;
> + var4 = var3 * ((s64) calib->T3);
> + var5 = (var2 << 18) + var4;
> + var6 = var5 >> 32;
> + data->t_fine = (s32) var6;
> + comp_temp = (var6 * 25) >> 14;
> +
> + comp_temp = clamp_val(comp_temp, BMP380_MIN_TEMP, BMP380_MAX_TEMP);
> + return (s32) comp_temp;
> +}
> +
> +/*
> + * Returns pressure in Pa as unsigned 32 bit integer in fractional Pascal.
> + * 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
> + */
> +static u32 bmp380_compensate_press(struct bmp280_data *data, u32 adc_press)
> +{
> + s64 var1, var2, var3, var4, var5, var6, offset, sensitivity;
> + u64 comp_press;
> + struct bmp380_calib *calib = &data->calib.bmp380;
> +
> + var1 = ((s64)data->t_fine) * ((s64)data->t_fine);
> + var2 = var1 >> 6;
> + var3 = (var2 * ((s64) data->t_fine)) >> 8;
> + var4 = (((s64)calib->P8) * var3) >> 5;
> + var5 = (((s64) calib->P7) * var1) << 4;
> + var6 = (((s64) calib->P6) * ((s64)data->t_fine)) << 22;
> + offset = (((s64)calib->P5) << 47) + var4 + var5 + var6;
> + var2 = (((s64)calib->P4) * var3) >> 5;
> + var4 = (((s64) calib->P3) * var1) << 2;
> + var5 = (((s64) calib->P2) - ((s64) 1<<14)) *
> + (((s64)data->t_fine) << 21);
> + sensitivity = ((((s64) calib->P1) - ((s64) 1 << 14)) << 46) +
> + var2 + var4 + var5;
> + var1 = (sensitivity >> 24) * ((s64)adc_press);
> + var2 = ((s64)calib->P10) * ((s64) data->t_fine);
> + var3 = var2 + (((s64) calib->P9) << 16);
> + var4 = (var3 * ((s64)adc_press)) >> 13;
> +
> + /* dividing by 10 followed by multiplying by 10
> + * To avoid overflow caused by (uncomp_data->pressure * partial_data4)
> + */
> + var5 = (((s64)adc_press) * (var4 / 10)) >> 9;
> + var5 *= 10;
> + var6 = ((s64)adc_press) * ((s64)adc_press);
> + var2 = (((s64)calib->P11) * var6) >> 16;
> + var3 = (var2 * ((s64)adc_press)) >> 7;
> + var4 = (offset >> 2) + var1 + var5 + var3;
> + comp_press = ((u64)var4 * 25) >> 40;
> +
> + comp_press = clamp_val(comp_press, BMP380_MIN_PRES, BMP380_MAX_PRES);
> + return (u32)comp_press;
> +}
> +
> +static int bmp380_read_temp(struct bmp280_data *data, int *val)
> +{
> + int ret;
> + __le32 tmp = 0;

It's not an 32 bits, so use an array of 3 bytes instead.

> + u32 adc_temp;
> + s32 comp_temp;
> +
> + ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB, &tmp, 3);
> + if (ret < 0) {
> + dev_err(data->dev, "failed to read temperature\n");
> + return ret;
> + }
> +
> + adc_temp = le32_to_cpu(tmp);
As below, get_unaligned_le24()

> + if (adc_temp == BMP380_TEMP_SKIPPED) {
> + /* reading was skipped */
> + dev_err(data->dev, "reading temperature skipped\n");
> + return -EIO;
> + }
> + comp_temp = bmp380_compensate_temp(data, adc_temp);
> +
> + /*
> + * val might be NULL if we're called by the read_press routine,
> + * who only cares about the carry over t_fine value.
> + */
> + if (val) {
> + /* IIO reports temperatures in mC */
> + *val = comp_temp * 10;
> + return IIO_VAL_INT;
> + }
> +
> + return 0;
> +}
> +
> +static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> +{
> + int ret;
> + __le32 tmp = 0;
> + u32 adc_press;
> + s32 comp_press;
> +
> + /* Read and compensate temperature so we get a reading of t_fine. */
> + ret = bmp380_read_temp(data, NULL);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB, &tmp, 3);
> + if (ret < 0) {
> + dev_err(data->dev, "failed to read pressure\n");
> + return ret;
> + }
> +
> + adc_press = le32_to_cpu(tmp);
only 3 bytes read, so this is le24. We have conversion functions for that.
get_unaligned_le24()

> + if (adc_press == BMP380_PRESS_SKIPPED) {
> + /* reading was skipped */
> + dev_err(data->dev, "reading pressure skipped\n");
> + return -EIO;
> + }
> + comp_press = bmp380_compensate_press(data, adc_press);
> +
> + *val = comp_press;
> + /* Compensated pressure is in cPa (centipascals) */
> + *val2 = 100000;
> +
> + return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int bmp380_read_calib(struct bmp280_data *data,
> + struct bmp380_calib *calib, unsigned int chip)
> +{
> + int ret;
> + u8 buf[BMP380_CALIB_REG_COUNT];
> +
> + /* Read temperature calibration values. */
> + ret = regmap_bulk_read(data->regmap, BMP380_REG_CALIB_TEMP_START, buf,
> + BMP380_CALIB_REG_COUNT);
> + 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(buf, sizeof(buf));
> +
> + /* Parse calibration data */
> + calib->T1 = le16_from_bytes(buf[BMP380_T1], buf[BMP380_T1 + 1]);

This looks like a call to get_unaligned_le16() or similar. Use that instead.

> + calib->T2 = le16_from_bytes(buf[BMP380_T2], buf[BMP380_T2 + 1]);
> + calib->T3 = buf[BMP380_T3];
> + calib->P1 = le16_from_bytes(buf[BMP380_P1], buf[BMP380_P1 + 1]);
> + calib->P2 = le16_from_bytes(buf[BMP380_P2], buf[BMP380_P2 + 1]);
> + calib->P3 = buf[BMP380_P3];
> + calib->P4 = buf[BMP380_P4];
> + calib->P5 = le16_from_bytes(buf[BMP380_P5], buf[BMP380_P5 + 1]);
> + calib->P6 = le16_from_bytes(buf[BMP380_P6], buf[BMP380_P6 + 1]);
> + calib->P7 = buf[BMP380_P7];
> + calib->P8 = buf[BMP380_P8];
> + calib->P9 = le16_from_bytes(buf[BMP380_P9], buf[BMP380_P9 + 1]);
> + calib->P10 = buf[BMP380_P10];
> + calib->P11 = buf[BMP380_P11];
> +
> + return 0;
> +}
> +
> +static int bmp380_chip_config(struct bmp280_data *data)
> +{
> + u8 osrs;
> + unsigned int tmp;
> + 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 |
> + BMP380_MODE_NORMAL);
> + if (ret < 0) {
> + dev_err(data->dev,
> + "failed to write operation control register\n");
> + return ret;
> + }
> +
> + /* configure oversampling */
> + osrs = BMP380_OSRS_TEMP_X(data->oversampling_temp) |
> + BMP380_OSRS_PRESS_X(data->oversampling_press);
> +
> + ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
> + BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK,
> + osrs);
> + if (ret < 0) {
> + dev_err(data->dev, "failed to write oversampling register\n");
> + return ret;
> + }
> +
> + /* configure output data rate */
> + ret = regmap_write_bits(data->regmap, BMP380_REG_ODR,
> + BMP380_ODRS_MASK, BMP380_ODRS_50HZ);
> + if (ret < 0) {
> + dev_err(data->dev, "failed to write ODR selection register\n");
> + return ret;
> + }
> +
> + /* set filter data */
> + ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG,
> + BMP380_FILTER_MASK, BMP380_FILTER_3X);
> + if (ret < 0) {
> + dev_err(data->dev, "failed to write config register\n");
> + return ret;
> + }
> +
> + /* 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");
> + ret = -EINVAL;
return -EINVAL;
> + }
> +

return 0;

> + return ret;
> +}
> +
> +static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16, 32 };
> +
> +static const struct bmp280_chip_info bmp380_chip_info = {
> + .oversampling_temp_avail = bmp380_oversampling_avail,
> + .num_oversampling_temp_avail = ARRAY_SIZE(bmp380_oversampling_avail),
> +
> + .oversampling_press_avail = bmp380_oversampling_avail,
> + .num_oversampling_press_avail = ARRAY_SIZE(bmp380_oversampling_avail),
> +
> + .chip_config = bmp380_chip_config,
> + .read_temp = bmp380_read_temp,
> + .read_press = bmp380_read_press,
> +};
> +
> static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
> {
> int ret;
> @@ -1032,6 +1367,13 @@ int bmp280_common_probe(struct device *dev,
> data->oversampling_temp = ilog2(2);
> data->start_up_time = 2000;
> break;
> + case BMP380_CHIP_ID:
> + indio_dev->num_channels = 2;
> + data->chip_info = &bmp380_chip_info;
> + data->oversampling_press = ilog2(4);
> + data->oversampling_temp = ilog2(1);
> + data->start_up_time = 2000;

We should put the default values + num_channels into the chip_info
structure so all this switch would do is pick the right chip_info
structure.

Everything else is just copies from that structure done unconditionally
with no need to duplicate similar lines like here.

> + break;
> default:
> return -EINVAL;
> }
> @@ -1071,7 +1413,18 @@ int bmp280_common_probe(struct device *dev,
> }
>
> data->regmap = regmap;
> - ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
> + switch (chip) {
> + case BMP180_CHIP_ID:

Why not put the address into the chip info structure? That way
we avoid the switch statement.

> + case BMP280_CHIP_ID:
> + case BME280_CHIP_ID:
> + ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
> + break;
> + case BMP380_CHIP_ID:
> + ret = regmap_read(regmap, BMP380_REG_ID, &chip_id);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> if (ret < 0)
> return ret;
> if (chip_id != chip) {
> @@ -1080,6 +1433,13 @@ int bmp280_common_probe(struct device *dev,
> return -EINVAL;
> }
>
> + /* BMP3xx requires soft-reset as part of initialization */
> + if (chip_id == BMP380_CHIP_ID) {
> + 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;
> @@ -1091,20 +1451,32 @@ 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) {
> + switch (chip_id) {
> + case BMP180_CHIP_ID:
I would move these into callbacks in chip_info. No need for a switch statement
here then as you just call the right one via chip_info->read_calib()

> 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) {
> + break;
> + case BMP280_CHIP_ID:
> + case 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;
> }
> + break;
> + case BMP380_CHIP_ID:
> + ret = bmp380_read_calib(data, &data->calib.bmp380, chip_id);
> + if (ret < 0) {
> + dev_err(data->dev,
> + "failed to read calibration coefficients\n");
> + return ret;
> + }
> + break;
> }
>
> /*
> diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
> index 35045bd92846..31a8a0daa39a 100644
> --- a/drivers/iio/pressure/bmp280-i2c.c
> +++ b/drivers/iio/pressure/bmp280-i2c.c
> @@ -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 },
> diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
> index da136dbadc8f..b440fa41bf12 100644
> --- a/drivers/iio/pressure/bmp280-regmap.c
> +++ b/drivers/iio/pressure/bmp280-regmap.c
> @@ -72,6 +72,49 @@ static bool bmp280_is_volatile_reg(struct device *dev, unsigned int reg)
> }
> }
>
> +static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case BMP380_REG_CMD:
> + case BMP380_REG_CONFIG:
> + case BMP380_REG_FIFO_CONFIG_1:
> + case BMP380_REG_FIFO_CONFIG_2:
> + case BMP380_REG_FIFO_WATERMARK_LSB:
> + case BMP380_REG_FIFO_WATERMARK_MSB:
> + case BMP380_REG_POWER_CONTROL:
> + case BMP380_REG_INT_CONTROL:
> + case BMP380_REG_IF_CONFIG:
> + case BMP380_REG_ODR:
> + case BMP380_REG_OSR:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool bmp380_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case BMP380_REG_TEMP_XLSB:
> + case BMP380_REG_TEMP_LSB:
> + case BMP380_REG_TEMP_MSB:
> + case BMP380_REG_PRESS_XLSB:
> + case BMP380_REG_PRESS_LSB:
> + case BMP380_REG_PRESS_MSB:
> + case BMP380_REG_SENSOR_TIME_XLSB:
> + case BMP380_REG_SENSOR_TIME_LSB:
> + case BMP380_REG_SENSOR_TIME_MSB:
> + case BMP380_REG_INT_STATUS:
> + case BMP380_REG_FIFO_DATA:
> + case BMP380_REG_STATUS:
> + case BMP380_REG_ERROR:
> + case BMP380_REG_EVENT:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> const struct regmap_config bmp280_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -83,3 +126,15 @@ const struct regmap_config bmp280_regmap_config = {
> .volatile_reg = bmp280_is_volatile_reg,
> };
> EXPORT_SYMBOL(bmp280_regmap_config);
> +
> +const struct regmap_config bmp380_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = BMP380_REG_CMD,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .writeable_reg = bmp380_is_writeable_reg,
> + .volatile_reg = bmp380_is_volatile_reg,
> +};
> +EXPORT_SYMBOL(bmp380_regmap_config);
> diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> index 41f6cc56d229..303c41130343 100644
> --- a/drivers/iio/pressure/bmp280-spi.c
> +++ b/drivers/iio/pressure/bmp280-spi.c
> @@ -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);
> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index 57ba0e85db91..b4c122525ffe 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -3,6 +3,122 @@
> #include <linux/device.h>
> #include <linux/regmap.h>
>
> +/* BMP380 specific registers */
> +#define BMP380_REG_CMD 0x7E
> +#define BMP380_REG_CONFIG 0x1F
> +#define BMP380_REG_ODR 0X1D
> +#define BMP380_REG_OSR 0X1C
> +#define BMP380_REG_POWER_CONTROL 0X1B
> +#define BMP380_REG_IF_CONFIG 0X1A
> +#define BMP380_REG_INT_CONTROL 0X19
> +#define BMP380_REG_INT_STATUS 0X11
> +#define BMP380_REG_EVENT 0X10
> +#define BMP380_REG_STATUS 0X03
> +#define BMP380_REG_ERROR 0X02
> +#define BMP380_REG_ID 0X00
> +
> +#define BMP380_REG_FIFO_CONFIG_1 0X18
> +#define BMP380_REG_FIFO_CONFIG_2 0X17
> +#define BMP380_REG_FIFO_WATERMARK_MSB 0X16
> +#define BMP380_REG_FIFO_WATERMARK_LSB 0X15
> +#define BMP380_REG_FIFO_DATA 0X14
> +#define BMP380_REG_FIFO_LENGTH_MSB 0X13
> +#define BMP380_REG_FIFO_LENGTH_LSB 0X12
> +
> +#define BMP380_REG_SENSOR_TIME_MSB 0X0E
> +#define BMP380_REG_SENSOR_TIME_LSB 0X0D
> +#define BMP380_REG_SENSOR_TIME_XLSB 0X0C
> +
> +#define BMP380_REG_TEMP_MSB 0X09
> +#define BMP380_REG_TEMP_LSB 0X08
> +#define BMP380_REG_TEMP_XLSB 0X07
> +
> +#define BMP380_REG_PRESS_MSB 0X06
> +#define BMP380_REG_PRESS_LSB 0X05
> +#define BMP380_REG_PRESS_XLSB 0X04
> +
> +#define BMP380_REG_CALIB_TEMP_START 0x31
> +#define BMP380_CALIB_REG_COUNT 21
> +#define le16_from_bytes(lsb, msb) (le16_to_cpu(((((__le16) msb) << 8) | \
> + (__le16) lsb)))

That doesn't look right. (msb << 8) | lsb will be cpu endian, not le16.

> +
> +#define BMP380_FILTER_MASK GENMASK(3, 1)
> +#define BMP380_FILTER_OFF 0
> +#define BMP380_FILTER_1X BIT(1)
> +#define BMP380_FILTER_3X BIT(2)
> +#define BMP380_FILTER_7X (BIT(2) | BIT(1))
> +#define BMP380_FILTER_15X BIT(3)
> +#define BMP380_FILTER_31X (BIT(3) | BIT(1))
> +#define BMP380_FILTER_63X (BIT(3) | BIT(2))
> +#define BMP380_FILTER_127X (BIT(3) | BIT(2) | BIT(1))

these are values, 0,1,2,3,4,5,6,7 not a bunch of bits.
So use with FIELD_PREP()

> +
> +#define BMP380_OSRS_TEMP_MASK GENMASK(5, 3)
> +#define BMP380_OSRS_TEMP_X(osrs_t) ((osrs_t) << 3)
> +#define BMP380_OSRS_TEMP_1X BMP380_OSRS_TEMP_X(0)
> +#define BMP380_OSRS_TEMP_2X BMP380_OSRS_TEMP_X(1)
> +#define BMP380_OSRS_TEMP_4X BMP380_OSRS_TEMP_X(2)
> +#define BMP380_OSRS_TEMP_8X BMP380_OSRS_TEMP_X(3)
> +#define BMP380_OSRS_TEMP_16X BMP380_OSRS_TEMP_X(4)
> +#define BMP380_OSRS_TEMP_32X BMP380_OSRS_TEMP_X(5)
> +
> +#define BMP380_OSRS_PRESS_MASK (BIT(2) | BIT(1) | BIT(0))

GENMASK unless this is made up of 3 bits with separate definitions.
If it is, give defines for them and use them to build this full
mask.

> +#define BMP380_OSRS_PRESS_X(osrs_p) ((osrs_p) << 0)

FIELD_PREP()

> +#define BMP380_OSRS_PRESS_1X BMP380_OSRS_PRESS_X(0)
> +#define BMP380_OSRS_PRESS_2X BMP380_OSRS_PRESS_X(1)
> +#define BMP380_OSRS_PRESS_4X BMP380_OSRS_PRESS_X(2)
> +#define BMP380_OSRS_PRESS_8X BMP380_OSRS_PRESS_X(3)
> +#define BMP380_OSRS_PRESS_16X BMP380_OSRS_PRESS_X(4)
> +#define BMP380_OSRS_PRESS_32X BMP380_OSRS_PRESS_X(5)
> +
> +#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)
> +#define BMP380_CTRL_SENSORS_TEMP_EN BIT(1)
> +#define BMP380_MODE_MASK GENMASK(5, 4)
> +#define BMP380_MODE_SLEEP 0
> +#define BMP380_MODE_FORCED BIT(4)
> +#define BMP380_MODE_NORMAL (BIT(5) | BIT(4))

That doesn't look like either a mask or a combination of values,
rather it looks like the number 3 in a shifted field.

Use FIELD_GET/PREP along with the mask to extract a 2 bit
vlaue which you can then compare with 0, 1, 3

> +#define BMP380_STATUS_CMD_RDY_MASK BIT(4)
> +#define BMP380_STATUS_DRDY_PRESS_MASK BIT(5)
> +#define BMP380_STATUS_DRDY_TEMP_MASK BIT(6)
> +
> +#define BMP380_ERR_FATAL_MASK BIT(0)
> +#define BMP380_ERR_CMD_MASK BIT(1)

Stray tab?

> +#define BMP380_ERR_CONF_MASK BIT(2)
> +
> +#define BMP380_TEMP_SKIPPED 0x800000
> +#define BMP380_PRESS_SKIPPED 0x800000
> +

2022-06-25 17:02:29

by kernel test robot

[permalink] [raw]

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

2022-06-25 18:10:58

by kernel test robot

[permalink] [raw]

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

Hi Angel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v5.19-rc3 next-20220624]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Angel-Iglesias/dt-bindings-iio-pressure-bmp085-Add-BMP380-compatible-string/20220625-231424
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-a002
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 42a7ddb428c999229491b0effbb1a4059149fba8)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/56e3f8aecddacdbe204fbe5e28032ef2befae647
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Angel-Iglesias/dt-bindings-iio-pressure-bmp085-Add-BMP380-compatible-string/20220625-231424
git checkout 56e3f8aecddacdbe204fbe5e28032ef2befae647
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>

All error/warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/iio/pressure/bmp280-core.c:1000:10: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
if (tmp && BMP380_ERR_CONF_MASK) {
^ ~~~~~~~~~~~~~~~~~~~~
drivers/iio/pressure/bmp280-core.c:1000:10: note: use '&' for a bitwise operation
if (tmp && BMP380_ERR_CONF_MASK) {
^~
&
drivers/iio/pressure/bmp280-core.c:1000:10: note: remove constant to silence this warning
if (tmp && BMP380_ERR_CONF_MASK) {
~^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
--
>> ERROR: modpost: "__divdi3" [drivers/iio/pressure/bmp280.ko] undefined!

vim +1000 drivers/iio/pressure/bmp280-core.c

945
946 static int bmp380_chip_config(struct bmp280_data *data)
947 {
948 u8 osrs;
949 unsigned int tmp;
950 int ret;
951
952 /* configure power control register */
953 ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
954 BMP380_CTRL_SENSORS_MASK |
955 BMP380_MODE_MASK,
956 BMP380_CTRL_SENSORS_PRESS_EN |
957 BMP380_CTRL_SENSORS_TEMP_EN |
958 BMP380_MODE_NORMAL);
959 if (ret < 0) {
960 dev_err(data->dev,
961 "failed to write operation control register\n");
962 return ret;
963 }
964
965 /* configure oversampling */
966 osrs = BMP380_OSRS_TEMP_X(data->oversampling_temp) |
967 BMP380_OSRS_PRESS_X(data->oversampling_press);
968
969 ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
970 BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK,
971 osrs);
972 if (ret < 0) {
973 dev_err(data->dev, "failed to write oversampling register\n");
974 return ret;
975 }
976
977 /* configure output data rate */
978 ret = regmap_write_bits(data->regmap, BMP380_REG_ODR,
979 BMP380_ODRS_MASK, BMP380_ODRS_50HZ);
980 if (ret < 0) {
981 dev_err(data->dev, "failed to write ODR selection register\n");
982 return ret;
983 }
984
985 /* set filter data */
986 ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG,
987 BMP380_FILTER_MASK, BMP380_FILTER_3X);
988 if (ret < 0) {
989 dev_err(data->dev, "failed to write config register\n");
990 return ret;
991 }
992
993 /* check config error flag */
994 ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);
995 if (ret < 0) {
996 dev_err(data->dev,
997 "failed to read error register\n");
998 return ret;
999 }
> 1000 if (tmp && BMP380_ERR_CONF_MASK) {
1001 dev_warn(data->dev,
1002 "sensor flagged configuration as incompatible\n");
1003 ret = -EINVAL;
1004 }
1005
1006 return ret;
1007 }
1008

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

2022-06-25 20:20:33

by kernel test robot

[permalink] [raw]

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

2022-06-25 21:00:39

by kernel test robot

[permalink] [raw]

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

2022-06-26 01:41:15

by kernel test robot

[permalink] [raw]

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

Hi Angel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v5.19-rc3 next-20220624]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Angel-Iglesias/dt-bindings-iio-pressure-bmp085-Add-BMP380-compatible-string/20220625-231424
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-s001
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-31-g4880bd19-dirty
# https://github.com/intel-lab-lkp/linux/commit/56e3f8aecddacdbe204fbe5e28032ef2befae647
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Angel-Iglesias/dt-bindings-iio-pressure-bmp085-Add-BMP380-compatible-string/20220625-231424
git checkout 56e3f8aecddacdbe204fbe5e28032ef2befae647
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/iio/pressure/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>

sparse warnings: (new ones prefixed by >>)
drivers/iio/pressure/bmp280-core.c:928:21: sparse: sparse: cast to restricted __le16
>> drivers/iio/pressure/bmp280-core.c:928:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:928:21: sparse: sparse: cast to restricted __le16
>> drivers/iio/pressure/bmp280-core.c:928:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:928:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:929:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:929:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:929:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:929:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:929:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:931:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:931:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:931:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:931:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:931:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:932:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:932:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:932:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:932:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:932:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:935:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:935:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:935:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:935:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:935:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:936:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:936:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:936:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:936:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:936:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:939:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:939:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:939:21: sparse: sparse: cast to restricted __le16
drivers/iio/pressure/bmp280-core.c:939:21: sparse: sparse: restricted __le16 degrades to integer
drivers/iio/pressure/bmp280-core.c:939:21: sparse: sparse: cast to restricted __le16

vim +928 drivers/iio/pressure/bmp280-core.c

908
909 static int bmp380_read_calib(struct bmp280_data *data,
910 struct bmp380_calib *calib, unsigned int chip)
911 {
912 int ret;
913 u8 buf[BMP380_CALIB_REG_COUNT];
914
915 /* Read temperature calibration values. */
916 ret = regmap_bulk_read(data->regmap, BMP380_REG_CALIB_TEMP_START, buf,
917 BMP380_CALIB_REG_COUNT);
918 if (ret < 0) {
919 dev_err(data->dev,
920 "failed to read temperature calibration parameters\n");
921 return ret;
922 }
923
924 /* Toss the temperature calibration data into the entropy pool */
925 add_device_randomness(buf, sizeof(buf));
926
927 /* Parse calibration data */
> 928 calib->T1 = le16_from_bytes(buf[BMP380_T1], buf[BMP380_T1 + 1]);
929 calib->T2 = le16_from_bytes(buf[BMP380_T2], buf[BMP380_T2 + 1]);
930 calib->T3 = buf[BMP380_T3];
931 calib->P1 = le16_from_bytes(buf[BMP380_P1], buf[BMP380_P1 + 1]);
932 calib->P2 = le16_from_bytes(buf[BMP380_P2], buf[BMP380_P2 + 1]);
933 calib->P3 = buf[BMP380_P3];
934 calib->P4 = buf[BMP380_P4];
935 calib->P5 = le16_from_bytes(buf[BMP380_P5], buf[BMP380_P5 + 1]);
936 calib->P6 = le16_from_bytes(buf[BMP380_P6], buf[BMP380_P6 + 1]);
937 calib->P7 = buf[BMP380_P7];
938 calib->P8 = buf[BMP380_P8];
939 calib->P9 = le16_from_bytes(buf[BMP380_P9], buf[BMP380_P9 + 1]);
940 calib->P10 = buf[BMP380_P10];
941 calib->P11 = buf[BMP380_P11];
942
943 return 0;
944 }
945

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

2022-06-27 08:20:49

by Dan Carpenter

[permalink] [raw]

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

Hi Angel,

url: https://github.com/intel-lab-lkp/linux/commits/Angel-Iglesias/dt-bindings-iio-pressure-bmp085-Add-BMP380-compatible-string/20220625-231424
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-m021
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>

smatch warnings:
drivers/iio/pressure/bmp280-core.c:1000 bmp380_chip_config() warn: should this be a bitwise op?

vim +1000 drivers/iio/pressure/bmp280-core.c

56e3f8aecddacd Angel Iglesias 2022-06-25 988 if (ret < 0) {
56e3f8aecddacd Angel Iglesias 2022-06-25 989 dev_err(data->dev, "failed to write config register\n");
56e3f8aecddacd Angel Iglesias 2022-06-25 990 return ret;
56e3f8aecddacd Angel Iglesias 2022-06-25 991 }
56e3f8aecddacd Angel Iglesias 2022-06-25 992
56e3f8aecddacd Angel Iglesias 2022-06-25 993 /* check config error flag */
56e3f8aecddacd Angel Iglesias 2022-06-25 994 ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);
56e3f8aecddacd Angel Iglesias 2022-06-25 995 if (ret < 0) {
56e3f8aecddacd Angel Iglesias 2022-06-25 996 dev_err(data->dev,
56e3f8aecddacd Angel Iglesias 2022-06-25 997 "failed to read error register\n");
56e3f8aecddacd Angel Iglesias 2022-06-25 998 return ret;
56e3f8aecddacd Angel Iglesias 2022-06-25 999 }
56e3f8aecddacd Angel Iglesias 2022-06-25 @1000 if (tmp && BMP380_ERR_CONF_MASK) {
^^^^^^^^^^^^^^^^^^^^^^^
Looks like & BMP380_ERR_CONF_MASK was intended.

56e3f8aecddacd Angel Iglesias 2022-06-25 1001 dev_warn(data->dev,
56e3f8aecddacd Angel Iglesias 2022-06-25 1002 "sensor flagged configuration as incompatible\n");
56e3f8aecddacd Angel Iglesias 2022-06-25 1003 ret = -EINVAL;
56e3f8aecddacd Angel Iglesias 2022-06-25 1004 }
56e3f8aecddacd Angel Iglesias 2022-06-25 1005
56e3f8aecddacd Angel Iglesias 2022-06-25 1006 return ret;
56e3f8aecddacd Angel Iglesias 2022-06-25 1007 }

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

2022-06-27 15:17:56

by Angel Iglesias

[permalink] [raw]

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

On lun, 2022-06-27 at 10:37 +0300, Dan Carpenter wrote:
> Hi Angel,
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Angel-Iglesias/dt-bindings-iio-pressure-bmp085-Add-BMP380-compatible-string/20220625-231424
> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: i386-randconfig-m021
> compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> smatch warnings:
> drivers/iio/pressure/bmp280-core.c:1000 bmp380_chip_config() warn:
> should this be a bitwise op?
>
> vim +1000 drivers/iio/pressure/bmp280-core.c
>
> 56e3f8aecddacd Angel Iglesias 2022-06-25   988          if (ret < 0)
> {
> 56e3f8aecddacd Angel Iglesias 2022-06-25
> 989                  dev_err(data->dev, "failed to write config
> register\n");
> 56e3f8aecddacd Angel Iglesias 2022-06-25
> 990                  return ret;
> 56e3f8aecddacd Angel Iglesias 2022-06-25   991          }
> 56e3f8aecddacd Angel Iglesias 2022-06-25   992
> 56e3f8aecddacd Angel Iglesias 2022-06-25   993          /* check
> config error flag */
> 56e3f8aecddacd Angel Iglesias 2022-06-25   994          ret =
> regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);
> 56e3f8aecddacd Angel Iglesias 2022-06-25   995          if (ret < 0)
> {
> 56e3f8aecddacd Angel Iglesias 2022-06-25
> 996                  dev_err(data->dev,
> 56e3f8aecddacd Angel Iglesias 2022-06-25
> 997                          "failed to read error register\n");
> 56e3f8aecddacd Angel Iglesias 2022-06-25
> 998                  return ret;
> 56e3f8aecddacd Angel Iglesias 2022-06-25   999          }
> 56e3f8aecddacd Angel Iglesias 2022-06-25 @1000          if (tmp &&
> BMP380_ERR_CONF_MASK) {
>
> ^^^^^^^^^^^^^^^^^^^^^^^
> Looks like & BMP380_ERR_CONF_MASK was intended.
>
> 56e3f8aecddacd Angel Iglesias 2022-06-25
> 1001                  dev_warn(data->dev,
> 56e3f8aecddacd Angel Iglesias 2022-06-25
> 1002                           "sensor flagged configuration as
> incompatible\n");
> 56e3f8aecddacd Angel Iglesias 2022-06-25 1003                  ret =
> -EINVAL;
> 56e3f8aecddacd Angel Iglesias 2022-06-25 1004          }
> 56e3f8aecddacd Angel Iglesias 2022-06-25 1005
> 56e3f8aecddacd Angel Iglesias 2022-06-25 1006          return ret;
> 56e3f8aecddacd Angel Iglesias 2022-06-25 1007 }
>
Thanks! that is an awkward mistake, my bad!

2022-06-27 15:55:24

by Angel Iglesias

[permalink] [raw]

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

On sáb, 2022-06-25 at 16:46 +0100, Jonathan Cameron wrote:
> On Sat, 25 Jun 2022 17:09:12 +0200
> Angel Iglesias <[email protected]> wrote:
>
> > Adds compatibility with the new generation of this sensor, the
> > BMP380
> >
> > Included 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]>
>
> Hi Angel,
>
> A few comments inline, but mostly looks good to me.
>
> Jonathan
>
> > ---
> > drivers/iio/pressure/bmp280-core.c   | 378
> > ++++++++++++++++++++++++++-
> > 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        | 118 +++++++++
> > 5 files changed, 558 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c
> > b/drivers/iio/pressure/bmp280-core.c
> > index bf8167f43c56..3887475a9059 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -74,6 +74,24 @@ struct bmp280_calib {
> >         s8 H6;
> > };
> >
> > +/* See datasheet Section 3.11.1. */
> > +struct bmp380_calib {
> > +       u16 T1;
> > +       u16 T2;
> > +       s8 T3;
> > +       s16 P1;
> > +       s16 P2;
> > +       s8 P3;
> > +       s8 P4;
> > +       u16 P5;
> > +       u16 P6;
> > +       s8 P7;
> > +       s8 P8;
> > +       s16 P9;
> > +       s8 P10;
> > +       s8 P11;
> > +};
> > +
> > static const char *const bmp280_supply_names[] = {
> >         "vddd", "vdda"
> > };
> > @@ -90,6 +108,7 @@ struct bmp280_data {
> >         union {
> >                 struct bmp180_calib bmp180;
> >                 struct bmp280_calib bmp280;
> > +               struct bmp380_calib bmp380;
> >         } calib;
> >         struct regulator_bulk_data supplies[BMP280_NUM_SUPPLIES];
> >         unsigned int start_up_time; /* in microseconds */
> > @@ -129,6 +148,25 @@ struct bmp280_chip_info {
> > enum { T1, T2, T3 };
> > enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
> >
> > +enum {
> > +       /* Temperature calib indexes */
> > +       BMP380_T1 = 0,
> > +       BMP380_T2 = 2,
> > +       BMP380_T3 = 4,
> > +       /* Pressure calib indexes */
> > +       BMP380_P1 = 5,
> > +       BMP380_P2 = 7,
> > +       BMP380_P3 = 9,
> > +       BMP380_P4 = 10,
> > +       BMP380_P5 = 11,
> > +       BMP380_P6 = 13,
> > +       BMP380_P7 = 15,
> > +       BMP380_P8 = 16,
> > +       BMP380_P9 = 17,
> > +       BMP380_P10 = 19,
> > +       BMP380_P11 = 20
> > +};
> > +
> > static const struct iio_chan_spec bmp280_channels[] = {
> >         {
> >                 .type = IIO_PRESSURE,
> > @@ -252,6 +290,7 @@ static int bmp280_read_calib(struct bmp280_data
> > *data,
> >
> >         return 0;
> > }
> > +
> Stray change. It's a good one, but doesn't belong in this patch.
> Feel free to
> do another patch tidying up whitespace in the driver.

Thanks! I'll drop this change for the next version of this series.

> > /*
> > * Returns humidity in percent, resolution is 0.01 percent. Output
> > value of
> > * "47445" represents 47445/1024 = 46.333 %RH.
> > @@ -685,6 +724,302 @@ static const struct bmp280_chip_info
> > bme280_chip_info = {
> >         .read_humid = bmp280_read_humid,
> > };
> >
> > +/* Send a command to BMP3XX sensors */
> > +static int bmp380_cmd(struct bmp280_data *data, u8 cmd)
> > +{
> > +       int ret;
> > +       unsigned int reg;
> > +
> > +       /* check if device is ready to process a command */
> > +       ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to read error
> > register\n");
> > +               goto failure;
> > +       }
> > +       if (!(cmd & BMP380_STATUS_CMD_RDY_MASK)) {
> > +               dev_err(data->dev, "device is not ready to accept
> > commands\n");
> > +               ret = -EBUSY;
> > +               goto failure;
> > +       }
> > +
> > +       /* send command to process */
> > +       ret = regmap_write(data->regmap, BMP380_REG_CMD, cmd);
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to send command to
> > device\n");
> > +               goto failure;
> > +       }
> > +       /* wait for 2ms for command to be proccessed */
> > +       usleep_range(2000, 2500);
> > +       /* check for command processing error */
> > +       ret = regmap_read(data->regmap, BMP380_REG_ERROR, &reg);
> > +       if (ret) {
> > +               dev_err(data->dev, "error reading ERROR reg\n");
> > +               goto failure;
> > +       }
> > +       if (reg & BMP380_ERR_CMD_MASK) {
> > +               dev_err(data->dev, "error processing command
> > 0x%X\n", cmd);
> > +               ret = -EINVAL;
>
> as nothing to do in failure path, direct return from here preferred.
>                 return -EINVAL;
> Same for all similar cases.

Thanks! I'll avoid unnecessary goto's when there's no further actions
to do after a failure

> > +               goto failure;
> > +       }
> > +       dev_dbg(data->dev, "Command 0x%X proccessed
> > successfully\n", cmd);
> > +
> > +failure:
> > +       return ret;
> > +}
> > +
> > +/*
> > + * Returns temperature in DegC, resolution is 0.01 DegC. Output
> > value of
> > + * "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
> > + */
> > +static s32 bmp380_compensate_temp(struct bmp280_data *data, u32
> > adc_temp)
> > +{
> > +       s64 var1, var2, var3, var4, var5, var6, comp_temp;
> > +       struct bmp380_calib *calib = &data->calib.bmp380;
> > +
> > +       var1 = ((s64) adc_temp) - (((s64) calib->T1) << 8);
> > +       var2 = var1 * ((s64) calib->T2);
> > +       var3 = var1 * var1;
> > +       var4 = var3 * ((s64) calib->T3);
> > +       var5 = (var2 << 18) + var4;
> > +       var6 = var5 >> 32;
> > +       data->t_fine = (s32) var6;
> > +       comp_temp = (var6 * 25) >> 14;
> > +
> > +       comp_temp = clamp_val(comp_temp, BMP380_MIN_TEMP,
> > BMP380_MAX_TEMP);
> > +       return (s32) comp_temp;
> > +}
> > +
> > +/*
> > + * Returns pressure in Pa as unsigned 32 bit integer in fractional
> > Pascal.
> > + * 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
> > + */
> > +static u32 bmp380_compensate_press(struct bmp280_data *data, u32
> > adc_press)
> > +{
> > +       s64 var1, var2, var3, var4, var5, var6, offset,
> > sensitivity;
> > +       u64 comp_press;
> > +       struct bmp380_calib *calib = &data->calib.bmp380;
> > +
> > +       var1 = ((s64)data->t_fine) * ((s64)data->t_fine);
> > +       var2 = var1 >> 6;
> > +       var3 = (var2 * ((s64) data->t_fine)) >> 8;
> > +       var4 = (((s64)calib->P8) * var3) >> 5;
> > +       var5 = (((s64) calib->P7) * var1) << 4;
> > +       var6 = (((s64) calib->P6) * ((s64)data->t_fine)) << 22;
> > +       offset = (((s64)calib->P5) << 47) + var4 + var5 + var6;
> > +       var2 = (((s64)calib->P4) * var3) >> 5;
> > +       var4 = (((s64) calib->P3) * var1) << 2;
> > +       var5 = (((s64) calib->P2) - ((s64) 1<<14)) *
> > +               (((s64)data->t_fine) << 21);
> > +       sensitivity = ((((s64) calib->P1) - ((s64) 1 << 14)) << 46)
> > +
> > +                       var2 + var4 + var5;
> > +       var1 = (sensitivity >> 24) * ((s64)adc_press);
> > +       var2 = ((s64)calib->P10) * ((s64) data->t_fine);
> > +       var3 = var2 + (((s64) calib->P9) << 16);
> > +       var4 = (var3 * ((s64)adc_press)) >> 13;
> > +
> > +       /* dividing by 10 followed by multiplying by 10
> > +        * To avoid overflow caused by (uncomp_data->pressure *
> > partial_data4)
> > +        */
> > +       var5 = (((s64)adc_press) * (var4 / 10)) >> 9;
> > +       var5 *= 10;
> > +       var6 = ((s64)adc_press) * ((s64)adc_press);
> > +       var2 = (((s64)calib->P11) * var6) >> 16;
> > +       var3 = (var2 * ((s64)adc_press)) >> 7;
> > +       var4 = (offset >> 2) + var1 + var5 + var3;
> > +       comp_press = ((u64)var4 * 25) >> 40;
> > +
> > +       comp_press = clamp_val(comp_press, BMP380_MIN_PRES,
> > BMP380_MAX_PRES);
> > +       return (u32)comp_press;
> > +}
> > +
> > +static int bmp380_read_temp(struct bmp280_data *data, int *val)
> > +{
> > +       int ret;
> > +       __le32 tmp = 0;
>
> It's not an 32 bits, so use an array of 3 bytes instead.

Understood!

> > +       u32 adc_temp;
> > +       s32 comp_temp;
> > +
> > +       ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB,
> > &tmp, 3);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to read temperature\n");
> > +               return ret;
> > +       }
> > +
> > +       adc_temp = le32_to_cpu(tmp);
> As below, get_unaligned_le24()

Ok! Thanks, I'll fix all le32_to_cpu() missuses

> > +       if (adc_temp == BMP380_TEMP_SKIPPED) {
> > +               /* reading was skipped */
> > +               dev_err(data->dev, "reading temperature
> > skipped\n");
> > +               return -EIO;
> > +       }
> > +       comp_temp = bmp380_compensate_temp(data, adc_temp);
> > +
> > +       /*
> > +        * val might be NULL if we're called by the read_press
> > routine,
> > +        * who only cares about the carry over t_fine value.
> > +        */
> > +       if (val) {
> > +               /* IIO reports temperatures in mC */
> > +               *val = comp_temp * 10;
> > +               return IIO_VAL_INT;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int bmp380_read_press(struct bmp280_data *data, int *val,
> > int *val2)
> > +{
> > +       int ret;
> > +       __le32 tmp = 0;
> > +       u32 adc_press;
> > +       s32 comp_press;
> > +
> > +       /* Read and compensate temperature so we get a reading of
> > t_fine. */
> > +       ret = bmp380_read_temp(data, NULL);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> > &tmp, 3);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to read pressure\n");
> > +               return ret;
> > +       }
> > +
> > +       adc_press = le32_to_cpu(tmp);
> only 3 bytes read, so this is le24. We have conversion functions for
> that.
> get_unaligned_le24()
>
>
> > +       if (adc_press == BMP380_PRESS_SKIPPED) {
> > +               /* reading was skipped */
> > +               dev_err(data->dev, "reading pressure skipped\n");
> > +               return -EIO;
> > +       }
> > +       comp_press = bmp380_compensate_press(data, adc_press);
> > +
> > +       *val = comp_press;
> > +       /* Compensated pressure is in cPa (centipascals) */
> > +       *val2 = 100000;
> > +
> > +       return IIO_VAL_FRACTIONAL;
> > +}
> > +
> > +static int bmp380_read_calib(struct bmp280_data *data,
> > +                            struct bmp380_calib *calib, unsigned
> > int chip)
> > +{
> > +       int ret;
> > +       u8 buf[BMP380_CALIB_REG_COUNT];
> > +
> > +       /* Read temperature calibration values. */
> > +       ret = regmap_bulk_read(data->regmap,
> > BMP380_REG_CALIB_TEMP_START, buf,
> > +                              BMP380_CALIB_REG_COUNT);
> > +       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(buf, sizeof(buf));
> > +
> > +       /* Parse calibration data */
> > +       calib->T1 = le16_from_bytes(buf[BMP380_T1], buf[BMP380_T1 +
> > 1]);
>
> This looks like a call to get_unaligned_le16() or similar. Use that
> instead.

Thanks! This macro was a bit of a mess. I wasn't aware of the
get_unaligned_* helpers

> > +       calib->T2 = le16_from_bytes(buf[BMP380_T2], buf[BMP380_T2 +
> > 1]);
> > +       calib->T3 = buf[BMP380_T3];
> > +       calib->P1 = le16_from_bytes(buf[BMP380_P1], buf[BMP380_P1 +
> > 1]);
> > +       calib->P2 = le16_from_bytes(buf[BMP380_P2], buf[BMP380_P2 +
> > 1]);
> > +       calib->P3 = buf[BMP380_P3];
> > +       calib->P4 = buf[BMP380_P4];
> > +       calib->P5 = le16_from_bytes(buf[BMP380_P5], buf[BMP380_P5 +
> > 1]);
> > +       calib->P6 = le16_from_bytes(buf[BMP380_P6], buf[BMP380_P6 +
> > 1]);
> > +       calib->P7 = buf[BMP380_P7];
> > +       calib->P8 = buf[BMP380_P8];
> > +       calib->P9 = le16_from_bytes(buf[BMP380_P9], buf[BMP380_P9 +
> > 1]);
> > +       calib->P10 = buf[BMP380_P10];
> > +       calib->P11 = buf[BMP380_P11];
> > +
> > +       return 0;
> > +}
> > +
> > +static int bmp380_chip_config(struct bmp280_data *data)
> > +{
> > +       u8 osrs;
> > +       unsigned int tmp;
> > +       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 |
> > +                               BMP380_MODE_NORMAL);
> > +       if (ret < 0) {
> > +               dev_err(data->dev,
> > +                       "failed to write operation control
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* configure oversampling */
> > +       osrs = BMP380_OSRS_TEMP_X(data->oversampling_temp) |
> > +                               BMP380_OSRS_PRESS_X(data-
> > >oversampling_press);
> > +
> > +       ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
> > +                               BMP380_OSRS_TEMP_MASK |
> > BMP380_OSRS_PRESS_MASK,
> > +                               osrs);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to write oversampling
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* configure output data rate */
> > +       ret = regmap_write_bits(data->regmap, BMP380_REG_ODR,
> > +                               BMP380_ODRS_MASK,
> > BMP380_ODRS_50HZ);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to write ODR selection
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* set filter data */
> > +       ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG,
> > +                               BMP380_FILTER_MASK,
> > BMP380_FILTER_3X);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to write config
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* 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");
> > +               ret = -EINVAL;
>                 return -EINVAL;
> > +       }
> > +
>
> return 0;

Ok!

> > +       return ret;
> > +}
> > +
> > +static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16,
> > 32 };
> > +
> > +static const struct bmp280_chip_info bmp380_chip_info = {
> > +       .oversampling_temp_avail = bmp380_oversampling_avail,
> > +       .num_oversampling_temp_avail =
> > ARRAY_SIZE(bmp380_oversampling_avail),
> > +
> > +       .oversampling_press_avail = bmp380_oversampling_avail,
> > +       .num_oversampling_press_avail =
> > ARRAY_SIZE(bmp380_oversampling_avail),
> > +
> > +       .chip_config = bmp380_chip_config,
> > +       .read_temp = bmp380_read_temp,
> > +       .read_press = bmp380_read_press,
> > +};
> > +
> > static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
> > {
> >         int ret;
> > @@ -1032,6 +1367,13 @@ int bmp280_common_probe(struct device *dev,
> >                 data->oversampling_temp = ilog2(2);
> >                 data->start_up_time = 2000;
> >                 break;
> > +       case BMP380_CHIP_ID:
> > +               indio_dev->num_channels = 2;
> > +               data->chip_info = &bmp380_chip_info;
> > +               data->oversampling_press = ilog2(4);
> > +               data->oversampling_temp = ilog2(1);
> > +               data->start_up_time = 2000;
>
>
> We should put the default values + num_channels into the chip_info
> structure so all this switch would do is pick the right chip_info
> structure.
>
> Everything else is just copies from that structure done
> unconditionally
> with no need to duplicate similar lines like here.

Yes, this block would be much cleaner doing this simple change. Thanks
for the input!

> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -1071,7 +1413,18 @@ int bmp280_common_probe(struct device *dev,
> >         }
> >
> >         data->regmap = regmap;
> > -       ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
> > +       switch (chip) {
> > +       case BMP180_CHIP_ID:
>
> Why not put the address into the chip info structure? That way
> we avoid the switch statement.

Ok!

> > +       case BMP280_CHIP_ID:
> > +       case BME280_CHIP_ID:
> > +               ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
> > +               break;
> > +       case BMP380_CHIP_ID:
> > +               ret = regmap_read(regmap, BMP380_REG_ID, &chip_id);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +       }
> >         if (ret < 0)
> >                 return ret;
> >         if (chip_id != chip) {
> > @@ -1080,6 +1433,13 @@ int bmp280_common_probe(struct device *dev,
> >                 return -EINVAL;
> >         }
> >
> > +       /* BMP3xx requires soft-reset as part of initialization */
> > +       if (chip_id == BMP380_CHIP_ID) {
> > +               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;
> > @@ -1091,20 +1451,32 @@ 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) {
> > +       switch (chip_id) {
> > +       case BMP180_CHIP_ID:
> I would move these into callbacks in chip_info. No need for a switch
> statement
> here then as you just call the right one via chip_info->read_calib()

Ok!

> >                 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) {
> > +               break;
> > +       case BMP280_CHIP_ID:
> > +       case 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;
> >                 }
> > +               break;
> > +       case BMP380_CHIP_ID:
> > +               ret = bmp380_read_calib(data, &data->calib.bmp380,
> > chip_id);
> > +               if (ret < 0) {
> > +                       dev_err(data->dev,
> > +                               "failed to read calibration
> > coefficients\n");
> > +                       return ret;
> > +               }
> > +               break;
> >         }
> >
> >         /*
> > diff --git a/drivers/iio/pressure/bmp280-i2c.c
> > b/drivers/iio/pressure/bmp280-i2c.c
> > index 35045bd92846..31a8a0daa39a 100644
> > --- a/drivers/iio/pressure/bmp280-i2c.c
> > +++ b/drivers/iio/pressure/bmp280-i2c.c
> > @@ -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 },
> > diff --git a/drivers/iio/pressure/bmp280-regmap.c
> > b/drivers/iio/pressure/bmp280-regmap.c
> > index da136dbadc8f..b440fa41bf12 100644
> > --- a/drivers/iio/pressure/bmp280-regmap.c
> > +++ b/drivers/iio/pressure/bmp280-regmap.c
> > @@ -72,6 +72,49 @@ static bool bmp280_is_volatile_reg(struct device
> > *dev, unsigned int reg)
> >         }
> > }
> >
> > +static bool bmp380_is_writeable_reg(struct device *dev, unsigned
> > int reg)
> > +{
> > +       switch (reg) {
> > +       case BMP380_REG_CMD:
> > +       case BMP380_REG_CONFIG:
> > +       case BMP380_REG_FIFO_CONFIG_1:
> > +       case BMP380_REG_FIFO_CONFIG_2:
> > +       case BMP380_REG_FIFO_WATERMARK_LSB:
> > +       case BMP380_REG_FIFO_WATERMARK_MSB:
> > +       case BMP380_REG_POWER_CONTROL:
> > +       case BMP380_REG_INT_CONTROL:
> > +       case BMP380_REG_IF_CONFIG:
> > +       case BMP380_REG_ODR:
> > +       case BMP380_REG_OSR:
> > +               return true;
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> > +static bool bmp380_is_volatile_reg(struct device *dev, unsigned
> > int reg)
> > +{
> > +       switch (reg) {
> > +       case BMP380_REG_TEMP_XLSB:
> > +       case BMP380_REG_TEMP_LSB:
> > +       case BMP380_REG_TEMP_MSB:
> > +       case BMP380_REG_PRESS_XLSB:
> > +       case BMP380_REG_PRESS_LSB:
> > +       case BMP380_REG_PRESS_MSB:
> > +       case BMP380_REG_SENSOR_TIME_XLSB:
> > +       case BMP380_REG_SENSOR_TIME_LSB:
> > +       case BMP380_REG_SENSOR_TIME_MSB:
> > +       case BMP380_REG_INT_STATUS:
> > +       case BMP380_REG_FIFO_DATA:
> > +       case BMP380_REG_STATUS:
> > +       case BMP380_REG_ERROR:
> > +       case BMP380_REG_EVENT:
> > +               return true;
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> > const struct regmap_config bmp280_regmap_config = {
> >         .reg_bits = 8,
> >         .val_bits = 8,
> > @@ -83,3 +126,15 @@ const struct regmap_config bmp280_regmap_config
> > = {
> >         .volatile_reg = bmp280_is_volatile_reg,
> > };
> > EXPORT_SYMBOL(bmp280_regmap_config);
> > +
> > +const struct regmap_config bmp380_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +
> > +       .max_register = BMP380_REG_CMD,
> > +       .cache_type = REGCACHE_RBTREE,
> > +
> > +       .writeable_reg = bmp380_is_writeable_reg,
> > +       .volatile_reg = bmp380_is_volatile_reg,
> > +};
> > +EXPORT_SYMBOL(bmp380_regmap_config);
> > diff --git a/drivers/iio/pressure/bmp280-spi.c
> > b/drivers/iio/pressure/bmp280-spi.c
> > index 41f6cc56d229..303c41130343 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -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);
> > diff --git a/drivers/iio/pressure/bmp280.h
> > b/drivers/iio/pressure/bmp280.h
> > index 57ba0e85db91..b4c122525ffe 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -3,6 +3,122 @@
> > #include <linux/device.h>
> > #include <linux/regmap.h>
> >
> > +/* BMP380 specific registers */
> > +#define BMP380_REG_CMD                 0x7E
> > +#define BMP380_REG_CONFIG              0x1F
> > +#define BMP380_REG_ODR                 0X1D
> > +#define BMP380_REG_OSR                 0X1C
> > +#define BMP380_REG_POWER_CONTROL       0X1B
> > +#define BMP380_REG_IF_CONFIG           0X1A
> > +#define BMP380_REG_INT_CONTROL         0X19
> > +#define BMP380_REG_INT_STATUS          0X11
> > +#define BMP380_REG_EVENT               0X10
> > +#define BMP380_REG_STATUS              0X03
> > +#define BMP380_REG_ERROR               0X02
> > +#define BMP380_REG_ID                  0X00
> > +
> > +#define BMP380_REG_FIFO_CONFIG_1       0X18
> > +#define BMP380_REG_FIFO_CONFIG_2       0X17
> > +#define BMP380_REG_FIFO_WATERMARK_MSB  0X16
> > +#define BMP380_REG_FIFO_WATERMARK_LSB  0X15
> > +#define BMP380_REG_FIFO_DATA           0X14
> > +#define BMP380_REG_FIFO_LENGTH_MSB     0X13
> > +#define BMP380_REG_FIFO_LENGTH_LSB     0X12
> > +
> > +#define BMP380_REG_SENSOR_TIME_MSB     0X0E
> > +#define BMP380_REG_SENSOR_TIME_LSB     0X0D
> > +#define BMP380_REG_SENSOR_TIME_XLSB    0X0C
> > +
> > +#define BMP380_REG_TEMP_MSB            0X09
> > +#define BMP380_REG_TEMP_LSB            0X08
> > +#define BMP380_REG_TEMP_XLSB           0X07
> > +
> > +#define BMP380_REG_PRESS_MSB           0X06
> > +#define BMP380_REG_PRESS_LSB           0X05
> > +#define BMP380_REG_PRESS_XLSB          0X04
> > +
> > +#define BMP380_REG_CALIB_TEMP_START    0x31
> > +#define BMP380_CALIB_REG_COUNT         21
> > +#define le16_from_bytes(lsb, msb)      (le16_to_cpu(((((__le16)
> > msb) << 8) | \
> > +                                                       (__le16)
> > lsb)))
>
> That doesn't look right. (msb << 8) | lsb will be cpu endian, not
> le16.

Yes, you're right! And with the changes to get_unaligned_le16() I can
drop this macro and use the utilities already present in the kernel

> > +
> > +#define BMP380_FILTER_MASK             GENMASK(3, 1)
> > +#define BMP380_FILTER_OFF              0
> > +#define BMP380_FILTER_1X               BIT(1)
> > +#define BMP380_FILTER_3X               BIT(2)
> > +#define BMP380_FILTER_7X               (BIT(2) | BIT(1))
> > +#define BMP380_FILTER_15X              BIT(3)
> > +#define BMP380_FILTER_31X              (BIT(3) | BIT(1))
> > +#define BMP380_FILTER_63X              (BIT(3) | BIT(2))
> > +#define BMP380_FILTER_127X             (BIT(3) | BIT(2) | BIT(1))
>
> these are values, 0,1,2,3,4,5,6,7 not a bunch of bits.
> So use with FIELD_PREP()

Should I convert the values to a enumeration or simply declare the
macros with the values? Thanks in advance

> > +
> > +#define BMP380_OSRS_TEMP_MASK          GENMASK(5, 3)
> > +#define BMP380_OSRS_TEMP_X(osrs_t)     ((osrs_t) << 3)
> > +#define BMP380_OSRS_TEMP_1X            BMP380_OSRS_TEMP_X(0)
> > +#define BMP380_OSRS_TEMP_2X            BMP380_OSRS_TEMP_X(1)
> > +#define BMP380_OSRS_TEMP_4X            BMP380_OSRS_TEMP_X(2)
> > +#define BMP380_OSRS_TEMP_8X            BMP380_OSRS_TEMP_X(3)
> > +#define BMP380_OSRS_TEMP_16X           BMP380_OSRS_TEMP_X(4)
> > +#define BMP380_OSRS_TEMP_32X           BMP380_OSRS_TEMP_X(5)
> > +
> > +#define BMP380_OSRS_PRESS_MASK         (BIT(2) | BIT(1) | BIT(0))
>
> GENMASK unless this is made up of 3 bits with separate definitions.
> If it is, give defines for them and use them to build this full
> mask.

I forgot to change this one to GENMASK. Thanks!

> > +#define BMP380_OSRS_PRESS_X(osrs_p)    ((osrs_p) << 0)
>
> FIELD_PREP()
>
> > +#define BMP380_OSRS_PRESS_1X           BMP380_OSRS_PRESS_X(0)
> > +#define BMP380_OSRS_PRESS_2X           BMP380_OSRS_PRESS_X(1)
> > +#define BMP380_OSRS_PRESS_4X           BMP380_OSRS_PRESS_X(2)
> > +#define BMP380_OSRS_PRESS_8X           BMP380_OSRS_PRESS_X(3)
> > +#define BMP380_OSRS_PRESS_16X          BMP380_OSRS_PRESS_X(4)
> > +#define BMP380_OSRS_PRESS_32X          BMP380_OSRS_PRESS_X(5)
> > +
> > +#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)
> > +#define BMP380_CTRL_SENSORS_TEMP_EN    BIT(1)
> > +#define BMP380_MODE_MASK               GENMASK(5, 4)
> > +#define BMP380_MODE_SLEEP              0
> > +#define BMP380_MODE_FORCED             BIT(4)
> > +#define BMP380_MODE_NORMAL             (BIT(5) | BIT(4))
>
> That doesn't look like either a mask or a combination of values,
> rather it looks like the number 3 in a shifted field.
>
> Use FIELD_GET/PREP along with the mask to extract a 2 bit
> vlaue which you can then compare with 0, 1, 3

Yes, it is a three in a shifted field. Thanks for insight!

>
> > +#define BMP380_STATUS_CMD_RDY_MASK     BIT(4)
> > +#define BMP380_STATUS_DRDY_PRESS_MASK  BIT(5)
> > +#define BMP380_STATUS_DRDY_TEMP_MASK   BIT(6)
> > +
> > +#define BMP380_ERR_FATAL_MASK          BIT(0)
> > +#define        BMP380_ERR_CMD_MASK             BIT(1)
>
> Stray tab?
>
> > +#define BMP380_ERR_CONF_MASK           BIT(2)
> > +
> > +#define BMP380_TEMP_SKIPPED            0x800000
> > +#define BMP380_PRESS_SKIPPED           0x800000
> > +

2022-07-16 16:14:46

by Jonathan Cameron

[permalink] [raw]

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

On Mon, 27 Jun 2022 17:42:49 +0200
Angel Iglesias <[email protected]> wrote:

> On sáb, 2022-06-25 at 16:46 +0100, Jonathan Cameron wrote:
> > On Sat, 25 Jun 2022 17:09:12 +0200
> > Angel Iglesias <[email protected]> wrote:
> >
> > > Adds compatibility with the new generation of this sensor, the
> > > BMP380
> > >
> > > Included 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]>
> >
> > Hi Angel,
> >
> > A few comments inline, but mostly looks good to me.

First a process comment. Cut out anything you agree with.
Too many emails to read so focus on the bits where there are questions
or they will get missed. Reviewers are happy to assume you agree
with them if you don't say otherwise :)

> >
> > Jonathan
...

>
> > > +
> > > +#define BMP380_FILTER_MASK             GENMASK(3, 1)
> > > +#define BMP380_FILTER_OFF              0
> > > +#define BMP380_FILTER_1X               BIT(1)
> > > +#define BMP380_FILTER_3X               BIT(2)
> > > +#define BMP380_FILTER_7X               (BIT(2) | BIT(1))
> > > +#define BMP380_FILTER_15X              BIT(3)
> > > +#define BMP380_FILTER_31X              (BIT(3) | BIT(1))
> > > +#define BMP380_FILTER_63X              (BIT(3) | BIT(2))
> > > +#define BMP380_FILTER_127X             (BIT(3) | BIT(2) | BIT(1))
> >
> > these are values, 0,1,2,3,4,5,6,7 not a bunch of bits.
> > So use with FIELD_PREP()
>
> Should I convert the values to a enumeration or simply declare the
> macros with the values? Thanks in advance

Slight preference for defines with the values.

J