iio:pressure:bmp280: read compensation data only at init

PATCH In review Hartmut Knaack 3 participants 13 messages Oct 31, 2014 – Nov 22, 2014 IIO

Share & tools

Hartmut Knaack

author

2014-10-31 01:26 UTC #0 lore raw

PATCH 3/3 iio:pressure:bmp280: read compensation data only at init

Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).

Signed-off-by Hartmut Knaack <[email protected]>

---

drivers/iio/pressure/bmp280.c +27 ~~-33~~

index 4f6ae4d..36e425c 100644
@@ -68,10 +68,19 @@
 #define BMP280_CHIP_ID			0x58
 #define BMP280_SOFT_RESET_VAL		0xB6
 
+/* Compensation parameters. */
+struct bmp280_comp {
+	u16 dig_p1;
+	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
+	u16 dig_t1;
+	s16 dig_t2, dig_t3;
+};
+
 struct bmp280_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	struct regmap *regmap;
+	struct bmp280_comp comp;
 
 	/*
 	 * Carryover value from temperature conversion, used in pressure

@@ -80,17 +89,6 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
-
 static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,

@@ -141,11 +139,11 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
+static int bmp280_read_compensation_temp(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
 			       buf, BMP280_COMP_TEMP_REG_COUNT);

@@ -162,11 +160,11 @@ static int bmp280_read_compensation_temp(struct bmp280_data *data,
 	return 0;
 }
 
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
+static int bmp280_read_compensation_press(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
 			       buf, BMP280_COMP_PRESS_REG_COUNT);

@@ -196,11 +194,10 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
-				  s32 adc_temp)
+static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp)
 {
 	s32 var1, var2;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;

@@ -219,11 +216,10 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
-				   s32 adc_press)
+static u32 bmp280_compensate_press(struct bmp280_data *data, s32 adc_press)
 {
 	s64 var1, var2, p;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = ((s64) data->t_fine) - 128000;
 	var2 = var1 * var1 * (s64) comp->dig_p6;

@@ -249,11 +245,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -263,7 +254,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -284,11 +275,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -303,7 +289,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

@@ -375,6 +361,14 @@ static int bmp280_chip_init(struct bmp280_data *data)
 		return ret;
 	}
 
+	ret = bmp280_read_compensation_temp(data);
+	if (ret < 0)
+		return ret;
+
+	ret = bmp280_read_compensation_press(data);
+	if (ret < 0)
+		return ret;
+
 	return ret;
 }

Vlad Dogaru

2014-10-31 11:46 UTC #1 lore raw

Quoted 4 lines · Hartmut Knaack

Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).

My first version of the patch did this, but Jonathan suggested [1] that, since we're using regmap, we can rely on it for caching the calibration parameters. I have no preference for either approach.

[1] http://www.spinics.net/lists/linux-iio/msg15099.html

Quoted 157 lines

Signed-off-by Hartmut Knaack <[email protected]>

---

drivers/iio/pressure/bmp280.c +27 ~~-33~~

index 4f6ae4d..36e425c 100644
@@ -68,10 +68,19 @@
 #define BMP280_CHIP_ID			0x58
 #define BMP280_SOFT_RESET_VAL		0xB6
 
+/* Compensation parameters. */
+struct bmp280_comp {
+	u16 dig_p1;
+	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
+	u16 dig_t1;
+	s16 dig_t2, dig_t3;
+};
+
 struct bmp280_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	struct regmap *regmap;
+	struct bmp280_comp comp;
 
 	/*
 	 * Carryover value from temperature conversion, used in pressure

@@ -80,17 +89,6 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
-
 static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,

@@ -141,11 +139,11 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
+static int bmp280_read_compensation_temp(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
 			       buf, BMP280_COMP_TEMP_REG_COUNT);

@@ -162,11 +160,11 @@ static int bmp280_read_compensation_temp(struct bmp280_data *data,
 	return 0;
 }
 
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
+static int bmp280_read_compensation_press(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
 			       buf, BMP280_COMP_PRESS_REG_COUNT);

@@ -196,11 +194,10 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
-				  s32 adc_temp)
+static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp)
 {
 	s32 var1, var2;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;

@@ -219,11 +216,10 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
-				   s32 adc_press)
+static u32 bmp280_compensate_press(struct bmp280_data *data, s32 adc_press)
 {
 	s64 var1, var2, p;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = ((s64) data->t_fine) - 128000;
 	var2 = var1 * var1 * (s64) comp->dig_p6;

@@ -249,11 +245,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -263,7 +254,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -284,11 +275,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -303,7 +289,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

@@ -375,6 +361,14 @@ static int bmp280_chip_init(struct bmp280_data *data)
 		return ret;
 	}
 
+	ret = bmp280_read_compensation_temp(data);
+	if (ret < 0)
+		return ret;
+
+	ret = bmp280_read_compensation_press(data);
+	if (ret < 0)
+		return ret;
+
 	return ret;
 }

Hartmut Knaack

author

2014-10-31 19:16 UTC #2 lore raw

Vlad Dogaru schrieb am 31.10.2014 12:47:

Quoted 10 lines · Hartmut Knaack

Quoted 4 lines
Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).
My first version of the patch did this, but Jonathan suggested [1] that, since we're using regmap, we can rely on it for caching the calibration parameters. I have no preference for either approach.

Indeed, I missed to have a closer look at that version, as I discarded it when your second or third version came out.
So, one question remaining is, if your implementation was, what Jonathan had in mind when he gave the hint about using the regmap cache. I mainly see the disadvantage of copying to the buffer and from there to the variables (and depending on endianness even a conversion) for every single measurement.

Quoted 164 lines

[1] http://www.spinics.net/lists/linux-iio/msg15099.html

Quoted 147 lines

Signed-off-by Hartmut Knaack <[email protected]>

---

drivers/iio/pressure/bmp280.c +20 ~~-33~~

index 4f6ae4d..36e425c 100644
@@ -68,10 +68,19 @@
 #define BMP280_CHIP_ID			0x58
 #define BMP280_SOFT_RESET_VAL		0xB6
 
+/* Compensation parameters. */
+struct bmp280_comp {
+	u16 dig_p1;
+	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
+	u16 dig_t1;
+	s16 dig_t2, dig_t3;
+};
+
 struct bmp280_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	struct regmap *regmap;
