* [PATCH v4 0/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt @ 2022-08-09 21:12 Eddie James 2022-08-09 21:12 ` [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure Eddie James 2022-08-09 21:12 ` [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt Eddie James 0 siblings, 2 replies; 12+ messages in thread From: Eddie James @ 2022-08-09 21:12 UTC (permalink / raw) To: linux-iio; +Cc: jic23, lars, linux-kernel, andy.shevchenko, joel, eajames 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 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 if MEAS_CFG is corrupt drivers/iio/pressure/dps310.c | 276 +++++++++++++++++++++------------- 1 file changed, 172 insertions(+), 104 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure 2022-08-09 21:12 [PATCH v4 0/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt Eddie James @ 2022-08-09 21:12 ` Eddie James 2022-08-12 22:03 ` Andy Shevchenko 2022-08-09 21:12 ` [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt Eddie James 1 sibling, 1 reply; 12+ messages in thread From: Eddie James @ 2022-08-09 21:12 UTC (permalink / raw) To: linux-iio; +Cc: jic23, lars, linux-kernel, andy.shevchenko, joel, eajames 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 <eajames@linux.ibm.com> Reviewed-by: Joel Stanley <joel@jms.id.au> --- drivers/iio/pressure/dps310.c | 188 ++++++++++++++++++---------------- 1 file changed, 99 insertions(+), 89 deletions(-) diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c index 36fb7ae0d0a9..c706a8b423b5 100644 --- a/drivers/iio/pressure/dps310.c +++ b/drivers/iio/pressure/dps310.c @@ -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, ®); + 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, ®); - 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); -- 2.31.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure 2022-08-09 21:12 ` [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure Eddie James @ 2022-08-12 22:03 ` Andy Shevchenko 2022-08-15 13:42 ` Eddie James 0 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2022-08-12 22:03 UTC (permalink / raw) To: Eddie James Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On Wed, Aug 10, 2022 at 12:12 AM Eddie James <eajames@linux.ibm.com> wrote: > > Move the startup procedure into a function, and correct a missing > check on the return code for writing the PRS_CFG register. Code looks good, one remark below. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Eddie James <eajames@linux.ibm.com> > Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > drivers/iio/pressure/dps310.c | 188 ++++++++++++++++++---------------- > 1 file changed, 99 insertions(+), 89 deletions(-) > > diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c > index 36fb7ae0d0a9..c706a8b423b5 100644 > --- a/drivers/iio/pressure/dps310.c > +++ b/drivers/iio/pressure/dps310.c > @@ -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, ®); > + 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; This code already exists, but still want to ask, is it really byte-registers here and not be16/le16 one? In such a case perhaps bulk write can be used to reflect it better? > + 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); Ditto. > +} > + > +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, ®); > - 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); > -- > 2.31.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure 2022-08-12 22:03 ` Andy Shevchenko @ 2022-08-15 13:42 ` Eddie James 2022-08-19 9:42 ` Andy Shevchenko 0 siblings, 1 reply; 12+ messages in thread From: Eddie James @ 2022-08-15 13:42 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On 8/12/22 17:03, Andy Shevchenko wrote: > On Wed, Aug 10, 2022 at 12:12 AM Eddie James <eajames@linux.ibm.com> wrote: >> Move the startup procedure into a function, and correct a missing >> check on the return code for writing the PRS_CFG register. > Code looks good, one remark below. > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> Signed-off-by: Eddie James <eajames@linux.ibm.com> >> Reviewed-by: Joel Stanley <joel@jms.id.au> >> --- >> drivers/iio/pressure/dps310.c | 188 ++++++++++++++++++---------------- >> 1 file changed, 99 insertions(+), 89 deletions(-) >> >> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c >> index 36fb7ae0d0a9..c706a8b423b5 100644 >> --- a/drivers/iio/pressure/dps310.c >> +++ b/drivers/iio/pressure/dps310.c >> @@ -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, ®); >> + 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; > This code already exists, but still want to ask, is it really > byte-registers here and not be16/le16 one? In such a case perhaps bulk > write can be used to reflect it better? The temperature and pressure regs are 24 bits big endian, and all the rest are 8 bits. I think the existing approach is best. Thanks Andy! > >> + 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); > Ditto. > >> +} >> + >> +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, ®); >> - 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); >> -- >> 2.31.1 >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure 2022-08-15 13:42 ` Eddie James @ 2022-08-19 9:42 ` Andy Shevchenko 2022-08-20 11:49 ` Jonathan Cameron 0 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2022-08-19 9:42 UTC (permalink / raw) To: Eddie James Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On Mon, Aug 15, 2022 at 4:42 PM Eddie James <eajames@linux.ibm.com> wrote: > On 8/12/22 17:03, Andy Shevchenko wrote: > > On Wed, Aug 10, 2022 at 12:12 AM Eddie James <eajames@linux.ibm.com> wrote: ... > >> + rc = regmap_write(data->regmap, 0x0e, 0xA5); > >> + if (rc) > >> + return rc; > >> + > >> + rc = regmap_write(data->regmap, 0x0f, 0x96); > >> + if (rc) > >> + return rc; > > This code already exists, but still want to ask, is it really > > byte-registers here and not be16/le16 one? In such a case perhaps bulk > > write can be used to reflect it better? > > The temperature and pressure regs are 24 bits big endian, and all the > rest are 8 bits. I think the existing approach is best. It doesn't look like you got what I was meaning... Or I misunderstood what you said. The code above writes two byte values to two sequential registers which make me think that they are 16-bit registers at offset 0x0e. ... > >> + rc = regmap_write(data->regmap, 0x0e, 0x00); > >> + if (rc) > >> + return rc; > >> + > >> + return regmap_write(data->regmap, 0x0f, 0x00); Ditto. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure 2022-08-19 9:42 ` Andy Shevchenko @ 2022-08-20 11:49 ` Jonathan Cameron 2022-09-06 19:48 ` Eddie James 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Cameron @ 2022-08-20 11:49 UTC (permalink / raw) To: Andy Shevchenko Cc: Eddie James, linux-iio, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On Fri, 19 Aug 2022 12:42:00 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Aug 15, 2022 at 4:42 PM Eddie James <eajames@linux.ibm.com> wrote: > > On 8/12/22 17:03, Andy Shevchenko wrote: > > > On Wed, Aug 10, 2022 at 12:12 AM Eddie James <eajames@linux.ibm.com> wrote: > > ... > > > >> + rc = regmap_write(data->regmap, 0x0e, 0xA5); > > >> + if (rc) > > >> + return rc; > > >> + > > >> + rc = regmap_write(data->regmap, 0x0f, 0x96); > > >> + if (rc) > > >> + return rc; > > > This code already exists, but still want to ask, is it really > > > byte-registers here and not be16/le16 one? In such a case perhaps bulk > > > write can be used to reflect it better? > > > > The temperature and pressure regs are 24 bits big endian, and all the > > rest are 8 bits. I think the existing approach is best. > > It doesn't look like you got what I was meaning... Or I misunderstood > what you said. > > The code above writes two byte values to two sequential registers > which make me think that they are 16-bit registers at offset 0x0e. Given they are undocumented, this is guessing territory. Probably best to just leave them as is. You could do a bulk write on an array though as that implies nothing about what's in the registers -just that they happen to be next to each other. > > ... > > > >> + rc = regmap_write(data->regmap, 0x0e, 0x00); > > >> + if (rc) > > >> + return rc; > > >> + > > >> + return regmap_write(data->regmap, 0x0f, 0x00); > > Ditto. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure 2022-08-20 11:49 ` Jonathan Cameron @ 2022-09-06 19:48 ` Eddie James 2022-09-11 9:56 ` Jonathan Cameron 0 siblings, 1 reply; 12+ messages in thread From: Eddie James @ 2022-09-06 19:48 UTC (permalink / raw) To: Jonathan Cameron, Andy Shevchenko Cc: linux-iio, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On 8/20/22 06:49, Jonathan Cameron wrote: > On Fri, 19 Aug 2022 12:42:00 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Mon, Aug 15, 2022 at 4:42 PM Eddie James <eajames@linux.ibm.com> wrote: >>> On 8/12/22 17:03, Andy Shevchenko wrote: >>>> On Wed, Aug 10, 2022 at 12:12 AM Eddie James <eajames@linux.ibm.com> wrote: >> ... >> >>>>> + rc = regmap_write(data->regmap, 0x0e, 0xA5); >>>>> + if (rc) >>>>> + return rc; >>>>> + >>>>> + rc = regmap_write(data->regmap, 0x0f, 0x96); >>>>> + if (rc) >>>>> + return rc; >>>> This code already exists, but still want to ask, is it really >>>> byte-registers here and not be16/le16 one? In such a case perhaps bulk >>>> write can be used to reflect it better? >>> The temperature and pressure regs are 24 bits big endian, and all the >>> rest are 8 bits. I think the existing approach is best. >> It doesn't look like you got what I was meaning... Or I misunderstood >> what you said. >> >> The code above writes two byte values to two sequential registers >> which make me think that they are 16-bit registers at offset 0x0e. > Given they are undocumented, this is guessing territory. > Probably best to just leave them as is. > You could do a bulk write on an array though as that implies > nothing about what's in the registers -just that they happen > to be next to each other. Indeed. Is it worth it to switch to bulk write for two 2-byte writes? I'm inclined to say no and will leave this as-is for v6, but if you think it is, I can switch it. Thanks, Eddie > >> ... >> >>>>> + rc = regmap_write(data->regmap, 0x0e, 0x00); >>>>> + if (rc) >>>>> + return rc; >>>>> + >>>>> + return regmap_write(data->regmap, 0x0f, 0x00); >> Ditto. >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure 2022-09-06 19:48 ` Eddie James @ 2022-09-11 9:56 ` Jonathan Cameron 0 siblings, 0 replies; 12+ messages in thread From: Jonathan Cameron @ 2022-09-11 9:56 UTC (permalink / raw) To: Eddie James Cc: Andy Shevchenko, linux-iio, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On Tue, 6 Sep 2022 14:48:20 -0500 Eddie James <eajames@linux.ibm.com> wrote: > On 8/20/22 06:49, Jonathan Cameron wrote: > > On Fri, 19 Aug 2022 12:42:00 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > >> On Mon, Aug 15, 2022 at 4:42 PM Eddie James <eajames@linux.ibm.com> wrote: > >>> On 8/12/22 17:03, Andy Shevchenko wrote: > >>>> On Wed, Aug 10, 2022 at 12:12 AM Eddie James <eajames@linux.ibm.com> wrote: > >> ... > >> > >>>>> + rc = regmap_write(data->regmap, 0x0e, 0xA5); > >>>>> + if (rc) > >>>>> + return rc; > >>>>> + > >>>>> + rc = regmap_write(data->regmap, 0x0f, 0x96); > >>>>> + if (rc) > >>>>> + return rc; > >>>> This code already exists, but still want to ask, is it really > >>>> byte-registers here and not be16/le16 one? In such a case perhaps bulk > >>>> write can be used to reflect it better? > >>> The temperature and pressure regs are 24 bits big endian, and all the > >>> rest are 8 bits. I think the existing approach is best. > >> It doesn't look like you got what I was meaning... Or I misunderstood > >> what you said. > >> > >> The code above writes two byte values to two sequential registers > >> which make me think that they are 16-bit registers at offset 0x0e. > > Given they are undocumented, this is guessing territory. > > Probably best to just leave them as is. > > You could do a bulk write on an array though as that implies > > nothing about what's in the registers -just that they happen > > to be next to each other. > > > Indeed. Is it worth it to switch to bulk write for two 2-byte writes? > I'm inclined to say no and will leave this as-is for v6, but if you > think it is, I can switch it. > As far as I'm concerned, fine either way. Jonathan > > Thanks, > > Eddie > > > > > >> ... > >> > >>>>> + rc = regmap_write(data->regmap, 0x0e, 0x00); > >>>>> + if (rc) > >>>>> + return rc; > >>>>> + > >>>>> + return regmap_write(data->regmap, 0x0f, 0x00); > >> Ditto. > >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt 2022-08-09 21:12 [PATCH v4 0/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt Eddie James 2022-08-09 21:12 ` [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure Eddie James @ 2022-08-09 21:12 ` Eddie James 2022-08-12 22:13 ` Andy Shevchenko 1 sibling, 1 reply; 12+ messages in thread From: Eddie James @ 2022-08-09 21:12 UTC (permalink / raw) To: linux-iio; +Cc: jic23, lars, linux-kernel, andy.shevchenko, joel, eajames 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. Signed-off-by: Eddie James <eajames@linux.ibm.com> Reviewed-by: Joel Stanley <joel@jms.id.au> --- drivers/iio/pressure/dps310.c | 88 +++++++++++++++++++++++++++++------ 1 file changed, 73 insertions(+), 15 deletions(-) diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c index c706a8b423b5..3a1aeeea3cdd 100644 --- a/drivers/iio/pressure/dps310.c +++ b/drivers/iio/pressure/dps310.c @@ -393,6 +393,44 @@ 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 meas_cfg; + int rc = regmap_read(data->regmap, DPS310_MEAS_CFG, &meas_cfg); + + if (rc < 0) + 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 < 0) + 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 +443,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 +502,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 < 0) + goto done; + } + rc = dps310_read_temp_ready(data); done: -- 2.31.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt 2022-08-09 21:12 ` [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt Eddie James @ 2022-08-12 22:13 ` Andy Shevchenko 2022-08-15 13:59 ` Eddie James 0 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2022-08-12 22:13 UTC (permalink / raw) To: Eddie James Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On Wed, Aug 10, 2022 at 12:12 AM 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. Looks like both patches miss the Fixes tag. Can you add them? ... > +/* > + * 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 meas_cfg; > + int rc = regmap_read(data->regmap, DPS310_MEAS_CFG, &meas_cfg); > + > + if (rc < 0) > + return rc; Please, split definition and assignment. > + /* 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 < 0) > + 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 +443,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; > + } If you split the condition body to a helper, it can be rewritten like (also note special definition for positive returned numbers): rc = ..._reset_meas_cfg(...); if (rc == DPS310_MEAS_NOT_READY) rc = ..._new_helper_func(...); if (rc) goto done; and looking at this it might be worth considering calling that conditional in the middle in the _reset_meas_cfg(), so the latter will return either 0 or negative error code. > + 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); Okay, I see this function is different, but still you may realize a helper from below and something like above suggestion can still be achieved. > + 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) Why out of a sudden ' < 0'? > + goto done; > + } As per above. > rc = dps310_read_temp_ready(data); -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt 2022-08-12 22:13 ` Andy Shevchenko @ 2022-08-15 13:59 ` Eddie James 2022-08-20 11:50 ` Jonathan Cameron 0 siblings, 1 reply; 12+ messages in thread From: Eddie James @ 2022-08-15 13:59 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On 8/12/22 17:13, Andy Shevchenko wrote: > On Wed, Aug 10, 2022 at 12:12 AM 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. > Looks like both patches miss the Fixes tag. Can you add them? Well this isn't really a software fix - there's no identifiable bug in the driver. Just trying to recover the chip in this observed mystery scenario. > > ... > >> +/* >> + * 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 meas_cfg; >> + int rc = regmap_read(data->regmap, DPS310_MEAS_CFG, &meas_cfg); >> + >> + if (rc < 0) >> + return rc; > Please, split definition and assignment. Ack. > >> + /* 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 < 0) >> + 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 +443,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; >> + } > If you split the condition body to a helper, it can be rewritten like > (also note special definition for positive returned numbers): > > rc = ..._reset_meas_cfg(...); > if (rc == DPS310_MEAS_NOT_READY) > rc = ..._new_helper_func(...); > if (rc) > goto done; > > and looking at this it might be worth considering calling that > conditional in the middle in the _reset_meas_cfg(), so the latter will > return either 0 or negative error code. To be honest that looks more complicated than the way it is now? And I don't think I can make it common between the temp and pressure without some complicated macro business. > >> + 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); > Okay, I see this function is different, but still you may realize a > helper from below and something like above suggestion can still be > achieved. > >> + 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) > Why out of a sudden ' < 0'? Good point, I'll fix that. > >> + goto done; >> + } > As per above. > >> rc = dps310_read_temp_ready(data); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt 2022-08-15 13:59 ` Eddie James @ 2022-08-20 11:50 ` Jonathan Cameron 0 siblings, 0 replies; 12+ messages in thread From: Jonathan Cameron @ 2022-08-20 11:50 UTC (permalink / raw) To: Eddie James Cc: Andy Shevchenko, linux-iio, Lars-Peter Clausen, Linux Kernel Mailing List, Joel Stanley On Mon, 15 Aug 2022 08:59:03 -0500 Eddie James <eajames@linux.ibm.com> wrote: > On 8/12/22 17:13, Andy Shevchenko wrote: > > On Wed, Aug 10, 2022 at 12:12 AM 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. > > Looks like both patches miss the Fixes tag. Can you add them? > > > Well this isn't really a software fix - there's no identifiable bug in > the driver. Just trying to recover the chip in this observed mystery > scenario. The tag is useful as well for where to backport this to. Probably just tag the driver introduction. Your description makes it clear we aren't finding faults in the driver - just that it didn't cover this undocumented case! Jonathan ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-09-11 10:30 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-09 21:12 [PATCH v4 0/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt Eddie James 2022-08-09 21:12 ` [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure Eddie James 2022-08-12 22:03 ` Andy Shevchenko 2022-08-15 13:42 ` Eddie James 2022-08-19 9:42 ` Andy Shevchenko 2022-08-20 11:49 ` Jonathan Cameron 2022-09-06 19:48 ` Eddie James 2022-09-11 9:56 ` Jonathan Cameron 2022-08-09 21:12 ` [PATCH v4 2/2] iio: pressure: dps310: Reset chip if MEAS_CFG is corrupt Eddie James 2022-08-12 22:13 ` Andy Shevchenko 2022-08-15 13:59 ` Eddie James 2022-08-20 11:50 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).