SBS: Make battery_get_params clear flags selectively

Currently, battery_get_params clear all flags in batt.flags. This patch
makes the function clear only the flags which are explicitly set or
unset by battery_get_params. With this change, the charger task can add
a flag to batt.flags.

There isn't yet functionality change by this patch.

BUG=b:263921114
BRANCH=None
TEST=run-battery_get_params_smart run-sbs_charging_v2
TEST=./twister -i --toolchain host -s \
     external/platform/ec/zephyr/test/drivers/drivers.default

Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
Change-Id: I5708ab6de81bc0a7c28961b13960fd89460b2e1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4136966
Code-Coverage: Zoss <zoss-cl-coverage@prod.google.com>
Reviewed-by: Paul Fagerburg <pfagerburg@chromium.org>
Commit-Queue: Paul Fagerburg <pfagerburg@chromium.org>
Reviewed-by: Bob Moragues <moragues@chromium.org>

4 files changed, 62 insertions, 21 deletions

diff --git a/driver/battery/smart.c b/driver/battery/smart.c
index 539580d201..aac76ace74 100644
--- a/driver/battery/smart.c
+++ b/driver/battery/smart.c
@@ -451,6 +451,34 @@ static void apply_fake_state_of_charge(struct batt_params *batt)
 	batt->flags &= ~BATT_FLAG_BAD_REMAINING_CAPACITY;
 }
 
+static bool battery_want_charge(struct batt_params *batt)
+{
+	if (batt->flags &
+	    (BATT_FLAG_BAD_DESIRED_VOLTAGE | BATT_FLAG_BAD_DESIRED_CURRENT |
+	     BATT_FLAG_BAD_STATE_OF_CHARGE))
+		return false;
+
+	/*
+	 * Charging allowed if both desired voltage and current are nonzero
+	 * and battery isn't full (and we read them all correctly).
+	 */
+	if (batt->desired_voltage && batt->desired_current &&
+	    batt->state_of_charge < BATTERY_LEVEL_FULL)
+		return true;
+
+	/*
+	 * TODO (crosbug.com/p/29467): remove this workaround for dead battery
+	 * that requests no voltage/current
+	 */
+	if (IS_ENABLED(CONFIG_BATTERY_REQUESTS_NIL_WHEN_DEAD)) {
+		if (batt->desired_voltage == 0 && batt->desired_current == 0 &&
+		    batt->state_of_charge == 0)
+			return true;
+	}
+
+	return false;
+}
+
 void battery_get_params(struct batt_params *batt)
 {
 	struct batt_params batt_new;
@@ -462,7 +490,7 @@ void battery_get_params(struct batt_params *batt)
 	 * will be preserved.
 	 */
 	memcpy(&batt_new, batt, sizeof(*batt));
-	batt_new.flags = 0;
+	batt_new.flags &= ~BATT_FLAG_VOLATILE;
 
 	if (sb_read(SB_TEMPERATURE, &batt_new.temperature) &&
 	    fake_temperature < 0)
@@ -487,6 +515,7 @@ void battery_get_params(struct batt_params *batt)
 
 	if (sb_read(SB_AVERAGE_CURRENT, &v))
 		batt_new.flags |= BATT_FLAG_BAD_AVERAGE_CURRENT;
+
 	if (sb_read(SB_CHARGING_VOLTAGE, &batt_new.desired_voltage))
 		batt_new.flags |= BATT_FLAG_BAD_DESIRED_VOLTAGE;
 
@@ -523,26 +552,7 @@ void battery_get_params(struct batt_params *batt)
 		batt_new.is_present = BP_NOT_SURE;
 #endif
 
-	/*
-	 * Charging allowed if both desired voltage and current are nonzero
-	 * and battery isn't full (and we read them all correctly).
-	 */
-	if (!(batt_new.flags &
-	      (BATT_FLAG_BAD_DESIRED_VOLTAGE | BATT_FLAG_BAD_DESIRED_CURRENT |
-	       BATT_FLAG_BAD_STATE_OF_CHARGE)) &&
-#ifdef CONFIG_BATTERY_REQUESTS_NIL_WHEN_DEAD
-	    /*
-	     * TODO (crosbug.com/p/29467): remove this workaround
-	     * for dead battery that requests no voltage/current
-	     */
-	    ((batt_new.desired_voltage && batt_new.desired_current &&
-	      batt_new.state_of_charge < BATTERY_LEVEL_FULL) ||
-	     (batt_new.desired_voltage == 0 && batt_new.desired_current == 0 &&
-	      batt_new.state_of_charge == 0)))
-#else
-	    batt_new.desired_voltage && batt_new.desired_current &&
-	    batt_new.state_of_charge < BATTERY_LEVEL_FULL)
-#endif
+	if (battery_want_charge(&batt_new))
 		batt_new.flags |= BATT_FLAG_WANT_CHARGE;
 	else
 		/* Force both to zero */
diff --git a/include/battery.h b/include/battery.h
index f754c9de91..d7e9a0f0c2 100644
--- a/include/battery.h
+++ b/include/battery.h
@@ -165,6 +165,10 @@ int battery_get_avg_voltage(void); /* in mV */
 #define BATT_FLAG_BAD_AVERAGE_CURRENT 0x00001000
 /* All of the above BATT_FLAG_BAD_* bits */
 #define BATT_FLAG_BAD_ANY 0x000017fc
