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

[Amit Sunil Dhamne]

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