| Message ID | 20220815145705.203017-3-eajames@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt | expand |
On Mon, Aug 15, 2022 at 5:57 PM Eddie James <eajames@linux.ibm.com> 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. ... > + * Called with lock held. Returns a negative value on error, a positive value > + * when the device is not ready, and zero when the device is ready. Can we have #define DPS310_DEVICE_NOT_READY 1 (or anonymous enum) and return it instead of abstract 1 or any other positive number? ... > + rc = dps310_check_reset_meas_cfg(data, DPS310_TMP_RDY); > if (rc < 0) > goto done; > > + if (rc > 0) { > + 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) > + goto done; > + } But have you tried to make a helper that takes a pointer to the respective function?
On Thu, 18 Aug 2022 23:16:55 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Aug 15, 2022 at 5:57 PM Eddie James <eajames@linux.ibm.com> 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. > > ... > > > + * Called with lock held. Returns a negative value on error, a positive value > > + * when the device is not ready, and zero when the device is ready. > > Can we have > > #define DPS310_DEVICE_NOT_READY 1 > > (or anonymous enum) and return it instead of abstract 1 or any other > positive number? Perhaps make it even clearer by returning the need to wait via another parameter rather than this being (probably) the only place in driver with a postive rc. bool reset_done; rc = dps310_check_reset_meas_cfg(data, DPS310_TMP_RDY, &reset_done); if (reset_done) { } > > ... > > > + rc = dps310_check_reset_meas_cfg(data, DPS310_TMP_RDY); > > if (rc < 0) > > goto done; > > > > + if (rc > 0) { > > + 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) > > + goto done; > > + } > > But have you tried to make a helper that takes a pointer to the > respective function? >
On 8/20/22 07:02, Jonathan Cameron wrote: > On Thu, 18 Aug 2022 23:16:55 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Mon, Aug 15, 2022 at 5:57 PM Eddie James <eajames@linux.ibm.com> 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. >> ... >> >>> + * Called with lock held. Returns a negative value on error, a positive value >>> + * when the device is not ready, and zero when the device is ready. >> Can we have >> >> #define DPS310_DEVICE_NOT_READY 1 >> >> (or anonymous enum) and return it instead of abstract 1 or any other >> positive number? > Perhaps make it even clearer by returning the need to wait via another parameter > rather than this being (probably) the only place in driver with a postive rc. > > bool reset_done; > > rc = dps310_check_reset_meas_cfg(data, DPS310_TMP_RDY, &reset_done); > > if (reset_done) { > } Thanks for the feedback. Unfortunately our issue is still presenting itself even with a correct MEAS_CFG register. The sensor ready bit never gets set. So I'm going to rework this patch to reset the device after a timeout. Thanks, Eddie > >> ... >> >>> + rc = dps310_check_reset_meas_cfg(data, DPS310_TMP_RDY); >>> if (rc < 0) >>> goto done; >>> >>> + if (rc > 0) { >>> + 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) >>> + goto done; >>> + } >> But have you tried to make a helper that takes a pointer to the >> respective function? >>
diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c index c706a8b423b5..bbeb2f3bcc8a 100644 --- a/drivers/iio/pressure/dps310.c +++ b/drivers/iio/pressure/dps310.c @@ -393,6 +393,45 @@ static int dps310_get_temp_k(struct dps310_data *data) return scale_factors[ilog2(rc)]; } +/* + * Called with lock held. Returns a negative value on error, a positive value + * when the device is not ready, and zero when the device is ready. + */ +static int dps310_check_reset_meas_cfg(struct dps310_data *data, int ready_bit) +{ + int rc; + int meas_cfg; + + rc = regmap_read(data->regmap, DPS310_MEAS_CFG, &meas_cfg); + if (rc) + return rc; + + /* Device is ready, proceed to measurement */ + if (meas_cfg & ready_bit) + return 0; + + /* Device is OK, just not ready */ + if (meas_cfg & (DPS310_PRS_EN | DPS310_TEMP_EN | DPS310_BACKGROUND)) + return 1; + + /* DPS310 register state corrupt, better start from scratch */ + 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); + + /* Reinitialize the chip */ + 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; @@ -405,16 +444,26 @@ 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) + rc = dps310_check_reset_meas_cfg(data, DPS310_PRS_RDY); + if (rc < 0) goto done; + if (rc > 0) { + 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) goto done; @@ -454,16 +503,26 @@ 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); + rc = dps310_check_reset_meas_cfg(data, DPS310_TMP_RDY); if (rc < 0) goto done; + if (rc > 0) { + 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) + goto done; + } + rc = dps310_read_temp_ready(data); done: