All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] power: supply_core: Pass pointer to battery info
@ 2021-12-06  0:06 Linus Walleij
  2021-12-08  6:46 ` Matti Vaittinen
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2021-12-06  0:06 UTC (permalink / raw)
  To: Sebastian Reichel; +Cc: linux-pm, Linus Walleij

The function to retrieve battery info (from the device tree) assumes
we have a static info struct that gets populated by calling into
power_supply_get_battery_info().

This is awkward since I want to support tables of static battery
info by just assigning a pointer to all info based on e.g. a
compatible value in the device tree.

We also have a mixture of static and dynamically allocated
variables here.

Bite the bullet and let power_supply_get_battery_info() allocate
also the memory used for the very top level
struct power_supply_battery_info container. Pass pointers
around and lifecycle this with the psy device just like the
stuff we allocate inside it.

Change all current users over.

In the bd99954 charger driver we need to stop issuing
power_supply_put_battery_info() at the end of the probe since
the struct continues to be used after probe().

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix two bugs causing compile errors - was hard to get
  compiler coverage but the build servers stepped in and
  corrected me.
---
 drivers/power/supply/ab8500-bm.h         |  2 +-
 drivers/power/supply/ab8500_bmdata.c     |  7 +++--
 drivers/power/supply/ab8500_btemp.c      | 10 +++++--
 drivers/power/supply/ab8500_chargalg.c   | 16 +++++-----
 drivers/power/supply/ab8500_fg.c         | 18 ++++++-----
 drivers/power/supply/axp20x_battery.c    |  6 ++--
 drivers/power/supply/bd99954-charger.c   | 24 +++++++--------
 drivers/power/supply/bq24190_charger.c   |  6 ++--
 drivers/power/supply/bq2515x_charger.c   |  8 ++---
 drivers/power/supply/bq256xx_charger.c   | 24 +++++++--------
 drivers/power/supply/bq25980_charger.c   |  6 ++--
 drivers/power/supply/bq27xxx_battery.c   | 38 ++++++++++++------------
 drivers/power/supply/cw2015_battery.c    | 20 ++++++++-----
 drivers/power/supply/ingenic-battery.c   | 14 ++++-----
 drivers/power/supply/power_supply_core.c | 15 ++++++++--
 drivers/power/supply/sc2731_charger.c    |  8 ++---
 drivers/power/supply/sc27xx_fuel_gauge.c | 22 +++++++-------
 drivers/power/supply/smb347-charger.c    | 34 ++++++++++-----------
 include/linux/power_supply.h             |  2 +-
 19 files changed, 152 insertions(+), 128 deletions(-)

diff --git a/drivers/power/supply/ab8500-bm.h b/drivers/power/supply/ab8500-bm.h
index 57e1a8e27e51..56a5aaf9a27a 100644
--- a/drivers/power/supply/ab8500-bm.h
+++ b/drivers/power/supply/ab8500-bm.h
@@ -439,7 +439,7 @@ struct ab8500_bm_charger_parameters {
  * @fg_params		fuel gauge parameters
  */
 struct ab8500_bm_data {
-	struct power_supply_battery_info bi;
+	struct power_supply_battery_info *bi;
 	int temp_now;
 	int temp_interval_chg;
 	int temp_interval_nochg;
diff --git a/drivers/power/supply/ab8500_bmdata.c b/drivers/power/supply/ab8500_bmdata.c
index 62953f9cb85a..7ae95f537580 100644
--- a/drivers/power/supply/ab8500_bmdata.c
+++ b/drivers/power/supply/ab8500_bmdata.c
@@ -167,15 +167,16 @@ struct ab8500_bm_data ab8500_bm_data = {
 int ab8500_bm_of_probe(struct power_supply *psy,
 		       struct ab8500_bm_data *bm)
 {
-	struct power_supply_battery_info *bi = &bm->bi;
+	struct power_supply_battery_info *bi;
 	struct device *dev = &psy->dev;
 	int ret;
 
-	ret = power_supply_get_battery_info(psy, bi);
+	ret = power_supply_get_battery_info(psy, &bm->bi);
 	if (ret) {
 		dev_err(dev, "cannot retrieve battery info\n");
 		return ret;
 	}
+	bi = bm->bi;
 
 	/* Fill in defaults for any data missing from the device tree */
 	if (bi->charge_full_design_uah < 0)
@@ -240,5 +241,5 @@ int ab8500_bm_of_probe(struct power_supply *psy,
 void ab8500_bm_of_remove(struct power_supply *psy,
 			 struct ab8500_bm_data *bm)
 {
-	power_supply_put_battery_info(psy, &bm->bi);
+	power_supply_put_battery_info(psy, bm->bi);
 }
diff --git a/drivers/power/supply/ab8500_btemp.c b/drivers/power/supply/ab8500_btemp.c
index 20253b8a7fe9..cc33c5187fbb 100644
--- a/drivers/power/supply/ab8500_btemp.c
+++ b/drivers/power/supply/ab8500_btemp.c
@@ -451,12 +451,13 @@ static int ab8500_btemp_res_to_temp(struct ab8500_btemp *di,
  */
 static int ab8500_btemp_measure_temp(struct ab8500_btemp *di)
 {
+	struct power_supply_battery_info *bi = di->bm->bi;
 	int temp, ret;
 	static int prev;
 	int rbat, rntc, vntc;
 
 	if ((di->bm->adc_therm == AB8500_ADC_THERM_BATCTRL) &&
-	    (di->bm->bi.technology == POWER_SUPPLY_TECHNOLOGY_UNKNOWN)) {
+	    (bi && (bi->technology == POWER_SUPPLY_TECHNOLOGY_UNKNOWN))) {
 
 		rbat = ab8500_btemp_get_batctrl_res(di);
 		if (rbat < 0) {
@@ -540,7 +541,7 @@ static int ab8500_btemp_id(struct ab8500_btemp *di)
 	 * that need it.
 	 */
 	if ((di->bm->adc_therm == AB8500_ADC_THERM_BATCTRL) &&
-	    (di->bm->bi.technology == POWER_SUPPLY_TECHNOLOGY_LIPO) &&
+	    (di->bm->bi && (di->bm->bi->technology == POWER_SUPPLY_TECHNOLOGY_LIPO)) &&
 	    (res <= 53407) && (res >= 12500)) {
 		dev_dbg(di->dev, "Set BATCTRL current source to 20uA\n");
 		di->curr_source = BTEMP_BATCTRL_CURR_SRC_20UA;
@@ -807,7 +808,10 @@ static int ab8500_btemp_get_property(struct power_supply *psy,
 			val->intval = 1;
 		break;
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
-		val->intval = di->bm->bi.technology;
+		if (di->bm->bi)
+			val->intval = di->bm->bi->technology;
+		else
+			val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 		break;
 	case POWER_SUPPLY_PROP_TEMP:
 		val->intval = ab8500_btemp_get_temp(di);
diff --git a/drivers/power/supply/ab8500_chargalg.c b/drivers/power/supply/ab8500_chargalg.c
index 86d740ce3a63..c4a2fe07126c 100644
--- a/drivers/power/supply/ab8500_chargalg.c
+++ b/drivers/power/supply/ab8500_chargalg.c
@@ -352,7 +352,7 @@ static void ab8500_chargalg_state_to(struct ab8500_chargalg *di,
 
 static int ab8500_chargalg_check_charger_enable(struct ab8500_chargalg *di)
 {
-	struct power_supply_battery_info *bi = &di->bm->bi;
+	struct power_supply_battery_info *bi = di->bm->bi;
 
 	switch (di->charge_state) {
 	case STATE_NORMAL:
@@ -731,7 +731,7 @@ static void ab8500_chargalg_start_charging(struct ab8500_chargalg *di,
  */
 static void ab8500_chargalg_check_temp(struct ab8500_chargalg *di)
 {
-	struct power_supply_battery_info *bi = &di->bm->bi;
+	struct power_supply_battery_info *bi = di->bm->bi;
 
 	if (di->batt_data.temp > (bi->temp_alert_min + di->t_hyst_norm) &&
 		di->batt_data.temp < (bi->temp_alert_max - di->t_hyst_norm)) {
@@ -802,10 +802,10 @@ static void ab8500_chargalg_end_of_charge(struct ab8500_chargalg *di)
 	if (di->charge_status == POWER_SUPPLY_STATUS_CHARGING &&
 		di->charge_state == STATE_NORMAL &&
 		!di->maintenance_chg && (di->batt_data.volt_uv >=
-		di->bm->bi.overvoltage_limit_uv ||
+		di->bm->bi->overvoltage_limit_uv ||
 		di->events.usb_cv_active || di->events.ac_cv_active) &&
 		di->batt_data.avg_curr_ua <
-		di->bm->bi.charge_term_current_ua &&
+		di->bm->bi->charge_term_current_ua &&
 		di->batt_data.avg_curr_ua > 0) {
 		if (++di->eoc_cnt >= EOC_COND_CNT) {
 			di->eoc_cnt = 0;
@@ -827,7 +827,7 @@ static void ab8500_chargalg_end_of_charge(struct ab8500_chargalg *di)
 
 static void init_maxim_chg_curr(struct ab8500_chargalg *di)
 {
-	struct power_supply_battery_info *bi = &di->bm->bi;
+	struct power_supply_battery_info *bi = di->bm->bi;
 
 	di->ccm.original_iset_ua = bi->constant_charge_current_max_ua;
 	di->ccm.current_iset_ua = bi->constant_charge_current_max_ua;
@@ -920,7 +920,7 @@ static enum maxim_ret ab8500_chargalg_chg_curr_maxim(struct ab8500_chargalg *di)
 
 static void handle_maxim_chg_curr(struct ab8500_chargalg *di)
 {
-	struct power_supply_battery_info *bi = &di->bm->bi;
+	struct power_supply_battery_info *bi = di->bm->bi;
 	enum maxim_ret ret;
 	int result;
 
@@ -1299,7 +1299,7 @@ static void ab8500_chargalg_external_power_changed(struct power_supply *psy)
  */
 static void ab8500_chargalg_algorithm(struct ab8500_chargalg *di)
 {
-	struct power_supply_battery_info *bi = &di->bm->bi;
+	struct power_supply_battery_info *bi = di->bm->bi;
 	int charger_status;
 	int ret;
 	int curr_step_lvl_ua;
@@ -1723,7 +1723,7 @@ static int ab8500_chargalg_get_property(struct power_supply *psy,
 		if (di->events.batt_ovv) {
 			val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
 		} else if (di->events.btemp_underover) {
-			if (di->batt_data.temp <= di->bm->bi.temp_min)
+			if (di->batt_data.temp <= di->bm->bi->temp_min)
 				val->intval = POWER_SUPPLY_HEALTH_COLD;
 			else
 				val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
diff --git a/drivers/power/supply/ab8500_fg.c b/drivers/power/supply/ab8500_fg.c
index eb3e5c4ca44f..b0919a6a6587 100644
--- a/drivers/power/supply/ab8500_fg.c
+++ b/drivers/power/supply/ab8500_fg.c
@@ -852,7 +852,7 @@ static int ab8500_fg_bat_voltage(struct ab8500_fg *di)
  */
 static int ab8500_fg_volt_to_capacity(struct ab8500_fg *di, int voltage_uv)
 {
-	struct power_supply_battery_info *bi = &di->bm->bi;
+	struct power_supply_battery_info *bi = di->bm->bi;
 
 	/* Multiply by 10 because the capacity is tracked in per mille */
 	return power_supply_batinfo_ocv2cap(bi, voltage_uv, di->bat_temp) *  10;
@@ -881,7 +881,7 @@ static int ab8500_fg_uncomp_volt_to_capacity(struct ab8500_fg *di)
  */
 static int ab8500_fg_battery_resistance(struct ab8500_fg *di)
 {
-	struct power_supply_battery_info *bi = &di->bm->bi;
+	struct power_supply_battery_info *bi = di->bm->bi;
 	int resistance_percent = 0;
 	int resistance;
 
@@ -2140,11 +2140,13 @@ static int ab8500_fg_get_ext_psy_data(struct device *dev, void *data)
 	struct power_supply *ext = dev_get_drvdata(dev);
 	const char **supplicants = (const char **)ext->supplied_to;
 	struct ab8500_fg *di;
+	struct power_supply_battery_info *bi;
 	union power_supply_propval ret;
 	int j;
 
 	psy = (struct power_supply *)data;
 	di = power_supply_get_drvdata(psy);
+	bi = di->bm->bi;
 
 	/*
 	 * For all psy where the name of your driver
@@ -2207,8 +2209,8 @@ static int ab8500_fg_get_ext_psy_data(struct device *dev, void *data)
 			switch (ext->desc->type) {
 			case POWER_SUPPLY_TYPE_BATTERY:
 				if (!di->flags.batt_id_received &&
-				    (di->bm->bi.technology !=
-				     POWER_SUPPLY_TECHNOLOGY_UNKNOWN)) {
+				    (bi && (bi->technology !=
+					    POWER_SUPPLY_TECHNOLOGY_UNKNOWN))) {
 					const struct ab8500_battery_type *b;
 
 					b = di->bm->bat_type;
@@ -2216,13 +2218,13 @@ static int ab8500_fg_get_ext_psy_data(struct device *dev, void *data)
 					di->flags.batt_id_received = true;
 
 					di->bat_cap.max_mah_design =
-						di->bm->bi.charge_full_design_uah;
+						di->bm->bi->charge_full_design_uah;
 
 					di->bat_cap.max_mah =
 						di->bat_cap.max_mah_design;
 
 					di->vbat_nom_uv =
-						di->bm->bi.voltage_max_design_uv;
+						di->bm->bi->voltage_max_design_uv;
 				}
 
 				if (ret.intval)
@@ -2992,9 +2994,9 @@ static int ab8500_fg_bind(struct device *dev, struct device *master,
 		return -ENOMEM;
 	}
 
-	di->bat_cap.max_mah_design = di->bm->bi.charge_full_design_uah;
+	di->bat_cap.max_mah_design = di->bm->bi->charge_full_design_uah;
 	di->bat_cap.max_mah = di->bat_cap.max_mah_design;
-	di->vbat_nom_uv = di->bm->bi.voltage_max_design_uv;
+	di->vbat_nom_uv = di->bm->bi->voltage_max_design_uv;
 
 	/* Start the coulomb counter */
 	ab8500_fg_coulomb_counter(di, true);
diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
index 18a9db0df4b1..5d197141f476 100644
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -561,7 +561,7 @@ static int axp20x_power_probe(struct platform_device *pdev)
 {
 	struct axp20x_batt_ps *axp20x_batt;
 	struct power_supply_config psy_cfg = {};
-	struct power_supply_battery_info info;
+	struct power_supply_battery_info *info;
 	struct device *dev = &pdev->dev;
 
 	if (!of_device_is_available(pdev->dev.of_node))
@@ -615,8 +615,8 @@ static int axp20x_power_probe(struct platform_device *pdev)
 	}
 
 	if (!power_supply_get_battery_info(axp20x_batt->batt, &info)) {
-		int vmin = info.voltage_min_design_uv;
-		int ccc = info.constant_charge_current_max_ua;
+		int vmin = info->voltage_min_design_uv;
+		int ccc = info->constant_charge_current_max_ua;
 
 		if (vmin > 0 && axp20x_set_voltage_min_design(axp20x_batt,
 							      vmin))
diff --git a/drivers/power/supply/bd99954-charger.c b/drivers/power/supply/bd99954-charger.c
index ffd8bfa08179..c22d0c804fd6 100644
--- a/drivers/power/supply/bd99954-charger.c
+++ b/drivers/power/supply/bd99954-charger.c
@@ -882,7 +882,7 @@ struct dt_init {
 static int bd9995x_fw_probe(struct bd9995x_device *bd)
 {
 	int ret;
-	struct power_supply_battery_info info;
+	struct power_supply_battery_info *info;
 	u32 property;
 	int i;
 	int regval;
@@ -891,49 +891,41 @@ static int bd9995x_fw_probe(struct bd9995x_device *bd)
 	struct battery_init battery_inits[] = {
 		{
 			.name = "trickle-charging current",
-			.info_data = &info.tricklecharge_current_ua,
 			.range = &charging_current_ranges[0],
 			.ranges = 2,
 			.data = &init->itrich_set,
 		}, {
 			.name = "pre-charging current",
-			.info_data = &info.precharge_current_ua,
 			.range = &charging_current_ranges[0],
 			.ranges = 2,
 			.data = &init->iprech_set,
 		}, {
 			.name = "pre-to-trickle charge voltage threshold",
-			.info_data = &info.precharge_voltage_max_uv,
 			.range = &trickle_to_pre_threshold_ranges[0],
 			.ranges = 2,
 			.data = &init->vprechg_th_set,
 		}, {
 			.name = "charging termination current",
-			.info_data = &info.charge_term_current_ua,
 			.range = &charging_current_ranges[0],
 			.ranges = 2,
 			.data = &init->iterm_set,
 		}, {
 			.name = "charging re-start voltage",
-			.info_data = &info.charge_restart_voltage_uv,
 			.range = &charge_voltage_regulation_ranges[0],
 			.ranges = 2,
 			.data = &init->vrechg_set,
 		}, {
 			.name = "battery overvoltage limit",
-			.info_data = &info.overvoltage_limit_uv,
 			.range = &charge_voltage_regulation_ranges[0],
 			.ranges = 2,
 			.data = &init->vbatovp_set,
 		}, {
 			.name = "fast-charging max current",
-			.info_data = &info.constant_charge_current_max_ua,
 			.range = &fast_charge_current_ranges[0],
 			.ranges = 1,
 			.data = &init->ichg_set,
 		}, {
 			.name = "fast-charging voltage",
-			.info_data = &info.constant_charge_voltage_max_uv,
 			.range = &charge_voltage_regulation_ranges[0],
 			.ranges = 2,
 			.data = &init->vfastchg_reg_set1,
@@ -966,6 +958,16 @@ static int bd9995x_fw_probe(struct bd9995x_device *bd)
 	if (ret < 0)
 		return ret;
 
+	/* Put pointers to the generic battery info */
+	battery_inits[0].info_data = &info->tricklecharge_current_ua;
+	battery_inits[1].info_data = &info->precharge_current_ua;
+	battery_inits[2].info_data = &info->precharge_voltage_max_uv;
+	battery_inits[3].info_data = &info->charge_term_current_ua;
+	battery_inits[4].info_data = &info->charge_restart_voltage_uv;
+	battery_inits[5].info_data = &info->overvoltage_limit_uv;
+	battery_inits[6].info_data = &info->constant_charge_current_max_ua;
+	battery_inits[7].info_data = &info->constant_charge_voltage_max_uv;
+
 	for (i = 0; i < ARRAY_SIZE(battery_inits); i++) {
 		int val = *battery_inits[i].info_data;
 		const struct linear_range *range = battery_inits[i].range;
@@ -980,7 +982,7 @@ static int bd9995x_fw_probe(struct bd9995x_device *bd)
 			dev_err(bd->dev, "Unsupported value for %s\n",
 				battery_inits[i].name);
 
-			power_supply_put_battery_info(bd->charger, &info);
+			power_supply_put_battery_info(bd->charger, info);
 			return -EINVAL;
 		}
 		if (!found) {
@@ -991,8 +993,6 @@ static int bd9995x_fw_probe(struct bd9995x_device *bd)
 		*(battery_inits[i].data) = regval;
 	}
 
-	power_supply_put_battery_info(bd->charger, &info);
-
 	for (i = 0; i < ARRAY_SIZE(props); i++) {
 		ret = device_property_read_u32(bd->dev, props[i].prop,
 					       &property);
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 35ff0c8fe96f..06c34b09349c 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1670,7 +1670,7 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 static int bq24190_get_config(struct bq24190_dev_info *bdi)
 {
 	const char * const s = "ti,system-minimum-microvolt";
-	struct power_supply_battery_info info = {};
+	struct power_supply_battery_info *info;
 	int v;
 
 	if (device_property_read_u32(bdi->dev, s, &v) == 0) {
@@ -1684,7 +1684,7 @@ static int bq24190_get_config(struct bq24190_dev_info *bdi)
 
 	if (bdi->dev->of_node &&
 	    !power_supply_get_battery_info(bdi->charger, &info)) {
-		v = info.precharge_current_ua / 1000;
+		v = info->precharge_current_ua / 1000;
 		if (v >= BQ24190_REG_PCTCC_IPRECHG_MIN
 		 && v <= BQ24190_REG_PCTCC_IPRECHG_MAX)
 			bdi->iprechg = v;
@@ -1692,7 +1692,7 @@ static int bq24190_get_config(struct bq24190_dev_info *bdi)
 			dev_warn(bdi->dev, "invalid value for battery:precharge-current-microamp: %d\n",
 				 v);
 
-		v = info.charge_term_current_ua / 1000;
+		v = info->charge_term_current_ua / 1000;
 		if (v >= BQ24190_REG_PCTCC_ITERM_MIN
 		 && v <= BQ24190_REG_PCTCC_ITERM_MAX)
 			bdi->iterm = v;
diff --git a/drivers/power/supply/bq2515x_charger.c b/drivers/power/supply/bq2515x_charger.c
index 374b112f712a..4f76ad9c2f18 100644
--- a/drivers/power/supply/bq2515x_charger.c
+++ b/drivers/power/supply/bq2515x_charger.c
@@ -945,7 +945,7 @@ static int bq2515x_power_supply_register(struct bq2515x_device *bq2515x,
 static int bq2515x_hw_init(struct bq2515x_device *bq2515x)
 {
 	int ret;
-	struct power_supply_battery_info bat_info = { };
+	struct power_supply_battery_info *bat_info;
 
 	ret = bq2515x_disable_watchdog_timers(bq2515x);
 	if (ret)
@@ -969,13 +969,13 @@ static int bq2515x_hw_init(struct bq2515x_device *bq2515x)
 
 	} else {
 		bq2515x->init_data.ichg =
-				bat_info.constant_charge_current_max_ua;
+				bat_info->constant_charge_current_max_ua;
 
 		bq2515x->init_data.vbatreg =
-				bat_info.constant_charge_voltage_max_uv;
+				bat_info->constant_charge_voltage_max_uv;
 
 		bq2515x->init_data.iprechg =
-				bat_info.precharge_current_ua;
+				bat_info->precharge_current_ua;
 	}
 
 	ret = bq2515x_set_const_charge_current(bq2515x,
diff --git a/drivers/power/supply/bq256xx_charger.c b/drivers/power/supply/bq256xx_charger.c
index f501ecd49202..b274942dc46a 100644
--- a/drivers/power/supply/bq256xx_charger.c
+++ b/drivers/power/supply/bq256xx_charger.c
@@ -1504,7 +1504,7 @@ static int bq256xx_power_supply_init(struct bq256xx_device *bq,
 
 static int bq256xx_hw_init(struct bq256xx_device *bq)
 {
-	struct power_supply_battery_info bat_info = { };
+	struct power_supply_battery_info *bat_info;
 	int wd_reg_val = BQ256XX_WATCHDOG_DIS;
 	int ret = 0;
 	int i;
@@ -1526,16 +1526,16 @@ static int bq256xx_hw_init(struct bq256xx_device *bq)
 	if (ret) {
 		dev_warn(bq->dev, "battery info missing, default values will be applied\n");
 
-		bat_info.constant_charge_current_max_ua =
+		bat_info->constant_charge_current_max_ua =
 				bq->chip_info->bq256xx_def_ichg;
 
-		bat_info.constant_charge_voltage_max_uv =
+		bat_info->constant_charge_voltage_max_uv =
 				bq->chip_info->bq256xx_def_vbatreg;
 
-		bat_info.precharge_current_ua =
+		bat_info->precharge_current_ua =
 				bq->chip_info->bq256xx_def_iprechg;
 
-		bat_info.charge_term_current_ua =
+		bat_info->charge_term_current_ua =
 				bq->chip_info->bq256xx_def_iterm;
 
 		bq->init_data.ichg_max =
@@ -1545,10 +1545,10 @@ static int bq256xx_hw_init(struct bq256xx_device *bq)
 				bq->chip_info->bq256xx_max_vbatreg;
 	} else {
 		bq->init_data.ichg_max =
-			bat_info.constant_charge_current_max_ua;
+			bat_info->constant_charge_current_max_ua;
 
 		bq->init_data.vbatreg_max =
-			bat_info.constant_charge_voltage_max_uv;
+			bat_info->constant_charge_voltage_max_uv;
 	}
 
 	ret = bq->chip_info->bq256xx_set_vindpm(bq, bq->init_data.vindpm);
@@ -1560,26 +1560,26 @@ static int bq256xx_hw_init(struct bq256xx_device *bq)
 		return ret;
 
 	ret = bq->chip_info->bq256xx_set_ichg(bq,
-				bat_info.constant_charge_current_max_ua);
+				bat_info->constant_charge_current_max_ua);
 	if (ret)
 		return ret;
 
 	ret = bq->chip_info->bq256xx_set_iprechg(bq,
-				bat_info.precharge_current_ua);
+				bat_info->precharge_current_ua);
 	if (ret)
 		return ret;
 
 	ret = bq->chip_info->bq256xx_set_vbatreg(bq,
-				bat_info.constant_charge_voltage_max_uv);
+				bat_info->constant_charge_voltage_max_uv);
 	if (ret)
 		return ret;
 
 	ret = bq->chip_info->bq256xx_set_iterm(bq,
-				bat_info.charge_term_current_ua);
+				bat_info->charge_term_current_ua);
 	if (ret)
 		return ret;
 
-	power_supply_put_battery_info(bq->charger, &bat_info);
+	power_supply_put_battery_info(bq->charger, bat_info);
 
 	return 0;
 }
diff --git a/drivers/power/supply/bq25980_charger.c b/drivers/power/supply/bq25980_charger.c
index 0008c229fd9c..9daa6d14db4d 100644
--- a/drivers/power/supply/bq25980_charger.c
+++ b/drivers/power/supply/bq25980_charger.c
@@ -1079,7 +1079,7 @@ static int bq25980_power_supply_init(struct bq25980_device *bq,
 
 static int bq25980_hw_init(struct bq25980_device *bq)
 {
-	struct power_supply_battery_info bat_info = { };
+	struct power_supply_battery_info *bat_info;
 	int wd_reg_val = BQ25980_WATCHDOG_DIS;
 	int wd_max_val = BQ25980_NUM_WD_VAL - 1;
 	int ret = 0;
@@ -1112,8 +1112,8 @@ static int bq25980_hw_init(struct bq25980_device *bq)
 		return -EINVAL;
 	}
 
-	bq->init_data.ichg_max = bat_info.constant_charge_current_max_ua;
-	bq->init_data.vreg_max = bat_info.constant_charge_voltage_max_uv;
+	bq->init_data.ichg_max = bat_info->constant_charge_current_max_ua;
+	bq->init_data.vreg_max = bat_info->constant_charge_voltage_max_uv;
 
 	if (bq->state.bypass) {
 		ret = regmap_update_bits(bq->regmap, BQ25980_CHRGR_CTRL_2,
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 7e5e24b585d8..72e727cd31e8 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1474,7 +1474,7 @@ static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
 
 static void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
 {
-	struct power_supply_battery_info info = {};
+	struct power_supply_battery_info *info;
 	unsigned int min, max;
 
 	if (power_supply_get_battery_info(di->bat, &info) < 0)
@@ -1485,43 +1485,43 @@ static void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
 		return;
 	}
 
-	if (info.energy_full_design_uwh != info.charge_full_design_uah) {
-		if (info.energy_full_design_uwh == -EINVAL)
+	if (info->energy_full_design_uwh != info->charge_full_design_uah) {
+		if (info->energy_full_design_uwh == -EINVAL)
 			dev_warn(di->dev, "missing battery:energy-full-design-microwatt-hours\n");
-		else if (info.charge_full_design_uah == -EINVAL)
+		else if (info->charge_full_design_uah == -EINVAL)
 			dev_warn(di->dev, "missing battery:charge-full-design-microamp-hours\n");
 	}
 
 	/* assume min == 0 */
 	max = di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max;
-	if (info.energy_full_design_uwh > max * 1000) {
+	if (info->energy_full_design_uwh > max * 1000) {
 		dev_err(di->dev, "invalid battery:energy-full-design-microwatt-hours %d\n",
-			info.energy_full_design_uwh);
-		info.energy_full_design_uwh = -EINVAL;
+			info->energy_full_design_uwh);
+		info->energy_full_design_uwh = -EINVAL;
 	}
 
 	/* assume min == 0 */
 	max = di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max;
-	if (info.charge_full_design_uah > max * 1000) {
+	if (info->charge_full_design_uah > max * 1000) {
 		dev_err(di->dev, "invalid battery:charge-full-design-microamp-hours %d\n",
-			info.charge_full_design_uah);
-		info.charge_full_design_uah = -EINVAL;
+			info->charge_full_design_uah);
+		info->charge_full_design_uah = -EINVAL;
 	}
 
 	min = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min;
 	max = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max;
-	if ((info.voltage_min_design_uv < min * 1000 ||
-	     info.voltage_min_design_uv > max * 1000) &&
-	     info.voltage_min_design_uv != -EINVAL) {
+	if ((info->voltage_min_design_uv < min * 1000 ||
+	     info->voltage_min_design_uv > max * 1000) &&
+	     info->voltage_min_design_uv != -EINVAL) {
 		dev_err(di->dev, "invalid battery:voltage-min-design-microvolt %d\n",
-			info.voltage_min_design_uv);
-		info.voltage_min_design_uv = -EINVAL;
+			info->voltage_min_design_uv);
+		info->voltage_min_design_uv = -EINVAL;
 	}
 
-	if ((info.energy_full_design_uwh != -EINVAL &&
-	     info.charge_full_design_uah != -EINVAL) ||
-	     info.voltage_min_design_uv  != -EINVAL)
-		bq27xxx_battery_set_config(di, &info);
+	if ((info->energy_full_design_uwh != -EINVAL &&
+	     info->charge_full_design_uah != -EINVAL) ||
+	     info->voltage_min_design_uv  != -EINVAL)
+		bq27xxx_battery_set_config(di, info);
 }
 
 /*
diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c
index 091868e9e9e8..0c87ad0dbf71 100644
--- a/drivers/power/supply/cw2015_battery.c
+++ b/drivers/power/supply/cw2015_battery.c
@@ -61,7 +61,7 @@ struct cw_battery {
 	struct delayed_work battery_delay_work;
 	struct regmap *regmap;
 	struct power_supply *rk_bat;
-	struct power_supply_battery_info battery;
+	struct power_supply_battery_info *battery;
 	u8 *bat_profile;
 
 	bool charger_attached;
@@ -505,22 +505,22 @@ static int cw_battery_get_property(struct power_supply *psy,
 
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
-		if (cw_bat->battery.charge_full_design_uah > 0)
-			val->intval = cw_bat->battery.charge_full_design_uah;
+		if (cw_bat->battery->charge_full_design_uah > 0)
+			val->intval = cw_bat->battery->charge_full_design_uah;
 		else
 			val->intval = 0;
 		break;
 
 	case POWER_SUPPLY_PROP_CHARGE_NOW:
-		val->intval = cw_bat->battery.charge_full_design_uah;
+		val->intval = cw_bat->battery->charge_full_design_uah;
 		val->intval = val->intval * cw_bat->soc / 100;
 		break;
 
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		if (cw_battery_valid_time_to_empty(cw_bat) &&
-		    cw_bat->battery.charge_full_design_uah > 0) {
+		    cw_bat->battery->charge_full_design_uah > 0) {
 			/* calculate remaining capacity */
-			val->intval = cw_bat->battery.charge_full_design_uah;
+			val->intval = cw_bat->battery->charge_full_design_uah;
 			val->intval = val->intval * cw_bat->soc / 100;
 
 			/* estimate current based on time to empty */
@@ -687,6 +687,12 @@ static int cw_bat_probe(struct i2c_client *client)
 
 	ret = power_supply_get_battery_info(cw_bat->rk_bat, &cw_bat->battery);
 	if (ret) {
+		/* Allocate an empty battery */
+		cw_bat->battery = devm_kzalloc(&client->dev,
+					       sizeof(cw_bat->battery),
+					       GFP_KERNEL);
+		if (!cw_bat->battery)
+			return -ENOMEM;
 		dev_warn(cw_bat->dev,
 			 "No monitored battery, some properties will be missing\n");
 	}
@@ -724,7 +730,7 @@ static int cw_bat_remove(struct i2c_client *client)
 	struct cw_battery *cw_bat = i2c_get_clientdata(client);
 
 	cancel_delayed_work_sync(&cw_bat->battery_delay_work);
-	power_supply_put_battery_info(cw_bat->rk_bat, &cw_bat->battery);
+	power_supply_put_battery_info(cw_bat->rk_bat, cw_bat->battery);
 	return 0;
 }
 
diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
index 8b18219ebe90..2e7fdfde47ec 100644
--- a/drivers/power/supply/ingenic-battery.c
+++ b/drivers/power/supply/ingenic-battery.c
@@ -18,7 +18,7 @@ struct ingenic_battery {
 	struct iio_channel *channel;
 	struct power_supply_desc desc;
 	struct power_supply *battery;
-	struct power_supply_battery_info info;
+	struct power_supply_battery_info *info;
 };
 
 static int ingenic_battery_get_property(struct power_supply *psy,
@@ -26,7 +26,7 @@ static int ingenic_battery_get_property(struct power_supply *psy,
 					union power_supply_propval *val)
 {
 	struct ingenic_battery *bat = power_supply_get_drvdata(psy);
-	struct power_supply_battery_info *info = &bat->info;
+	struct power_supply_battery_info *info = bat->info;
 	int ret;
 
 	switch (psp) {
@@ -80,7 +80,7 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat)
 	if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
 		return -EINVAL;
 
-	max_mV = bat->info.voltage_max_design_uv / 1000;
+	max_mV = bat->info->voltage_max_design_uv / 1000;
 
 	for (i = 0; i < scale_len; i += 2) {
 		u64 scale_mV = (max_raw * scale_raw[i]) >> scale_raw[i + 1];
@@ -156,13 +156,13 @@ static int ingenic_battery_probe(struct platform_device *pdev)
 		dev_err(dev, "Unable to get battery info: %d\n", ret);
 		return ret;
 	}
-	if (bat->info.voltage_min_design_uv < 0) {
+	if (bat->info->voltage_min_design_uv < 0) {
 		dev_err(dev, "Unable to get voltage min design\n");
-		return bat->info.voltage_min_design_uv;
+		return bat->info->voltage_min_design_uv;
 	}
-	if (bat->info.voltage_max_design_uv < 0) {
+	if (bat->info->voltage_max_design_uv < 0) {
 		dev_err(dev, "Unable to get voltage max design\n");
-		return bat->info.voltage_max_design_uv;
+		return bat->info->voltage_max_design_uv;
 	}
 
 	return ingenic_battery_set_scale(bat);
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 2723d7d0ced3..879fac182f28 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -564,14 +564,19 @@ EXPORT_SYMBOL_GPL(devm_power_supply_get_by_phandle);
 #endif /* CONFIG_OF */
 
 int power_supply_get_battery_info(struct power_supply *psy,
-				  struct power_supply_battery_info *info)
+				  struct power_supply_battery_info **info_out)
 {
 	struct power_supply_resistance_temp_table *resist_table;
+	struct power_supply_battery_info *info;
 	struct device_node *battery_np;
 	const char *value;
 	int err, len, index;
 	const __be32 *list;
 
+	info = devm_kmalloc(&psy->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
 	info->technology                     = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 	info->energy_full_design_uwh         = -EINVAL;
 	info->charge_full_design_uah         = -EINVAL;
@@ -728,7 +733,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 
 	list = of_get_property(battery_np, "resistance-temp-table", &len);
 	if (!list || !len)
-		goto out_put_node;
+		goto out_ret_pointer;
 
 	info->resist_table_size = len / (2 * sizeof(__be32));
 	resist_table = info->resist_table = devm_kcalloc(&psy->dev,
@@ -746,6 +751,10 @@ int power_supply_get_battery_info(struct power_supply *psy,
 		resist_table[index].resistance = be32_to_cpu(*list++);
 	}
 
+out_ret_pointer:
+	/* Finally return the whole thing */
+	*info_out = info;
+
 out_put_node:
 	of_node_put(battery_np);
 	return err;
@@ -764,6 +773,8 @@ void power_supply_put_battery_info(struct power_supply *psy,
 
 	if (info->resist_table)
 		devm_kfree(&psy->dev, info->resist_table);
+
+	devm_kfree(&psy->dev, info);
 }
 EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
 
diff --git a/drivers/power/supply/sc2731_charger.c b/drivers/power/supply/sc2731_charger.c
index 288b79836c13..9ac17cf7a126 100644
--- a/drivers/power/supply/sc2731_charger.c
+++ b/drivers/power/supply/sc2731_charger.c
@@ -368,7 +368,7 @@ static int sc2731_charger_usb_change(struct notifier_block *nb,
 
 static int sc2731_charger_hw_init(struct sc2731_charger_info *info)
 {
-	struct power_supply_battery_info bat_info = { };
+	struct power_supply_battery_info *bat_info;
 	u32 term_currrent, term_voltage, cur_val, vol_val;
 	int ret;
 
@@ -390,7 +390,7 @@ static int sc2731_charger_hw_init(struct sc2731_charger_info *info)
 		cur_val = 0x2;
 		vol_val = 0x1;
 	} else {
-		term_currrent = bat_info.charge_term_current_ua / 1000;
+		term_currrent = bat_info->charge_term_current_ua / 1000;
 
 		if (term_currrent <= 90)
 			cur_val = 0;
@@ -399,7 +399,7 @@ static int sc2731_charger_hw_init(struct sc2731_charger_info *info)
 		else
 			cur_val = ((term_currrent - 90) / 25) + 1;
 
-		term_voltage = bat_info.constant_charge_voltage_max_uv / 1000;
+		term_voltage = bat_info->constant_charge_voltage_max_uv / 1000;
 
 		if (term_voltage > 4500)
 			term_voltage = 4500;
@@ -409,7 +409,7 @@ static int sc2731_charger_hw_init(struct sc2731_charger_info *info)
 		else
 			vol_val = 0;
 
-		power_supply_put_battery_info(info->psy_usb, &bat_info);
+		power_supply_put_battery_info(info->psy_usb, bat_info);
 	}
 
 	/* Set charge termination current */
diff --git a/drivers/power/supply/sc27xx_fuel_gauge.c b/drivers/power/supply/sc27xx_fuel_gauge.c
index ae45069bd5e1..632977f84b95 100644
--- a/drivers/power/supply/sc27xx_fuel_gauge.c
+++ b/drivers/power/supply/sc27xx_fuel_gauge.c
@@ -998,7 +998,7 @@ static int sc27xx_fgu_calibration(struct sc27xx_fgu_data *data)
 
 static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
 {
-	struct power_supply_battery_info info = { };
+	struct power_supply_battery_info *info;
 	struct power_supply_battery_ocv_table *table;
 	int ret, delta_clbcnt, alarm_adc;
 
@@ -1008,16 +1008,16 @@ static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
 		return ret;
 	}
 
-	data->total_cap = info.charge_full_design_uah / 1000;
-	data->max_volt = info.constant_charge_voltage_max_uv / 1000;
-	data->internal_resist = info.factory_internal_resistance_uohm / 1000;
-	data->min_volt = info.voltage_min_design_uv;
+	data->total_cap = info->charge_full_design_uah / 1000;
+	data->max_volt = info->constant_charge_voltage_max_uv / 1000;
+	data->internal_resist = info->factory_internal_resistance_uohm / 1000;
+	data->min_volt = info->voltage_min_design_uv;
 
 	/*
 	 * For SC27XX fuel gauge device, we only use one ocv-capacity
 	 * table in normal temperature 20 Celsius.
 	 */
-	table = power_supply_find_ocv2cap_table(&info, 20, &data->table_len);
+	table = power_supply_find_ocv2cap_table(info, 20, &data->table_len);
 	if (!table)
 		return -EINVAL;
 
@@ -1025,7 +1025,7 @@ static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
 				       data->table_len * sizeof(*table),
 				       GFP_KERNEL);
 	if (!data->cap_table) {
-		power_supply_put_battery_info(data->battery, &info);
+		power_supply_put_battery_info(data->battery, info);
 		return -ENOMEM;
 	}
 
@@ -1035,19 +1035,19 @@ static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
 	if (!data->alarm_cap)
 		data->alarm_cap += 1;
 
-	data->resist_table_len = info.resist_table_size;
+	data->resist_table_len = info->resist_table_size;
 	if (data->resist_table_len > 0) {
-		data->resist_table = devm_kmemdup(data->dev, info.resist_table,
+		data->resist_table = devm_kmemdup(data->dev, info->resist_table,
 						  data->resist_table_len *
 						  sizeof(struct power_supply_resistance_temp_table),
 						  GFP_KERNEL);
 		if (!data->resist_table) {
-			power_supply_put_battery_info(data->battery, &info);
+			power_supply_put_battery_info(data->battery, info);
 			return -ENOMEM;
 		}
 	}
 
-	power_supply_put_battery_info(data->battery, &info);
+	power_supply_put_battery_info(data->battery, info);
 
 	ret = sc27xx_fgu_calibration(data);
 	if (ret)
diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c
index 753944e774c4..d56e469043bb 100644
--- a/drivers/power/supply/smb347-charger.c
+++ b/drivers/power/supply/smb347-charger.c
@@ -1281,7 +1281,7 @@ static void smb347_dt_parse_dev_info(struct smb347_charger *smb)
 
 static int smb347_get_battery_info(struct smb347_charger *smb)
 {
-	struct power_supply_battery_info info = {};
+	struct power_supply_battery_info *info;
 	struct power_supply *supply;
 	int err;
 
@@ -1296,29 +1296,29 @@ static int smb347_get_battery_info(struct smb347_charger *smb)
 	if (err)
 		return err;
 
-	if (info.constant_charge_current_max_ua != -EINVAL)
-		smb->max_charge_current = info.constant_charge_current_max_ua;
+	if (info->constant_charge_current_max_ua != -EINVAL)
+		smb->max_charge_current = info->constant_charge_current_max_ua;
 
-	if (info.constant_charge_voltage_max_uv != -EINVAL)
-		smb->max_charge_voltage = info.constant_charge_voltage_max_uv;
+	if (info->constant_charge_voltage_max_uv != -EINVAL)
+		smb->max_charge_voltage = info->constant_charge_voltage_max_uv;
 
-	if (info.precharge_current_ua != -EINVAL)
-		smb->pre_charge_current = info.precharge_current_ua;
+	if (info->precharge_current_ua != -EINVAL)
+		smb->pre_charge_current = info->precharge_current_ua;
 
-	if (info.charge_term_current_ua != -EINVAL)
-		smb->termination_current = info.charge_term_current_ua;
+	if (info->charge_term_current_ua != -EINVAL)
+		smb->termination_current = info->charge_term_current_ua;
 
-	if (info.temp_alert_min != INT_MIN)
-		smb->soft_cold_temp_limit = info.temp_alert_min;
+	if (info->temp_alert_min != INT_MIN)
+		smb->soft_cold_temp_limit = info->temp_alert_min;
 
-	if (info.temp_alert_max != INT_MAX)
-		smb->soft_hot_temp_limit = info.temp_alert_max;
+	if (info->temp_alert_max != INT_MAX)
+		smb->soft_hot_temp_limit = info->temp_alert_max;
 
-	if (info.temp_min != INT_MIN)
-		smb->hard_cold_temp_limit = info.temp_min;
+	if (info->temp_min != INT_MIN)
+		smb->hard_cold_temp_limit = info->temp_min;
 
-	if (info.temp_max != INT_MAX)
-		smb->hard_hot_temp_limit = info.temp_max;
+	if (info->temp_max != INT_MAX)
+		smb->hard_hot_temp_limit = info->temp_max;
 
 	/* Suspend when battery temperature is outside hard limits */
 	if (smb->hard_cold_temp_limit != SMB3XX_TEMP_USE_DEFAULT ||
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index b5079109ac00..f8e318440e26 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -574,7 +574,7 @@ devm_power_supply_get_by_phandle(struct device *dev, const char *property)
 #endif /* CONFIG_OF */
 
 extern int power_supply_get_battery_info(struct power_supply *psy,
-					 struct power_supply_battery_info *info);
+					 struct power_supply_battery_info **info_out);
 extern void power_supply_put_battery_info(struct power_supply *psy,
 					  struct power_supply_battery_info *info);
 extern int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-06  0:06 [PATCH v2] power: supply_core: Pass pointer to battery info Linus Walleij
@ 2021-12-08  6:46 ` Matti Vaittinen
  2021-12-09  0:46   ` Linus Walleij
  0 siblings, 1 reply; 9+ messages in thread
From: Matti Vaittinen @ 2021-12-08  6:46 UTC (permalink / raw)
  To: Linus Walleij, Sebastian Reichel; +Cc: linux-pm

Good Morning Linus,

Hmm.. LKML skipped on purpose(?)

On 12/6/21 02:06, Linus Walleij wrote:
> The function to retrieve battery info (from the device tree) assumes
> we have a static info struct that gets populated by calling into
> power_supply_get_battery_info().

I kind of see the value of having the static info structure. It is damn 
easy to use and works well for small items. IMNAAHAWBA-O (In My Not 
Always As Humble As Would Be Appropriate - Opinion) the mistake has been 
adding the large, fixed size arrays in the same struct.

> This is awkward since I want to support tables of static battery
> info by just assigning a pointer to all info based on e.g. a
> compatible value in the device tree.

Do you have a case where you have battery data in multiple DT nodes? 
Some kind of multi-battery use-case? I'd like to understand how you plan 
to do mapping to compatibles - AFAIR, at the moment the compatible must 
be "simple-battery". Do you plan to have multiple compatibles in the DT, 
one of them matching the "simple-battery", rest being used as a key? Or 
do you plan to use the charger compatible (charger which references the 
battery with the monitored-battery as a key? My initial feeling is that 
it kind of makes sense).

I was wondering how it would work out if the battery info was splitted 
to smaller (optional) pieces instead of being just one big struct? It 
kind of makes no sense to always reserve space for all of the 
calibration data arrays when some of them are likely to be missing...(?)

something like:

struct static_batinfo static_info;
struct dynamic_batinfo *dynamic_info;

power_supply_get_battery_info(bd->charger, &static_info, &dynamic_info)

dynamic_info can be NULL if it is not expected.
dynamic_info will be NULL'ed by call if it is not populated
dynamic_info will be allocated if it is not NULL when called and if 
dynamic data is found from the firmware.
The dynamic data must be freed by (keep put batttery info API or just 
allow user to kfree?)

Or is this just adding a lot of extra complexity in order to save few 
bytes? I am not pushing this - it's up to you guys. I am "just a random 
guy from the streets" as someone once put it ;) For me it is just 
troubling to always have all the arrays in battery data - and I still do 
like the ability to avoid dynamic allocation when we don't have much of 
the info in the DT.

> 
> We also have a mixture of static and dynamically allocated
> variables here.
> 
> Bite the bullet and let power_supply_get_battery_info() allocate
> also the memory used for the very top level
> struct power_supply_battery_info container. Pass pointers
> around and lifecycle this with the psy device just like the
> stuff we allocate inside it.
> 
> Change all current users over.
> 
> In the bd99954 charger driver we need to stop issuing
> power_supply_put_battery_info() at the end of the probe since
> the struct continues to be used after probe().

Hm. Are you sure?
The patch didn't apply on v5.16-rc4 so I may have missed something as I 
was just reading the diff. AFAIR, the original idea was that the batery 
info lifetime is only the fw_probe - where the value from battery info 
was used to get the initialization register values which are then stored 
to init data. The init data was allocated at probe.

	/* Allocated at probe */
	struct bd9995x_init_data *init = &bd->init_data;
	struct battery_init battery_inits[] = {
	{
		.name = "trickle-charging current",
		.info_data = &info.tricklecharge_current_ua,
		.range = &charging_current_ranges[0],
		.ranges = 2,

		/* Pointer to allocated init data */
		.data = &init->itrich_set,
	}, {
		...


	ret = power_supply_get_battery_info(bd->charger, &info);
	...

	/* Use info to get correct register value */
	...
	
	/* Store info to allocated init data */
	*(battery_inits[i].data) = regval;

Maybe I am missing something?


Best Regards
	-- Matti


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-08  6:46 ` Matti Vaittinen
@ 2021-12-09  0:46   ` Linus Walleij
  2021-12-10  5:57     ` Matti Vaittinen
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2021-12-09  0:46 UTC (permalink / raw)
  To: Matti Vaittinen; +Cc: Sebastian Reichel, linux-pm

