diff mbox series

[PATCHv1,13/19] power: supply: sbs-battery: add POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED support

Message ID 20200513185615.508236-14-sebastian.reichel@collabora.com (mailing list archive)
State Not Applicable, archived
Headers show
Series Improve SBS battery support | expand

Commit Message

Sebastian Reichel May 13, 2020, 6:56 p.m. UTC
Add support for reporting the SBS battery's condition flag
to userspace using the new "Calibration required" health status.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/power/supply/sbs-battery.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Comments

Emil Velikov May 15, 2020, 3:35 p.m. UTC | #1
On 2020/05/13, Sebastian Reichel wrote:
> Add support for reporting the SBS battery's condition flag
> to userspace using the new "Calibration required" health status.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/power/supply/sbs-battery.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 4fa553d61db2..2a2b926ad75c 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -23,6 +23,7 @@
>  
>  enum {
>  	REG_MANUFACTURER_DATA,
> +	REG_BATTERY_MODE,
>  	REG_TEMPERATURE,
>  	REG_VOLTAGE,
>  	REG_CURRENT_NOW,
> @@ -94,6 +95,8 @@ static const struct chip_data {
>  } sbs_data[] = {
>  	[REG_MANUFACTURER_DATA] =
>  		SBS_DATA(POWER_SUPPLY_PROP_PRESENT, 0x00, 0, 65535),
> +	[REG_BATTERY_MODE] =
> +		SBS_DATA(-1, 0x03, 0, 65535),

Fwiw I really like how neatly the driver is split into components. One thing
which makes me wonder, have you considered reshuffling the sbs_data struct.

In particular:
 - index POWER_SUPPLY_PROP, kill off the REG_ enum
   - sbs_get_property_index() can go, alongside a couple of unreachable paths
   - replace batter_mode (needs calibration) with with PROP_HEALTH + comment
   - perhaps even add REG_ADDR_SPEC_INFO 0x1a under POWER_SUPPLY_PROP_PRESENT
 - using the min/max seems wasteful, considering only one register is in s16
   range while everything else is within u16


Regardless of the questions and trivial suggestions, the series looks spot on.

For the lot:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>


-Emil
P.S. The reg table is nearly complete only 0x01-0x07, 0x0E, 0x11, 0x1d-0x1f
remain o/
diff mbox series

Patch

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 4fa553d61db2..2a2b926ad75c 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -23,6 +23,7 @@ 
 
 enum {
 	REG_MANUFACTURER_DATA,
+	REG_BATTERY_MODE,
 	REG_TEMPERATURE,
 	REG_VOLTAGE,
 	REG_CURRENT_NOW,
@@ -94,6 +95,8 @@  static const struct chip_data {
 } sbs_data[] = {
 	[REG_MANUFACTURER_DATA] =
 		SBS_DATA(POWER_SUPPLY_PROP_PRESENT, 0x00, 0, 65535),
+	[REG_BATTERY_MODE] =
+		SBS_DATA(-1, 0x03, 0, 65535),
 	[REG_TEMPERATURE] =
 		SBS_DATA(POWER_SUPPLY_PROP_TEMP, 0x08, 0, 65535),
 	[REG_VOLTAGE] =
@@ -366,6 +369,17 @@  static int sbs_status_correct(struct i2c_client *client, int *intval)
 	return 0;
 }
 
+static bool sbs_bat_needs_calibration(struct i2c_client *client)
+{
+	int ret;
+
+	ret = sbs_read_word_data(client, sbs_data[REG_BATTERY_MODE].addr);
+	if (ret < 0)
+		return false;
+
+	return !!(ret & BIT(7));
+}
+
 static int sbs_get_battery_presence_and_health(
 	struct i2c_client *client, enum power_supply_property psp,
 	union power_supply_propval *val)
@@ -385,9 +399,14 @@  static int sbs_get_battery_presence_and_health(
 
 	if (psp == POWER_SUPPLY_PROP_PRESENT)
 		val->intval = 1; /* battery present */
-	else /* POWER_SUPPLY_PROP_HEALTH */
-		/* SBS spec doesn't have a general health command. */
-		val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+	else { /* POWER_SUPPLY_PROP_HEALTH */
+		if (sbs_bat_needs_calibration(client)) {
+			val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
+		} else {
+			/* SBS spec doesn't have a general health command. */
+			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+		}
+	}
 
 	return 0;
 }
@@ -441,6 +460,8 @@  static int sbs_get_ti_battery_presence_and_health(
 			val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
 		else if (ret == 0x0C)
 			val->intval = POWER_SUPPLY_HEALTH_DEAD;
+		else if (sbs_bat_needs_calibration(client))
+			val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
 		else
 			val->intval = POWER_SUPPLY_HEALTH_GOOD;
 	}