Bug 203829

Summary: [Bisected] 9bcf15f75cac3c6a00d8f8083a635de9c8537799: Battery status no longer properly working on Lenovo Ideapad 100S
Product: Drivers Reporter: klappnase (klappnase)
Component: OtherAssignee: Hans de Goede (jwrdegoede)
Status: CLOSED CODE_FIX    
Severity: normal CC: rui.zhang
Priority: P1    
Hardware: Intel   
OS: Linux   
Kernel Version: 4.19.22 Subsystem:
Regression: Yes Bisected commit-id:
Attachments: dmidecode info
syslog
lsmod info
acpidump output
[PATCH] iio: adc: axp288: Override TS pin bias current for some models

Description klappnase 2019-06-05 22:28:08 UTC
Created attachment 283123 [details]
dmidecode info

With the Lenovo Ideapd 100S beginning with Kernel 4.19.22 the battery status no longer works properly. The bug still persists with 5.2-rc3.
Unfortunately I have not been able to find useful messages in the syslog or dmesg output; I will try to describe the symptoms the best I can.

When booting the system without the charger, during the boot process for some reason the laptop's battery light starts blinking white. Apart from this the battery status seems to be reported correctly. When the charger is plugged in, the battery light still blinks instead of glowing steadily. The battery appears to be charged, however the status (resp. the percentage) reported by upower never changes. When the charger is disconnected again, the reported percentage remains at the incorrect "before charging" value until the battery is discharged to a lower value. When the laptop is shut down, the battery light keeps blinking, however its color changes to red. When the laptop is being rebooted after charging, the incorrect percentage value wil still be reported, regardless of which kernel is used for the reboot.

None of the above symptoms occurs with 4.19.21, both the battery light and the reported battery status work as expected.

I attached dmidecode info of the Ideapad. Please let me know, if you need more information.

Best regards

Michael
Comment 1 klappnase 2019-06-05 22:31:02 UTC
Created attachment 283125 [details]
syslog

A syslog excerpt that shows two subsequent boots into 4.19.21 and 4.19.22
Comment 2 klappnase 2019-06-05 22:32:22 UTC
Created attachment 283127 [details]
lsmod info

Sorted output of lsmod
Comment 3 klappnase 2019-06-06 17:20:20 UTC
I tried something else and recompiled 4.19.22 with the file drivers/iio/adc/axp288_adc.c replaced with the one from 4.19.21 . Surprisingly from a quick test this seems to fix the issue here, so it looks like the changes applied to the axp288_adc module seem to be the problem.
Of course I have no idea why that is so, but I hope that maybe this gives you something to work with.

Best regards
Michael
Comment 4 klappnase 2019-06-09 16:42:20 UTC
I should add that I only noticed yesterday that there is an updated BIOS firmware available from Lenovo. So I upgraded the laptop's BIOS to the latest version (E2CN15WW), still the behavior remains the same. The output of dmidecode now looks (apart from the BIOS version) exactly the same as the one I attached earlier.

Best regards
Michael
Comment 5 Zhang Rui 2019-09-03 07:59:18 UTC
there is only one change in drivers/iio/adc/axp288_adc.c recently.
commit 9bcf15f75cac3c6a00d8f8083a635de9c8537799
Author:     Hans de Goede <hdegoede@redhat.com>
AuthorDate: Sat Jan 5 19:36:18 2019 +0100
Commit:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CommitDate: Sat Jan 12 17:09:55 2019 +0000

    iio: adc: axp288: Fix TS-pin handling

please try revert this commit in the latest upstream kernel, and see if things improve or not.
Comment 6 klappnase 2019-09-04 10:26:40 UTC
Thanks for the feedback!

I tried with 5.3-rc7.

Just to double-check I compiled 5.3-rc7 in its original form and found that the bug still persists (same behavior as described above).

Compiled again with the commit reverted, then again the battery status works fine, just as expected.

Best regards
Michael
Comment 7 Zhang Rui 2019-09-04 11:06:24 UTC
good to know, thanks for the confirmation.

Reassign to Hans. :)
Comment 8 Hans de Goede 2019-09-04 18:00:09 UTC
Hi,

klappnase, thank you for the bug report. Zhang Rui thank you for the initial triaging of the bug.

klappnase, to figure out a fix for this I need some more info. First of all please install the i2c-tools, or whatever the package providing the i2cdump command is called on your distro and then do:

sudo modprobe i2c-dev

# note the filename below may be slightly different, e.g. :01 at the end
ls -l /sys/bus/i2c/devices/i2c-INT33F4:00

This will print something like this:

/sys/bus/i2c/devices/i2c-INT33F4:00 -> ../../../devices/pci0000:00/0000:00:1d.1/0000:03:00.0/0000:04:00.0/i2c-5/i2c-INT33F4:00

