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

[Vlad Dogaru <vlad.dogaru@intel.com>]

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