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

[Konrad Dybcio]

On 4/20/26 5:43 PM, 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 | 179 ++++++++++++++++++++++++++++++++
>  1 file changed, 179 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 85fbf10f74bb3393071bc65681356312f27b7527..1add15d73cac67ad8c0b45aaad6fb2ae9b388180 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -14,6 +14,7 @@
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/regulator/of_regulator.h>
> +#include <linux/bits.h>

Please sort the includes

[...]

> +/**
> + * rpmh_regulator_read_data() - read data from RPMh
> + * @vreg:		Pointer to the RPMh regulator
> + * @cmd:		Pointer to the RPMh command struct to readback data
> + *
> + * Return: 0 on success, or a negative error number on failure
> + */
> +static int rpmh_regulator_read_data(struct rpmh_vreg *vreg, struct tcs_cmd *cmd)
> +{
> +	return rpmh_read(vreg->dev, cmd);
> +}

Since this is a wrapper of a oneliner, perhaps let's just
ctrl-x + ctrl-v it into the usage

[...]

> +static int rpmh_regulator_determine_initial_status(struct rpmh_vreg *vreg)
> +{
> +	struct tcs_cmd cmd = {
> +		.addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
> +	};
> +	int ret, pmic_mode, mode;
> +	int sts = 0;

Drop the initialization, it's always initialized before usage

[...]

> +	/*
> +	 * NOTE: Since BOB4 BYPASS_MODE value = 0 we cannot confirm if that BOB
> +	 * regulator has been sent into bypass mode by bootloader or if bootloader
> +	 * just has not requested for any mode voting. Due this limitation, we
> +	 * must check if the read pmic_mode value is non-zero before comparing it
> +	 * to bypass mode value. This also is needed to avoid setting BYPASS status
> +	 * for LDOs which dont support bypass mode, and have the pmic_bypass_mode
> +	 * uninitialized value as zero in the vreg hw data. For such cases assume
> +	 * lowest mode/status, if pmic_mode is zero, to allow for mode voting.
> +	 */
> +	if (!pmic_mode) {
> +		for (mode = REGULATOR_MODE_STANDBY; mode > REGULATOR_MODE_INVALID; mode >>= 1) {
> +			if (vreg->hw_data->pmic_mode_map[mode] >= 0) {
> +				vreg->mode = mode;
> +				break;
> +			}
> +		}
> +
> +		vreg->status = regulator_mode_to_status(vreg->mode);
> +		return 0;

nit: since you did so above, please keep a \n above the return statements

Konrad