| Message ID | 1441073435-12349-3-git-send-email-dannenberg@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 08/31/2015 09:10 PM, Andreas Dannenberg wrote: > A missing/disconnected battery is now reported as dead rather than an > unspecified failure via the charger's sysfs health property. > > $ cat health > Dead Poor cat :( > > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> > --- > drivers/power/bq24257_charger.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c > index db81356..0b34528 100644 > --- a/drivers/power/bq24257_charger.c > +++ b/drivers/power/bq24257_charger.c > @@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, > val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE; > break; > > + case FAULT_NO_BAT: > + val->intval = POWER_SUPPLY_HEALTH_DEAD; > + break; > + I think the best thing to do would be to return -ENODEV as suggested by power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT property check and set intval to 0 when there is no battery. Regards, Andrew > default: > val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE; > break; > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrew- thanks for your feeback. Answers below... On Tue, Sep 01, 2015 at 02:33:11PM -0500, Andrew F. Davis wrote: > On 08/31/2015 09:10 PM, Andreas Dannenberg wrote: > >A missing/disconnected battery is now reported as dead rather than an > >unspecified failure via the charger's sysfs health property. > > > >$ cat health > >Dead > > Poor cat :( > I had to laugh pretty hard when I saw that getting printed onto the console for the first time so I had to include it here. > > > >Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> > >--- > > drivers/power/bq24257_charger.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > >diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c > >index db81356..0b34528 100644 > >--- a/drivers/power/bq24257_charger.c > >+++ b/drivers/power/bq24257_charger.c > >@@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, > > val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE; > > break; > > > >+ case FAULT_NO_BAT: > >+ val->intval = POWER_SUPPLY_HEALTH_DEAD; > >+ break; > >+ > > I think the best thing to do would be to return -ENODEV as suggested by > power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT > property check and set intval to 0 when there is no battery. Good find with -ENODEV. However in this case here the power supply is really a combination of a charger and a battery (which could be split out accordingly but that's a different story). If I were to report -ENODEV I would basically say the entire power supply is gone which is not correct IMHO. Even with a missing battery a system is typically functional as it gets powered through USB and the charger IC is still there and functioning. So I think I would either leave the reporting as *_DEAD or skip the patch. I'm curious if there are additional opinions. -- Andreas Dannenberg Texas Instruments Inc > > Regards, > Andrew > > > default: > > val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE; > > break; > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/01/2015 04:04 PM, Andreas Dannenberg wrote: > Andrew- thanks for your feeback. Answers below... > > On Tue, Sep 01, 2015 at 02:33:11PM -0500, Andrew F. Davis wrote: >> On 08/31/2015 09:10 PM, Andreas Dannenberg wrote: >>> A missing/disconnected battery is now reported as dead rather than an >>> unspecified failure via the charger's sysfs health property. >>> >>> $ cat health >>> Dead >> >> Poor cat :( >> > > I had to laugh pretty hard when I saw that getting printed onto the > console for the first time so I had to include it here. > >>> >>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> >>> --- >>> drivers/power/bq24257_charger.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c >>> index db81356..0b34528 100644 >>> --- a/drivers/power/bq24257_charger.c >>> +++ b/drivers/power/bq24257_charger.c >>> @@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, >>> val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE; >>> break; >>> >>> + case FAULT_NO_BAT: >>> + val->intval = POWER_SUPPLY_HEALTH_DEAD; >>> + break; >>> + >> >> I think the best thing to do would be to return -ENODEV as suggested by >> power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT >> property check and set intval to 0 when there is no battery. > > Good find with -ENODEV. However in this case here the power supply is > really a combination of a charger and a battery (which could be split > out accordingly but that's a different story). If I were to report > -ENODEV I would basically say the entire power supply is gone which is > not correct IMHO. Even with a missing battery a system is typically > functional as it gets powered through USB and the charger IC is still > there and functioning. So I think I would either leave the reporting > as *_DEAD or skip the patch. I'm curious if there are additional > opinions. > It looks like returning -ENODEV for one property would not mark the whole device gone, just POWER_SUPPLY_PROP_HEALTH, but I guess that depends on what user-space does with the info. I would think that this is what POWER_SUPPLY_PROP_PRESENT is for but different drivers seem to use it for different things. Anyway, reporting DEAD for a missing battery is probably not the most correct option, maybe drop the patch ( for the cat's sake :) ).
On Tue, Sep 01, 2015 at 04:16:26PM -0500, Andrew F. Davis wrote: > > > On 09/01/2015 04:04 PM, Andreas Dannenberg wrote: > >Andrew- thanks for your feeback. Answers below... > > > >On Tue, Sep 01, 2015 at 02:33:11PM -0500, Andrew F. Davis wrote: > >>On 08/31/2015 09:10 PM, Andreas Dannenberg wrote: > >>>A missing/disconnected battery is now reported as dead rather than an > >>>unspecified failure via the charger's sysfs health property. > >>> > >>>$ cat health > >>>Dead > >> > >>Poor cat :( > >> > > > >I had to laugh pretty hard when I saw that getting printed onto the > >console for the first time so I had to include it here. > > > >>> > >>>Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> > >>>--- > >>> drivers/power/bq24257_charger.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>>diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c > >>>index db81356..0b34528 100644 > >>>--- a/drivers/power/bq24257_charger.c > >>>+++ b/drivers/power/bq24257_charger.c > >>>@@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, > >>> val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE; > >>> break; > >>> > >>>+ case FAULT_NO_BAT: > >>>+ val->intval = POWER_SUPPLY_HEALTH_DEAD; > >>>+ break; > >>>+ > >> > >>I think the best thing to do would be to return -ENODEV as suggested by > >>power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT > >>property check and set intval to 0 when there is no battery. > > > >Good find with -ENODEV. However in this case here the power supply is > >really a combination of a charger and a battery (which could be split > >out accordingly but that's a different story). If I were to report > >-ENODEV I would basically say the entire power supply is gone which is > >not correct IMHO. Even with a missing battery a system is typically > >functional as it gets powered through USB and the charger IC is still > >there and functioning. So I think I would either leave the reporting > >as *_DEAD or skip the patch. I'm curious if there are additional > >opinions. > > > > It looks like returning -ENODEV for one property would not mark the whole > device gone, just POWER_SUPPLY_PROP_HEALTH, but I guess that depends on > what user-space does with the info. I would think that this is what > POWER_SUPPLY_PROP_PRESENT is for but different drivers seem to use it for > different things. Anyway, reporting DEAD for a missing battery is probably > not the most correct option, maybe drop the patch ( for the cat's sake :) ). I'm also in favor of dropping this patch for the same reasons. Also, since the latest phones/tablets do not allow battery removal, it's highly unlikely we'll even hit this case nowadays. Perhaps, we could use DEAD if the battery did not charge and the safety timer expired? But, if one alters iilimit and sets a limit that's too low for the battery to charge in time, that doesn't mean the battery is dead... :/ That's the main reason I'm so reluctant on having properties like charge current, charge voltage, or even input current limit, writable in sysfs. laurentiu -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 04, 2015 at 04:28:05PM +0300, Laurentiu Palcu wrote: > On Tue, Sep 01, 2015 at 04:16:26PM -0500, Andrew F. Davis wrote: > > On 09/01/2015 04:04 PM, Andreas Dannenberg wrote: > > >>> drivers/power/bq24257_charger.c | 4 ++++ > > >>> 1 file changed, 4 insertions(+) > > >>> > > >>>diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c > > >>>index db81356..0b34528 100644 > > >>>--- a/drivers/power/bq24257_charger.c > > >>>+++ b/drivers/power/bq24257_charger.c > > >>>@@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, > > >>> val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE; > > >>> break; > > >>> > > >>>+ case FAULT_NO_BAT: > > >>>+ val->intval = POWER_SUPPLY_HEALTH_DEAD; > > >>>+ break; > > >>>+ > > >> > > >>I think the best thing to do would be to return -ENODEV as suggested by > > >>power_supply_sysfs.c:305. Also you should probably add the POWER_SUPPLY_PROP_PRESENT > > >>property check and set intval to 0 when there is no battery. > > > > > >Good find with -ENODEV. However in this case here the power supply is > > >really a combination of a charger and a battery (which could be split > > >out accordingly but that's a different story). If I were to report > > >-ENODEV I would basically say the entire power supply is gone which is > > >not correct IMHO. Even with a missing battery a system is typically > > >functional as it gets powered through USB and the charger IC is still > > >there and functioning. So I think I would either leave the reporting > > >as *_DEAD or skip the patch. I'm curious if there are additional > > >opinions. > > > > > > > It looks like returning -ENODEV for one property would not mark the whole > > device gone, just POWER_SUPPLY_PROP_HEALTH, but I guess that depends on > > what user-space does with the info. I would think that this is what > > POWER_SUPPLY_PROP_PRESENT is for but different drivers seem to use it for > > different things. Anyway, reporting DEAD for a missing battery is probably > > not the most correct option, maybe drop the patch ( for the cat's sake :) ). > I'm also in favor of dropping this patch for the same reasons. Also, > since the latest phones/tablets do not allow battery removal, it's > highly unlikely we'll even hit this case nowadays. Perhaps, we could use Hi Laurentiu, ok I'll drop it. Understood about the unlikeliness of a battery removal for most devices using LiIon during runtime but a more granular reporting would help in case there is an issue with the HW itself (loose contact, manufacturing defect in the battery, etc.) > DEAD if the battery did not charge and the safety timer expired? > But, if one alters iilimit and sets a limit that's too low for the > battery to charge in time, that doesn't mean the battery is dead... :/ Yeah unless there is a specific register bit or some very specific guidance in a device datasheet as how to detect a truly dead battery I would be a bit hesitant trying to interpret device register values to derive to such a conclusion. Sometimes algorithms which seem simple are only simple because one doesn't consider all the corner cases initially :) > That's the main reason I'm so reluctant on having properties like charge > current, charge voltage, or even input current limit, writable in > sysfs. Agreed from a safety perspective. And if someone really wants to play with fire (pun intended) they can always change the driver in their own copy of the Kernel. -- Andreas Dannenberg Texas Instruments Inc -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c index db81356..0b34528 100644 --- a/drivers/power/bq24257_charger.c +++ b/drivers/power/bq24257_charger.c @@ -274,6 +274,10 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, val->intval = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE; break; + case FAULT_NO_BAT: + val->intval = POWER_SUPPLY_HEALTH_DEAD; + break; + default: val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE; break;
A missing/disconnected battery is now reported as dead rather than an unspecified failure via the charger's sysfs health property. $ cat health Dead Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> --- drivers/power/bq24257_charger.c | 4 ++++ 1 file changed, 4 insertions(+)