Re: [PATCH v3 3/3] regulator: Add regulator driver for Unisoc SC2730 PMIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The voltage control register for LDO_VDDCAMIO is defined with an incorrect offset (0x28), which will corrupt unrelated PMIC hardware state when written to.
--

commit d755f6665748cda3b92f492a8c7a6cfc5194b4c6
Author: Otto Pflüger <otto.pflueger@abscue.de>

regulator: Add regulator driver for Unisoc SC2730 PMIC

Add a regulator driver for the Spreadtrum/Unisoc SC2730 PMIC, used
e.g. with the UMS512 and UMS9230 SoCs. This version of the driver is
based on a downstream driver provided by Unisoc [1][2] and the existing
SC2731 driver.

[1]: https://github.com/MotorolaMobilityLLC/kernel-sprd/commit/30be0ddfe6b9a877fc9c328fbd2bae84e645eb31
[2]: https://github.com/MotorolaMobilityLLC/kernel-sprd/blob/android-13-release-tla33/drivers/regulator/sc2730-regulator.c

> diff --git a/drivers/regulator/sc2730-regulator.c b/drivers/regulator/sc2730-regulator.c
> new file mode 100644
> index 0000000000000..9e40c6eae4943
> --- /dev/null
> +++ b/drivers/regulator/sc2730-regulator.c
> @@ -0,0 +1,411 @@
[ ... ]
> +/*
> + * SC2730 enable register
> + */
> +#define SC2730_POWER_PD_SW		(SC2730_REGULATOR_BASE + 0x01c)
> +#define SC2730_LDO_VDDRF18_PD		(SC2730_REGULATOR_BASE + 0x10c)
> +#define SC2730_LDO_VDDCAMIO_PD		(SC2730_REGULATOR_BASE + 0x118)
> +#define SC2730_LDO_VDDWCN_PD		(SC2730_REGULATOR_BASE + 0x11c)
[ ... ]
> +/*
> + * SC2730 vsel register
> + */
[ ... ]
> +#define SC2730_LDO_AVDD18_VOL		(SC2730_REGULATOR_BASE + 0x104)
> +#define SC2730_LDO_VDDRF18_VOL		(SC2730_REGULATOR_BASE + 0x110)
> +#define SC2730_LDO_VDDCAMIO_VOL		(SC2730_REGULATOR_BASE + 0x28)

Is this offset for SC2730_LDO_VDDCAMIO_VOL correct?

In this driver, LDO voltage control registers (_VOL) appear to strictly follow
their power-down registers (_PD) at a +4 byte offset (e.g., VDDRF18_PD is 0x10c
and VDDRF18_VOL is 0x110).

SC2730_LDO_VDDCAMIO_VOL is defined as (SC2730_REGULATOR_BASE + 0x28). This
offset is outside the LDO register block (which spans 0x100 to 0x210) and
sits between the global power register (0x1c) and the DCDC block (0x44). Will
writing to 0x28 overwrite an unrelated PMIC register instead of setting the
LDO voltage?

Additionally, VDDCAMIO_PD is defined at 0x118, which leaves no room for a
+4 byte _VOL register because VDDWCN_PD occupies 0x11c. Should VDDCAMIO_PD be
0x114 and VDDCAMIO_VOL be 0x118 to fit the established 8-byte register stride?

> +#define SC2730_LDO_VDDWCN_VOL		(SC2730_REGULATOR_BASE + 0x120)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-sc2730-regulators-v3-0-5bf0e02507e3@abscue.de?part=3