[PATCH 0/2] regulator: mt6363: Fix the regmap value width and the VSRAM LDO registers

[PATCH 0/2] regulator: mt6363: Fix the regmap value width and the VSRAM LDO registers

[GIBEOM LEE (이기범)]

These patches fix two problems in the MT6363 regulator driver that can be
seen from the code. They are on top of regulator for-next, where the driver
is the same as in master and v7.2.12.

Patch 1: the registers have 8 bits but the regmap has 16-bit values, so
every write also writes the next register. For example, disabling one of
VS2 and VBUCK1-7 also disables VS1, VS3 and VSRAM_{DIGRF,MDFE,MODEM}. The
MT6373 driver on the list has 16-bit values too, so its next version will
want the same change.

Patch 2: VSRAM_{CPUB,CPUM,CPUL,APU} use the enable and low power registers
of the BUCK1 group instead of their own, so disabling one of them disables
VS1.

Testing: none of this has run on hardware, because the bootloader of my
board (Lenovo TB375FC, MT6897) is still locked. Each patch builds with W=1.
The driver file also ran unchanged in user space, against a model of the
registers made from MediaTek's header and driver, before and after the
patches:

 - 16 of the 33 checks of the model fail before and pass after. The 9 that
   still fail are about the VEMC voltage callbacks and the overcurrent
   handling, for which I have fixes that I would like to test first.
 - ASan reports a stack buffer overflow in the BUCK_TOP key write before
   patch 1, and nothing after it.
 - The resolved table of the 35 rails differs from MediaTek's driver in 8
   entries before patch 2, and in none after it.

The model comes from MediaTek's sources, so it does not show how the chip
really behaves. A Tested-by from anyone with an MT8196 or MT6991 board would
be very welcome.

How this was made: Claude (claude-opus-5-5, an AI coding assistant) found
both problems while comparing the driver with MediaTek's driver in the
OnePlus MT6897 kernel, during a longer session of porting mainline Linux to
this tablet. It wrote the patches, their changelogs and this letter, and it
ran the checks above. I reviewed the result before signing off.

GIBEOM LEE (이기범) (2):
  regulator: mt6363: Use 8-bit regmap values
  regulator: mt6363: Fix the registers of the VSRAM CPU and APU LDOs

 drivers/regulator/mt6363-regulator.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

[PATCH 1/2] regulator: mt6363: Use 8-bit regmap values

[GIBEOM LEE (이기범)]

The registers of the MT6363 are 8 bits wide, but the regmap has 16-bit
values. On SPMI that makes every access two bytes long: a read also returns
the next register in the upper byte, and a write also writes the next
register, with 0 or with what a read-modify-write found there.

The next register is rarely related to the one the driver means, so this
goes wrong in several places:

 - mt6363_vreg_disable_setclr() writes the mask to the CLR register and 0
   to the one after it. For VS2 and VBUCK1-7 that is the enable register of
   VS1, VS3 and VSRAM_{DIGRF,MDFE,MODEM}, so disabling any of them disables
   those five as well. For those five it is the low power register of VS2
   and VBUCK1-7.
 - mt6363_regulator_set_load() zeroes the register after OP_CFG and the one
   after the third OP_EN register.
 - A read-modify-write of the low power register of a buck writes back to
   its SET register what it read from it. If SET reads like the register
   itself, that sets again the bit that was just cleared.
 - TOP_TRAP is read together with MISC_TOP_INT_CON0, so the VEMC voltage
   callbacks fail with -EINVAL once a MISC interrupt is enabled.
 - regmap_bulk_write() takes a number of registers. The sizeof() of the u16
   keys of TMA and BUCK_TOP asks for two 16-bit registers, so it reads past
   the key on the stack and writes that to the two registers after the key.

MediaTek's regulator driver uses the regmap of their MFD driver, which has
8-bit values (mt6363-regulator.c and mtk-spmi-pmic.c in the OnePlus MT6897
kernel, OnePlusOSS android_kernel_modules_and_devicetree_oneplus_mt6897,
branch oneplus/mt6897_b_16.0.0_oneplus_pad). With 8-bit values one access
is one register, and the sizeof() of the keys is the right number of
registers.

Fixes: 3c36965df808 ("regulator: Add support for MediaTek MT6363 SPMI PMIC Regulators")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: GIBEOM LEE (이기범) <rtx4060@gmail.com>
---
 drivers/regulator/mt6363-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/mt6363-regulator.c b/drivers/regulator/mt6363-regulator.c