+	struct bmp280_comp comp;
 
 	/*
 	 * Carryover value from temperature conversion, used in pressure

@@ -80,17 +89,6 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
-
 static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,

@@ -141,11 +139,11 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
+static int bmp280_read_compensation_temp(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
 			       buf, BMP280_COMP_TEMP_REG_COUNT);

@@ -162,11 +160,11 @@ static int bmp280_read_compensation_temp(struct bmp280_data *data,
 	return 0;
 }
 
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
+static int bmp280_read_compensation_press(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
 			       buf, BMP280_COMP_PRESS_REG_COUNT);

@@ -196,11 +194,10 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
-				  s32 adc_temp)
+static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp)
 {
 	s32 var1, var2;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;

@@ -219,11 +216,10 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
-				   s32 adc_press)
+static u32 bmp280_compensate_press(struct bmp280_data *data, s32 adc_press)
 {
 	s64 var1, var2, p;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = ((s64) data->t_fine) - 128000;
 	var2 = var1 * var1 * (s64) comp->dig_p6;

@@ -249,11 +245,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -263,7 +254,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -284,11 +275,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -303,7 +289,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

@@ -375,6 +361,14 @@ static int bmp280_chip_init(struct bmp280_data *data)
 		return ret;
 	}
 
+	ret = bmp280_read_compensation_temp(data);

drivers/iio/pressure/bmp280.c static int bmp280_chip_init(struct bmp280_data *data) · line 374

Quoted 10 lines

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

+	if (ret < 0)
+		return ret;
+
+	ret = bmp280_read_compensation_press(data);
+	if (ret < 0)
+		return ret;
+
 	return ret;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jonathan Cameron

maintainer

2014-11-05 15:53 UTC #3 lore raw

Quoted 7 lines · Hartmut Knaack

Vlad Dogaru schrieb am 31.10.2014 12:47:
Quoted 6 lines · Hartmut Knaack
Quoted 4 lines
Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).

Quoted 6 lines

Quoted 4 lines
My first version of the patch did this, but Jonathan suggested [1] that, since we're using regmap, we can rely on it for caching the calibration parameters. I have no preference for either approach.
Indeed, I missed to have a closer look at that version, as I discarded it when your second or third version came out.
So, one question remaining is, if your implementation was, what Jonathan had in mind when he gave the hint about using the regmap cache. I mainly see the disadvantage of copying to the buffer and from there to the variables (and depending on endianness even a conversion) for every single measurement.

Gah. Now I have to remember what I was thinking!

Anyhow, I just didn't much like the double cache of these values. The little endian conversions are pretty trivial...
The code only ended up a little involved because of the structures to pass this data around. Why not just have an enum saying what data is where and pass the buffer. Then do the little endian on demand and let the compiler filter out the repeats?

So combine bmp280_read_compensation_press and bmp280_compensate_press to something like...

enum {
	P1,
	P2,
	P3,
	P4,
	P5,
	P6,
	P7,
	P8,
	P9
}; // a rare occasion where maybe all on one line would be good ;)

int bmp280_compensate_press(struct bmp280_data *data)

{
	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       buf, BMP280_COMP_PRESS_REG_COUNT);
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(buf[P6]);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(buf[P5])) << 17);
	var2 = var2 + (((s64) le16_to_cpu(buf[P4])) << 35);
	var1 = ((var1 * var1 * (s64) le16_to_cpu(buf[P3])) >> 8) +
		((var1 * (s64) le16_to_cpu(buf[P2])) << 12);
	var1 = (((((s64) 1) << 47) + var1)) * ((s64) le16_to_cpu(buf[P1])) >> 33;

	if (var1 == 0)
		return 0;

	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
	p = div64_s64(p, var1);
	var1 = (((s64) le16_to_cpu(buf[P9]) * (p >> 13) * (p >> 13)) >> 25;
	var2 = (((s64) le16_to_cpu[buf[P8]) * p) >> 19;
	p = ((p + var1 + var2) >> 8) + (((s64) le16_to_cpu(buf[P7])) << 4);

	return (u32) p;
}

Just a thought. I was never terribly fussed about this other than disliking data duplication!

Quoted 170 lines

Quoted 148 lines

[1] http://www.spinics.net/lists/linux-iio/msg15099.html

Quoted 146 lines

Signed-off-by Hartmut Knaack <[email protected]>

---

drivers/iio/pressure/bmp280.c +19 ~~-33~~

index 4f6ae4d..36e425c 100644
@@ -68,10 +68,19 @@
 #define BMP280_CHIP_ID			0x58
 #define BMP280_SOFT_RESET_VAL		0xB6
 
+/* Compensation parameters. */
+struct bmp280_comp {
+	u16 dig_p1;
+	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
+	u16 dig_t1;
+	s16 dig_t2, dig_t3;
+};
+
 struct bmp280_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	struct regmap *regmap;
+	struct bmp280_comp comp;
 
 	/*
 	 * Carryover value from temperature conversion, used in pressure

@@ -80,17 +89,6 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
-
 static const struct iio_chan_spec bmp280_channels[] = {
 	{
 		.type = IIO_PRESSURE,

@@ -141,11 +139,11 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
+static int bmp280_read_compensation_temp(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
 			       buf, BMP280_COMP_TEMP_REG_COUNT);

@@ -162,11 +160,11 @@ static int bmp280_read_compensation_temp(struct bmp280_data *data,
 	return 0;
 }
 
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
+static int bmp280_read_compensation_press(struct bmp280_data *data)
 {
 	int ret;
 	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+	struct bmp280_comp *comp = &data->comp;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
 			       buf, BMP280_COMP_PRESS_REG_COUNT);

@@ -196,11 +194,10 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
-				  s32 adc_temp)
+static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp)
 {
 	s32 var1, var2;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;

@@ -219,11 +216,10 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  *
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
-static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
-				   s32 adc_press)
+static u32 bmp280_compensate_press(struct bmp280_data *data, s32 adc_press)
 {
 	s64 var1, var2, p;
+	struct bmp280_comp *comp = &data->comp;
 
 	var1 = ((s64) data->t_fine) - 128000;
 	var2 = var1 * var1 * (s64) comp->dig_p6;

@@ -249,11 +245,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -263,7 +254,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -284,11 +275,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -303,7 +289,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

@@ -375,6 +361,14 @@ static int bmp280_chip_init(struct bmp280_data *data)
 		return ret;
 	}

Quoted 16 lines

drivers/iio/pressure/bmp280.c static int bmp280_chip_init(struct bmp280_data *data) · line 374
Quoted 11 lines
drivers/iio/pressure/bmp280.c +8 -0
+	ret = bmp280_read_compensation_temp(data);
+	if (ret < 0)
+		return ret;
+
+	ret = bmp280_read_compensation_press(data);
+	if (ret < 0)
+		return ret;
+
 	return ret;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vlad Dogaru

2014-11-06 13:03 UTC #4 lore raw

Quoted 70 lines · Jonathan Cameron

Quoted 7 lines · Hartmut Knaack
Vlad Dogaru schrieb am 31.10.2014 12:47:
Quoted 6 lines · Hartmut Knaack
Quoted 4 lines
Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).
Quoted 6 lines
Quoted 4 lines
My first version of the patch did this, but Jonathan suggested [1] that, since we're using regmap, we can rely on it for caching the calibration parameters. I have no preference for either approach.
Indeed, I missed to have a closer look at that version, as I discarded it when your second or third version came out.
So, one question remaining is, if your implementation was, what Jonathan had in mind when he gave the hint about using the regmap cache. I mainly see the disadvantage of copying to the buffer and from there to the variables (and depending on endianness even a conversion) for every single measurement.
Gah. Now I have to remember what I was thinking!
Anyhow, I just didn't much like the double cache of these values. The little endian conversions are pretty trivial...
The code only ended up a little involved because of the structures to pass this data around. Why not just have an enum saying what data is where and pass the buffer. Then do the little endian on demand and let the compiler filter out the repeats?
So combine bmp280_read_compensation_press and bmp280_compensate_press to something like...
enum {
	P1,
	P2,
	P3,
	P4,
	P5,
	P6,
	P7,
	P8,
	P9
}; // a rare occasion where maybe all on one line would be good ;)
int bmp280_compensate_press(struct bmp280_data *data)
{
	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       buf, BMP280_COMP_PRESS_REG_COUNT);
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(buf[P6]);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(buf[P5])) << 17);
	var2 = var2 + (((s64) le16_to_cpu(buf[P4])) << 35);
	var1 = ((var1 * var1 * (s64) le16_to_cpu(buf[P3])) >> 8) +
		((var1 * (s64) le16_to_cpu(buf[P2])) << 12);
	var1 = (((((s64) 1) << 47) + var1)) * ((s64) le16_to_cpu(buf[P1])) >> 33;

	if (var1 == 0)
		return 0;

	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
	p = div64_s64(p, var1);
	var1 = (((s64) le16_to_cpu(buf[P9]) * (p >> 13) * (p >> 13)) >> 25;
	var2 = (((s64) le16_to_cpu[buf[P8]) * p) >> 19;
	p = ((p + var1 + var2) >> 8) + (((s64) le16_to_cpu(buf[P7])) << 4);

	return (u32) p;
}

Quoted 3 lines

Just a thought. I was never terribly fussed about this other than disliking data duplication!
Quoted 1 line
Quoted 1 line
[1] http://www.spinics.net/lists/linux-iio/msg15099.html

I don't think the enum helps readability too much here. At least it doesn't help me :)

How about reading data straight to the (packed) calibration struct and doing the endinanness conversions inline? Something like this:

int bmp280_compensate_press(struct bmp280_data *data)

{
	struct bmp280_comp_press comp;
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       &comp, sizeof (comp));
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(comp.dig_p6);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(comp.dig_p5)) << 17);

	/* etc */

