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

Hi Sebastian,

On 7/25/26 5:01 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jul 14, 2026 at 09:10:53PM +0000, Amit Sunil Dhamne via B4 Relay wrote:
>> From: Amit Sunil Dhamne <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Badhri Jagan Sridharan <[email protected]>
>> Acked-by: Heikki Krogerus <[email protected]>
>> ---
>>  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.
> 

You are completely right. I was approaching this primarily from the
smartphone side (Pixel 6), where batteries are effectively fixed and
inaccessible to the user. Because I don't have a setup with TCPM +
hot-swappable (in the context of the spec) batteries to test with, I 
only implemented the fixed case.

I mentioned this constraint in the cover letter, but I agree it could
have been in the commit message as well. Since Greg has already picked
this series up into his tree, I can't amend the commit message now.
However, if you think it's necessary, I can send a small incremental
patch to add a comment in the code clarifying this assumption.
Otherwise, we can leave it as-is until someone has the hardware to
properly implement and test the hot-swappable support. Let me know what
you prefer.

>> [...]
>> +	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?
> 

Good point. Our fuel gauge uses charge_* properties, so charge_now was
sufficient for our immediate use case, but it makes sense to support
energy_now natively for other users.

Since the original patch is already merged, I could write an incremental
follow-up patch that checks POWER_SUPPLY_PROP_ENERGY_NOW first, and if
it's not supported, falls back to calculating it via CHARGE_NOW *
VOLTAGE_AVG. Please let me know if this works?


> P.S.: Sorry for slow review.

No worries, thanks for your feedback!

Regards,
Amit
> 
> Greetings,
> 
> -- Sebastian
>