From mboxrd@z Thu Jan 1 00:00:00 1970 From: Phil Reid Subject: Re: sbs-battery: Reading CHARGE in uAh Date: Mon, 10 Jul 2017 09:35:56 +0800 Message-ID: <5aaaba0a-d52e-eb0f-cbb0-24a0078aa059@electromag.com.au> References: <164cda30-47ba-5289-a6c8-4740752b047d@electromag.com.au> <51e4c77c0a6a5fb9d31d2d79bb998235@heine.so> <058c8617-3b9b-58b8-1fae-056ee90fed80@electromag.com.au> <1c63583aadf77510ce85946f7fe3c815@heine.so> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from anchovy2.45ru.net.au ([203.30.46.146]:59763 "EHLO anchovy.45ru.net.au" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752838AbdGJBgB (ORCPT ); Sun, 9 Jul 2017 21:36:01 -0400 In-Reply-To: <1c63583aadf77510ce85946f7fe3c815@heine.so> Content-Language: en-AU Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Michael Heinemann Cc: linux-pm@vger.kernel.org On 7/07/2017 21:06, Michael Heinemann wrote: > On 2017-07-07 10:27, Phil Reid wrote: >> On 7/07/2017 16:01, Michael Heinemann wrote: >>> Hi Phil, >>> On 2017-07-07 03:49, Phil Reid wrote: >>>> G'day Michael, >>>> On 6/07/2017 23:23, Michael Heinemann wrote: >>>>> >>>>> >>>>> I got an INSPIRED ENERGY NH2057 Battery which is compliant to SBS Specification Version 1.1. >>>>> >>>>> The Battery is generally working but I noticed that the values I get from sysfs for the CHARGE values >>>>> are identical to the values I get as ENERGY and they are in uWh. According to the power_supply_class >>>>> documentation CHARGE should be in uAh and ENERGY in uWh. >>>>> >>>>> So I took a look into the drivers/power/supply/sbs-battery.c and noticed a possible bug in the code >>>>> when switching from BATTERY_MODE_WATTS to BATTERY_MODE_AMPS: >>>>> >>>>> The configuration bit, according to the SBS Spec, is bit 15 of the BatteryMode Register. >>>>> >>>>> These two modes are reflected in the code as: >>>>> >>>>> enum sbs_battery_mode { >>>>> BATTERY_MODE_AMPS, >>>>> BATTERY_MODE_WATTS >>>>> }; >>>>> >>>>> When I look into the 'sbs_set_battery_mode' function I don't see any point where these mode bits are >>>>> shifted to bit 15. When I debug I only see the register being changed from 0x8000 to 0x8001 and back. >>>>> >>>>> So I'm a bit confused, is this actually working for somebody? I see in the 'for-next' branch [1] that >>>>> there is currently work done to add a mutex to the mode switch. So at least it seems to be working for >>>>> some people. But I don't see why. Am I missing something? >>>>> >>>>> >>>> >>>> Your analyse looks correct. I've not look that closely at the data >>>> from the battery as yet. >>>> There's looks to be a few issues in there related to switching mode. >>>> >>>> The return value from sbs_set_battery_mode also looks suspect. >>>> Perhaps it should just return 0 on success. >>>> >>>> And then sbs_get_battery_capacity needs cleaning up as it's using the >>>> return value to set the mode at the end. >>>> >>>> It setts the mode, and then I guess the intention was to set it back again. >>>> But do we event need to do that? The next get is going to set the correct mode. >>>> And the mode setting will soon be wrapped in a mutex. >>> >>> I spend some more time to look at the code and my conclusion is that the cause >>> of the problem is a mix up of two different approaches for the mode. >>> >>> The initial mode is passed in as 0 or 1 coming from enum sbs_battery_mode. >>> But when the setting back is done the value comes from the return value of >>> the previous call to sbs_set_battery_mode which is 0x8000 or 0x0000. >>> >>> At least for my battery this leads to a situation where the status register is >>> 0x8000 so it's in mWh Mode. Then the mode=0 is passed in to set uAh. >>> >>> But this statement >>> >>> if ((original_val & BATTERY_MODE_MASK) == mode) >>> return mode; >>> >>> is true by mistake ((0x8000 & 0x8000) == 0) so the function exits and never >>> sets anything. >> Yeap that what I thought. >> >>> >>> What would be the "kernel style" way to fix this? >> That I'm not sure of. My personal style would be against using the enum value. >> Unless is was set to a constant. ie: >> BATTERY_MODE_AMPS = 0, >> BATTERY_MODE_WATTS = 0x8000 >> But I'd probably make the enum a #define instead with above value >> >> >>> Define the sbs_battery_mode like this: >>> >>> enum sbs_battery_mode { >>> BATTERY_MODE_AMPS, >>> BATTERY_MODE_WATTS >>> }; >>> >>> Or add a #define CAPACITY_MODE_OFFSET 0xf and shift the mode bit in the function? >>> >>> I tested a bit with the shifted bit and the added mutex and get I correct values now. >>> Without the mutex it's more or less randomly changing between uAh and mWh. >>> I would writeup a patch and submit it as proposal. >>> >>> From my understanding of the SBS Spec (Page 19) the intention of the Mode setting is >>> to select one Mode for your power source and leave it at that. For Example the AtRate() >>> calculations become wrong when you change the mode. But as the driver is not using >>> these features that might not be an issue. >> Had a quick look at that and it'd have to do the same thing as the >> sbs_get_battery_capacity >> call and use the mutex if it was implemented. Don't think we need to >> wrory about it at present thou. >> >>> >>> I would agree that the driver does not need to change back the Mode bit as it is >>> set on every read anyway. So this is unnecessary overhead. >>> If I find time I'll submit a patch to remove this and clean up a bit. >>> >> I like any speed improvement. It takes a while to poll all the values >> from the battery. > > I did some more testing and came to a bit disappointing outcome. I > don't know if this is specific to my battery or a general thing with > SBS devices. > When I set the CAPACITY_MODE bit the change is not always effective > immediately. Sometimes it works, sometimes it takes a few ms and > sometimes the bit never changes and I have to write it again. > > So there seems to be no guarantee to get the reading in the expected > format. > > How could this be handled? Return an error when the CAPACITY_MODE bit > was not set succesfully? Retry setting it (which takes time)? Where you getting an i2c error or it just failing to set. I've got a number of different Inspired Energy batteries on hand that I can test against. I'll try and run some tests with you patch today and see what I get with them. I have had problems with some older models not reporting data correctly under some conditions. Inspired energy acknowledged a bug in the battery at the time. We've been using them for a while, but not with the linux driver and I've not used the mode bit before. > > I guess if some service is using the values from the battery it > normaly relies on ENERGY or CHARGE and not both. So not setting > back the mode after each read would be a first step in the > right direction. What do you think? > -- Regards Phil Reid