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

[Kamal Wadhwa]

On Wed, Apr 22, 2026 at 12:54:33PM +0200, Konrad Dybcio wrote:
> 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
> 
> [...]

Sorry, will fix this in next version.

> 
> > +/**
> > + * 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
> 
> [...]

Sure, will fix this in next version.

> 
> > +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
> 
> [...]

Will fix this in next version.

>
> > +	/*
> > +	 * 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
> 
will fix it in next version.

Regards,
Kamal