From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rhyland Klein Subject: Re: [PATCH] sbs-battery: fix power status when battery is dry Date: Tue, 29 Mar 2016 11:05:52 -0400 Message-ID: <56FA99D0.1010004@nvidia.com> References: <1458726794-48298-1-git-send-email-yh.huang@mediatek.com> <1458801832.16645.7.camel@mtksdaap41> <1459132330.16645.14.camel@mtksdaap41> <56F95451.5040607@nvidia.com> <1459216353.16645.20.camel@mtksdaap41> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <1459216353.16645.20.camel@mtksdaap41> Sender: linux-kernel-owner@vger.kernel.org To: YH Huang Cc: Daniel Kurtz , linux-pm@vger.kernel.org, Dmitry Eremin-Solenikov , Sebastian Reichel , "linux-kernel@vger.kernel.org" , "moderated list:ARM/Mediatek SoC support" , Matthias Brugger , David Woodhouse , "linux-arm-kernel@lists.infradead.org" List-Id: linux-pm@vger.kernel.org On 3/28/2016 9:52 PM, YH Huang wrote: > On Mon, 2016-03-28 at 11:57 -0400, Rhyland Klein wrote: >> On 3/28/2016 6:05 AM, Daniel Kurtz wrote: >>> +Rhyland Klein who original wrote this code... >>> >>> On Mon, Mar 28, 2016 at 10:32 AM, YH Huang = wrote: >>>> >>>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: >>>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang = wrote: >>>>>> >>>>>> Hi Daniel, >>>>>> >>>>>> On Thu, 2016-03-24 at 12:01 +0800, Daniel Kurtz wrote: >>>>>>> Hi YH, >>>>>>> >>>>>>> On Wed, Mar 23, 2016 at 5:53 PM, YH Huang wrote: >>>>>>>> When the battery is dry and BATTERY_FULL_DISCHARGED is set, >>>>>>>> we should check BATTERY_DISCHARGING to decide the power status= =2E >>>>>>>> If BATTERY_DISCHARGING is set, the power status is not chargin= g. >>>>>>>> Or the power status should be charging. >>>>>>>> >>>>>>>> Signed-off-by: YH Huang >>>>>>>> --- >>>>>>>> drivers/power/sbs-battery.c | 22 ++++++++++++---------- >>>>>>>> 1 file changed, 12 insertions(+), 10 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/power/sbs-battery.c b/drivers/power/sbs-b= attery.c >>>>>>>> index d6226d6..d86db0e 100644 >>>>>>>> --- a/drivers/power/sbs-battery.c >>>>>>>> +++ b/drivers/power/sbs-battery.c >>>>>>>> @@ -382,11 +382,12 @@ static int sbs_get_battery_property(stru= ct i2c_client *client, >>>>>>>> >>>>>>>> if (ret & BATTERY_FULL_CHARGED) >>>>>>>> val->intval =3D POWER_SUPPLY_STATUS_FU= LL; >>>>>>>> - else if (ret & BATTERY_FULL_DISCHARGED) >>>>>>>> - val->intval =3D POWER_SUPPLY_STATUS_NO= T_CHARGING; >>>>>>>> - else if (ret & BATTERY_DISCHARGING) >>>>>>>> - val->intval =3D POWER_SUPPLY_STATUS_DI= SCHARGING; >>>>>>>> - else >>>>>>>> + else if (ret & BATTERY_DISCHARGING) { >>>>>>>> + if (ret & BATTERY_FULL_DISCHARGED) >>>>>>>> + val->intval =3D POWER_SUPPLY_S= TATUS_NOT_CHARGING; >>>>>>>> + else >>>>>>>> + val->intval =3D POWER_SUPPLY_S= TATUS_DISCHARGING; >>>>>>>> + } else >>>>>>> >>>>>>> >>>>>>> I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is sti= ll >>>>>>> POWER_SUPPLY_STATUS_DISCHARGING. >>>>>>> So, let's just report what the battery says and do: >>>>>>> >>>>>>> else if (ret & BATTERY_DISCHARGING) >>>>>>> val->intval =3D POWER_SUPPLY_STA= TUS_DISCHARGING; >>>>>>> >>>>>> So we just ignore the special situation (BATTERY_DISCHARGING && >>>>>> BATTERY_FULL_DISCHARGED). >>>>>> Isn't POWER_SUPPLY_STATUS_NOT_CHARGING a useful information? >>>>> >>>>> The battery is discharging. The fact that it is also reporting t= hat >>>>> it is already "discharged" just seems premature. I would expect= to >>>>> only see NOT_CHARGING if completely discharged *and* not discharg= ing. >>>> >>>> I check the "Smart Battery Data Specification Revision 1.1". >>>> And there are some words about FULLY_DISCHARGED. >>>> "Discharge should be stopped soon." >>>> "This status bit may be set prior to the >>>> =E2=80=98TERMINATE_DISCHARGE_ALARM=E2=80=99 as an early or first l= evel warning of end of >>>> battery charge." >>>> It looks like the FULLY_DISCHARGED status is used to announce the >>>> warning of battery charge and it is still discharging if there is = no one >>>> takes care of it. >> >> >> The only difference I see in the patch above is that in the case whe= re >> DISCHARGING isn't set, it won't check FULL_DISCHARGE. Nothing seems = to >> be changed in the case where FULL_DISCHARGE & DISCHARGING are set. >=20 > If battery is dry(FULLY_DISCHARGED) and is charging(No > BATTERY_DISCHARGING) by AC at the same time, > I think it is better to mark as POWER_SUPPLY_STATUS_CHARGING. > Is this right? > Hmm. I can see where you patch would address that situation. From the spec, it looks like its expected that the flags should look something like this: capacity (in the course of running from fully_charged to dry to recharging...) full: FULLY_CHARGED high->low: DISCHARGING ~0%: (DISCHARGING & FULLY_DISCHARGED) ->~20%: FULLY_DISCHARGED >~20%: =3D charging =46rom this understanding, it seems like we can't expect FULLY_DISCHARG= ED to ever be the only flag, nor can we expect it to go away when the system is initially plugged in. In light of this, I can see why your patch is preferable to the existing code, as the existing code could imply that the system is either still near 0% when it is in fact charging. Of course, ideally the status returned would be "LOW BUT CHARGING" but I can see how CHARGING seems like a better option. I think this patch would be fine if we wanted to cover that case, thoug= h if we do merge this, we should probably flush out the patch description better to clarify why we have to treat FULLY_DISCHARGED as only applicable while DISCHARGING. This, IMHO, isn't because the =46ULLY_DISCHARGED flag comes on early, but rather because it doesn't t= urn off until ~20%. -rhyland --=20 nvpublic