[PATCH v2 0/2] regulator: fp9931: fix voltage mapping and runtime PM issues

[PATCH v2 0/2] regulator: fp9931: fix voltage mapping and runtime PM issues

[robby.cai]

From: Robby Cai <robby.cai@nxp.com>

This series fixes two issues in the FP9931/JD9930 regulator driver.

Patch 1 fixes errors in the VPOS/VNEG voltage selector table, correcting
an off-by-one entry and extending the table to cover the full selector
range defined by the datasheet [1].

Patch 2 fixes a Runtime PM reference counting bug in the V3P3 regulator
callbacks that can lead to "Runtime PM usage count underflow!" warnings.


[1] https://www.fitipower.com/dl/file/flXa6hIchVeu0W3K (Datasheet, p.13)

Thanks,
Robby


---

Changes in v2:
- [1/2] Extend VPOSNEG_table[] to cover selectors 0x29-0x3f as defined by the datasheet and reformat the table for readability.
- [2/2] Refine commit message; no code change.

Link to v1: https://lore.kernel.org/imx/20260721095959.1855436-1-robby.cai@oss.nxp.com/


---

Robby Cai (2):
  regulator: fp9931: Fix VPOS/VNEG voltage selector table
  regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops

 drivers/regulator/fp9931.c | 61 +++++++++++---------------------------
 1 file changed, 17 insertions(+), 44 deletions(-)

-- 
2.50.1

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

[robby.cai]

From: Robby Cai <robby.cai@nxp.com>

The VPOSNEG_table[] mapping does not match the FP9931 datasheet.

The datasheet defines the VPOS/VNEG voltage mapping as:

  00h-04h ->  7.04V (-7.04V)
  05h     ->  7.26V (-7.26V)
  06h     ->  7.49V (-7.49V)
  ...
  28h-3Fh -> 15.06V (-15.06V)

However, VPOSNEG_table[] has two issues:

1. Selector 0x00~0x04 should all map to 7.04V (5 entries), but the
   table has 6 entries of 7.04V, causing all subsequent entries to be
   shifted by one position.

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.

Fix both issues by removing the duplicate 7.04V entry and appending
the missing 23 clamped 15.06V entries, bringing the table to the
correct size of 64 entries (0x00~0x3F).

Fixes: 12d821bd13d4 ("regulator: Add FP9931/JD9930 driver")
Signed-off-by: Robby Cai <robby.cai@nxp.com>
---
 drivers/regulator/fp9931.c | 54 +++++++++-----------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/regulator/fp9931.c b/drivers/regulator/fp9931.c
