Received-SPF: pass (google.com: domain of linux-kernel+bounces-103760-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Vasileios Amoiridis <vassilisamir@gmail.com>
Date: Thu, 14 Mar 2024 21:17:18 +0100
To: Jonathan Cameron <jic23@kernel.org>
Cc: Vasileios Amoiridis <vassilisamir@gmail.com>, lars@metafoo.de,
	andriy.shevchenko@linux.intel.com, ang.iglesiasg@gmail.com,
	mazziesaccount@gmail.com, ak@it-klinger.de,
	petre.rodan@subdimension.ro, linus.walleij@linaro.org,
	phil@raspberrypi.com, 579lpy@gmail.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/6] iio: pressure: Simplify and make more clear
 temperature readings
Message-ID: <20240314201718.GD1964894@vamoiridPC>
References: <20240313174007.1934983-1-vassilisamir@gmail.com>
 <20240313174007.1934983-5-vassilisamir@gmail.com>
 <20240314150959.585367b5@jic23-huawei>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240314150959.585367b5@jic23-huawei>

On Thu, Mar 14, 2024 at 03:09:59PM +0000, Jonathan Cameron wrote:
> On Wed, 13 Mar 2024 18:40:05 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The read_press/read_humid functions need the updated t_fine value
> > in order to calculate the current pressure/humidity. Temperature
> > reads should be removed from the read_press/read_humid functions
> > and should be placed in the oneshot captures before the pressure
> > and humidity reads. This makes the code more intuitive.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> To me this makes the use of these calls less obvious than they were
> previously.  The calls are made close to where t_fine is used and
> don't have to go via the indirection of chip_info.
> 
> So I disagree. I think this change makes the code a lot less
> clear.
> 

This was mainly driven by the fact that I wanted to avoid reading
the temperature 3 times in case temp, press and humid are enabled
and there are consecutive buffer readings. But thank you for the
proposal I really appreciate it!

Best regards,
Vasilis

