Virtual battery: no support for MANUFACTURER_ACCESS? |
|||||||
Issue description
The EC virtual battery explicitly does not support this command:
case SB_MANUFACTURER_ACCESS:
/* No manuf. access reg access allowed over VB interface */
return EC_ERROR_INVAL;
But the kernel SBS battery driver definitely wants it; that's how it checks for things like battery presence.
--- Side Note: I will probably file a separate bug for the following; it's useful context though ---
This is compounded by the fact that the kernel I2C core doesn't actually check for partial command failures -- typical I2C/SMBUS checks consist of a write followed by a read; the write command succeeds but the read command fails (due to the above lack of support). See code like this:
https://elixir.bootlin.com/linux/v4.16.13/source/drivers/i2c/i2c-core-smbus.c#L462
where 'status' will be 1 -- we sent 2 commands, and only 1 succeeded. For now, the kernel treats this as "success", which is really really wrong, but apparently hasn't caused problems so far.
-- End Side Note ---
Anyway, back to the original problem: lack of support for this command means that the kernel can't do proper error handling at the moment. If I cause SB_MANUFACTURER_ACCESS to *really* fail in the kernel, then we can't boot at all -- the battery driver gets really screwed up.
So, what should we do with this? Should we improve the virtual battery driver in the EC? Or are we stuck with having to do some horrendous workarounds within the kernel?
,
May 31 2018
I thought on things like Kevin (which have a real smart battery) when we run across a battery command that's not supported by the virtual battery then it falls back to passthrough mode. I complained about that because some devices (like scarlet, I think) don't have a real smart battery to fall back to. See bug #717753 ...so it's possible that kevin works OK but scarlet fails? I can dig more if need be...
,
May 31 2018
Actually...the code I highlighted above is in ToT but it's not in firmware-gru-8785.B branch. See: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/547004 As I read the code, we'd have the same problem on Kevin too otherwise.
,
May 31 2018
> As I read the code, we'd have the same problem on Kevin too otherwise. Actually, I'm not so sure about that one. The code *is* significantly different between the gru and scarlet/ToT firmware branches, but this particular portion is effectively similar (for -gru, it's just the 'default' case, whereas ToT breaks it out specifically). I'd have to look a little deeper about why exactly Kevin isn't seeing so many truncated xfers (I double checked; the same MANUFACTURER_ACCESS commands appear to be succeeding) whereas Scarlet is, but you might be right that it's because we don't have any actual smart battery backing us up.
,
May 31 2018
Why is kernel interested in SB_MANUFACTURER_ACCESS? SB_MANUFACTURER_ACCESS is specific to battery manufacturer. Even if the virtual battery driver returns something back to kernel, kernel won't be able to parse it.
,
May 31 2018
@philip: That's actually a good question. I was briefly curious (and then forgot about my curiosity) about why we were trying to parse this field the way we do [1], and I couldn't figure out anything -- the SBS spec does leave it 100% implementation specific. Tracing back the history of this kernel driver, it seems to have been originally written for a specific battery: commit a7640bfa10c558b7cbabb4b98d6bc356d3c2ec55 Author: Rhyland Klein <rklein@nvidia.com> Date: Sun Sep 5 15:31:23 2010 -0700 power_supply: Add driver for TI BQ20Z75 gas gauge IC And eventually, it was refactored a bit and renamed as a generic SBS battery driver. Since then, I guess we've noticed that at least *writes* to this are not supported uniformly: https://chromium-review.googlesource.com/177576 https://chromium-review.googlesource.com/381591 Eventually, Guenter upstreamed that: commit 17c6d3979e5bbff1de36a4e89015fa09ac495d27 Author: Guenter Roeck <linux@roeck-us.net> Date: Thu Sep 8 19:10:00 2016 -0700 sbs-battery: make writes to ManufacturerAccess optional So maybe it's about time to admit that using this register like this is not correct at all, and do something else in the kernel. [1] It's currently used as a "presence" or "health" check: https://elixir.bootlin.com/linux/v4.16/source/drivers/power/supply/sbs-battery.c#L321
,
May 31 2018
"not correct" is relative. I can understand why we don't want to permit real access to manufacturer data for the virtual battery, but why not fake it ?
,
May 31 2018
I mean, we *could* fake it, but then we still may have to deal with old firmware. Also, it's really not clear *what* we should fake (esp. on non-smart batteries, like Scarlet)...I guess we'd have to imitate whatever that old TI battery IC did [1]? BTW, I just checked out Kevin: it looks like its battery just returns 0x0 or 0x100 in "good" cases, and we get an I2C/SMBUS error when not connected (which fortunately gets properly interpreted as "battery not present"). It's not clear those are actually matching the TI part, since I get the "wake up" (0000b) state in both charging and discharging modes, and sometimes Normal Discharge (0001b) even when charging. I could check a few other batteries, but I think it's probably wise we stop pretending we can parse this data. --- Fun note: Scarlet (due to $subject + lack of I2C fallback (no smart battery) + bug 848112 ) is getting totally random garbage back for 'Manufacturer Status' when I 'cat /sys/class/power_supply/sbs-9-000b/present'. A few sample values that are getting returned to the SBS driver: 0xe00 0xbb00 0xba00 0x3500 0x3200 And this is a very lovely result: # for i in $(seq 1 1000); do cat /sys/class/power_supply/sbs-9-000b/present; done | sort | uniq -c 2 0 998 1 [1] I guess this was a reusable IC that probably got used a lot? http://www.ti.com/lit/er/sluu265a/sluu265a.pdf A.1 ManufacturerAccess(0x00) The result of these commands need to be read from ManufacturerAccess after a write with the command word to ManufacturerAccess. ... A.1.1.5 Manufacturer Status(0x0006) [Brian's note: STATE{3..0} are bits[11:8] STATE3, STATE2, STATE1, STATE0 —Indicates the battery state. 0,0,0,0 = Wake Up 0,0,0,1 = Normal Discharge 0,0,1,1 = Pre-Charge 0,1,0,1 = Charge 0,1,1,1 = Charge Termination 1,0,0,1 = Permanent Failure 1,0,1,0 = Overcurrent 1,0,1,1 = Overtemperature 1,1,0,0 = Battery Failure 1,1,0,1 = Sleep 1,1,1,0 = Reserved 1,1,1,1 = Battery Pack Removed
,
May 31 2018
OK, so my plan is to restrict the current parsing of Manufacturer Access fields to the 'ti,bq20z75' compatible property that is already present, and otherwise (for batteries that are only claiming 'sbs,sbs-battery') do: * HEALTH: Report "Unknown" (I couldn't find a clear, standard command that gives the values the kernel power-supply framework is looking for) * PRESENT: Check whether REG_STATUS succeeds; if so, we're present That will sidestep this problem and possibly improve compatibility of this kernel driver in general. I've already cooked up a patch locally that works just fine on Scarlet and Kevin. Let me know if you have any thoughts.
,
Jun 1 2018
I decided to sync with upstream while we're at it. All our local patches were upstream already, and the rest were very straightforward to bring in (except 1 that used an I2C callback we don't have in 4.4, so I dropped it). Here's the whole stack. If y'all would prefer, I can pare this down: remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081686 Revert "FIXUP: CHROMIUM: sbs-battery: make writes to ManufacturerAccess ... remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081787 Revert "CHROMIUM: sbs-battery: make writes to ManufacturerAccess optional" remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081788 Revert "CHROMIUM: power: sbs-battery: Prevent CAPACITY_MODE races" remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081789 Revert "CHROMIUM: power: sbs-battery: Don't needlessly set CAPACITY_MODE" remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081790 UPSTREAM: power: sbs-battery: Use devm_kzalloc to alloc data remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081791 UPSTREAM: power: sbs-battery: Request threaded irq and fix dev callback cookie remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081792 UPSTREAM: power: sbs-battery: Use devm_power_supply_register remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081793 UPSTREAM: sbs-battery: add ability to get battery capacity remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081794 UPSTREAM: power: supply: sbs-battery: Use gpio_desc and sleeping calls for ... remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081795 UPSTREAM: power: supply: sbs-battery: simplify DT parsing remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081796 UPSTREAM: sbs-battery: make writes to ManufacturerAccess optional remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081797 UPSTREAM: power: supply: sbs-battery: Cleanup removal of chip->pdata remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081798 UPSTREAM: power: supply: sbs-battery: fix the sbs interrupt request remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081799 UPSTREAM: power: supply: sbs-battery: Don't ignore the first external power ... remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081800 UPSTREAM: power: supply: sbs-battery: Correct supply status with current draw remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081801 UPSTREAM: power: supply: sbs-battery: remove incorrect le16_to_cpu calls remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081802 UPSTREAM: power: supply: sbs-battery: Prevent CAPACITY_MODE races remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081803 UPSTREAM: power: supply: sbs-battery: Don't needlessly set CAPACITY_MODE remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081804 UPSTREAM: power: supply: sbs-battery: Remove FSF mailing address from comments remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081805 UPSTREAM: power: supply: sbs-battery: sort includes remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081806 UPSTREAM: power: supply: sbs-battery: Add delay when changing capacity mode bit remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081807 UPSTREAM: power: supply: sbs-battery: move gpio present detect to sbs_get_pro... remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081808 UPSTREAM: power: supply: sbs-battery: remove superfluous variable init remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081809 UPSTREAM: power: supply: sbs-battery: remove unchecked return var remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1081810 FROMLIST: power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/551f9e9ed43424e8eb0d0113ee396b783e36fe92 commit 551f9e9ed43424e8eb0d0113ee396b783e36fe92 Author: Brian Norris <briannorris@chromium.org> Date: Sat Jun 02 00:43:16 2018 Revert "FIXUP: CHROMIUM: sbs-battery: make writes to ManufacturerAccess optional" This reverts commit 8678441f4eb8fbb01b702e384e93c26d11c711da. We're going to replace this with the upstream version. CQ-DEPEND=I243b6db3e9a20f0e81a5942d61a7be305569255e BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I735373dc4779277e04598d2f5f3d9bf5c08ab0b8 Reviewed-on: https://chromium-review.googlesource.com/1081686 Commit-Ready: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/551f9e9ed43424e8eb0d0113ee396b783e36fe92/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/31303d108bbb367e0dc0f4d01bfe3e469550f906 commit 31303d108bbb367e0dc0f4d01bfe3e469550f906 Author: Brian Norris <briannorris@chromium.org> Date: Sat Jun 02 00:43:18 2018 Revert "CHROMIUM: sbs-battery: make writes to ManufacturerAccess optional" This reverts commit 57ffef3beb2be5d4fa842a07e88e8ba274856add. We're going to replace this with the upstream version. CQ-DEPEND=I243b6db3e9a20f0e81a5942d61a7be305569255e BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I330abe28b8860bce8715c3c31673fa7f05f56388 Reviewed-on: https://chromium-review.googlesource.com/1081787 Commit-Ready: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/31303d108bbb367e0dc0f4d01bfe3e469550f906/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/c17e8b7d32ac7f881c7d3ddf36d519f4e0589400 commit c17e8b7d32ac7f881c7d3ddf36d519f4e0589400 Author: Brian Norris <briannorris@chromium.org> Date: Sat Jun 02 00:43:19 2018 Revert "CHROMIUM: power: sbs-battery: Prevent CAPACITY_MODE races" This reverts commit 28878159180029badf429ae844b6a78221707ee8. We're going to replace this with the upstream version. CQ-DEPEND=I214d2d342ca34ea0ec0add4e90a6e72a3f32e917 BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I836c318600fdb05a4307216a07f42fffdc5d7741 Reviewed-on: https://chromium-review.googlesource.com/1081788 Commit-Ready: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/c17e8b7d32ac7f881c7d3ddf36d519f4e0589400/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/1b295d938cb2ad2b3985f122df565387567793a7 commit 1b295d938cb2ad2b3985f122df565387567793a7 Author: Brian Norris <briannorris@chromium.org> Date: Sat Jun 02 00:43:20 2018 Revert "CHROMIUM: power: sbs-battery: Don't needlessly set CAPACITY_MODE" This reverts commit a060991669c871de8c6188c6119690224e5f9b90. We're going to replace this with the upstream version. CQ-DEPEND=I29fcf42ed5e23dce5bbad07027d87bdeaad9f461 BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I7974e0ced404368c9335593a9b4c96357bede2db Reviewed-on: https://chromium-review.googlesource.com/1081789 Commit-Ready: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/1b295d938cb2ad2b3985f122df565387567793a7/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/d9f0a06fe471ced2916cfde72bb5c8f3785d1f49 commit d9f0a06fe471ced2916cfde72bb5c8f3785d1f49 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:22 2018 UPSTREAM: power: sbs-battery: Use devm_kzalloc to alloc data Use devm_kzalloc to allow memory to be freed automatically on driver probe failure or removal. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit 9239a86f0976b58d3da7a2261ed659ac9eba0f25) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I286d1dbcfd2b3dff1e8aea3f07b5e37e5208d08a Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081790 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/d9f0a06fe471ced2916cfde72bb5c8f3785d1f49/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/63b5491bdd0aba28e102c44485cf08bf8cf2a492 commit 63b5491bdd0aba28e102c44485cf08bf8cf2a492 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:23 2018 UPSTREAM: power: sbs-battery: Request threaded irq and fix dev callback cookie Currently the battery detect gpio can not be used with a chained interrupt controller that requires threaded irq handlers. Use threaded irq instead. In addition this was not going to be working at present because chip->power_supply is assigned after the request irq call. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit d2cec82c28802da31596b395ad292cb8f132fd63) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I6f7394e89557ca934a2488c80ee60cac4aba7590 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081791 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/63b5491bdd0aba28e102c44485cf08bf8cf2a492/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/35375a6b62dc09fd3981d4195131fdbb958e3e58 commit 35375a6b62dc09fd3981d4195131fdbb958e3e58 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:25 2018 UPSTREAM: power: sbs-battery: Use devm_power_supply_register Use devm_power_supply_register instead of power_supply_register. Remove call to power_supply_unregister. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit 492ff9d8f5fa6ad44288050238b7961d457a239d) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I601c0b9dd17debc686a99f07d9cee3cb36aa43eb Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081792 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/35375a6b62dc09fd3981d4195131fdbb958e3e58/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/6db299b8395d95db09f6deadf0e281d19474a2c4 commit 6db299b8395d95db09f6deadf0e281d19474a2c4 Author: Joshua Clayton <stillcompiling@gmail.com> Date: Sat Jun 02 00:43:26 2018 UPSTREAM: sbs-battery: add ability to get battery capacity Battery capacity level is a standard feature of sbs battery That can be used to tell what the remainig battery capacity is, and can tell if the battery has not been calibrated/initialized, which makes the capacity and charging/discharging percentages invalid. Signed-off-by: Joshua Clayton <stillcompiling@gmail.com> Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit 957cb720518341abf19009044f240a6342bdd039) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: If5bfc185845ff0b356be9b9a0b43e7637f359fde Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081793 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/6db299b8395d95db09f6deadf0e281d19474a2c4/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ddf8e5a6bbd2b284b0f3fb20c60427b37509b392 commit ddf8e5a6bbd2b284b0f3fb20c60427b37509b392 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:28 2018 UPSTREAM: power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect Switch to using new gpio_desc interface and devm gpio get calls to automatically manage gpio resource. Use gpiod_get_value which handles active high / low calls. If gpio_detect is set then force loading of the driver as it is reasonable to assume that the battery may not be present. Update the is_present flag immediately in the IRQ. Remove legacy gpio specification from platform data. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit 3b5dd3a49496220b35af83c96e3d2ff5716550ae) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: Iffd44662c1f659a0a55f032d9a99141c6de6dd5c Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081794 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/ddf8e5a6bbd2b284b0f3fb20c60427b37509b392/include/linux/power/sbs-battery.h [modify] https://crrev.com/ddf8e5a6bbd2b284b0f3fb20c60427b37509b392/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/2f87057a96f644e14893156c609f665416987687 commit 2f87057a96f644e14893156c609f665416987687 Author: Arnd Bergmann <arnd@arndb.de> Date: Sat Jun 02 00:43:29 2018 UPSTREAM: power: supply: sbs-battery: simplify DT parsing After the change to use the gpio descriptor interface, we get a warning if -Wmaybe-uninitialized is added back to the build flags (it is currently disabled: drivers/power/supply/sbs-battery.c: In function 'sbs_probe': drivers/power/supply/sbs-battery.c:760:28: error: 'pdata' may be used uninitialized in this function [-Werror=maybe-uninitialized] The problem is that if neither the DT properties nor a platform_data pointer are provided, the chip->pdata pointer gets set to an uninitialized value. Looking at the code some more, I found that the sbs_of_populate_pdata function is more complex than necessary and has confusing calling conventions of possibly returning a valid pointer, a NULL pointer or an ERR_PTR pointer (in addition to the uninitialized pointer). To fix all of that, this gets rid of the chip->pdata pointer and simply moves the two integers into the sbs_info structure. This makes it much clearer from reading sbs_probe() what the precedence of the three possible values are (pdata, DT, hardcoded defaults) and completely avoids the #ifdef CONFIG_OF guards as of_property_read_u32() gets replaced with a compile-time stub when that is disabled, and returns an error if passed a NULL of_node pointer. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: 3b5dd3a49496 ("power: supply: sbs-battery: Use gpio_desc and sleeping calls for battery detect") Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit 9edeaada19a21eb669ae0dfe749be88f1810ea92) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I3268b48ebcd1f923c2b86d89a58fb73440f3817d Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081795 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/2f87057a96f644e14893156c609f665416987687/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4b59bb8e375a4566bec69451128e8eed3f5de768 commit 4b59bb8e375a4566bec69451128e8eed3f5de768 Author: Guenter Roeck <linux@roeck-us.net> Date: Sat Jun 02 00:43:31 2018 UPSTREAM: sbs-battery: make writes to ManufacturerAccess optional According to the Smart Battery Data Specification, the use of ManufacturerAcess (register 0x0) is implementation-defined. It appears that some batteries use writes to this register in order to implement certain functionality, but others may simply NAK all writes to it. As a result, write failures to ManufacturerAccess should not be used as an indicator of battery presence, nor as a failure to enter sleep mode. The failed write access was seen with SANYO AP13J3K. Cc: Brian Norris <briannorris@chromium.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit 17c6d3979e5bbff1de36a4e89015fa09ac495d27) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I243b6db3e9a20f0e81a5942d61a7be305569255e Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081796 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/4b59bb8e375a4566bec69451128e8eed3f5de768/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/72727787bc4fda447e585fb7d696808cba661d43 commit 72727787bc4fda447e585fb7d696808cba661d43 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:32 2018 UPSTREAM: power: supply: sbs-battery: Cleanup removal of chip->pdata There where still a few lingering references to pdata after commit power: supply: sbs-battery: simplify DT parsing. Remove pdata from structsbs_info and conditional checks to ser if this was set from the i2c read / write functions. Instead of call max in each function for incrementing poll_retry_count do it once in the probe function. Fixup null pointer dereference in to pdata in sbs_external_power_changed. Change retry counts to u32 to avoid need for max. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sre@kernel.org> (cherry picked from commit 389958bb6be8b08c9f6d350dcaa9fc127123eada) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: Iaf90fb6422143866c30aa9b3c6e81754d9a80fd5 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081797 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/72727787bc4fda447e585fb7d696808cba661d43/include/linux/power/sbs-battery.h [modify] https://crrev.com/72727787bc4fda447e585fb7d696808cba661d43/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/64a870afbc7ad7c629246a6fe4fe0ee7ffa303a4 commit 64a870afbc7ad7c629246a6fe4fe0ee7ffa303a4 Author: Ryosuke Saito <raitosyo@gmail.com> Date: Sat Jun 02 00:43:34 2018 UPSTREAM: power: supply: sbs-battery: fix the sbs interrupt request Since we use the default primary handler for the irq, IRQF_ONESHOT must be set. Otherwise the request fails and the following errors are displayed: genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 129 sbs-battery 0-000b: Failed to request irq: -22 Signed-off-by: Ryosuke Saito <raitosyo@gmail.com> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit bb8fe8ea0067083e0452d5c67a4ab70ad72cc52f) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I72ae21d02e11fbb4f60822b6d53c8af211a73bfe Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081798 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/64a870afbc7ad7c629246a6fe4fe0ee7ffa303a4/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/d8c3d24cb0eb9684cd44e0de04b9d8832c7a9a20 commit d8c3d24cb0eb9684cd44e0de04b9d8832c7a9a20 Author: Paul Kocialkowski <contact@paulk.fr> Date: Sat Jun 02 00:43:35 2018 UPSTREAM: power: supply: sbs-battery: Don't ignore the first external power change A mechanism to ignore the first external power change notification was put in place years ago to ignore the power_supply_register notification. However, this doesn't apply to the current situation anymore, as the first notification is always the result of a legitimate power change. This removes this deprecated mechanism, which puts back the driver's state machine to a sane state (an ignored first notification previously caused a charging/discharging status inversion). Signed-off-by: Paul Kocialkowski <contact@paulk.fr> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit 4df2cce4722d35eb35bb9371e7c9a600a84558f9) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I98db747e3a9bb4881d03219f8442d825d407ed5d Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081799 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/d8c3d24cb0eb9684cd44e0de04b9d8832c7a9a20/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/dbc8cc301e690254d206da452e3141e0927a4c28 commit dbc8cc301e690254d206da452e3141e0927a4c28 Author: Paul Kocialkowski <contact@paulk.fr> Date: Sat Jun 02 00:43:37 2018 UPSTREAM: power: supply: sbs-battery: Correct supply status with current draw The status reported directly by the battery controller is not always reliable and should be corrected based on the current draw information. This implements such a correction with a dedicated function, called where the supply status is retrieved. Signed-off-by: Paul Kocialkowski <contact@paulk.fr> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit 7f93e1fa032bb5ee19b868b9649bc98c82553003) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: Icc40fa8ab912af82d42fd0a673759ed563687d28 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081800 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/dbc8cc301e690254d206da452e3141e0927a4c28/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0c8be2f8ffcf13c612498966b4766896472051d6 commit 0c8be2f8ffcf13c612498966b4766896472051d6 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:38 2018 UPSTREAM: power: supply: sbs-battery: remove incorrect le16_to_cpu calls i2c_smbus commands handle the correct byte order for smbus transactions internally. This will currently result in incorrect operation on big endian systems. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit a1bbec72f9fef100bb00762e36c20055deacd0e2) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I1408b03c87586ad18ac6f6ce35702d5b86cfa130 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081801 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/0c8be2f8ffcf13c612498966b4766896472051d6/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0944f41e28dafae75a4f8d3273360a6b00448681 commit 0944f41e28dafae75a4f8d3273360a6b00448681 Author: Shawn Nematbakhsh <shawnn@chromium.org> Date: Sat Jun 02 00:43:40 2018 UPSTREAM: power: supply: sbs-battery: Prevent CAPACITY_MODE races A subset of smart battery commands return charge or energy depending on the CAPACITY_MODE bit setting of BatteryMode(). In order to unambiguously read a charge or energy value, it is necessary to ensure that CAPACITY_MODE is set as desired, and not changed for the duration of the attribute read. Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit fe8a6534396807b8a0863cd146b72c0bce0b0c88) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I214d2d342ca34ea0ec0add4e90a6e72a3f32e917 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081802 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/0944f41e28dafae75a4f8d3273360a6b00448681/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/751967dfd9409109f8dfe0dc1c89b23e5ae8b78d commit 751967dfd9409109f8dfe0dc1c89b23e5ae8b78d Author: Shawn Nematbakhsh <shawnn@chromium.org> Date: Sat Jun 02 00:43:41 2018 UPSTREAM: power: supply: sbs-battery: Don't needlessly set CAPACITY_MODE According to the smart battery spec (1), the CAPACITY_MODE bit does not influence the value read from RelativeStateOfCharge(), so don't bother changing CAPACITY_MODE when doing such a read. (1) - Smart Battery Data Specification, Rev 1.1, Dec. 11, 1998 Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit bfa953d336cdd713f6968c85ca820ef22333dc35) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I29fcf42ed5e23dce5bbad07027d87bdeaad9f461 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081803 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/751967dfd9409109f8dfe0dc1c89b23e5ae8b78d/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/858e9d99b94551b6f782f95d88f0d94027198849 commit 858e9d99b94551b6f782f95d88f0d94027198849 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:43 2018 UPSTREAM: power: supply: sbs-battery: Remove FSF mailing address from comments checkpatch issued an error in having the FSF address in the comment. As address may change and Linux already includes a copy. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit 6d1eebc99bd5ed17ad6d8554be7b76bde2238e4a) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: Ib34b10157775266410be155d157a8229cf4f2719 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081804 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/858e9d99b94551b6f782f95d88f0d94027198849/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/c18810fd341e6f310a03af118871dfc94f4c4ab4 commit c18810fd341e6f310a03af118871dfc94f4c4ab4 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:44 2018 UPSTREAM: power: supply: sbs-battery: sort includes Sort the header includes prior to adding to the list. Signed-off-by: Phil Reid <preid@electromag.com.au> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit b70f0a28966082d5392f90c2d4cb1514a2d65ac6) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I01d9e586b24e8d2682306b1c6d8ad00067b8efee Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081805 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/c18810fd341e6f310a03af118871dfc94f4c4ab4/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ab4ce1f28b75f62bd2171eea3256a07866969a3a commit ab4ce1f28b75f62bd2171eea3256a07866969a3a Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:45 2018 UPSTREAM: power: supply: sbs-battery: Add delay when changing capacity mode bit At least with the Inspired Energy compatible batteries a delay is required after setting the capacity mode bit from amp to watts or the reverse. Setting the bit and then immediately pooling the status register results in an unknown error being returned in the register. Add the delay results in and ok status being return. This was also seen when reading the charge and energy registers where the wrong value was returned for the requested mode. Signed-off-by: Phil Reid <preid@electromag.com.au> Tested-by: Michael Heinemann <committed@heine.so> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> (cherry picked from commit adcf04c9f8fce54e65f11bae2fbd37c1282ef128) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I9ef34215bb70f809f09b2e737fcffca99379c990 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081806 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/ab4ce1f28b75f62bd2171eea3256a07866969a3a/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ca66470926eae454436b7dc86397a3376a18e00c commit ca66470926eae454436b7dc86397a3376a18e00c Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 02 00:43:47 2018 UPSTREAM: power: supply: sbs-battery: move gpio present detect to sbs_get_property Currently when a gpio is defined for battery presence it is only used in the sbs_get_battery_presence_and_health function for 2 properties. All other properties currently try to read data form the battery before returning an error if not present. We should know in advance that no data is going to returned. As the driver tries multiple times to access a property, this prevents a lot of smbus accesses, which had a significant effect on device boot-up. As when the device is registered lots of property accesses are attempted during boot. If no gpio is used for presence detection no change in behaviour should occur. Signed-off-by: Phil Reid <preid@electromag.com.au> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 1cf855535b034579ac4d39f49da853594ce968dc) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I73ec8bc7b8dc796f94775f4e4efc29ed356b6fd8 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081807 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/ca66470926eae454436b7dc86397a3376a18e00c/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/823d6949792e70c85654d4ef5571b993d1870d22 commit 823d6949792e70c85654d4ef5571b993d1870d22 Author: Wolfram Sang <wsa@the-dreams.de> Date: Sat Jun 02 00:43:48 2018 UPSTREAM: power: supply: sbs-battery: remove superfluous variable init Those variables are immediately assigned a value afterwards. Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 9410b7d71032cc76c4211cbf8f76bad322f0c334) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I8e86dd74034976a7bc22c4e91aa38891b35442a3 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081808 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/823d6949792e70c85654d4ef5571b993d1870d22/drivers/power/sbs-battery.c
,
Jun 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/d4fc9bb14c0213cf9c13363cef9ccc0afd59979f commit d4fc9bb14c0213cf9c13363cef9ccc0afd59979f Author: Wolfram Sang <wsa@the-dreams.de> Date: Sat Jun 02 00:43:50 2018 UPSTREAM: power: supply: sbs-battery: remove unchecked return var Since the return value is not checked anyhow, we don't need to store it. Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 5e9bee5267fe626df39ca04d6c30e135a9e8b157) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I09d156e7ec3bba640a4c15524780e2b916ca382e Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081809 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/d4fc9bb14c0213cf9c13363cef9ccc0afd59979f/drivers/power/sbs-battery.c
,
Jun 2 2018
Copying some comments from the CL here, for posterity: Per Doug's suggestion, I took a look through the EC smart battery driver (both commit logs and code itself) to see if there was any knowledge to be gleaned there...it looks like that driver: (a) doesn't interpret ManufacturerAccess at all (there's only a console command to read it if you want) (b) performs something similar to a "presence" check by declaring the battery "responsive" if any of its basic status commands succeed (c) doesn't seem to try to determine a "health" status So, seems in line with this patch. (Oh, and by the way, I believe I said _DATA here in some places where I meant _ACCESS. Oh well.)
,
Jun 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/90724867be1ba138cd8f5a3ff082c5bc35927888 commit 90724867be1ba138cd8f5a3ff082c5bc35927888 Author: Brian Norris <briannorris@chromium.org> Date: Tue Jun 05 00:23:42 2018 FROMLIST: power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats This driver was originally submitted for the TI BQ20Z75 battery IC (commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge IC")) and later renamed to express generic SBS support. While it's mostly true that this driver implemented a standard SBS command set, it takes liberties with the REG_MANUFACTURER_DATA register. This register is specified in the SBS spec, but it doesn't make any mention of what its actual contents are. We've sort of noticed this optionality previously, with commit 17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess optional"), where we found that some batteries NAK writes to this register. What this really means is that so far, we've just been lucky that most batteries have either been compatible with the TI chip, or else at least haven't reported highly-unexpected values. For instance, one battery I have here seems to report either 0x0000 or 0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to match either Wake Up (bits[11:8] = 0000b) or Normal Discharge (bits[11:8] = 0001b) status for the TI part [1], they don't seem to actually correspond to real states (for instance, I never see 0101b = Charge, even when charging). On other batteries, I'm getting apparently random data in return, which means that occasionally, we interpret this as "battery not present" or "battery is not healthy". All in all, it seems to be a really bad idea to make assumptions about REG_MANUFACTURER_DATA, unless we already know what battery we're using. Therefore, this patch reimplements the "present" and "health" checks to the following on most SBS batteries: 1. HEALTH: report "unknown" -- I couldn't find a standard SBS command that gives us much useful here 2. PRESENT: just send a REG_STATUS command; if it succeeds, then the battery is present Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have no proof that this is useful and supported. If someone explicitly provided a 'ti,bq20z75' compatible property, then we retain the existing TI command behaviors. [1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <linux@roeck-us.net> Acked-by: Rhyland Klein <rklein@nvidia.com> (am from https://patchwork.kernel.org/patch/10444489/) BUG= chromium:848112 TEST=kevin and scarlet battery; stress-test the 'present' field Change-Id: I171dd41dc31e898fd282067e4d68382d65f3089b Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081810 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/90724867be1ba138cd8f5a3ff082c5bc35927888/drivers/power/sbs-battery.c
,
Jun 5 2018
Kernel 4.4 should be fixed. 4.14 CLs are here, though I don't have a good device to test with: remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1086368 UPSTREAM: power: supply: sbs-battery: move gpio present detect to sbs_get_pro... remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1086369 UPSTREAM: power: supply: sbs-battery: remove superfluous variable init remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1086370 UPSTREAM: power: supply: sbs-battery: remove unchecked return var remote: https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/1086371 FROMLIST: power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats I don't think there's much else to do on the EC side, unless we decide that the EC should still provide *something* to MANUFACTURER_ACCESS (since it's technically part of the spec...).
,
Jun 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/3aa86a42927abb3e584cd2be2aea9c2d4328b101 commit 3aa86a42927abb3e584cd2be2aea9c2d4328b101 Author: Phil Reid <preid@electromag.com.au> Date: Sat Jun 09 09:19:10 2018 UPSTREAM: power: supply: sbs-battery: move gpio present detect to sbs_get_property Currently when a gpio is defined for battery presence it is only used in the sbs_get_battery_presence_and_health function for 2 properties. All other properties currently try to read data form the battery before returning an error if not present. We should know in advance that no data is going to returned. As the driver tries multiple times to access a property, this prevents a lot of smbus accesses, which had a significant effect on device boot-up. As when the device is registered lots of property accesses are attempted during boot. If no gpio is used for presence detection no change in behaviour should occur. Signed-off-by: Phil Reid <preid@electromag.com.au> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 1cf855535b034579ac4d39f49da853594ce968dc) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I73ec8bc7b8dc796f94775f4e4efc29ed356b6fd8 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081807 Reviewed-by: Guenter Roeck <groeck@chromium.org> (cherry picked from commit ca66470926eae454436b7dc86397a3376a18e00c) Reviewed-on: https://chromium-review.googlesource.com/1086368 [modify] https://crrev.com/3aa86a42927abb3e584cd2be2aea9c2d4328b101/drivers/power/supply/sbs-battery.c
,
Jun 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e3bc98608ada6508b6f5fd89c3d1901804d9f315 commit e3bc98608ada6508b6f5fd89c3d1901804d9f315 Author: Wolfram Sang <wsa@the-dreams.de> Date: Sat Jun 09 09:19:12 2018 UPSTREAM: power: supply: sbs-battery: remove superfluous variable init Those variables are immediately assigned a value afterwards. Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 9410b7d71032cc76c4211cbf8f76bad322f0c334) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I8e86dd74034976a7bc22c4e91aa38891b35442a3 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081808 Reviewed-by: Guenter Roeck <groeck@chromium.org> (cherry picked from commit 823d6949792e70c85654d4ef5571b993d1870d22) Reviewed-on: https://chromium-review.googlesource.com/1086369 [modify] https://crrev.com/e3bc98608ada6508b6f5fd89c3d1901804d9f315/drivers/power/supply/sbs-battery.c
,
Jun 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4093e42f091d35b607e242eaa0571e3626381892 commit 4093e42f091d35b607e242eaa0571e3626381892 Author: Wolfram Sang <wsa@the-dreams.de> Date: Sat Jun 09 09:19:14 2018 UPSTREAM: power: supply: sbs-battery: remove unchecked return var Since the return value is not checked anyhow, we don't need to store it. Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 5e9bee5267fe626df39ca04d6c30e135a9e8b157) BUG= chromium:848112 TEST=kevin and scarlet battery Change-Id: I09d156e7ec3bba640a4c15524780e2b916ca382e Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1081809 Reviewed-by: Guenter Roeck <groeck@chromium.org> (cherry picked from commit d4fc9bb14c0213cf9c13363cef9ccc0afd59979f) Reviewed-on: https://chromium-review.googlesource.com/1086370 [modify] https://crrev.com/4093e42f091d35b607e242eaa0571e3626381892/drivers/power/supply/sbs-battery.c
,
Jun 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ba37bb4a72f7789f4272f52ef2333571d979312a commit ba37bb4a72f7789f4272f52ef2333571d979312a Author: Brian Norris <briannorris@chromium.org> Date: Wed Jun 13 04:50:59 2018 FROMLIST: power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats This driver was originally submitted for the TI BQ20Z75 battery IC (commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge IC")) and later renamed to express generic SBS support. While it's mostly true that this driver implemented a standard SBS command set, it takes liberties with the REG_MANUFACTURER_DATA register. This register is specified in the SBS spec, but it doesn't make any mention of what its actual contents are. We've sort of noticed this optionality previously, with commit 17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess optional"), where we found that some batteries NAK writes to this register. What this really means is that so far, we've just been lucky that most batteries have either been compatible with the TI chip, or else at least haven't reported highly-unexpected values. For instance, one battery I have here seems to report either 0x0000 or 0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to match either Wake Up (bits[11:8] = 0000b) or Normal Discharge (bits[11:8] = 0001b) status for the TI part [1], they don't seem to actually correspond to real states (for instance, I never see 0101b = Charge, even when charging). On other batteries, I'm getting apparently random data in return, which means that occasionally, we interpret this as "battery not present" or "battery is not healthy". All in all, it seems to be a really bad idea to make assumptions about REG_MANUFACTURER_DATA, unless we already know what battery we're using. Therefore, this patch reimplements the "present" and "health" checks to the following on most SBS batteries: 1. HEALTH: report "unknown" -- I couldn't find a standard SBS command that gives us much useful here 2. PRESENT: just send a REG_STATUS command; if it succeeds, then the battery is present Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have no proof that this is useful and supported. If someone explicitly provided a 'ti,bq20z75' compatible property, then we continue to use the existing TI command behaviors, and we effectively revert commit 17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess optional") to again make these commands required. [1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <linux@roeck-us.net> Acked-by: Rhyland Klein <rklein@nvidia.com> (am from https://patchwork.kernel.org/patch/10455285/) BUG= chromium:848112 TEST=kevin and scarlet battery; stress-test the 'present' field Change-Id: I171dd41dc31e898fd282067e4d68382d65f3089b Reviewed-on: https://chromium-review.googlesource.com/1086371 Commit-Ready: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/ba37bb4a72f7789f4272f52ef2333571d979312a/drivers/power/supply/sbs-battery.c
,
Jun 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/1400905161cee7f1db746dec8a68619dcf5a6261 commit 1400905161cee7f1db746dec8a68619dcf5a6261 Author: Brian Norris <briannorris@chromium.org> Date: Wed Jun 13 08:01:00 2018 FROMLIST: dt-bindings: power: sbs-battery: re-document "ti,bq20z75" This compatible property was documented before the driver was renamed to "SBS" (see commit e57f1b68c406 ("devicetree-bindings: Propagate bq20z75->sbs rename to dt bindings")). The driver has continued to support this property as an alternative to "sbs,sbs-battery", and because we've noticed there are some lingering TI specifics (in the manufacturer-specific portion of the SBS spec), we'd like to start using this property again to differentiate. Signed-off-by: Brian Norris <briannorris@chromium.org> Acked-by: Rhyland Klein <rklein@nvidia.com> Reviewed-by: Rob Herring <robh@kernel.org> (am from https://patchwork.kernel.org/patch/10455287/) BUG= chromium:848112 TEST=None Change-Id: I133c3ff155a3bbf2d5522857d25d9f0d939e6ff9 Reviewed-on: https://chromium-review.googlesource.com/1093955 Commit-Ready: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> [modify] https://crrev.com/1400905161cee7f1db746dec8a68619dcf5a6261/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
,
Jun 13 2018
Alright, kernels 4.4 and 4.14 (and soon, upstream) don't use these MANUFACTURER_* commands now, unless we explicitly state which vendor/part we're using (e.g., in the device tree). We probably don't need to do anything on the EC side -- this function is actually completely optional, not just implementation-defined: "This function is optional and its meaning is implementation specific." |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by briannorris@chromium.org
, May 31 2018