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

Hi Sebastian,


On 7/30/26 3:24 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jul 29, 2026 at 05:56:29PM -0700, Amit Sunil Dhamne wrote:
>> [...]
>>
>>>> +/*
>>>> + * 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.
> 
> You tested only the fixed variant, but your implementation is in the
> generic TCPM and does not check if it runs on a Pixel 6. If you
> touch generic code you need to think generic :)
> 
> Generally I would expect it is "safer" to expose batteries as
> hot-swappable when they are fixed than the other way around.
> FWIW the kernel's power-supply framework has no concept of fixed
> batteries.
> 

Misclassifying a battery would lead to compliance failures. Also, 
classifying a fixed battery as hot-swappable (even if it's a telemetry 
message to the port partner) may indicate to users that it's safe to 
hot-swap batteries which may not be the case (either for mechanical 
reasons or data corruption when suddenly removing battery from live 
battery powered devices). So I don't think it'd be safer.

Would it make sense to introduce a device-tree property (e.g., something 
like fixed-batteries label) so the hardware can explicitly define this? 
Let me know what you think the best path forward is.

>>>> [...]
>>>> +	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?
> 
> That sounds sensible to me.

I will send a new patch for this.

Regards,
Amit

> 
> Greetings,
> 
> -- Sebastian