[PATCH] iio: pressure: dps310: support negative pressure and temperature values

[PATCH] iio: pressure: dps310: support negative pressure and temperature values

[Thomas Haemmerle]

The current implementation interprets negative values returned from
function invocation as error codes, even those that report actual data.
This has a side effect that when temperature values are calculated -
they also converted by error code, which leads to false interpretation
of results.

Fix this by using the return values only for error handling and passing
a pointer for the values.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 122 +++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index 1ff091b2f764..373d1c063b05 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -171,7 +171,7 @@ static int dps310_temp_workaround(struct dps310_data *data)
 	int reg;
 
 	rc = regmap_read(data->regmap, 0x32, &reg);
-	if (rc)
+	if (rc < 0)
 		return rc;
 
 	/*
@@ -256,24 +256,24 @@ static int dps310_startup(struct dps310_data *data)
 	return dps310_temp_workaround(data);
 }
 
-static int dps310_get_pres_precision(struct dps310_data *data)
+static int dps310_get_pres_precision(struct dps310_data *data, int *val)
 {
 	int rc;
-	int val;
 
-	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
 	if (rc < 0)
 		return rc;
 
-	return BIT(val & GENMASK(2, 0));
+	*val = BIT(*val & GENMASK(2, 0));
+
+	return 0;
 }
 
-static int dps310_get_temp_precision(struct dps310_data *data)
+static int dps310_get_temp_precision(struct dps310_data *data, int *val)
 {
 	int rc;
-	int val;
 
-	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
 	if (rc < 0)
 		return rc;
 
@@ -281,7 +281,9 @@ static int dps310_get_temp_precision(struct dps310_data *data)
 	 * Scale factor is bottom 4 bits of the register, but 1111 is
 	 * reserved so just grab bottom three
 	 */
-	return BIT(val & GENMASK(2, 0));
+	*val = BIT(*val & GENMASK(2, 0));
+
+	return 0;
 }
 
 /* Called with lock held */
@@ -350,48 +352,56 @@ static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
 				  DPS310_TMP_RATE_BITS, val);
 }
 
-static int dps310_get_pres_samp_freq(struct dps310_data *data)
+static int dps310_get_pres_samp_freq(struct dps310_data *data, int *val)
 {
 	int rc;
-	int val;
 
-	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
 	if (rc < 0)
 		return rc;
 
-	return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
+	*val = BIT((*val & DPS310_PRS_RATE_BITS) >> 4);
+
+	return 0;
 }
 
-static int dps310_get_temp_samp_freq(struct dps310_data *data)
+static int dps310_get_temp_samp_freq(struct dps310_data *data, int *val)
 {
 	int rc;
-	int val;
 
-	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
 	if (rc < 0)
 		return rc;
 
-	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
+	*val = BIT((*val & DPS310_TMP_RATE_BITS) >> 4);
+
+	return 0;
 }
 
-static int dps310_get_pres_k(struct dps310_data *data)
+static int dps310_get_pres_k(struct dps310_data *data, int *val)
 {
-	int rc = dps310_get_pres_precision(data);
+	int rc;
 
-	if (rc < 0)
+	rc = dps310_get_pres_precision(data, val);
+	if (rc)
 		return rc;
 
-	return scale_factors[ilog2(rc)];
+	*val = scale_factors[ilog2(*val)];
+
+	return 0;
 }
 
-static int dps310_get_temp_k(struct dps310_data *data)
+static int dps310_get_temp_k(struct dps310_data *data, int *val)
 {
-	int rc = dps310_get_temp_precision(data);
+	int rc;
 
-	if (rc < 0)
+	rc = dps310_get_temp_precision(data, val);
+	if (rc)
 		return rc;
 
-	return scale_factors[ilog2(rc)];
+	*val = scale_factors[ilog2(*val)];
+
+	return 0;
 }
 
 static int dps310_reset_wait(struct dps310_data *data)
@@ -464,7 +474,10 @@ static int dps310_read_pres_raw(struct dps310_data *data)
 	if (mutex_lock_interruptible(&data->lock))
 		return -EINTR;
 
-	rate = dps310_get_pres_samp_freq(data);
+	rc = dps310_get_pres_samp_freq(data, &rate);
+	if (rc)
+		return rc;
+
 	timeout = DPS310_POLL_TIMEOUT_US(rate);
 
 	/* Poll for sensor readiness; base the timeout upon the sample rate. */
@@ -510,7 +523,10 @@ static int dps310_read_temp_raw(struct dps310_data *data)
 	if (mutex_lock_interruptible(&data->lock))
 		return -EINTR;
 
-	rate = dps310_get_temp_samp_freq(data);
+	rc = dps310_get_temp_samp_freq(data, &rate);
+	if (rc)
+		return rc;
+
 	timeout = DPS310_POLL_TIMEOUT_US(rate);
 
 	/* Poll for sensor readiness; base the timeout upon the sample rate. */
@@ -612,13 +628,13 @@ static int dps310_write_raw(struct iio_dev *iio,
 	return rc;
 }
 
