Re: [PATCH v4 2/2] power: supply: qcom_smbx: add SMB5 support

[David Heidelberg]

On 20/08/2026 12:03, Robin Snyders via B4 Relay wrote:
> From: Casey Connolly <casey.connolly@xxxxxxxxxx>
>
> Introduce support for the SMB5 charger found on PM7250B, PM8150B and
> related Qualcomm PMICs.
>
> SMB5 uses different DCDC status offsets, charger-state encodings and
> electrical ranges. Select these from per-PMIC match data, with PM7250B
> using the PM8150B compatible fallback and parameter block. Read
> overvoltage from the SMB5 status bit, use the already-prescaled IIO
> voltage reading, and convert the SMB5 current-sense voltage to microamps.
> Use battery-info property presence when selecting voltage and current
> targets.
>
> Keep Type-C power-role and VBUS control with the dedicated TCPM and
> regulator drivers. Clear the unsupported HVDCP negotiation modes so stale
> firmware settings cannot raise VBUS. Leave the firmware recharge policy
> unchanged and match the downstream default of ADC-based AICL disabled,
> while enabling periodic hardware AICL with its twelve-second SMB5 rerun
> interval. Preserve the existing three-second SMB2 interval.
>
> PM8150B places the charger and VBUS regulator in the same DCDC peripheral,
> but the SMB5 path does not write the regulator registers. qcom_smbx reads
> 0x1108 and 0x110b, while qcom_usb_vbus-regulator writes 0x1140, 0x1152
> and 0x1153. The SMB2-only OTG configuration write to 0x1153 is not part
> of the SMB5 initialization sequence. The TCPM port and PD PHY use the
> separate 0x15xx and 0x17xx peripherals. Name the USBIN BC1.2 integration
> register and SMB2-only OTG definitions accordingly to make this ownership
> boundary explicit.
>
> Program the battery limits and complete SMB5 input and charging setup
> from the power-supply registration init callback before device_add
> publishes the properties. This makes all public callbacks safe without
> driver-specific probe synchronization.
>
> Suspend USB input and charging before SMB5 initialization. On a later
> failure, restore the original charging-enable state before the
> input-suspend state; leave the input suspended if charging cannot be
> restored. Cancel status work before unregistering the power supply during
> managed teardown. Update the Kconfig description to cover both charger
> generations.
>
> On a OnePlus 7T Pro, register reads from the initial implementation
> confirmed the programmed 4.40 V, 1.50 A and 500 mA limits. A 180-second
> guarded charging trace and a subsequent 600-second runtime trace
> completed without crossing the voltage guard.
>
> Signed-off-by: Casey Connolly <casey.connolly@xxxxxxxxxx>
> Co-developed-by: Joel Selvaraj <foss@xxxxxxxxxxxxxxxx>
> Signed-off-by: Joel Selvaraj <foss@xxxxxxxxxxxxxxxx>
> Co-developed-by: Robin Snyders <robin@xxxxxxxxxxx>
> Signed-off-by: Robin Snyders <robin@xxxxxxxxxxx>
> ---
>   drivers/power/supply/Kconfig     |   8 +-
>   drivers/power/supply/qcom_smbx.c | 757 ++++++++++++++++++++++++++++++---------
>   2 files changed, 598 insertions(+), 167 deletions(-)

[...]