Notice the i2c-5 just before the i2c-INT33F4:00, the 5 is the number we are interested in, now do:

sudo i2cdump -y -f # 0x34

Replace the # with the number you found by doing the ls -l /sys/bus/i2c/devices/i2c-INT33F4:00 and then copy and paste the output here.

Please paste the i2cdump output for both a boot with a working kernel and a boot with a kernel with the troublesome 9bcf15f75cac3c6a00d8f8083a635de9c8537799 commit.

Thanks,

Hans
Comment 9 Hans de Goede 2019-09-04 18:00:42 UTC
Note to self make sure whatever fix I come up with does not regress:
https://bugzilla.redhat.com/show_bug.cgi?id=1610545
Comment 10 klappnase 2019-09-04 23:44:56 UTC
Hi Hans,

thanks for the the quick response!

I did what you suggested with the two versions of 5.3-rc7 I built the other day and got the following output:

"broken" kernel 5.3-rc7 (original sources):

# i2cdump -y -f 4 0x34
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 30 e0 51 00 00 3b 01 00 00 00 00 00 00 00 00    .0?Q..;?........
10: 7f 00 7a ff 48 16 16 1a 1a 00 16 00 0b 0b 00 00    ?.z.H????.?.??..
20: 02 9f b2 9e ac a8 a8 f4 15 1a 1a 00 01 30 40 00    ???????????.?0@.
30: 29 03 4b 4d 23 78 08 00 a5 1f 80 08 e8 09 00 00    )?KM#x?.??????..
40: 1c 3c ff 03 60 03 00 00 00 00 00 04 00 00 00 00    ?<.?`?.....?....
50: d0 00 00 00 65 06 b2 0a 1c 03 00 01 00 00 00 00    ?...e?????.?....
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 d1 0f 00 00 51 02 00 00    ........??..Q?..
80: 00 80 f1 00 93 b4 00 00 00 00 00 00 00 00 40 0c    .??.??........@?
90: 07 1a 03 1a 00 00 00 00 00 00 42 32 22 64 0a 00    ????......B2"d?.
a0: d0 03 00 00 65 07 21 0c 00 00 00 00 00 00 00 00    ??..e?!?........
b0: 00 00 00 00 00 00 00 00 e0 b2 c0 63 d9 0c 00 00    ........???c??..
c0: 00 00 00 00 00 00 01 01 02 03 04 0c 13 20 2b 30    ......??????? +0
d0: 35 39 3b 41 45 4b 4e 51 58 5e 62 64 64 64 64 64    59;AEKNQX^bddddd
e0: 96 51 8b 17 b4 b2 d2 00 00 00 00 00 00 00 50 00    ?Q?????.......P.
f0: 01 00 80 03 00 00 00 00 00 00 00 00 00 00 00 00    ?.??............

"working" kernel 5.3-rc7 (commit reverted):

# i2cdump -y -f 4 0x34
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 30 e0 51 00 00 3b 01 00 00 00 00 00 00 00 00    .0?Q..;?........
10: 7f 00 7a ff 48 16 16 1a 1a 00 16 00 0b 0b 00 00    ?.z.H????.?.??..
20: 02 9e b2 9d ac a8 a8 f4 15 1a 1a 00 01 30 40 00    ???????????.?0@.
30: 29 03 4b 4d 23 78 08 00 a5 1f 80 08 e8 09 00 00    )?KM#x?.??????..
40: 1c 3c ff 03 60 03 00 00 00 00 00 04 00 00 00 00    ?<.?`?.....?....
50: cf 07 00 00 67 0c b3 00 39 04 00 02 00 00 00 00    ??..g??.9?.?....
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 d1 0f 00 00 4d 07 00 00    ........??..M?..
80: 00 80 f1 00 f3 b4 00 00 00 00 00 00 00 00 40 0c    .??.??........@?
90: 07 1a 03 1a 00 00 00 00 00 00 42 32 22 64 0a 00    ????......B2"d?.
a0: cf 04 00 00 64 08 22 00 00 00 00 00 00 00 00 00    ??..d?".........
b0: 00 00 00 00 00 00 00 00 e0 b1 c0 63 d9 07 00 00    ........???c??..
c0: 00 00 00 00 00 00 01 01 02 03 04 0c 13 20 2b 30    ......??????? +0
d0: 35 39 3b 41 45 4b 4e 51 58 5e 62 64 64 64 64 64    59;AEKNQX^bddddd
e0: 96 51 8a e4 b2 b1 d2 00 00 00 00 00 00 00 50 00    ?Q?????.......P.
f0: 01 00 80 03 00 00 00 00 00 00 00 00 00 00 00 00    ?.??............


I hope this helps (and thanks for working on this!)

Michael
Comment 11 Hans de Goede 2019-09-05 13:12:13 UTC
Ok, so the onyl 2 registers impacted by the commit causing this problem are:

#define AXP20X_ADC_EN1                      0x82
#define AXP288_ADC_TS_PIN_CTRL              0x84

Register 0x82 is 0xf1 in both cases, with the 0x01 part indicating that the TS pin is enabled. 0x84 changes from 0x93 (non working kernel) to 0xf3 (working kernel).

This is caused by the change described by this part of the commit msg of the troublesome commit:

"Problem 2 is we were writing hardcoded values to the ADC TS pin-ctrl
register, overwriting various other unrelated bits. Specifically we were
overwriting the current-source setting for the TS and GPIO0 pins, forcing
it to 80ųA independent of its original setting. On a Chuwi Vi10 tablet
this was causing us to get a too high adc value (due to a too high
current-source) resulting in the following errors being logged:

ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion]
ACPI Error: Method parse/execution failed \_SB.SXP1._TMP, AE_ERROR

This commit fixes this by using regmap_update_bits to change only the
relevant bits."

I believe that that part of the commit is pretty much full on correct.

So what the difference for reg 0x84 changing from 0x93 (non working kernel) to 0xf3 (working kernel) means is that the bias current for the TS and GPIO0 pins are being configured differently, specificly the bias current for the TS pin is 40ųA on the non working kernel and 80ųA on the working kernel.

IMHO this is a firmware bug, the firmware should really initialize the bias current correctly since there is no way for us to know what the correct bias current is. So It looks like we will need to fix thus using a DMI based quirk specificly for the Lenovo Ideapad 100S.

To confirm that this really is the problem, can you try executing the following command with the *non* working kernel and confirm that this fixes things? :

sudo i2cset -y -f 4 0x34 0x84 0xf3

If this does not fix things, please run i2cdump again, the value may not stick as it gets cached by the regmap subsys in the kernel. If the value does not stick I will just need to prepare the fix assuming it will work.

Also please do:

sudo acpidump -o acpidump.lenovo-ideapad-100s

And attach the generated acpidump.lenovo-ideapad-100s file here. I want to check if maybe the ACPI code is changing the bias current on the fly, in that case there might be a cleaner fix.
Comment 12 klappnase 2019-09-05 22:13:35 UTC
Created attachment 284865 [details]
acpidump output
Comment 13 klappnase 2019-09-05 22:15:37 UTC
Thanks for the explanation.

As far as I can see, calling the i2cset command immediately fixes things (battery light stops blinking, battery status gets updated when charging).
According to i2cdump the setting of 0xf3 for the 0x84 register appears to stick, at least for the 15 minutes or so that I tried.
Comment 14 Hans de Goede 2019-09-06 10:39:36 UTC
Ok, the ACPI tables do not seem to touch the 0x84 register. So it looks like a DMI based quirk is going to be the answer here. I see you've already attached dmidecode info :)

Before I write a patch for this, the power-on-reset value of register 0x84 is 0xf2 (according to the datasheet), this suggests that the BIOS does actually program the bias current, but with a wrong setting, which is weird.

So 2 questions:

1) Setting 0x84 to 0xf3 changes both the GPIO0 and the TS pin currents, 
can you try using:

sudo i2cset -y -f 4 0x34 0x84 0xb3

To change just the TS pin bias current and see if that fixes things ? I would prefer for the patch for this to only change the TS pin bias and to leave the
GPIO0 bias alone.

2) Did you perhaps replace the battery of the laptop? That might explain why the BIOS set bias current is not correct...
Comment 15 klappnase 2019-09-06 19:47:20 UTC
Setting the register to 0xb3 seems to fix things just as setting it to 0xf3 did.

The battery is still the original one (but maybe those ideapads are equipped with cheap replacement batteries from the start :)
Comment 16 Hans de Goede 2019-09-08 13:39:01 UTC
Created attachment 284885 [details]
[PATCH] iio: adc: axp288: Override TS pin bias current for some models

klappnase,

Here is a patch which should fix this by overriding the TS pin bias current on the Ideapad 100S, please give this a try with a recent kernel.

Once you confirm that this fixes things I will submit this patch upstream.

Thanks,

Hans
Comment 17 klappnase 2019-09-09 16:26:43 UTC
Hi Hans,

I applied this patch to 5.3-rc7. Battery status works as expected again.
According to i2cdump the 0x84 register is set to 0xb3, as intended.

Excellent work, thank you so much! :)
Comment 18 Hans de Goede 2019-09-15 18:55:11 UTC
(In reply to klappnase from comment #17)
> I applied this patch to 5.3-rc7. Battery status works as expected again.
> According to i2cdump the 0x84 register is set to 0xb3, as intended.

Thank you for testing, I've submitted the patch upstream, it has a Cc: stable so it should show up in the stable kernels series eventually.