This saves us the extra copying of parameters from buffer to structure, and is a bit more clear than indexing the buffer with enum members, IMO.

Thanks,
Vlad

Hartmut Knaack

author

2014-11-10 23:11 UTC #5 lore raw

Vlad Dogaru schrieb am 06.11.2014 14:02:

Quoted 100 lines · Jonathan Cameron

Quoted 70 lines · Hartmut Knaack
Quoted 7 lines
Vlad Dogaru schrieb am 31.10.2014 12:47:
Quoted 6 lines · Hartmut Knaack
Quoted 4 lines
Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).
Quoted 6 lines
Quoted 4 lines
My first version of the patch did this, but Jonathan suggested [1] that, since we're using regmap, we can rely on it for caching the calibration parameters. I have no preference for either approach.
Indeed, I missed to have a closer look at that version, as I discarded it when your second or third version came out.
So, one question remaining is, if your implementation was, what Jonathan had in mind when he gave the hint about using the regmap cache. I mainly see the disadvantage of copying to the buffer and from there to the variables (and depending on endianness even a conversion) for every single measurement.
Gah. Now I have to remember what I was thinking!
Anyhow, I just didn't much like the double cache of these values. The little endian conversions are pretty trivial...
The code only ended up a little involved because of the structures to pass this data around. Why not just have an enum saying what data is where and pass the buffer. Then do the little endian on demand and let the compiler filter out the repeats?
So combine bmp280_read_compensation_press and bmp280_compensate_press to something like...
enum {
	P1,
	P2,
	P3,
	P4,
	P5,
	P6,
	P7,
	P8,
	P9
}; // a rare occasion where maybe all on one line would be good ;)
int bmp280_compensate_press(struct bmp280_data *data)
{
	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       buf, BMP280_COMP_PRESS_REG_COUNT);
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(buf[P6]);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(buf[P5])) << 17);
	var2 = var2 + (((s64) le16_to_cpu(buf[P4])) << 35);
	var1 = ((var1 * var1 * (s64) le16_to_cpu(buf[P3])) >> 8) +
		((var1 * (s64) le16_to_cpu(buf[P2])) << 12);
	var1 = (((((s64) 1) << 47) + var1)) * ((s64) le16_to_cpu(buf[P1])) >> 33;

	if (var1 == 0)
		return 0;

	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
	p = div64_s64(p, var1);
	var1 = (((s64) le16_to_cpu(buf[P9]) * (p >> 13) * (p >> 13)) >> 25;
	var2 = (((s64) le16_to_cpu[buf[P8]) * p) >> 19;
	p = ((p + var1 + var2) >> 8) + (((s64) le16_to_cpu(buf[P7])) << 4);

	return (u32) p;
}
Quoted 3 lines
Just a thought. I was never terribly fussed about this other than disliking data duplication!
Quoted 1 line
Quoted 1 line
[1] http://www.spinics.net/lists/linux-iio/msg15099.html
I don't think the enum helps readability too much here. At least it doesn't help me :)
How about reading data straight to the (packed) calibration struct and doing the endinanness conversions inline? Something like this:
int bmp280_compensate_press(struct bmp280_data *data)
{
	struct bmp280_comp_press comp;
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       &comp, sizeof (comp));
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(comp.dig_p6);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(comp.dig_p5)) << 17);

	/* etc */

Quoted 5 lines

This saves us the extra copying of parameters from buffer to structure, and is a bit more clear than indexing the buffer with enum members, IMO.
Thanks,
Vlad

I compile tested several solutions, and Jonathans' resulted in the lowest module size. Downside of course are the increased complexity of the equations introduced by the endianness conversion and that copying from regmap cache to buf costs some resources. But I think that memory footprint has a higher importance, so I slightly favor Jonathans solution.

Jonathan, do you like to send a patch with your solution?

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jonathan Cameron

maintainer

2014-11-15 16:06 UTC #6 lore raw

Quoted 104 lines · Hartmut Knaack

Vlad Dogaru schrieb am 06.11.2014 14:02:
Quoted 103 lines · Jonathan Cameron
Quoted 70 lines · Hartmut Knaack
Quoted 7 lines
Vlad Dogaru schrieb am 31.10.2014 12:47:
Quoted 6 lines · Hartmut Knaack
Quoted 4 lines
Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).
Quoted 6 lines
Quoted 4 lines
My first version of the patch did this, but Jonathan suggested [1] that, since we're using regmap, we can rely on it for caching the calibration parameters. I have no preference for either approach.
Indeed, I missed to have a closer look at that version, as I discarded it when your second or third version came out.
So, one question remaining is, if your implementation was, what Jonathan had in mind when he gave the hint about using the regmap cache. I mainly see the disadvantage of copying to the buffer and from there to the variables (and depending on endianness even a conversion) for every single measurement.
Gah. Now I have to remember what I was thinking!
Anyhow, I just didn't much like the double cache of these values. The little endian conversions are pretty trivial...
The code only ended up a little involved because of the structures to pass this data around. Why not just have an enum saying what data is where and pass the buffer. Then do the little endian on demand and let the compiler filter out the repeats?
So combine bmp280_read_compensation_press and bmp280_compensate_press to something like...
enum {
	P1,
	P2,
	P3,
	P4,
	P5,
	P6,
	P7,
	P8,
	P9
}; // a rare occasion where maybe all on one line would be good ;)
int bmp280_compensate_press(struct bmp280_data *data)
{
	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       buf, BMP280_COMP_PRESS_REG_COUNT);
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(buf[P6]);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(buf[P5])) << 17);
	var2 = var2 + (((s64) le16_to_cpu(buf[P4])) << 35);
	var1 = ((var1 * var1 * (s64) le16_to_cpu(buf[P3])) >> 8) +
		((var1 * (s64) le16_to_cpu(buf[P2])) << 12);
	var1 = (((((s64) 1) << 47) + var1)) * ((s64) le16_to_cpu(buf[P1])) >> 33;

	if (var1 == 0)
		return 0;

	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
	p = div64_s64(p, var1);
	var1 = (((s64) le16_to_cpu(buf[P9]) * (p >> 13) * (p >> 13)) >> 25;
	var2 = (((s64) le16_to_cpu[buf[P8]) * p) >> 19;
	p = ((p + var1 + var2) >> 8) + (((s64) le16_to_cpu(buf[P7])) << 4);

	return (u32) p;
}
Quoted 3 lines
Just a thought. I was never terribly fussed about this other than disliking data duplication!
Quoted 1 line
Quoted 1 line
[1] http://www.spinics.net/lists/linux-iio/msg15099.html
I don't think the enum helps readability too much here. At least it doesn't help me :)
How about reading data straight to the (packed) calibration struct and doing the endinanness conversions inline? Something like this:
int bmp280_compensate_press(struct bmp280_data *data)
{
	struct bmp280_comp_press comp;
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       &comp, sizeof (comp));
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(comp.dig_p6);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(comp.dig_p5)) << 17);

	/* etc */