+/* Flags which are set or unset on every access (via battery_get_params) */
+#define BATT_FLAG_VOLATILE                                                  \
+	(BATT_FLAG_BAD_ANY | BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE | \
+	 BATT_FLAG_IMBALANCED_CELL)
 
 /* Battery constants */
 struct battery_info {
diff --git a/test/battery_get_params_smart.c b/test/battery_get_params_smart.c
index 48273c4be0..63a0abe64c 100644
--- a/test/battery_get_params_smart.c
+++ b/test/battery_get_params_smart.c
@@ -163,6 +163,16 @@ static int test_flags(void)
 	test_flag(SB_BATTERY_STATUS, BATT_FLAG_BAD_STATUS);
 
 	/*
+	 * Volatile flags should be cleared and other flags should be preserved.
+	 */
+	reset_and_fail_on(0, 0, -1);
+	batt.flags |= BATT_FLAG_BAD_TEMPERATURE;
+	batt.flags |= BIT(31);
+	battery_get_params(&batt);
+	TEST_ASSERT(batt.flags & BIT(31));
+	TEST_ASSERT(!(batt.flags & BATT_FLAG_BAD_ANY));
+
+	/*
 	 * All reads succeed. BATT_FLAG_RESPONSIVE should be set. Then, all
 	 * reads fail. BATT_FLAG_RESPONSIVE should be cleared.
 	 */
diff --git a/zephyr/test/drivers/default/src/smart.c b/zephyr/test/drivers/default/src/smart.c
index a69487ccf8..7ee0c2a219 100644
--- a/zephyr/test/drivers/default/src/smart.c
+++ b/zephyr/test/drivers/default/src/smart.c
@@ -257,6 +257,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_TEMPERATURE);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_TEMPERATURE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -264,6 +265,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data,
 					  SB_RELATIVE_STATE_OF_CHARGE);
 	flags = BATT_FLAG_RESPONSIVE | BATT_FLAG_BAD_STATE_OF_CHARGE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -271,6 +273,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_VOLTAGE);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_VOLTAGE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -278,6 +281,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_CURRENT);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_CURRENT;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -285,18 +289,21 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_AVERAGE_CURRENT);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_AVERAGE_CURRENT;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
 	/* Fail charging voltage read; want charge cannot be set */
 	i2c_common_emul_set_read_fail_reg(common_data, SB_CHARGING_VOLTAGE);
 	flags = BATT_FLAG_RESPONSIVE | BATT_FLAG_BAD_DESIRED_VOLTAGE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
 	/* Fail charging voltage read; want charge cannot be set */
 	i2c_common_emul_set_read_fail_reg(common_data, SB_CHARGING_CURRENT);
 	flags = BATT_FLAG_RESPONSIVE | BATT_FLAG_BAD_DESIRED_CURRENT;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -304,6 +311,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_REMAINING_CAPACITY);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_REMAINING_CAPACITY;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -311,6 +319,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_FULL_CHARGE_CAPACITY);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_FULL_CAPACITY;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -318,6 +327,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_BATTERY_STATUS);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_STATUS;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -325,6 +335,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data,
 					  I2C_COMMON_EMUL_FAIL_ALL_REG);
 	flags = BATT_FLAG_BAD_ANY;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 
@@ -332,6 +343,7 @@ ZTEST_USER(smart_battery, test_battery_get_params)
 	i2c_common_emul_set_read_fail_reg(common_data,
 					  I2C_COMMON_EMUL_NO_FAIL_REG);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 }
@@ -471,6 +483,7 @@ ZTEST_USER(smart_battery, test_battery_fake_charge)
 
 	/* Test that fake charge level is applied */
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 	zassert_equal(fake_charge, batt.state_of_charge, "%d%% != %d%%",
@@ -483,6 +496,7 @@ ZTEST_USER(smart_battery, test_battery_fake_charge)
 	i2c_common_emul_set_read_fail_reg(common_data, SB_FULL_CHARGE_CAPACITY);
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE |
 		BATT_FLAG_BAD_FULL_CAPACITY;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 	zassert_equal(fake_charge, batt.state_of_charge, "%d%% != %d%%",
@@ -499,6 +513,7 @@ ZTEST_USER(smart_battery, test_battery_fake_charge)
 
 	/* Test that fake charge level is not applied */
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 	charge = 100 * bat->cap / bat->full_cap;
@@ -551,6 +566,7 @@ ZTEST_USER(smart_battery, test_battery_fake_temperature)
 
 	/* Test that fake temperature is applied */
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 	zassert_equal(fake_temp, batt.temperature, "%d != %d", fake_temp,
@@ -563,6 +579,7 @@ ZTEST_USER(smart_battery, test_battery_fake_temperature)
 
 	/* Test that fake temperature is not applied */
 	flags = BATT_FLAG_WANT_CHARGE | BATT_FLAG_RESPONSIVE;
+	batt.flags = 0;
 	battery_get_params(&batt);
 	zassert_equal(flags, batt.flags, "0x%x != 0x%x", flags, batt.flags);
 	zassert_equal(bat->temp, batt.temperature, "%d != %d", bat->temp,