[PATCH v2 0/2] iio: pressure: bmp280: add support for BMP180 and oversampling rate control

[PATCH v2 0/2] iio: pressure: bmp280: add support for BMP180 and oversampling rate control

[Akinobu Mita]

This series includes two patches: one that adds support for the BMP180
chip, and the second which adds configuration for the oversampling ratio
for both BMP180 and BMP280.

* v2
- split into two patches
- add error check for reading calibration data.
- s/be16_to_cpu(0)/cpu_to_be16(0)/
- default pressure oversampling ratio for bmp180 from x1 to x8
- add kconfig dependency to not select if bmp085-i2c is enabled
- create bmp280_chip_info includes all elements depending on the chip
- add ACPI ids for bmp180

Akinobu Mita (2):
  iio: pressure: bmp280: add support for BMP180
  iio: pressure: bmp280: add ability to control oversampling rate

 drivers/iio/pressure/Kconfig  |   5 +-
 drivers/iio/pressure/bmp280.c | 581 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 555 insertions(+), 31 deletions(-)

-- 
2.5.0

[PATCH v2 1/2] iio: pressure: bmp280: add support for BMP180

[Akinobu Mita]

This adds support for the BMP180 to the bmp280 iio driver.

The BMP180 has already been supported by misc/bmp085 driver but it
doesn't use iio framework.  This change adds the kconfig dependency
not to be selected both of them in order to avoid any issues.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Vlad Dogaru <vlad.dogaru@intel.com>
Cc: Christoph Mair <christoph.mair@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald <pmeerw@pmeerw.net>
---
 drivers/iio/pressure/Kconfig  |   5 +-
 drivers/iio/pressure/bmp280.c | 385 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 374 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index e8f60db..31e5d36 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -6,11 +6,12 @@
 menu "Pressure sensors"
 
 config BMP280
-	tristate "Bosch Sensortec BMP280 pressure sensor driver"
+	tristate "Bosch Sensortec BMP180 and BMP280 pressure sensor driver"
 	depends on I2C
+	depends on !(BMP085_I2C=y || BMP085_I2C=m)
 	select REGMAP_I2C
 	help
-	 Say yes here to build support for Bosch Sensortec BMP280
+	 Say yes here to build support for Bosch Sensortec BMP180 and BMP280
 	 pressure and temperature sensor.
 
 	 To compile this driver as a module, choose M here: the module
diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
index a2602d8..28daa74 100644
--- a/drivers/iio/pressure/bmp280.c
+++ b/drivers/iio/pressure/bmp280.c
@@ -1,12 +1,15 @@
 /*
  * Copyright (c) 2014 Intel Corporation
  *
- * Driver for Bosch Sensortec BMP280 digital pressure sensor.
+ * Driver for Bosch Sensortec BMP180 and BMP280 digital pressure sensor.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  *
+ * Datasheet:
+ * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP180-DS000-121.pdf
+ * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP280-DS001-12.pdf
  */
 
 #define pr_fmt(fmt) "bmp280: " fmt
@@ -15,9 +18,13 @@
 #include <linux/i2c.h>
 #include <linux/acpi.h>
 #include <linux/regmap.h>
+#include <linux/delay.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
+/*
+ * BMP280 specific registers
+ */
 #define BMP280_REG_TEMP_XLSB		0xFC
 #define BMP280_REG_TEMP_LSB		0xFB
 #define BMP280_REG_TEMP_MSB		0xFA
@@ -26,10 +33,7 @@
 #define BMP280_REG_PRESS_MSB		0xF7
 
 #define BMP280_REG_CONFIG		0xF5
-#define BMP280_REG_CTRL_MEAS		0xF4
 #define BMP280_REG_STATUS		0xF3
-#define BMP280_REG_RESET		0xE0
-#define BMP280_REG_ID			0xD0
 
 #define BMP280_REG_COMP_TEMP_START	0x88
 #define BMP280_COMP_TEMP_REG_COUNT	6
@@ -65,6 +69,32 @@
 #define BMP280_MODE_FORCED		BIT(0)
 #define BMP280_MODE_NORMAL		(BIT(1) | BIT(0))
 
+/*
+ * BMP180 specific registers
+ */
+#define BMP180_REG_OUT_XLSB		0xF8
+#define BMP180_REG_OUT_LSB		0xF7
+#define BMP180_REG_OUT_MSB		0xF6
+
+#define BMP180_REG_CALIB_START		0xAA
+#define BMP180_REG_CALIB_COUNT		22
+
+#define BMP180_MEAS_SCO			BIT(5)
+#define BMP180_MEAS_TEMP		(0x0E | BMP180_MEAS_SCO)
+#define BMP180_MEAS_PRESS_X(oss)	((oss) << 6 | 0x14 | BMP180_MEAS_SCO)
+#define BMP180_MEAS_PRESS_1X		BMP180_MEAS_PRESS_X(0)
+#define BMP180_MEAS_PRESS_2X		BMP180_MEAS_PRESS_X(1)
+#define BMP180_MEAS_PRESS_4X		BMP180_MEAS_PRESS_X(2)
+#define BMP180_MEAS_PRESS_8X		BMP180_MEAS_PRESS_X(3)
+
+/*
+ * BMP180 and BMP280 common registers
+ */
+#define BMP280_REG_CTRL_MEAS		0xF4
+#define BMP280_REG_RESET		0xE0
+#define BMP280_REG_ID			0xD0
+
+#define BMP180_CHIP_ID			0x55
 #define BMP280_CHIP_ID			0x58
 #define BMP280_SOFT_RESET_VAL		0xB6
 
@@ -72,6 +102,7 @@ struct bmp280_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	struct regmap *regmap;
+	const struct bmp280_chip_info *chip_info;
 
 	/*
 	 * Carryover value from temperature conversion, used in pressure
@@ -80,9 +111,17 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
+struct bmp280_chip_info {
+	const struct regmap_config *regmap_config;
+
+	int (*chip_config)(struct bmp280_data *);
+	int (*read_temp)(struct bmp280_data *, int *);
+	int (*read_press)(struct bmp280_data *, int *, int *);
+};
+
 /*
  * These enums are used for indexing into the array of compensation
- * parameters.
+ * parameters for BMP280.
  */
 enum { T1, T2, T3 };
 enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
@@ -290,10 +329,10 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_PRESSURE:
-			ret = bmp280_read_press(data, val, val2);
+			ret = data->chip_info->read_press(data, val, val2);
 			break;
 		case IIO_TEMP:
-			ret = bmp280_read_temp(data, val);
+			ret = data->chip_info->read_temp(data, val);
 			break;
 		default:
 			ret = -EINVAL;
@@ -315,7 +354,7 @@ static const struct iio_info bmp280_info = {
 	.read_raw = &bmp280_read_raw,
 };
 
-static int bmp280_chip_init(struct bmp280_data *data)
+static int bmp280_chip_config(struct bmp280_data *data)
 {
 	int ret;
 
@@ -344,6 +383,308 @@ static int bmp280_chip_init(struct bmp280_data *data)
 	return ret;
 }
 