index aa6a8eb..cc2e841 100644
--- a/drivers/regulator/mt6363-regulator.c
+++ b/drivers/regulator/mt6363-regulator.c
@@ -811,7 +811,7 @@ static struct regmap *mt6363_spmi_register_regmap(struct device *dev)
 {
 	struct regmap_config mt6363_regmap_config = {
 		.reg_bits = 16,
-		.val_bits = 16,
+		.val_bits = 8,
 		.max_register = 0x1f90,
 		.fast_io = true,
 	};

[PATCH 2/2] regulator: mt6363: Fix the registers of the VSRAM CPU and APU LDOs

[GIBEOM LEE (이기범)]

VSRAM_CPUB, VSRAM_CPUM, VSRAM_CPUL and VSRAM_APU have a register of their
own for the enable (bit 0) and low power (bit 1) bits, which the header
defines as MT6363_RG_LDO_VSRAM_*_ADDR. The driver does not use it:
MT6363_LDO_L() takes the registers of the BUCK1 group, which is right only
for the VSRAM_{DIGRF,MDFE,MODEM} LDOs of MT6363_LDO_L_SC().

Bit 0 of the BUCK1 enable register is VS1 and bit 1 of the BUCK1 low power
register is VS3. So disabling one of these LDOs disables VS1, their state
is the state of VS1, and asking for their low power mode puts VS3 in it.
The LDOs themselves are never switched.

Use the register of the LDO, as MediaTek's driver does.

Fixes: 3c36965df808 ("regulator: Add support for MediaTek MT6363 SPMI PMIC Regulators")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: GIBEOM LEE (이기범) <rtx4060@gmail.com>
---
 drivers/regulator/mt6363-regulator.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/mt6363-regulator.c b/drivers/regulator/mt6363-regulator.c
index cc2e841..c5de049 100644
--- a/drivers/regulator/mt6363-regulator.c
+++ b/drivers/regulator/mt6363-regulator.c
@@ -147,7 +147,7 @@ struct mt6363_regulator_info {
 }
 
 #define MT6363_LDO_LINEAR_OPS(match, vreg, in_sup, vops, min, max,	\
-			      step, buck_reg, ocp_intn)			\
+			      step, en_reg, lp_reg, ocp_intn)		\
 [MT6363_ID_##vreg] = {							\
 	.desc = {							\
 		.name = match,						\
@@ -160,13 +160,13 @@ struct mt6363_regulator_info {
 		.n_voltages = (max - min) / step + 1,			\
 		.min_uV = min,						\
 		.uV_step = step,					\
-		.enable_reg = MT6363_RG_##buck_reg##_EN_ADDR,		\
+		.enable_reg = en_reg,					\
 		.enable_mask = BIT(MT6363_RG_LDO_##vreg##_EN_BIT),	\
 		.vsel_reg = MT6363_RG_LDO_##vreg##_VOSEL_ADDR,		\
 		.vsel_mask = MT6363_RG_LDO_##vreg##_VOSEL_MASK,		\
 		.of_map_mode = mt6363_map_mode,				\
 	},								\
-	.lp_mode_reg = MT6363_RG_##buck_reg##_LP_ADDR,			\
+	.lp_mode_reg = lp_reg,						\
 	.lp_mode_mask = BIT(MT6363_RG_LDO_##vreg##_LP_BIT),		\
 	.hw_lp_mode_reg = MT6363_LDO_##vreg##_HW_LP_MODE,		\
 	.hw_lp_mode_mask = 0x4,						\
@@ -176,12 +176,15 @@ struct mt6363_regulator_info {
 #define MT6363_LDO_L_SC(match, vreg, inp, min, max, step, buck_reg,	\
 			ocp_intn)					\
 	MT6363_LDO_LINEAR_OPS(match, vreg, inp, mt6363_vreg_setclr_ops,	\
-			      min, max, step, buck_reg, ocp_intn)
+			      min, max, step,				\
+			      MT6363_RG_##buck_reg##_EN_ADDR,		\
+			      MT6363_RG_##buck_reg##_LP_ADDR, ocp_intn)
 
-#define MT6363_LDO_L(match, vreg, inp, min, max, step, buck_reg,	\
-		     ocp_intn)						\
+#define MT6363_LDO_L(match, vreg, inp, min, max, step, ocp_intn)	\
 	MT6363_LDO_LINEAR_OPS(match, vreg, inp, mt6363_ldo_linear_ops,	\
-			      min, max, step, buck_reg, ocp_intn)
+			      min, max, step,				\
+			      MT6363_RG_LDO_##vreg##_ADDR,		\
+			      MT6363_RG_LDO_##vreg##_ADDR, ocp_intn)
 
 #define MT6363_LDO_LINEAR_CAL_OPS(match, vreg, in_sup, vops, vrnum,	\
 				  ocp_intn)				\
@@ -754,10 +757,10 @@ static struct mt6363_regulator_info mt6363_regulators[] = {
 	MT6363_LDO_VT("vio0p75", VIO075, "vs1-ldo1", 5, 36),
 	MT6363_LDO_VT("vio18", VIO18, "vs1-ldo2", 3, 35),
 	MT6363_LDO_VT("vm18", VM18, "vs1-ldo1", 4, 40),
-	MT6363_LDO_L("vsram-apu", VSRAM_APU, "vs3-ldo1", 400000, 1193750, 6250, BUCK1, 30),
-	MT6363_LDO_L("vsram-cpub", VSRAM_CPUB, "vs2-ldo1", 400000, 1193750, 6250, BUCK1, 27),
-	MT6363_LDO_L("vsram-cpum", VSRAM_CPUM, "vs2-ldo1", 400000, 1193750, 6250, BUCK1, 28),
-	MT6363_LDO_L("vsram-cpul", VSRAM_CPUL, "vs2-ldo2", 400000, 1193750, 6250, BUCK1, 29),
+	MT6363_LDO_L("vsram-apu", VSRAM_APU, "vs3-ldo1", 400000, 1193750, 6250, 30),
+	MT6363_LDO_L("vsram-cpub", VSRAM_CPUB, "vs2-ldo1", 400000, 1193750, 6250, 27),
+	MT6363_LDO_L("vsram-cpum", VSRAM_CPUM, "vs2-ldo1", 400000, 1193750, 6250, 28),
+	MT6363_LDO_L("vsram-cpul", VSRAM_CPUL, "vs2-ldo2", 400000, 1193750, 6250, 29),
 	MT6363_LDO_L_SC("vsram-digrf", VSRAM_DIGRF, "vs3-ldo1", 400000, 1193750, 6250, BUCK1, 23),
 	MT6363_LDO_L_SC("vsram-mdfe", VSRAM_MDFE, "vs3-ldo1", 400000, 1193750, 6250, BUCK1, 24),
 	MT6363_LDO_L_SC("vsram-modem", VSRAM_MODEM, "vs3-ldo2", 400000, 1193750, 6250, BUCK1, 25),