| Message ID | 20180430042546.8801-1-shrirang.bagul@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 30 Apr 2018 12:25:46 +0800 Shrirang Bagul <shrirang.bagul@canonical.com> wrote: > CTRL1 register (ODR & BDU settings) gets reset after system comes back > from suspend, causing subsequent reads from the sensor to fail. > > This patch restores the CTRL1 register after resume. > > Based on: > git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git iio-fixes-for-4.14b > > Since 4.17.rc1, this driver uses REGMAP; I'll send a separate patch to > address this issue. > > Cc: stable@vger.kernel.org > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) Looks like part of the problem was introduced in that patch, part well predated it (BDU). > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> As you way, this needs to be a bit different to take into account the change to regmap. We'll need to have that upstream before we look at a back port. One element inline surprises me and needs further explanation. > --- > drivers/iio/humidity/hts221_core.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c > index 32524a8dc66f..fed2da64fa3b 100644 > --- a/drivers/iio/humidity/hts221_core.c > +++ b/drivers/iio/humidity/hts221_core.c > @@ -674,11 +674,15 @@ static int __maybe_unused hts221_resume(struct device *dev) > struct hts221_hw *hw = iio_priv(iio_dev); > int err = 0; > > - if (hw->enabled) > - err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > - HTS221_ENABLE_MASK, true); Why drop the enable setting? Seems that we want to do this 'as well', if the device was previous enabled. > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > + HTS221_BDU_MASK, 1); > + if (err < 0) > + goto fail_err; > > - return err; > + err = hts221_update_odr(hw, hw->odr); > + > +fail_err: > + return err < 0 ? err : 0; > } > > const struct dev_pm_ops hts221_pm_ops = { -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2018-05-06 at 17:19 +0100, Jonathan Cameron wrote: > On Mon, 30 Apr 2018 12:25:46 +0800 > Shrirang Bagul <shrirang.bagul@canonical.com> wrote: > > > CTRL1 register (ODR & BDU settings) gets reset after system comes back > > from suspend, causing subsequent reads from the sensor to fail. > > > > This patch restores the CTRL1 register after resume. > > > > Based on: > > git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git iio-fixes-for-4.14b > > > > Since 4.17.rc1, this driver uses REGMAP; I'll send a separate patch to > > address this issue. > > > > Cc: stable@vger.kernel.org > > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) > > Looks like part of the problem was introduced in that patch, part well predated > it (BDU). > > > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> > > As you way, this needs to be a bit different to take into account > the change to regmap. We'll need to have that upstream before we look > at a back port. One element inline surprises me and needs further > explanation. I have sent a patch based on iio-for-4.17b [1], Lorenzo and I are still discussing our findings. It's not just the CTRL1 reg, but also the AV_CONF(0x10) reg. which loses it's contents coming out of suspend. [1] https://marc.info/?l=linux-iio&m=152534455701742&w=2 > > > > --- > > drivers/iio/humidity/hts221_core.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c > > index 32524a8dc66f..fed2da64fa3b 100644 > > --- a/drivers/iio/humidity/hts221_core.c > > +++ b/drivers/iio/humidity/hts221_core.c > > @@ -674,11 +674,15 @@ static int __maybe_unused hts221_resume(struct device *dev) > > struct hts221_hw *hw = iio_priv(iio_dev); > > int err = 0; > > > > - if (hw->enabled) > > - err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > > - HTS221_ENABLE_MASK, true); > > Why drop the enable setting? Seems that we want to do this 'as well', > if the device was previous enabled. Yes, will cover this in v2. > > > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > > + HTS221_BDU_MASK, 1); > > + if (err < 0) > > + goto fail_err; > > > > - return err; > > + err = hts221_update_odr(hw, hw->odr); > > + > > +fail_err: > > + return err < 0 ? err : 0; > > } > > > > const struct dev_pm_ops hts221_pm_ops = { > >
> On Sun, 2018-05-06 at 17:19 +0100, Jonathan Cameron wrote: >> On Mon, 30 Apr 2018 12:25:46 +0800 >> Shrirang Bagul <shrirang.bagul@canonical.com> wrote: >> >> > CTRL1 register (ODR & BDU settings) gets reset after system comes back >> > from suspend, causing subsequent reads from the sensor to fail. >> > >> > This patch restores the CTRL1 register after resume. >> > >> > Based on: >> > git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git iio-fixes-for-4.14b >> > >> > Since 4.17.rc1, this driver uses REGMAP; I'll send a separate patch to >> > address this issue. >> > >> > Cc: stable@vger.kernel.org >> > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) >> >> Looks like part of the problem was introduced in that patch, part well predated >> it (BDU). >> >> > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> >> >> As you way, this needs to be a bit different to take into account >> the change to regmap. We'll need to have that upstream before we look >> at a back port. One element inline surprises me and needs further >> explanation. > I have sent a patch based on iio-for-4.17b [1], Lorenzo and I are still > discussing our findings. It's not just the CTRL1 reg, but also the AV_CONF(0x10) reg. > which loses it's contents coming out of suspend. > > [1] https://marc.info/?l=linux-iio&m=152534455701742&w=2 >> >> >> > --- >> > drivers/iio/humidity/hts221_core.c | 12 ++++++++---- >> > 1 file changed, 8 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c >> > index 32524a8dc66f..fed2da64fa3b 100644 >> > --- a/drivers/iio/humidity/hts221_core.c >> > +++ b/drivers/iio/humidity/hts221_core.c >> > @@ -674,11 +674,15 @@ static int __maybe_unused hts221_resume(struct device *dev) >> > struct hts221_hw *hw = iio_priv(iio_dev); >> > int err = 0; >> > >> > - if (hw->enabled) >> > - err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, >> > - HTS221_ENABLE_MASK, true); >> >> Why drop the enable setting? Seems that we want to do this 'as well', >> if the device was previous enabled. > Yes, will cover this in v2. >> >> > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, >> > + HTS221_BDU_MASK, 1); >> > + if (err < 0) >> > + goto fail_err; >> > >> > - return err; >> > + err = hts221_update_odr(hw, hw->odr); >> > + >> > +fail_err: >> > + return err < 0 ? err : 0; >> > } >> > >> > const struct dev_pm_ops hts221_pm_ops = { >> >> I guess we need to double-check the setup first since BDU is configured just at bootstrap and it is never reset during suspend/resume or normal operation. It seems an electrical issue (e.g. Vdd not stable during suspend). Shrirang could you please double check the device is powered up during the suspend phase? Is device connected through I2C or SPI? Could you please paste register map before and after the suspend/resume operation? Regards, Lorenzo
On Mon, 2018-05-07 at 23:34 +0200, Lorenzo Bianconi wrote: > > On Sun, 2018-05-06 at 17:19 +0100, Jonathan Cameron wrote: > > > On Mon, 30 Apr 2018 12:25:46 +0800 > > > Shrirang Bagul <shrirang.bagul@canonical.com> wrote: > > > > > > > CTRL1 register (ODR & BDU settings) gets reset after system comes back > > > > from suspend, causing subsequent reads from the sensor to fail. > > > > > > > > This patch restores the CTRL1 register after resume. > > > > > > > > Based on: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git iio-fixes-for-4.14b > > > > > > > > Since 4.17.rc1, this driver uses REGMAP; I'll send a separate patch to > > > > address this issue. > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) > > > > > > Looks like part of the problem was introduced in that patch, part well predated > > > it (BDU). > > > > > > > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> > > > > > > As you way, this needs to be a bit different to take into account > > > the change to regmap. We'll need to have that upstream before we look > > > at a back port. One element inline surprises me and needs further > > > explanation. > > > > I have sent a patch based on iio-for-4.17b [1], Lorenzo and I are still > > discussing our findings. It's not just the CTRL1 reg, but also the AV_CONF(0x10) reg. > > which loses it's contents coming out of suspend. > > > > [1] https://marc.info/?l=linux-iio&m=152534455701742&w=2 > > > > > > > > > > --- > > > > drivers/iio/humidity/hts221_core.c | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c > > > > index 32524a8dc66f..fed2da64fa3b 100644 > > > > --- a/drivers/iio/humidity/hts221_core.c > > > > +++ b/drivers/iio/humidity/hts221_core.c > > > > @@ -674,11 +674,15 @@ static int __maybe_unused hts221_resume(struct device *dev) > > > > struct hts221_hw *hw = iio_priv(iio_dev); > > > > int err = 0; > > > > > > > > - if (hw->enabled) > > > > - err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > > > > - HTS221_ENABLE_MASK, true); > > > > > > Why drop the enable setting? Seems that we want to do this 'as well', > > > if the device was previous enabled. > > > > Yes, will cover this in v2. > > > > > > > + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, > > > > + HTS221_BDU_MASK, 1); > > > > + if (err < 0) > > > > + goto fail_err; > > > > > > > > - return err; > > > > + err = hts221_update_odr(hw, hw->odr); > > > > + > > > > +fail_err: > > > > + return err < 0 ? err : 0; > > > > } > > > > > > > > const struct dev_pm_ops hts221_pm_ops = { > > > > > > > > I guess we need to double-check the setup first since BDU is > configured just at bootstrap and it is never reset during > suspend/resume or normal operation. > It seems an electrical issue (e.g. Vdd not stable during suspend). > Shrirang could you please double check the device is powered up during > the suspend phase? > Is device connected through I2C or SPI? Could you please paste > register map before and after the suspend/resume operation? The sensor is mounted on the main board of Dell Edge Gateway 300x [1]. As I've mentioned before, I have no resources to test the HW. These devices are already in production. I have already shared the register contents of hts221 in [2], but in case you've missed it, here they are: root@caracalla:/sys/bus/iio/devices/iio:device0# cat in_*_raw -2032 1023 root@caracalla:/sys/bus/iio/devices/iio:device0# cat in_*_raw -2024 1022 root@caracalla:/sys/bus/iio/devices/iio:device0# /home/admin/i2c-tools-4.0/tools/i2cdump -f -y 0 0x5f b 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef 00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bc ...............? 10: 1b 00 65 32 99 be 32 a2 9e b2 fb 00 e8 01 00 82 ?.e2??2????.??.? 20: 05 00 00 00 00 00 00 02 db f7 fe 03 c7 fb 16 04 ?......????????? 30: 37 85 a8 1b 00 c4 e8 ff 86 03 80 ce ff ff 19 03 7???.??.????..?? 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bc ...............? 90: 1b 00 65 32 99 be 32 a2 9e b2 fb 00 e8 01 00 82 ?.e2??2????.??.? a0: 05 00 00 00 00 00 00 00 db f7 fe 03 c7 fb 16 04 ?.......???????? b0: 37 85 a8 1b 00 c4 e8 ff 86 03 80 ce ff ff 19 03 7???.??.????..?? c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ root@caracalla:/sys/bus/iio/devices/iio:device0# rtcwake -a -v -s 40 -m mem rtcwake: assuming RTC uses UTC ... Using UTC time. delta = 0 tzone = 0 tzname = UTC systime = 1525343782, (UTC) Thu May 3 10:36:22 2018 rtctime = 1525343782, (UTC) Thu May 3 10:36:22 2018 alarm 0, sys_time 1525343782, rtc_time 1525343782, seconds 40 rtcwake: wakeup from "mem" using /dev/rtc0 at Thu May 3 10:37:03 2018 suspend mode: mem; suspending system root@caracalla:/sys/bus/iio/devices/iio:device0# /home/admin/i2c-tools-4.0/tools/i2cdump -f -y 0 0x5f b 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef 00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bc ...............? 10: 3f 00 65 32 99 be 32 a2 9e b2 fb 00 e8 01 80 82 ?.e2??2????.???? 20: 00 00 00 00 00 00 00 03 7e f6 78 03 f4 f9 90 03 .......?~?x????? 30: 37 85 a8 1b 00 c4 e8 ff 86 03 80 ce ff ff 19 03 7???.??.????..?? 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 bc ...............? 90: 3f 00 65 32 99 be 32 a2 9e b2 fb 00 e8 01 80 82 ?.e2??2????.???? a0: 00 00 00 00 00 00 00 00 7e f6 78 03 f4 f9 90 03 ........~?x????? b0: 37 85 a8 1b 00 c4 e8 ff 86 03 80 ce ff ff 19 03 7???.??.????..?? c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ root@caracalla:/sys/bus/iio/devices/iio:device0# cat in_*_raw -2434 888 root@caracalla:/sys/bus/iio/devices/iio:device0# cat in_*_raw -2434 888 root@caracalla:/sys/bus/iio/devices/iio:device0# cat in_humidityrelative_oversampling_ratio 32 root@caracalla:/sys/bus/iio/devices/iio:device0# cat in_temp_oversampling_ratio 16 Regards, Shrirang. [1] http://i.dell.com/sites/doccontent/shared-content/data-sheets/en/Documents/Dell_Edge_Gateway_3000_Series_spec_sheet.pdf?newtab=true [2] https://marc.info/?l=linux-iio&m=152534455701742&w=2 > > Regards, > Lorenzo >
diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c index 32524a8dc66f..fed2da64fa3b 100644 --- a/drivers/iio/humidity/hts221_core.c +++ b/drivers/iio/humidity/hts221_core.c @@ -674,11 +674,15 @@ static int __maybe_unused hts221_resume(struct device *dev) struct hts221_hw *hw = iio_priv(iio_dev); int err = 0; - if (hw->enabled) - err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, - HTS221_ENABLE_MASK, true); + err = hts221_write_with_mask(hw, HTS221_REG_CNTRL1_ADDR, + HTS221_BDU_MASK, 1); + if (err < 0) + goto fail_err; - return err; + err = hts221_update_odr(hw, hw->odr); + +fail_err: + return err < 0 ? err : 0; } const struct dev_pm_ops hts221_pm_ops = {
CTRL1 register (ODR & BDU settings) gets reset after system comes back from suspend, causing subsequent reads from the sensor to fail. This patch restores the CTRL1 register after resume. Based on: git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git iio-fixes-for-4.14b Since 4.17.rc1, this driver uses REGMAP; I'll send a separate patch to address this issue. Cc: stable@vger.kernel.org Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR reconfiguration) Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com> --- drivers/iio/humidity/hts221_core.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)