Re: [PATCH v2 3/4] regulator: Add driver for MediaTek MT6328 PMIC regulators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Undefined behavior (negative bitwise shift) in LDO mode accessors for fixed regulators.
- [High] Hardware logic error: `buck_vproc` is incorrectly assigned the voltage selection register for `VPA`.
- [High] Mismatch between voltage table size and hardware mask for `ldo_vrf18_1` leads to silent undervolting.
- [Medium] The driver destructively mutates the global static `mt6328_regulators` array during device probe.
- [Low] The `MT6328_LDO_RANGE` macro assigns mode registers but uses an ops struct lacking mode accessors.
- [Low] Duplicate definition of `ldo_vcamio` regulator in initialization array.
- [Low] Unused static voltage tables `ldo_volt_table7` and `ldo_volt_table8`.
--

commit 1a95db9b71950cc46e1f082052c7c441b06a008e
Author: Yassine Oudjana <y.oudjana@protonmail.com>

regulator: Add driver for MediaTek MT6328 PMIC regulators

This commit adds a new driver for the regulators found on the MT6328 PMIC.
It supports various bucks and LDOs using the regulator framework.

> diff --git a/drivers/regulator/mt6328-regulator.c b/drivers/regulator/mt6328-regulator.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/regulator/mt6328-regulator.c

[ ... ]

> +static const unsigned int ldo_volt_table6[] = {
> +	2900000, 3000000, 3300000
> +};
> +
> +static const unsigned int ldo_volt_table7[] = {
> +	2500000, 2900000, 3000000, 3300000
> +};
> +
> +static const unsigned int ldo_volt_table8[] = {
> +	1300000, 1800000, 2900000, 3300000
> +};

[Severity: Low]
Are these static voltage tables used anywhere in the driver? It looks like
ldo_volt_table7 and ldo_volt_table8 are defined here but are never
referenced in the mt6328_regulators array.

[ ... ]

