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
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
