iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt

PATCH Superseded Eddie James 3 participants 6 messages May 18, 2022 – May 24, 2022

Subsystems (2)

IIO Infineon DPS310 Driver

lore

Share & tools

lore

Eddie James

author maintainer

2022-05-18 14:49 UTC #0 lore raw

v2 cover iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt

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. Include a patch to move the startup procedure into a function.

Changes since v1:
- Separate into two patches
- Rename 'dps310_verify_meas_cfg' to 'dps310_check_reset_meas_cfg'

Eddie James (2):
iio: pressure: dps310: Refactor startup procedure
iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt

 drivers/iio/pressure/dps310.c | 281 +++++++++++++++++++++-------------
 1 file changed, 174 insertions(+), 107 deletions(-)

Signature · 1 line

2.27.0

Eddie James

author maintainer

2022-05-18 14:49 UTC #1 lore raw

PATCH v2 1/2 iio: pressure: dps310: Refactor startup procedure

Move the startup procedure into a function, and correct a missing check on the return code for writing the PRS_CFG register.

Signed-off-by Eddie James <[email protected]>

---
 drivers/iio/pressure/dps310.c | 192 ++++++++++++++++++----------------
 1 file changed, 103 insertions(+), 89 deletions(-)

drivers/iio/pressure/dps310.c +103 ~~-89~~

index 36fb7ae0d0a9..f79b274bb38d 100644
@@ -159,6 +159,106 @@ static int dps310_get_coefs(struct dps310_data *data)
 	return 0;
 }
 
