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

PATCH In review Eddie James 4 participants 6 messages Sep 15, 2022 – Sep 18, 2022

Subsystems (3)

Stable branch IIO Infineon DPS310 Driver

lore

Share & tools

lore

Eddie James

author maintainer

2022-09-15 19:57 UTC #0 lore raw

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

The DPS310 chip has been observed to get "stuck" such that pressure and temperature measurements are never indicated as "ready" in the MEAS_CFG register. The only solution is to reset the device and try again. In order to avoid continual failures, use a boolean flag to only try the reset after timeout once if errors persist. Include a patch to move the startup procedure into a function.

Changes since v7:
- Condense the code a bit by dropping rc2

Changes since v6:
- Use helper instead of the lengthy regmap_read_poll_timeout twice
- Just return dps310_startup in dps310_reset_reinit

Changes since v5:
- Completely rework the second patch to reset and reinit in any timeout condition, if there haven't been previous timeouts that failed to recover the chip.

Changes since v4:
- Just check for rc rather than rc < 0 in some cases
- Split declaration and init of rc

Changes since v3:
- Don't check regmap* return codes for < 0
- Fix comment spelling

Changes since v2:
- Add some comments
- Fix the clunky control flow

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 after timeout

 drivers/iio/pressure/dps310.c | 262 +++++++++++++++++++++-------------
 1 file changed, 163 insertions(+), 99 deletions(-)

Signature · 1 line

2.31.1

Eddie James

author maintainer

2022-09-15 19:57 UTC #1 lore raw

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

Cc <[email protected]>

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

Reviewed-by Joel Stanley <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

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

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

index 36fb7ae0d0a9..c706a8b423b5 100644
@@ -159,6 +159,102 @@ static int dps310_get_coefs(struct dps310_data *data)
 	return 0;
 }
 