-static int dps310_calculate_pressure(struct dps310_data *data)
+static int dps310_calculate_pressure(struct dps310_data *data, int *val)
 {
 	int i;
 	int rc;
 	int t_ready;
-	int kpi = dps310_get_pres_k(data);
-	int kti = dps310_get_temp_k(data);
+	int kpi;
+	int kti;
 	s64 rem = 0ULL;
 	s64 pressure = 0ULL;
 	s64 p;
@@ -629,11 +645,13 @@ static int dps310_calculate_pressure(struct dps310_data *data)
 	s64 kp;
 	s64 kt;
 
-	if (kpi < 0)
-		return kpi;
+	rc = dps310_get_pres_k(data, &kpi);
+	if (rc < 0)
+		return rc;
 
-	if (kti < 0)
-		return kti;
+	rc = dps310_get_temp_k(data, &kti);
+	if (rc < 0)
+		return rc;
 
 	kp = (s64)kpi;
 	kt = (s64)kti;
@@ -687,7 +705,9 @@ static int dps310_calculate_pressure(struct dps310_data *data)
 	if (pressure < 0LL)
 		return -ERANGE;
 
-	return (int)min_t(s64, pressure, INT_MAX);
+	*val = (int)min_t(s64, pressure, INT_MAX);
+
+	return 0;
 }
 
 static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
@@ -697,11 +717,10 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		rc = dps310_get_pres_samp_freq(data);
+		rc = dps310_get_pres_samp_freq(data, val);
 		if (rc < 0)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_PROCESSED:
@@ -709,20 +728,17 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 		if (rc)
 			return rc;
 
-		rc = dps310_calculate_pressure(data);
+		rc = dps310_calculate_pressure(data, val);
 		if (rc < 0)
 			return rc;
 
-		*val = rc;
 		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */
 		return IIO_VAL_FRACTIONAL;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		rc = dps310_get_pres_precision(data);
+		rc = dps310_get_pres_precision(data, val);
 		if (rc < 0)
 			return rc;
-
-		*val = rc;
 		return IIO_VAL_INT;
 
 	default:
@@ -730,14 +746,15 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 	}
 }
 
-static int dps310_calculate_temp(struct dps310_data *data)
+static int dps310_calculate_temp(struct dps310_data *data, int *val)
 {
 	s64 c0;
 	s64 t;
-	int kt = dps310_get_temp_k(data);
+	int kt, rc;
 
-	if (kt < 0)
-		return kt;
+	rc = dps310_get_temp_k(data, &kt);
+	if (rc < 0)
+		return rc;
 
 	/* Obtain inverse-scaled offset */
 	c0 = div_s64((s64)kt * (s64)data->c0, 2);
@@ -746,7 +763,9 @@ static int dps310_calculate_temp(struct dps310_data *data)
 	t = c0 + ((s64)data->temp_raw * (s64)data->c1);
 
 	/* Convert to milliCelsius and scale the temperature */
-	return (int)div_s64(t * 1000LL, kt);
+	*val = (int)div_s64(t * 1000LL, kt);
+
+	return 0;
 }
 
 static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
@@ -756,11 +775,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		rc = dps310_get_temp_samp_freq(data);
+		rc = dps310_get_temp_samp_freq(data, val);
 		if (rc < 0)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_PROCESSED:
@@ -768,19 +786,17 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 		if (rc)
 			return rc;
 
-		rc = dps310_calculate_temp(data);
+		rc = dps310_calculate_temp(data, val);
 		if (rc < 0)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		rc = dps310_get_temp_precision(data);
+		rc = dps310_get_temp_precision(data, val);
 		if (rc < 0)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	default:
-- 
2.34.1

Re: [PATCH] iio: pressure: dps310: support negative pressure and temperature values

[Jonathan Cameron]

On Wed, 27 Mar 2024 09:49:36 +0100
Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com> wrote:

> The current implementation interprets negative values returned from
> function invocation as error codes, even those that report actual data.
> This has a side effect that when temperature values are calculated -
> they also converted by error code, which leads to false interpretation
> of results.
> 
> Fix this by using the return values only for error handling and passing
> a pointer for the values.
> 
> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
Hi Thomas,

This needs a fixes tag so we know where to backport it to.

A few other comments inline.  Note that one aim in a fix is to keep things
minimal to make it easy to backport.  If you want to the follow the fix
with a cleanup patch that makes the driver more consistent that is great,
just don't combine that with the bug fix.

Jonathan

> ---
>  drivers/iio/pressure/dps310.c | 122 +++++++++++++++++++---------------
>  1 file changed, 69 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index 1ff091b2f764..373d1c063b05 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c
> @@ -171,7 +171,7 @@ static int dps310_temp_workaround(struct dps310_data *data)
>  	int reg;
>  
>  	rc = regmap_read(data->regmap, 0x32, &reg);
> -	if (rc)
> +	if (rc < 0)
>  		return rc;

Why this change?  It seems unrelated to the issue you are fixing.

