[PATCH 0/3] Add support for Battery Status AMS

[PATCH 0/3] Add support for Battery Status AMS

[Amit Sunil Dhamne via B4 Relay]

PD 3.1 v1.8 Spec necessitates a response to Get_Battery_Status request
from the port partner (see "6.13.2 Applicability of Data Message").
This patchset adds support to get all the battery type power supplies
and query them to report the telemetry required to build a Battery
Status Message. Right now, this submission assumes all the battery type
power supplies that exist in the system are fixed (meaning cannot be hot
swapped).

Previously, I had sent a patch series [1]. However there were some
concerns. Broadly:
  * No client drivers
  * Duplicating dt properties
To address the above issues, we now have Fuel Gauge and Charger drivers.
Also, I have rectified my approach to fetch information about batteries
from the power supply core.

While, the original patch series [1] added support for Battery Caps as
well, this patch series only adds support for Battery Status. Therefore,
I am sending it as a new series while incorporating relevant feedback.

[1] https://lore.kernel.org/all/20250507-batt_ops-v2-0-8d06130bffe6@google.com/

Patches in series:
[A] "power: supply: core: Add power_supply_get_battery_all()"
[B] "usb: typec: tcpm: Add support for Battery Status response message"
[C] "power: supply: max17042: add handler for energy_now property"

Technical dependency of patches:
[C] is independent and can be applied in any order.
[B] depends on [A] due to `power_supply_get_battery_all` symbol.

Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
Amit Sunil Dhamne (3):
      power: Add power_supply_get_battery_all() to fetch battery psy handles
      usb: typec: tcpm: Add support for Battery Status response message
      power: supply: max17042: add handler for energy_now property

 drivers/power/supply/max17042_battery.c  |  60 ++++++++++---
 drivers/power/supply/power_supply_core.c | 122 +++++++++++++++++++++++++++
 drivers/usb/typec/tcpm/tcpm.c            | 140 ++++++++++++++++++++++++++++++-
 include/linux/power_supply.h             |   9 ++
 include/linux/usb/pd.h                   |  29 +++++++
 5 files changed, 344 insertions(+), 16 deletions(-)
---
base-commit: 57b8e2d666a31fa201432d58f5fe3469a0dd83ba
change-id: 20260501-batt-status-16b6761c0bb1

Best regards,
-- 
Amit Sunil Dhamne <amitsd@google.com>

[PATCH 1/3] power: Add power_supply_get_battery_all() to fetch battery psy handles

[Amit Sunil Dhamne via B4 Relay]

From: Amit Sunil Dhamne <amitsd@google.com>

Add power_supply_get_battery_all() to allow drivers to obtain a list of
registered battery type power supply references in the system.

Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
 drivers/power/supply/power_supply_core.c | 122 +++++++++++++++++++++++++++++++
 include/linux/power_supply.h             |   9 +++
 2 files changed, 131 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index a446d3d086fc..697645426fb1 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -482,6 +482,128 @@ struct power_supply *power_supply_get_by_name(const char *name)
 }
 EXPORT_SYMBOL_GPL(power_supply_get_by_name);
 