+/*
+ * Some versions of the chip will read temperatures in the ~60C range when
+ * it's 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)
+		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)
+		return rc;
+
+	rc = regmap_write(data->regmap, 0x0f, 0x96);
+	if (rc)
+		return rc;
+
+	rc = regmap_write(data->regmap, 0x62, 0x02);
+	if (rc)
+		return rc;
+
+	rc = regmap_write(data->regmap, 0x0e, 0x00);
+	if (rc)
+		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)
+		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)
+		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)
+		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)
+		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)
+		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)
+		return rc;
+
+	rc = dps310_get_coefs(data);
+	if (rc)
+		return rc;
+
+	return dps310_temp_workaround(data);
+}
+
 static int dps310_get_pres_precision(struct dps310_data *data)
 {
 	int rc;

@@ -677,52 +773,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 +803,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.31.1

Eddie James

author maintainer

2022-09-15 19:57 UTC #2 lore raw

PATCH v8 2/2 iio: pressure: dps310: Reset chip after timeout

In reply to Eddie James

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

Cc <[email protected]>

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

Reviewed-by Andy Shevchenko <[email protected]>

---
 drivers/iio/pressure/dps310.c | 74 ++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 10 deletions(-)

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

index c706a8b423b5..984a3f511a1a 100644
@@ -89,6 +89,7 @@ struct dps310_data {
 	s32 c00, c10, c20, c30, c01, c11, c21;
 	s32 pressure_raw;
 	s32 temp_raw;
+	bool timeout_recovery_failed;
 };
 
 static const struct iio_chan_spec dps310_channels[] = {

@@ -393,11 +394,69 @@ static int dps310_get_temp_k(struct dps310_data *data)
 	return scale_factors[ilog2(rc)];
 }
 
+static int dps310_reset_wait(struct dps310_data *data)
+{
+	int rc;
+
+	rc = regmap_write(data->regmap, DPS310_RESET, DPS310_RESET_MAGIC);
+	if (rc)
+		return rc;
+
+	/* Wait for device chip access: 2.5ms in specification */
+	usleep_range(2500, 12000);
+	return 0;
+}
+
+static int dps310_reset_reinit(struct dps310_data *data)
+{
+	int rc;
+
+	rc = dps310_reset_wait(data);
+	if (rc)
+		return rc;
+
+	return dps310_startup(data);
+}
+
+static int dps310_ready_status(struct dps310_data *data, int ready_bit, int timeout)
+{
+	int sleep = DPS310_POLL_SLEEP_US(timeout);
+	int ready;
+
+	return regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready, ready & ready_bit,
+					sleep, timeout);
+}
+
+static int dps310_ready(struct dps310_data *data, int ready_bit, int timeout)
+{
+	int rc;
+
+	rc = dps310_ready_status(data, ready_bit, timeout);
+	if (rc) {
+		if (rc == -ETIMEDOUT && !data->timeout_recovery_failed) {
+			/* Reset and reinitialize the chip. */
+			if (dps310_reset_reinit(data)) {
+				data->timeout_recovery_failed = true;
+			} else {
+				/* Try again to get sensor ready status. */
+				if (dps310_ready_status(data, ready_bit, timeout))
+					data->timeout_recovery_failed = true;
+				else
+					return 0;
+			}
+		}
+
+		return rc;
+	}
+
+	data->timeout_recovery_failed = false;
+	return 0;
+}
+
 static int dps310_read_pres_raw(struct dps310_data *data)
 {
 	int rc;
 	int rate;
-	int ready;
 	int timeout;
 	s32 raw;
 	u8 val[3];

@@ -409,9 +468,7 @@ static int dps310_read_pres_raw(struct dps310_data *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);
+	rc = dps310_ready(data, DPS310_PRS_RDY, timeout);
 	if (rc)
 		goto done;

@@ -448,7 +505,6 @@ static int dps310_read_temp_raw(struct dps310_data *data)
 {
 	int rc;
 	int rate;
-	int ready;
 	int timeout;
 
 	if (mutex_lock_interruptible(&data->lock))

@@ -458,10 +514,8 @@ static int dps310_read_temp_raw(struct dps310_data *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)
+	rc = dps310_ready(data, DPS310_TMP_RDY, timeout);
+	if (rc)
 		goto done;
 
 	rc = dps310_read_temp_ready(data);

@@ -756,7 +810,7 @@ static void dps310_reset(void *action_data)
 {
 	struct dps310_data *data = action_data;
 
-	regmap_write(data->regmap, DPS310_RESET, DPS310_RESET_MAGIC);
+	dps310_reset_wait(data);
 }
 
 static const struct regmap_config dps310_regmap_config = {

Signature · 1 line

2.31.1

Andy Shevchenko

2022-09-16 05:51 UTC #3 lore raw

Re: [PATCH v8 2/2] iio: pressure: dps310: Reset chip after timeout

Quoted 6 lines · Eddie James

The DPS310 chip has been observed to get "stuck" such that pressure and temperature measurements are never indicated as "ready" in the MEAS_CFG register. The only solution is to reset the device and try again. In order to avoid continual failures, use a boolean flag to only try the reset after timeout once if errors persist.

...

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

Quoted 8 lines

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

+static int dps310_ready(struct dps310_data *data, int ready_bit, int timeout)
+{
+       int rc;
+
+       rc = dps310_ready_status(data, ready_bit, timeout);
+       if (rc) {
 
+               if (rc == -ETIMEDOUT && !data->timeout_recovery_failed) {

Here you compare rc with a certain error code...

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

Quoted 7 lines

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

+                       /* Reset and reinitialize the chip. */
+                       if (dps310_reset_reinit(data)) {
+                               data->timeout_recovery_failed = true;
+                       } else {
+                               /* Try again to get sensor ready status. */
 
+                               if (dps310_ready_status(data, ready_bit, timeout))

...but here you assume that the only error code is -ETIMEDOUT. It's kinda inconsistent (if you rely on internals of ready_status, then why to check the certain error code, or otherwise why not to return a real second error code). That's why I asked about re-using rc here.

In any case I don't think this justifies a v9, let's wait for your answer and Jonathan's opinion.

Quoted 12 lines

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

+                                       data->timeout_recovery_failed = true;
+                               else
+                                       return 0;
+                       }
+               }
+
+               return rc;
+       }
+
+       data->timeout_recovery_failed = false;
+       return 0;
+}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jonathan Cameron

2022-09-16 15:25 UTC #4 lore raw

Re: [PATCH v8 2/2] iio: pressure: dps310: Reset chip after timeout

Quoted 27 lines · On Fri, 16 Sep 2022 08:51:13 +0300 Andy Shevchenko <[email protected]>…

Quoted 6 lines · Eddie James
The DPS310 chip has been observed to get "stuck" such that pressure and temperature measurements are never indicated as "ready" in the MEAS_CFG register. The only solution is to reset the device and try again. In order to avoid continual failures, use a boolean flag to only try the reset after timeout once if errors persist.
...
drivers/iio/pressure/dps310.c static int dps310_get_temp_k(struct dps310_data *data) · line 436
Quoted 8 lines
drivers/iio/pressure/dps310.c +7 -0
+static int dps310_ready(struct dps310_data *data, int ready_bit, int timeout)
+{
+       int rc;
+
+       rc = dps310_ready_status(data, ready_bit, timeout);
+       if (rc) {  
 
+               if (rc == -ETIMEDOUT && !data->timeout_recovery_failed) {  
Here you compare rc with a certain error code...
Quoted 5 lines
drivers/iio/pressure/dps310.c +5 -0
+                       /* Reset and reinitialize the chip. */
+                       if (dps310_reset_reinit(data)) {
+                               data->timeout_recovery_failed = true;
+                       } else {
+                               /* Try again to get sensor ready status. */  

Quoted 6 lines

Quoted 1 line
+ if (dps310_ready_status(data, ready_bit, timeout))
...but here you assume that the only error code is -ETIMEDOUT. It's kinda inconsistent (if you rely on internals of ready_status, then why to check the certain error code, or otherwise why not to return a real second error code). That's why I asked about re-using rc here.

Hmm.

1st time around, any other error code would result in us just returning directly which is fine.
2nd time around I'm not sure we care about what the error code is. Even if something else went wrong we failed to recover and the first error that lead to that was -ETIMEDOUT.

So I think this is correct as is, be it a little unusual!

Jonathan

Quoted 17 lines

In any case I don't think this justifies a v9, let's wait for your answer and Jonathan's opinion.

Quoted 12 lines

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

+                                       data->timeout_recovery_failed = true;
+                               else
+                                       return 0;
+                       }
+               }
+
+               return rc;
+       }
+
+       data->timeout_recovery_failed = false;
+       return 0;
+}

Jonathan Cameron

maintainer

2022-09-18 15:52 UTC #5 lore raw

Re: [PATCH v8 2/2] iio: pressure: dps310: Reset chip after timeout

Quoted 39 lines · On Fri, 16 Sep 2022 16:25:35 +0100 Jonathan Cameron…

Quoted 27 lines · On Fri, 16 Sep 2022 08:51:13 +0300 Andy Shevchenko <[email protected]>…
Quoted 6 lines · Eddie James
The DPS310 chip has been observed to get "stuck" such that pressure and temperature measurements are never indicated as "ready" in the MEAS_CFG register. The only solution is to reset the device and try again. In order to avoid continual failures, use a boolean flag to only try the reset after timeout once if errors persist.
...
drivers/iio/pressure/dps310.c static int dps310_get_temp_k(struct dps310_data *data) · line 436
Quoted 8 lines
drivers/iio/pressure/dps310.c +7 -0
+static int dps310_ready(struct dps310_data *data, int ready_bit, int timeout)
+{
+       int rc;
+
+       rc = dps310_ready_status(data, ready_bit, timeout);
+       if (rc) {    
 
+               if (rc == -ETIMEDOUT && !data->timeout_recovery_failed) {    
Here you compare rc with a certain error code...
Quoted 5 lines
drivers/iio/pressure/dps310.c +5 -0
+                       /* Reset and reinitialize the chip. */
+                       if (dps310_reset_reinit(data)) {
+                               data->timeout_recovery_failed = true;
+                       } else {
+                               /* Try again to get sensor ready status. */    
Quoted 6 lines
Quoted 1 line
+ if (dps310_ready_status(data, ready_bit, timeout))
...but here you assume that the only error code is -ETIMEDOUT. It's kinda inconsistent (if you rely on internals of ready_status, then why to check the certain error code, or otherwise why not to return a real second error code). That's why I asked about re-using rc here.
Hmm.

Quoted 7 lines

1st time around, any other error code would result in us just returning directly which is fine.
2nd time around I'm not sure we care about what the error code is. Even if something else went wrong we failed to recover and the first error that lead to that was -ETIMEDOUT.
So I think this is correct as is, be it a little unusual!

Given timing late in the cycle, I've queued this up for the next merge window rather than rushing it in.

Applied to the togreg branch of iio.git and pushed out as testing. Note I plan to rebase that branch shortly as I need some stuff that is in upstream for other series. Hence still time for this discussion to continue if anyone wants to!

Thanks,

Jonathan

Quoted 21 lines

Jonathan

Quoted 17 lines

In any case I don't think this justifies a v9, let's wait for your answer and Jonathan's opinion.

Quoted 12 lines

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

+                                       data->timeout_recovery_failed = true;
+                               else
+                                       return 0;
+                       }
+               }
+
+               return rc;
+       }
+
+       data->timeout_recovery_failed = false;
+       return 0;
+}