> The only improvement I can readily see would be to move the
> temperature read into the compensation functions themselves, possibly
> removing t_fine from data and having a function that reads everything
> relevant to computing it directly but doesn't do the maths to get
> a temperature reading.  That can be reused in bmp280_compensate_temp()
> 
> Something along lines of.
> 
> static s32 bmp280_calc_tfine(struct bmp280_calib *calib, s32 adc_temp) 
> {
> 	s32 var1, var2;
> 
> 	var1 = (((adc_temp >> 3) - ((s32)calib->T1 << 1)) *
> 		((s32)calib->T2)) >> 11;
> 	var2 = (((((adc_temp >> 4) - ((s32)calib->T1)) *
> 		  ((adc_temp >> 4) - ((s32)calib->T1))) >> 12) *
> 		((s32)calib->T3)) >> 14;
> 	return var1 + var2;
> }
> 
> static int bmp280_read_temp_raw(struct bmp280_data *data,
> 			    	s32 *raw)
> {
> 	s32 adc_temp;
> 	int ret;
> 
> 	ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> 			       data->buf, sizeof(data->buf));
> 	if (ret < 0) {
> 		dev_err(data->dev, "failed to read temperature\n");
> 		return ret;
> 	}
> 
> 	adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(data->buf));
> 	if (adc_temp == BMP280_TEMP_SKIPPED) {
> 		/* reading was skipped */
> 		dev_err(data->dev, "reading temperature skipped\n");
> 		return -EIO;
> 	}
> 	*raw = adc_temp;
> 
> 	return 0;
> }
> static int bmp280_get_t_fine(.., s32 *t_fine)
> {
> 	s32 adc_temp, comp_temp;
> 	s32 t_fine;
> 	int ret;
> 
> 	ret = bmp280_read_temp_raw(data, &adc_temp;
> 	if (ret)
> 		return ret;
> 
> 	*t_fine = bmp280_calc_tfine(&data->calib.bmp280, adc_temp);
> 	return 0;
> }
> 
> static int bmp280_read_temp(struct bmp280_data *data, s32 *temp)
> {
> 	int ret;
> 	s32 t_fine;
> 
> 	ret = bmp280_get_t_fine(data, &t_fine);
> 	if (ret)
> 		return ret;
> 
> 	*temp = (t_fine * 5 + 128) / 256;
> //division rather than shift as then it's obvious what the 128 is there for
> 	return 0;
> }
> 
> Now you have a nice function to get you t_fine which is all you want in some
> of these paths.  Call it directly where it is needed instead of as
> a side effect of a temperature read.
> 
> 
> 
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 38 ++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 6d7734f867bc..377e90d9e5a2 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -404,11 +404,6 @@ static u32 bmp280_read_press(struct bmp280_data *data)
> >  	s32 adc_press;
> >  	int ret;
> >  
> > -	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp280_read_temp(data);
> > -	if (ret < 0)
> > -		return ret;
> > -
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> >  			       data->buf, sizeof(data->buf));
> >  	if (ret < 0) {
> > @@ -433,11 +428,6 @@ static u32 bmp280_read_humid(struct bmp280_data *data)
> >  	s32 adc_humidity;
> >  	int ret;
> >  
> > -	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp280_read_temp(data);
> > -	if (ret < 0)
> > -		return ret;
> > -
> >  	ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> >  			       &data->be16, sizeof(data->be16));
> >  	if (ret < 0) {
> > @@ -470,12 +460,21 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_PROCESSED:
> >  		switch (chan->type) {
> >  		case IIO_HUMIDITYRELATIVE:
> > +			/* Read temperature to update the t_fine value */
> > +			data->chip_info->read_temp(data);
> >  			ret = data->chip_info->read_humid(data);
> >  			*val = data->chip_info->humid_coeffs[0] * ret;
> >  			*val2 = data->chip_info->humid_coeffs[1];
> >  			ret = IIO_VAL_FRACTIONAL;
> >  			break;
> >  		case IIO_PRESSURE:
> > +			/*
> > +			 * Read temperature to update the t_fine value.
> > +			 * BMP5xx devices do this in hardware, so skip it.
> > +			 */
> > +			if (strcmp(indio_dev->name, "bmp580"))
> > +				data->chip_info->read_temp(data);
> > +
> >  			ret = data->chip_info->read_press(data);
> >  			*val = data->chip_info->press_coeffs[0] * ret;
> >  			*val2 = data->chip_info->press_coeffs[1];
> > @@ -500,10 +499,19 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_RAW:
> >  		switch (chan->type) {
> >  		case IIO_HUMIDITYRELATIVE:
> > +			/* Read temperature to update the t_fine value */
> > +			data->chip_info->read_temp(data);
> >  			*val = data->chip_info->read_humid(data);
> >  			ret = IIO_VAL_INT;
> >  			break;
> >  		case IIO_PRESSURE:
> > +			/*
> > +			 * Read temperature to update the t_fine value.
> > +			 * BMP5xx devices do this in hardware, so skip it.
> > +			 */
> > +			if (strcmp(indio_dev->name, "bmp580"))
> > +				data->chip_info->read_temp(data);
> > +
> >  			*val = data->chip_info->read_press(data);
> >  			ret = IIO_VAL_INT;
> >  			break;
> > @@ -1092,11 +1100,6 @@ static u32 bmp380_read_press(struct bmp280_data *data)
> >  	s32 adc_press;
> >  	int ret;
> >  
> > -	/* Read and compensate for temperature so we get a reading of t_fine */
> > -	ret = bmp380_read_temp(data);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> >  			       data->buf, sizeof(data->buf));
> >  	if (ret) {
> > @@ -2009,11 +2012,6 @@ static u32 bmp180_read_press(struct bmp280_data *data)
> >  	s32 adc_press;
> >  	int ret;
> >  
> > -	/* Read and compensate temperature so we get a reading of t_fine. */
> > -	ret = bmp180_read_temp(data);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = bmp180_read_adc_press(data, &adc_press);
> >  	if (ret)
> >  		return ret;
>