+static int __power_supply_get_num_battery(struct power_supply *epsy, void *data)
+{
+	int *count = data;
+
+	if (epsy->desc->type == POWER_SUPPLY_TYPE_BATTERY)
+		(*count)++;
+
+	return 0;
+}
+
+static int power_supply_get_num_battery(struct device *dev)
+{
+	int ret, count = 0;
+
+	ret = power_supply_for_each_psy(&count, __power_supply_get_num_battery);
+
+	dev_dbg(dev, "%s: count: %d ret %d\n", __func__, count, ret);
+
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+struct psy_get_supplies_data {
+	int cnt;
+	int size;
+	struct power_supply **psys;
+};
+
+static int __power_supply_populate_battery_array(struct power_supply *epsy,
+						 void *_data)
+{
+	struct psy_get_supplies_data *data = _data;
+
+	if (epsy->desc->type == POWER_SUPPLY_TYPE_BATTERY) {
+		if (data->size <= data->cnt)
+			return -EOVERFLOW;
+
+		get_device(&epsy->dev);
+		data->psys[data->cnt] = epsy;
+		atomic_inc(&epsy->use_cnt);
+		data->cnt++;
+	}
+
+	return 0;
+}
+
+static int power_supply_populate_battery_array(struct device *dev, int size,
+					       struct power_supply **batteries)
+{
+	int ret, i;
+
+	struct psy_get_supplies_data data = {
+		.cnt = 0,
+		.size = size,
+		.psys = batteries,
+	};
+
+	ret = power_supply_for_each_psy(&data, __power_supply_populate_battery_array);
+
+	dev_dbg(dev, "%s Found %d batteries with array size %d ret %d\n",
+		__func__, data.cnt, data.size, ret);
+
+	if (ret < 0) {
+		for (i = 0; i < data.cnt; i++)
+			power_supply_put(batteries[i]);
+		return ret;
+	}
+
+	return data.cnt;
+}
+
+/**
+ * power_supply_get_battery_all() - Searches for all battery type power supplies
+ *				    and returns their references.
+ * @dev: Pointer to device requesting the power supply refs.
+ * @psys: Pointer to an array of power supply refs that will be filled by this
+ *	  function.
+ *
+ * This function helps drivers get handles to all battery type power supplies.
+ * If acquiring a ref to a power supply results in error, then the search for
+ * battery type power supplies will abort and the acquired power supplies will
+ * be "put".
+ *
+ * Return: Indicates the number of battery type power supplies returned on
+ * success or the negative error code on failure.
+ *
+ * It's the responsibility of the caller to invoke power_supply_put() on the
+ * individual psy refs and free the array returned by this function using kfree().
+ */
+int __must_check power_supply_get_battery_all(struct device *dev,
+					      struct power_supply ***psys)
+{
+	int ret;
+
+	if (!psys)
+		return -EINVAL;
+
+	ret = power_supply_get_num_battery(dev);
+	if (ret < 0)
+		return ret;
+
+	if (!ret) {
+		*psys = NULL;
+		return 0;
+	}
+
+	*psys = kzalloc_objs(**psys, ret);
+	if (!*psys)
+		return -ENOMEM;
+
+	ret = power_supply_populate_battery_array(dev, ret, *psys);
+	if (ret <= 0) {
+		kfree(*psys);
+		*psys = NULL;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(power_supply_get_battery_all);
+
 /**
  * power_supply_put() - Drop reference obtained with power_supply_get_by_name
  * @psy: Reference to put
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 7a5e4c3242a0..2467530a5740 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -806,11 +806,20 @@ extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
 #if IS_ENABLED(CONFIG_POWER_SUPPLY)
 extern struct power_supply *power_supply_get_by_name(const char *name);
+extern int __must_check power_supply_get_battery_all(struct device *dev,
+						     struct power_supply ***psys);
 extern void power_supply_put(struct power_supply *psy);
 #else
 static inline void power_supply_put(struct power_supply *psy) {}
 static inline struct power_supply *power_supply_get_by_name(const char *name)
 { return NULL; }
+static inline int __must_check power_supply_get_battery_all(struct device *dev,
+							    struct power_supply ***psys)
+{
+	if (psys)
+		*psys = NULL;
+	return 0;
+}
 #endif
 extern struct power_supply *power_supply_get_by_reference(struct fwnode_handle *fwnode,
 							  const char *property);

-- 
2.54.0.563.g4f69b47b94-goog

[PATCH 2/3] usb: typec: tcpm: Add support for Battery Status response message

[Amit Sunil Dhamne via B4 Relay]

From: Amit Sunil Dhamne <amitsd@google.com>

Add support for responding to a Get_Battery_Status request with a
Battery_Status message. The port partner shall request the status of a
port's battery by providing an index in the Get_Battery_Status AMS. In
case of failure to identify the battery, the port shall reply with an
appropriate message indicating so.

Support for Battery_Status message is required for sinks that contain
battery as specified in USB PD Rev3.1 v1.8
("Applicability of Data Messages" section).

Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 140 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/usb/pd.h        |  29 +++++++++
 2 files changed, 166 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index dfbb94ddc98a..a1abe0a177ac 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -232,7 +232,8 @@ enum pd_msg_request {
 	PD_MSG_DATA_SINK_CAP,
 	PD_MSG_DATA_SOURCE_CAP,
 	PD_MSG_DATA_REV,
-	PD_MSG_EXT_SINK_CAP_EXT
+	PD_MSG_EXT_SINK_CAP_EXT,
+	PD_MSG_DATA_BATT_STATUS
 };
 
 enum adev_actions {
@@ -389,6 +390,14 @@ struct pd_timings {
 /* Convert microwatt to watt */
 #define UW_TO_W(pow)					((pow) / 1000000)
 
+/*
+ * As per USB PD Spec Rev 3.18 (Sec. 6.5.13.11), the number of fixed batteries
+ * that a port can be queried is restricted to 4.
+ */
+#define MAX_NUM_FIXED_BATT				4
+
+#define BATTERY_PROPERTY_UNKNOWN			0xffff
+
 /*
  * struct pd_identifier - Contains info about PD identifiers
  * @vid: Vendor ID (assigned by USB-IF)
@@ -683,6 +692,9 @@ struct tcpm_port {
 
 	struct pd_identifier pd_ident;
 	struct sink_caps_ext_data sink_caps_ext;
+	struct power_supply *fixed_batt[MAX_NUM_FIXED_BATT];
+	u32 fixed_batt_cnt;
+	u32 batt_request_id;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	struct mutex logbuffer_lock;	/* log buffer access lock */
@@ -1391,6 +1403,34 @@ static int tcpm_pd_send_revision(struct tcpm_port *port)
 	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
 }
 
+static void tcpm_init_fixed_batt(struct tcpm_port *port)
+{
+	struct power_supply **batteries;
+	int ret, i;
+
+	if (port->fixed_batt_cnt > 0)
+		return;
+
+	ret = power_supply_get_battery_all(port->dev, &batteries);
+	if (ret < 0) {
+		tcpm_log(port, "Failed to fetch batteries refs ret=%d", ret);
+		return;
+	}
+
+	for (i = 0; i < ret; i++) {
+		if (!batteries[i])
+			continue;
+
+		if (port->fixed_batt_cnt < MAX_NUM_FIXED_BATT)
+			port->fixed_batt[port->fixed_batt_cnt++] = batteries[i];
+		else
+			power_supply_put(batteries[i]);
+	}
+
+	if (ret)
+		kfree(batteries);
+}
+
 static int tcpm_pd_send_source_caps(struct tcpm_port *port)
 {
 	struct pd_message msg;
@@ -1476,6 +1516,8 @@ static int tcpm_pd_send_sink_cap_ext(struct tcpm_port *port)
 
 	if (!port->self_powered)
 		data->spr_op_pdp = operating_snk_watt;
+	else
+		tcpm_init_fixed_batt(port);
 
 	/*
 	 * SPR Sink Minimum PDP indicates the minimum power required to operate
@@ -1502,6 +1544,7 @@ static int tcpm_pd_send_sink_cap_ext(struct tcpm_port *port)
 	skedb.load_step = data->load_step;
 	skedb.load_char = cpu_to_le16(data->load_char);
 	skedb.compliance = data->compliance;
+	skedb.batt_info = port->fixed_batt_cnt;
 	skedb.modes = data->modes;
 	skedb.spr_min_pdp = data->spr_min_pdp;
 	skedb.spr_op_pdp = data->spr_op_pdp;
@@ -1520,6 +1563,74 @@ static int tcpm_pd_send_sink_cap_ext(struct tcpm_port *port)
 					   port->message_id,
 					   data_obj_cnt,
 					   1 /* Denotes if ext header */));
+
+	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
+}
+
+static int tcpm_pd_send_batt_status(struct tcpm_port *port)
+{
+	u16 present_charge = BATTERY_PROPERTY_UNKNOWN;
+	bool batt_present = false, invalid_ref = true;
+	u32 batt_id = port->batt_request_id;
+	union power_supply_propval val;
+	struct power_supply *batt;
+	u8 charging_status = 0;
+	struct pd_message msg;
+	u32 bsdo;
+	int ret;
+
+	memset(&msg, 0, sizeof(msg));
+
+	if (batt_id >= port->fixed_batt_cnt || !port->fixed_batt[batt_id])
+		goto send_status;
+
+	invalid_ref = false;
+	batt = port->fixed_batt[batt_id];
+	ret = power_supply_get_property(batt, POWER_SUPPLY_PROP_PRESENT, &val);
+	if (ret)
+		tcpm_log(port,
+			 "Failed to fetch power_supply_prop_present ret %d",
+			 ret);
+	else
+		batt_present = val.intval > 0;
+
+	ret = power_supply_get_property(batt, POWER_SUPPLY_PROP_ENERGY_NOW,
+					&val);
+
+	/* Battery Present Charge is reported in increments of 0.1WH */
+	if (!ret)
+		present_charge = (u16)UW_TO_W(val.intval * 10);
+
+	ret = power_supply_get_property(batt, POWER_SUPPLY_PROP_STATUS, &val);
+	if (!ret) {
+		switch (val.intval) {
+		case POWER_SUPPLY_STATUS_CHARGING:
+			charging_status = BSDO_BATTERY_INFO_CHARGING;
+			break;
+		case POWER_SUPPLY_STATUS_DISCHARGING:
+			charging_status = BSDO_BATTERY_INFO_DISCHARGING;
+			break;
+		case POWER_SUPPLY_STATUS_NOT_CHARGING:
+		case POWER_SUPPLY_STATUS_FULL:
+			charging_status = BSDO_BATTERY_INFO_IDLE;
+			break;
+		default:
+			charging_status = BSDO_BATTERY_INFO_RSVD;
+			break;
+		}
+	}
+
+send_status:
+
+	bsdo = BSDO(present_charge, charging_status, batt_present, invalid_ref);
+	msg.payload[0] = cpu_to_le32(bsdo);
+	msg.header = PD_HEADER_LE(PD_DATA_BATT_STATUS,
+				  port->pwr_role,
+				  port->data_role,
+				  port->negotiated_rev,
+				  port->message_id,
+				  1);
+
 	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
 }
 