>   
> +/* Return 1 when in overvoltage state, else 0 or -errno */
> +static int smbx_ov_status(struct smb_chip *chip)
> +{
> +	u8 mask;
> +	int rc;
> +	u32 val;
> +
> +	switch (chip->gen) {
> +	case SMB2:
> +		mask = SMB2_CHARGER_ERROR_STATUS_BAT_OV_BIT;
> +		break;
> +	case SMB5:
> +		mask = SMB5_CHARGER_ERROR_STATUS_BAT_OV_BIT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	rc = regmap_read(chip->regmap,
> +			 chip->base + BATTERY_CHARGER_STATUS_2, &val);
> +	if (rc)
> +		return rc;
> +
> +	return !!(val & mask);
> +}
> +
> +static int smb_map_charge_status(struct smb_chip *chip, u32 stat, int *val)

would it make sense to split into smb{2,5}_map_charge_status and pass as .data 
and call match_data->map_charge_status(chip, stat, val)?

> +{
> +	switch (chip->gen) {
> +	case SMB2:
> +		switch (stat) {
> +		case SMB2_TRICKLE_CHARGE:
> +		case SMB2_PRE_CHARGE:
> +		case SMB2_FAST_CHARGE:
> +		case SMB2_FULLON_CHARGE:
> +		case SMB2_TAPER_CHARGE:
> +			*val = POWER_SUPPLY_STATUS_CHARGING;
> +			return 0;
> +		case SMB2_TERMINATE_CHARGE:
> +		case SMB2_INHIBIT_CHARGE:
> +			*val = POWER_SUPPLY_STATUS_FULL;
> +			return 0;
> +		case SMB2_DISABLE_CHARGE:
> +			*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +			return 0;
> +		}
> +		break;
> +	case SMB5:
> +		switch (stat) {
> +		case SMB5_TRICKLE_CHARGE:
> +		case SMB5_PRE_CHARGE:
> +		case SMB5_FULLON_CHARGE:
> +		case SMB5_TAPER_CHARGE:
> +			*val = POWER_SUPPLY_STATUS_CHARGING;
> +			return 0;
> +		case SMB5_TERMINATE_CHARGE:
> +		case SMB5_INHIBIT_CHARGE:
> +			*val = POWER_SUPPLY_STATUS_FULL;
> +			return 0;
> +		case SMB5_PAUSE_CHARGE:
> +		case SMB5_DISABLE_CHARGE:
> +			*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +			return 0;
> +		}
> +		break;
> +	}
> +
> +	*val = POWER_SUPPLY_STATUS_UNKNOWN;
> +	return 0;
> +}
> +
>   static int smb_get_prop_status(struct smb_chip *chip, int *val)
>   {
> -	unsigned char stat[2];
> +	u32 stat;
>   	int usb_online = 0;
>   	int rc;
>   
> @@ -491,49 +619,36 @@ static int smb_get_prop_status(struct smb_chip *chip, int *val)
>   		return rc;
>   	}
>   
> -	rc = regmap_bulk_read(chip->regmap,
> -			      chip->base + BATTERY_CHARGER_STATUS_1, &stat, 2);
> +	rc = regmap_read(chip->regmap,
> +			 chip->base + BATTERY_CHARGER_STATUS_1, &stat);
>   	if (rc < 0) {
>   		dev_err(chip->dev, "Failed to read charging status ret=%d\n",
>   			rc);
>   		return rc;
>   	}
>   
> -	if (stat[1] & CHARGER_ERROR_STATUS_BAT_OV_BIT) {
> +	rc = smbx_ov_status(chip);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* In overvoltage state */
> +	if (rc == 1) {
>   		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
>   		return 0;
>   	}
>   
> -	stat[0] = stat[0] & BATTERY_CHARGER_STATUS_MASK;
> +	stat &= BATTERY_CHARGER_STATUS_MASK;
>   
> -	switch (stat[0]) {
> -	case TRICKLE_CHARGE:
> -	case PRE_CHARGE:
> -	case FAST_CHARGE:
> -	case FULLON_CHARGE:
> -	case TAPER_CHARGE:
> -		*val = POWER_SUPPLY_STATUS_CHARGING;
> -		return rc;
> -	case DISABLE_CHARGE:
> -		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> -		return rc;
> -	case TERMINATE_CHARGE:
> -	case INHIBIT_CHARGE:
> -		*val = POWER_SUPPLY_STATUS_FULL;
> -		return rc;
> -	default:
> -		*val = POWER_SUPPLY_STATUS_UNKNOWN;
> -		return rc;
> -	}
> +	return smb_map_charge_status(chip, stat, val);
>   }
>   
>   static inline int smb_get_current_limit(struct smb_chip *chip,
>   					 unsigned int *val)
>   {
> -	int rc = regmap_read(chip->regmap, chip->base + ICL_STATUS, val);
> +	int rc = regmap_read(chip->regmap, chip->base + chip->icl_status, val);
>   
>   	if (rc >= 0)
> -		*val *= CURRENT_SCALE_FACTOR;
> +		*val *= chip->icl_step_ua;
>   	return rc;
>   }
>   
> @@ -541,12 +656,13 @@ static int smb_set_current_limit(struct smb_chip *chip, unsigned int val)
>   {
>   	unsigned char val_raw;
>   
> -	if (val > 4800000) {
> +	if (val > chip->icl_max_ua) {
>   		dev_err(chip->dev,
> -			"Can't set current limit higher than 4800000uA");
> +			"Can't set current limit higher than %uuA",
> +			chip->icl_max_ua);
>   		return -EINVAL;
>   	}
> -	val_raw = val / CURRENT_SCALE_FACTOR;
> +	val_raw = val / chip->icl_step_ua;
>   
>   	return regmap_write(chip->regmap, chip->base + USBIN_CURRENT_LIMIT_CFG,
>   			    val_raw);
> @@ -607,12 +723,10 @@ static void smb_status_change_work(struct work_struct *work)
>   static int smb_get_iio_chan(struct smb_chip *chip, struct iio_channel *chan,
>   			     int *val)
>   {
> -	int rc;
> -	union power_supply_propval status;
> +	int rc, status;
>   
> -	rc = power_supply_get_property(chip->chg_psy, POWER_SUPPLY_PROP_STATUS,
> -				       &status);
> -	if (rc < 0 || status.intval != POWER_SUPPLY_STATUS_CHARGING) {
> +	rc = smb_get_prop_status(chip, &status);
> +	if (rc < 0 || status != POWER_SUPPLY_STATUS_CHARGING) {
>   		*val = 0;
>   		return 0;
>   	}
> @@ -625,7 +739,61 @@ static int smb_get_iio_chan(struct smb_chip *chip, struct iio_channel *chan,
>   	return iio_read_channel_processed(chan, val);
>   }
>   
> -static int smb_get_prop_health(struct smb_chip *chip, int *val)
> +static int smb_get_prop_current_now(struct smb_chip *chip, int *val)
> +{
> +	s64 current_ua;
> +	int rc;
> +
> +	rc = smb_get_iio_chan(chip, chip->usb_in_i_chan, val);
> +	if (rc < 0)
> +		return rc;
> +
> +	current_ua = (s64)*val * chip->usbin_current_scale;
> +	if (current_ua < INT_MIN || current_ua > INT_MAX)
> +		return -ERANGE;
> +
> +	*val = (int)current_ua;
> +	return 0;
> +}
> +
> +static int smb5_get_prop_health(struct smb_chip *chip, int *val)
> +{
> +	int rc;
> +	unsigned int stat;
> +
> +	rc = smbx_ov_status(chip);
> +	if (rc < 0) {
> +		dev_err(chip->dev,
> +			"Couldn't determine overvoltage status: %d\n", rc);
> +		return rc;
> +	}
> +	if (rc) {
> +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +		return 0;
> +	}
> +
> +	rc = regmap_read(chip->regmap, chip->base + BATTERY_CHARGER_STATUS_7,
> +			 &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't read charger status 7 rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	if (stat & SMB5_BAT_TEMP_STATUS_TOO_COLD_BIT)
> +		*val = POWER_SUPPLY_HEALTH_COLD;
> +	else if (stat & SMB5_BAT_TEMP_STATUS_TOO_HOT_BIT)
> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> +	else if (stat & SMB5_BAT_TEMP_STATUS_COLD_SOFT_BIT)
> +		*val = POWER_SUPPLY_HEALTH_COOL;
> +	else if (stat & SMB5_BAT_TEMP_STATUS_HOT_SOFT_BIT)
> +		*val = POWER_SUPPLY_HEALTH_WARM;
> +	else
> +		*val = POWER_SUPPLY_HEALTH_GOOD;
> +
> +	return 0;
> +}
> +
> +static int smb2_get_prop_health(struct smb_chip *chip, int *val)
>   {
>   	int rc;
>   	unsigned int stat;
> @@ -637,15 +805,15 @@ static int smb_get_prop_health(struct smb_chip *chip, int *val)
>   		return rc;
>   	}
>   
> -	if (stat & CHARGER_ERROR_STATUS_BAT_OV_BIT)
> +	if (stat & SMB2_CHARGER_ERROR_STATUS_BAT_OV_BIT)
>   		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> -	else if (stat & BAT_TEMP_STATUS_TOO_COLD_BIT)
> +	else if (stat & SMB2_BAT_TEMP_STATUS_TOO_COLD_BIT)
>   		*val = POWER_SUPPLY_HEALTH_COLD;
> -	else if (stat & BAT_TEMP_STATUS_TOO_HOT_BIT)
> +	else if (stat & SMB2_BAT_TEMP_STATUS_TOO_HOT_BIT)
>   		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> -	else if (stat & BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT)
> +	else if (stat & SMB2_BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT)
>   		*val = POWER_SUPPLY_HEALTH_COOL;
> -	else if (stat & BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT)
> +	else if (stat & SMB2_BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT)
>   		*val = POWER_SUPPLY_HEALTH_WARM;
>   	else
>   		*val = POWER_SUPPLY_HEALTH_GOOD;
> @@ -653,6 +821,19 @@ static int smb_get_prop_health(struct smb_chip *chip, int *val)
>   	return 0;
>   }
>   
> +static int smb_get_prop_health(struct smb_chip *chip, int *val)

I would drop this function and just called
smb{2,5}_get_prop_health directly with

match_data->get_prop_health(chip, val)

> +{
> +	switch (chip->gen) {
> +	case SMB2:
> +		return smb2_get_prop_health(chip, val);
> +	case SMB5:
> +		return smb5_get_prop_health(chip, val);
> +	default:
> +		dev_err(chip->dev, "unsupported SMB chip generation\n");
> +		return -EINVAL;
> +	}
> +}
> +
>   static int smb_get_property(struct power_supply *psy,
>   			     enum power_supply_property psp,
>   			     union power_supply_propval *val)
> @@ -669,8 +850,7 @@ static int smb_get_property(struct power_supply *psy,
>   	case POWER_SUPPLY_PROP_CURRENT_MAX:
>   		return smb_get_current_limit(chip, &val->intval);
>   	case POWER_SUPPLY_PROP_CURRENT_NOW:
> -		return smb_get_iio_chan(chip, chip->usb_in_i_chan,
> -					 &val->intval);
> +		return smb_get_prop_current_now(chip, &val->intval);
>   	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>   		return smb_get_iio_chan(chip, chip->usb_in_v_chan,
>   					 &val->intval);

[...]

> +static const struct smb_match_data pmi8998_match_data = {
> +	.init_seq = smb2_init_seq,
> +	.init_seq_len = ARRAY_SIZE(smb2_init_seq),
> +	.name = "pmi8998",
> +	.gen = SMB2,
> +	.fv_min_uv = 3487500,
> +	.fv_max_uv = 4920000,
> +	.fv_step_uv = 7500,
> +	.fcc_max_ua = 4500000,
> +	.fcc_step_ua = 25000,
> +	.icl_max_ua = 4800000,
> +	.icl_step_ua = 25000,
> +	.icl_status = SMB2_ICL_STATUS,
> +	.usbin_current_scale = 1,
> +};
> +
> +static const struct smb_match_data pm660_match_data = {
> +	.init_seq = smb2_init_seq,
> +	.init_seq_len = ARRAY_SIZE(smb2_init_seq),
> +	.name = "pm660",
> +	.gen = SMB2,
> +	.fv_min_uv = 3487500,
> +	.fv_max_uv = 4920000,
> +	.fv_step_uv = 7500,
> +	.fcc_max_ua = 4500000,
> +	.fcc_step_ua = 25000,
> +	.icl_max_ua = 4800000,
> +	.icl_step_ua = 25000,
> +	.icl_status = SMB2_ICL_STATUS,
> +	.usbin_current_scale = 1,
>   };

I don't see any difference between pmi8998 and pm660 (which applies for many 
parts between these chips usually). Maybe would be worth it just to ruse the 
pmi8998 for the pm660 and only set the name?

>   
> -static int smb_init_hw(struct smb_chip *chip)
> +static const struct smb_match_data pm8150b_match_data = {
> +	.init_seq = smb5_init_seq,
> +	.init_seq_len = ARRAY_SIZE(smb5_init_seq),
> +	.name = "pm8150b",
> +	.gen = SMB5,
> +	.fv_min_uv = 3600000,
> +	.fv_max_uv = 4790000,
> +	.fv_step_uv = 10000,
> +	.fcc_max_ua = 8000000,
> +	.fcc_step_ua = 50000,
> +	.icl_max_ua = 5000000,
> +	.icl_step_ua = 50000,
> +	.icl_status = SMB5_AICL_ICL_STATUS,
> +	.usbin_current_scale = 5,
> +};
> +
> +static int smb_init_hw(struct smb_chip *chip,
> +		       const struct smb_init_register *init_seq, size_t len)
>   {
>   	int rc, i;
>   
> -	for (i = 0; i < ARRAY_SIZE(smb_init_seq); i++) {
> +	for (i = 0; i < len; i++) {

since you touching this - C99 would be nice:
for (int i = ...

So far running these patches on Pixel 3 since yesterday, so far looks ok. I 
still need debug some crashes, but these usually happen regardless to charging, 
so likely not issue of this driver :))

David