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

[Sebastian Reichel]

Hi,

On Tue, Jul 14, 2026 at 09:10:53PM +0000, Amit Sunil Dhamne via B4 Relay wrote:
> From: Amit Sunil Dhamne <amitsd@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Reviewed-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> Acked-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 138 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/usb/pd.h        |  29 +++++++++
>  2 files changed, 163 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 7ef746a90a17..cd33ee131ebd 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -12,6 +12,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/kthread.h>
> +#include <linux/math64.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -232,7 +233,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 {
> @@ -387,7 +389,15 @@ struct pd_timings {
>  };
>  
>  /* Convert microwatt to watt */
> -#define UW_TO_W(pow)					((pow) / 1000000)
> +#define UW_TO_W(pow)				(div_u64((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

If I understand the spec correctly, the presence of a fixed battery
should never change for fixed batteries. I guess the rationale is,
that one only has to the battery capabilities once for these kind of
batteries. But for the Linux kernel this concept does not exist and
all batteries are potentialle hot-swappable. For real hardware with
TCPM and hot-swappable battery, this code will now incorrectly
expose them as fixed battery and violate the spec. I think this should
at least be mentioned in the commit message.

> [...]
> +	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_CHARGE_NOW,
> +					&val);
> +	if (!ret) {
> +		charge_now = val.intval;
> +		ret = power_supply_get_property(batt,
> +						POWER_SUPPLY_PROP_VOLTAGE_AVG,
> +						&val);
> +		if (!ret) {
> +			energy_now = div_u64((u64)charge_now * val.intval,
> +					     1000000);
> +
> +			/*
> +			 * Battery Present Charge is reported in
> +			 * increments of 0.1WH.
> +			 */
> +			present_charge = (u16)UW_TO_W(energy_now * 10);
> +		}
> +	}
> [...]

What about fuel gauges, which expose POWER_SUPPLY_PROP_ENERGY_NOW
instead of POWER_SUPPLY_PROP_CHARGE_NOW?

P.S.: Sorry for slow review.

Greetings,

-- Sebastian

Attachment: signature.asc
Description: PGP signature