| Message ID | 20200218102805.21321-1-shrirang.bagul@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,B] UBUNTU: SAUCE: iio: humidity: hts221: Fix sensor reads after resume | expand |
Hi Shrirang, On 18.02.20 11:28, Shrirang Bagul wrote: > BugLink: https://bugs.launchpad.net/bugs/1863732 > > AV_CONF register (RH & TEMP. oversampling ratio's) and CTRL1 register > (ODR & BDU settings) values are lost after suspend. > > While the change in AV_CONF updates the sensor resolution modes > (overriding the user configuration before the device went to suspend); > loss of the contents of the CTRL1 register leads to failure in reading > sensor output. > > This patch restores the AV_CONF & CTRL1 registers after resume. > > Already submitted upstream for both 4.17.y and 4.14.y (LTS) and under > review: > 4.17.y: https://marc.info/?t=152455871600007&r=1&w=2 > 4.14.y: https://marc.info/?t=152506242000001&r=1&w=2 These discussions took place almost 2 years ago. Have we had any results from them? > > However, the issue is only affecting the Dell GW and not ST micro devel. boards. > > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) > > linux-oem buglink: https://bugs.launchpad.net/bugs/1769658 > > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> > --- > drivers/iio/humidity/hts221_core.c | 31 ++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c > index daef177219b6..7d24e3d8fbdf 100644 > --- a/drivers/iio/humidity/hts221_core.c > +++ b/drivers/iio/humidity/hts221_core.c > @@ -671,13 +671,40 @@ static int __maybe_unused hts221_resume(struct device *dev) > { > struct iio_dev *iio_dev = dev_get_drvdata(dev); > struct hts221_hw *hw = iio_priv(iio_dev); > + const struct hts221_avg *avg; > + u8 data, idx; > int err = 0; > > + /* Restore contents of AV_CONF (RH & TEMP. oversampling ratio's) */ > + avg = &hts221_avg_list[HTS221_SENSOR_H]; > + idx = hw->sensors[HTS221_SENSOR_H].cur_avg_idx; > + data = avg->avg_avl[idx]; > + err = hts221_update_avg(hw, HTS221_SENSOR_H, data); > + if (err < 0) > + goto fail_err; > + > + avg = &hts221_avg_list[HTS221_SENSOR_T]; > + idx = hw->sensors[HTS221_SENSOR_T].cur_avg_idx; > + data = avg->avg_avl[idx]; > + err = hts221_update_avg(hw, HTS221_SENSOR_T, data); > + if (err < 0) > + goto fail_err; > + > + /* Restore contents of CTRL1 (BDU & ODR) */ > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > + HTS221_BDU_MASK, 1); > + if (err < 0) > + goto fail_err; > + > + err = hts221_update_odr(hw, hw->odr); > + if (err < 0) > + goto fail_err; > + > if (hw->enabled) > err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > HTS221_ENABLE_MASK, true); > - > - return err; > +fail_err: > + return err < 0 ? err : 0; > } > > const struct dev_pm_ops hts221_pm_ops = { >
On Tue, 2020-02-25 at 12:55 +0100, Kleber Souza wrote: > Hi Shrirang, > > On 18.02.20 11:28, Shrirang Bagul wrote: > > BugLink: https://bugs.launchpad.net/bugs/1863732 > > > > AV_CONF register (RH & TEMP. oversampling ratio's) and CTRL1 register > > (ODR & BDU settings) values are lost after suspend. > > > > While the change in AV_CONF updates the sensor resolution modes > > (overriding the user configuration before the device went to suspend); > > loss of the contents of the CTRL1 register leads to failure in reading > > sensor output. > > > > This patch restores the AV_CONF & CTRL1 registers after resume. > > > > Already submitted upstream for both 4.17.y and 4.14.y (LTS) and under > > review: > > 4.17.y: https://marc.info/?t=152455871600007&r=1&w=2 > > 4.14.y: https://marc.info/?t=152506242000001&r=1&w=2 > > These discussions took place almost 2 years ago. Have we had any > results from them? > > > > > However, the issue is only affecting the Dell GW and not ST micro devel. boards. This is the conclusion. ST Mirco developer thought that restoring the sensor config. registers was not required (and therefore a redundant step) when tested on their development boards. But it definitely affected the Dell 300x Edge GW's. Because the Dell GW was already in production, there was no chance we could insist Dell to review the HW design, hence the SAUCE patch. > > > > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) > > > > linux-oem buglink: https://bugs.launchpad.net/bugs/1769658 > > > > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> > > --- > > drivers/iio/humidity/hts221_core.c | 31 ++++++++++++++++++++++++++++-- > > 1 file changed, 29 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c > > index daef177219b6..7d24e3d8fbdf 100644 > > --- a/drivers/iio/humidity/hts221_core.c > > +++ b/drivers/iio/humidity/hts221_core.c > > @@ -671,13 +671,40 @@ static int __maybe_unused hts221_resume(struct device *dev) > > { > > struct iio_dev *iio_dev = dev_get_drvdata(dev); > > struct hts221_hw *hw = iio_priv(iio_dev); > > + const struct hts221_avg *avg; > > + u8 data, idx; > > int err = 0; > > > > + /* Restore contents of AV_CONF (RH & TEMP. oversampling ratio's) */ > > + avg = &hts221_avg_list[HTS221_SENSOR_H]; > > + idx = hw->sensors[HTS221_SENSOR_H].cur_avg_idx; > > + data = avg->avg_avl[idx]; > > + err = hts221_update_avg(hw, HTS221_SENSOR_H, data); > > + if (err < 0) > > + goto fail_err; > > + > > + avg = &hts221_avg_list[HTS221_SENSOR_T]; > > + idx = hw->sensors[HTS221_SENSOR_T].cur_avg_idx; > > + data = avg->avg_avl[idx]; > > + err = hts221_update_avg(hw, HTS221_SENSOR_T, data); > > + if (err < 0) > > + goto fail_err; > > + > > + /* Restore contents of CTRL1 (BDU & ODR) */ > > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > > + HTS221_BDU_MASK, 1); > > + if (err < 0) > > + goto fail_err; > > + > > + err = hts221_update_odr(hw, hw->odr); > > + if (err < 0) > > + goto fail_err; > > + > > if (hw->enabled) > > err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > > HTS221_ENABLE_MASK, true); > > - > > - return err; > > +fail_err: > > + return err < 0 ? err : 0; > > } > > > > const struct dev_pm_ops hts221_pm_ops = { > > > >
This has been in the linux-oem for quite a while and no regression.
Acked-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
On 18.02.20 11:28, Shrirang Bagul wrote: > BugLink: https://bugs.launchpad.net/bugs/1863732 > > AV_CONF register (RH & TEMP. oversampling ratio's) and CTRL1 register > (ODR & BDU settings) values are lost after suspend. > > While the change in AV_CONF updates the sensor resolution modes > (overriding the user configuration before the device went to suspend); > loss of the contents of the CTRL1 register leads to failure in reading > sensor output. > > This patch restores the AV_CONF & CTRL1 registers after resume. > > Already submitted upstream for both 4.17.y and 4.14.y (LTS) and under > review: > 4.17.y: https://marc.info/?t=152455871600007&r=1&w=2 > 4.14.y: https://marc.info/?t=152506242000001&r=1&w=2 > > However, the issue is only affecting the Dell GW and not ST micro devel. boards. > > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) > > linux-oem buglink: https://bugs.launchpad.net/bugs/1769658 > > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > drivers/iio/humidity/hts221_core.c | 31 ++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c > index daef177219b6..7d24e3d8fbdf 100644 > --- a/drivers/iio/humidity/hts221_core.c > +++ b/drivers/iio/humidity/hts221_core.c > @@ -671,13 +671,40 @@ static int __maybe_unused hts221_resume(struct device *dev) > { > struct iio_dev *iio_dev = dev_get_drvdata(dev); > struct hts221_hw *hw = iio_priv(iio_dev); > + const struct hts221_avg *avg; > + u8 data, idx; > int err = 0; > > + /* Restore contents of AV_CONF (RH & TEMP. oversampling ratio's) */ > + avg = &hts221_avg_list[HTS221_SENSOR_H]; > + idx = hw->sensors[HTS221_SENSOR_H].cur_avg_idx; > + data = avg->avg_avl[idx]; > + err = hts221_update_avg(hw, HTS221_SENSOR_H, data); > + if (err < 0) > + goto fail_err; > + > + avg = &hts221_avg_list[HTS221_SENSOR_T]; > + idx = hw->sensors[HTS221_SENSOR_T].cur_avg_idx; > + data = avg->avg_avl[idx]; > + err = hts221_update_avg(hw, HTS221_SENSOR_T, data); > + if (err < 0) > + goto fail_err; > + > + /* Restore contents of CTRL1 (BDU & ODR) */ > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > + HTS221_BDU_MASK, 1); > + if (err < 0) > + goto fail_err; > + > + err = hts221_update_odr(hw, hw->odr); > + if (err < 0) > + goto fail_err; > + > if (hw->enabled) > err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > HTS221_ENABLE_MASK, true); > - > - return err; > +fail_err: > + return err < 0 ? err : 0; > } > > const struct dev_pm_ops hts221_pm_ops = { >
On 2020-02-18 18:28:05 , Shrirang Bagul wrote: > BugLink: https://bugs.launchpad.net/bugs/1863732 > > AV_CONF register (RH & TEMP. oversampling ratio's) and CTRL1 register > (ODR & BDU settings) values are lost after suspend. > > While the change in AV_CONF updates the sensor resolution modes > (overriding the user configuration before the device went to suspend); > loss of the contents of the CTRL1 register leads to failure in reading > sensor output. > > This patch restores the AV_CONF & CTRL1 registers after resume. > > Already submitted upstream for both 4.17.y and 4.14.y (LTS) and under > review: > 4.17.y: https://marc.info/?t=152455871600007&r=1&w=2 > 4.14.y: https://marc.info/?t=152506242000001&r=1&w=2 > > However, the issue is only affecting the Dell GW and not ST micro devel. boards. > > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) > > linux-oem buglink: https://bugs.launchpad.net/bugs/1769658 > > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> > --- > drivers/iio/humidity/hts221_core.c | 31 ++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c > index daef177219b6..7d24e3d8fbdf 100644 > --- a/drivers/iio/humidity/hts221_core.c > +++ b/drivers/iio/humidity/hts221_core.c > @@ -671,13 +671,40 @@ static int __maybe_unused hts221_resume(struct device *dev) > { > struct iio_dev *iio_dev = dev_get_drvdata(dev); > struct hts221_hw *hw = iio_priv(iio_dev); > + const struct hts221_avg *avg; > + u8 data, idx; > int err = 0; > > + /* Restore contents of AV_CONF (RH & TEMP. oversampling ratio's) */ > + avg = &hts221_avg_list[HTS221_SENSOR_H]; > + idx = hw->sensors[HTS221_SENSOR_H].cur_avg_idx; > + data = avg->avg_avl[idx]; > + err = hts221_update_avg(hw, HTS221_SENSOR_H, data); > + if (err < 0) > + goto fail_err; > + > + avg = &hts221_avg_list[HTS221_SENSOR_T]; > + idx = hw->sensors[HTS221_SENSOR_T].cur_avg_idx; > + data = avg->avg_avl[idx]; > + err = hts221_update_avg(hw, HTS221_SENSOR_T, data); > + if (err < 0) > + goto fail_err; > + > + /* Restore contents of CTRL1 (BDU & ODR) */ > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > + HTS221_BDU_MASK, 1); > + if (err < 0) > + goto fail_err; > + > + err = hts221_update_odr(hw, hw->odr); > + if (err < 0) > + goto fail_err; > + > if (hw->enabled) > err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > HTS221_ENABLE_MASK, true); > - > - return err; > +fail_err: > + return err < 0 ? err : 0; > } > > const struct dev_pm_ops hts221_pm_ops = { > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c index daef177219b6..7d24e3d8fbdf 100644 --- a/drivers/iio/humidity/hts221_core.c +++ b/drivers/iio/humidity/hts221_core.c @@ -671,13 +671,40 @@ static int __maybe_unused hts221_resume(struct device *dev) { struct iio_dev *iio_dev = dev_get_drvdata(dev); struct hts221_hw *hw = iio_priv(iio_dev); + const struct hts221_avg *avg; + u8 data, idx; int err = 0; + /* Restore contents of AV_CONF (RH & TEMP. oversampling ratio's) */ + avg = &hts221_avg_list[HTS221_SENSOR_H]; + idx = hw->sensors[HTS221_SENSOR_H].cur_avg_idx; + data = avg->avg_avl[idx]; + err = hts221_update_avg(hw, HTS221_SENSOR_H, data); + if (err < 0) + goto fail_err; + + avg = &hts221_avg_list[HTS221_SENSOR_T]; + idx = hw->sensors[HTS221_SENSOR_T].cur_avg_idx; + data = avg->avg_avl[idx]; + err = hts221_update_avg(hw, HTS221_SENSOR_T, data); + if (err < 0) + goto fail_err; + + /* Restore contents of CTRL1 (BDU & ODR) */ + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, + HTS221_BDU_MASK, 1); + if (err < 0) + goto fail_err; + + err = hts221_update_odr(hw, hw->odr); + if (err < 0) + goto fail_err; + if (hw->enabled) err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, HTS221_ENABLE_MASK, true); - - return err; +fail_err: + return err < 0 ? err : 0; } const struct dev_pm_ops hts221_pm_ops = {
BugLink: https://bugs.launchpad.net/bugs/1863732 AV_CONF register (RH & TEMP. oversampling ratio's) and CTRL1 register (ODR & BDU settings) values are lost after suspend. While the change in AV_CONF updates the sensor resolution modes (overriding the user configuration before the device went to suspend); loss of the contents of the CTRL1 register leads to failure in reading sensor output. This patch restores the AV_CONF & CTRL1 registers after resume. Already submitted upstream for both 4.17.y and 4.14.y (LTS) and under review: 4.17.y: https://marc.info/?t=152455871600007&r=1&w=2 4.14.y: https://marc.info/?t=152506242000001&r=1&w=2 However, the issue is only affecting the Dell GW and not ST micro devel. boards. Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) linux-oem buglink: https://bugs.launchpad.net/bugs/1769658 Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> --- drivers/iio/humidity/hts221_core.c | 31 ++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)