Messages in this thread | | | Date | Mon, 15 Aug 2022 08:42:56 -0500 | Subject | Re: [PATCH v4 1/2] iio: pressure: dps310: Refactor startup procedure | From | Eddie James <> |
| |
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 >> >
| |