This saves us the extra copying of parameters from buffer to structure, and is a bit more clear than indexing the buffer with enum members, IMO.

Quoted 6 lines

Quoted 2 lines
Thanks,
Vlad
I compile tested several solutions, and Jonathans' resulted in the lowest module size. Downside of course are the increased complexity of the equations introduced by the endianness conversion and that copying from regmap cache to buf costs some resources. But I think that memory footprint has a higher importance, so I slightly favor Jonathans solution.
Jonathan, do you like to send a patch with your solution?

Err. not really :) Bit snowed under so whilst I'll probably get to it one day it might not be anytime soon.

Anyone else who happens to want to do it then feel free ;)

Quoted 6 lines

Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vlad Dogaru

2014-11-17 15:16 UTC #7 lore raw

Quoted 102 lines · Jonathan Cameron

Quoted 101 lines · Hartmut Knaack
Vlad Dogaru schrieb am 06.11.2014 14:02:
Quoted 100 lines · Jonathan Cameron
Quoted 70 lines · Hartmut Knaack
Quoted 7 lines
Vlad Dogaru schrieb am 31.10.2014 12:47:
Quoted 6 lines · Hartmut Knaack
Quoted 4 lines
Compensation data is hard coded into the sensors, so it is sufficient to just read it once during device initialization. Therefor struct bmp280_comp should be part of bmp280_data (since the elements of bmp280_comp_temp and bmp280_comp_press have distinct names, they could be merged into bmp280_comp).
Quoted 6 lines
Quoted 4 lines
My first version of the patch did this, but Jonathan suggested [1] that, since we're using regmap, we can rely on it for caching the calibration parameters. I have no preference for either approach.
Indeed, I missed to have a closer look at that version, as I discarded it when your second or third version came out.
So, one question remaining is, if your implementation was, what Jonathan had in mind when he gave the hint about using the regmap cache. I mainly see the disadvantage of copying to the buffer and from there to the variables (and depending on endianness even a conversion) for every single measurement.
Gah. Now I have to remember what I was thinking!
Anyhow, I just didn't much like the double cache of these values. The little endian conversions are pretty trivial...
The code only ended up a little involved because of the structures to pass this data around. Why not just have an enum saying what data is where and pass the buffer. Then do the little endian on demand and let the compiler filter out the repeats?
So combine bmp280_read_compensation_press and bmp280_compensate_press to something like...
enum {
	P1,
	P2,
	P3,
	P4,
	P5,
	P6,
	P7,
	P8,
	P9
}; // a rare occasion where maybe all on one line would be good ;)
int bmp280_compensate_press(struct bmp280_data *data)
{
	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       buf, BMP280_COMP_PRESS_REG_COUNT);
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(buf[P6]);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(buf[P5])) << 17);
	var2 = var2 + (((s64) le16_to_cpu(buf[P4])) << 35);
	var1 = ((var1 * var1 * (s64) le16_to_cpu(buf[P3])) >> 8) +
		((var1 * (s64) le16_to_cpu(buf[P2])) << 12);
	var1 = (((((s64) 1) << 47) + var1)) * ((s64) le16_to_cpu(buf[P1])) >> 33;

	if (var1 == 0)
		return 0;

	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
	p = div64_s64(p, var1);
	var1 = (((s64) le16_to_cpu(buf[P9]) * (p >> 13) * (p >> 13)) >> 25;
	var2 = (((s64) le16_to_cpu[buf[P8]) * p) >> 19;
	p = ((p + var1 + var2) >> 8) + (((s64) le16_to_cpu(buf[P7])) << 4);

	return (u32) p;
}
Quoted 3 lines
Just a thought. I was never terribly fussed about this other than disliking data duplication!
Quoted 1 line
Quoted 1 line
[1] http://www.spinics.net/lists/linux-iio/msg15099.html
I don't think the enum helps readability too much here. At least it doesn't help me :)
How about reading data straight to the (packed) calibration struct and doing the endinanness conversions inline? Something like this:
int bmp280_compensate_press(struct bmp280_data *data)
{
	struct bmp280_comp_press comp;
	s64 var1, var2, p;

	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
			       &comp, sizeof (comp));
	if (ret < 0) {
		dev_err(&data->client->dev,
			"failed to read pressure calibration parameters\n");
		return ret;
	}

	var1 = ((s64) data->t_fine) - 128000;
	var2 = var1 * var1 * (s64) le16_to_cpu(comp.dig_p6);
	var2 = var2 + ((var1 * (s64) le16_to_cpu(comp.dig_p5)) << 17);

	/* etc */

Quoted 7 lines

Quoted 7 lines
Quoted 5 lines
This saves us the extra copying of parameters from buffer to structure, and is a bit more clear than indexing the buffer with enum members, IMO.
Thanks,
Vlad
I compile tested several solutions, and Jonathans' resulted in the lowest module size. Downside of course are the increased complexity of the equations introduced by the endianness conversion and that copying from regmap cache to buf costs some resources. But I think that memory footprint has a higher importance, so I slightly favor Jonathans solution.

Sounds good, thanks for the input.

Quoted 5 lines

Quoted 1 line
Jonathan, do you like to send a patch with your solution?
Err. not really :) Bit snowed under so whilst I'll probably get to it one day it might not be anytime soon.
Anyone else who happens to want to do it then feel free ;)

I'll send a patch within a day or two if no one beats me to it :)

Thanks,
Vlad

Vlad Dogaru

2014-11-20 12:00 UTC #8 lore raw

PATCH 1/1 iio: bmp280: refactor compensation code

This version of the code avoids extra memory copy operations and is somewhat smaller in code size.

Signed-off-by Vlad Dogaru <[email protected]>

---
 drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 88 deletions(-)

drivers/iio/pressure/bmp280.c +52 ~~-88~~

index 75038da..47dfd34 100644
@@ -80,16 +80,12 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
+/*
+ * These enums are used for indexing into the array of compensation
+ * parameters.
+ */
+enum { T1, T2, T3 };
+enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 
 static const struct iio_chan_spec bmp280_channels[] = {
 	{

@@ -141,54 +137,6 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
-			       buf, BMP280_COMP_TEMP_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read temperature calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_t1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_t2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_t3 = (s16) le16_to_cpu(buf[2]);
-
-	return 0;
-}
-
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
-			       buf, BMP280_COMP_PRESS_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read pressure calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_p1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_p2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_p3 = (s16) le16_to_cpu(buf[2]);
-	comp->dig_p4 = (s16) le16_to_cpu(buf[3]);
-	comp->dig_p5 = (s16) le16_to_cpu(buf[4]);
-	comp->dig_p6 = (s16) le16_to_cpu(buf[5]);
-	comp->dig_p7 = (s16) le16_to_cpu(buf[6]);
-	comp->dig_p8 = (s16) le16_to_cpu(buf[7]);
-	comp->dig_p9 = (s16) le16_to_cpu(buf[8]);
-
-	return 0;
-}
-
 /*
  * Returns temperature in DegC, resolution is 0.01 DegC.  Output value of
  * "5123" equals 51.23 DegC.  t_fine carries fine temperature as global

@@ -197,16 +145,33 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
+	int ret;
 	s32 var1, var2, t;
+	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
+			       buf, BMP280_COMP_TEMP_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read temperature calibration parameters\n");
+		return ret;
+	}
 
-	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
-		((s32) comp->dig_t2)) >> 11;
-	var2 = (((((adc_temp >> 4) - ((s32) comp->dig_t1)) *
-		  ((adc_temp >> 4) - ((s32) comp->dig_t1))) >> 12) *
-		((s32) comp->dig_t3)) >> 14;
+	/*
+	 * The double casts are necessary because le16_to_cpu returns an
+	 * unsigned 16-bit value.  Casting that value directly to a
+	 * signed 32-bit will not do proper sign extension.
+	 *
+	 * Conversely, T1 and P1 are unsigned values, so they can be
+	 * cast straight to the larger type.
+	 */
+	var1 = (((adc_temp >> 3) - ((s32)le16_to_cpu(buf[T1]) << 1)) *
+		((s32)(s16)le16_to_cpu(buf[T2]))) >> 11;
+	var2 = (((((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1]))) *
+		  ((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1])))) >> 12) *
+		((s32)(s16)le16_to_cpu(buf[T3]))) >> 14;
 
 	data->t_fine = var1 + var2;
 	t = (data->t_fine * 5 + 128) >> 8;

