Re: [PATCH v11 9/9] arm64: dts: mediatek: Add MediaTek MT6392 PMIC dtsi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: mediatek: pwrap_probe() fails due to unindexed mediatek,mt6392
- [High] arm64: dts: mediatek: mt6392: unconstrained rails disabled by regulator_late_cleanup()
--

commit d14bb1584df5376f55003a68da2a5898709dcb49
Author: Val Packett <val@packett.cool>

arm64: dts: mediatek: Add MediaTek MT6392 PMIC dtsi

This commit adds the device tree include file for the MediaTek mt6392 pmic,
defining nodes for its keys, pinctrl, regulators, and rtc subdevices. It also
includes the new file in pumpkin-common.dtsi for the Pumpkin boards.

> diff --git a/arch/arm64/boot/dts/mediatek/mt6392.dtsi b/arch/arm64/boot/dts/mediatek/mt6392.dtsi
> new file mode 100644
> index 0000000000000..cc4770d70246c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt6392.dtsi
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + * Copyright (c) 2024 Val Packett <val@packett.cool>
> + * Copyright (c) 2026 Luca Leonardo Scorcia <l.scorcia@gmail.com>
> + */
> +
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/regulator/mediatek,mt6392-regulator.h>
> +
> +&pwrap {
> +	pmic: pmic {
> +		compatible = "mediatek,mt6392";

[Severity: High]
When pwrap_probe() in drivers/soc/mediatek/mtk-pmic-wrap.c probes the pwrap
node on boards including this file, it matches np->child against
of_slave_match_tbl[]:

drivers/soc/mediatek/mtk-pmic-wrap.c:pwrap_probe() {
	...
	if (np->child)
		of_slave_id = of_match_node(of_slave_match_tbl, np->child);

	if (!of_slave_id) {
		dev_dbg(&pdev->dev, "slave pmic should be defined in dts\n");
		return -EINVAL;
	}
	...
}

However, of_slave_match_tbl[] in drivers/soc/mediatek/mtk-pmic-wrap.c only
lists mediatek,mt6323, mediatek,mt6331, mediatek,mt6351, mediatek,mt6357,
mediatek,mt6358, mediatek,mt6359, mediatek,mt6380-regulator, and
mediatek,mt6397, and does not have an entry for mediatek,mt6392:

static const struct of_device_id of_slave_match_tbl[] = {
	{ .compatible = "mediatek,mt6323", .data = &pmic_mt6323 },
	{ .compatible = "mediatek,mt6331", .data = &pmic_mt6331 },
	{ .compatible = "mediatek,mt6351", .data = &pmic_mt6351 },
	{ .compatible = "mediatek,mt6357", .data = &pmic_mt6357 },
	{ .compatible = "mediatek,mt6358", .data = &pmic_mt6358 },
	{ .compatible = "mediatek,mt6359", .data = &pmic_mt6359 },

	/* The MT6380 PMIC only implements a regulator, so we bind it
	 * directly instead of using a MFD.
	 */
	{ .compatible = "mediatek,mt6380-regulator", .data = &pmic_mt6380 },
	{ .compatible = "mediatek,mt6397", .data = &pmic_mt6397 },
	{ /* sentinel */ }
};

Will pwrap_probe() fail with -EINVAL before initializing wrp->regmap or
calling of_platform_populate(), preventing the mt6392 pmic device and all of
its subdevices from probing?

> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +
> +		mt6392keys: keys {
> +			compatible = "mediatek,mt6392-keys";
> +
> +			key-power {
> +				linux,keycodes = <KEY_POWER>;
> +				wakeup-source;
> +			};
> +
> +			key-home {
> +				linux,keycodes = <KEY_HOME>;
> +				wakeup-source;
> +			};
> +		};
> +
> +		mt6392pio: pinctrl {
> +			compatible = "mediatek,mt6392-pinctrl";
> +
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +
> +		mt6392regulators: regulators {
> +			compatible = "mediatek,mt6392-regulator";
> +
> +			/* Fixed supply defined in the data sheet */
> +			avddldo-supply = <&mt6392_vsys_reg>;
> +
> +			mt6392_vcore_reg: vcore {
> +				regulator-allowed-modes = <MT6392_REGULATOR_MODE_NORMAL
> +							   MT6392_BUCK_MODE_FORCE_PWM>;
> +			};
> +			mt6392_vproc_reg: vproc {
> +				regulator-allowed-modes = <MT6392_REGULATOR_MODE_NORMAL
> +							   MT6392_BUCK_MODE_FORCE_PWM>;
> +			};
> +			mt6392_vsys_reg: vsys {
> +				regulator-allowed-modes = <MT6392_REGULATOR_MODE_NORMAL
> +							   MT6392_BUCK_MODE_FORCE_PWM>;
> +			};

[Severity: High]
All 26 regulator child nodes under mt6392regulators are defined without
regulator-always-on, and arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi
includes mt6392.dtsi without marking the core system rails as
regulator-always-on or attaching consumers.

When mt6392_regulator_probe() in drivers/regulator/mt6392-regulator.c
registers each regulator through devm_regulator_register() ->
regulator_register() -> regulator_of_get_init_data() ->
of_get_regulation_constraints(), any matched node without regulator-always-on
allows status changes:

drivers/regulator/of_regulator.c:of_get_regulation_constraints() {
	...
	constraints->always_on = of_property_read_bool(np, "regulator-always-on");
	if (!constraints->always_on) /* status change should be possible. */
		constraints->valid_ops_mask |= REGULATOR_CHANGE_STATUS;
	...
}

Because the switchable regulators in mt6392_regulators[] provide
regulator_is_enabled_regmap and regulator_disable_regmap operations and have
rdev->use_count == 0 when no consumers are attached,
regulator_init_complete_work_function() -> regulator_late_cleanup() in
drivers/regulator/core.c will turn off every enabled rail:

drivers/regulator/core.c:regulator_late_cleanup() {
	...
	if (c && c->always_on)
		return 0;

	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
		return 0;

	regulator_lock(rdev);

	if (rdev->use_count)
		goto unlock;

	/* If reading the status failed, assume that it's off. */
	if (_regulator_is_enabled(rdev) <= 0)
		goto unlock;

	if (have_full_constraints()) {
		/* We log since this may kill the system if it goes
		 * wrong.
		 */
		rdev_info(rdev, "disabling\n");
		ret = _regulator_do_disable(rdev);
	...
}

Without regulator-always-on or consumer supplies on core system rails in
mt6392.dtsi or pumpkin-common.dtsi, won't regulator_late_cleanup() disable
active system supplies such as vproc, vcore, vsys, vm, vxo22, vio18, and
vio28 after late init and power off or hang the Pumpkin boards?

[ ... ]

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