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 <[email protected]>
---
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
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 YH,
On Wed, Mar 23, 2016 at 5:53 PM, YH Huang <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
>
> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote:
> > On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
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 <[email protected]> wrote:
>>
>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote:
>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
--
nvpublic
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 <[email protected]> wrote:
> >>
> >> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote:
> >>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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 <[email protected]> wrote:
>>>>
>>>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote:
>>>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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
--
nvpublic
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 <[email protected]> wrote:
> >>>>
> >>>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote:
> >>>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>>>> ---
> >>>>>>>> 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 <[email protected]> wrote:
>>>>>>
>>>>>> On Fri, 2016-03-25 at 11:06 +0800, Daniel Kurtz wrote:
>>>>>>> On Thu, Mar 24, 2016 at 2:43 PM, YH Huang <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> 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
--
nvpublic
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