@@ -222,27 +187,36 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
 				   s32 adc_press)
 {
+	int ret;
 	s64 var1, var2, p;
+	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
+			       buf, BMP280_COMP_PRESS_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read pressure calibration parameters\n");
+		return ret;
+	}
 
-	var1 = ((s64) data->t_fine) - 128000;
-	var2 = var1 * var1 * (s64) comp->dig_p6;
-	var2 = var2 + ((var1 * (s64) comp->dig_p5) << 17);
-	var2 = var2 + (((s64) comp->dig_p4) << 35);
-	var1 = ((var1 * var1 * (s64) comp->dig_p3) >> 8) +
-		((var1 * (s64) comp->dig_p2) << 12);
-	var1 = (((((s64) 1) << 47) + var1)) * ((s64) comp->dig_p1) >> 33;
+	var1 = ((s64)data->t_fine) - 128000;
+	var2 = var1 * var1 * (s64)(s16)le16_to_cpu(buf[P6]);
+	var2 = var2 + ((var1 * (s64)(s16)le16_to_cpu(buf[P5])) << 17);
+	var2 = var2 + (((s64)(s16)le16_to_cpu(buf[P4])) << 35);
+	var1 = ((var1 * var1 * (s64)(s16)le16_to_cpu(buf[P3])) >> 8) +
+		((var1 * (s64)(s16)le16_to_cpu(buf[P2])) << 12);
+	var1 = ((((s64)1) << 47) + var1) * ((s64)le16_to_cpu(buf[P1])) >> 33;
 
 	if (var1 == 0)
 		return 0;
 
-	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
+	p = ((((s64)1048576 - adc_press) << 31) - var2) * 3125;
 	p = div64_s64(p, var1);
-	var1 = (((s64) comp->dig_p9) * (p >> 13) * (p >> 13)) >> 25;
-	var2 = (((s64) comp->dig_p8) * p) >> 19;
-	p = ((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
+	var1 = (((s64)(s16)le16_to_cpu(buf[P9])) * (p >> 13) * (p >> 13)) >> 25;
+	var2 = (((s64)(s16)le16_to_cpu(buf[P8])) * p) >> 19;
+	p = ((p + var1 + var2) >> 8) + (((s64)(s16)le16_to_cpu(buf[P7])) << 4);
 
 	return (u32) p;
 }

@@ -253,11 +227,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -267,7 +236,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -288,11 +257,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -307,7 +271,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

Signature · 1 line

1.9.1

Jonathan Cameron

maintainer

2014-11-22 12:05 UTC #9 lore raw

Re: [PATCH] iio: bmp280: refactor compensation code

Quoted 4 lines · Vlad Dogaru

This version of the code avoids extra memory copy operations and is somewhat smaller in code size.
Signed-off-by Vlad Dogaru <[email protected]>

Thanks for doing this - I'm happy with the result, but would like to let Hartmut have a few days to take a look if he wants to so will let it sit on the list for now.

Thanks,

Jonathan

Quoted 218 lines

---
 drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 88 deletions(-)

drivers/iio/pressure/bmp280.c +52 ~~-88~~

index 75038da..47dfd34 100644
@@ -80,16 +80,12 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
+/*
+ * These enums are used for indexing into the array of compensation
+ * parameters.
+ */
+enum { T1, T2, T3 };
+enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 
 static const struct iio_chan_spec bmp280_channels[] = {
 	{

@@ -141,54 +137,6 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
-			       buf, BMP280_COMP_TEMP_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read temperature calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_t1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_t2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_t3 = (s16) le16_to_cpu(buf[2]);
-
-	return 0;
-}
-
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
-			       buf, BMP280_COMP_PRESS_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read pressure calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_p1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_p2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_p3 = (s16) le16_to_cpu(buf[2]);
-	comp->dig_p4 = (s16) le16_to_cpu(buf[3]);
-	comp->dig_p5 = (s16) le16_to_cpu(buf[4]);
-	comp->dig_p6 = (s16) le16_to_cpu(buf[5]);
-	comp->dig_p7 = (s16) le16_to_cpu(buf[6]);
-	comp->dig_p8 = (s16) le16_to_cpu(buf[7]);
-	comp->dig_p9 = (s16) le16_to_cpu(buf[8]);
-
-	return 0;
-}
-
 /*
  * Returns temperature in DegC, resolution is 0.01 DegC.  Output value of
  * "5123" equals 51.23 DegC.  t_fine carries fine temperature as global

@@ -197,16 +145,33 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
+	int ret;
 	s32 var1, var2, t;
+	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
+			       buf, BMP280_COMP_TEMP_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read temperature calibration parameters\n");
+		return ret;
+	}
 
-	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
-		((s32) comp->dig_t2)) >> 11;
-	var2 = (((((adc_temp >> 4) - ((s32) comp->dig_t1)) *
-		  ((adc_temp >> 4) - ((s32) comp->dig_t1))) >> 12) *
-		((s32) comp->dig_t3)) >> 14;
+	/*
+	 * The double casts are necessary because le16_to_cpu returns an
+	 * unsigned 16-bit value.  Casting that value directly to a
+	 * signed 32-bit will not do proper sign extension.
+	 *
+	 * Conversely, T1 and P1 are unsigned values, so they can be
+	 * cast straight to the larger type.
+	 */
+	var1 = (((adc_temp >> 3) - ((s32)le16_to_cpu(buf[T1]) << 1)) *
+		((s32)(s16)le16_to_cpu(buf[T2]))) >> 11;
+	var2 = (((((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1]))) *
+		  ((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1])))) >> 12) *
+		((s32)(s16)le16_to_cpu(buf[T3]))) >> 14;
 
 	data->t_fine = var1 + var2;
 	t = (data->t_fine * 5 + 128) >> 8;

@@ -222,27 +187,36 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
 				   s32 adc_press)
 {
+	int ret;
 	s64 var1, var2, p;
+	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
+			       buf, BMP280_COMP_PRESS_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read pressure calibration parameters\n");
+		return ret;
+	}
 