> +static int mt6328_ldo_table_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{
> +	int ret, val = 0;
> +	struct mt6328_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +	switch (mode) {
> +	case REGULATOR_MODE_STANDBY:
> +		val = MT6328_LDO_TABLE_MODE_LP;
> +		break;
> +	case REGULATOR_MODE_NORMAL:
> +		val = MT6328_LDO_TABLE_MODE_NORMAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val <<= ffs(info->modeset_mask) - 1;
> +
> +	ret = regmap_update_bits(rdev->regmap, info->modeset_reg,
> +				  info->modeset_mask, val);

[Severity: High]
Could this bitwise shift result in undefined behavior? If modeset_mask is 0,
ffs(0) evaluates to 0, resulting in a shift by -1.

I noticed ldo_vtref is initialized with a modeset_mask of 0 down in the
mt6328_regulators array, but it uses mt6328_volt_fixed_ops which exposes
both the set_mode and get_mode callbacks.

[ ... ]

> +static const struct regulator_ops mt6328_volt_range_ops = {
> +	.list_voltage = regulator_list_voltage_linear_range,
> +	.map_voltage = regulator_map_voltage_linear_range,
> +	.set_voltage_sel = regulator_set_voltage_sel_regmap,
> +	.get_voltage_sel = regulator_get_voltage_sel_regmap,
> +	.set_voltage_time_sel = regulator_set_voltage_time_sel,
> +	.enable = regulator_enable_regmap,
> +	.disable = regulator_disable_regmap,
> +	.is_enabled = regulator_is_enabled_regmap,
> +	.get_status = mt6328_get_status,
> +};

[Severity: Low]
Should this operations structure include the .set_mode and .get_mode callbacks?

Down below, ldo_vsram is initialized using the MT6328_LDO_RANGE macro which
sets up modeset_reg and modeset_mask, but since it is mapped to
mt6328_volt_range_ops, the mode data will be silently ignored by the
regulator core.

[ ... ]

> +/* The array is indexed by id(MT6328_ID_XXX) */
> +static struct mt6328_regulator_info mt6328_regulators[] = {
> +	MT6328_BUCK("buck_vpa", VPA, 500000, 3650000, 50000, buck_volt_range1, MT6328_VPA_CON9,
> +		    MT6328_VPA_CON11, 0x3f, MT6328_VPA_CON12, MT6328_VPA_CON7),
> +	MT6328_BUCK("buck_vproc", VPROC, 600000, 1393750, 6250, buck_volt_range2, MT6328_VPROC_CON9,
> +		    MT6328_VPA_CON11, 0x7f, MT6328_VPROC_CON12, MT6328_VPROC_CON7),

[Severity: High]
Does buck_vproc use the wrong voltage selection register here?

It looks like MT6328_VPA_CON11 was accidentally copied from buck_vpa. This
would cause voltage scaling for the processor to write to the VPA's voltage
selection register instead, breaking voltage scaling for both regulators.

[ ... ]

> +	MT6328_REG_FIXED("ldo_vrf18_0", VRF18_0, MT6328_VRF18_0_CON0, 1, 1825000,
> +			 MT6328_VRF18_0_CON0, BIT(0)),
> +	MT6328_LDO_TABLE("ldo_vrf18_1", VRF18_1, ldo_volt_table12, MT6328_VRF18_1_CON0, 1,
> +			 MT6328_SLDO_ANA_CON4, 0x30, MT6328_VRF18_1_CON0, BIT(0)),

[Severity: High]
Does this 2-bit mask (0x30) conflict with the size of ldo_volt_table12?

That voltage table contains 5 entries, which requires at least a 3-bit mask.
If the core requests the highest voltage (1.825V, index 4), it will be masked
to 0 when updated, which could silently undervolt the regulator to 1.2V instead.

[ ... ]

> +	MT6328_REG_FIXED("ldo_vcn18", VCN18, MT6328_VCN18_CON0, 1, 1800000, MT6328_VCN18_CON0,
> +			 BIT(0)),
> +	MT6328_LDO_TABLE("ldo_vcamio", VCAMIO, ldo_volt_table13, MT6328_VCAMIO_CON0, 1,
> +			 MT6328_SLDO_ANA_CON7, 0x30, MT6328_VCAMIO_CON0, BIT(0)),
> +	MT6328_LDO_TABLE("ldo_vcamio", VCAMIO, ldo_volt_table13, MT6328_VCAMIO_CON0, 1,
> +			 MT6328_SLDO_ANA_CON7, 0x30, MT6328_VCAMIO_CON0, BIT(0)),

[Severity: Low]
Is this duplicate definition of ldo_vcamio intentional? It appears this
entry was accidentally listed twice.

[ ... ]

> +static int mt6328_set_buck_vosel_reg(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6328 = dev_get_drvdata(pdev->dev.parent);
> +	int i;
> +	u32 regval;
> +
> +	for (i = 0; i < MT6328_MAX_REGULATOR; i++) {
> +		if (mt6328_regulators[i].vselctrl_reg) {
> +			if (regmap_read(mt6328->regmap,
> +				mt6328_regulators[i].vselctrl_reg,
> +				&regval) < 0) {
> +				dev_err(&pdev->dev,
> +					"Failed to read buck ctrl\n");
> +				return -EIO;
> +			}
> +
> +			if (regval & mt6328_regulators[i].vselctrl_mask) {
> +				mt6328_regulators[i].desc.vsel_reg =
> +				mt6328_regulators[i].vselon_reg;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does modifying mt6328_regulators[i].desc.vsel_reg permanently alter the
global static array?

If the driver is unbound and rebound later, and the hardware condition is no
longer met, the driver will incorrectly retain the mutated vsel_reg value
since there is no restore path for the original value.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-mt6735-pwrap-mt6328-pmic-v2-0-dac37bf92894@protonmail.com?part=3