On 5/23/22 21:12, Joel Stanley wrote:
On Wed, 18 May 2022 at 14:48, Eddie James <eajames@xxxxxxxxxxxxx> wrote:
Corruption of the MEAS_CFG register has been observed soon after
system boot. In order to recover this scenario, check MEAS_CFG if
measurement isn't ready, and if it's incorrect, reset the DPS310
and execute the startup procedure.
I have some suggestions below on how to rework to make the code easier
to understand. But before we got to that, I had some high level
questions:
You don't seem to be setting the en bits in the CFG register after
doing the reset. Is that required?
It does set the enable bits in the startup procedure, called after the
reset.
Are we ok to sleep for 2.5ms in the iio_info->read_raw callback?
I believe it's safe... the code already has a mutex, so its not called
in atomic context.
Fixes: ba6ec48e76bc ("iio: Add driver for Infineon DPS310")
Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
---
drivers/iio/pressure/dps310.c | 89 ++++++++++++++++++++++++++++-------
1 file changed, 71 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index f79b274bb38d..c6d02679ef33 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -397,6 +397,39 @@ static int dps310_get_temp_k(struct dps310_data *data)
return scale_factors[ilog2(rc)];
}
+/* Called with lock held */
Perhaps add this to your comment: Returns a negative value on error, a
positive value when the device is not ready (and may have been reset
due to corruption), and zero when the device is ready.
Good idea.
+static int dps310_check_reset_meas_cfg(struct dps310_data *data, int ready_bit)
+{
+ int en = DPS310_PRS_EN | DPS310_TEMP_EN | DPS310_BACKGROUND;
+ int meas_cfg;
+ int rc = regmap_read(data->regmap, DPS310_MEAS_CFG, &meas_cfg);
+
+ if (rc < 0)
+ return rc;
+
+ if (meas_cfg & ready_bit)
+ return 0;
+
+ if ((meas_cfg & en) != en) {
+ /* DPS310 register state corrupt, better start from scratch */
+ rc = regmap_write(data->regmap, DPS310_RESET,
+ DPS310_RESET_MAGIC);
+ if (rc < 0)
+ return rc;
+
+ /* Wait for device chip access: 2.5ms in specification */
+ usleep_range(2500, 12000);
+ rc = dps310_startup(data);
+ if (rc)
+ return rc;
+
+ dev_info(&data->client->dev,
+ "recovered from corrupted MEAS_CFG=%02x\n", meas_cfg);
+ }
+
+ return 1;
I'm confused about this case. We get there when the device doesn't
have ready_bit set in meas_cfg and we've done a reset, but we also get
here when the bit isn't set and we haven't done anything to resolve it
(after re-reading the code I understand now, but perhaps reworking it
as follows will make it clear):
Could we write it like this:
if (meas_cfg & ready_bit) {
/* Device ready, must be okay */
return 0;
}
if (meas_cfg & en) {
/* Device okay (but not ready), no action required */
return 1;
}
/* DPS310 register state corrupt, better start from scratch */
...
return 1;
Yea it could be clearer, I can update that.
+}
+
static int dps310_read_pres_raw(struct dps310_data *data)
{
int rc;
@@ -409,15 +442,25 @@ 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);
- timeout = DPS310_POLL_TIMEOUT_US(rate);
-
- /* Poll for sensor readiness; base the timeout upon the sample rate. */
- rc = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
- ready & DPS310_PRS_RDY,
- DPS310_POLL_SLEEP_US(timeout), timeout);
- if (rc)
- goto done;
+ rc = dps310_check_reset_meas_cfg(data, DPS310_PRS_RDY);
can we do this:
if (rc < 0)
goto done;
if (rc > 0) {
}
The rework I suggest makes it clearer that we've considered the '0'
case, when the device is ready before this code runs.
Sure. Thanks for the review, I'll get a v3 up.
Thanks,
Eddie
+ if (rc) {
+ if (rc < 0)
+ goto done;
+
+ rate = dps310_get_pres_samp_freq(data);
+ timeout = DPS310_POLL_TIMEOUT_US(rate);
+
+ /*
+ * Poll for sensor readiness; base the timeout upon the sample
+ * rate.
+ */
+ rc = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG,
+ ready, ready & DPS310_PRS_RDY,
+ DPS310_POLL_SLEEP_US(timeout),
+ timeout);
+ if (rc)
+ goto done;
+ }
rc = regmap_bulk_read(data->regmap, DPS310_PRS_BASE, val, sizeof(val));
if (rc < 0)
@@ -458,15 +501,25 @@ 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);
- timeout = DPS310_POLL_TIMEOUT_US(rate);
-
- /* Poll for sensor readiness; base the timeout upon the sample rate. */
- rc = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
- ready & DPS310_TMP_RDY,
- DPS310_POLL_SLEEP_US(timeout), timeout);
- if (rc < 0)
- goto done;
+ rc = dps310_check_reset_meas_cfg(data, DPS310_TMP_RDY);
+ if (rc) {
+ if (rc < 0)
+ goto done;
+
+ rate = dps310_get_temp_samp_freq(data);
+ timeout = DPS310_POLL_TIMEOUT_US(rate);
+
+ /*
+ * Poll for sensor readiness; base the timeout upon the sample
+ * rate.
+ */
+ rc = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG,
+ ready, ready & DPS310_TMP_RDY,
+ DPS310_POLL_SLEEP_US(timeout),
+ timeout);
+ if (rc < 0)
+ goto done;
+ }
rc = dps310_read_temp_ready(data);
--
2.27.0
