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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Probe failure leaves SMB5 charging and USB input permanently disabled.
- [High] Watchdog configuration in `smb5_init_seq` uses hardcoded SMB2 offsets, corrupting unrelated hardware blocks.

Pre-existing issues:
- [High] Missing battery voltage properties cause unsigned integer underflow and write garbage hardware configurations on SMB2.
- [High] Logic bug in `smb2_get_prop_health` masks critical hardware faults like overvoltage and overheating.
- [High] The watchdog interrupt handler writes to a fixed PMIC address instead of the relative charger base, corrupting unrelated hardware.
--

--- Patch [2]: [PATCH 2/3] power: supply: qcom_smbx: add SMB5 support ---
commit 6307e65683cce1f0d817fcc2cf3983ee19ae24c0
Author: Casey Connolly <casey.connolly@linaro.org>

power: supply: qcom_smbx: add SMB5 support

Introduce support for the SMB5 charger found on PM8150B and other
modern Qualcomm PMICs. It selects register layout and limits from
match data, and suspends USB input/charging during init.

> diff --git a/drivers/power/supply/qcom_smbx.c b/drivers/power/supply/qcom_smbx.c
> index 67fdb43353388..09d9e0f95cb56 100644
> --- a/drivers/power/supply/qcom_smbx.c
> +++ b/drivers/power/supply/qcom_smbx.c

[ ... ]

> @@ -633,19 +761,19 @@ static int smb_get_prop_health(struct smb_chip *chip, int *val)
>  	}
>  
>  	switch (stat) {
> -	case CHARGER_ERROR_STATUS_BAT_OV_BIT:
> +	case SMB2_CHARGER_ERROR_STATUS_BAT_OV_BIT:
>  		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>  		return 0;

[ ... ]

> -	case BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT:
> +	case SMB2_BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT:
>  		*val = POWER_SUPPLY_HEALTH_WARM;
>  		return 0;
>  	default:
>  		*val = POWER_SUPPLY_HEALTH_GOOD;
>  		return 0;
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does this switch statement correctly handle
cases where multiple status bits are set?

Looking at smb2_get_prop_health(), the switch evaluates the unmasked register
value. If a normal status bit is active alongside a fault bit, it seems the
exact match fails and it falls through to the default case, incorrectly
returning POWER_SUPPLY_HEALTH_GOOD during hardware faults.

[ ... ]

> @@ -773,7 +916,7 @@ static irqreturn_t smb_handle_wdog_bark(int irq, void *data)
>  }
>  
>  static const struct power_supply_desc smb_psy_desc = {
> -	.name = "pmi8998_charger",
> +	.name = "SMB2_charger",

[Severity: High]
This is a pre-existing issue, but does smb_handle_wdog_bark() write to the
correct PMIC hardware block?

In smb_handle_wdog_bark(), the code performs a register write to the absolute
address BARK_BITE_WDOG_PET. It omits the chip->base offset which is used by
other register accesses to target the correct peripheral block. Could this
corrupt unrelated PMIC hardware blocks when the watchdog interrupt fires?

[ ... ]

> @@ -787,16 +930,84 @@ static const struct power_supply_desc smb_psy_desc = {
>  };
>  
>  /* Init sequence derived from vendor downstream driver */
> -static const struct smb_init_register smb_init_seq[] = {
> -	{ .addr = AICL_RERUN_TIME_CFG, .mask = AICL_RERUN_TIME_MASK, .val = 0 },
> +static const struct smb_init_register smb5_init_seq[] = {

[ ... ]

> +	/* Disable watchdog */
> +	{ .addr = SNARL_BARK_BITE_WD_CFG, .mask = 0xff, .val = 0 },
> +	{ .addr = WD_CFG,
> +	  .mask = WATCHDOG_TRIGGER_AFP_EN_BIT | WDOG_TIMER_EN_ON_PLUGIN_BIT |
> +		  BARK_WDOG_INT_EN_BIT,
> +	  .val = 0 },

[Severity: High]
Are these register offsets correct for SMB5 devices?

In smb5_init_seq(), SNARL_BARK_BITE_WD_CFG and WD_CFG appear to use hardcoded
SMB2 absolute offsets (0x653 and 0x651). When these are added to chip->base
in smb_init_hw(), will this write the watchdog configuration to the wrong
hardware block on SMB5 PMICs?

[ ... ]

> @@ -965,7 +1237,34 @@ static int smb_probe(struct platform_device *pdev)

[ ... ]

> +	if (chip->gen == SMB5) {
> +		/* Do not use bootloader charge limits while configuring SMB5. */
> +		rc = regmap_update_bits(chip->regmap,
> +					chip->base + USBIN_CMD_IL,
> +					USBIN_SUSPEND_BIT, USBIN_SUSPEND_BIT);
> +		if (rc < 0)
> +			return dev_err_probe(chip->dev, rc,
> +					     "could not suspend USB input\n");
> +
> +		rc = regmap_update_bits(chip->regmap,
> +					chip->base + CHARGING_ENABLE_CMD,
> +					CHARGING_ENABLE_CMD_BIT, 0);
> +		if (rc < 0)
> +			return dev_err_probe(chip->dev, rc,
> +					     "could not disable charging\n");
> +	}
> +

[Severity: High]
If any subsequent initialization step fails and probe returns early, does
this leave SMB5 charging and USB input permanently disabled?

In smb_probe(), the USBIN_SUSPEND_BIT is set and CHARGING_ENABLE_CMD_BIT is
cleared. If probe subsequently fails (for example, due to a missing device
tree property or IIO channel), the function returns without restoring these
bits. Could this prevent the device from charging or receiving USB power
until it is rebooted?

[ ... ]

> @@ -999,11 +1298,66 @@ static int smb_probe(struct platform_device *pdev)

[ ... ]

> +	} else {
> +		rc = (chip->batt_info->voltage_max_design_uv -
> +		      match_data->fv_min_uv) / match_data->fv_step_uv + 1;
> +		rc = regmap_update_bits(chip->regmap,
> +					chip->base + FLOAT_VOLTAGE_CFG,
> +					FLOAT_VOLTAGE_SETTING_MASK, rc);
> +		if (rc < 0)
> +			return dev_err_probe(chip->dev, rc,
> +					     "could not set vbat max\n");
> +	}

[Severity: High]
This is a pre-existing issue, but does this calculation handle missing
battery device tree properties safely?

If the battery node lacks a maximum voltage, power_supply_get_battery_info()
populates voltage_max_design_uv with a negative error code (like -EINVAL).
In smb_probe(), subtracting the unsigned fv_min_uv from this negative value
could cause an unsigned integer underflow, writing a garbage configuration
value to the FLOAT_VOLTAGE_CFG hardware register.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-submit-qcom-smbx-send-v1-v1-0-feec6cfa123a@snyders.xyz?part=2