* [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
@ 2014-10-31 1:25 Hartmut Knaack
2014-10-31 11:47 ` Vlad Dogaru
0 siblings, 1 reply; 13+ messages in thread
From: Hartmut Knaack @ 2014-10-31 1:25 UTC (permalink / raw)
To: IIO; +Cc: vlad.dogaru
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 <knaack.h@gmx.de>
---
diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
index 4f6ae4d..36e425c 100644
--- a/drivers/iio/pressure/bmp280.c
+++ b/drivers/iio/pressure/bmp280.c
@@ -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;
}
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
2014-10-31 1:25 [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init Hartmut Knaack
@ 2014-10-31 11:47 ` Vlad Dogaru
2014-10-31 19:16 ` Hartmut Knaack
0 siblings, 1 reply; 13+ messages in thread
From: Vlad Dogaru @ 2014-10-31 11:47 UTC (permalink / raw)
To: Hartmut Knaack; +Cc: IIO
On Fri, Oct 31, 2014 at 02:25:22AM +0100, Hartmut Knaack wrote:
> 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
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
> index 4f6ae4d..36e425c 100644
> --- a/drivers/iio/pressure/bmp280.c
> +++ b/drivers/iio/pressure/bmp280.c
> @@ -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;
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
2014-10-31 11:47 ` Vlad Dogaru
@ 2014-10-31 19:16 ` Hartmut Knaack
2014-11-05 15:53 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Hartmut Knaack @ 2014-10-31 19:16 UTC (permalink / raw)
To: Vlad Dogaru; +Cc: IIO
Vlad Dogaru schrieb am 31.10.2014 12:47:
> On Fri, Oct 31, 2014 at 02:25:22AM +0100, Hartmut Knaack wrote:
>> 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.
> [1] http://www.spinics.net/lists/linux-iio/msg15099.html
>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> ---
>> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
>> index 4f6ae4d..36e425c 100644
>> --- a/drivers/iio/pressure/bmp280.c
>> +++ b/drivers/iio/pressure/bmp280.c
>> @@ -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;
>> }
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
2014-10-31 19:16 ` Hartmut Knaack
@ 2014-11-05 15:53 ` Jonathan Cameron
2014-11-06 13:02 ` Vlad Dogaru
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2014-11-05 15:53 UTC (permalink / raw)
To: Hartmut Knaack, Vlad Dogaru; +Cc: IIO
On 31/10/14 19:16, Hartmut Knaack wrote:
> Vlad Dogaru schrieb am 31.10.2014 12:47:
>> On Fri, Oct 31, 2014 at 02:25:22AM +0100, Hartmut Knaack wrote:
>>> 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.
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!
>> [1] http://www.spinics.net/lists/linux-iio/msg15099.html
>>
>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>> ---
>>> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
>>> index 4f6ae4d..36e425c 100644
>>> --- a/drivers/iio/pressure/bmp280.c
>>> +++ b/drivers/iio/pressure/bmp280.c
>>> @@ -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;
>>> }
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
2014-11-05 15:53 ` Jonathan Cameron
@ 2014-11-06 13:02 ` Vlad Dogaru
2014-11-10 23:11 ` Hartmut Knaack
0 siblings, 1 reply; 13+ messages in thread
From: Vlad Dogaru @ 2014-11-06 13:02 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Hartmut Knaack, IIO
On Wed, Nov 05, 2014 at 03:53:20PM +0000, Jonathan Cameron wrote:
> On 31/10/14 19:16, Hartmut Knaack wrote:
> > Vlad Dogaru schrieb am 31.10.2014 12:47:
> >> On Fri, Oct 31, 2014 at 02:25:22AM +0100, Hartmut Knaack wrote:
> >>> 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.
> 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!
> >> [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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
2014-11-06 13:02 ` Vlad Dogaru
@ 2014-11-10 23:11 ` Hartmut Knaack
2014-11-15 16:06 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Hartmut Knaack @ 2014-11-10 23:11 UTC (permalink / raw)
To: Vlad Dogaru, Jonathan Cameron; +Cc: IIO
Vlad Dogaru schrieb am 06.11.2014 14:02:
> On Wed, Nov 05, 2014 at 03:53:20PM +0000, Jonathan Cameron wrote:
>> On 31/10/14 19:16, Hartmut Knaack wrote:
>>> Vlad Dogaru schrieb am 31.10.2014 12:47:
>>>> On Fri, Oct 31, 2014 at 02:25:22AM +0100, Hartmut Knaack wrote:
>>>>> 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.
>> 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!
>>>> [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
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?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
2014-11-10 23:11 ` Hartmut Knaack
@ 2014-11-15 16:06 ` Jonathan Cameron
2014-11-17 15:16 ` Vlad Dogaru
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2014-11-15 16:06 UTC (permalink / raw)
To: Hartmut Knaack, Vlad Dogaru; +Cc: IIO
On 10/11/14 23:11, Hartmut Knaack wrote:
> Vlad Dogaru schrieb am 06.11.2014 14:02:
>> On Wed, Nov 05, 2014 at 03:53:20PM +0000, Jonathan Cameron wrote:
>>> On 31/10/14 19:16, Hartmut Knaack wrote:
>>>> Vlad Dogaru schrieb am 31.10.2014 12:47:
>>>>> On Fri, Oct 31, 2014 at 02:25:22AM +0100, Hartmut Knaack wrote:
>>>>>> 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.
>>> 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!
>>>>> [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
> 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 ;)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init
2014-11-15 16:06 ` Jonathan Cameron
@ 2014-11-17 15:16 ` Vlad Dogaru
2014-11-20 12:00 ` [PATCH] iio: bmp280: refactor compensation code Vlad Dogaru
0 siblings, 1 reply; 13+ messages in thread
From: Vlad Dogaru @ 2014-11-17 15:16 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Hartmut Knaack, IIO
On Sat, Nov 15, 2014 at 04:06:37PM +0000, Jonathan Cameron wrote:
> On 10/11/14 23:11, Hartmut Knaack wrote:
> > Vlad Dogaru schrieb am 06.11.2014 14:02:
> >> On Wed, Nov 05, 2014 at 03:53:20PM +0000, Jonathan Cameron wrote:
> >>> On 31/10/14 19:16, Hartmut Knaack wrote:
> >>>> Vlad Dogaru schrieb am 31.10.2014 12:47:
> >>>>> On Fri, Oct 31, 2014 at 02:25:22AM +0100, Hartmut Knaack wrote:
> >>>>>> 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.
> >>> 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!
> >>>>> [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
> > 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.
> > 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] iio: bmp280: refactor compensation code
2014-11-17 15:16 ` Vlad Dogaru
@ 2014-11-20 12:00 ` Vlad Dogaru
2014-11-22 12:05 ` Jonathan Cameron
2014-11-22 20:27 ` Hartmut Knaack
0 siblings, 2 replies; 13+ messages in thread
From: Vlad Dogaru @ 2014-11-20 12:00 UTC (permalink / raw)
To: linux-iio, jic23, knaack.h; +Cc: Vlad Dogaru
This version of the code avoids extra memory copy operations and is
somewhat smaller in code size.
Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
---
drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
1 file changed, 52 insertions(+), 88 deletions(-)
diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
index 75038da..47dfd34 100644
--- a/drivers/iio/pressure/bmp280.c
+++ b/drivers/iio/pressure/bmp280.c
@@ -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;
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: bmp280: refactor compensation code
2014-11-20 12:00 ` [PATCH] iio: bmp280: refactor compensation code Vlad Dogaru
@ 2014-11-22 12:05 ` Jonathan Cameron
2014-11-22 20:33 ` Hartmut Knaack
2014-11-22 20:27 ` Hartmut Knaack
1 sibling, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2014-11-22 12:05 UTC (permalink / raw)
To: Vlad Dogaru, linux-iio, knaack.h
On 20/11/14 12:00, Vlad Dogaru wrote:
> This version of the code avoids extra memory copy operations and is
> somewhat smaller in code size.
>
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
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
> ---
> drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
> 1 file changed, 52 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
> index 75038da..47dfd34 100644
> --- a/drivers/iio/pressure/bmp280.c
> +++ b/drivers/iio/pressure/bmp280.c
> @@ -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;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: bmp280: refactor compensation code
2014-11-20 12:00 ` [PATCH] iio: bmp280: refactor compensation code Vlad Dogaru
2014-11-22 12:05 ` Jonathan Cameron
@ 2014-11-22 20:27 ` Hartmut Knaack
2014-11-22 20:43 ` Jonathan Cameron
1 sibling, 1 reply; 13+ messages in thread
From: Hartmut Knaack @ 2014-11-22 20:27 UTC (permalink / raw)
To: Vlad Dogaru, linux-iio, jic23
Vlad Dogaru schrieb am 20.11.2014 13:00:
> This version of the code avoids extra memory copy operations and is
> somewhat smaller in code size.
>
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
Acked-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
> drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
> 1 file changed, 52 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
> index 75038da..47dfd34 100644
> --- a/drivers/iio/pressure/bmp280.c
> +++ b/drivers/iio/pressure/bmp280.c
> @@ -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;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: bmp280: refactor compensation code
2014-11-22 12:05 ` Jonathan Cameron
@ 2014-11-22 20:33 ` Hartmut Knaack
0 siblings, 0 replies; 13+ messages in thread
From: Hartmut Knaack @ 2014-11-22 20:33 UTC (permalink / raw)
To: Jonathan Cameron, Vlad Dogaru, linux-iio
Jonathan Cameron schrieb am 22.11.2014 13:05:
> On 20/11/14 12:00, Vlad Dogaru wrote:
>> This version of the code avoids extra memory copy operations and is
>> somewhat smaller in code size.
>>
>> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
> 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.
>
> Thanks,
>
> Jonathan
>> ---
>> drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
>> 1 file changed, 52 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
>> index 75038da..47dfd34 100644
>> --- a/drivers/iio/pressure/bmp280.c
>> +++ b/drivers/iio/pressure/bmp280.c
>> @@ -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;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: bmp280: refactor compensation code
2014-11-22 20:27 ` Hartmut Knaack
@ 2014-11-22 20:43 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2014-11-22 20:43 UTC (permalink / raw)
To: Hartmut Knaack, Vlad Dogaru, linux-iio
On 22/11/14 20:27, Hartmut Knaack wrote:
> Vlad Dogaru schrieb am 20.11.2014 13:00:
>> This version of the code avoids extra memory copy operations and is
>> somewhat smaller in code size.
>>
>> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
> Acked-by: Hartmut Knaack <knaack.h@gmx.de>
Applied to the togreg branch of iio.git.
>> ---
>> drivers/iio/pressure/bmp280.c | 140 ++++++++++++++++--------------------------
>> 1 file changed, 52 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
>> index 75038da..47dfd34 100644
>> --- a/drivers/iio/pressure/bmp280.c
>> +++ b/drivers/iio/pressure/bmp280.c
>> @@ -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;
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-11-22 20:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-31 1:25 [PATCH 3/3]iio:pressure:bmp280: read compensation data only at init Hartmut Knaack
2014-10-31 11:47 ` Vlad Dogaru
2014-10-31 19:16 ` Hartmut Knaack
2014-11-05 15:53 ` Jonathan Cameron
2014-11-06 13:02 ` Vlad Dogaru
2014-11-10 23:11 ` Hartmut Knaack
2014-11-15 16:06 ` Jonathan Cameron
2014-11-17 15:16 ` Vlad Dogaru
2014-11-20 12:00 ` [PATCH] iio: bmp280: refactor compensation code Vlad Dogaru
2014-11-22 12:05 ` Jonathan Cameron
2014-11-22 20:33 ` Hartmut Knaack
2014-11-22 20:27 ` Hartmut Knaack
2014-11-22 20:43 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).