+/*
+ * Some verions of chip will read temperatures in the ~60C range when
+ * its actually ~20C. This is the manufacturer recommended workaround
+ * to correct the issue. The registers used below are undocumented.
+ */
+static int dps310_temp_workaround(struct dps310_data *data)
+{
+	int rc;
+	int reg;
+
+	rc = regmap_read(data->regmap, 0x32, &reg);
+	if (rc < 0)
+		return rc;
+
+	/*
+	 * If bit 1 is set then the device is okay, and the workaround does not
+	 * need to be applied
+	 */
+	if (reg & BIT(1))
+		return 0;
+
+	rc = regmap_write(data->regmap, 0x0e, 0xA5);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_write(data->regmap, 0x0f, 0x96);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_write(data->regmap, 0x62, 0x02);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_write(data->regmap, 0x0e, 0x00);
+	if (rc < 0)
+		return rc;
+
+	return regmap_write(data->regmap, 0x0f, 0x00);
+}
+
+static int dps310_startup(struct dps310_data *data)
+{
+	int rc;
+	int ready;
+
+	/*
+	 * Set up pressure sensor in single sample, one measurement per second
+	 * mode
+	 */
+	rc = regmap_write(data->regmap, DPS310_PRS_CFG, 0);
+	if (rc < 0)
+		return rc;
+
+	/*
+	 * Set up external (MEMS) temperature sensor in single sample, one
+	 * measurement per second mode
+	 */
+	rc = regmap_write(data->regmap, DPS310_TMP_CFG, DPS310_TMP_EXT);
+	if (rc < 0)
+		return rc;
+
+	/* Temp and pressure shifts are disabled when PRC <= 8 */
+	rc = regmap_write_bits(data->regmap, DPS310_CFG_REG,
+			       DPS310_PRS_SHIFT_EN | DPS310_TMP_SHIFT_EN, 0);
+	if (rc < 0)
+		return rc;
+
+	/* MEAS_CFG doesn't update correctly unless first written with 0 */
+	rc = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
+			       DPS310_MEAS_CTRL_BITS, 0);
+	if (rc < 0)
+		return rc;
+
+	/* Turn on temperature and pressure measurement in the background */
+	rc = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
+			       DPS310_MEAS_CTRL_BITS, DPS310_PRS_EN |
+			       DPS310_TEMP_EN | DPS310_BACKGROUND);
+	if (rc < 0)
+		return rc;
+
+	/*
+	 * Calibration coefficients required for reporting temperature.
+	 * They are available 40ms after the device has started
+	 */
+	rc = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
+				      ready & DPS310_COEF_RDY, 10000, 40000);
+	if (rc < 0)
+		return rc;
+
+	rc = dps310_get_coefs(data);
+	if (rc < 0)
+		return rc;
+
+	rc = dps310_temp_workaround(data);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
 static int dps310_get_pres_precision(struct dps310_data *data)
 {
 	int rc;

@@ -677,52 +777,12 @@ static const struct iio_info dps310_info = {
 	.write_raw = dps310_write_raw,
 };
 
-/*
- * Some verions of chip will read temperatures in the ~60C range when
- * its actually ~20C. This is the manufacturer recommended workaround
- * to correct the issue. The registers used below are undocumented.
- */
-static int dps310_temp_workaround(struct dps310_data *data)
-{
-	int rc;
-	int reg;
-
-	rc = regmap_read(data->regmap, 0x32, &reg);
-	if (rc < 0)
-		return rc;
-
-	/*
-	 * If bit 1 is set then the device is okay, and the workaround does not
-	 * need to be applied
-	 */
-	if (reg & BIT(1))
-		return 0;
-
-	rc = regmap_write(data->regmap, 0x0e, 0xA5);
-	if (rc < 0)
-		return rc;
-
-	rc = regmap_write(data->regmap, 0x0f, 0x96);
-	if (rc < 0)
-		return rc;
-
-	rc = regmap_write(data->regmap, 0x62, 0x02);
-	if (rc < 0)
-		return rc;
-
-	rc = regmap_write(data->regmap, 0x0e, 0x00);
-	if (rc < 0)
-		return rc;
-
-	return regmap_write(data->regmap, 0x0f, 0x00);
-}
-
 static int dps310_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct dps310_data *data;
 	struct iio_dev *iio;
-	int rc, ready;
+	int rc;
 
 	iio = devm_iio_device_alloc(&client->dev,  sizeof(*data));
 	if (!iio)

@@ -747,54 +807,8 @@ static int dps310_probe(struct i2c_client *client,
 	if (rc)
 		return rc;
 
-	/*
-	 * Set up pressure sensor in single sample, one measurement per second
-	 * mode
-	 */
-	rc = regmap_write(data->regmap, DPS310_PRS_CFG, 0);
-
-	/*
-	 * Set up external (MEMS) temperature sensor in single sample, one
-	 * measurement per second mode
-	 */
-	rc = regmap_write(data->regmap, DPS310_TMP_CFG, DPS310_TMP_EXT);
-	if (rc < 0)
-		return rc;
-
-	/* Temp and pressure shifts are disabled when PRC <= 8 */
-	rc = regmap_write_bits(data->regmap, DPS310_CFG_REG,
-			       DPS310_PRS_SHIFT_EN | DPS310_TMP_SHIFT_EN, 0);
-	if (rc < 0)
-		return rc;
-
-	/* MEAS_CFG doesn't update correctly unless first written with 0 */
-	rc = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
-			       DPS310_MEAS_CTRL_BITS, 0);
-	if (rc < 0)
-		return rc;
-
-	/* Turn on temperature and pressure measurement in the background */
-	rc = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
-			       DPS310_MEAS_CTRL_BITS, DPS310_PRS_EN |
-			       DPS310_TEMP_EN | DPS310_BACKGROUND);
-	if (rc < 0)
-		return rc;
-
-	/*
-	 * Calibration coefficients required for reporting temperature.
-	 * They are available 40ms after the device has started
-	 */
-	rc = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
-				      ready & DPS310_COEF_RDY, 10000, 40000);
-	if (rc < 0)
-		return rc;
-
-	rc = dps310_get_coefs(data);
-	if (rc < 0)
-		return rc;
-
-	rc = dps310_temp_workaround(data);
-	if (rc < 0)
+	rc = dps310_startup(data);
+	if (rc)
 		return rc;
 
 	rc = devm_iio_device_register(&client->dev, iio);

Signature · 1 line

2.27.0

Eddie James

author maintainer

2022-05-18 14:49 UTC #2 lore raw

PATCH v2 2/2 iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt

In reply to Eddie James

Fixes ba6ec48e76bc ("iio: Add driver for Infineon DPS310")

Signed-off-by Eddie James <[email protected]>

---
 drivers/iio/pressure/dps310.c | 89 ++++++++++++++++++++++++++++-------
 1 file changed, 71 insertions(+), 18 deletions(-)

drivers/iio/pressure/dps310.c +71 ~~-18~~

index f79b274bb38d..c6d02679ef33 100644
@@ -397,6 +397,39 @@ static int dps310_get_temp_k(struct dps310_data *data)
 	return scale_factors[ilog2(rc)];
 }
 
+/* Called with lock held */
+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;
+}
+
 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);
+	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);

Signature · 1 line

2.27.0

Joel Stanley