+static const struct bmp280_chip_info bmp280_chip_info = {
+	.regmap_config = &bmp280_regmap_config,
+	.chip_config = bmp280_chip_config,
+	.read_temp = bmp280_read_temp,
+	.read_press = bmp280_read_press,
+};
+
+static bool bmp180_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMP280_REG_CTRL_MEAS:
+	case BMP280_REG_RESET:
+		return true;
+	default:
+		return false;
+	};
+}
+
+static bool bmp180_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case BMP180_REG_OUT_XLSB:
+	case BMP180_REG_OUT_LSB:
+	case BMP180_REG_OUT_MSB:
+	case BMP280_REG_CTRL_MEAS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config bmp180_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = BMP180_REG_OUT_XLSB,
+	.cache_type = REGCACHE_RBTREE,
+
+	.writeable_reg = bmp180_is_writeable_reg,
+	.volatile_reg = bmp180_is_volatile_reg,
+};
+
+static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
+{
+	int ret;
+	const int conversion_time_max[] = { 4500, 7500, 13500, 25500 };
+	unsigned int delay_us;
+	unsigned int ctrl;
+
+	ret = regmap_write(data->regmap, BMP280_REG_CTRL_MEAS, ctrl_meas);
+	if (ret)
+		return ret;
+
+	if (ctrl_meas == BMP180_MEAS_TEMP)
+		delay_us = 4500;
+	else
+		delay_us = conversion_time_max[ilog2(8)];
+
+	usleep_range(delay_us, delay_us + 1000);
+
+	ret = regmap_read(data->regmap, BMP280_REG_CTRL_MEAS, &ctrl);
+	if (ret)
+		return ret;
+
+	/*
+	 * The value of this bit reset to "0" after conversion is complete
+	 */
+	if (ctrl & BMP180_MEAS_SCO)
+		return -EIO;
+
+	return 0;
+}
+
+static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
+{
+	int ret;
+	__be16 tmp = 0;
+
+	ret = bmp180_measure(data, BMP180_MEAS_TEMP);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 2);
+	if (ret)
+		return ret;
+
+	*val = be16_to_cpu(tmp);
+
+	return 0;
+}
+
+/*
+ * These enums are used for indexing into the array of calibration
+ * coefficients for BMP180.
+ */
+enum { AC1, AC2, AC3, AC4, AC5, AC6, B1, B2, MB, MC, MD };
+
+struct bmp180_calib {
+	s32 AC1;
+	s32 AC2;
+	s32 AC3;
+	u32 AC4;
+	u32 AC5;
+	u32 AC6;
+	s32 B1;
+	s32 B2;
+	s32 MB;
+	s32 MC;
+	s32 MD;
+};
+
+static int bmp180_read_calib(struct bmp280_data *data,
+			     struct bmp180_calib *calib)
+{
+	int ret;
+	int i;
+	__be16 buf[BMP180_REG_CALIB_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,
+			       sizeof(buf));
+
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * None of the words has the value 0 or 0xFFFF
+	 */
+	for (i = 0; i < ARRAY_SIZE(buf); i++) {
+		if (buf[i] == cpu_to_be16(0) || buf[i] == cpu_to_be16(0xffff))
+			return -EIO;
+	}
+
+	/*
+	 * The double casts are necessary because be16_to_cpu returns an
+	 * unsigned 16-bit value.  Casting that value directly to a
+	 * signed 32-bit will not do proper sign extension.
+	 *
+	 * Conversely, AC4, AC5 and AC6 are unsigned values, so they can be
+	 * cast straight to the larger type.
+	 */
+	calib->AC1 = (s32)(s16)be16_to_cpu(buf[AC1]);
+	calib->AC2 = (s32)(s16)be16_to_cpu(buf[AC2]);
+	calib->AC3 = (s32)(s16)be16_to_cpu(buf[AC3]);
+	calib->AC4 = be16_to_cpu(buf[AC4]);
+	calib->AC5 = be16_to_cpu(buf[AC5]);
+	calib->AC6 = be16_to_cpu(buf[AC6]);
+	calib->B1 = (s32)(s16)be16_to_cpu(buf[B1]);
+	calib->B2 = (s32)(s16)be16_to_cpu(buf[B2]);
+	calib->MB = (s32)(s16)be16_to_cpu(buf[MB]);
+	calib->MC = (s32)(s16)be16_to_cpu(buf[MC]);
+	calib->MD = (s32)(s16)be16_to_cpu(buf[MD]);
+
+	return 0;
+}
+
+/*
+ * Returns temperature in DegC, resolution is 0.1 DegC.
+ * t_fine carries fine temperature as global value.
+ *
+ * Taken from datasheet, Section 3.5, "Calculating pressure and temperature".
+ */
+static s32 bmp180_compensate_temp(struct bmp280_data *data, s32 adc_temp)
+{
+	int ret;
+	s32 x1, x2;
+	struct bmp180_calib calib;
+
+	ret = bmp180_read_calib(data, &calib);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read calibration coefficients\n");
+		return ret;
+	}
+
+	x1 = ((adc_temp - calib.AC6) * calib.AC5) >> 15;
+	x2 = (calib.MC << 11) / (x1 + calib.MD);
+	data->t_fine = x1 + x2;
+
+	return (data->t_fine + 8) >> 4;
+}
+
+static int bmp180_read_temp(struct bmp280_data *data, int *val)
+{
+	int ret;
+	s32 adc_temp, comp_temp;
+
+	ret = bmp180_read_adc_temp(data, &adc_temp);
+	if (ret)
+		return ret;
+
+	comp_temp = bmp180_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) {
+		*val = comp_temp * 100;
+		return IIO_VAL_INT;
+	}
+
+	return 0;
+}
+
+static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
+{
+	int ret;
+	__be32 tmp = 0;
+	u8 oss = ilog2(8);
+
+	ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 3);
+	if (ret)
+		return ret;
+
+	*val = (be32_to_cpu(tmp) >> 8) >> (8 - oss);
+
+	return 0;
+}
+
+/*
+ * Returns pressure in Pa, resolution is 1 Pa.
+ *
+ * Taken from datasheet, Section 3.5, "Calculating pressure and temperature".
+ */
+static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press)
+{
+	int ret;
+	s32 x1, x2, x3, p;
+	s32 b3, b6;
+	u32 b4, b7;
+	s32 oss = ilog2(8);
+	struct bmp180_calib calib;
+
+	ret = bmp180_read_calib(data, &calib);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read calibration coefficients\n");
+		return ret;
+	}
+
+	b6 = data->t_fine - 4000;
+	x1 = (calib.B2 * (b6 * b6 >> 12)) >> 11;
+	x2 = calib.AC2 * b6 >> 11;
+	x3 = x1 + x2;
+	b3 = (((calib.AC1 * 4 + x3) << oss) + 2) / 4;
+	x1 = calib.AC3 * b6 >> 13;
+	x2 = (calib.B1 * ((b6 * b6) >> 12)) >> 16;
+	x3 = (x1 + x2 + 2) >> 2;
+	b4 = calib.AC4 * (u32)(x3 + 32768) >> 15;
+	b7 = ((u32)adc_press - b3) * (50000 >> oss);
+	if (b7 < 0x80000000)
+		p = (b7 * 2) / b4;
+	else
+		p = (b7 / b4) * 2;
+
+	x1 = (p >> 8) * (p >> 8);
+	x1 = (x1 * 3038) >> 16;
+	x2 = (-7357 * p) >> 16;
+
+	return p + ((x1 + x2 + 3791) >> 4);
+}
+
+static int bmp180_read_press(struct bmp280_data *data,
+			     int *val, int *val2)
+{
+	int ret;
+	s32 adc_press;
+	u32 comp_press;
+
+	/* Read and compensate temperature so we get a reading of t_fine. */
+	ret = bmp180_read_temp(data, NULL);
+	if (ret)
+		return ret;
+
+	ret = bmp180_read_adc_press(data, &adc_press);
+	if (ret)
+		return ret;
+
+	comp_press = bmp180_compensate_press(data, adc_press);
+
+	*val = comp_press;
+	*val2 = 1000;
+
+	return IIO_VAL_FRACTIONAL;
+}
+
+static int bmp180_chip_config(struct bmp280_data *data)
+{
+	return 0;
+}
+
+static const struct bmp280_chip_info bmp180_chip_info = {
+	.regmap_config = &bmp180_regmap_config,
+	.chip_config = bmp180_chip_config,
+	.read_temp = bmp180_read_temp,
+	.read_press = bmp180_read_press,
+};
+
 static int bmp280_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -367,7 +708,19 @@ static int bmp280_probe(struct i2c_client *client,
 	indio_dev->info = &bmp280_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	data->regmap = devm_regmap_init_i2c(client, &bmp280_regmap_config);
+	switch (id->driver_data) {
+	case BMP180_CHIP_ID:
+		data->chip_info = &bmp180_chip_info;
+		break;
+	case BMP280_CHIP_ID:
+		data->chip_info = &bmp280_chip_info;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	data->regmap = devm_regmap_init_i2c(client,
+					data->chip_info->regmap_config);
 	if (IS_ERR(data->regmap)) {
 		dev_err(&client->dev, "failed to allocate register map\n");
 		return PTR_ERR(data->regmap);
@@ -376,13 +729,13 @@ static int bmp280_probe(struct i2c_client *client,
 	ret = regmap_read(data->regmap, BMP280_REG_ID, &chip_id);
 	if (ret < 0)
 		return ret;
-	if (chip_id != BMP280_CHIP_ID) {
+	if (chip_id != id->driver_data) {
 		dev_err(&client->dev, "bad chip id.  expected %x got %x\n",
 			BMP280_CHIP_ID, chip_id);
 		return -EINVAL;
 	}
 
-	ret = bmp280_chip_init(data);
+	ret = data->chip_info->chip_config(data);
 	if (ret < 0)
 		return ret;
 
@@ -390,13 +743,17 @@ static int bmp280_probe(struct i2c_client *client,
 }
 
 static const struct acpi_device_id bmp280_acpi_match[] = {
-	{"BMP0280", 0},
+	{"BMP0280", BMP280_CHIP_ID },
+	{"BMP0180", BMP180_CHIP_ID },
+	{"BMP0085", BMP180_CHIP_ID },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, bmp280_acpi_match);
 
 static const struct i2c_device_id bmp280_id[] = {
-	{"bmp280", 0},
+	{"bmp280", BMP280_CHIP_ID },
+	{"bmp180", BMP180_CHIP_ID },
+	{"bmp085", BMP180_CHIP_ID },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, bmp280_id);
@@ -412,5 +769,5 @@ static struct i2c_driver bmp280_driver = {
 module_i2c_driver(bmp280_driver);
 
 MODULE_AUTHOR("Vlad Dogaru <vlad.dogaru@intel.com>");
-MODULE_DESCRIPTION("Driver for Bosch Sensortec BMP280 pressure and temperature sensor");
+MODULE_DESCRIPTION("Driver for Bosch Sensortec BMP180/BMP280 pressure and temperature sensor");
 MODULE_LICENSE("GPL v2");
-- 
2.5.0

Re: [PATCH v2 1/2] iio: pressure: bmp280: add support for BMP180

[Jonathan Cameron]

On 15/04/16 18:53, Akinobu Mita wrote:
> This adds support for the BMP180 to the bmp280 iio driver.
> 
> The BMP180 has already been supported by misc/bmp085 driver but it
> doesn't use iio framework.  This change adds the kconfig dependency
> not to be selected both of them in order to avoid any issues.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Vlad Dogaru <vlad.dogaru@intel.com>
> Cc: Christoph Mair <christoph.mair@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
A few comments inline - some on things that could have been done slightly
better in the original driver but which we are now stuck with for ABI
compatibility reasons.. Ah well.

Jonathan
> ---
>  drivers/iio/pressure/Kconfig  |   5 +-
>  drivers/iio/pressure/bmp280.c | 385 ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 374 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index e8f60db..31e5d36 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -6,11 +6,12 @@
>  menu "Pressure sensors"
>  
>  config BMP280
> -	tristate "Bosch Sensortec BMP280 pressure sensor driver"
> +	tristate "Bosch Sensortec BMP180 and BMP280 pressure sensor driver"
>  	depends on I2C
> +	depends on !(BMP085_I2C=y || BMP085_I2C=m)
>  	select REGMAP_I2C
>  	help
> -	 Say yes here to build support for Bosch Sensortec BMP280
> +	 Say yes here to build support for Bosch Sensortec BMP180 and BMP280
>  	 pressure and temperature sensor.
>  
>  	 To compile this driver as a module, choose M here: the module
> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
> index a2602d8..28daa74 100644
> --- a/drivers/iio/pressure/bmp280.c
> +++ b/drivers/iio/pressure/bmp280.c
> @@ -1,12 +1,15 @@
>  /*
>   * Copyright (c) 2014 Intel Corporation
>   *
> - * Driver for Bosch Sensortec BMP280 digital pressure sensor.
> + * Driver for Bosch Sensortec BMP180 and BMP280 digital pressure sensor.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   *
> + * Datasheet:
> + * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP180-DS000-121.pdf
> + * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP280-DS001-12.pdf
>   */
>  
>  #define pr_fmt(fmt) "bmp280: " fmt
> @@ -15,9 +18,13 @@
>  #include <linux/i2c.h>
>  #include <linux/acpi.h>
>  #include <linux/regmap.h>
> +#include <linux/delay.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> +/*
> + * BMP280 specific registers
> + */
single line comment syntax preferred.
>  #define BMP280_REG_TEMP_XLSB		0xFC
>  #define BMP280_REG_TEMP_LSB		0xFB
>  #define BMP280_REG_TEMP_MSB		0xFA
> @@ -26,10 +33,7 @@
>  #define BMP280_REG_PRESS_MSB		0xF7
>  
>  #define BMP280_REG_CONFIG		0xF5
> -#define BMP280_REG_CTRL_MEAS		0xF4
>  #define BMP280_REG_STATUS		0xF3
> -#define BMP280_REG_RESET		0xE0
> -#define BMP280_REG_ID			0xD0
>  
>  #define BMP280_REG_COMP_TEMP_START	0x88
>  #define BMP280_COMP_TEMP_REG_COUNT	6
> @@ -65,6 +69,32 @@
>  #define BMP280_MODE_FORCED		BIT(0)
>  #define BMP280_MODE_NORMAL		(BIT(1) | BIT(0))
>  
> +/*
> + * BMP180 specific registers
> + */
> +#define BMP180_REG_OUT_XLSB		0xF8
> +#define BMP180_REG_OUT_LSB		0xF7
> +#define BMP180_REG_OUT_MSB		0xF6
> +
> +#define BMP180_REG_CALIB_START		0xAA
> +#define BMP180_REG_CALIB_COUNT		22
> +
> +#define BMP180_MEAS_SCO			BIT(5)
> +#define BMP180_MEAS_TEMP		(0x0E | BMP180_MEAS_SCO)
> +#define BMP180_MEAS_PRESS_X(oss)	((oss) << 6 | 0x14 | BMP180_MEAS_SCO)
> +#define BMP180_MEAS_PRESS_1X		BMP180_MEAS_PRESS_X(0)
> +#define BMP180_MEAS_PRESS_2X		BMP180_MEAS_PRESS_X(1)
> +#define BMP180_MEAS_PRESS_4X		BMP180_MEAS_PRESS_X(2)
> +#define BMP180_MEAS_PRESS_8X		BMP180_MEAS_PRESS_X(3)
> +
> +/*
> + * BMP180 and BMP280 common registers
> + */
> +#define BMP280_REG_CTRL_MEAS		0xF4
> +#define BMP280_REG_RESET		0xE0
> +#define BMP280_REG_ID			0xD0
> +
> +#define BMP180_CHIP_ID			0x55
>  #define BMP280_CHIP_ID			0x58
>  #define BMP280_SOFT_RESET_VAL		0xB6
>  
> @@ -72,6 +102,7 @@ struct bmp280_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
>  	struct regmap *regmap;
> +	const struct bmp280_chip_info *chip_info;
>  
>  	/*
>  	 * Carryover value from temperature conversion, used in pressure
> @@ -80,9 +111,17 @@ struct bmp280_data {
>  	s32 t_fine;
>  };
>  
> +struct bmp280_chip_info {
> +	const struct regmap_config *regmap_config;
> +
> +	int (*chip_config)(struct bmp280_data *);
> +	int (*read_temp)(struct bmp280_data *, int *);
> +	int (*read_press)(struct bmp280_data *, int *, int *);
> +};
> +
>  /*
>   * These enums are used for indexing into the array of compensation
> - * parameters.
> + * parameters for BMP280.
>   */
>  enum { T1, T2, T3 };
>  enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
> @@ -290,10 +329,10 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_PROCESSED:
>  		switch (chan->type) {
>  		case IIO_PRESSURE:
> -			ret = bmp280_read_press(data, val, val2);
> +			ret = data->chip_info->read_press(data, val, val2);
>  			break;
>  		case IIO_TEMP:
> -			ret = bmp280_read_temp(data, val);
> +			ret = data->chip_info->read_temp(data, val);
>  			break;
>  		default:
>  			ret = -EINVAL;
> @@ -315,7 +354,7 @@ static const struct iio_info bmp280_info = {
>  	.read_raw = &bmp280_read_raw,
>  };
>  
> -static int bmp280_chip_init(struct bmp280_data *data)
> +static int bmp280_chip_config(struct bmp280_data *data)
>  {
>  	int ret;
>  
> @@ -344,6 +383,308 @@ static int bmp280_chip_init(struct bmp280_data *data)
>  	return ret;
>  }
>  
> +static const struct bmp280_chip_info bmp280_chip_info = {
> +	.regmap_config = &bmp280_regmap_config,
> +	.chip_config = bmp280_chip_config,
> +	.read_temp = bmp280_read_temp,
> +	.read_press = bmp280_read_press,
> +};
> +
> +static bool bmp180_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case BMP280_REG_CTRL_MEAS:
> +	case BMP280_REG_RESET:
> +		return true;
> +	default:
> +		return false;
> +	};
> +}
> +
> +static bool bmp180_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case BMP180_REG_OUT_XLSB:
> +	case BMP180_REG_OUT_LSB:
> +	case BMP180_REG_OUT_MSB:
> +	case BMP280_REG_CTRL_MEAS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config bmp180_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = BMP180_REG_OUT_XLSB,
> +	.cache_type = REGCACHE_RBTREE,
> +
> +	.writeable_reg = bmp180_is_writeable_reg,
> +	.volatile_reg = bmp180_is_volatile_reg,
> +};
> +
> +static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
> +{
> +	int ret;
> +	const int conversion_time_max[] = { 4500, 7500, 13500, 25500 };
> +	unsigned int delay_us;
> +	unsigned int ctrl;
> +
> +	ret = regmap_write(data->regmap, BMP280_REG_CTRL_MEAS, ctrl_meas);
> +	if (ret)
> +		return ret;
> +
> +	if (ctrl_meas == BMP180_MEAS_TEMP)
> +		delay_us = 4500;
> +	else
> +		delay_us = conversion_time_max[ilog2(8)];
> +
> +	usleep_range(delay_us, delay_us + 1000);
> +
> +	ret = regmap_read(data->regmap, BMP280_REG_CTRL_MEAS, &ctrl);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The value of this bit reset to "0" after conversion is complete
> +	 */
Single line comment syntax...
> +	if (ctrl & BMP180_MEAS_SCO)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int bmp180_read_adc_temp(struct bmp280_data *data, int *val)
> +{
> +	int ret;
> +	__be16 tmp = 0;
> +
> +	ret = bmp180_measure(data, BMP180_MEAS_TEMP);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 2);
> +	if (ret)
> +		return ret;
> +
> +	*val = be16_to_cpu(tmp);
> +
> +	return 0;
> +}
> +
> +/*
> + * These enums are used for indexing into the array of calibration
> + * coefficients for BMP180.
> + */
> +enum { AC1, AC2, AC3, AC4, AC5, AC6, B1, B2, MB, MC, MD };
> +
> +struct bmp180_calib {
> +	s32 AC1;
> +	s32 AC2;
> +	s32 AC3;
> +	u32 AC4;
> +	u32 AC5;
> +	u32 AC6;
> +	s32 B1;
> +	s32 B2;
> +	s32 MB;
> +	s32 MC;
> +	s32 MD;
> +};
> +
> +static int bmp180_read_calib(struct bmp280_data *data,
> +			     struct bmp180_calib *calib)
> +{
> +	int ret;
> +	int i;
> +	__be16 buf[BMP180_REG_CALIB_COUNT / 2];
> +
> +	ret = regmap_bulk_read(data->regmap, BMP180_REG_CALIB_START, buf,
> +			       sizeof(buf));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * None of the words has the value 0 or 0xFFFF
> +	 */
Single line comment, so single line comment syntax please (nitpick!)
> +	for (i = 0; i < ARRAY_SIZE(buf); i++) {
> +		if (buf[i] == cpu_to_be16(0) || buf[i] == cpu_to_be16(0xffff))
> +			return -EIO;
> +	}
> +
> +	/*
> +	 * The double casts are necessary because be16_to_cpu returns an
> +	 * unsigned 16-bit value.  Casting that value directly to a
> +	 * signed 32-bit will not do proper sign extension.
> +	 *
Would using sign_extend32 perhaps make this clear without the need for the
comment?

I think you'd need
calib->AC1 = sign_extend32(be16_to_cpu(buf[AC1]), 16);
etc.  Compiler should optimize that to end up with similar assembler..

Or alternatively just define them as s16 if that is what they are...
> +	 * Conversely, AC4, AC5 and AC6 are unsigned values, so they can be
> +	 * cast straight to the larger type.
> +	 */
> +	calib->AC1 = (s32)(s16)be16_to_cpu(buf[AC1]);
> +	calib->AC2 = (s32)(s16)be16_to_cpu(buf[AC2]);
> +	calib->AC3 = (s32)(s16)be16_to_cpu(buf[AC3]);
> +	calib->AC4 = be16_to_cpu(buf[AC4]);
> +	calib->AC5 = be16_to_cpu(buf[AC5]);
> +	calib->AC6 = be16_to_cpu(buf[AC6]);
> +	calib->B1 = (s32)(s16)be16_to_cpu(buf[B1]);
> +	calib->B2 = (s32)(s16)be16_to_cpu(buf[B2]);
> +	calib->MB = (s32)(s16)be16_to_cpu(buf[MB]);
> +	calib->MC = (s32)(s16)be16_to_cpu(buf[MC]);
> +	calib->MD = (s32)(s16)be16_to_cpu(buf[MD]);
> +
> +	return 0;
> +}
> +
> +/*
> + * Returns temperature in DegC, resolution is 0.1 DegC.
> + * t_fine carries fine temperature as global value.
> + *
> + * Taken from datasheet, Section 3.5, "Calculating pressure and temperature".
> + */
> +static s32 bmp180_compensate_temp(struct bmp280_data *data, s32 adc_temp)
> +{
> +	int ret;
> +	s32 x1, x2;
> +	struct bmp180_calib calib;
> +
> +	ret = bmp180_read_calib(data, &calib);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev,
> +			"failed to read calibration coefficients\n");
> +		return ret;
> +	}
> +
> +	x1 = ((adc_temp - calib.AC6) * calib.AC5) >> 15;
> +	x2 = (calib.MC << 11) / (x1 + calib.MD);
> +	data->t_fine = x1 + x2;
> +
> +	return (data->t_fine + 8) >> 4;
> +}
> +
> +static int bmp180_read_temp(struct bmp280_data *data, int *val)
> +{
> +	int ret;
> +	s32 adc_temp, comp_temp;
> +
> +	ret = bmp180_read_adc_temp(data, &adc_temp);
> +	if (ret)
> +		return ret;
> +
> +	comp_temp = bmp180_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) {
> +		*val = comp_temp * 100;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
> +{
> +	int ret;
> +	__be32 tmp = 0;
> +	u8 oss = ilog2(8);
> +
> +	ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB, (u8 *)&tmp, 3);
> +	if (ret)
> +		return ret;
> +
> +	*val = (be32_to_cpu(tmp) >> 8) >> (8 - oss);
> +
> +	return 0;
> +}
> +
> +/*
> + * Returns pressure in Pa, resolution is 1 Pa.
> + *
> + * Taken from datasheet, Section 3.5, "Calculating pressure and temperature".
> + */
> +static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press)
> +{
> +	int ret;
> +	s32 x1, x2, x3, p;
> +	s32 b3, b6;
> +	u32 b4, b7;
> +	s32 oss = ilog2(8);
> +	struct bmp180_calib calib;
> +
> +	ret = bmp180_read_calib(data, &calib);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev,
> +			"failed to read calibration coefficients\n");
> +		return ret;
> +	}
> +
> +	b6 = data->t_fine - 4000;
> +	x1 = (calib.B2 * (b6 * b6 >> 12)) >> 11;
> +	x2 = calib.AC2 * b6 >> 11;
> +	x3 = x1 + x2;
> +	b3 = (((calib.AC1 * 4 + x3) << oss) + 2) / 4;
> +	x1 = calib.AC3 * b6 >> 13;
> +	x2 = (calib.B1 * ((b6 * b6) >> 12)) >> 16;
> +	x3 = (x1 + x2 + 2) >> 2;
> +	b4 = calib.AC4 * (u32)(x3 + 32768) >> 15;
> +	b7 = ((u32)adc_press - b3) * (50000 >> oss);
> +	if (b7 < 0x80000000)
> +		p = (b7 * 2) / b4;
> +	else
> +		p = (b7 / b4) * 2;
> +
> +	x1 = (p >> 8) * (p >> 8);
> +	x1 = (x1 * 3038) >> 16;
> +	x2 = (-7357 * p) >> 16;
> +
> +	return p + ((x1 + x2 + 3791) >> 4);
> +}
> +
> +static int bmp180_read_press(struct bmp280_data *data,
> +			     int *val, int *val2)
> +{
> +	int ret;
> +	s32 adc_press;
> +	u32 comp_press;
> +
> +	/* Read and compensate temperature so we get a reading of t_fine. */
> +	ret = bmp180_read_temp(data, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = bmp180_read_adc_press(data, &adc_press);
> +	if (ret)
> +		return ret;
> +
> +	comp_press = bmp180_compensate_press(data, adc_press);
> +
> +	*val = comp_press;
> +	*val2 = 1000;
This would be better done using a _scale info_mask element to handle the divide
by 1000.  That lets the decision on whether the conversion is even needed be
moved to userspace.

Unfortunately that's also true for the bmp280 as it stands which is annoying
as that makes it ABI which we shouldn't be changing.  Ah well, the device
is slow anyway so not terrible to leave this as it is.  I should have picked
that up when reviewing that driver originally - sometimes we have live with
our mistakes!
> +
> +	return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int bmp180_chip_config(struct bmp280_data *data)
> +{
> +	return 0;
> +}
> +
> +static const struct bmp280_chip_info bmp180_chip_info = {
> +	.regmap_config = &bmp180_regmap_config,
> +	.chip_config = bmp180_chip_config,
> +	.read_temp = bmp180_read_temp,
> +	.read_press = bmp180_read_press,
> +};
> +
>  static int bmp280_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -367,7 +708,19 @@ static int bmp280_probe(struct i2c_client *client,
>  	indio_dev->info = &bmp280_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	data->regmap = devm_regmap_init_i2c(client, &bmp280_regmap_config);
> +	switch (id->driver_data) {
> +	case BMP180_CHIP_ID:
> +		data->chip_info = &bmp180_chip_info;
> +		break;
> +	case BMP280_CHIP_ID:
> +		data->chip_info = &bmp280_chip_info;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	data->regmap = devm_regmap_init_i2c(client,
> +					data->chip_info->regmap_config);
>  	if (IS_ERR(data->regmap)) {
>  		dev_err(&client->dev, "failed to allocate register map\n");
>  		return PTR_ERR(data->regmap);
> @@ -376,13 +729,13 @@ static int bmp280_probe(struct i2c_client *client,
>  	ret = regmap_read(data->regmap, BMP280_REG_ID, &chip_id);
>  	if (ret < 0)
>  		return ret;
> -	if (chip_id != BMP280_CHIP_ID) {
> +	if (chip_id != id->driver_data) {
>  		dev_err(&client->dev, "bad chip id.  expected %x got %x\n",
>  			BMP280_CHIP_ID, chip_id);
>  		return -EINVAL;
>  	}
>  
> -	ret = bmp280_chip_init(data);
> +	ret = data->chip_info->chip_config(data);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -390,13 +743,17 @@ static int bmp280_probe(struct i2c_client *client,
>  }
>  
>  static const struct acpi_device_id bmp280_acpi_match[] = {
> -	{"BMP0280", 0},
> +	{"BMP0280", BMP280_CHIP_ID },
> +	{"BMP0180", BMP180_CHIP_ID },
> +	{"BMP0085", BMP180_CHIP_ID },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, bmp280_acpi_match);
>  
>  static const struct i2c_device_id bmp280_id[] = {
> -	{"bmp280", 0},
> +	{"bmp280", BMP280_CHIP_ID },
> +	{"bmp180", BMP180_CHIP_ID },
> +	{"bmp085", BMP180_CHIP_ID },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, bmp280_id);
> @@ -412,5 +769,5 @@ static struct i2c_driver bmp280_driver = {
>  module_i2c_driver(bmp280_driver);
>  
>  MODULE_AUTHOR("Vlad Dogaru <vlad.dogaru@intel.com>");
> -MODULE_DESCRIPTION("Driver for Bosch Sensortec BMP280 pressure and temperature sensor");
> +MODULE_DESCRIPTION("Driver for Bosch Sensortec BMP180/BMP280 pressure and temperature sensor");
>  MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v2 1/2] iio: pressure: bmp280: add support for BMP180

[Vlad Dogaru]

Hi Jonathan,

On Sat, Apr 16, 2016 at 11:04:05AM +0100, Jonathan Cameron wrote:
> On 15/04/16 18:53, Akinobu Mita wrote:
[...]
> > +static int bmp180_read_press(struct bmp280_data *data,
> > +			     int *val, int *val2)
> > +{
> > +	int ret;
> > +	s32 adc_press;
> > +	u32 comp_press;
> > +
> > +	/* Read and compensate temperature so we get a reading of t_fine. */
> > +	ret = bmp180_read_temp(data, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = bmp180_read_adc_press(data, &adc_press);
> > +	if (ret)
> > +		return ret;
> > +
> > +	comp_press = bmp180_compensate_press(data, adc_press);
> > +
> > +	*val = comp_press;
> > +	*val2 = 1000;
> This would be better done using a _scale info_mask element to handle the divide
> by 1000.  That lets the decision on whether the conversion is even needed be
> moved to userspace.
> 
> Unfortunately that's also true for the bmp280 as it stands which is annoying
> as that makes it ABI which we shouldn't be changing.  Ah well, the device
> is slow anyway so not terrible to leave this as it is.  I should have picked
> that up when reviewing that driver originally - sometimes we have live with
> our mistakes!

I don't think that qualifies as breaking ABI.  The ABI specifies which
files *may* exist and how they should be interpreted, if they exist.
It does not guarantee that a specific sensor such as BMP280 exposes its
measurement as in_temp_input, rather than in_temp_raw and in_temp_scale.

That said, I don't feel strongly about the changes either way.  When I
initially wrote the driver I thought the operations were harmless enough
to do in the kernel.

Thanks,
Vlad

Re: [PATCH v2 1/2] iio: pressure: bmp280: add support for BMP180

[jic23]

On 18.04.2016 13:01, Vlad Dogaru wrote:
> Hi Jonathan,
> 
> On Sat, Apr 16, 2016 at 11:04:05AM +0100, Jonathan Cameron wrote:
>> On 15/04/16 18:53, Akinobu Mita wrote:
> [...]
>> > +static int bmp180_read_press(struct bmp280_data *data,
>> > +			     int *val, int *val2)
>> > +{
>> > +	int ret;
>> > +	s32 adc_press;
>> > +	u32 comp_press;
>> > +
>> > +	/* Read and compensate temperature so we get a reading of t_fine. */
>> > +	ret = bmp180_read_temp(data, NULL);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	ret = bmp180_read_adc_press(data, &adc_press);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	comp_press = bmp180_compensate_press(data, adc_press);
>> > +
>> > +	*val = comp_press;
>> > +	*val2 = 1000;
>> This would be better done using a _scale info_mask element to handle 
>> the divide
>> by 1000.  That lets the decision on whether the conversion is even 
>> needed be
>> moved to userspace.
>> 
>> Unfortunately that's also true for the bmp280 as it stands which is 
>> annoying
>> as that makes it ABI which we shouldn't be changing.  Ah well, the 
>> device
>> is slow anyway so not terrible to leave this as it is.  I should have 
>> picked
>> that up when reviewing that driver originally - sometimes we have live 
>> with
>> our mistakes!
> 
> I don't think that qualifies as breaking ABI.  The ABI specifies which
> files *may* exist and how they should be interpreted, if they exist.
> It does not guarantee that a specific sensor such as BMP280 exposes its
> measurement as in_temp_input, rather than in_temp_raw and 
> in_temp_scale.
It's kind of an open question when this happens.  Sadly it counts as 
breaking the
ABI even if the program that was using it was using it wrongly.  
However, rule
1 of ABI breakage is 'If no one notices then it isn't broken ;)'.  So if
people are all using the ABI correctly then we are fine...  The question 
is
'Do we think they are?' :)
> 
> That said, I don't feel strongly about the changes either way.  When I
> initially wrote the driver I thought the operations were harmless 
> enough
> to do in the kernel.
Leave it as is, doesn't really matter either way.

Jonathan
> 
> Thanks,
> Vlad

[PATCH v2 2/2] iio: pressure: bmp280: add ability to control oversampling rate

[Akinobu Mita]

This adds ability to control the oversampling ratio of the temperature
and pressure measurement for both bmp180 and bmp280.
(bmp280 is untested for now)

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Vlad Dogaru <vlad.dogaru@intel.com>
Cc: Christoph Mair <christoph.mair@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald <pmeerw@pmeerw.net>
---
 drivers/iio/pressure/bmp280.c | 202 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 184 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
index 28daa74..fd931c7 100644
--- a/drivers/iio/pressure/bmp280.c
+++ b/drivers/iio/pressure/bmp280.c
@@ -50,19 +50,21 @@
 
 #define BMP280_OSRS_TEMP_MASK		(BIT(7) | BIT(6) | BIT(5))
 #define BMP280_OSRS_TEMP_SKIP		0
-#define BMP280_OSRS_TEMP_1X		BIT(5)
-#define BMP280_OSRS_TEMP_2X		BIT(6)
-#define BMP280_OSRS_TEMP_4X		(BIT(6) | BIT(5))
-#define BMP280_OSRS_TEMP_8X		BIT(7)
-#define BMP280_OSRS_TEMP_16X		(BIT(7) | BIT(5))
+#define BMP280_OSRS_TEMP_X(osrs_t)	((osrs_t) << 5)
+#define BMP280_OSRS_TEMP_1X		BMP280_OSRS_TEMP_X(1)
+#define BMP280_OSRS_TEMP_2X		BMP280_OSRS_TEMP_X(2)
+#define BMP280_OSRS_TEMP_4X		BMP280_OSRS_TEMP_X(3)
+#define BMP280_OSRS_TEMP_8X		BMP280_OSRS_TEMP_X(4)
+#define BMP280_OSRS_TEMP_16X		BMP280_OSRS_TEMP_X(5)
 
 #define BMP280_OSRS_PRESS_MASK		(BIT(4) | BIT(3) | BIT(2))
 #define BMP280_OSRS_PRESS_SKIP		0
-#define BMP280_OSRS_PRESS_1X		BIT(2)
-#define BMP280_OSRS_PRESS_2X		BIT(3)
-#define BMP280_OSRS_PRESS_4X		(BIT(3) | BIT(2))
-#define BMP280_OSRS_PRESS_8X		BIT(4)
-#define BMP280_OSRS_PRESS_16X		(BIT(4) | BIT(2))
+#define BMP280_OSRS_PRESS_X(osrs_p)	((osrs_p) << 2)
+#define BMP280_OSRS_PRESS_1X		BMP280_OSRS_PRESS_X(1)
+#define BMP280_OSRS_PRESS_2X		BMP280_OSRS_PRESS_X(2)
+#define BMP280_OSRS_PRESS_4X		BMP280_OSRS_PRESS_X(3)
+#define BMP280_OSRS_PRESS_8X		BMP280_OSRS_PRESS_X(4)
+#define BMP280_OSRS_PRESS_16X		BMP280_OSRS_PRESS_X(5)
 
 #define BMP280_MODE_MASK		(BIT(1) | BIT(0))
 #define BMP280_MODE_SLEEP		0
@@ -104,6 +106,9 @@ struct bmp280_data {
 	struct regmap *regmap;
 	const struct bmp280_chip_info *chip_info;
 
+	u8 oversampling_press;
+	u8 oversampling_temp;
+
 	/*
 	 * Carryover value from temperature conversion, used in pressure
 	 * calculation.
@@ -114,6 +119,12 @@ struct bmp280_data {
 struct bmp280_chip_info {
 	const struct regmap_config *regmap_config;
 
+	const int *oversampling_temp_avail;
+	int num_oversampling_temp_avail;
+
+	const int *oversampling_press_avail;
+	int num_oversampling_press_avail;
+
 	int (*chip_config)(struct bmp280_data *);
 	int (*read_temp)(struct bmp280_data *, int *);
 	int (*read_press)(struct bmp280_data *, int *, int *);
@@ -129,11 +140,13 @@ enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 	{
 		.type = IIO_TEMP,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 	},
 };
 
@@ -339,6 +352,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 			break;
 		}
 		break;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			*val = 1 << data->oversampling_press;
+			ret = IIO_VAL_INT;
+			break;
+		case IIO_TEMP:
+			*val = 1 << data->oversampling_temp;
+			ret = IIO_VAL_INT;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -349,22 +377,135 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int bmp280_write_oversampling_ratio_temp(struct bmp280_data *data,
+					       int val)
+{
+	int i;
+	const int *avail = data->chip_info->oversampling_temp_avail;
+	const int n = data->chip_info->num_oversampling_temp_avail;
+
+	for (i = 0; i < n; i++) {
+		if (avail[i] == val) {
+			data->oversampling_temp = ilog2(val);
+
+			return data->chip_info->chip_config(data);
+		}
+	}
+	return -EINVAL;
+}
+
+static int bmp280_write_oversampling_ratio_press(struct bmp280_data *data,
+					       int val)
+{
+	int i;
+	const int *avail = data->chip_info->oversampling_press_avail;
+	const int n = data->chip_info->num_oversampling_press_avail;
+
+	for (i = 0; i < n; i++) {
+		if (avail[i] == val) {
+			data->oversampling_press = ilog2(val);
+
+			return data->chip_info->chip_config(data);
+		}
+	}
+	return -EINVAL;
+}
+
+static int bmp280_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	int ret = 0;
+	struct bmp280_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		mutex_lock(&data->lock);
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			ret = bmp280_write_oversampling_ratio_press(data, val);
+			break;
+		case IIO_TEMP:
+			ret = bmp280_write_oversampling_ratio_temp(data, val);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		mutex_unlock(&data->lock);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static ssize_t bmp280_show_avail(char *buf, const int *vals, const int n)
+{
+	size_t len = 0;
+	int i;
+
+	for (i = 0; i < n; i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ", vals[i]);
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t bmp280_show_temp_oversampling_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	return bmp280_show_avail(buf, data->chip_info->oversampling_temp_avail,
+				 data->chip_info->num_oversampling_temp_avail);
+}
+
+static ssize_t bmp280_show_press_oversampling_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
+
+	return bmp280_show_avail(buf, data->chip_info->oversampling_press_avail,
+				 data->chip_info->num_oversampling_press_avail);
+}
+
+static IIO_DEVICE_ATTR(in_temp_oversampling_ratio_available,
+	S_IRUGO, bmp280_show_temp_oversampling_avail, NULL, 0);
+
+static IIO_DEVICE_ATTR(in_pressure_oversampling_ratio_available,
+	S_IRUGO, bmp280_show_press_oversampling_avail, NULL, 0);
+
+static struct attribute *bmp280_attributes[] = {
+	&iio_dev_attr_in_temp_oversampling_ratio_available.dev_attr.attr,
+	&iio_dev_attr_in_pressure_oversampling_ratio_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group bmp280_attrs_group = {
+	.attrs = bmp280_attributes,
+};
+
 static const struct iio_info bmp280_info = {
 	.driver_module = THIS_MODULE,
 	.read_raw = &bmp280_read_raw,
+	.write_raw = &bmp280_write_raw,
+	.attrs = &bmp280_attrs_group,
 };
 
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	int ret;
+	u8 osrs = BMP280_OSRS_TEMP_X(data->oversampling_temp) |
+		  BMP280_OSRS_PRESS_X(data->oversampling_press);
 
 	ret = regmap_update_bits(data->regmap, BMP280_REG_CTRL_MEAS,
 				 BMP280_OSRS_TEMP_MASK |
 				 BMP280_OSRS_PRESS_MASK |
 				 BMP280_MODE_MASK,
-				 BMP280_OSRS_TEMP_2X |
-				 BMP280_OSRS_PRESS_16X |
-				 BMP280_MODE_NORMAL);
+				 osrs | BMP280_MODE_NORMAL);
 	if (ret < 0) {
 		dev_err(&data->client->dev,
 			"failed to write ctrl_meas register\n");
@@ -383,8 +524,17 @@ static int bmp280_chip_config(struct bmp280_data *data)
 	return ret;
 }
 
+static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
+
 static const struct bmp280_chip_info bmp280_chip_info = {
 	.regmap_config = &bmp280_regmap_config,
+
+	.oversampling_temp_avail = bmp280_oversampling_avail,
+	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+
+	.oversampling_press_avail = bmp280_oversampling_avail,
+	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
+
 	.chip_config = bmp280_chip_config,
 	.read_temp = bmp280_read_temp,
 	.read_press = bmp280_read_press,
@@ -439,7 +589,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
 	if (ctrl_meas == BMP180_MEAS_TEMP)
 		delay_us = 4500;
 	else
-		delay_us = conversion_time_max[ilog2(8)];
+		delay_us = conversion_time_max[data->oversampling_press];
 
 	usleep_range(delay_us, delay_us + 1000);
 
@@ -591,7 +741,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
 {
 	int ret;
 	__be32 tmp = 0;
-	u8 oss = ilog2(8);
+	u8 oss = data->oversampling_press;
 
 	ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
 	if (ret)
@@ -617,7 +767,7 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press)
 	s32 x1, x2, x3, p;
 	s32 b3, b6;
 	u32 b4, b7;
-	s32 oss = ilog2(8);
+	s32 oss = data->oversampling_press;
 	struct bmp180_calib calib;
 
 	ret = bmp180_read_calib(data, &calib);
@@ -678,8 +828,20 @@ static int bmp180_chip_config(struct bmp280_data *data)
 	return 0;
 }
 
+static const int bmp180_oversampling_temp_avail[] = { 1 };
+static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
+
 static const struct bmp280_chip_info bmp180_chip_info = {
 	.regmap_config = &bmp180_regmap_config,
+
+	.oversampling_temp_avail = bmp180_oversampling_temp_avail,
+	.num_oversampling_temp_avail =
+		ARRAY_SIZE(bmp180_oversampling_temp_avail),
+
+	.oversampling_press_avail = bmp180_oversampling_press_avail,
+	.num_oversampling_press_avail =
+		ARRAY_SIZE(bmp180_oversampling_press_avail),
+
 	.chip_config = bmp180_chip_config,
 	.read_temp = bmp180_read_temp,
 	.read_press = bmp180_read_press,
@@ -711,9 +873,13 @@ static int bmp280_probe(struct i2c_client *client,
 	switch (id->driver_data) {
 	case BMP180_CHIP_ID:
 		data->chip_info = &bmp180_chip_info;
+		data->oversampling_press = ilog2(8);
+		data->oversampling_temp = ilog2(1);
 		break;
 	case BMP280_CHIP_ID:
 		data->chip_info = &bmp280_chip_info;
+		data->oversampling_press = ilog2(16);
+		data->oversampling_temp = ilog2(2);
 		break;
 	default:
 		return -EINVAL;
-- 
2.5.0

Re: [PATCH v2 2/2] iio: pressure: bmp280: add ability to control oversampling rate

[Jonathan Cameron]

On 15/04/16 18:53, Akinobu Mita wrote:
> This adds ability to control the oversampling ratio of the temperature
> and pressure measurement for both bmp180 and bmp280.
> (bmp280 is untested for now)
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Vlad Dogaru <vlad.dogaru@intel.com>
> Cc: Christoph Mair <christoph.mair@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
This looks good to me...

Jonathan
> ---
>  drivers/iio/pressure/bmp280.c | 202 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 184 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
> index 28daa74..fd931c7 100644
> --- a/drivers/iio/pressure/bmp280.c
> +++ b/drivers/iio/pressure/bmp280.c
> @@ -50,19 +50,21 @@
>  
>  #define BMP280_OSRS_TEMP_MASK		(BIT(7) | BIT(6) | BIT(5))
>  #define BMP280_OSRS_TEMP_SKIP		0
> -#define BMP280_OSRS_TEMP_1X		BIT(5)
> -#define BMP280_OSRS_TEMP_2X		BIT(6)
> -#define BMP280_OSRS_TEMP_4X		(BIT(6) | BIT(5))
> -#define BMP280_OSRS_TEMP_8X		BIT(7)
> -#define BMP280_OSRS_TEMP_16X		(BIT(7) | BIT(5))
> +#define BMP280_OSRS_TEMP_X(osrs_t)	((osrs_t) << 5)
> +#define BMP280_OSRS_TEMP_1X		BMP280_OSRS_TEMP_X(1)
> +#define BMP280_OSRS_TEMP_2X		BMP280_OSRS_TEMP_X(2)
> +#define BMP280_OSRS_TEMP_4X		BMP280_OSRS_TEMP_X(3)
> +#define BMP280_OSRS_TEMP_8X		BMP280_OSRS_TEMP_X(4)
> +#define BMP280_OSRS_TEMP_16X		BMP280_OSRS_TEMP_X(5)
>  
>  #define BMP280_OSRS_PRESS_MASK		(BIT(4) | BIT(3) | BIT(2))
>  #define BMP280_OSRS_PRESS_SKIP		0
> -#define BMP280_OSRS_PRESS_1X		BIT(2)
> -#define BMP280_OSRS_PRESS_2X		BIT(3)
> -#define BMP280_OSRS_PRESS_4X		(BIT(3) | BIT(2))
> -#define BMP280_OSRS_PRESS_8X		BIT(4)
> -#define BMP280_OSRS_PRESS_16X		(BIT(4) | BIT(2))
> +#define BMP280_OSRS_PRESS_X(osrs_p)	((osrs_p) << 2)
> +#define BMP280_OSRS_PRESS_1X		BMP280_OSRS_PRESS_X(1)
> +#define BMP280_OSRS_PRESS_2X		BMP280_OSRS_PRESS_X(2)
> +#define BMP280_OSRS_PRESS_4X		BMP280_OSRS_PRESS_X(3)
> +#define BMP280_OSRS_PRESS_8X		BMP280_OSRS_PRESS_X(4)
> +#define BMP280_OSRS_PRESS_16X		BMP280_OSRS_PRESS_X(5)
>  
>  #define BMP280_MODE_MASK		(BIT(1) | BIT(0))
>  #define BMP280_MODE_SLEEP		0
> @@ -104,6 +106,9 @@ struct bmp280_data {
>  	struct regmap *regmap;
>  	const struct bmp280_chip_info *chip_info;
>  
> +	u8 oversampling_press;
> +	u8 oversampling_temp;
> +
>  	/*
>  	 * Carryover value from temperature conversion, used in pressure
>  	 * calculation.
> @@ -114,6 +119,12 @@ struct bmp280_data {
>  struct bmp280_chip_info {
>  	const struct regmap_config *regmap_config;
>  
> +	const int *oversampling_temp_avail;
> +	int num_oversampling_temp_avail;
> +
> +	const int *oversampling_press_avail;
> +	int num_oversampling_press_avail;
> +
>  	int (*chip_config)(struct bmp280_data *);
>  	int (*read_temp)(struct bmp280_data *, int *);
>  	int (*read_press)(struct bmp280_data *, int *, int *);
> @@ -129,11 +140,13 @@ enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
>  static const struct iio_chan_spec bmp280_channels[] = {
>  	{
>  		.type = IIO_PRESSURE,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>  	},
>  	{
>  		.type = IIO_TEMP,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>  	},
>  };
>  
> @@ -339,6 +352,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  			break;
>  		}
>  		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			*val = 1 << data->oversampling_press;
> +			ret = IIO_VAL_INT;
> +			break;
> +		case IIO_TEMP:
> +			*val = 1 << data->oversampling_temp;
> +			ret = IIO_VAL_INT;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -349,22 +377,135 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int bmp280_write_oversampling_ratio_temp(struct bmp280_data *data,
> +					       int val)
> +{
> +	int i;
> +	const int *avail = data->chip_info->oversampling_temp_avail;
> +	const int n = data->chip_info->num_oversampling_temp_avail;
> +
> +	for (i = 0; i < n; i++) {
> +		if (avail[i] == val) {
> +			data->oversampling_temp = ilog2(val);
> +
> +			return data->chip_info->chip_config(data);
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int bmp280_write_oversampling_ratio_press(struct bmp280_data *data,
> +					       int val)
> +{
> +	int i;
> +	const int *avail = data->chip_info->oversampling_press_avail;
> +	const int n = data->chip_info->num_oversampling_press_avail;
> +
> +	for (i = 0; i < n; i++) {
> +		if (avail[i] == val) {
> +			data->oversampling_press = ilog2(val);
> +
> +			return data->chip_info->chip_config(data);
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int bmp280_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	int ret = 0;
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		mutex_lock(&data->lock);
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			ret = bmp280_write_oversampling_ratio_press(data, val);
> +			break;
> +		case IIO_TEMP:
> +			ret = bmp280_write_oversampling_ratio_temp(data, val);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		mutex_unlock(&data->lock);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t bmp280_show_avail(char *buf, const int *vals, const int n)
> +{
> +	size_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < n; i++)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ", vals[i]);
> +
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static ssize_t bmp280_show_temp_oversampling_avail(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
> +
> +	return bmp280_show_avail(buf, data->chip_info->oversampling_temp_avail,
> +				 data->chip_info->num_oversampling_temp_avail);
> +}
> +
> +static ssize_t bmp280_show_press_oversampling_avail(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct bmp280_data *data = iio_priv(dev_to_iio_dev(dev));
> +
> +	return bmp280_show_avail(buf, data->chip_info->oversampling_press_avail,
> +				 data->chip_info->num_oversampling_press_avail);
> +}
> +
> +static IIO_DEVICE_ATTR(in_temp_oversampling_ratio_available,
> +	S_IRUGO, bmp280_show_temp_oversampling_avail, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_pressure_oversampling_ratio_available,
> +	S_IRUGO, bmp280_show_press_oversampling_avail, NULL, 0);
> +
> +static struct attribute *bmp280_attributes[] = {
> +	&iio_dev_attr_in_temp_oversampling_ratio_available.dev_attr.attr,
> +	&iio_dev_attr_in_pressure_oversampling_ratio_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group bmp280_attrs_group = {
> +	.attrs = bmp280_attributes,
> +};
> +
>  static const struct iio_info bmp280_info = {
>  	.driver_module = THIS_MODULE,
>  	.read_raw = &bmp280_read_raw,
> +	.write_raw = &bmp280_write_raw,
> +	.attrs = &bmp280_attrs_group,
>  };
>  
>  static int bmp280_chip_config(struct bmp280_data *data)
>  {
>  	int ret;
> +	u8 osrs = BMP280_OSRS_TEMP_X(data->oversampling_temp) |
> +		  BMP280_OSRS_PRESS_X(data->oversampling_press);
>  
>  	ret = regmap_update_bits(data->regmap, BMP280_REG_CTRL_MEAS,
>  				 BMP280_OSRS_TEMP_MASK |
>  				 BMP280_OSRS_PRESS_MASK |
>  				 BMP280_MODE_MASK,
> -				 BMP280_OSRS_TEMP_2X |
> -				 BMP280_OSRS_PRESS_16X |
> -				 BMP280_MODE_NORMAL);
> +				 osrs | BMP280_MODE_NORMAL);
>  	if (ret < 0) {
>  		dev_err(&data->client->dev,
>  			"failed to write ctrl_meas register\n");
> @@ -383,8 +524,17 @@ static int bmp280_chip_config(struct bmp280_data *data)
>  	return ret;
>  }
>  
> +static const int bmp280_oversampling_avail[] = { 1, 2, 4, 8, 16 };
> +
>  static const struct bmp280_chip_info bmp280_chip_info = {
>  	.regmap_config = &bmp280_regmap_config,
> +
> +	.oversampling_temp_avail = bmp280_oversampling_avail,
> +	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> +
> +	.oversampling_press_avail = bmp280_oversampling_avail,
> +	.num_oversampling_press_avail = ARRAY_SIZE(bmp280_oversampling_avail),
> +
>  	.chip_config = bmp280_chip_config,
>  	.read_temp = bmp280_read_temp,
>  	.read_press = bmp280_read_press,
> @@ -439,7 +589,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
>  	if (ctrl_meas == BMP180_MEAS_TEMP)
>  		delay_us = 4500;
>  	else
> -		delay_us = conversion_time_max[ilog2(8)];
> +		delay_us = conversion_time_max[data->oversampling_press];
>  
>  	usleep_range(delay_us, delay_us + 1000);
>  
> @@ -591,7 +741,7 @@ static int bmp180_read_adc_press(struct bmp280_data *data, int *val)
>  {
>  	int ret;
>  	__be32 tmp = 0;
> -	u8 oss = ilog2(8);
> +	u8 oss = data->oversampling_press;
>  
>  	ret = bmp180_measure(data, BMP180_MEAS_PRESS_X(oss));
>  	if (ret)
> @@ -617,7 +767,7 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, s32 adc_press)
>  	s32 x1, x2, x3, p;
>  	s32 b3, b6;
>  	u32 b4, b7;
> -	s32 oss = ilog2(8);
> +	s32 oss = data->oversampling_press;
>  	struct bmp180_calib calib;
>  
>  	ret = bmp180_read_calib(data, &calib);
> @@ -678,8 +828,20 @@ static int bmp180_chip_config(struct bmp280_data *data)
>  	return 0;
>  }
>  
> +static const int bmp180_oversampling_temp_avail[] = { 1 };
> +static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
> +
>  static const struct bmp280_chip_info bmp180_chip_info = {
>  	.regmap_config = &bmp180_regmap_config,
> +
> +	.oversampling_temp_avail = bmp180_oversampling_temp_avail,
> +	.num_oversampling_temp_avail =
> +		ARRAY_SIZE(bmp180_oversampling_temp_avail),
> +
> +	.oversampling_press_avail = bmp180_oversampling_press_avail,
> +	.num_oversampling_press_avail =
> +		ARRAY_SIZE(bmp180_oversampling_press_avail),
> +
>  	.chip_config = bmp180_chip_config,
>  	.read_temp = bmp180_read_temp,
>  	.read_press = bmp180_read_press,
> @@ -711,9 +873,13 @@ static int bmp280_probe(struct i2c_client *client,
>  	switch (id->driver_data) {
>  	case BMP180_CHIP_ID:
>  		data->chip_info = &bmp180_chip_info;
> +		data->oversampling_press = ilog2(8);
> +		data->oversampling_temp = ilog2(1);
>  		break;
>  	case BMP280_CHIP_ID:
>  		data->chip_info = &bmp280_chip_info;
> +		data->oversampling_press = ilog2(16);
> +		data->oversampling_temp = ilog2(2);
>  		break;
>  	default:
>  		return -EINVAL;
> 

Re: [PATCH v2 2/2] iio: pressure: bmp280: add ability to control oversampling rate

[Vlad Dogaru]

Hi Akinobu,

On Sat, Apr 16, 2016 at 02:53:47AM +0900, Akinobu Mita wrote:
> This adds ability to control the oversampling ratio of the temperature
> and pressure measurement for both bmp180 and bmp280.
> (bmp280 is untested for now)

Code looks good, but I found a pretty big issue when testing with the
actual device.  Details inline.

> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Vlad Dogaru <vlad.dogaru@intel.com>
> Cc: Christoph Mair <christoph.mair@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald <pmeerw@pmeerw.net>
> ---
>  drivers/iio/pressure/bmp280.c | 202 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 184 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
> index 28daa74..fd931c7 100644
> --- a/drivers/iio/pressure/bmp280.c
> +++ b/drivers/iio/pressure/bmp280.c
> @@ -50,19 +50,21 @@
>  
>  #define BMP280_OSRS_TEMP_MASK		(BIT(7) | BIT(6) | BIT(5))
>  #define BMP280_OSRS_TEMP_SKIP		0
> -#define BMP280_OSRS_TEMP_1X		BIT(5)
> -#define BMP280_OSRS_TEMP_2X		BIT(6)
> -#define BMP280_OSRS_TEMP_4X		(BIT(6) | BIT(5))
> -#define BMP280_OSRS_TEMP_8X		BIT(7)
> -#define BMP280_OSRS_TEMP_16X		(BIT(7) | BIT(5))
> +#define BMP280_OSRS_TEMP_X(osrs_t)	((osrs_t) << 5)
> +#define BMP280_OSRS_TEMP_1X		BMP280_OSRS_TEMP_X(1)
> +#define BMP280_OSRS_TEMP_2X		BMP280_OSRS_TEMP_X(2)
> +#define BMP280_OSRS_TEMP_4X		BMP280_OSRS_TEMP_X(3)
> +#define BMP280_OSRS_TEMP_8X		BMP280_OSRS_TEMP_X(4)
> +#define BMP280_OSRS_TEMP_16X		BMP280_OSRS_TEMP_X(5)
>  
>  #define BMP280_OSRS_PRESS_MASK		(BIT(4) | BIT(3) | BIT(2))
>  #define BMP280_OSRS_PRESS_SKIP		0
> -#define BMP280_OSRS_PRESS_1X		BIT(2)
> -#define BMP280_OSRS_PRESS_2X		BIT(3)
> -#define BMP280_OSRS_PRESS_4X		(BIT(3) | BIT(2))
> -#define BMP280_OSRS_PRESS_8X		BIT(4)
> -#define BMP280_OSRS_PRESS_16X		(BIT(4) | BIT(2))
> +#define BMP280_OSRS_PRESS_X(osrs_p)	((osrs_p) << 2)
> +#define BMP280_OSRS_PRESS_1X		BMP280_OSRS_PRESS_X(1)
> +#define BMP280_OSRS_PRESS_2X		BMP280_OSRS_PRESS_X(2)
> +#define BMP280_OSRS_PRESS_4X		BMP280_OSRS_PRESS_X(3)
> +#define BMP280_OSRS_PRESS_8X		BMP280_OSRS_PRESS_X(4)
> +#define BMP280_OSRS_PRESS_16X		BMP280_OSRS_PRESS_X(5)
>  
>  #define BMP280_MODE_MASK		(BIT(1) | BIT(0))
>  #define BMP280_MODE_SLEEP		0
> @@ -104,6 +106,9 @@ struct bmp280_data {
>  	struct regmap *regmap;
>  	const struct bmp280_chip_info *chip_info;
>  
> +	u8 oversampling_press;
> +	u8 oversampling_temp;
> +
>  	/*
>  	 * Carryover value from temperature conversion, used in pressure
>  	 * calculation.
> @@ -114,6 +119,12 @@ struct bmp280_data {
>  struct bmp280_chip_info {
>  	const struct regmap_config *regmap_config;
>  
> +	const int *oversampling_temp_avail;
> +	int num_oversampling_temp_avail;
> +
> +	const int *oversampling_press_avail;
> +	int num_oversampling_press_avail;
> +
>  	int (*chip_config)(struct bmp280_data *);
>  	int (*read_temp)(struct bmp280_data *, int *);
>  	int (*read_press)(struct bmp280_data *, int *, int *);
> @@ -129,11 +140,13 @@ enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
>  static const struct iio_chan_spec bmp280_channels[] = {
>  	{
>  		.type = IIO_PRESSURE,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>  	},
>  	{
>  		.type = IIO_TEMP,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>  	},
>  };
>  
> @@ -339,6 +352,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  			break;
>  		}
>  		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			*val = 1 << data->oversampling_press;
> +			ret = IIO_VAL_INT;
> +			break;
> +		case IIO_TEMP:
> +			*val = 1 << data->oversampling_temp;
> +			ret = IIO_VAL_INT;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -349,22 +377,135 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int bmp280_write_oversampling_ratio_temp(struct bmp280_data *data,
> +					       int val)
> +{
> +	int i;
> +	const int *avail = data->chip_info->oversampling_temp_avail;
> +	const int n = data->chip_info->num_oversampling_temp_avail;
> +
> +	for (i = 0; i < n; i++) {
> +		if (avail[i] == val) {
> +			data->oversampling_temp = ilog2(val);

This is incorrect.  According to the datasheet, you should add 1 to the
return value of ilog2.  Thus, if val is 16, ilog2 is 4 and the
oversampling factor should be set to 5.  See Table 21 and Table 22 of
the BMP280 datasheet.

I wouldn't have noticed this but for the following bug: as the code
stands right now, setting an oversampling factor of 1 will result in
setting the bits in the register to 0, which corresponds to "skip
measurement" and a constant reading of 25.0C and ~77kPa.

> +
> +			return data->chip_info->chip_config(data);
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int bmp280_write_oversampling_ratio_press(struct bmp280_data *data,
> +					       int val)
> +{
> +	int i;
> +	const int *avail = data->chip_info->oversampling_press_avail;
> +	const int n = data->chip_info->num_oversampling_press_avail;
> +
> +	for (i = 0; i < n; i++) {
> +		if (avail[i] == val) {
> +			data->oversampling_press = ilog2(val);

Same remark as above.

Thanks,
Vlad

Re: [PATCH v2 2/2] iio: pressure: bmp280: add ability to control oversampling rate

[Akinobu Mita]

Hi Vlad,

2016-04-18 20:52 GMT+09:00 Vlad Dogaru <vlad.dogaru@intel.com>:
> Hi Akinobu,
>
> On Sat, Apr 16, 2016 at 02:53:47AM +0900, Akinobu Mita wrote:
>> This adds ability to control the oversampling ratio of the temperature
>> and pressure measurement for both bmp180 and bmp280.
>> (bmp280 is untested for now)
>
> Code looks good, but I found a pretty big issue when testing with the
> actual device.  Details inline.
>
>>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> Cc: Vlad Dogaru <vlad.dogaru@intel.com>
>> Cc: Christoph Mair <christoph.mair@gmail.com>
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Cc: Hartmut Knaack <knaack.h@gmx.de>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: Peter Meerwald <pmeerw@pmeerw.net>
>> ---
>>  drivers/iio/pressure/bmp280.c | 202 ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 184 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
>> index 28daa74..fd931c7 100644
>> --- a/drivers/iio/pressure/bmp280.c
>> +++ b/drivers/iio/pressure/bmp280.c
>> @@ -50,19 +50,21 @@
>>
>>  #define BMP280_OSRS_TEMP_MASK                (BIT(7) | BIT(6) | BIT(5))
>>  #define BMP280_OSRS_TEMP_SKIP                0
>> -#define BMP280_OSRS_TEMP_1X          BIT(5)
>> -#define BMP280_OSRS_TEMP_2X          BIT(6)
>> -#define BMP280_OSRS_TEMP_4X          (BIT(6) | BIT(5))
>> -#define BMP280_OSRS_TEMP_8X          BIT(7)
>> -#define BMP280_OSRS_TEMP_16X         (BIT(7) | BIT(5))
>> +#define BMP280_OSRS_TEMP_X(osrs_t)   ((osrs_t) << 5)
>> +#define BMP280_OSRS_TEMP_1X          BMP280_OSRS_TEMP_X(1)
>> +#define BMP280_OSRS_TEMP_2X          BMP280_OSRS_TEMP_X(2)
>> +#define BMP280_OSRS_TEMP_4X          BMP280_OSRS_TEMP_X(3)
>> +#define BMP280_OSRS_TEMP_8X          BMP280_OSRS_TEMP_X(4)
>> +#define BMP280_OSRS_TEMP_16X         BMP280_OSRS_TEMP_X(5)
>>
>>  #define BMP280_OSRS_PRESS_MASK               (BIT(4) | BIT(3) | BIT(2))
>>  #define BMP280_OSRS_PRESS_SKIP               0
>> -#define BMP280_OSRS_PRESS_1X         BIT(2)
>> -#define BMP280_OSRS_PRESS_2X         BIT(3)
>> -#define BMP280_OSRS_PRESS_4X         (BIT(3) | BIT(2))
>> -#define BMP280_OSRS_PRESS_8X         BIT(4)
>> -#define BMP280_OSRS_PRESS_16X                (BIT(4) | BIT(2))
>> +#define BMP280_OSRS_PRESS_X(osrs_p)  ((osrs_p) << 2)
>> +#define BMP280_OSRS_PRESS_1X         BMP280_OSRS_PRESS_X(1)
>> +#define BMP280_OSRS_PRESS_2X         BMP280_OSRS_PRESS_X(2)
>> +#define BMP280_OSRS_PRESS_4X         BMP280_OSRS_PRESS_X(3)
>> +#define BMP280_OSRS_PRESS_8X         BMP280_OSRS_PRESS_X(4)
>> +#define BMP280_OSRS_PRESS_16X                BMP280_OSRS_PRESS_X(5)
>>
>>  #define BMP280_MODE_MASK             (BIT(1) | BIT(0))
>>  #define BMP280_MODE_SLEEP            0
>> @@ -104,6 +106,9 @@ struct bmp280_data {
>>       struct regmap *regmap;
>>       const struct bmp280_chip_info *chip_info;
>>
>> +     u8 oversampling_press;
>> +     u8 oversampling_temp;
>> +
>>       /*
>>        * Carryover value from temperature conversion, used in pressure
>>        * calculation.
>> @@ -114,6 +119,12 @@ struct bmp280_data {
>>  struct bmp280_chip_info {
>>       const struct regmap_config *regmap_config;
>>
>> +     const int *oversampling_temp_avail;
>> +     int num_oversampling_temp_avail;
>> +
>> +     const int *oversampling_press_avail;
>> +     int num_oversampling_press_avail;
>> +
>>       int (*chip_config)(struct bmp280_data *);
>>       int (*read_temp)(struct bmp280_data *, int *);
>>       int (*read_press)(struct bmp280_data *, int *, int *);
>> @@ -129,11 +140,13 @@ enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
>>  static const struct iio_chan_spec bmp280_channels[] = {
>>       {
>>               .type = IIO_PRESSURE,
>> -             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
>> +                                   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>>       },
>>       {
>>               .type = IIO_TEMP,
>> -             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
>> +                                   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>>       },
>>  };
>>
>> @@ -339,6 +352,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>>                       break;
>>               }
>>               break;
>> +     case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +             switch (chan->type) {
>> +             case IIO_PRESSURE:
>> +                     *val = 1 << data->oversampling_press;
>> +                     ret = IIO_VAL_INT;
>> +                     break;
>> +             case IIO_TEMP:
>> +                     *val = 1 << data->oversampling_temp;
>> +                     ret = IIO_VAL_INT;
>> +                     break;
>> +             default:
>> +                     ret = -EINVAL;
>> +                     break;
>> +             }
>> +             break;
>>       default:
>>               ret = -EINVAL;
>>               break;
>> @@ -349,22 +377,135 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
>>       return ret;
>>  }
>>
>> +static int bmp280_write_oversampling_ratio_temp(struct bmp280_data *data,
>> +                                            int val)
>> +{
>> +     int i;
>> +     const int *avail = data->chip_info->oversampling_temp_avail;
>> +     const int n = data->chip_info->num_oversampling_temp_avail;
>> +
>> +     for (i = 0; i < n; i++) {
>> +             if (avail[i] == val) {
>> +                     data->oversampling_temp = ilog2(val);
>
> This is incorrect.  According to the datasheet, you should add 1 to the
> return value of ilog2.  Thus, if val is 16, ilog2 is 4 and the
> oversampling factor should be set to 5.  See Table 21 and Table 22 of
> the BMP280 datasheet.
>
> I wouldn't have noticed this but for the following bug: as the code
> stands right now, setting an oversampling factor of 1 will result in
> setting the bits in the register to 0, which corresponds to "skip
> measurement" and a constant reading of 25.0C and ~77kPa.

Thanks for testing and spotting the issue!

I'll fix the calculation for osrs_t and osrs_p in bmp280_chip_config().
And also add comment describing bmp280_data->oversampling_{temp,press}
are log of base 2 of oversampling rate, not oversampling register settings.