| Message ID | 1458726794-48298-1-git-send-email-yh.huang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi YH, On Wed, Mar 23, 2016 at 5:53 PM, YH Huang <yh.huang@mediatek.com> wrote: > When the battery is dry and BATTERY_FULL_DISCHARGED is set, > we should check BATTERY_DISCHARGING to decide the power status. > If BATTERY_DISCHARGING is set, the power status is not charging. > Or the power status should be charging. > > Signed-off-by: YH Huang <yh.huang@mediatek.com> > --- > 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-battery.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(struct i2c_client *client, > > if (ret & BATTERY_FULL_CHARGED) > val->intval = POWER_SUPPLY_STATUS_FULL; > - else if (ret & BATTERY_FULL_DISCHARGED) > - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > - else if (ret & BATTERY_DISCHARGING) > - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > - else > + else if (ret & BATTERY_DISCHARGING) { > + if (ret & BATTERY_FULL_DISCHARGED) > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > + else > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + } else I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still POWER_SUPPLY_STATUS_DISCHARGING. So, let's just report what the battery says and do: else if (ret & BATTERY_DISCHARGING) val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > val->intval = POWER_SUPPLY_STATUS_CHARGING; > > if (chip->poll_time == 0) > @@ -702,11 +703,12 @@ static void sbs_delayed_work(struct work_struct *work) > > if (ret & BATTERY_FULL_CHARGED) > ret = POWER_SUPPLY_STATUS_FULL; > - else if (ret & BATTERY_FULL_DISCHARGED) > - ret = POWER_SUPPLY_STATUS_NOT_CHARGING; > - else if (ret & BATTERY_DISCHARGING) > - ret = POWER_SUPPLY_STATUS_DISCHARGING; > - else > + else if (ret & BATTERY_DISCHARGING) { > + if (ret & BATTERY_FULL_DISCHARGED) > + ret = POWER_SUPPLY_STATUS_NOT_CHARGING; > + else > + ret = POWER_SUPPLY_STATUS_DISCHARGING; > + } else > ret = POWER_SUPPLY_STATUS_CHARGING; > > if (chip->last_state != ret) { > -- > 1.7.9.5 >
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 <yh.huang@mediatek.com> wrote: > > When the battery is dry and BATTERY_FULL_DISCHARGED is set, > > we should check BATTERY_DISCHARGING to decide the power status. > > If BATTERY_DISCHARGING is set, the power status is not charging. > > Or the power status should be charging. > > > > Signed-off-by: YH Huang <yh.huang@mediatek.com> > > --- > > 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-battery.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(struct i2c_client *client, > > > > if (ret & BATTERY_FULL_CHARGED) > > val->intval = POWER_SUPPLY_STATUS_FULL; > > - else if (ret & BATTERY_FULL_DISCHARGED) > > - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > - else if (ret & BATTERY_DISCHARGING) > > - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > - else > > + else if (ret & BATTERY_DISCHARGING) { > > + if (ret & BATTERY_FULL_DISCHARGED) > > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > + else > > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > + } else > > > I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still > POWER_SUPPLY_STATUS_DISCHARGING. > So, let's just report what the battery says and do: > > else if (ret & BATTERY_DISCHARGING) > val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > So we just ignore the special situation (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED). Isn't POWER_SUPPLY_STATUS_NOT_CHARGING a useful information? Regards, YH Huang
On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: > > > When the battery is dry and BATTERY_FULL_DISCHARGED is set, > > > we should check BATTERY_DISCHARGING to decide the power status. > > > If BATTERY_DISCHARGING is set, the power status is not charging. > > > Or the power status should be charging. > > > > > > Signed-off-by: YH Huang <yh.huang@mediatek.com> > > > --- > > > 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-battery.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(struct i2c_client *client, > > > > > > if (ret & BATTERY_FULL_CHARGED) > > > val->intval = POWER_SUPPLY_STATUS_FULL; > > > - else if (ret & BATTERY_FULL_DISCHARGED) > > > - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > > - else if (ret & BATTERY_DISCHARGING) > > > - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > > - else > > > + else if (ret & BATTERY_DISCHARGING) { > > > + if (ret & BATTERY_FULL_DISCHARGED) > > > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > > + else > > > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > > + } else > > > > > > I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still > > POWER_SUPPLY_STATUS_DISCHARGING. > > So, let's just report what the battery says and do: > > > > else if (ret & BATTERY_DISCHARGING) > > val->intval = POWER_SUPPLY_STATUS_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 that it is already "discharged" just seems premature. I would expect to only see NOT_CHARGING if completely discharged *and* not discharging. > > > Regards, > YH Huang >
On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: > On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: > > > > When the battery is dry and BATTERY_FULL_DISCHARGED is set, > > > > we should check BATTERY_DISCHARGING to decide the power status. > > > > If BATTERY_DISCHARGING is set, the power status is not charging. > > > > Or the power status should be charging. > > > > > > > > Signed-off-by: YH Huang <yh.huang@mediatek.com> > > > > --- > > > > 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-battery.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(struct i2c_client *client, > > > > > > > > if (ret & BATTERY_FULL_CHARGED) > > > > val->intval = POWER_SUPPLY_STATUS_FULL; > > > > - else if (ret & BATTERY_FULL_DISCHARGED) > > > > - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > > > - else if (ret & BATTERY_DISCHARGING) > > > > - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > > > - else > > > > + else if (ret & BATTERY_DISCHARGING) { > > > > + if (ret & BATTERY_FULL_DISCHARGED) > > > > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > > > + else > > > > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > > > + } else > > > > > > > > > I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still > > > POWER_SUPPLY_STATUS_DISCHARGING. > > > So, let's just report what the battery says and do: > > > > > > else if (ret & BATTERY_DISCHARGING) > > > val->intval = POWER_SUPPLY_STATUS_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 that > it is already "discharged" just seems premature. I would expect to > only see NOT_CHARGING if completely discharged *and* not discharging. 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 ‘TERMINATE_DISCHARGE_ALARM’ as an early or first level 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.
+Rhyland Klein who original wrote this code... On Mon, Mar 28, 2016 at 10:32 AM, YH Huang <yh.huang@mediatek.com> wrote: > > On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: > > On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: > > > > > When the battery is dry and BATTERY_FULL_DISCHARGED is set, > > > > > we should check BATTERY_DISCHARGING to decide the power status. > > > > > If BATTERY_DISCHARGING is set, the power status is not charging. > > > > > Or the power status should be charging. > > > > > > > > > > Signed-off-by: YH Huang <yh.huang@mediatek.com> > > > > > --- > > > > > 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-battery.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(struct i2c_client *client, > > > > > > > > > > if (ret & BATTERY_FULL_CHARGED) > > > > > val->intval = POWER_SUPPLY_STATUS_FULL; > > > > > - else if (ret & BATTERY_FULL_DISCHARGED) > > > > > - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > > > > - else if (ret & BATTERY_DISCHARGING) > > > > > - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > > > > - else > > > > > + else if (ret & BATTERY_DISCHARGING) { > > > > > + if (ret & BATTERY_FULL_DISCHARGED) > > > > > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > > > > + else > > > > > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > > > > + } else > > > > > > > > > > > > I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still > > > > POWER_SUPPLY_STATUS_DISCHARGING. > > > > So, let's just report what the battery says and do: > > > > > > > > else if (ret & BATTERY_DISCHARGING) > > > > val->intval = POWER_SUPPLY_STATUS_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 that > > it is already "discharged" just seems premature. I would expect to > > only see NOT_CHARGING if completely discharged *and* not discharging. > > 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 > ‘TERMINATE_DISCHARGE_ALARM’ as an early or first level 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. Sure, it is in the sbs spec. That is why the battery is setting that bit. The problem isn't with what the battery is doing, the issue here is in how the kernel is interpreting it, and what it is choosing to expose to user space. It looks like the current property interface is not able to accurately report the full status of the battery: "discharging & nearly empty". Instead, IMHO, the closest it can report is that it is still discharging (== POWER_SUPPLY_STATUS_DISCHARGING), and use some other mechanism to report how much charge is actually left. Re-using "POWER_SUPPLY_STATUS_NOT_CHARGING" to report "discharging & nearly empty" seems arbitrary, wrong and unnecessary. Of course, this bit of code is very old, as it was added in the original TI BQ20Z75 gas gauge patch: commit d3ab61ecbab2b8950108b3541bc9eda515d605e0 Author: Rhyland Klein <rklein@nvidia.com> Date: Tue Sep 21 15:33:55 2010 -0700 bq20z75: Add support for more power supply properties So, it would be nice if an sbs battery expert could chime in here, since I don't really know what I am talking about, I am just interpreting #define names. -Dan
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 <yh.huang@mediatek.com> wrote: >> >> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: >>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: >>>>>> When the battery is dry and BATTERY_FULL_DISCHARGED is set, >>>>>> we should check BATTERY_DISCHARGING to decide the power status. >>>>>> If BATTERY_DISCHARGING is set, the power status is not charging. >>>>>> Or the power status should be charging. >>>>>> >>>>>> Signed-off-by: YH Huang <yh.huang@mediatek.com> >>>>>> --- >>>>>> 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-battery.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(struct i2c_client *client, >>>>>> >>>>>> if (ret & BATTERY_FULL_CHARGED) >>>>>> val->intval = POWER_SUPPLY_STATUS_FULL; >>>>>> - else if (ret & BATTERY_FULL_DISCHARGED) >>>>>> - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >>>>>> - else if (ret & BATTERY_DISCHARGING) >>>>>> - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >>>>>> - else >>>>>> + else if (ret & BATTERY_DISCHARGING) { >>>>>> + if (ret & BATTERY_FULL_DISCHARGED) >>>>>> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >>>>>> + else >>>>>> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >>>>>> + } else >>>>> >>>>> >>>>> I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still >>>>> POWER_SUPPLY_STATUS_DISCHARGING. >>>>> So, let's just report what the battery says and do: >>>>> >>>>> else if (ret & BATTERY_DISCHARGING) >>>>> val->intval = POWER_SUPPLY_STATUS_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 that >>> it is already "discharged" just seems premature. I would expect to >>> only see NOT_CHARGING if completely discharged *and* not discharging. >> >> 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 >> ‘TERMINATE_DISCHARGE_ALARM’ as an early or first level 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 where DISCHARGING isn't set, it won't check FULL_DISCHARGE. Nothing seems to be changed in the case where FULL_DISCHARGE & DISCHARGING are set. I think in the case that both flags are set, it makes sense to prioritize the FULL_DISCHARGE, since as quote above actually starts with "FULLY_DISCHARGED bit is set when the Smart Battery determines that it has supplied all the charge it can." I think at the point where both statuses are reported, the fact that it is still DISCHARGING doesn't matter. The code as is handles this in that manner, so I don't see a need to change it. -rhyland
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 <yh.huang@mediatek.com> wrote: > >> > >> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: > >>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: > >>>>>> When the battery is dry and BATTERY_FULL_DISCHARGED is set, > >>>>>> we should check BATTERY_DISCHARGING to decide the power status. > >>>>>> If BATTERY_DISCHARGING is set, the power status is not charging. > >>>>>> Or the power status should be charging. > >>>>>> > >>>>>> Signed-off-by: YH Huang <yh.huang@mediatek.com> > >>>>>> --- > >>>>>> 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-battery.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(struct i2c_client *client, > >>>>>> > >>>>>> if (ret & BATTERY_FULL_CHARGED) > >>>>>> val->intval = POWER_SUPPLY_STATUS_FULL; > >>>>>> - else if (ret & BATTERY_FULL_DISCHARGED) > >>>>>> - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > >>>>>> - else if (ret & BATTERY_DISCHARGING) > >>>>>> - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > >>>>>> - else > >>>>>> + else if (ret & BATTERY_DISCHARGING) { > >>>>>> + if (ret & BATTERY_FULL_DISCHARGED) > >>>>>> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > >>>>>> + else > >>>>>> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > >>>>>> + } else > >>>>> > >>>>> > >>>>> I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still > >>>>> POWER_SUPPLY_STATUS_DISCHARGING. > >>>>> So, let's just report what the battery says and do: > >>>>> > >>>>> else if (ret & BATTERY_DISCHARGING) > >>>>> val->intval = POWER_SUPPLY_STATUS_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 that > >>> it is already "discharged" just seems premature. I would expect to > >>> only see NOT_CHARGING if completely discharged *and* not discharging. > >> > >> 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 > >> ‘TERMINATE_DISCHARGE_ALARM’ as an early or first level 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 where > DISCHARGING isn't set, it won't check FULL_DISCHARGE. Nothing seems to > be changed in the case where FULL_DISCHARGE & DISCHARGING are set. 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? > > I think in the case that both flags are set, it makes sense to > prioritize the FULL_DISCHARGE, since as quote above actually starts with > "FULLY_DISCHARGED bit is set when the Smart Battery determines that it > has supplied all the charge it can." I think at the point where both > statuses are reported, the fact that it is still DISCHARGING doesn't > matter. The code as is handles this in that manner, so I don't see a > need to change it. > > -rhyland > >
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 <yh.huang@mediatek.com> wrote: >>>> >>>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: >>>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: >>>>>>>> When the battery is dry and BATTERY_FULL_DISCHARGED is set, >>>>>>>> we should check BATTERY_DISCHARGING to decide the power status. >>>>>>>> If BATTERY_DISCHARGING is set, the power status is not charging. >>>>>>>> Or the power status should be charging. >>>>>>>> >>>>>>>> Signed-off-by: YH Huang <yh.huang@mediatek.com> >>>>>>>> --- >>>>>>>> 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-battery.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(struct i2c_client *client, >>>>>>>> >>>>>>>> if (ret & BATTERY_FULL_CHARGED) >>>>>>>> val->intval = POWER_SUPPLY_STATUS_FULL; >>>>>>>> - else if (ret & BATTERY_FULL_DISCHARGED) >>>>>>>> - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >>>>>>>> - else if (ret & BATTERY_DISCHARGING) >>>>>>>> - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >>>>>>>> - else >>>>>>>> + else if (ret & BATTERY_DISCHARGING) { >>>>>>>> + if (ret & BATTERY_FULL_DISCHARGED) >>>>>>>> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >>>>>>>> + else >>>>>>>> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >>>>>>>> + } else >>>>>>> >>>>>>> >>>>>>> I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still >>>>>>> POWER_SUPPLY_STATUS_DISCHARGING. >>>>>>> So, let's just report what the battery says and do: >>>>>>> >>>>>>> else if (ret & BATTERY_DISCHARGING) >>>>>>> val->intval = POWER_SUPPLY_STATUS_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 that >>>>> it is already "discharged" just seems premature. I would expect to >>>>> only see NOT_CHARGING if completely discharged *and* not discharging. >>>> >>>> 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 >>>> ‘TERMINATE_DISCHARGE_ALARM’ as an early or first level 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 where >> DISCHARGING isn't set, it won't check FULL_DISCHARGE. Nothing seems to >> be changed in the case where FULL_DISCHARGE & DISCHARGING are set. > > 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 <unplug> high->low: DISCHARGING ~0%: (DISCHARGING & FULLY_DISCHARGED) <plug in> ->~20%: FULLY_DISCHARGED >~20%: <nothing> = charging From this understanding, it seems like we can't expect FULLY_DISCHARGED 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, though 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 FULLY_DISCHARGED flag comes on early, but rather because it doesn't turn off until ~20%. -rhyland
On Tue, 2016-03-29 at 11:05 -0400, Rhyland Klein wrote: > 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 <yh.huang@mediatek.com> wrote: > >>>> > >>>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: > >>>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: > >>>>>>>> When the battery is dry and BATTERY_FULL_DISCHARGED is set, > >>>>>>>> we should check BATTERY_DISCHARGING to decide the power status. > >>>>>>>> If BATTERY_DISCHARGING is set, the power status is not charging. > >>>>>>>> Or the power status should be charging. > >>>>>>>> > >>>>>>>> Signed-off-by: YH Huang <yh.huang@mediatek.com> > >>>>>>>> --- > >>>>>>>> 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-battery.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(struct i2c_client *client, > >>>>>>>> > >>>>>>>> if (ret & BATTERY_FULL_CHARGED) > >>>>>>>> val->intval = POWER_SUPPLY_STATUS_FULL; > >>>>>>>> - else if (ret & BATTERY_FULL_DISCHARGED) > >>>>>>>> - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > >>>>>>>> - else if (ret & BATTERY_DISCHARGING) > >>>>>>>> - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > >>>>>>>> - else > >>>>>>>> + else if (ret & BATTERY_DISCHARGING) { > >>>>>>>> + if (ret & BATTERY_FULL_DISCHARGED) > >>>>>>>> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > >>>>>>>> + else > >>>>>>>> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > >>>>>>>> + } else > >>>>>>> > >>>>>>> > >>>>>>> I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still > >>>>>>> POWER_SUPPLY_STATUS_DISCHARGING. > >>>>>>> So, let's just report what the battery says and do: > >>>>>>> > >>>>>>> else if (ret & BATTERY_DISCHARGING) > >>>>>>> val->intval = POWER_SUPPLY_STATUS_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 that > >>>>> it is already "discharged" just seems premature. I would expect to > >>>>> only see NOT_CHARGING if completely discharged *and* not discharging. > >>>> > >>>> 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 > >>>> ‘TERMINATE_DISCHARGE_ALARM’ as an early or first level 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 where > >> DISCHARGING isn't set, it won't check FULL_DISCHARGE. Nothing seems to > >> be changed in the case where FULL_DISCHARGE & DISCHARGING are set. > > > > 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 > <unplug> > high->low: DISCHARGING > ~0%: (DISCHARGING & FULLY_DISCHARGED) > <plug in> > ->~20%: FULLY_DISCHARGED > >~20%: <nothing> = charging > > From this understanding, it seems like we can't expect FULLY_DISCHARGED > 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, though > 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 > FULLY_DISCHARGED flag comes on early, but rather because it doesn't turn > off until ~20%. If I revise the description in this way(using your clear explanation): ------------------------------------------------------------------------ The battery capacity changing course is like this: full: BATTERY_FULLY_CHARGED => POWER_SUPPLY_STATUS_FULL <unplug AC> high->low: BATTERY_DISCHARGING => POWER_SUPPLY_STATUS_DISCHARGING ~0%: DISCHARGING & FULLY_DISCHARGED => POWER_SUPPLY_STATUS_NOT_CHARGING <plug in AC> 0%~20%: FULLY_DISCHARGED => POWER_SUPPLY_STATUS_CHARGING 20%~: No flag => POWER_SUPPLY_STATUS_CHARGING For now, it is not exactly right to show the status as POWER_SUPPLY_STATUS_NOT_CHARGING when the battery is dry (FULLY_DISCHARGED) and AC is plugged in. Although the battery is in a low level, system works fine with the AC charging. It is better to say that the battery is charging. ------------------------------------------------------------------------ How about this? By the way, should I also revise the title?
On 3/30/2016 4:58 AM, YH Huang wrote: > On Tue, 2016-03-29 at 11:05 -0400, Rhyland Klein wrote: >> 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 <yh.huang@mediatek.com> wrote: >>>>>> >>>>>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote: >>>>>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <yh.huang@mediatek.com> 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 <yh.huang@mediatek.com> wrote: >>>>>>>>>> When the battery is dry and BATTERY_FULL_DISCHARGED is set, >>>>>>>>>> we should check BATTERY_DISCHARGING to decide the power status. >>>>>>>>>> If BATTERY_DISCHARGING is set, the power status is not charging. >>>>>>>>>> Or the power status should be charging. >>>>>>>>>> >>>>>>>>>> Signed-off-by: YH Huang <yh.huang@mediatek.com> >>>>>>>>>> --- >>>>>>>>>> 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-battery.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(struct i2c_client *client, >>>>>>>>>> >>>>>>>>>> if (ret & BATTERY_FULL_CHARGED) >>>>>>>>>> val->intval = POWER_SUPPLY_STATUS_FULL; >>>>>>>>>> - else if (ret & BATTERY_FULL_DISCHARGED) >>>>>>>>>> - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >>>>>>>>>> - else if (ret & BATTERY_DISCHARGING) >>>>>>>>>> - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >>>>>>>>>> - else >>>>>>>>>> + else if (ret & BATTERY_DISCHARGING) { >>>>>>>>>> + if (ret & BATTERY_FULL_DISCHARGED) >>>>>>>>>> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >>>>>>>>>> + else >>>>>>>>>> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >>>>>>>>>> + } else >>>>>>>>> >>>>>>>>> >>>>>>>>> I think (BATTERY_DISCHARGING && BATTERY_FULL_DISCHARGED) is still >>>>>>>>> POWER_SUPPLY_STATUS_DISCHARGING. >>>>>>>>> So, let's just report what the battery says and do: >>>>>>>>> >>>>>>>>> else if (ret & BATTERY_DISCHARGING) >>>>>>>>> val->intval = POWER_SUPPLY_STATUS_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 that >>>>>>> it is already "discharged" just seems premature. I would expect to >>>>>>> only see NOT_CHARGING if completely discharged *and* not discharging. >>>>>> >>>>>> 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 >>>>>> ‘TERMINATE_DISCHARGE_ALARM’ as an early or first level 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 where >>>> DISCHARGING isn't set, it won't check FULL_DISCHARGE. Nothing seems to >>>> be changed in the case where FULL_DISCHARGE & DISCHARGING are set. >>> >>> 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 >> <unplug> >> high->low: DISCHARGING >> ~0%: (DISCHARGING & FULLY_DISCHARGED) >> <plug in> >> ->~20%: FULLY_DISCHARGED >>> ~20%: <nothing> = charging >> >> From this understanding, it seems like we can't expect FULLY_DISCHARGED >> 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, though >> 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 >> FULLY_DISCHARGED flag comes on early, but rather because it doesn't turn >> off until ~20%. > > If I revise the description in this way(using your clear explanation): > ------------------------------------------------------------------------ > The battery capacity changing course is like this: > > full: BATTERY_FULLY_CHARGED => POWER_SUPPLY_STATUS_FULL > <unplug AC> > high->low: BATTERY_DISCHARGING => POWER_SUPPLY_STATUS_DISCHARGING > ~0%: DISCHARGING & FULLY_DISCHARGED => POWER_SUPPLY_STATUS_NOT_CHARGING > <plug in AC> > 0%~20%: FULLY_DISCHARGED => POWER_SUPPLY_STATUS_CHARGING > 20%~: No flag => POWER_SUPPLY_STATUS_CHARGING > > For now, it is not exactly right to show the status as > POWER_SUPPLY_STATUS_NOT_CHARGING when the battery is dry > (FULLY_DISCHARGED) and AC is plugged in. > Although the battery is in a low level, system works fine with the AC > charging. > It is better to say that the battery is charging. > ------------------------------------------------------------------------ Sounds good. > > How about this? > By the way, should I also revise the title? > well, since the case this is specifically addressing is more to do with charging when very low than being dry, it would probably make sense to change it to "sbs-battery: fix power status when battery charging near dry" or something like that. -rhyland
Hi, On Wed, Mar 30, 2016 at 04:58:30PM +0800, YH Huang wrote: > If I revise the description in this way(using your clear explanation): > ------------------------------------------------------------------------ > The battery capacity changing course is like this: > > full: BATTERY_FULLY_CHARGED => POWER_SUPPLY_STATUS_FULL > <unplug AC> > high->low: BATTERY_DISCHARGING => POWER_SUPPLY_STATUS_DISCHARGING > ~0%: DISCHARGING & FULLY_DISCHARGED => POWER_SUPPLY_STATUS_NOT_CHARGING > <plug in AC> > 0%~20%: FULLY_DISCHARGED => POWER_SUPPLY_STATUS_CHARGING > 20%~: No flag => POWER_SUPPLY_STATUS_CHARGING > > For now, it is not exactly right to show the status as > POWER_SUPPLY_STATUS_NOT_CHARGING when the battery is dry > (FULLY_DISCHARGED) and AC is plugged in. > Although the battery is in a low level, system works fine with the AC > charging. > It is better to say that the battery is charging. > ------------------------------------------------------------------------ > > How about this? > By the way, should I also revise the title? POWER_SUPPLY_STATUS_NOT_CHARGING is used for AC connected, but battery not charging (e.g. because battery temperature is out of acceptable range). If you are discharging use POWER_SUPPLY_STATUS_DISCHARGING. You should just ignore the FULLY_DISCHARGED bit in the status property. If you don't want to loose the information about fully discharged battery add POWER_SUPPLY_PROP_CAPACITY_LEVEL, which maps: BATTERY_FULLY_CHARGED => POWER_SUPPLY_CAPACITY_LEVEL_FULL FULLY_DISCHARGED => POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL otherwise => POWER_SUPPLY_CAPACITY_LEVEL_NORMAL -- Sebastian
On Wed, 2016-03-30 at 17:09 +0200, Sebastian Reichel wrote: > Hi, > > On Wed, Mar 30, 2016 at 04:58:30PM +0800, YH Huang wrote: > > If I revise the description in this way(using your clear explanation): > > ------------------------------------------------------------------------ > > The battery capacity changing course is like this: > > > > full: BATTERY_FULLY_CHARGED => POWER_SUPPLY_STATUS_FULL > > <unplug AC> > > high->low: BATTERY_DISCHARGING => POWER_SUPPLY_STATUS_DISCHARGING > > ~0%: DISCHARGING & FULLY_DISCHARGED => POWER_SUPPLY_STATUS_NOT_CHARGING > > <plug in AC> > > 0%~20%: FULLY_DISCHARGED => POWER_SUPPLY_STATUS_CHARGING > > 20%~: No flag => POWER_SUPPLY_STATUS_CHARGING > > > > For now, it is not exactly right to show the status as > > POWER_SUPPLY_STATUS_NOT_CHARGING when the battery is dry > > (FULLY_DISCHARGED) and AC is plugged in. > > Although the battery is in a low level, system works fine with the AC > > charging. > > It is better to say that the battery is charging. > > ------------------------------------------------------------------------ > > > > How about this? > > By the way, should I also revise the title? > > POWER_SUPPLY_STATUS_NOT_CHARGING is used for AC connected, but > battery not charging (e.g. because battery temperature is out > of acceptable range). If you are discharging use > POWER_SUPPLY_STATUS_DISCHARGING. > > You should just ignore the FULLY_DISCHARGED bit in the status > property. If you don't want to loose the information about fully > discharged battery add POWER_SUPPLY_PROP_CAPACITY_LEVEL, which maps: > > BATTERY_FULLY_CHARGED => POWER_SUPPLY_CAPACITY_LEVEL_FULL > FULLY_DISCHARGED => POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL > otherwise => POWER_SUPPLY_CAPACITY_LEVEL_NORMAL Oops. It looks like I misunderstand POWER_SUPPLY_STATUS_NOT_CHARGING. As Daniel said before, we could just ignore FULLY_DISCHARGED. So change like the sbs-battery: fix power status when battery charging near dry POWER_SUPPLY_STATUS_NOT_CHARGING is used for AC connected, but battery not charging (e.g. because battery temperature is out of acceptable range). When battery is charging near dry and BATTERY_FULL_DISCHARGED is set, it is wrong to set as POWER_SUPPLY_STATUS_NOT_CHARGING. Just use BATTERY_DISCHARGING to decide the power supply status is discharging or charging. if (ret & BATTERY_FULL_CHARGED) val->intval = POWER_SUPPLY_STATUS_FULL; - else if (ret & BATTERY_FULL_DISCHARGED) - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; else if (ret & BATTERY_DISCHARGING) val->intval = POWER_SUPPLY_STATUS_DISCHARGING; YH Huang
Hi, On Thu, Mar 31, 2016 at 10:42:31AM +0800, YH Huang wrote: > On Wed, 2016-03-30 at 17:09 +0200, Sebastian Reichel wrote: > > Hi, > > > > On Wed, Mar 30, 2016 at 04:58:30PM +0800, YH Huang wrote: > > > If I revise the description in this way(using your clear explanation): > > > ------------------------------------------------------------------------ > > > The battery capacity changing course is like this: > > > > > > full: BATTERY_FULLY_CHARGED => POWER_SUPPLY_STATUS_FULL > > > <unplug AC> > > > high->low: BATTERY_DISCHARGING => POWER_SUPPLY_STATUS_DISCHARGING > > > ~0%: DISCHARGING & FULLY_DISCHARGED => POWER_SUPPLY_STATUS_NOT_CHARGING > > > <plug in AC> > > > 0%~20%: FULLY_DISCHARGED => POWER_SUPPLY_STATUS_CHARGING > > > 20%~: No flag => POWER_SUPPLY_STATUS_CHARGING > > > > > > For now, it is not exactly right to show the status as > > > POWER_SUPPLY_STATUS_NOT_CHARGING when the battery is dry > > > (FULLY_DISCHARGED) and AC is plugged in. > > > Although the battery is in a low level, system works fine with the AC > > > charging. > > > It is better to say that the battery is charging. > > > ------------------------------------------------------------------------ > > > > > > How about this? > > > By the way, should I also revise the title? > > > > POWER_SUPPLY_STATUS_NOT_CHARGING is used for AC connected, but > > battery not charging (e.g. because battery temperature is out > > of acceptable range). If you are discharging use > > POWER_SUPPLY_STATUS_DISCHARGING. > > > > You should just ignore the FULLY_DISCHARGED bit in the status > > property. If you don't want to loose the information about fully > > discharged battery add POWER_SUPPLY_PROP_CAPACITY_LEVEL, which maps: > > > > BATTERY_FULLY_CHARGED => POWER_SUPPLY_CAPACITY_LEVEL_FULL > > FULLY_DISCHARGED => POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL > > otherwise => POWER_SUPPLY_CAPACITY_LEVEL_NORMAL > > Oops. > It looks like I misunderstand POWER_SUPPLY_STATUS_NOT_CHARGING. > As Daniel said before, we could just ignore FULLY_DISCHARGED. > > So change like the > > sbs-battery: fix power status when battery charging near > dry > > POWER_SUPPLY_STATUS_NOT_CHARGING is used for AC connected, but > battery not charging (e.g. because battery temperature is out > of acceptable range). > > When battery is charging near dry and BATTERY_FULL_DISCHARGED is set, > it is wrong to set as POWER_SUPPLY_STATUS_NOT_CHARGING. > Just use BATTERY_DISCHARGING to decide the power supply status is > discharging or charging. > > if (ret & BATTERY_FULL_CHARGED) > val->intval = POWER_SUPPLY_STATUS_FULL; > - else if (ret & BATTERY_FULL_DISCHARGED) > - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > else if (ret & BATTERY_DISCHARGING) > val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > Yes, that looks fine. Can you send this correctly formated for git? -- Sebastian
On Sat, 2016-04-02 at 05:46 +0200, Sebastian Reichel wrote: > Hi, > > On Thu, Mar 31, 2016 at 10:42:31AM +0800, YH Huang wrote: > > On Wed, 2016-03-30 at 17:09 +0200, Sebastian Reichel wrote: > > > Hi, > > > > > > On Wed, Mar 30, 2016 at 04:58:30PM +0800, YH Huang wrote: > > > > If I revise the description in this way(using your clear explanation): > > > > ------------------------------------------------------------------------ > > > > The battery capacity changing course is like this: > > > > > > > > full: BATTERY_FULLY_CHARGED => POWER_SUPPLY_STATUS_FULL > > > > <unplug AC> > > > > high->low: BATTERY_DISCHARGING => POWER_SUPPLY_STATUS_DISCHARGING > > > > ~0%: DISCHARGING & FULLY_DISCHARGED => POWER_SUPPLY_STATUS_NOT_CHARGING > > > > <plug in AC> > > > > 0%~20%: FULLY_DISCHARGED => POWER_SUPPLY_STATUS_CHARGING > > > > 20%~: No flag => POWER_SUPPLY_STATUS_CHARGING > > > > > > > > For now, it is not exactly right to show the status as > > > > POWER_SUPPLY_STATUS_NOT_CHARGING when the battery is dry > > > > (FULLY_DISCHARGED) and AC is plugged in. > > > > Although the battery is in a low level, system works fine with the AC > > > > charging. > > > > It is better to say that the battery is charging. > > > > ------------------------------------------------------------------------ > > > > > > > > How about this? > > > > By the way, should I also revise the title? > > > > > > POWER_SUPPLY_STATUS_NOT_CHARGING is used for AC connected, but > > > battery not charging (e.g. because battery temperature is out > > > of acceptable range). If you are discharging use > > > POWER_SUPPLY_STATUS_DISCHARGING. > > > > > > You should just ignore the FULLY_DISCHARGED bit in the status > > > property. If you don't want to loose the information about fully > > > discharged battery add POWER_SUPPLY_PROP_CAPACITY_LEVEL, which maps: > > > > > > BATTERY_FULLY_CHARGED => POWER_SUPPLY_CAPACITY_LEVEL_FULL > > > FULLY_DISCHARGED => POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL > > > otherwise => POWER_SUPPLY_CAPACITY_LEVEL_NORMAL > > > > Oops. > > It looks like I misunderstand POWER_SUPPLY_STATUS_NOT_CHARGING. > > As Daniel said before, we could just ignore FULLY_DISCHARGED. > > > > So change like the > > > > sbs-battery: fix power status when battery charging near > > dry > > > > POWER_SUPPLY_STATUS_NOT_CHARGING is used for AC connected, but > > battery not charging (e.g. because battery temperature is out > > of acceptable range). > > > > When battery is charging near dry and BATTERY_FULL_DISCHARGED is set, > > it is wrong to set as POWER_SUPPLY_STATUS_NOT_CHARGING. > > Just use BATTERY_DISCHARGING to decide the power supply status is > > discharging or charging. > > > > if (ret & BATTERY_FULL_CHARGED) > > val->intval = POWER_SUPPLY_STATUS_FULL; > > - else if (ret & BATTERY_FULL_DISCHARGED) > > - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > else if (ret & BATTERY_DISCHARGING) > > val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > > > Yes, that looks fine. Can you send this correctly formated for git? Sure, I will send a new patch for it. YH Huang
diff --git a/drivers/power/sbs-battery.c b/drivers/power/sbs-battery.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(struct i2c_client *client, if (ret & BATTERY_FULL_CHARGED) val->intval = POWER_SUPPLY_STATUS_FULL; - else if (ret & BATTERY_FULL_DISCHARGED) - val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; - else if (ret & BATTERY_DISCHARGING) - val->intval = POWER_SUPPLY_STATUS_DISCHARGING; - else + else if (ret & BATTERY_DISCHARGING) { + if (ret & BATTERY_FULL_DISCHARGED) + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; + else + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; + } else val->intval = POWER_SUPPLY_STATUS_CHARGING; if (chip->poll_time == 0) @@ -702,11 +703,12 @@ static void sbs_delayed_work(struct work_struct *work) if (ret & BATTERY_FULL_CHARGED) ret = POWER_SUPPLY_STATUS_FULL; - else if (ret & BATTERY_FULL_DISCHARGED) - ret = POWER_SUPPLY_STATUS_NOT_CHARGING; - else if (ret & BATTERY_DISCHARGING) - ret = POWER_SUPPLY_STATUS_DISCHARGING; - else + else if (ret & BATTERY_DISCHARGING) { + if (ret & BATTERY_FULL_DISCHARGED) + ret = POWER_SUPPLY_STATUS_NOT_CHARGING; + else + ret = POWER_SUPPLY_STATUS_DISCHARGING; + } else ret = POWER_SUPPLY_STATUS_CHARGING; if (chip->last_state != ret) {
When the battery is dry and BATTERY_FULL_DISCHARGED is set, we should check BATTERY_DISCHARGING to decide the power status. If BATTERY_DISCHARGING is set, the power status is not charging. Or the power status should be charging. Signed-off-by: YH Huang <yh.huang@mediatek.com> --- drivers/power/sbs-battery.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)