| Message ID | 20240327084937.3801125-1-thomas.haemmerle@leica-geosystems.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: pressure: dps310: support negative pressure and temperature values | expand |
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, ®); > - 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 <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, ®); >> - 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; > >> } > > >
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:
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(-)