On Wed, Dec 8, 2021 at 7:46 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hmm.. LKML skipped on purpose(?)

Yeah, MAINTAINERS says this:

POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
M:      Sebastian Reichel <sre@kernel.org>
L:      linux-pm@vger.kernel.org

LKML is only the bin where we put everything that doesn't have a proper
maintenance mailing list.

> On 12/6/21 02:06, Linus Walleij wrote:

> > This is awkward since I want to support tables of static battery
> > info by just assigning a pointer to all info based on e.g. a
> > compatible value in the device tree.
>
> Do you have a case where you have battery data in multiple DT nodes?
> Some kind of multi-battery use-case?

No it is a single battery. But the data is kept in the kernel and matched
per-battery, I pushed the latest code from my desk so you can see how
it looks in practice:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/commit/?h=ux500-href-charging-v5.16-rc1

> I'd like to understand how you plan
> to do mapping to compatibles - AFAIR, at the moment the compatible must
> be "simple-battery".

This is for the Samsung batteries using these bindings:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/supply/samsung,battery.yaml

> Do you plan to have multiple compatibles in the DT,
> one of them matching the "simple-battery", rest being used as a key? Or
> do you plan to use the charger compatible (charger which references the
> battery with the monitored-battery as a key? My initial feeling is that
> it kind of makes sense).