>  
>  	/*
> @@ -256,24 +256,24 @@ static int dps310_startup(struct dps310_data *data)
>  	return dps310_temp_workaround(data);
>  }
>  
> -static int dps310_get_pres_precision(struct dps310_data *data)
> +static int dps310_get_pres_precision(struct dps310_data *data, int *val)
>  {
>  	int rc;
> -	int val;
>  
> -	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
> +	rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
>  	if (rc < 0)
>  		return rc;
I'd prefer a local variable here for the intermediate result.
>  
> -	return BIT(val & GENMASK(2, 0));
> +	*val = BIT(*val & GENMASK(2, 0));
For these precision values, it's positive anyway, so why
change it to report this way?  Consistency only or am I missing something else?
> +
> +	return 0;
>  }
>  
> -static int dps310_get_temp_precision(struct dps310_data *data)
> +static int dps310_get_temp_precision(struct dps310_data *data, int *val)
>  {
>  	int rc;
> -	int val;
>  
> -	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
> +	rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
As above, local variable for intermediate result would be clearer.
>  	if (rc < 0)
>  		return rc;
>  
> @@ -281,7 +281,9 @@ static int dps310_get_temp_precision(struct dps310_data *data)
>  	 * Scale factor is bottom 4 bits of the register, but 1111 is
>  	 * reserved so just grab bottom three
>  	 */
> -	return BIT(val & GENMASK(2, 0));
> +	*val = BIT(*val & GENMASK(2, 0));
> +
> +	return 0;
>  }
>  
>  /* Called with lock held */
> @@ -350,48 +352,56 @@ static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
>  				  DPS310_TMP_RATE_BITS, val);
>  }
>  
> -static int dps310_get_pres_samp_freq(struct dps310_data *data)
> +static int dps310_get_pres_samp_freq(struct dps310_data *data, int *val)
>  {
>  	int rc;
> -	int val;
>  
> -	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
> +	rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
Same again.
>  	if (rc < 0)
>  		return rc;
>  
> -	return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
> +	*val = BIT((*val & DPS310_PRS_RATE_BITS) >> 4);
Whilst here nice to use BIT(FIELD_GET(regval, DPS310_PRS_RATE_BITS));
> +
> +	return 0;
>  }
>  
> -static int dps310_get_temp_samp_freq(struct dps310_data *data)
> +static int dps310_get_temp_samp_freq(struct dps310_data *data, int *val)
>  {
>  	int rc;
> -	int val;
>  
> -	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
> +	rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
>  	if (rc < 0)
>  		return rc;
>  
> -	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
> +	*val = BIT((*val & DPS310_TMP_RATE_BITS) >> 4);
As above.

> +
> +	return 0;
>  }
>  
> -static int dps310_get_pres_k(struct dps310_data *data)
> +static int dps310_get_pres_k(struct dps310_data *data, int *val)
>  {
> -	int rc = dps310_get_pres_precision(data);
> +	int rc;
>  
> -	if (rc < 0)
> +	rc = dps310_get_pres_precision(data, val);
> +	if (rc)
>  		return rc;
>  
> -	return scale_factors[ilog2(rc)];
> +	*val = scale_factors[ilog2(*val)];
This only just went to the effort of 2^val, so why not skip that step and
pull the BIT() section out to read_pressure() where we do want that form.
You will need an extra local variable at that call site I think, but
in general it is a useful additional simplification of the code.
> +
> +	return 0;
>  }
>  
> -static int dps310_get_temp_k(struct dps310_data *data)
> +static int dps310_get_temp_k(struct dps310_data *data, int *val)
>  {
> -	int rc = dps310_get_temp_precision(data);
> +	int rc;
>  
> -	if (rc < 0)
> +	rc = dps310_get_temp_precision(data, val);
> +	if (rc)
>  		return rc;
>  
> -	return scale_factors[ilog2(rc)];
> +	*val = scale_factors[ilog2(*val)];
As above.
> +
> +	return 0;
>  }

Re: [PATCH] iio: pressure: dps310: support negative pressure and temperature values

[HAEMMERLE Thomas]

Hi Jonathan!

Thanks for the review!

On 28.03.24 14:34, Jonathan Cameron wrote:
> 
> On Wed, 27 Mar 2024 09:49:36 +0100
> Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com> wrote:
> 
>> The current implementation interprets negative values returned from
>> function invocation as error codes, even those that report actual data.
>> This has a side effect that when temperature values are calculated -
>> they also converted by error code, which leads to false interpretation
>> of results.
>>
>> Fix this by using the return values only for error handling and passing
>> a pointer for the values.
>>
>> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
> Hi Thomas,
> 
> This needs a fixes tag so we know where to backport it to.

Will add it.

> 
> A few other comments inline.  Note that one aim in a fix is to keep things
> minimal to make it easy to backport.  If you want to the follow the fix
> with a cleanup patch that makes the driver more consistent that is great,
> just don't combine that with the bug fix.

ACK - I will split the patch.

> 
> Jonathan
> 
>> ---
>>   drivers/iio/pressure/dps310.c | 122 +++++++++++++++++++---------------
>>   1 file changed, 69 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
>> index 1ff091b2f764..373d1c063b05 100644
>> --- a/drivers/iio/pressure/dps310.c
>> +++ b/drivers/iio/pressure/dps310.c
>> @@ -171,7 +171,7 @@ static int dps310_temp_workaround(struct dps310_data *data)
>>        int reg;
>>
>>        rc = regmap_read(data->regmap, 0x32, &reg);
>> -     if (rc)
>> +     if (rc < 0)
>>                return rc;
> 
> Why this change?  It seems unrelated to the issue you are fixing.

The return values in this driver are not checked consistently, and this 
aligns with the other call(s) of `regmap_read`. But I agree - it's not 
related to the issue.

