Re: [PATCH v3 3/4] regulator: qcom-rpmh: readback voltage/bypass/mode/status set during bootup

[Konrad Dybcio]

On 4/7/26 6:43 AM, Kamal Wadhwa wrote:
> Currently, during regulator registration, regulator framework sends an
> unnecessary `min-microvolts` request for the rpmh-regulator device. This
> happens because in current design, we do not have a way to readback the
> voltage settings that was set during the bootloader stage.
> 
> Fix this by using the rpmh_read() API to read the regulator voltage
> settings done during boot and make it available to regulator framework
> from the very first read after the bootup.
> 
> Also use this API to read the status/mode/bypass settings as well. This
> will provide the regulator framework a sense of the initial settings
> done by bootloader and thus preventing any redundents writes for any
> setting post bootup incase the same setting was already applied during
> bootup.
> 
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@xxxxxxxxxxxxxxxx>
> ---
>  drivers/regulator/qcom-rpmh-regulator.c | 178 ++++++++++++++++++++++++++++++++
>  1 file changed, 178 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 85fbf10f74bb3393071bc65681356312f27b7527..8e1c576b718b595bbbff7f5fa76de84d4e90f3bb 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -61,8 +61,13 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
>  };
>  
>  #define RPMH_REGULATOR_REG_VRM_VOLTAGE		0x0
> +#define RPMH_REGULATOR_VOLTAGE_MASK		0x1FFF

GENMASK(12, 0)

> +
>  #define RPMH_REGULATOR_REG_ENABLE		0x4
> +#define RPMH_REGULATOR_ENABLE_MASK		0x1

BIT(0)

> +
>  #define RPMH_REGULATOR_REG_VRM_MODE		0x8
> +#define RPMH_REGULATOR_MODE_MASK		0x7

GENMASK(2, 0)

[...]

>  static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
>  {
>  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +	int ret, uV = 0;
> +
> +	if (vreg->voltage_selector < 0) {

This assumes that the voltage selector can't change due to an
intervention from say ADSP - is that true, as far as the _read() lets
us know?

But I suppose we can't know about __every__ change since they could
happen without a notification to HLOS and it's probably much saner to
stick to what Linux believes is set on the hw..

[...]

> +	sts = cmd.data & RPMH_REGULATOR_ENABLE_MASK;
> +	if (!sts) {
> +		vreg->status = REGULATOR_STATUS_OFF;
> +
> +		return 0;
> +	}
> +
> +	if (vreg->hw_data->regulator_type == XOB) {
> +		vreg->status = sts ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF;

The 'else' case is unreachable, since you return early if !sts beforehand


Otherwise LGTM

Konrad