Re: [PATCH v2 1/2] regulator: fp9931: Fix VPOS/VNEG voltage selector table

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

New issues:
- [Medium] The commit message makes technically inaccurate claims about the VPOSNEG_table size and an out-of-bounds access.

Pre-existing issues:
- [High] The vcom regulator is configured with n_voltages = 255 but its vsel_mask is 0xFF, preventing the maximum voltage level from being reached.
- [High] fp9931_set_enable() leaves en_gpio asserted when wait_for_completion_timeout() fails.
- [High] Incomplete error handling in fp9931_runtime_resume() leaks physical regulator enable and GPIO state on regcache_sync failure.
--

--- Patch [1]: [PATCH v2 1/2] regulator: fp9931: Fix VPOS/VNEG voltage selector table ---
commit 96adb91729b2025db8107f97477744ebe2be390d
Author: Robby Cai <[email protected]>

regulator: fp9931: Fix VPOS/VNEG voltage selector table

This commit fixes the VPOSNEG_table voltage mapping in the FP9931 regulator
driver. It removes a duplicate entry for 7.04V and appends missing entries
to properly clamp higher selectors to 15.06V, matching the datasheet
specifications.

> 2. Selectors 0x29~0x3F should all clamp to 15.06V (23 entries), but
>    the table has only 41 entries. Any selector value above 0x28
>    would result in an out-of-bounds table access.

[Severity: Medium]
Are these claims accurate?

The original table had 42 entries, not 41 (six 7.04V entries plus 36 other
entries).

Also, an out-of-bounds array access is not possible here since the regulator
core bounds-checks the requested selector against n_voltages (which was
correctly set to ARRAY_SIZE(VPOSNEG_table)).


While reviewing this patch, I noticed a few pre-existing issues in the same
file:

drivers/regulator/fp9931.c:fp9931_runtime_resume() {
...
> 	if (data->en_ts_gpio) {
> 		gpiod_set_value_cansleep(data->en_ts_gpio, 1);
> 		/* wait for one ADC conversion to have sane temperature */
> 		usleep_range(10000, 15000);
> 	}
> 
> 	ret = regcache_sync(data->regmap);
> 
> 	return ret;
> }

[Severity: High]
This isn't a bug introduced by this patch, but could this leak resources if
regcache_sync() fails?

It returns early without disabling data->vin_reg or clearing
data->en_ts_gpio.

This leaves the hardware perpetually on since the PM core will keep the
device in RPM_SUSPENDED without calling runtime_suspend().


drivers/regulator/fp9931.c:fp9931_set_enable() {
...
> 	wait_for_completion_timeout(&data->pgood_completion,
> 				    msecs_to_jiffies(PGOOD_TIMEOUT_MSECS));
> 	dev_dbg(data->dev, "turned on");
> 	if (gpiod_get_value_cansleep(data->pgood_gpio) != 1) {
> 		pm_runtime_put_autosuspend(data->dev);
> 		return -ETIMEDOUT;
> 	}
> 
> 	return 0;
> }

[Severity: High]
This is a pre-existing issue, but might this timeout path leave en_gpio
asserted?

If the enable operation times out, it returns -ETIMEDOUT but leaves
data->en_gpio set to 1.

Since the regulator framework assumes the device is disabled, it will not
issue a disable() call, which might leave the physical pin asserted
indefinitely.


drivers/regulator/fp9931.c:regulators
...
> 	{
> 		.name = "vcom",
> 		.of_match = of_match_ptr("vcom"),
> 		.regulators_node = of_match_ptr("regulators"),
> 		.id = 2,
> 		.ops = &fp9931_vcom_ops,
> 		.type = REGULATOR_VOLTAGE,
> 		.owner = THIS_MODULE,
> 		.n_voltages = 255,
> 		.min_uV = 0,
> 		.uV_step = 5000000 / 255,
> 		.vsel_reg = FP9931_REG_VCOM_SETTING,
> 		.vsel_mask = 0xFF
> 	},

[Severity: High]
This isn't a bug introduced by this patch, but does the n_voltages value
prevent the maximum voltage level from being reached?

By setting n_voltages to 255 instead of 256 for an 8-bit vsel_mask (0xFF),
the valid selectors are restricted to 0..254.

The maximum selector 0xFF (255) would be rejected by the regulator core,
meaning the maximum intended voltage cannot be set.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1