Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing error cleanup on DVS initialization failure paths leaves dangling GPIO pointers or unconfigured hardware state, causing silent voltage scaling failures.
--
--- Patch [9]: [PATCH v2 09/11] regulator: wm831x: remove legacy DVS platform data ---
commit 314c31c6b7ec71e1f10af8307e95e6718e0fea28
Author: Dmitry Torokhov [off-list ref]
regulator: wm831x: remove legacy DVS platform data
Now that the only board file in mainline using wm831x buckv DVS has been
converted to use software properties, we can remove the unused legacy
DVS fields from 'struct wm831x_buckv_pdata'. Also remove the fallback
logic from the regulator driver, making DVS support purely
property-based when software nodes are used.
quoted hunk ↗ jump to hunk
diff --git a/drivers/regulator/wm831x-dcdc.c b/drivers/regulator/wm831x-dcdc.c
index ce43c51c01704..05f49b2f5bade 100644
--- a/drivers/regulator/wm831x-dcdc.c
+++ b/drivers/regulator/wm831x-dcdc.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -352,7 +347,7 @@ static void wm831x_buckv_dvs_init(struct platform_device *pdev,
if (device_property_read_u32(&pdev->dev, "wlf,dvs-init-state", &val) == 0)
dcdc->dvs_gpio_state = val;
else
- dcdc->dvs_gpio_state = pdata->dvs_init_state;
+ dcdc->dvs_gpio_state = 0;
dcdc->dvs_gpiod = devm_gpiod_get(&pdev->dev, "dvs",
dcdc->dvs_gpio_state ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
@@ -362,10 +357,13 @@ static void wm831x_buckv_dvs_init(struct platform_device *pdev,
return;
}
- if (device_property_read_u32(&pdev->dev, "wlf,dvs-control-src", &val) == 0)
- dvs_control_src = val;
- else
- dvs_control_src = pdata->dvs_control_src;
+ ret = device_property_read_u32(&pdev->dev, "wlf,dvs-control-src", &val);
+ if (ret) {
+ dev_err(wm831x->dev, "Failed to read DVS control source for %s: %d\n",
+ dcdc->name, ret);
+ return;
+ }
[Severity: High]
This is a pre-existing issue, but the new error return path here caught my
eye while reviewing.
If initialization fails here (or slightly above when devm_gpiod_get()
returns an error pointer), we return early but don't clear dcdc->dvs_gpiod.
If we fail to read "wlf,dvs-control-src", dcdc->dvs_gpiod is already
successfully populated, but the hardware DVS source is left unconfigured.
If devm_gpiod_get() failed earlier, dcdc->dvs_gpiod holds an error pointer.
In both scenarios, could this cause silent voltage scaling failures later on?
Looking at wm831x_buckv_set_voltage_sel(), there is this check:
wm831x_buckv_set_voltage_sel() {
...
if (dcdc->dvs_gpiod && dcdc->dvs_vsel == vsel)
return wm831x_buckv_set_dvs(rdev, 1);
...
}
Since dcdc->dvs_gpiod is either an error pointer or a valid GPIO (but without
configured hardware), this condition still evaluates to true. Does this skip
updating the hardware on_reg, causing the voltage change to silently fail
and leaving the system at a lower voltage?
Should dcdc->dvs_gpiod be explicitly set to NULL on these error paths so the
driver correctly falls back to standard voltage scaling?
quoted hunk ↗ jump to hunk
+ dvs_control_src = val;
switch (dvs_control_src) {
case 1:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-samsung-kp-v2-0-3c6ed4c9b3b6@gmail.com?part=9