diff mbox series

[v2,3/3] power: supply: bq27xxx: make status more robust

Message ID 20210303095420.29054-3-matthias.schiffer@ew.tq-group.com
State Accepted
Commit c3a6d6a1dfc8a9bf12d79a0b1a30cb24c92a2ddf
Headers show
Series None | expand

Commit Message

Matthias Schiffer March 3, 2021, 9:54 a.m. UTC 
There are multiple issues in bq27xxx_battery_status():

- On BQ28Q610 is was observed that the "full" flag may be set even while
  the battery is charging or discharging. With the current logic to make
  "full" override everything else, it look a very long time (>20min) for
  the status to change from "full" to "discharging" after unplugging the
  supply on a device with low power consumption
- The POWER_SUPPLY_STATUS_NOT_CHARGING check depends on
  power_supply_am_i_supplied(), which will not work when the supply
  doesn't exist as a separate device known to Linux

We can solve both issues by deriving the status from the current instead
of the flags field. The flags are now only used to distinguish "full"
from "not charging", and to determine the sign of the current on
BQ27XXX_O_ZERO devices.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

v2: no changes

 drivers/power/supply/bq27xxx_battery.c | 88 +++++++++++++-------------
 1 file changed, 43 insertions(+), 45 deletions(-)

Comments

Sebastian Reichel March 15, 2021, 1:55 a.m. UTC | #1 
Hi,

On Wed, Mar 03, 2021 at 10:54:20AM +0100, Matthias Schiffer wrote:
> There are multiple issues in bq27xxx_battery_status():

> 

> - On BQ28Q610 is was observed that the "full" flag may be set even while

>   the battery is charging or discharging. With the current logic to make

>   "full" override everything else, it look a very long time (>20min) for

>   the status to change from "full" to "discharging" after unplugging the

>   supply on a device with low power consumption

> - The POWER_SUPPLY_STATUS_NOT_CHARGING check depends on

>   power_supply_am_i_supplied(), which will not work when the supply

>   doesn't exist as a separate device known to Linux

> 

> We can solve both issues by deriving the status from the current instead

> of the flags field. The flags are now only used to distinguish "full"

> from "not charging", and to determine the sign of the current on

> BQ27XXX_O_ZERO devices.

> 

> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

> ---


Thanks, queued.

-- Sebastian

> 

> v2: no changes

> 

>  drivers/power/supply/bq27xxx_battery.c | 88 +++++++++++++-------------

>  1 file changed, 43 insertions(+), 45 deletions(-)

> 

> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c

> index 20e1dc8a87cf..b62a8cfd9d09 100644

> --- a/drivers/power/supply/bq27xxx_battery.c

> +++ b/drivers/power/supply/bq27xxx_battery.c

> @@ -1777,14 +1777,27 @@ static void bq27xxx_battery_poll(struct work_struct *work)

>  		schedule_delayed_work(&di->work, poll_interval * HZ);

>  }

>  

> +static bool bq27xxx_battery_is_full(struct bq27xxx_device_info *di, int flags)

> +{

> +	if (di->opts & BQ27XXX_O_ZERO)

> +		return (flags & BQ27000_FLAG_FC);

> +	else if (di->opts & BQ27Z561_O_BITS)

> +		return (flags & BQ27Z561_FLAG_FC);

> +	else

> +		return (flags & BQ27XXX_FLAG_FC);

> +}

> +

>  /*

> - * Return the battery average current in µA

> + * Return the battery average current in µA and the status

>   * Note that current can be negative signed as well

>   * Or 0 if something fails.

>   */

> -static int bq27xxx_battery_current(struct bq27xxx_device_info *di,

> -				   union power_supply_propval *val)

> +static int bq27xxx_battery_current_and_status(

> +	struct bq27xxx_device_info *di,

> +	union power_supply_propval *val_curr,

> +	union power_supply_propval *val_status)

>  {

> +	bool single_flags = (di->opts & BQ27XXX_O_ZERO);

>  	int curr;

>  	int flags;

>  

> @@ -1794,17 +1807,39 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,

>  		return curr;

>  	}

>  

> +	flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, single_flags);

> +	if (flags < 0) {

> +		dev_err(di->dev, "error reading flags\n");

> +		return flags;

> +	}

> +

>  	if (di->opts & BQ27XXX_O_ZERO) {

> -		flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);

>  		if (!(flags & BQ27000_FLAG_CHGS)) {

>  			dev_dbg(di->dev, "negative current!\n");

>  			curr = -curr;

>  		}