> 
>>
>>        /*
>> @@ -256,24 +256,24 @@ static int dps310_startup(struct dps310_data *data)
>>        return dps310_temp_workaround(data);
>>   }
>>
>> -static int dps310_get_pres_precision(struct dps310_data *data)
>> +static int dps310_get_pres_precision(struct dps310_data *data, int *val)
>>   {
>>        int rc;
>> -     int val;
>>
>> -     rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
>> +     rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
>>        if (rc < 0)
>>                return rc;
> I'd prefer a local variable here for the intermediate result.

ACK.

>>
>> -     return BIT(val & GENMASK(2, 0));
>> +     *val = BIT(*val & GENMASK(2, 0));
> For these precision values, it's positive anyway, so why
> change it to report this way?  Consistency only or am I missing something else?

Yes - for consistency.

>> +
>> +     return 0;
>>   }
>>
>> -static int dps310_get_temp_precision(struct dps310_data *data)
>> +static int dps310_get_temp_precision(struct dps310_data *data, int *val)
>>   {
>>        int rc;
>> -     int val;
>>
>> -     rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
>> +     rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
> As above, local variable for intermediate result would be clearer.

ACK.

>>        if (rc < 0)
>>                return rc;
>>
>> @@ -281,7 +281,9 @@ static int dps310_get_temp_precision(struct dps310_data *data)
>>         * Scale factor is bottom 4 bits of the register, but 1111 is
>>         * reserved so just grab bottom three
>>         */
>> -     return BIT(val & GENMASK(2, 0));
>> +     *val = BIT(*val & GENMASK(2, 0));
>> +
>> +     return 0;
>>   }
>>
>>   /* Called with lock held */
>> @@ -350,48 +352,56 @@ static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
>>                                  DPS310_TMP_RATE_BITS, val);
>>   }
>>
>> -static int dps310_get_pres_samp_freq(struct dps310_data *data)
>> +static int dps310_get_pres_samp_freq(struct dps310_data *data, int *val)
>>   {
>>        int rc;
>> -     int val;
>>
>> -     rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
>> +     rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
> Same again.

ACK.

>>        if (rc < 0)
>>                return rc;
>>
>> -     return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
>> +     *val = BIT((*val & DPS310_PRS_RATE_BITS) >> 4);
> Whilst here nice to use BIT(FIELD_GET(regval, DPS310_PRS_RATE_BITS));
>> +
>> +     return 0;
>>   }
>>
>> -static int dps310_get_temp_samp_freq(struct dps310_data *data)
>> +static int dps310_get_temp_samp_freq(struct dps310_data *data, int *val)
>>   {
>>        int rc;
>> -     int val;
>>
>> -     rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
>> +     rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
>>        if (rc < 0)
>>                return rc;
>>
>> -     return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
>> +     *val = BIT((*val & DPS310_TMP_RATE_BITS) >> 4);
> As above.
> 

ACK.

>> +
>> +     return 0;
>>   }
>>
>> -static int dps310_get_pres_k(struct dps310_data *data)
>> +static int dps310_get_pres_k(struct dps310_data *data, int *val)
>>   {
>> -     int rc = dps310_get_pres_precision(data);
>> +     int rc;
>>
>> -     if (rc < 0)
>> +     rc = dps310_get_pres_precision(data, val);
>> +     if (rc)
>>                return rc;
>>
>> -     return scale_factors[ilog2(rc)];
>> +     *val = scale_factors[ilog2(*val)];
> This only just went to the effort of 2^val, so why not skip that step and
> pull the BIT() section out to read_pressure() where we do want that form.
> You will need an extra local variable at that call site I think, but
> in general it is a useful additional simplification of the code.

I'm not sure if I get you correct, as this function is not directly 
called in `read_pressure`:
You suggest dropping this function at all, call 
`dps310_get_pres_precision` directly in `dps310_calculate_pressure` and 
move the lookup of the compensation scale factor there?

>> +
>> +     return 0;
>>   }
>>
>> -static int dps310_get_temp_k(struct dps310_data *data)
>> +static int dps310_get_temp_k(struct dps310_data *data, int *val)
>>   {
>> -     int rc = dps310_get_temp_precision(data);
>> +     int rc;
>>
>> -     if (rc < 0)
>> +     rc = dps310_get_temp_precision(data, val);
>> +     if (rc)
>>                return rc;
>>
>> -     return scale_factors[ilog2(rc)];
>> +     *val = scale_factors[ilog2(*val)];
> As above.

Based on my interpretation above:
For `dps310_get_temp_k` it would require to move the lookup of the 
compensation scale factor to `dps310_calculate_pressure` and 
`dps310_calculate_temp`.
Maybe this would simplify the code, but it would make it harder to read.


Thomas

>> +
>> +     return 0;
>>   }
> 

Re: [PATCH] iio: pressure: dps310: support negative pressure and temperature values

[Jonathan Cameron]

