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 <[email protected]>
---
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, ®);
- 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
On Wed, 27 Mar 2024 09:49:36 +0100
Thomas Haemmerle <[email protected]> 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 <[email protected]>
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, ®);
> - 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;
> }
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 <[email protected]> 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 <[email protected]>
> 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, ®);
>> - 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;
>> }
>
> >> -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;
> >> }
> >
>
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
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 <[email protected]>
---
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, ®_val);
- if (rc)
+ rc = regmap_read(data->regmap, DPS310_PRS_CFG, ®_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, ®_val);
- if (rc)
+ rc = regmap_read(data->regmap, DPS310_TMP_CFG, ®_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
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 <[email protected]>
---
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, ®_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, ®_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, ®_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, ®_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, ®_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, ®_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
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 <[email protected]>
---
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
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/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
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
On Wed, 10 Apr 2024 12:36:00 +0200
Thomas Haemmerle <[email protected]> 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
On Wed, 10 Apr 2024 12:36:02 +0200
Thomas Haemmerle <[email protected]> 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 <[email protected]>
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