-	var1 = ((s64) data->t_fine) - 128000;
-	var2 = var1 * var1 * (s64) comp->dig_p6;
-	var2 = var2 + ((var1 * (s64) comp->dig_p5) << 17);
-	var2 = var2 + (((s64) comp->dig_p4) << 35);
-	var1 = ((var1 * var1 * (s64) comp->dig_p3) >> 8) +
-		((var1 * (s64) comp->dig_p2) << 12);
-	var1 = (((((s64) 1) << 47) + var1)) * ((s64) comp->dig_p1) >> 33;
+	var1 = ((s64)data->t_fine) - 128000;
+	var2 = var1 * var1 * (s64)(s16)le16_to_cpu(buf[P6]);
+	var2 = var2 + ((var1 * (s64)(s16)le16_to_cpu(buf[P5])) << 17);
+	var2 = var2 + (((s64)(s16)le16_to_cpu(buf[P4])) << 35);
+	var1 = ((var1 * var1 * (s64)(s16)le16_to_cpu(buf[P3])) >> 8) +
+		((var1 * (s64)(s16)le16_to_cpu(buf[P2])) << 12);
+	var1 = ((((s64)1) << 47) + var1) * ((s64)le16_to_cpu(buf[P1])) >> 33;
 
 	if (var1 == 0)
 		return 0;
 
-	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
+	p = ((((s64)1048576 - adc_press) << 31) - var2) * 3125;
 	p = div64_s64(p, var1);
-	var1 = (((s64) comp->dig_p9) * (p >> 13) * (p >> 13)) >> 25;
-	var2 = (((s64) comp->dig_p8) * p) >> 19;
-	p = ((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
+	var1 = (((s64)(s16)le16_to_cpu(buf[P9])) * (p >> 13) * (p >> 13)) >> 25;
+	var2 = (((s64)(s16)le16_to_cpu(buf[P8])) * p) >> 19;
+	p = ((p + var1 + var2) >> 8) + (((s64)(s16)le16_to_cpu(buf[P7])) << 4);
 
 	return (u32) p;
 }

@@ -253,11 +227,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -267,7 +236,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -288,11 +257,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -307,7 +271,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

Hartmut Knaack

author Acked-by on v1 1/1

2014-11-22 20:27 UTC #11 lore raw

Re: [PATCH] iio: bmp280: refactor compensation code

In reply to Vlad Dogaru

Vlad Dogaru schrieb am 20.11.2014 13:00:

Quoted 4 lines

This version of the code avoids extra memory copy operations and is somewhat smaller in code size.
Signed-off-by Vlad Dogaru <[email protected]>

Acked-by Hartmut Knaack <[email protected]>

Quoted 218 lines

---
 drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 88 deletions(-)

drivers/iio/pressure/bmp280.c +52 ~~-88~~

index 75038da..47dfd34 100644
@@ -80,16 +80,12 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
+/*
+ * These enums are used for indexing into the array of compensation
+ * parameters.
+ */
+enum { T1, T2, T3 };
+enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 
 static const struct iio_chan_spec bmp280_channels[] = {
 	{

@@ -141,54 +137,6 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
-			       buf, BMP280_COMP_TEMP_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read temperature calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_t1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_t2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_t3 = (s16) le16_to_cpu(buf[2]);
-
-	return 0;
-}
-
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
-			       buf, BMP280_COMP_PRESS_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read pressure calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_p1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_p2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_p3 = (s16) le16_to_cpu(buf[2]);
-	comp->dig_p4 = (s16) le16_to_cpu(buf[3]);
-	comp->dig_p5 = (s16) le16_to_cpu(buf[4]);
-	comp->dig_p6 = (s16) le16_to_cpu(buf[5]);
-	comp->dig_p7 = (s16) le16_to_cpu(buf[6]);
-	comp->dig_p8 = (s16) le16_to_cpu(buf[7]);
-	comp->dig_p9 = (s16) le16_to_cpu(buf[8]);
-
-	return 0;
-}
-
 /*
  * Returns temperature in DegC, resolution is 0.01 DegC.  Output value of
  * "5123" equals 51.23 DegC.  t_fine carries fine temperature as global

@@ -197,16 +145,33 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
+	int ret;
 	s32 var1, var2, t;
+	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
+			       buf, BMP280_COMP_TEMP_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read temperature calibration parameters\n");
+		return ret;
+	}
 
-	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
-		((s32) comp->dig_t2)) >> 11;
-	var2 = (((((adc_temp >> 4) - ((s32) comp->dig_t1)) *
-		  ((adc_temp >> 4) - ((s32) comp->dig_t1))) >> 12) *
-		((s32) comp->dig_t3)) >> 14;
+	/*
+	 * The double casts are necessary because le16_to_cpu returns an
+	 * unsigned 16-bit value.  Casting that value directly to a
+	 * signed 32-bit will not do proper sign extension.
+	 *
+	 * Conversely, T1 and P1 are unsigned values, so they can be
+	 * cast straight to the larger type.
+	 */
+	var1 = (((adc_temp >> 3) - ((s32)le16_to_cpu(buf[T1]) << 1)) *
+		((s32)(s16)le16_to_cpu(buf[T2]))) >> 11;
+	var2 = (((((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1]))) *
+		  ((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1])))) >> 12) *
+		((s32)(s16)le16_to_cpu(buf[T3]))) >> 14;
 
 	data->t_fine = var1 + var2;
 	t = (data->t_fine * 5 + 128) >> 8;

@@ -222,27 +187,36 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
 				   s32 adc_press)
 {
+	int ret;
 	s64 var1, var2, p;
+	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
+			       buf, BMP280_COMP_PRESS_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read pressure calibration parameters\n");
+		return ret;
+	}
 
-	var1 = ((s64) data->t_fine) - 128000;
-	var2 = var1 * var1 * (s64) comp->dig_p6;
-	var2 = var2 + ((var1 * (s64) comp->dig_p5) << 17);
-	var2 = var2 + (((s64) comp->dig_p4) << 35);
-	var1 = ((var1 * var1 * (s64) comp->dig_p3) >> 8) +
-		((var1 * (s64) comp->dig_p2) << 12);
-	var1 = (((((s64) 1) << 47) + var1)) * ((s64) comp->dig_p1) >> 33;
+	var1 = ((s64)data->t_fine) - 128000;
+	var2 = var1 * var1 * (s64)(s16)le16_to_cpu(buf[P6]);
+	var2 = var2 + ((var1 * (s64)(s16)le16_to_cpu(buf[P5])) << 17);
+	var2 = var2 + (((s64)(s16)le16_to_cpu(buf[P4])) << 35);
+	var1 = ((var1 * var1 * (s64)(s16)le16_to_cpu(buf[P3])) >> 8) +
+		((var1 * (s64)(s16)le16_to_cpu(buf[P2])) << 12);
+	var1 = ((((s64)1) << 47) + var1) * ((s64)le16_to_cpu(buf[P1])) >> 33;
 
 	if (var1 == 0)
 		return 0;
 
-	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
+	p = ((((s64)1048576 - adc_press) << 31) - var2) * 3125;
 	p = div64_s64(p, var1);