> >> -static int dps310_get_pres_k(struct dps310_data *data)
> >> +static int dps310_get_pres_k(struct dps310_data *data, int *val)
> >>   {
> >> -     int rc = dps310_get_pres_precision(data);
> >> +     int rc;
> >>
> >> -     if (rc < 0)
> >> +     rc = dps310_get_pres_precision(data, val);
> >> +     if (rc)
> >>                return rc;
> >>
> >> -     return scale_factors[ilog2(rc)];
> >> +     *val = scale_factors[ilog2(*val)];  
> > This only just went to the effort of 2^val, so why not skip that step and
> > pull the BIT() section out to read_pressure() where we do want that form.
> > You will need an extra local variable at that call site I think, but
> > in general it is a useful additional simplification of the code.  
> 
> I'm not sure if I get you correct, as this function is not directly 
> called in `read_pressure`:
> You suggest dropping this function at all, call 
> `dps310_get_pres_precision` directly in `dps310_calculate_pressure` and 
> move the lookup of the compensation scale factor there?

More simply avoid _get_pres_precision returning a value that is in the form
that requires us to immediately undo the BIT() logic at the end of that function.
One way to do that is to just call the regmap_read() directly here.

> 
> >> +
> >> +     return 0;
> >>   }
> >>
> >> -static int dps310_get_temp_k(struct dps310_data *data)
> >> +static int dps310_get_temp_k(struct dps310_data *data, int *val)
> >>   {
> >> -     int rc = dps310_get_temp_precision(data);
> >> +     int rc;
> >>
> >> -     if (rc < 0)
> >> +     rc = dps310_get_temp_precision(data, val);
> >> +     if (rc)
> >>                return rc;
> >>
> >> -     return scale_factors[ilog2(rc)];
> >> +     *val = scale_factors[ilog2(*val)];  
> > As above.  
> 
> Based on my interpretation above:
> For `dps310_get_temp_k` it would require to move the lookup of the 
> compensation scale factor to `dps310_calculate_pressure` and 
> `dps310_calculate_temp`.
> Maybe this would simplify the code, but it would make it harder to read.
Just call rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
here and use 
*val = scale_factors[val & GENMASK(2, 0)];
here which I think ends up with the same value.

> 
> 
> Thomas
> 
> >> +
> >> +     return 0;
> >>   }  
> >   
> 

[PATCH v2 0/4] iio: pressure: dps310: support negative temperature values

[Thomas Haemmerle]

This patch set fixes the reading of negative temperatures (returned in
millidegree celsius). As this requires a change of the error handling
other functions are aligned with this.
In addition a small code simplification for reading the scale factors
for temperature and pressure is included.

---
Changes in v2:
 - include fixes tag
 - Split up patch
 - introduce variables for intermediate results in functions
 - simplify scale factor reading

Thomas Haemmerle (4):
  iio: pressure: dps310: support negative temperature values
  iio: pressure: dps310: introduce consistent error handling
  iio: pressure: dps310: consistently check return value of
    `regmap_read`
  iio: pressure: dps310: simplify scale factor reading

 drivers/iio/pressure/dps310.c | 138 +++++++++++++++++++---------------
 1 file changed, 77 insertions(+), 61 deletions(-)


base-commit: 2c71fdf02a95b3dd425b42f28fd47fb2b1d22702
-- 
2.34.1

[PATCH v2 1/4] iio: pressure: dps310: support negative temperature values

[Thomas Haemmerle]

The current implementation interprets negative values returned from
`dps310_calculate_temp` as error codes.
This has a side effect that when negative temperature values are
calculated, they are interpreted as error.

Fix this by using the return value only for error handling and passing a
pointer for the value.

Fixes: ba6ec48e76bc ("iio: Add driver for Infineon DPS310")
Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index 1ff091b2f764..d0a516d56da4 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -730,7 +730,7 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 	}
 }
 
-static int dps310_calculate_temp(struct dps310_data *data)
+static int dps310_calculate_temp(struct dps310_data *data, int *val)
 {
 	s64 c0;
 	s64 t;
@@ -746,7 +746,9 @@ static int dps310_calculate_temp(struct dps310_data *data)
 	t = c0 + ((s64)data->temp_raw * (s64)data->c1);
 
 	/* Convert to milliCelsius and scale the temperature */
-	return (int)div_s64(t * 1000LL, kt);
+	*val = (int)div_s64(t * 1000LL, kt);
+
+	return 0;
 }
 
 static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
@@ -768,11 +770,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 		if (rc)
 			return rc;
 
-		rc = dps310_calculate_temp(data);
-		if (rc < 0)
+		rc = dps310_calculate_temp(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-- 
2.34.1

[PATCH v2 2/4] iio: pressure: dps310: introduce consistent error handling

[Thomas Haemmerle]

Align error handling with `dps310_calculate_temp`, where it's not
possible to differentiate between errors and valid calculations by
checking if the returned value is negative.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 129 +++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index d0a516d56da4..a48e8adf63ae 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -256,24 +256,24 @@ static int dps310_startup(struct dps310_data *data)
 	return dps310_temp_workaround(data);
 }
 
-static int dps310_get_pres_precision(struct dps310_data *data)
+static int dps310_get_pres_precision(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
-	return BIT(val & GENMASK(2, 0));
+	*val = BIT(reg_val & GENMASK(2, 0));
+
+	return 0;
 }
 
-static int dps310_get_temp_precision(struct dps310_data *data)
+static int dps310_get_temp_precision(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
@@ -281,7 +281,9 @@ static int dps310_get_temp_precision(struct dps310_data *data)
 	 * Scale factor is bottom 4 bits of the register, but 1111 is
 	 * reserved so just grab bottom three
 	 */
-	return BIT(val & GENMASK(2, 0));
+	*val = BIT(reg_val & GENMASK(2, 0));
+
+	return 0;
 }
 
 /* Called with lock held */
@@ -350,48 +352,56 @@ static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
 				  DPS310_TMP_RATE_BITS, val);
 }
 
