iio:pressure:bmp280: cleanup

PATCH In review Hartmut Knaack 3 participants 5 messages Oct 31, 2014 – Nov 6, 2014 IIO

Share & tools

Hartmut Knaack

author

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

PATCH 2/3 iio:pressure:bmp280: cleanup

The calculations for temperature and pressure compensation were already slightly optimized in comparison to the data sheet. So, it makes sense to optimize even a bit more, making proper use of C operators:
- variable t in bmp280_compensate_temp() can be eliminated by directly returning the result of the relevant equation.
- make use of the += operator, eliminate an unnecessary parenthesis level and directly return the result of the last equation in bmp280_compensate_press().
When the initialization of the ctrl_meas register fails, the error message will now mention the right register name.
During probe, i2c_set_clientdata() is called, although it is not necessary. Drop it.

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

---

drivers/iio/pressure/bmp280.c +7 ~~-12~~

index 75038da..4f6ae4d 100644
@@ -200,7 +200,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
-	s32 var1, var2, t;
+	s32 var1, var2;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;

@@ -209,9 +209,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 		((s32) comp->dig_t3)) >> 14;
 
 	data->t_fine = var1 + var2;
-	t = (data->t_fine * 5 + 128) >> 8;
-
-	return t;
+	return (data->t_fine * 5 + 128) >> 8;
 }
 
 /*

@@ -229,11 +227,11 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 
 	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);
+	var2 += ((var1 * (s64) comp->dig_p5) << 17);
+	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) 1) << 47) + var1) * ((s64) comp->dig_p1) >> 33;
 
 	if (var1 == 0)
 		return 0;

@@ -242,9 +240,7 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 	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);
-
-	return (u32) p;
+	return (u32)((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
 }
 
 static int bmp280_read_temp(struct bmp280_data *data,

@@ -366,7 +362,7 @@ static int bmp280_chip_init(struct bmp280_data *data)
 				 BMP280_MODE_NORMAL);
 	if (ret < 0) {
 		dev_err(&data->client->dev,
-			"failed to write config register\n");
+			"failed to write ctrl_meas register\n");
 		return ret;
 	}

@@ -394,7 +390,6 @@ static int bmp280_probe(struct i2c_client *client,
 	if (!indio_dev)
 		return -ENOMEM;
 
-	i2c_set_clientdata(client, indio_dev);
 	data = iio_priv(indio_dev);
 	mutex_init(&data->lock);
 	data->client = client;

Vlad Dogaru

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

Quoted 29 lines · Hartmut Knaack

The calculations for temperature and pressure compensation were already slightly optimized in comparison to the data sheet. So, it makes sense to optimize even a bit more, making proper use of C operators:
- variable t in bmp280_compensate_temp() can be eliminated by directly returning the result of the relevant equation.
- make use of the += operator, eliminate an unnecessary parenthesis level and directly return the result of the last equation in bmp280_compensate_press().
When the initialization of the ctrl_meas register fails, the error message will now mention the right register name.
During probe, i2c_set_clientdata() is called, although it is not necessary. Drop it.
Signed-off-by Hartmut Knaack <[email protected]>
---
drivers/iio/pressure/bmp280.c +1 -1
index 75038da..4f6ae4d 100644
@@ -200,7 +200,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
-	s32 var1, var2, t;
+	s32 var1, var2;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;

drivers/iio/pressure/bmp280.c static s32 bmp280_compensate_temp(struct bmp280_data *data, · line 212

Quoted 7 lines

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

@@ -209,9 +209,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 		((s32) comp->dig_t3)) >> 14;
 
 	data->t_fine = var1 + var2;
-	t = (data->t_fine * 5 + 128) >> 8;
-
-	return t;
+	return (data->t_fine * 5 + 128) >> 8;

Shouldn't the compiler take care of this?

Quoted 19 lines

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

 }
 
 /*

@@ -229,11 +227,11 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 
 	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);
+	var2 += ((var1 * (s64) comp->dig_p5) << 17);
+	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) 1) << 47) + var1) * ((s64) comp->dig_p1) >> 33;
 
 	if (var1 == 0)
 		return 0;

drivers/iio/pressure/bmp280.c static u32 bmp280_compensate_press(struct bmp280_data *data, · line 243

Quoted 7 lines

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

@@ -242,9 +240,7 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 	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);
-
-	return (u32) p;
+	return (u32)((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);

And this?

Hartmut Knaack

author

2014-10-31 18:43 UTC #2 lore raw

Vlad Dogaru schrieb am 31.10.2014 12:44:

Quoted 32 lines · Hartmut Knaack

Quoted 31 lines
The calculations for temperature and pressure compensation were already slightly optimized in comparison to the data sheet. So, it makes sense to optimize even a bit more, making proper use of C operators:
- variable t in bmp280_compensate_temp() can be eliminated by directly returning the result of the relevant equation.
- make use of the += operator, eliminate an unnecessary parenthesis level and directly return the result of the last equation in bmp280_compensate_press().
When the initialization of the ctrl_meas register fails, the error message will now mention the right register name.
During probe, i2c_set_clientdata() is called, although it is not necessary. Drop it.
Signed-off-by Hartmut Knaack <[email protected]>
---
drivers/iio/pressure/bmp280.c +1 -1
index 75038da..4f6ae4d 100644
@@ -200,7 +200,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
-	s32 var1, var2, t;
+	s32 var1, var2;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;
@@ -209,9 +209,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 		((s32) comp->dig_t3)) >> 14;
 

Quoted 7 lines

drivers/iio/pressure/bmp280.c static s32 bmp280_compensate_temp(struct bmp280_data *data, · line 212
Quoted 5 lines
drivers/iio/pressure/bmp280.c +1 -3
 	data->t_fine = var1 + var2;
-	t = (data->t_fine * 5 + 128) >> 8;
-
-	return t;
+	return (data->t_fine * 5 + 128) >> 8;
Shouldn't the compiler take care of this?

That would be preferable. I just don't see the real benefit in having the extra step of storing the result (and taking care of an extra variable) before returning it. And I am aware, that this calculation is derived from the one in the data sheet (which looks a bit questionable to me with its unnecessary parenthesis and variable). But since you already started optimizing, it seemed legitimate to consolidate it even a bit more.

Quoted 34 lines

Quoted 20 lines

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

 }
 
 /*

@@ -229,11 +227,11 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 
 	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);
+	var2 += ((var1 * (s64) comp->dig_p5) << 17);
+	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) 1) << 47) + var1) * ((s64) comp->dig_p1) >> 33;
 
 	if (var1 == 0)
 		return 0;

drivers/iio/pressure/bmp280.c static u32 bmp280_compensate_press(struct bmp280_data *data, · line 243

Quoted 7 lines

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

@@ -242,9 +240,7 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 	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);
-
-	return (u32) p;
+	return (u32)((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);

And this?
--
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:55 UTC #3 lore raw

Quoted 33 lines · Hartmut Knaack

Vlad Dogaru schrieb am 31.10.2014 12:44:
Quoted 32 lines · Hartmut Knaack
Quoted 31 lines
The calculations for temperature and pressure compensation were already slightly optimized in comparison to the data sheet. So, it makes sense to optimize even a bit more, making proper use of C operators:
- variable t in bmp280_compensate_temp() can be eliminated by directly returning the result of the relevant equation.
- make use of the += operator, eliminate an unnecessary parenthesis level and directly return the result of the last equation in bmp280_compensate_press().
When the initialization of the ctrl_meas register fails, the error message will now mention the right register name.
During probe, i2c_set_clientdata() is called, although it is not necessary. Drop it.
Signed-off-by Hartmut Knaack <[email protected]>
---
drivers/iio/pressure/bmp280.c +1 -1
index 75038da..4f6ae4d 100644
@@ -200,7 +200,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
-	s32 var1, var2, t;
+	s32 var1, var2;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;
@@ -209,9 +209,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 		((s32) comp->dig_t3)) >> 14;
 

Quoted 8 lines

Quoted 7 lines
drivers/iio/pressure/bmp280.c static s32 bmp280_compensate_temp(struct bmp280_data *data, · line 212
Quoted 5 lines
drivers/iio/pressure/bmp280.c +1 -3
 	data->t_fine = var1 + var2;
-	t = (data->t_fine * 5 + 128) >> 8;
-
-	return t;
+	return (data->t_fine * 5 + 128) >> 8;
Shouldn't the compiler take care of this?
That would be preferable. I just don't see the real benefit in having the extra step of storing the result (and taking care of an extra variable) before returning it. And I am aware, that this calculation is derived from the one in the data sheet (which looks a bit questionable to me with its unnecessary parenthesis and variable). But since you already started optimizing, it seemed legitimate to consolidate it even a bit more.

I'm with Hartmut on this, no point in having more actual code / local variables than needed... Just a few more lines of code for no gain :)

Quoted 40 lines

Quoted 17 lines
Quoted 17 lines
drivers/iio/pressure/bmp280.c +3 -3
 }
 
 /*
@@ -229,11 +227,11 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 
 	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);
+	var2 += ((var1 * (s64) comp->dig_p5) << 17);
+	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) 1) << 47) + var1) * ((s64) comp->dig_p1) >> 33;
 
Quoted 17 lines
drivers/iio/pressure/bmp280.c static u32 bmp280_compensate_press(struct bmp280_data *data, · line 243
Quoted 10 lines
drivers/iio/pressure/bmp280.c +1 -3
 	if (var1 == 0)
 		return 0;
@@ -242,9 +240,7 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 	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);
-
-	return (u32) p;
+	return (u32)((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
And this?
--
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

Tested-by

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

Quoted 41 lines · Jonathan Cameron

Quoted 40 lines · Hartmut Knaack
Vlad Dogaru schrieb am 31.10.2014 12:44:
Quoted 39 lines · Hartmut Knaack
Quoted 29 lines
The calculations for temperature and pressure compensation were already slightly optimized in comparison to the data sheet. So, it makes sense to optimize even a bit more, making proper use of C operators:
- variable t in bmp280_compensate_temp() can be eliminated by directly returning the result of the relevant equation.
- make use of the += operator, eliminate an unnecessary parenthesis level and directly return the result of the last equation in bmp280_compensate_press().
When the initialization of the ctrl_meas register fails, the error message will now mention the right register name.
During probe, i2c_set_clientdata() is called, although it is not necessary. Drop it.
Signed-off-by Hartmut Knaack <[email protected]>
---
drivers/iio/pressure/bmp280.c +1 -1
index 75038da..4f6ae4d 100644
@@ -200,7 +200,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 				  struct bmp280_comp_temp *comp,
 				  s32 adc_temp)
 {
-	s32 var1, var2, t;
+	s32 var1, var2;
 
 	var1 = (((adc_temp >> 3) - ((s32) comp->dig_t1 << 1)) *
 		((s32) comp->dig_t2)) >> 11;
drivers/iio/pressure/bmp280.c static s32 bmp280_compensate_temp(struct bmp280_data *data, · line 212
Quoted 7 lines
drivers/iio/pressure/bmp280.c +1 -3
@@ -209,9 +209,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
 		((s32) comp->dig_t3)) >> 14;
 
 	data->t_fine = var1 + var2;
-	t = (data->t_fine * 5 + 128) >> 8;
-
-	return t;
+	return (data->t_fine * 5 + 128) >> 8;
Shouldn't the compiler take care of this?

Quoted 5 lines

Quoted 1 line
That would be preferable. I just don't see the real benefit in having the extra step of storing the result (and taking care of an extra variable) before returning it. And I am aware, that this calculation is derived from the one in the data sheet (which looks a bit questionable to me with its unnecessary parenthesis and variable). But since you already started optimizing, it seemed legitimate to consolidate it even a bit more.
I'm with Hartmut on this, no point in having more actual code / local variables than needed... Just a few more lines of code for no gain :)

I guess the problem is I started with the exact code from the datasheet, then refactored a bit to accomodate the usage of div64_s64 below. Code does look cleaner now, thanks Hartmut!

Tested-by Vlad Dogaru <[email protected]>

Quoted 40 lines

Quoted 40 lines
Quoted 17 lines
Quoted 17 lines
drivers/iio/pressure/bmp280.c +3 -3
 }
 
 /*
@@ -229,11 +227,11 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 
 	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);
+	var2 += ((var1 * (s64) comp->dig_p5) << 17);
+	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) 1) << 47) + var1) * ((s64) comp->dig_p1) >> 33;
 
Quoted 17 lines
drivers/iio/pressure/bmp280.c static u32 bmp280_compensate_press(struct bmp280_data *data, · line 243
Quoted 10 lines
drivers/iio/pressure/bmp280.c +1 -3
 	if (var1 == 0)
 		return 0;
@@ -242,9 +240,7 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
 	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);
-
-	return (u32) p;
+	return (u32)((p + var1 + var2) >> 8) + (((s64) comp->dig_p7) << 4);
And this?
--
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