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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Suspicious register offset for SC2730_LDO_VDDCAMIO_VOL likely causing hardware corruption.
- [Medium] VDDSIM0 and VDDSIM1 regulators are defined in the ID enum but missing from the implementation.
--

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

regulator: Add regulator driver for Unisoc SC2730 PMIC

This commit adds a regulator driver for the Spreadtrum/Unisoc SC2730 PMIC,
used with SoCs like the UMS512 and UMS9230. It is based on a downstream
driver provided by Unisoc and the existing SC2731 driver.

Link: https://github.com/MotorolaMobilityLLC/kernel-sprd/commit/30be0ddfe6b9a877fc9c328fbd2bae84e645eb31
Link: 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..18c1280330ef9
> --- /dev/null
> +++ b/drivers/regulator/sc2730-regulator.c

[ ... ]

> +/*
> + * 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)
> +#define SC2730_LDO_VDDWCN_VOL		(SC2730_REGULATOR_BASE + 0x120)

Could this offset be missing a leading 1? This offset places the register
in the pre-DCDC space rather than the LDO block that starts at 0x100.

When adjusting the VDDCAMIO voltage, could writing to 0x28 corrupt
unrelated PMIC registers instead of setting the intended voltage?

[ ... ]

> +enum sc2730_regulator_id {

[ ... ]

> +	SC2730_LDO_VDDCAMMOT,
> +	SC2730_LDO_VDDSIM0,
> +	SC2730_LDO_VDDSIM1,
> +	SC2730_LDO_VDDSIM2,
> +	SC2730_LDO_VDDEMMCCORE,

Are VDDSIM0 and VDDSIM1 supposed to be supported by this driver?
They are declared in the enumeration here, but are missing from the
regulators[] array implementation below.

There is a 36-byte gap between VDDCAMMOT_PD (0x170) and VDDSIM2_PD (0x194)
in the enablement definitions, which would perfectly fit a standard 12-byte
stride for two missing LDOs.

If these regulators are managed directly by a modem or intentionally
excluded, should they be removed from the enumeration to avoid unused
identifiers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-sc2730-regulators-v2-0-9a5b3a7b1e49@abscue.de?part=3