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

[Shrirang Bagul <shrirang.bagul@canonical.com>]

[-- 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 --]