-	var1 = (((s64) comp->dig_p9) * (p >> 13) * (p >> 13)) >> 25;
-	var2 = (((s64) comp->dig_p8) * p) >> 19;
-	p = ((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
+	var1 = (((s64)(s16)le16_to_cpu(buf[P9])) * (p >> 13) * (p >> 13)) >> 25;
+	var2 = (((s64)(s16)le16_to_cpu(buf[P8])) * p) >> 19;
+	p = ((p + var1 + var2) >> 8) + (((s64)(s16)le16_to_cpu(buf[P7])) << 4);
 
 	return (u32) p;
 }

@@ -253,11 +227,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -267,7 +236,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -288,11 +257,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -307,7 +271,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

Hartmut Knaack

author

2014-11-22 20:33 UTC #10 lore raw

Re: [PATCH] iio: bmp280: refactor compensation code

In reply to Jonathan Cameron

Jonathan Cameron schrieb am 22.11.2014 13:05:

Quoted 8 lines · Vlad Dogaru

Quoted 4 lines
This version of the code avoids extra memory copy operations and is somewhat smaller in code size.
Signed-off-by Vlad Dogaru <[email protected]>
Thanks for doing this - I'm happy with the result, but would like to let Hartmut have a few days to take a look if he wants to so will let it sit on the list for now.

Good solution. So, as soon as this one gets applied, I will rebase and resend my cleanup patch.

Quoted 227 lines

Thanks,

Jonathan

Quoted 210 lines

---
 drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 88 deletions(-)

drivers/iio/pressure/bmp280.c +51 ~~-87~~

index 75038da..47dfd34 100644
@@ -80,16 +80,12 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
+/*
+ * These enums are used for indexing into the array of compensation
+ * parameters.
+ */
+enum { T1, T2, T3 };
+enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 
 static const struct iio_chan_spec bmp280_channels[] = {
 	{

@@ -141,54 +137,6 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
-			       buf, BMP280_COMP_TEMP_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read temperature calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_t1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_t2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_t3 = (s16) le16_to_cpu(buf[2]);
-
-	return 0;
-}
-
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
-			       buf, BMP280_COMP_PRESS_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read pressure calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_p1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_p2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_p3 = (s16) le16_to_cpu(buf[2]);
-	comp->dig_p4 = (s16) le16_to_cpu(buf[3]);
-	comp->dig_p5 = (s16) le16_to_cpu(buf[4]);
-	comp->dig_p6 = (s16) le16_to_cpu(buf[5]);
-	comp->dig_p7 = (s16) le16_to_cpu(buf[6]);
-	comp->dig_p8 = (s16) le16_to_cpu(buf[7]);
-	comp->dig_p9 = (s16) le16_to_cpu(buf[8]);
-
-	return 0;
-}
-
 /*
  * Returns temperature in DegC, resolution is 0.01 DegC.  Output value of
  * "5123" equals 51.23 DegC.  t_fine carries fine temperature as global

@@ -197,16 +145,33 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
+	int ret;
 	s32 var1, var2, t;
+	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
+			       buf, BMP280_COMP_TEMP_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read temperature calibration parameters\n");
+		return ret;
+	}
 
-	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
-		((s32) comp->dig_t2)) >> 11;
-	var2 = (((((adc_temp >> 4) - ((s32) comp->dig_t1)) *
-		  ((adc_temp >> 4) - ((s32) comp->dig_t1))) >> 12) *
-		((s32) comp->dig_t3)) >> 14;
+	/*
+	 * The double casts are necessary because le16_to_cpu returns an
+	 * unsigned 16-bit value.  Casting that value directly to a
+	 * signed 32-bit will not do proper sign extension.
+	 *
+	 * Conversely, T1 and P1 are unsigned values, so they can be
+	 * cast straight to the larger type.
+	 */
+	var1 = (((adc_temp >> 3) - ((s32)le16_to_cpu(buf[T1]) << 1)) *
+		((s32)(s16)le16_to_cpu(buf[T2]))) >> 11;
+	var2 = (((((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1]))) *
+		  ((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1])))) >> 12) *
+		((s32)(s16)le16_to_cpu(buf[T3]))) >> 14;
 
 	data->t_fine = var1 + var2;
 	t = (data->t_fine * 5 + 128) >> 8;

@@ -222,27 +187,36 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
 				   s32 adc_press)
 {
+	int ret;
 	s64 var1, var2, p;
+	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
+			       buf, BMP280_COMP_PRESS_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read pressure calibration parameters\n");
+		return ret;
+	}
 
-	var1 = ((s64) data->t_fine) - 128000;
-	var2 = var1 * var1 * (s64) comp->dig_p6;
-	var2 = var2 + ((var1 * (s64) comp->dig_p5) << 17);
-	var2 = var2 + (((s64) comp->dig_p4) << 35);
-	var1 = ((var1 * var1 * (s64) comp->dig_p3) >> 8) +
-		((var1 * (s64) comp->dig_p2) << 12);
-	var1 = (((((s64) 1) << 47) + var1)) * ((s64) comp->dig_p1) >> 33;
+	var1 = ((s64)data->t_fine) - 128000;
+	var2 = var1 * var1 * (s64)(s16)le16_to_cpu(buf[P6]);
+	var2 = var2 + ((var1 * (s64)(s16)le16_to_cpu(buf[P5])) << 17);
+	var2 = var2 + (((s64)(s16)le16_to_cpu(buf[P4])) << 35);
+	var1 = ((var1 * var1 * (s64)(s16)le16_to_cpu(buf[P3])) >> 8) +
+		((var1 * (s64)(s16)le16_to_cpu(buf[P2])) << 12);
+	var1 = ((((s64)1) << 47) + var1) * ((s64)le16_to_cpu(buf[P1])) >> 33;
 
 	if (var1 == 0)
 		return 0;
 
-	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
+	p = ((((s64)1048576 - adc_press) << 31) - var2) * 3125;
 	p = div64_s64(p, var1);