-static int dps310_get_pres_samp_freq(struct dps310_data *data)
+static int dps310_get_pres_samp_freq(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
-	return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
+	*val = BIT((reg_val & DPS310_PRS_RATE_BITS) >> 4);
+
+	return 0;
 }
 
-static int dps310_get_temp_samp_freq(struct dps310_data *data)
+static int dps310_get_temp_samp_freq(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
-	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
+	*val = BIT((reg_val & DPS310_TMP_RATE_BITS) >> 4);
+
+	return 0;
 }
 
-static int dps310_get_pres_k(struct dps310_data *data)
+static int dps310_get_pres_k(struct dps310_data *data, int *val)
 {
-	int rc = dps310_get_pres_precision(data);
+	int reg_val, rc;
 
-	if (rc < 0)
+	rc = dps310_get_pres_precision(data, &reg_val);
+	if (rc)
 		return rc;
 
-	return scale_factors[ilog2(rc)];
+	*val = scale_factors[ilog2(reg_val)];
+
+	return 0;
 }
 
-static int dps310_get_temp_k(struct dps310_data *data)
+static int dps310_get_temp_k(struct dps310_data *data, int *val)
 {
-	int rc = dps310_get_temp_precision(data);
+	int reg_val, rc;
 
-	if (rc < 0)
+	rc = dps310_get_temp_precision(data, &reg_val);
+	if (rc)
 		return rc;
 
-	return scale_factors[ilog2(rc)];
+	*val = scale_factors[ilog2(reg_val)];
+
+	return 0;
 }
 
 static int dps310_reset_wait(struct dps310_data *data)
@@ -464,7 +474,10 @@ static int dps310_read_pres_raw(struct dps310_data *data)
 	if (mutex_lock_interruptible(&data->lock))
 		return -EINTR;
 
-	rate = dps310_get_pres_samp_freq(data);
+	rc = dps310_get_pres_samp_freq(data, &rate);
+	if (rc)
+		return rc;
+
 	timeout = DPS310_POLL_TIMEOUT_US(rate);
 
 	/* Poll for sensor readiness; base the timeout upon the sample rate. */
@@ -510,7 +523,10 @@ static int dps310_read_temp_raw(struct dps310_data *data)
 	if (mutex_lock_interruptible(&data->lock))
 		return -EINTR;
 
-	rate = dps310_get_temp_samp_freq(data);
+	rc = dps310_get_temp_samp_freq(data, &rate);
+	if (rc)
+		return rc;
+
 	timeout = DPS310_POLL_TIMEOUT_US(rate);
 
 	/* Poll for sensor readiness; base the timeout upon the sample rate. */
@@ -612,13 +628,13 @@ static int dps310_write_raw(struct iio_dev *iio,
 	return rc;
 }
 
-static int dps310_calculate_pressure(struct dps310_data *data)
+static int dps310_calculate_pressure(struct dps310_data *data, int *val)
 {
 	int i;
 	int rc;
 	int t_ready;
-	int kpi = dps310_get_pres_k(data);
-	int kti = dps310_get_temp_k(data);
+	int kpi;
+	int kti;
 	s64 rem = 0ULL;
 	s64 pressure = 0ULL;
 	s64 p;
@@ -629,11 +645,13 @@ static int dps310_calculate_pressure(struct dps310_data *data)
 	s64 kp;
 	s64 kt;
 
-	if (kpi < 0)
-		return kpi;
+	rc = dps310_get_pres_k(data, &kpi);
+	if (rc)
+		return rc;
 
-	if (kti < 0)
-		return kti;
+	rc = dps310_get_temp_k(data, &kti);
+	if (rc)
+		return rc;
 
 	kp = (s64)kpi;
 	kt = (s64)kti;
@@ -687,7 +705,9 @@ static int dps310_calculate_pressure(struct dps310_data *data)
 	if (pressure < 0LL)
 		return -ERANGE;
 
-	return (int)min_t(s64, pressure, INT_MAX);
+	*val = (int)min_t(s64, pressure, INT_MAX);
+
+	return 0;
 }
 
 static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