@@ -3854,6 +3965,7 @@ static void tcpm_pd_ext_msg_request(struct tcpm_port *port,
 {
 	enum pd_ext_msg_type type = pd_header_type_le(msg->header);
 	unsigned int data_size = pd_ext_header_data_size_le(msg->ext_msg.header);
+	const struct pd_chunked_ext_message_data *ext_msg = &msg->ext_msg;
 
 	/* stopping VDM state machine if interrupted by other Messages */
 	if (tcpm_vdm_ams(port)) {
@@ -3862,7 +3974,7 @@ static void tcpm_pd_ext_msg_request(struct tcpm_port *port,
 		mod_vdm_delayed_work(port, 0);
 	}
 
-	if (!(le16_to_cpu(msg->ext_msg.header) & PD_EXT_HDR_CHUNKED)) {
+	if (!(le16_to_cpu(ext_msg->header) & PD_EXT_HDR_CHUNKED)) {
 		tcpm_pd_handle_msg(port, PD_MSG_CTRL_NOT_SUPP, NONE_AMS);
 		tcpm_log(port, "Unchunked extended messages unsupported");
 		return;
@@ -3887,9 +3999,17 @@ static void tcpm_pd_ext_msg_request(struct tcpm_port *port,
 					     NONE_AMS, 0);
 		}
 		break;
+	case PD_EXT_GET_BATT_STATUS:
+		if (data_size >= 1) {
+			port->batt_request_id = ext_msg->data[0];
+			tcpm_pd_handle_msg(port, PD_MSG_DATA_BATT_STATUS,
+					   GETTING_BATTERY_STATUS);
+		} else {
+			tcpm_set_state(port, SOFT_RESET_SEND, 0);
+		}
+		break;
 	case PD_EXT_SOURCE_CAP_EXT:
 	case PD_EXT_GET_BATT_CAP:
-	case PD_EXT_GET_BATT_STATUS:
 	case PD_EXT_BATT_CAP:
 	case PD_EXT_GET_MANUFACTURER_INFO:
 	case PD_EXT_MANUFACTURER_INFO:
@@ -4100,6 +4220,14 @@ static bool tcpm_send_queued_message(struct tcpm_port *port)
 					 ret);
 			tcpm_ams_finish(port);
 			break;
+		case PD_MSG_DATA_BATT_STATUS:
+			ret = tcpm_pd_send_batt_status(port);
+			if (ret)
+				tcpm_log(port,
+					 "Failed to send battery status ret=%d",
+					 ret);
+			tcpm_ams_finish(port);
+			break;
 		default:
 			break;
 		}
@@ -8566,6 +8694,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	typec_port_register_cable_ops(port->port_altmode, ARRAY_SIZE(port->port_altmode),
 				      &tcpm_cable_ops);
 	port->registered = true;
+	port->fixed_batt_cnt = 0;
 
 	mutex_lock(&port->lock);
 	tcpm_init(port);
@@ -8597,6 +8726,11 @@ void tcpm_unregister_port(struct tcpm_port *port)
 	hrtimer_cancel(&port->vdm_state_machine_timer);
 	hrtimer_cancel(&port->state_machine_timer);
 
+	for (i = 0; i < port->fixed_batt_cnt; i++) {
+		if (port->fixed_batt[i])
+			power_supply_put(port->fixed_batt[i]);
+	}
+
 	tcpm_reset_port(port);
 
 	tcpm_port_unregister_pd(port);
diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index 337a5485af7c..1f126f477c30 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -724,4 +724,33 @@ void usb_power_delivery_unlink_device(struct usb_power_delivery *pd, struct devi
 
 #endif /* CONFIG_TYPEC */
 
+/* Battery Status Data Object */
+#define BSDO_PRESENT_CAPACITY				GENMASK(31, 16)
+#define BSDO_CHG_STATUS					GENMASK(11, 10)
+#define BSDO_BATTERY_PRESENT				BIT(9)
+#define BSDO_INVALID_BATTERY_REFERENCE			BIT(8)
+
+/*
+ * Battery Charge Status: Battery Charging Status Values as defined in
+ * "USB PD Spec Rev3.1 Ver1.8", "Table 6-46 Battery Status Data Object (BSDO)".
+ */
+#define BSDO_BATTERY_INFO_CHARGING			0x0
+#define BSDO_BATTERY_INFO_DISCHARGING			0x1
+#define BSDO_BATTERY_INFO_IDLE				0x2
+#define BSDO_BATTERY_INFO_RSVD				0x3
+
+/**
+ * BSDO() - Pack data into Battery Status Data Object format
+ * @batt_charge: Battery's present state of charge in 0.1WH increment
+ * @chg_status: Battery charge status
+ * @batt_present: Indicates that battery is present/attached when set else absent when unset
+ * @invalid_ref: Indicates that an invalid battery reference was made in the Get_Battery_Status
+ *		 request
+ */
+#define BSDO(batt_charge, chg_status, batt_present, invalid_ref)	\
+	((FIELD_PREP(BSDO_PRESENT_CAPACITY, batt_charge)) |		\
+	 (FIELD_PREP(BSDO_CHG_STATUS, chg_status)) |			\
+	 ((batt_present) ? BSDO_BATTERY_PRESENT : 0) |			\
+	 ((invalid_ref) ? BSDO_INVALID_BATTERY_REFERENCE : 0))
+
 #endif /* __LINUX_USB_PD_H */

-- 
2.54.0.563.g4f69b47b94-goog

[PATCH 3/3] power: supply: max17042: add handler for energy_now property

[Amit Sunil Dhamne via B4 Relay]

From: Amit Sunil Dhamne <amitsd@google.com>

Add handler to report power_supply_prop_energy_now so that users can get
current SoC in uWH. Additionally, add helper functions to get avg_vcell
and repcap values in uv and uah units respectively to avoid code
duplication.

Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
 drivers/power/supply/max17042_battery.c | 60 ++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 167fb3fb3732..e16eb6985b70 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -81,6 +81,7 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_NOW,
 	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+	POWER_SUPPLY_PROP_ENERGY_NOW,
 	POWER_SUPPLY_PROP_TEMP,
 	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
 	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
@@ -95,6 +96,36 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_CURRENT_AVG,
 };
 
+static int max17042_get_repcap_uah(struct max17042_chip *chip, u64 *rep_cap)
+{
+	u32 data;
+	int ret;
+
+	ret = regmap_read(chip->regmap, MAX17042_RepCap, &data);
+	if (ret < 0)
+		return ret;
+
+	*rep_cap = data * 5000000ll;
+	*rep_cap *= chip->task_period;
+	do_div(*rep_cap, MAX17042_DEFAULT_TASK_PERIOD);
+	do_div(*rep_cap, chip->pdata->r_sns);
+
+	return 0;
+}
+
+static int max17042_get_avgvcell_uv(struct max17042_chip *chip, u32 *vcell)
+{
+	int ret;
+
+	ret = regmap_read(chip->regmap, MAX17042_AvgVCELL, vcell);
+	if (ret < 0)
+		return ret;
+
+	*vcell = (*vcell * 625) / 8;
+
+	return 0;
+}
+
 static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
 {
 	int ret;
@@ -180,14 +211,12 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
 	int temp, vavg, vbatt, ret;
 	u32 val;
 
-	ret = regmap_read(chip->regmap, MAX17042_AvgVCELL, &val);
+	ret = max17042_get_avgvcell_uv(chip, &val);
 	if (ret < 0)
 		goto health_error;
 
-	/* bits [0-3] unused */
-	vavg = val * 625 / 8;
 	/* Convert to millivolts */
-	vavg /= 1000;
+	vavg = val / 1000;
 
 	ret = regmap_read(chip->regmap, MAX17042_VCELL, &val);
 	if (ret < 0)
@@ -304,11 +333,10 @@ static int max17042_get_property(struct power_supply *psy,
 		val->intval = data * 625 / 8;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
-		ret = regmap_read(map, MAX17042_AvgVCELL, &data);
+		ret = max17042_get_avgvcell_uv(chip, &data);
 		if (ret < 0)
 			return ret;
-
-		val->intval = data * 625 / 8;
+		val->intval = data;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
 		ret = regmap_read(map, MAX17042_OCVInternal, &data);
@@ -350,14 +378,9 @@ static int max17042_get_property(struct power_supply *psy,
 		val->intval = data64;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_NOW:
-		ret = regmap_read(map, MAX17042_RepCap, &data);
+		ret = max17042_get_repcap_uah(chip, &data64);
 		if (ret < 0)
 			return ret;
-
-		data64 = data * 5000000ll;
-		data64 *= chip->task_period;
-		do_div(data64, MAX17042_DEFAULT_TASK_PERIOD);
-		do_div(data64, chip->pdata->r_sns);
 		val->intval = data64;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
@@ -370,6 +393,17 @@ static int max17042_get_property(struct power_supply *psy,
 		data64 = div_s64(data64, MAX17042_DEFAULT_TASK_PERIOD);
 		val->intval = div_s64(data64, chip->pdata->r_sns);
 		break;
+	case POWER_SUPPLY_PROP_ENERGY_NOW:
+		ret = max17042_get_repcap_uah(chip, &data64);
+		if (ret < 0)
+			return ret;
+
+		ret = max17042_get_avgvcell_uv(chip, &data);
+		if (ret < 0)
+			return ret;
+
+		val->intval = data64 * data / 1000000;
+		break;
 	case POWER_SUPPLY_PROP_TEMP:
 		ret = max17042_get_temperature(chip, &val->intval);
 		if (ret < 0)

-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH 1/3] power: Add power_supply_get_battery_all() to fetch battery psy handles

[Hans de Goede]

Hi,

On 15-May-26 07:48, Amit Sunil Dhamne via B4 Relay wrote:
> From: Amit Sunil Dhamne <amitsd@google.com>
> 
> Add power_supply_get_battery_all() to allow drivers to obtain a list of
> registered battery type power supply references in the system.
> 
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> ---
>  drivers/power/supply/power_supply_core.c | 122 +++++++++++++++++++++++++++++++
>  include/linux/power_supply.h             |   9 +++
>  2 files changed, 131 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index a446d3d086fc..697645426fb1 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -482,6 +482,128 @@ struct power_supply *power_supply_get_by_name(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>  
> +static int __power_supply_get_num_battery(struct power_supply *epsy, void *data)
> +{
> +	int *count = data;
> +
> +	if (epsy->desc->type == POWER_SUPPLY_TYPE_BATTERY)
> +		(*count)++;
> +
> +	return 0;
> +}
> +
> +static int power_supply_get_num_battery(struct device *dev)
> +{
> +	int ret, count = 0;
> +
> +	ret = power_supply_for_each_psy(&count, __power_supply_get_num_battery);
> +
> +	dev_dbg(dev, "%s: count: %d ret %d\n", __func__, count, ret);
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +struct psy_get_supplies_data {
> +	int cnt;
> +	int size;
> +	struct power_supply **psys;
> +};
> +
> +static int __power_supply_populate_battery_array(struct power_supply *epsy,
> +						 void *_data)
> +{
> +	struct psy_get_supplies_data *data = _data;
> +
> +	if (epsy->desc->type == POWER_SUPPLY_TYPE_BATTERY) {
> +		if (data->size <= data->cnt)
> +			return -EOVERFLOW;
> +
> +		get_device(&epsy->dev);
> +		data->psys[data->cnt] = epsy;
> +		atomic_inc(&epsy->use_cnt);
> +		data->cnt++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int power_supply_populate_battery_array(struct device *dev, int size,
> +					       struct power_supply **batteries)
> +{
> +	int ret, i;
> +
> +	struct psy_get_supplies_data data = {
> +		.cnt = 0,
> +		.size = size,
> +		.psys = batteries,
> +	};
> +
> +	ret = power_supply_for_each_psy(&data, __power_supply_populate_battery_array);
> +
> +	dev_dbg(dev, "%s Found %d batteries with array size %d ret %d\n",
> +		__func__, data.cnt, data.size, ret);
> +
> +	if (ret < 0) {
> +		for (i = 0; i < data.cnt; i++)
> +			power_supply_put(batteries[i]);
> +		return ret;
> +	}
> +
> +	return data.cnt;
> +}
> +
> +/**
> + * power_supply_get_battery_all() - Searches for all battery type power supplies
> + *				    and returns their references.
> + * @dev: Pointer to device requesting the power supply refs.
> + * @psys: Pointer to an array of power supply refs that will be filled by this
> + *	  function.
> + *
> + * This function helps drivers get handles to all battery type power supplies.
> + * If acquiring a ref to a power supply results in error, then the search for
> + * battery type power supplies will abort and the acquired power supplies will
> + * be "put".
> + *
> + * Return: Indicates the number of battery type power supplies returned on
> + * success or the negative error code on failure.
> + *
> + * It's the responsibility of the caller to invoke power_supply_put() on the
> + * individual psy refs and free the array returned by this function using kfree().

Not a full review, just a quick remark: It seems to me that there should
be some helper function e.g. :

void power_supply_put_array(struct power_supply **psys, int count);

Which can be called with the returned array + count to do this for the caller,
rather then expecting all callers to open code this.

Regards,

Hans


> + */
> +int __must_check power_supply_get_battery_all(struct device *dev,
> +					      struct power_supply ***psys)
> +{
> +	int ret;
> +
> +	if (!psys)
> +		return -EINVAL;
> +
> +	ret = power_supply_get_num_battery(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!ret) {
> +		*psys = NULL;
> +		return 0;
> +	}
> +
> +	*psys = kzalloc_objs(**psys, ret);
> +	if (!*psys)
> +		return -ENOMEM;
> +
> +	ret = power_supply_populate_battery_array(dev, ret, *psys);
> +	if (ret <= 0) {
> +		kfree(*psys);
> +		*psys = NULL;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(power_supply_get_battery_all);
> +
>  /**
>   * power_supply_put() - Drop reference obtained with power_supply_get_by_name
>   * @psy: Reference to put
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 7a5e4c3242a0..2467530a5740 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -806,11 +806,20 @@ extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);
>  #if IS_ENABLED(CONFIG_POWER_SUPPLY)
>  extern struct power_supply *power_supply_get_by_name(const char *name);
> +extern int __must_check power_supply_get_battery_all(struct device *dev,
> +						     struct power_supply ***psys);
>  extern void power_supply_put(struct power_supply *psy);
>  #else
>  static inline void power_supply_put(struct power_supply *psy) {}
>  static inline struct power_supply *power_supply_get_by_name(const char *name)
>  { return NULL; }
> +static inline int __must_check power_supply_get_battery_all(struct device *dev,
> +							    struct power_supply ***psys)
> +{
> +	if (psys)
> +		*psys = NULL;
> +	return 0;
> +}
>  #endif
>  extern struct power_supply *power_supply_get_by_reference(struct fwnode_handle *fwnode,
>  							  const char *property);
> 

Re: [PATCH 3/3] power: supply: max17042: add handler for energy_now property

[Hans de Goede]

Hi,

On 15-May-26 07:48, Amit Sunil Dhamne via B4 Relay wrote:
> From: Amit Sunil Dhamne <amitsd@google.com>
> 
> Add handler to report power_supply_prop_energy_now so that users can get
> current SoC in uWH. Additionally, add helper functions to get avg_vcell
> and repcap values in uv and uah units respectively to avoid code
> duplication.
> 
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>

NACK for multiple reasons:

1. We don't want to do this in all drivers which only support
charge_now and not energy_now, instead the TCPM driver should
convert charge_now to energy_now when necessary itself so that
the TCPM code will work with all battery type power-supply drivers
not just those which provide energy_now.

2. Having energy_now without energy_full is problematic and will
confuse userspace which prefers energy_* over charge_* since
userspace will now miss a reference value for full to report
a charging progress percentage. I also wonder how this works
on the TCPM side does the  Battery Status response message not
have a full value / percentage ?

3. IIRC userspace (upower) picks either energy_* or charge_*
values depending on which are present. I'm not sure if having
both will not confuse userspace. As mentioned in 2. having both
while one of them has an incomplete set of properties is sure
to confuse userspace.

Regards,

Hans





> ---
>  drivers/power/supply/max17042_battery.c | 60 ++++++++++++++++++++++++++-------
>  1 file changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index 167fb3fb3732..e16eb6985b70 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -81,6 +81,7 @@ static enum power_supply_property max17042_battery_props[] = {
>  	POWER_SUPPLY_PROP_CHARGE_NOW,
>  	POWER_SUPPLY_PROP_CHARGE_COUNTER,
>  	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> +	POWER_SUPPLY_PROP_ENERGY_NOW,
>  	POWER_SUPPLY_PROP_TEMP,
>  	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
>  	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
> @@ -95,6 +96,36 @@ static enum power_supply_property max17042_battery_props[] = {
>  	POWER_SUPPLY_PROP_CURRENT_AVG,
>  };
>  
> +static int max17042_get_repcap_uah(struct max17042_chip *chip, u64 *rep_cap)
> +{
> +	u32 data;
> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, MAX17042_RepCap, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	*rep_cap = data * 5000000ll;
> +	*rep_cap *= chip->task_period;
> +	do_div(*rep_cap, MAX17042_DEFAULT_TASK_PERIOD);
> +	do_div(*rep_cap, chip->pdata->r_sns);
> +
> +	return 0;
> +}
> +
> +static int max17042_get_avgvcell_uv(struct max17042_chip *chip, u32 *vcell)
> +{
> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, MAX17042_AvgVCELL, vcell);
> +	if (ret < 0)
> +		return ret;
> +
> +	*vcell = (*vcell * 625) / 8;
> +
> +	return 0;
> +}
> +
>  static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
>  {
>  	int ret;
> @@ -180,14 +211,12 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>  	int temp, vavg, vbatt, ret;
>  	u32 val;
>  
> -	ret = regmap_read(chip->regmap, MAX17042_AvgVCELL, &val);
> +	ret = max17042_get_avgvcell_uv(chip, &val);
>  	if (ret < 0)
>  		goto health_error;
>  
> -	/* bits [0-3] unused */
> -	vavg = val * 625 / 8;
>  	/* Convert to millivolts */
> -	vavg /= 1000;
> +	vavg = val / 1000;
>  
>  	ret = regmap_read(chip->regmap, MAX17042_VCELL, &val);
>  	if (ret < 0)
> @@ -304,11 +333,10 @@ static int max17042_get_property(struct power_supply *psy,
>  		val->intval = data * 625 / 8;
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
> -		ret = regmap_read(map, MAX17042_AvgVCELL, &data);
> +		ret = max17042_get_avgvcell_uv(chip, &data);
>  		if (ret < 0)
>  			return ret;
> -
> -		val->intval = data * 625 / 8;
> +		val->intval = data;
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
>  		ret = regmap_read(map, MAX17042_OCVInternal, &data);
> @@ -350,14 +378,9 @@ static int max17042_get_property(struct power_supply *psy,
>  		val->intval = data64;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_NOW:
> -		ret = regmap_read(map, MAX17042_RepCap, &data);
> +		ret = max17042_get_repcap_uah(chip, &data64);
>  		if (ret < 0)
>  			return ret;
> -
> -		data64 = data * 5000000ll;
> -		data64 *= chip->task_period;
> -		do_div(data64, MAX17042_DEFAULT_TASK_PERIOD);
> -		do_div(data64, chip->pdata->r_sns);
>  		val->intval = data64;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
> @@ -370,6 +393,17 @@ static int max17042_get_property(struct power_supply *psy,
>  		data64 = div_s64(data64, MAX17042_DEFAULT_TASK_PERIOD);
>  		val->intval = div_s64(data64, chip->pdata->r_sns);
>  		break;
> +	case POWER_SUPPLY_PROP_ENERGY_NOW:
> +		ret = max17042_get_repcap_uah(chip, &data64);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = max17042_get_avgvcell_uv(chip, &data);
> +		if (ret < 0)
> +			return ret;
> +
> +		val->intval = data64 * data / 1000000;
> +		break;
>  	case POWER_SUPPLY_PROP_TEMP:
>  		ret = max17042_get_temperature(chip, &val->intval);
>  		if (ret < 0)
>