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

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 3675 bytes --]

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.

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

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]