@@ -697,11 +717,10 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		rc = dps310_get_pres_samp_freq(data);
-		if (rc < 0)
+		rc = dps310_get_pres_samp_freq(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_PROCESSED:
@@ -709,20 +728,17 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 		if (rc)
 			return rc;
 
-		rc = dps310_calculate_pressure(data);
-		if (rc < 0)
+		rc = dps310_calculate_pressure(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */
 		return IIO_VAL_FRACTIONAL;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		rc = dps310_get_pres_precision(data);
-		if (rc < 0)
+		rc = dps310_get_pres_precision(data, val);
+		if (rc)
 			return rc;
-
-		*val = rc;
 		return IIO_VAL_INT;
 
 	default:
@@ -734,10 +750,11 @@ static int dps310_calculate_temp(struct dps310_data *data, int *val)
 {
 	s64 c0;
 	s64 t;
-	int kt = dps310_get_temp_k(data);
+	int kt, rc;
 
-	if (kt < 0)
-		return kt;
+	rc = dps310_get_temp_k(data, &kt);
+	if (rc)
+		return rc;
 
 	/* Obtain inverse-scaled offset */
 	c0 = div_s64((s64)kt * (s64)data->c0, 2);
@@ -758,11 +775,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		rc = dps310_get_temp_samp_freq(data);
-		if (rc < 0)
+		rc = dps310_get_temp_samp_freq(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_PROCESSED:
@@ -777,11 +793,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		rc = dps310_get_temp_precision(data);
-		if (rc < 0)
+		rc = dps310_get_temp_precision(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	default:
-- 
2.34.1

Re: [PATCH v2 2/4] iio: pressure: dps310: introduce consistent error handling

[Dan Carpenter]

Hi Thomas,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Haemmerle/iio-pressure-dps310-support-negative-temperature-values/20240410-183937
base:   2c71fdf02a95b3dd425b42f28fd47fb2b1d22702
patch link:    https://lore.kernel.org/r/20240410103604.992989-3-thomas.haemmerle%40leica-geosystems.com
patch subject: [PATCH v2 2/4] iio: pressure: dps310: introduce consistent error handling
config: i386-randconfig-141-20240411 (https://download.01.org/0day-ci/archive/20240411/202404110708.7BQYVa7Z-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202404110708.7BQYVa7Z-lkp@intel.com/

smatch warnings:
drivers/iio/pressure/dps310.c:497 dps310_read_pres_raw() warn: inconsistent returns '&data->lock'.
drivers/iio/pressure/dps310.c:541 dps310_read_temp_raw() warn: inconsistent returns '&data->lock'.

vim +497 drivers/iio/pressure/dps310.c

d711a3c7dc829c Eddie James      2019-05-20  466  static int dps310_read_pres_raw(struct dps310_data *data)
ba6ec48e76bcd4 Joel Stanley     2019-05-20  467  {
ba6ec48e76bcd4 Joel Stanley     2019-05-20  468  	int rc;
ba6ec48e76bcd4 Joel Stanley     2019-05-20  469  	int rate;
ba6ec48e76bcd4 Joel Stanley     2019-05-20  470  	int timeout;
ba6ec48e76bcd4 Joel Stanley     2019-05-20  471  	s32 raw;
ba6ec48e76bcd4 Joel Stanley     2019-05-20  472  	u8 val[3];
ba6ec48e76bcd4 Joel Stanley     2019-05-20  473  
ba6ec48e76bcd4 Joel Stanley     2019-05-20  474  	if (mutex_lock_interruptible(&data->lock))
ba6ec48e76bcd4 Joel Stanley     2019-05-20  475  		return -EINTR;
ba6ec48e76bcd4 Joel Stanley     2019-05-20  476  
f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  477  	rc = dps310_get_pres_samp_freq(data, &rate);
f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  478  	if (rc)
f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  479  		return rc;

goto unlock

f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  480  
ba6ec48e76bcd4 Joel Stanley     2019-05-20  481  	timeout = DPS310_POLL_TIMEOUT_US(rate);
ba6ec48e76bcd4 Joel Stanley     2019-05-20  482  
ba6ec48e76bcd4 Joel Stanley     2019-05-20  483  	/* Poll for sensor readiness; base the timeout upon the sample rate. */
7b4ab4abcea4c0 Eddie James      2022-09-15  484  	rc = dps310_ready(data, DPS310_PRS_RDY, timeout);
d711a3c7dc829c Eddie James      2019-05-20  485  	if (rc)
d711a3c7dc829c Eddie James      2019-05-20  486  		goto done;
d711a3c7dc829c Eddie James      2019-05-20  487  
d711a3c7dc829c Eddie James      2019-05-20  488  	rc = regmap_bulk_read(data->regmap, DPS310_PRS_BASE, val, sizeof(val));
ba6ec48e76bcd4 Joel Stanley     2019-05-20  489  	if (rc < 0)
ba6ec48e76bcd4 Joel Stanley     2019-05-20  490  		goto done;
ba6ec48e76bcd4 Joel Stanley     2019-05-20  491  
d711a3c7dc829c Eddie James      2019-05-20  492  	raw = (val[0] << 16) | (val[1] << 8) | val[2];
d711a3c7dc829c Eddie James      2019-05-20  493  	data->pressure_raw = sign_extend32(raw, 23);
d711a3c7dc829c Eddie James      2019-05-20  494  
d711a3c7dc829c Eddie James      2019-05-20  495  done:
d711a3c7dc829c Eddie James      2019-05-20  496  	mutex_unlock(&data->lock);
d711a3c7dc829c Eddie James      2019-05-20 @497  	return rc;
d711a3c7dc829c Eddie James      2019-05-20  498  }

[ snip ]

d711a3c7dc829c Eddie James      2019-05-20  517  static int dps310_read_temp_raw(struct dps310_data *data)
d711a3c7dc829c Eddie James      2019-05-20  518  {
d711a3c7dc829c Eddie James      2019-05-20  519  	int rc;
d711a3c7dc829c Eddie James      2019-05-20  520  	int rate;
d711a3c7dc829c Eddie James      2019-05-20  521  	int timeout;
d711a3c7dc829c Eddie James      2019-05-20  522  
d711a3c7dc829c Eddie James      2019-05-20  523  	if (mutex_lock_interruptible(&data->lock))
d711a3c7dc829c Eddie James      2019-05-20  524  		return -EINTR;
d711a3c7dc829c Eddie James      2019-05-20  525  
f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  526  	rc = dps310_get_temp_samp_freq(data, &rate);
f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  527  	if (rc)
f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  528  		return rc;

goto unlock

f3e28d813ae8d1 Thomas Haemmerle 2024-04-10  529  
d711a3c7dc829c Eddie James      2019-05-20  530  	timeout = DPS310_POLL_TIMEOUT_US(rate);
d711a3c7dc829c Eddie James      2019-05-20  531  
d711a3c7dc829c Eddie James      2019-05-20  532  	/* Poll for sensor readiness; base the timeout upon the sample rate. */
7b4ab4abcea4c0 Eddie James      2022-09-15  533  	rc = dps310_ready(data, DPS310_TMP_RDY, timeout);
7b4ab4abcea4c0 Eddie James      2022-09-15  534  	if (rc)
d711a3c7dc829c Eddie James      2019-05-20  535  		goto done;
d711a3c7dc829c Eddie James      2019-05-20  536  
d711a3c7dc829c Eddie James      2019-05-20  537  	rc = dps310_read_temp_ready(data);
d711a3c7dc829c Eddie James      2019-05-20  538  
ba6ec48e76bcd4 Joel Stanley     2019-05-20  539  done:
ba6ec48e76bcd4 Joel Stanley     2019-05-20  540  	mutex_unlock(&data->lock);
ba6ec48e76bcd4 Joel Stanley     2019-05-20 @541  	return rc;
ba6ec48e76bcd4 Joel Stanley     2019-05-20  542  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/4] iio: pressure: dps310: introduce consistent error handling

[Jonathan Cameron]

On Wed, 10 Apr 2024 12:36:02 +0200
Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com> wrote:

> Align error handling with `dps310_calculate_temp`, where it's not
> possible to differentiate between errors and valid calculations by
> checking if the returned value is negative.
> 
> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
Other than the reported locking issue the rest of this series now looks
fine to me.  So hopefully that should be easy to resolve and I'll pick
up v3.  Given we are some way into the cycle and the negative value bug
has been there quite a while I'll probably just queue the whole series
for the next merge window rather than going through the dance of taking
the fix patch separately and having to wait before merging the rest.

Thanks,


Jonathan

[PATCH v2 3/4] iio: pressure: dps310: consistently check return value of `regmap_read`

[Thomas Haemmerle]

Align the check of return values `regmap_read` so that it's consistent
across this driver code.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index a48e8adf63ae..4abce7e40715 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -171,7 +171,7 @@ static int dps310_temp_workaround(struct dps310_data *data)
 	int reg;
 
 	rc = regmap_read(data->regmap, 0x32, &reg);
-	if (rc)
+	if (rc < 0)
 		return rc;
 
 	/*
-- 
2.34.1

[PATCH v2 4/4] iio: pressure: dps310: simplify scale factor reading

[Thomas Haemmerle]

Both functions `dps310_get_pres_precision` and
`dps310_get_temp_precision` provide the oversampling rate by calling the
`BIT()` macro. However, to look up the corresponding scale factor, we
need the register value itself. Currently, this is achieved by undoing
the calculation of the oversampling rate with `ilog2()`.

Simplify the two functions for getting the scale factor and directly
use the register content for the lookup.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index 4abce7e40715..320e34ff9381 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -382,11 +382,11 @@ static int dps310_get_pres_k(struct dps310_data *data, int *val)
 {
 	int reg_val, rc;
 
-	rc = dps310_get_pres_precision(data, &reg_val);
-	if (rc)
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &reg_val);
+	if (rc < 0)
 		return rc;
 
-	*val = scale_factors[ilog2(reg_val)];
+	*val = scale_factors[reg_val & GENMASK(2, 0)];
 
 	return 0;
 }
@@ -395,11 +395,11 @@ static int dps310_get_temp_k(struct dps310_data *data, int *val)
 {
 	int reg_val, rc;
 
-	rc = dps310_get_temp_precision(data, &reg_val);
-	if (rc)
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &reg_val);
+	if (rc < 0)
 		return rc;
 
-	*val = scale_factors[ilog2(reg_val)];
+	*val = scale_factors[reg_val & GENMASK(2, 0)];
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2 0/4] iio: pressure: dps310: support negative temperature values

[Jonathan Cameron]

On Wed, 10 Apr 2024 12:36:00 +0200
Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com> wrote:

> This patch set fixes the reading of negative temperatures (returned in
> millidegree celsius). As this requires a change of the error handling
> other functions are aligned with this.
> In addition a small code simplification for reading the scale factors
> for temperature and pressure is included.
One quick process thing.
For IIO (and probably most of the rest of the kernel) we strongly discourage
sending new versions in reply to a previous version.

The only real result is that in a typical email client the threads become
confused and the new version may be missed entirely.

Just sent a fresh thread - the naming makes it easy to connect new
versions to older ones and tools like b4 deal with this automatically.

Jonathan
> 
> ---
> Changes in v2:
>  - include fixes tag
>  - Split up patch
>  - introduce variables for intermediate results in functions
>  - simplify scale factor reading
> 
> Thomas Haemmerle (4):
>   iio: pressure: dps310: support negative temperature values
>   iio: pressure: dps310: introduce consistent error handling
>   iio: pressure: dps310: consistently check return value of
>     `regmap_read`
>   iio: pressure: dps310: simplify scale factor reading
> 
>  drivers/iio/pressure/dps310.c | 138 +++++++++++++++++++---------------
>  1 file changed, 77 insertions(+), 61 deletions(-)
> 
> 
> base-commit: 2c71fdf02a95b3dd425b42f28fd47fb2b1d22702