-	var1 = (((s64) comp->dig_p9) * (p >> 13) * (p >> 13)) >> 25;
-	var2 = (((s64) comp->dig_p8) * p) >> 19;
-	p = ((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
+	var1 = (((s64)(s16)le16_to_cpu(buf[P9])) * (p >> 13) * (p >> 13)) >> 25;
+	var2 = (((s64)(s16)le16_to_cpu(buf[P8])) * p) >> 19;
+	p = ((p + var1 + var2) >> 8) + (((s64)(s16)le16_to_cpu(buf[P7])) << 4);
 
 	return (u32) p;
 }

@@ -253,11 +227,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -267,7 +236,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -288,11 +257,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

drivers/iio/pressure/bmp280.c static int bmp280_read_press(struct bmp280_data *data, · line 277

Quoted 8 lines

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

@@ -307,7 +271,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jonathan Cameron

maintainer Applied

2014-11-22 20:43 UTC #12 lore raw

Re: [PATCH] iio: bmp280: refactor compensation code

In reply to Hartmut Knaack

Quoted 6 lines · Hartmut Knaack

Vlad Dogaru schrieb am 20.11.2014 13:00:
Quoted 4 lines
This version of the code avoids extra memory copy operations and is somewhat smaller in code size.
Signed-off-by Vlad Dogaru <[email protected]>
Acked-by Hartmut Knaack <[email protected]>

Applied to the togreg branch of iio.git.

Quoted 219 lines

Quoted 218 lines

---
 drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 88 deletions(-)

drivers/iio/pressure/bmp280.c +52 ~~-88~~

index 75038da..47dfd34 100644
@@ -80,16 +80,12 @@ struct bmp280_data {
 	s32 t_fine;
 };
 
-/* Compensation parameters. */
-struct bmp280_comp_temp {
-	u16 dig_t1;
-	s16 dig_t2, dig_t3;
-};
-
-struct bmp280_comp_press {
-	u16 dig_p1;
-	s16 dig_p2, dig_p3, dig_p4, dig_p5, dig_p6, dig_p7, dig_p8, dig_p9;
-};
+/*
+ * These enums are used for indexing into the array of compensation
+ * parameters.
+ */
+enum { T1, T2, T3 };
+enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
 
 static const struct iio_chan_spec bmp280_channels[] = {
 	{

@@ -141,54 +137,6 @@ static const struct regmap_config bmp280_regmap_config = {
 	.volatile_reg = bmp280_is_volatile_reg,
 };
 
-static int bmp280_read_compensation_temp(struct bmp280_data *data,
-					 struct bmp280_comp_temp *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
-			       buf, BMP280_COMP_TEMP_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read temperature calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_t1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_t2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_t3 = (s16) le16_to_cpu(buf[2]);
-
-	return 0;
-}
-
-static int bmp280_read_compensation_press(struct bmp280_data *data,
-					  struct bmp280_comp_press *comp)
-{
-	int ret;
-	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
-
-	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
-			       buf, BMP280_COMP_PRESS_REG_COUNT);
-	if (ret < 0) {
-		dev_err(&data->client->dev,
-			"failed to read pressure calibration parameters\n");
-		return ret;
-	}
-
-	comp->dig_p1 = (u16) le16_to_cpu(buf[0]);
-	comp->dig_p2 = (s16) le16_to_cpu(buf[1]);
-	comp->dig_p3 = (s16) le16_to_cpu(buf[2]);
-	comp->dig_p4 = (s16) le16_to_cpu(buf[3]);
-	comp->dig_p5 = (s16) le16_to_cpu(buf[4]);
-	comp->dig_p6 = (s16) le16_to_cpu(buf[5]);
-	comp->dig_p7 = (s16) le16_to_cpu(buf[6]);
-	comp->dig_p8 = (s16) le16_to_cpu(buf[7]);
-	comp->dig_p9 = (s16) le16_to_cpu(buf[8]);
-
-	return 0;
-}
-
 /*
  * Returns temperature in DegC, resolution is 0.01 DegC.  Output value of
  * "5123" equals 51.23 DegC.  t_fine carries fine temperature as global

@@ -197,16 +145,33 @@ static int bmp280_read_compensation_press(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static s32 bmp280_compensate_temp(struct bmp280_data *data,
-				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
+	int ret;
 	s32 var1, var2, t;
+	__le16 buf[BMP280_COMP_TEMP_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
+			       buf, BMP280_COMP_TEMP_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read temperature calibration parameters\n");
+		return ret;
+	}
 
-	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
-		((s32) comp->dig_t2)) >> 11;
-	var2 = (((((adc_temp >> 4) - ((s32) comp->dig_t1)) *
-		  ((adc_temp >> 4) - ((s32) comp->dig_t1))) >> 12) *
-		((s32) comp->dig_t3)) >> 14;
+	/*
+	 * The double casts are necessary because le16_to_cpu returns an
+	 * unsigned 16-bit value.  Casting that value directly to a
+	 * signed 32-bit will not do proper sign extension.
+	 *
+	 * Conversely, T1 and P1 are unsigned values, so they can be
+	 * cast straight to the larger type.
+	 */
+	var1 = (((adc_temp >> 3) - ((s32)le16_to_cpu(buf[T1]) << 1)) *
+		((s32)(s16)le16_to_cpu(buf[T2]))) >> 11;
+	var2 = (((((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1]))) *
+		  ((adc_temp >> 4) - ((s32)le16_to_cpu(buf[T1])))) >> 12) *
+		((s32)(s16)le16_to_cpu(buf[T3]))) >> 14;
 
 	data->t_fine = var1 + var2;
 	t = (data->t_fine * 5 + 128) >> 8;

@@ -222,27 +187,36 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
  * Taken from datasheet, Section 3.11.3, "Compensation formula".
  */
 static u32 bmp280_compensate_press(struct bmp280_data *data,
-				   struct bmp280_comp_press *comp,
 				   s32 adc_press)
 {
+	int ret;
 	s64 var1, var2, p;
+	__le16 buf[BMP280_COMP_PRESS_REG_COUNT / 2];
+
+	ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_PRESS_START,
+			       buf, BMP280_COMP_PRESS_REG_COUNT);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"failed to read pressure calibration parameters\n");
+		return ret;
+	}
 
-	var1 = ((s64) data->t_fine) - 128000;
-	var2 = var1 * var1 * (s64) comp->dig_p6;
-	var2 = var2 + ((var1 * (s64) comp->dig_p5) << 17);
-	var2 = var2 + (((s64) comp->dig_p4) << 35);
-	var1 = ((var1 * var1 * (s64) comp->dig_p3) >> 8) +
-		((var1 * (s64) comp->dig_p2) << 12);
-	var1 = (((((s64) 1) << 47) + var1)) * ((s64) comp->dig_p1) >> 33;
+	var1 = ((s64)data->t_fine) - 128000;
+	var2 = var1 * var1 * (s64)(s16)le16_to_cpu(buf[P6]);
+	var2 = var2 + ((var1 * (s64)(s16)le16_to_cpu(buf[P5])) << 17);
+	var2 = var2 + (((s64)(s16)le16_to_cpu(buf[P4])) << 35);
+	var1 = ((var1 * var1 * (s64)(s16)le16_to_cpu(buf[P3])) >> 8) +
+		((var1 * (s64)(s16)le16_to_cpu(buf[P2])) << 12);
+	var1 = ((((s64)1) << 47) + var1) * ((s64)le16_to_cpu(buf[P1])) >> 33;
 
 	if (var1 == 0)
 		return 0;
 
-	p = ((((s64) 1048576 - adc_press) << 31) - var2) * 3125;
+	p = ((((s64)1048576 - adc_press) << 31) - var2) * 3125;
 	p = div64_s64(p, var1);
-	var1 = (((s64) comp->dig_p9) * (p >> 13) * (p >> 13)) >> 25;
-	var2 = (((s64) comp->dig_p8) * p) >> 19;
-	p = ((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
+	var1 = (((s64)(s16)le16_to_cpu(buf[P9])) * (p >> 13) * (p >> 13)) >> 25;
+	var2 = (((s64)(s16)le16_to_cpu(buf[P8])) * p) >> 19;
+	p = ((p + var1 + var2) >> 8) + (((s64)(s16)le16_to_cpu(buf[P7])) << 4);
 
 	return (u32) p;
 }

@@ -253,11 +227,6 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	int ret;
 	__be32 tmp = 0;
 	s32 adc_temp, comp_temp;
-	struct bmp280_comp_temp comp;
-
-	ret = bmp280_read_compensation_temp(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
 			       (u8 *) &tmp, 3);

@@ -267,7 +236,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
 	}
 
 	adc_temp = be32_to_cpu(tmp) >> 12;
-	comp_temp = bmp280_compensate_temp(data, &comp, adc_temp);
+	comp_temp = bmp280_compensate_temp(data, adc_temp);
 
 	/*
 	 * val might be NULL if we're called by the read_press routine,

@@ -288,11 +257,6 @@ static int bmp280_read_press(struct bmp280_data *data,
 	__be32 tmp = 0;
 	s32 adc_press;
 	u32 comp_press;
-	struct bmp280_comp_press comp;
-
-	ret = bmp280_read_compensation_press(data, &comp);
-	if (ret < 0)
-		return ret;
 
 	/* Read and compensate temperature so we get a reading of t_fine. */
 	ret = bmp280_read_temp(data, NULL);

@@ -307,7 +271,7 @@ static int bmp280_read_press(struct bmp280_data *data,
 	}
 
 	adc_press = be32_to_cpu(tmp) >> 12;
-	comp_press = bmp280_compensate_press(data, &comp, adc_press);
+	comp_press = bmp280_compensate_press(data, adc_press);
 
 	*val = comp_press;
 	*val2 = 256000;