Re: [PATCH v10 8/9] regulator: Add MediaTek MT6392 regulator

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver mutates the statically defined global array `mt6392_regulators` during probe, causing state corruption and race conditions for multi-instance deployments or re-probing.
--

commit b458a23ed7fdbe7db2ebc29e150ed87ba4a12dbe
Author: Fabien Parent [off-list ref]

regulator: Add MediaTek MT6392 regulator

This commit introduces support for the MediaTek MT6392 PMIC regulator.
The PMIC connects as a SPI slave wrapped inside PWRAP and is found on
MT8167 and MT8516 SoC based boards.

[ ... ]

+// Buck regulators can be in Register mode or Normal mode.
+// Each mode uses a different register to set the desired voltage.
+static int mt6392_set_buck_vsel_reg(struct platform_device *pdev)
+{
+	struct mt6397_chip *mt6392 = dev_get_drvdata(pdev->dev.parent);
+	int i;
+	u32 regval;
+
+	for (i = 0; i < MT6392_MAX_REGULATOR; i++) {
+		if (mt6392_regulators[i].vselctrl_reg) {
+			// Read the vselctrl_reg register
+			if (regmap_read(mt6392->regmap,
+					mt6392_regulators[i].vselctrl_reg,
+					&regval) < 0) {
+				dev_err(&pdev->dev,
+					"Failed to read buck ctrl\n");
+				return -EIO;
+			}
+
+			// vselctrl_reg[vselctrl_mask] defines the mode
+			if (regval & mt6392_regulators[i].vselctrl_mask) {
+				// Regulator in Normal mode
+				mt6392_regulators[i].desc.vsel_reg =
+					mt6392_regulators[i].vsel_normal_mode_reg;
+				mt6392_regulators[i].desc.vsel_mask =
+					mt6392_regulators[i].vsel_normal_mode_mask;

[Severity: High]
Does this code mutate the shared global mt6392_regulators array based on
per-instance hardware state? If multiple instances of this PMIC are present,
or during unbind/re-bind cycles, won't this corrupt the state for other
instances since the regulator core expects regulator_desc to be static and
independent across deployments?

+			} else {
+				// Regulator in Register mode
+				mt6392_regulators[i].desc.vsel_reg =
+					mt6392_regulators[i].vsel_reg_mode_reg;
+				mt6392_regulators[i].desc.vsel_mask =
+					mt6392_regulators[i].vsel_reg_mode_mask;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int mt6392_regulator_probe(struct platform_device *pdev)
+{
+	struct mt6397_chip *mt6392 = dev_get_drvdata(pdev->dev.parent);
+	struct regulator_config config = {};
+	struct regulator_dev *rdev;
+	int i;
+
+	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
+
+	// Initialize the bucks' vsel_reg and vsel_mask according to current HW state
+	if (mt6392_set_buck_vsel_reg(pdev))
+		return -EIO;
+
+	config.dev = mt6392->dev;
+	config.regmap = mt6392->regmap;
+	for (i = 0; i < MT6392_MAX_REGULATOR; i++) {
+		config.driver_data = &mt6392_regulators[i];
+
+		rdev = devm_regulator_register(&pdev->dev,
+					       &mt6392_regulators[i].desc,
+					       &config);

[Severity: High]
Because the regulator core stores a pointer to the passed regulator_desc,
will passing the mutated shared descriptor here cause the first PMIC
instance to use the second PMIC's register offsets if a second instance
is probed? Could this result in writing to incorrect registers when changing
voltages?

+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev, "failed to register %s\n",
+				mt6392_regulators[i].desc.name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902193351.185771-1-l.scorcia@gmail.com?part=8