index 002b41f53eff..ff743a8b0dfe 100644
--- a/drivers/regulator/fp9931.c
+++ b/drivers/regulator/fp9931.c
@@ -37,48 +37,18 @@ struct fp9931_data {
 };
 
 static const unsigned int VPOSNEG_table[] = {
-	7040000,
-	7040000,
-	7040000,
-	7040000,
-	7040000,
-	7040000,
-	7260000,
-	7490000,
-	7710000,
-	7930000,
-	8150000,
-	8380000,
-	8600000,
-	8820000,
-	9040000,
-	9270000,
-	9490000,
-	9710000,
-	9940000,
-	10160000,
-	10380000,
-	10600000,
-	10830000,
-	11050000,
-	11270000,
-	11490000,
-	11720000,
-	11940000,
-	12160000,
-	12380000,
-	12610000,
-	12830000,
-	13050000,
-	13280000,
-	13500000,
-	13720000,
-	13940000,
-	14170000,
-	14390000,
-	14610000,
-	14830000,
-	15060000,
+	7040000,  7040000,  7040000,  7040000,  7040000,            /* 00h-04h */
+	7260000,  7490000,  7710000,  7930000,  8150000,  8380000,  /* 05h-0Ah */
+	8600000,  8820000,  9040000,  9270000,  9490000,  9710000,  /* 0Bh-10h */
+	9940000,  10160000, 10380000, 10600000, 10830000, 11050000, /* 11h-16h */
+	11270000, 11490000, 11720000, 11940000, 12160000, 12380000, /* 17h-1Ch */
+	12610000, 12830000, 13050000, 13280000, 13500000, 13720000, /* 1Dh-22h */
+	13940000, 14170000, 14390000, 14610000, 14830000, 15060000, /* 23h-28h */
+	/* 29h-3Fh: clamped to 15.06V per datasheet */
+	15060000, 15060000, 15060000, 15060000, 15060000, 15060000,
+	15060000, 15060000, 15060000, 15060000, 15060000, 15060000,
+	15060000, 15060000, 15060000, 15060000, 15060000, 15060000,
+	15060000, 15060000, 15060000, 15060000, 15060000,
 };
 
 static const struct hwmon_channel_info *fp9931_info[] = {
-- 
2.50.1

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

[sashiko-bot]

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 <robby.cai@nxp.com>

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/20260724103441.800522-1-robby.cai@oss.nxp.com?part=1

[PATCH v2 2/2] regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops

[robby.cai]

From: Robby Cai <robby.cai@nxp.com>

The fp9931_v3p3_enable() callback acquires a Runtime PM reference which
remains held until fp9931_v3p3_disable() releases it.

Device-level Runtime PM lifetime is also managed through the shared
VCOM enable path: fp9931_set_enable() acquires a reference and
fp9931_clear_enable() releases it.

As a result, the Runtime PM usage count may already have reached 0
before fp9931_v3p3_disable() is invoked. In that case,
fp9931_v3p3_disable() calls pm_runtime_put_autosuspend() on an
already-zero count, triggering:

  fp9931 1-0018: Runtime PM usage count underflow!

Holding a Runtime PM reference across the V3P3 regulator lifetime is
unnecessary. V3P3 only needs the device to be runtime-active for the
duration of the register access.

Fix this by scoping the Runtime PM reference lifetime to the register
access itself, pairing pm_runtime_resume_and_get() and
pm_runtime_put_autosuspend() within each callback.

Fixes: 12d821bd13d4 ("regulator: Add FP9931/JD9930 driver")
Signed-off-by: Robby Cai <robby.cai@nxp.com>
---
 drivers/regulator/fp9931.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/fp9931.c b/drivers/regulator/fp9931.c
index ff743a8b0dfe..0d926546221a 100644
--- a/drivers/regulator/fp9931.c
+++ b/drivers/regulator/fp9931.c
@@ -214,8 +214,7 @@ static int fp9931_v3p3_enable(struct regulator_dev *rdev)
 		return ret;
 
 	ret = regulator_enable_regmap(rdev);
-	if (ret < 0)
-		pm_runtime_put_autosuspend(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
 
 	return ret;
 }
@@ -225,6 +224,10 @@ static int fp9931_v3p3_disable(struct regulator_dev *rdev)
 	struct fp9931_data *data = rdev_get_drvdata(rdev);
 	int ret;
 
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = regulator_disable_regmap(rdev);
 	pm_runtime_put_autosuspend(data->dev);
 
-- 
2.50.1

Re: [PATCH v2 2/2] regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dropping the runtime PM reference in fp9931_v3p3_enable() allows the PMIC to auto-suspend, physically powering off the active V3P3 regulator and violating framework guarantees.
- [High] Disabling the V3P3 regulator while the PMIC is runtime-suspended causes a spurious 3.3V power-on glitch.
--

--- Patch [2]: [PATCH v2 2/2] regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops ---
commit 195ff0977cf3484ecb06ab9189df30e12979d50c
Author: Robby Cai <robby.cai@nxp.com>

regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops

This commit scopes the Runtime PM reference lifetime to the register
access itself in the v3p3 enable and disable callbacks. It pairs
pm_runtime_resume_and_get() and pm_runtime_put_autosuspend() within
each function to fix a usage count underflow issue.

> diff --git a/drivers/regulator/fp9931.c b/drivers/regulator/fp9931.c
> index ff743a8b0dfe8..0d926546221a6 100644
> --- a/drivers/regulator/fp9931.c
> +++ b/drivers/regulator/fp9931.c

[ ... ]

> @@ -214,8 +214,7 @@ static int fp9931_v3p3_enable(struct regulator_dev *rdev)
>  		return ret;
>  
>  	ret = regulator_enable_regmap(rdev);
> -	if (ret < 0)
> -		pm_runtime_put_autosuspend(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);

[Severity: High]
Does dropping the runtime PM reference here allow the PMIC to auto-suspend
while the V3P3 regulator is still logically enabled by consumers?

If a consumer enables V3P3, fp9931_v3p3_enable() will acquire and then
unconditionally release the reference after setting the register. 

When the PMIC subsequently auto-suspends, fp9931_runtime_suspend() could
disable the main vin_reg power supply, causing the V3P3 output to physically
power down and break framework guarantees.

>  
>  	return ret;
>  }
> @@ -225,6 +224,10 @@ static int fp9931_v3p3_disable(struct regulator_dev *rdev)
>  	struct fp9931_data *data = rdev_get_drvdata(rdev);
>  	int ret;
>  
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = regulator_disable_regmap(rdev);

[Severity: High]
Can this resume trigger a spurious 3.3V power-on glitch if the device was
already suspended?

Since the device can now auto-suspend while V3P3 is logically enabled, calling
regulator_disable() could trigger fp9931_v3p3_disable() while the device is
suspended.

In this state, pm_runtime_resume_and_get() wakes the device and
invokes fp9931_runtime_resume(), which calls regcache_sync().

Because the regcache still contains the 'enabled' state for V3P3, wouldn't
regcache_sync() write that state to hardware, physically turning V3P3 ON just
before regulator_disable_regmap() clears the bit and turns it OFF?

>  	pm_runtime_put_autosuspend(data->dev);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724103441.800522-1-robby.cai@oss.nxp.com?part=2