The Samsung battery bindings are already in use, I used them for
example in these:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ste-ux500-samsung-janice.dts
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ste-ab8500.dtsi

It's very simplistic:

battery: battery {
    compatible = "samsung,eb535151vu";
};
(...)

ab8500 {
   compatible = "stericsson,ab8500";
(...)
  ab8500_charger {
     compatible = "stericsson,ab8500-charger";
(...)
       monitored-battery = <&battery>;

> I was wondering how it would work out if the battery info was splitted
> to smaller (optional) pieces instead of being just one big struct? It
> kind of makes no sense to always reserve space for all of the
> calibration data arrays when some of them are likely to be missing...(?)

I suppose if footprint becomes a problem one can just split the
battery data into smaller files, and compile each one with a
KConfig.

As you can see in my example the Samsung batteries actually
share the same capacity table across three different batteries
so they actually benefit from being defined together when we
want to support all (which we do).

> something like:
>
> struct static_batinfo static_info;
> struct dynamic_batinfo *dynamic_info;
>
> power_supply_get_battery_info(bd->charger, &static_info, &dynamic_info)
>
> dynamic_info can be NULL if it is not expected.
> dynamic_info will be NULL'ed by call if it is not populated
> dynamic_info will be allocated if it is not NULL when called and if
> dynamic data is found from the firmware.

But we already (partly) do that, look in power_supply_get_battery_info()
as touched by this patch:

(...)
        list = of_get_property(battery_np, "resistance-temp-table", &len);
        if (!list || !len)
                goto out_ret_pointer;

        info->resist_table_size = len / (2 * sizeof(__be32));
        resist_table = info->resist_table = devm_kcalloc(&psy->dev,

info->resist_table_size,
                                                         sizeof(*resist_table),
                                                         GFP_KERNEL);

Notice the goto out_ret_pointer; we return the pointer without
allocating any resistance-temp-table if it is not present in the
device tree. The same can be done with static data and
consumers need to cope with some members being NULL.

I don't see the usecase for mixing static and dynamic info
really, I think we have two cases:

- All information about the battery obtained from one single
  key such as the compatible string (as for the Samsung
  SDI batteries)

- All information about the battery contained in the device
  tree (as for simple-battery)

The way discussion has been going in the device tree
community the "define it all in devicetree" approach is
kind of soft discouraged.

The simple-battery IMO should be for
things like prototypes or devices where you don't really
know which battery is connected so you will have some
locally modified device trees for them.

In the Samsung case, these phones are mechanically
designed to just fit with one battery, and this seems to
be what Samsung does consistently: a battery with one
physical characteristic should not fit physically in the
slot of a device with different requirements.

That said I see what you're doing with the init data in
the bd99954 charger driver and it is similar to what the
regulator subsystem does too.

> The dynamic data must be freed by (keep put batttery info API or just
> allow user to kfree?)

The way the drivers use it is that some just call
power_supply_get_battery_info() copy some values from the
struct power_supply_battery_info into its own structs and then
just call power_supply_put_battery_info() and get rid of it all
again, and then the use is zero. (bd99954 for example)

Other drivers keep it around and reference it but it is
lifecycled with devres to the psy so it goes away when
removing the module (etc).

Either way works I guess, I just have a habit of using the
kernel structs as-is.

> Or is this just adding a lot of extra complexity in order to save few
> bytes?

On a general note the footprint discussion is kind of dead because
the community does not really prioritize that. Nicolas Pitre
invested lots of time to make the kernel configurable and
strippable with the goal of using it for IoT targets in 2018, and there
just wasn't interest. But anyone can drive such change of course.

The power supply core isn't really the worst offender though.

> For me it is just
> troubling to always have all the arrays in battery data - and I still do
> like the ability to avoid dynamic allocation when we don't have much of
> the info in the DT.

I will probably need to add more entries to it as well and then I expect
to make them optional not compulsory, like what we do for resistance
already.

> > In the bd99954 charger driver we need to stop issuing
> > power_supply_put_battery_info() at the end of the probe since
> > the struct continues to be used after probe().
>
> Hm. Are you sure?

This driver was a challenge so no :) it uses the info
differently from all others.

I looked over your comments and I missed how the init
data is used and then discarded. I will respin the patch
and fix this.

Also I would love if you could test v2 on hardware!

> The patch didn't apply on v5.16-rc4

It's based on Sebastians tree so you need to use that, or just
grab my branch (should be possible to merge in v5.16-rc4
if you need)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-09  0:46   ` Linus Walleij
@ 2021-12-10  5:57     ` Matti Vaittinen
  2021-12-13  9:23       ` Matti Vaittinen
  2021-12-13  9:23       ` Matti Vaittinen
  0 siblings, 2 replies; 9+ messages in thread
From: Matti Vaittinen @ 2021-12-10  5:57 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Linux PM list

Hi deee Ho peeps!

Hmm.. I see I accidentally sent this from my gmail address. I guess
all tags (like RBT) should be sent from the address which is used in
the tag? (I've two accounts in the same client and occasionally send
from the wrong one...)

On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Dec 8, 2021 at 7:46 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> > Hmm.. LKML skipped on purpose(?)
>
> LKML is only the bin where we put everything that doesn't have a proper
> maintenance mailing list.

Oh. I was under the impression that the LKML is collecting more or
less all the messages. I am pretty sure the get_maintainers.pl does
always add the LKML too. (I almost never look the MAINTAINERS directly
but rely on the get_maintainer.pl to do the _almost_ right thing). I
should probably revise my list subscriptions then to ensure my filters
won't miss interesting patches. Good to know this.

> > On 12/6/21 02:06, Linus Walleij wrote:
>
> > > This is awkward since I want to support tables of static battery
> > > info by just assigning a pointer to all info based on e.g. a
> > > compatible value in the device tree.
> >
> > Do you have a case where you have battery data in multiple DT nodes?
> > Some kind of multi-battery use-case?
>
> No it is a single battery. But the data is kept in the kernel and matched
> per-battery, I pushed the latest code from my desk so you can see how
> it looks in practice:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/commit/?h=ux500-href-charging-v5.16-rc1

1. Thanks.

> > I'd like to understand how you plan
> > to do mapping to compatibles - AFAIR, at the moment the compatible must
> > be "simple-battery".
>
> This is for the Samsung batteries using these bindings:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/supply/samsung,battery.yaml

2. Thanks.

>
> > Do you plan to have multiple compatibles in the DT,
> > one of them matching the "simple-battery", rest being used as a key? Or
> > do you plan to use the charger compatible (charger which references the
> > battery with the monitored-battery as a key? My initial feeling is that
> > it kind of makes sense).
>
> The Samsung battery bindings are already in use, I used them for
> example in these:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ste-ux500-samsung-janice.dts
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ste-ab8500.dtsi
>
> It's very simplistic:
>
> battery: battery {
>     compatible = "samsung,eb535151vu";
> };
> (...)
>
> ab8500 {
>    compatible = "stericsson,ab8500";
> (...)
>   ab8500_charger {
>      compatible = "stericsson,ab8500-charger";
> (...)
>        monitored-battery = <&battery>;

3. Thanks.

>
> > I was wondering how it would work out if the battery info was splitted
> > to smaller (optional) pieces instead of being just one big struct? It
> > kind of makes no sense to always reserve space for all of the
> > calibration data arrays when some of them are likely to be missing...(?)
>
> I suppose if footprint becomes a problem one can just split the
> battery data into smaller files, and compile each one with a
> KConfig.
>
> As you can see in my example the Samsung batteries actually
> share the same capacity table across three different batteries
> so they actually benefit from being defined together when we
> want to support all (which we do).

Thanks 4.
I think I am about to learn something. I didn't think we could use
KConfig to flag-out some of the battery-data - or to give some
battery-data as "configured-in" values.

> > something like:
> >
> > struct static_batinfo static_info;
> > struct dynamic_batinfo *dynamic_info;
> >
> > power_supply_get_battery_info(bd->charger, &static_info, &dynamic_info)
> >
> > dynamic_info can be NULL if it is not expected.
> > dynamic_info will be NULL'ed by call if it is not populated
> > dynamic_info will be allocated if it is not NULL when called and if
> > dynamic data is found from the firmware.
>
> But we already (partly) do that, look in power_supply_get_battery_info()
> as touched by this patch:
>
> (...)
>         list = of_get_property(battery_np, "resistance-temp-table", &len);
>         if (!list || !len)
>                 goto out_ret_pointer;
>
>         info->resist_table_size = len / (2 * sizeof(__be32));
>         resist_table = info->resist_table = devm_kcalloc(&psy->dev,
>
> info->resist_table_size,
>                                                          sizeof(*resist_table),
>                                                          GFP_KERNEL);
>
> Notice the goto out_ret_pointer; we return the pointer without
> allocating any resistance-temp-table if it is not present in the
> device tree. The same can be done with static data and
> consumers need to cope with some members being NULL.

Please, don't get me wrong. I do think your patch improves the
situation in a sense. It becomes more obvious one needs to use the
put_battery_info() as battery data is now clearly allocated. I just
wondered if we could still handle batteries which do not have
data-arrays w/o the allocation/freeing. Well, I tend to agree with you
here now so please just ignore my babbling regarding the allocation.

> I don't see the usecase for mixing static and dynamic info
> really, I think we have two cases:
>
> - All information about the battery obtained from one single
>   key such as the compatible string (as for the Samsung
>   SDI batteries)

> - All information about the battery contained in the device
>   tree (as for simple-battery)
>
> The way discussion has been going in the device tree
> community the "define it all in devicetree" approach is
> kind of soft discouraged.

Thanks! This is likely to teach me a thing or two! I feel I owe you a
beer should we ever met :]

> > For me it is just
> > troubling to always have all the arrays in battery data - and I still do
> > like the ability to avoid dynamic allocation when we don't have much of
> > the info in the DT.
>
> I will probably need to add more entries to it as well and then I expect
> to make them optional not compulsory, like what we do for resistance
> already.

Ok.

> Also I would love if you could test v2 on hardware!

I don't have a real device with battery connected - but I thinkI do
have a break-out board with BD99954 lying around somewhere. I'll see
if I remember how to wire it to a beagle-bone - and if I do, then I
can try some very limited testing. Adding automated tests for the
BD99954 is still on my TODO-list, somewhere at the bottom of it... :(

>
> > The patch didn't apply on v5.16-rc4
>
> It's based on Sebastians tree so you need to use that, or just
> grab my branch (should be possible to merge in v5.16-rc4
> if you need)

Yeah, one more remotes to my work-area git won't change a thing ^_^;
It's already a horrible mess :rolleyes:

>
> Yours,
> Linus Walleij



-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Matti "Maz" Vaittinen

When you feel blue, no one sees your tears...
When your down, no one understands your struggle...
When you feel happy, no one notices your smile...
But fart just once...
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-10  5:57     ` Matti Vaittinen
@ 2021-12-13  9:23       ` Matti Vaittinen
  2021-12-13  9:23       ` Matti Vaittinen
  1 sibling, 0 replies; 9+ messages in thread
From: Matti Vaittinen @ 2021-12-13  9:23 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Linux PM list

On 12/10/21 07:57, Matti Vaittinen wrote:
> On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij <linus.walleij@linaro.org> wrote:

>> Also I would love if you could test v2 on hardware!
> 
> I don't have a real device with battery connected - but I thinkI do
> have a break-out board with BD99954 lying around somewhere. I'll see
> if I remember how to wire it to a beagle-bone - and if I do, then I
> can try some very limited testing. Adding automated tests for the
> BD99954 is still on my TODO-list, somewhere at the bottom of it... :(

I have good and not-so-good news. Good news is that I did indeed find a 
BD99954 evaluation board from my collection PCBs I have "temporarily" 
stored in a box under my bed XD

The not-so-good news is that I thought I do remember how to connect the 
board - but it seems I did not. As a result my beaglebone said "Zzzzap!" 
- and lost the magic smoke which kept it operational. I have only one 
spare board left - and I do noeed it for bunch of tasks - so I am afraid 
I can't re-try testing untill I have ordered and received few 
replacement boards... So sorry - no testing to be done in the next few 
days - potentially no testing until year 2022 :(

Best Regards
	--Matti Vaittinen

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-10  5:57     ` Matti Vaittinen
  2021-12-13  9:23       ` Matti Vaittinen
@ 2021-12-13  9:23       ` Matti Vaittinen
  2021-12-14  6:44         ` Matti Vaittinen
  1 sibling, 1 reply; 9+ messages in thread
From: Matti Vaittinen @ 2021-12-13  9:23 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Linux PM list

On 12/10/21 07:57, Matti Vaittinen wrote:
> On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij <linus.walleij@linaro.org> wrote:

>> Also I would love if you could test v2 on hardware!
> 
> I don't have a real device with battery connected - but I thinkI do
> have a break-out board with BD99954 lying around somewhere. I'll see
> if I remember how to wire it to a beagle-bone - and if I do, then I
> can try some very limited testing. Adding automated tests for the
> BD99954 is still on my TODO-list, somewhere at the bottom of it... :(

I have good and not-so-good news. Good news is that I did indeed find a 
BD99954 evaluation board from my collection PCBs I have "temporarily" 
stored in a box under my bed XD

The not-so-good news is that I thought I do remember how to connect the 
board - but it seems I did not. As a result my beaglebone said "Zzzzap!" 
- and lost the magic smoke which kept it operational. I have only one 
spare board left - and I do noeed it for bunch of tasks - so I am afraid 
I can't re-try testing untill I have ordered and received few 
replacement boards... So sorry - no testing to be done in the next few 
days - potentially no testing until year 2022 :(

Best Regards
	--Matti Vaittinen

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-13  9:23       ` Matti Vaittinen
@ 2021-12-14  6:44         ` Matti Vaittinen
  2021-12-14  6:53           ` Vaittinen, Matti
  0 siblings, 1 reply; 9+ messages in thread
From: Matti Vaittinen @ 2021-12-14  6:44 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Linux PM list

Hi deee Ho peeps,

On 12/13/21 11:23, Matti Vaittinen wrote:
> On 12/10/21 07:57, Matti Vaittinen wrote:
>> On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij 
>> <linus.walleij@linaro.org> wrote:
> 
>>> Also I would love if you could test v2 on hardware!

... snip ...

> spare board left - and I do noeed it for bunch of tasks - so I am afraid 
> I can't re-try testing untill I have ordered and received few 
> replacement boards...

It seems I like to live on the edge. I nevertheless connected my spare 
BBB board to the BD99954 and got it working. (I noticed I had two loose 
wires touching each others during my previous trial. It resulted 
shorting +5V from beagle to GND - which was not a good idea).

It appears the patch worked as expected - but it also appears the 
BD99954 driver does not handle missing info too well... I typoed the 
trickle-charger current property in DT - and as a result the driver 
decided -EINVAL to be valid value (just too large) and set the largest 
current BD99954 supports for trickle-charging... I think this same 
problem is there for all the -EINVAL values. If I am not mistaken this 
is somewhat nasty as it means that if some information is missing the 
driver will change (potentially pre-configured) values to something 
which may fry things...

Linus, want to fix this while at it - or do you prefer me to patch the 
BD99954 with some sanity checks? I think it'd be nice to get the fixes 
in stable so it might be best to add the sanity checks before changing 
the battery-info allocation - that might be nicer for the stable folks. 
(I guess you have plenty of other things to code + some IRL tasks as 
well ...;] So, I can patch this but it means there is likely to be some 
conflicts with your series. Hence I thought I'll ask if you wish to add 
checks for uninitialized battery-info values)

Best Regards
	--Matti Vaittinen

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-14  6:44         ` Matti Vaittinen
@ 2021-12-14  6:53           ` Vaittinen, Matti
  2021-12-14  7:49             ` Vaittinen, Matti
  0 siblings, 1 reply; 9+ messages in thread
From: Vaittinen, Matti @ 2021-12-14  6:53 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Linux PM list

On 12/14/21 08:44, Matti Vaittinen wrote:
> Hi deee Ho peeps,
> 
> On 12/13/21 11:23, Matti Vaittinen wrote:
>> On 12/10/21 07:57, Matti Vaittinen wrote:
>>> On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij 
>>> <linus.walleij@linaro.org> wrote:
>>
>>>> Also I would love if you could test v2 on hardware!
> 

> It appears the patch worked as expected - but it also appears the 
> BD99954 driver does not handle missing info too well... I typoed the 
> trickle-charger current property in DT - and as a result the driver 
> decided -EINVAL to be valid value (just too large) and set the largest 
> current BD99954 supports for trickle-charging... 

I should have looked this more carefully. It appears the BD99954 does 
check for the -EINVAL - but the power_supply_core does not initialize 
the tricklecharge_current_ua to -EINVAL.

> Linus, want to fix this while at it - or do you prefer me to patch the 
> BD99954 with some sanity checks? I think it'd be nice to get the fixes 
> in stable so it might be best to add the sanity checks before changing 
> the battery-info allocation - that might be nicer for the stable folks. 
> (I guess you have plenty of other things to code + some IRL tasks as 
> well ...;] So, I can patch this but it means there is likely to be some 
> conflicts with your series. Hence I thought I'll ask if you wish to add 
> checks for uninitialized battery-info values)

I think this is a trivial thing to fix and won't be too hard a conflict 
to resolve :) So I'll just send a patch

Sorry for the hassle.

So, what it's worth:
Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Tested-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] power: supply_core: Pass pointer to battery info
  2021-12-14  6:53           ` Vaittinen, Matti
@ 2021-12-14  7:49             ` Vaittinen, Matti
  0 siblings, 0 replies; 9+ messages in thread
From: Vaittinen, Matti @ 2021-12-14  7:49 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Linux PM list

On 12/14/21 08:53, Matti Vaittinen wrote:
> On 12/14/21 08:44, Matti Vaittinen wrote:
>> Hi deee Ho peeps,
>>
>> On 12/13/21 11:23, Matti Vaittinen wrote:
>>> On 12/10/21 07:57, Matti Vaittinen wrote:
>>>> On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij 
>>>> <linus.walleij@linaro.org> wrote:
>>>
>>>>> Also I would love if you could test v2 on hardware!
>>
> 
>> It appears the patch worked as expected - but it also appears the 
>> BD99954 driver does not handle missing info too well... I typoed the 
>> trickle-charger current property in DT - and as a result the driver 
>> decided -EINVAL to be valid value (just too large) and set the largest 
>> current BD99954 supports for trickle-charging... 
> 
> I should have looked this more carefully. It appears the BD99954 does 
> check for the -EINVAL - but the power_supply_core does not initialize 
> the tricklecharge_current_ua to -EINVAL.
> 
>> Linus, want to fix this while at it - or do you prefer me to patch the 
>> BD99954 with some sanity checks? I think it'd be nice to get the fixes 
>> in stable so it might be best to add the sanity checks before changing 
>> the battery-info allocation - that might be nicer for the stable 
>> folks. (I guess you have plenty of other things to code + some IRL 
>> tasks as well ...;] So, I can patch this but it means there is likely 
>> to be some conflicts with your series. Hence I thought I'll ask if you 
>> wish to add checks for uninitialized battery-info values)
> 
> I think this is a trivial thing to fix and won't be too hard a conflict 
> to resolve :) So I'll just send a patch
> 
> Sorry for the hassle.
> 
> So, what it's worth:
> Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Tested-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> 

May I withdraw all of my statements regarding this?

I did suddenly recall why I did not initialize any of the battery-info 
fields I added back when I developed BD99954 driver. Before this change, 
the content and initialization of battery-info fields (which I added) 
was in control of the caller. It made sense to _not_ override values 
caller might have added to battery-info buffer if there was no better 
information in device-tree available. For example, the BD99954 driver 
did initialize the information to zero - which is in BD99954 case is the 
safest option as it yields lowest voltages/current limits. It did work 
untill this change - which now requires the core to initialize all the 
fields.

So, as I see it (at this particular moment and hopefully also 10 minutes 
from now :] ) - initialization was _not_ needed prior this change from 
Linus. It however is required after this change - so Linus, could you 
please add the:

         info->charge_term_current_ua         = -EINVAL;
         info->constant_charge_current_max_ua = -EINVAL;
         info->constant_charge_voltage_max_uv = -EINVAL;
+       info->tricklecharge_current_ua       = -EINVAL;
+       info->precharge_voltage_max_uv       = -EINVAL;
+       info->charge_restart_voltage_uv      = -EINVAL;
+       info->overvoltage_limit_uv           = -EINVAL;
         info->temp_ambient_alert_min         = INT_MIN;
         info->temp_ambient_alert_max         = INT_MAX;
         info->temp_alert_min                 = INT_MIN;

After this my tested-by and reviewed-by can stay there :)

Best Regards
	-- Matti Vaittinen

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-12-14  7:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-06  0:06 [PATCH v2] power: supply_core: Pass pointer to battery info Linus Walleij
2021-12-08  6:46 ` Matti Vaittinen
2021-12-09  0:46   ` Linus Walleij
2021-12-10  5:57     ` Matti Vaittinen
2021-12-13  9:23       ` Matti Vaittinen
2021-12-13  9:23       ` Matti Vaittinen
2021-12-14  6:44         ` Matti Vaittinen
2021-12-14  6:53           ` Vaittinen, Matti
2021-12-14  7:49             ` Vaittinen, Matti

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.