Received-SPF: pass (google.com: domain of linux-kernel+bounces-103459-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 14 Mar 2024 15:09:59 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Vasileios Amoiridis <vassilisamir@gmail.com>
Cc: 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: <20240314150959.585367b5@jic23-huawei>
In-Reply-To: <20240313174007.1934983-5-vassilisamir@gmail.com>
References: <20240313174007.1934983-1-vassilisamir@gmail.com>
	<20240313174007.1934983-5-vassilisamir@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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.

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;