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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver modifies the globally shared static `mt6392_regulators` array during probe, leading to cross-device state corruption and data races.
- [Medium] mt6392_ldo_set_mode() clears the low-power mode bit instead of setting it when requesting STANDBY mode, due to confusing a devicetree constant with a register bit value.
--

commit 93833e30ee73cbbe6e2371cc49b2ce2c9207ea99
Author: Fabien Parent [off-list ref]

regulator: Add MediaTek MT6392 regulator

This commit adds a new regulator driver for the MediaTek MT6392 PMIC. It
supports control over different power rails on boards based on MT8167, MT8516,
and other SoCs. The driver uses regmap for voltage and mode control across
multiple buck and LDO regulators.

[ ... ]

+static int mt6392_ldo_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	int ret, val = 0;
+	struct mt6392_regulator_info *info = rdev_get_drvdata(rdev);
+
+	if (!info->lp_modeset_mask) {
+		dev_err(&rdev->dev, "regulator %s doesn't support set_mode\n",
+			info->desc.name);
+		return -EINVAL;
+	}
+
+	switch (mode) {
+	case REGULATOR_MODE_STANDBY:
+		val = MT6392_LDO_MODE_LP;
+		break;

[Severity: Medium]
Will this clear the low-power mode bit instead of setting it?

MT6392_LDO_MODE_LP is defined as 2. If the mask is a single bit, shifting val
by ffs(mask) - 1 will result in a value that evaluates to 0 when masked in
regmap_update_bits().

Does val need to be 1 here to correctly set the bit for STANDBY mode?

+	case REGULATOR_MODE_NORMAL:
+		val = MT6392_REGULATOR_MODE_NORMAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val <<= ffs(info->lp_modeset_mask) - 1;
+
+	ret = regmap_update_bits(rdev->regmap, info->lp_modeset_reg,
+				 info->lp_modeset_mask, val);

[ ... ]

+// 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 globally shared static mt6392_regulators array
during probe?

Since the driver uses PROBE_PREFER_ASYNCHRONOUS, concurrent probes of multiple
PMIC instances could cause a data race.

Furthermore, as devm_regulator_register() stores a pointer to the desc
structure, all instantiated PMICs would end up sharing the mutated vsel_reg
from the last probed PMIC.

Could this break voltage control for any instances whose hardware state
differs?

+			} 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;
+}

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