Re: [PATCH 2/2] usb: typec: ucsi: psy: Fix voltage and current max for non-Fixed PDOs

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Mon, Dec 08, 2025 at 05:48:48PM +0000, Benson Leung kirjoitti:
> ucsi_psy_get_voltage_max and ucsi_psy_get_current_max are calculated
> using whichever pdo is in the last position of the src_pdos array, presuming
> it to be a fixed pdo, so the pdo_fixed_voltage or pdo_max_current
> helpers are used on that last pdo.
> 
> However, non-Fixed PDOs such as Battery PDOs, Augmented PDOs (used for AVS and
> for PPS) may exist, and are always at the end of the array if they do.
> In the event one of these more advanced chargers are attached the helpers for
> fixed return mangled values.
> 
> Here's an example case of a Google Pixel Flex Dual Port 67W USB-C Fast Charger
> with PPS support:
> POWER_SUPPLY_NAME=ucsi-source-psy-cros_ec_ucsi.4.auto2
> POWER_SUPPLY_TYPE=USB
> POWER_SUPPLY_CHARGE_TYPE=Standard
> POWER_SUPPLY_USB_TYPE=C [PD] PD_PPS PD_DRP
> POWER_SUPPLY_ONLINE=1
> POWER_SUPPLY_VOLTAGE_MIN=5000000
> POWER_SUPPLY_VOLTAGE_MAX=13400000
> POWER_SUPPLY_VOLTAGE_NOW=20000000
> POWER_SUPPLY_CURRENT_MAX=5790000
> POWER_SUPPLY_CURRENT_NOW=3250000
> 
> Voltage Max is reading as 13.4V, but that's an incorrect decode of the PPS
> APDO in the last position. Same goes for CURRENT_MAX. 5.79A is incorrect.
> 
> Instead, enumerate through the src_pdos and filter just for Fixed PDOs for
> now, and find the one with the highest voltage and current respectively.
> 
> After, from the same charger:
> POWER_SUPPLY_NAME=ucsi-source-psy-cros_ec_ucsi.4.auto2
> POWER_SUPPLY_TYPE=USB
> POWER_SUPPLY_CHARGE_TYPE=Standard
> POWER_SUPPLY_USB_TYPE=C [PD] PD_PPS PD_DRP
> POWER_SUPPLY_ONLINE=1
> POWER_SUPPLY_VOLTAGE_MIN=5000000
> POWER_SUPPLY_VOLTAGE_MAX=20000000
> POWER_SUPPLY_VOLTAGE_NOW=20000000
> POWER_SUPPLY_CURRENT_MAX=4000000
> POWER_SUPPLY_CURRENT_NOW=3250000
> 
> Signed-off-by: Benson Leung <bleung@chromium.org>

Stable material as well?

Instead of using the ternary operator you could have used a simple if
statement. But that's minor.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/psy.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> index b828719e33df..c8ebab8ba7e7 100644
> --- a/drivers/usb/typec/ucsi/psy.c
> +++ b/drivers/usb/typec/ucsi/psy.c
> @@ -112,15 +112,20 @@ static int ucsi_psy_get_voltage_max(struct ucsi_connector *con,
>  				    union power_supply_propval *val)
>  {
>  	u32 pdo;
> +	int max_voltage = 0;
>  
>  	switch (UCSI_CONSTAT(con, PWR_OPMODE)) {
>  	case UCSI_CONSTAT_PWR_OPMODE_PD:
> -		if (con->num_pdos > 0) {
> -			pdo = con->src_pdos[con->num_pdos - 1];
> -			val->intval = pdo_fixed_voltage(pdo) * 1000;
> -		} else {
> -			val->intval = 0;
> +		for (int i = 0; i < con->num_pdos; i++) {
> +			int pdo_voltage = 0;
> +
> +			pdo = con->src_pdos[i];
> +			if (pdo_type(pdo) == PDO_TYPE_FIXED)
> +				pdo_voltage = pdo_fixed_voltage(pdo) * 1000;
> +			max_voltage = (pdo_voltage > max_voltage) ? pdo_voltage
> +								  : max_voltage;
>  		}
> +		val->intval = max_voltage;
>  		break;
>  	case UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0:
>  	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
> @@ -168,6 +173,7 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con,
>  				    union power_supply_propval *val)
>  {
>  	u32 pdo;
> +	int max_current = 0;
>  
>  	if (!UCSI_CONSTAT(con, CONNECTED)) {
>  		val->intval = 0;
> @@ -176,12 +182,16 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con,
>  
>  	switch (UCSI_CONSTAT(con, PWR_OPMODE)) {
>  	case UCSI_CONSTAT_PWR_OPMODE_PD:
> -		if (con->num_pdos > 0) {
> -			pdo = con->src_pdos[con->num_pdos - 1];
> -			val->intval = pdo_max_current(pdo) * 1000;
> -		} else {
> -			val->intval = 0;
> +		for (int i = 0; i < con->num_pdos; i++) {
> +			int pdo_current = 0;
> +
> +			pdo = con->src_pdos[i];
> +			if (pdo_type(pdo) == PDO_TYPE_FIXED)
> +				pdo_current = pdo_max_current(pdo) * 1000;
> +			max_current = (pdo_current > max_current) ? pdo_current
> +								  : max_current;
>  		}
> +		val->intval = max_current;
>  		break;
>  	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
>  		val->intval = UCSI_TYPEC_1_5_CURRENT * 1000;
> -- 
> 2.52.0.223.gf5cc29aaa4-goog

-- 
heikki