2022-05-24 02:13 UTC #3 lore raw

Quoted 5 lines · Eddie James

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?

Are we ok to sleep for 2.5ms in the iio_info->read_raw callback?

Quoted 12 lines

Fixes ba6ec48e76bc ("iio: Add driver for Infineon DPS310")
Signed-off-by Eddie James <[email protected]>
---
 drivers/iio/pressure/dps310.c | 89 ++++++++++++++++++++++++++++-------
 1 file changed, 71 insertions(+), 18 deletions(-)
drivers/iio/pressure/dps310.c +0 -0
index f79b274bb38d..c6d02679ef33 100644

drivers/iio/pressure/dps310.c static int dps310_get_temp_k(struct dps310_data *data) · line 400

Quoted 4 lines

drivers/iio/pressure/dps310.c +1 -0

@@ -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.

Quoted 20 lines

drivers/iio/pressure/dps310.c +20 -0

+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 */

drivers/iio/pressure/dps310.c static int dps310_get_temp_k(struct dps310_data *data) · line 430

Quoted 10 lines

drivers/iio/pressure/dps310.c +10 -0

+               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;

Quoted 9 lines

drivers/iio/pressure/dps310.c +2 -0

+}
+
 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;

drivers/iio/pressure/dps310.c static int dps310_read_pres_raw(struct dps310_data *data) · line 445

Quoted 10 lines

drivers/iio/pressure/dps310.c +1 -9

-       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.

Quoted 59 lines

drivers/iio/pressure/dps310.c +37 ~~-10~~

+       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

Eddie James

author maintainer

2022-05-24 14:18 UTC #4 lore raw

Quoted 5 lines · Joel Stanley

Quoted 4 lines · Eddie James
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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 16 lines

Quoted 16 lines
Fixes ba6ec48e76bc ("iio: Add driver for Infineon DPS310")
Signed-off-by Eddie James <[email protected]>
---
  drivers/iio/pressure/dps310.c | 89 ++++++++++++++++++++++++++++-------
  1 file changed, 71 insertions(+), 18 deletions(-)
drivers/iio/pressure/dps310.c +0 -0
index f79b274bb38d..c6d02679ef33 100644
@@ -397,6 +397,39 @@ static int dps310_get_temp_k(struct dps310_data *data)
         return scale_factors[ilog2(rc)];
  }
 

Quoted 4 lines

drivers/iio/pressure/dps310.c static int dps310_get_temp_k(struct dps310_data *data) · line 400
Quoted 1 line
drivers/iio/pressure/dps310.c +1 -0
+/* 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.

Quoted 44 lines

Quoted 21 lines

drivers/iio/pressure/dps310.c +20 -0

+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 */

drivers/iio/pressure/dps310.c static int dps310_get_temp_k(struct dps310_data *data) · line 430

Quoted 10 lines

drivers/iio/pressure/dps310.c +10 -0

+               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;

Could we write it like this:

if (meas_cfg & ready_bit) {
   /* Device ready, must be okay */
   return 0;
}

Quoted 8 lines

  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.

Quoted 24 lines

Quoted 11 lines

drivers/iio/pressure/dps310.c +2 -0

+}
+
  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;

drivers/iio/pressure/dps310.c static int dps310_read_pres_raw(struct dps310_data *data) · line 445

Quoted 10 lines

drivers/iio/pressure/dps310.c +1 -9

-       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)

Quoted 8 lines

    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

Quoted 60 lines

drivers/iio/pressure/dps310.c +37 ~~-10~~

+       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

Jonathan Cameron

maintainer

2022-05-22 11:32 UTC #5 lore raw

In reply to Eddie James

Quoted 9 lines · On Wed, 18 May 2022 09:48:16 -0500 Eddie James <[email protected]> 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. Include a patch to move the startup procedure into a function.
Changes since v1:
- Separate into two patches
- Rename 'dps310_verify_meas_cfg' to 'dps310_check_reset_meas_cfg'

Looks good to me. I'll leave this a little longer on list to give Joel and others the opportunity to take a look.

If I seem to have lost it in a few weeks time, feel free to ping me (it's been known to happen though I think my tracking is much less error prone now I have moved to patchwork + my local system).

Jonathan

Quoted 8 lines

Eddie James (2):
iio: pressure: dps310: Refactor startup procedure
iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt
 drivers/iio/pressure/dps310.c | 281 +++++++++++++++++++++-------------
 1 file changed, 174 insertions(+), 107 deletions(-)