>  

> -		val->intval = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;

> +		curr = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;

>  	} else {

>  		/* Other gauges return signed value */

> -		val->intval = (int)((s16)curr) * 1000;

> +		curr = (int)((s16)curr) * 1000;

> +	}

> +

> +	if (val_curr)

> +		val_curr->intval = curr;

> +

> +	if (val_status) {

> +		if (curr > 0) {

> +			val_status->intval = POWER_SUPPLY_STATUS_CHARGING;

> +		} else if (curr < 0) {

> +			val_status->intval = POWER_SUPPLY_STATUS_DISCHARGING;

> +		} else {

> +			if (bq27xxx_battery_is_full(di, flags))

> +				val_status->intval = POWER_SUPPLY_STATUS_FULL;

> +			else

> +				val_status->intval =

> +					POWER_SUPPLY_STATUS_NOT_CHARGING;

> +		}

>  	}

>  

>  	return 0;

> @@ -1836,43 +1871,6 @@ static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,

>  	return 0;

>  }

>  

> -static int bq27xxx_battery_status(struct bq27xxx_device_info *di,

> -				  union power_supply_propval *val)

> -{

> -	int status;

> -

> -	if (di->opts & BQ27XXX_O_ZERO) {

> -		if (di->cache.flags & BQ27000_FLAG_FC)

> -			status = POWER_SUPPLY_STATUS_FULL;

> -		else if (di->cache.flags & BQ27000_FLAG_CHGS)

> -			status = POWER_SUPPLY_STATUS_CHARGING;

> -		else

> -			status = POWER_SUPPLY_STATUS_DISCHARGING;

> -	} else if (di->opts & BQ27Z561_O_BITS) {

> -		if (di->cache.flags & BQ27Z561_FLAG_FC)

> -			status = POWER_SUPPLY_STATUS_FULL;

> -		else if (di->cache.flags & BQ27Z561_FLAG_DIS_CH)

> -			status = POWER_SUPPLY_STATUS_DISCHARGING;

> -		else

> -			status = POWER_SUPPLY_STATUS_CHARGING;

> -	} else {

> -		if (di->cache.flags & BQ27XXX_FLAG_FC)

> -			status = POWER_SUPPLY_STATUS_FULL;

> -		else if (di->cache.flags & BQ27XXX_FLAG_DSC)

> -			status = POWER_SUPPLY_STATUS_DISCHARGING;

> -		else

> -			status = POWER_SUPPLY_STATUS_CHARGING;

> -	}

> -

> -	if ((status == POWER_SUPPLY_STATUS_DISCHARGING) &&

> -	    (power_supply_am_i_supplied(di->bat) > 0))

> -		status = POWER_SUPPLY_STATUS_NOT_CHARGING;

> -

> -	val->intval = status;

> -

> -	return 0;

> -}

> -

>  static int bq27xxx_battery_capacity_level(struct bq27xxx_device_info *di,

>  					  union power_supply_propval *val)

>  {

> @@ -1960,7 +1958,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,

>  

>  	switch (psp) {

>  	case POWER_SUPPLY_PROP_STATUS:

> -		ret = bq27xxx_battery_status(di, val);

> +		ret = bq27xxx_battery_current_and_status(di, NULL, val);

>  		break;

>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:

>  		ret = bq27xxx_battery_voltage(di, val);

> @@ -1969,7 +1967,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,

>  		val->intval = di->cache.flags < 0 ? 0 : 1;

>  		break;

>  	case POWER_SUPPLY_PROP_CURRENT_NOW:

> -		ret = bq27xxx_battery_current(di, val);

> +		ret = bq27xxx_battery_current_and_status(di, val, NULL);

>  		break;

>  	case POWER_SUPPLY_PROP_CAPACITY:

>  		ret = bq27xxx_simple_value(di->cache.capacity, val);

> -- 

> 2.17.1

>
diff mbox series

Patch

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 20e1dc8a87cf..b62a8cfd9d09 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1777,14 +1777,27 @@  static void bq27xxx_battery_poll(struct work_struct *work)
 		schedule_delayed_work(&di->work, poll_interval * HZ);
 }
 
+static bool bq27xxx_battery_is_full(struct bq27xxx_device_info *di, int flags)
+{
+	if (di->opts & BQ27XXX_O_ZERO)
+		return (flags & BQ27000_FLAG_FC);
+	else if (di->opts & BQ27Z561_O_BITS)
+		return (flags & BQ27Z561_FLAG_FC);
+	else
+		return (flags & BQ27XXX_FLAG_FC);
+}
+
 /*
- * Return the battery average current in µA
+ * Return the battery average current in µA and the status
  * Note that current can be negative signed as well
  * Or 0 if something fails.
  */
-static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
-				   union power_supply_propval *val)
+static int bq27xxx_battery_current_and_status(
+	struct bq27xxx_device_info *di,
+	union power_supply_propval *val_curr,
+	union power_supply_propval *val_status)
 {
+	bool single_flags = (di->opts & BQ27XXX_O_ZERO);
 	int curr;
 	int flags;
 
@@ -1794,17 +1807,39 @@  static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
 		return curr;
 	}
 
