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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
