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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Data race and state corruption due to mutation of the shared global `mt6392_regulators` array.
- [High] Logic error in `mt6392_ldo_get_mode()` reads the wrong register to determine mode.
- [Medium] Missing `.of_map_mode` callback breaks `regulator-initial-mode` parsing.
- [Medium] Poor error handling and unnecessary kernel log spamming in `mt6392_buck_set_mode`.
--

commit cf9cb32e08ea85698033220fad437d4b27bf421d
Author: Fabien Parent <parent.f@gmail.com>

regulator: Add MediaTek MT6392 regulator
    
The MT6392 is a regulator found on boards based on the MediaTek
MT8167, MT8516, and probably other SoCs. It is a so called PMIC and
connects as a slave to a SoC using SPI, wrapped inside PWRAP.

> diff --git a/drivers/regulator/mt6392-regulator.c b/drivers/regulator/mt6392-regulator.c
> new file mode 100644
> index 0000000000000..1f91aac3917ab
> --- /dev/null
> +++ b/drivers/regulator/mt6392-regulator.c

[ ... ]

> +static int mt6392_buck_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{

[ ... ]

> +	ret = regmap_update_bits(rdev->regmap, info->pwm_modeset_reg,
> +				 info->pwm_modeset_mask, val);
> +
> +	if (regmap_read(rdev->regmap, info->pwm_modeset_reg, &reg_value) < 0) {
> +		dev_err(&rdev->dev, "Failed to read register value\n");
> +		return -EIO;
> +	}
> +
> +	dev_info(&rdev->dev, "%s: info->pwm_modeset_reg 0x%x = 0x%x\n",
> +		 info->desc.name, info->pwm_modeset_reg, reg_value);
> +
> +	return ret;
> +}

[Severity: Medium]
Does this error handling correctly propagate failures?

If regmap_update_bits() fails, 'ret' contains the error code, but the
subsequent regmap_read() might succeed, masking the original error.

Also, since set_mode can be a hot path called frequently by power management,
could this dev_info() spam the kernel ring buffer? Would dev_dbg() be more
appropriate here?

[ ... ]

> +static unsigned int mt6392_ldo_get_mode(struct regulator_dev *rdev)
> +{

[ ... ]

> +	ret = regmap_read(rdev->regmap, info->lp_modeset_reg, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= info->lp_modeset_mask;
> +	val >>= ffs(info->lp_modeset_mask) - 1;

[Severity: High]
Should this read from info->lp_modeget_reg instead?

By reading from lp_modeset_reg, it seems this retrieves the requested mode
rather than the actual hardware state, leaving the lp_modeget fields unused.

[ ... ]

> +static const struct regulator_ops mt6392_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,

[Severity: Medium]
Is this missing the .of_map_mode callback?

The driver defines custom device tree mode constants (e.g.,
MT6392_BUCK_MODE_AUTO), but without .of_map_mode, the regulator framework
will ignore the regulator-initial-mode DT property and log a warning.

[ ... ]

> +static int mt6392_set_buck_vsel_reg(struct platform_device *pdev)
> +{

[ ... ]

> +			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]
Can this cause a data race or state corruption?

mt6392_regulators is a shared global static array, and the driver modifies it
during probe based on the current hardware state.

Since the driver requests PROBE_PREFER_ASYNCHRONOUS later on:

> +static struct platform_driver mt6392_regulator_driver = {
> +	.driver = {
> +		.name = "mt6392-regulator",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},

Could probing multiple MT6392 instances concurrently overwrite the desc
fields for the other instances? Does the driver need to allocate per-device
copies of the regulator_desc?

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