[PATCH] iio: humidity: hts221: Fix sensor reads after resume

[PATCH] iio: humidity: hts221: Fix sensor reads after resume

[Shrirang Bagul]

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(-)

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 = {
-- 
2.14.1

Re: [PATCH] iio: humidity: hts221: Fix sensor reads after resume

[Jonathan Cameron]

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 = {

Re: [PATCH] iio: humidity: hts221: Fix sensor reads after resume

[Shrirang Bagul]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]

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 = {
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] iio: humidity: hts221: Fix sensor reads after resume

[Lorenzo Bianconi]

> 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

-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: [PATCH] iio: humidity: hts221: Fix sensor reads after resume

[Shrirang Bagul]

[-- Attachment #1: Type: text/plain, Size: 7624 bytes --]

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
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]