+	flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, single_flags);
+	if (flags < 0) {
+		dev_err(di->dev, "error reading flags\n");
+		return flags;
+	}
+
 	if (di->opts & BQ27XXX_O_ZERO) {
-		flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);
 		if (!(flags & BQ27000_FLAG_CHGS)) {
 			dev_dbg(di->dev, "negative current!\n");
 			curr = -curr;
 		}
 
-		val->intval = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
+		curr = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
 	} else {
 		/* Other gauges return signed value */
-		val->intval = (int)((s16)curr) * 1000;
+		curr = (int)((s16)curr) * 1000;
+	}
+
+	if (val_curr)
+		val_curr->intval = curr;
+
+	if (val_status) {
+		if (curr > 0) {
+			val_status->intval = POWER_SUPPLY_STATUS_CHARGING;
+		} else if (curr < 0) {
+			val_status->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		} else {
+			if (bq27xxx_battery_is_full(di, flags))
+				val_status->intval = POWER_SUPPLY_STATUS_FULL;
+			else
+				val_status->intval =
+					POWER_SUPPLY_STATUS_NOT_CHARGING;
+		}
 	}
 
 	return 0;
@@ -1836,43 +1871,6 @@  static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,
 	return 0;
 }
 
-static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
-				  union power_supply_propval *val)
-{
-	int status;
-
-	if (di->opts & BQ27XXX_O_ZERO) {
-		if (di->cache.flags & BQ27000_FLAG_FC)
-			status = POWER_SUPPLY_STATUS_FULL;
-		else if (di->cache.flags & BQ27000_FLAG_CHGS)
-			status = POWER_SUPPLY_STATUS_CHARGING;
-		else
-			status = POWER_SUPPLY_STATUS_DISCHARGING;
-	} else if (di->opts & BQ27Z561_O_BITS) {
-		if (di->cache.flags & BQ27Z561_FLAG_FC)
-			status = POWER_SUPPLY_STATUS_FULL;
-		else if (di->cache.flags & BQ27Z561_FLAG_DIS_CH)
-			status = POWER_SUPPLY_STATUS_DISCHARGING;
-		else
-			status = POWER_SUPPLY_STATUS_CHARGING;
-	} else {
-		if (di->cache.flags & BQ27XXX_FLAG_FC)
-			status = POWER_SUPPLY_STATUS_FULL;
-		else if (di->cache.flags & BQ27XXX_FLAG_DSC)
-			status = POWER_SUPPLY_STATUS_DISCHARGING;
-		else
-			status = POWER_SUPPLY_STATUS_CHARGING;
-	}
-
-	if ((status == POWER_SUPPLY_STATUS_DISCHARGING) &&
-	    (power_supply_am_i_supplied(di->bat) > 0))
-		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
-
-	val->intval = status;
-
-	return 0;
-}
-
 static int bq27xxx_battery_capacity_level(struct bq27xxx_device_info *di,
 					  union power_supply_propval *val)
 {
@@ -1960,7 +1958,7 @@  static int bq27xxx_battery_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
-		ret = bq27xxx_battery_status(di, val);
+		ret = bq27xxx_battery_current_and_status(di, NULL, val);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
 		ret = bq27xxx_battery_voltage(di, val);
@@ -1969,7 +1967,7 @@  static int bq27xxx_battery_get_property(struct power_supply *psy,
 		val->intval = di->cache.flags < 0 ? 0 : 1;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
-		ret = bq27xxx_battery_current(di, val);
+		ret = bq27xxx_battery_current_and_status(di, val, NULL);
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
 		ret = bq27xxx_simple_value(di->cache.capacity, val);