Currently, when `rpmh_regulator_set_mode_bypass()` helper function
is called to set bypass mode, it sends PMIC4's BOB bypass mode
value for even if its a PMIC5 BOB.
To fix this, introduce new hw_data parameters:
- `bypass_supported` to check if bypass is supported.
- `pmic_bypass_mode` to store bypass mode value.
Use these 2 parameters to send correct PMIC bypass mode that
corresponds to PMIC4/5 BOB regulators from the helper function.
Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
drivers/regulator/qcom-rpmh-regulator.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
index 0a561f1d94523bf479f48e8c2062f79cf64f5b5f..947fb5241233c92eaeda974b1b64d227d5946a59 100644
--- a/drivers/regulator/qcom-rpmh-regulator.c
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -111,6 +111,9 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
* @hpm_min_load_uA: Minimum load current in microamps that requires
* high power mode (HPM) operation. This is used
* for LDO hardware type regulators only.
+ * @pmic_bypass_mode: The PMIC bypass mode value. This is only
+ * used if bypass_supported == true.
+ * @bypass_supported: Indicates if bypass mode is supported
* @pmic_mode_map: Array indexed by regulator framework mode
* containing PMIC hardware modes. Must be large
* enough to index all framework modes supported
@@ -125,6 +128,8 @@ struct rpmh_vreg_hw_data {
int n_linear_ranges;
int n_voltages;
int hpm_min_load_uA;
+ int pmic_bypass_mode;
+ bool bypass_supported;
const int *pmic_mode_map;
unsigned int (*of_map_mode)(unsigned int mode);
};
@@ -310,10 +315,13 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
if (pmic_mode < 0)
return pmic_mode;
- if (bypassed)
- cmd.data = PMIC4_BOB_MODE_PASS;
- else
+ if (bypassed) {
+ if(!vreg->hw_data->bypass_supported)
+ return -EINVAL;
+ cmd.data = vreg->hw_data->pmic_bypass_mode;
+ } else {
cmd.data = pmic_mode;
+ }
return rpmh_regulator_send_request(vreg, &cmd, true);
}
@@ -767,6 +775,8 @@ static const struct rpmh_vreg_hw_data pmic4_bob = {
},
.n_linear_ranges = 1,
.n_voltages = 84,
+ .bypass_supported = true,
+ .pmic_bypass_mode = PMIC4_BOB_MODE_PASS,
.pmic_mode_map = pmic_mode_map_pmic4_bob,
.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
};
@@ -975,6 +985,8 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
},
.n_linear_ranges = 1,
.n_voltages = 32,
+ .bypass_supported = true,
+ .pmic_bypass_mode = PMIC5_BOB_MODE_PASS,
.pmic_mode_map = pmic_mode_map_pmic5_bob,
.of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
};
--
2.25.1On 10/21/25 11:08 PM, Kamal Wadhwa wrote: > Currently, when `rpmh_regulator_set_mode_bypass()` helper function > is called to set bypass mode, it sends PMIC4's BOB bypass mode > value for even if its a PMIC5 BOB. > > To fix this, introduce new hw_data parameters: > - `bypass_supported` to check if bypass is supported. > - `pmic_bypass_mode` to store bypass mode value. > > Use these 2 parameters to send correct PMIC bypass mode that > corresponds to PMIC4/5 BOB regulators from the helper function. > > Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> Konrad
On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote: > Currently, when `rpmh_regulator_set_mode_bypass()` helper function > is called to set bypass mode, it sends PMIC4's BOB bypass mode > value for even if its a PMIC5 BOB. > > To fix this, introduce new hw_data parameters: > - `bypass_supported` to check if bypass is supported. > - `pmic_bypass_mode` to store bypass mode value. > > Use these 2 parameters to send correct PMIC bypass mode that > corresponds to PMIC4/5 BOB regulators from the helper function. > > Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com> Also missing the Fixes tag. > --- > drivers/regulator/qcom-rpmh-regulator.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > -- With best wishes Dmitry
On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> value for even if its a PMIC5 BOB.
The universe will end, the Sun will explode and Ragnarök will be upon
us. Please describe the issue, why sending bypass value is bad.
>
> To fix this, introduce new hw_data parameters:
> - `bypass_supported` to check if bypass is supported.
> - `pmic_bypass_mode` to store bypass mode value.
>
> Use these 2 parameters to send correct PMIC bypass mode that
> corresponds to PMIC4/5 BOB regulators from the helper function.
>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---
> drivers/regulator/qcom-rpmh-regulator.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index 0a561f1d94523bf479f48e8c2062f79cf64f5b5f..947fb5241233c92eaeda974b1b64d227d5946a59 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -111,6 +111,9 @@ static const struct resource_name_formats vreg_rsc_name_lookup[NUM_REGULATOR_TYP
> * @hpm_min_load_uA: Minimum load current in microamps that requires
> * high power mode (HPM) operation. This is used
> * for LDO hardware type regulators only.
> + * @pmic_bypass_mode: The PMIC bypass mode value. This is only
> + * used if bypass_supported == true.
> + * @bypass_supported: Indicates if bypass mode is supported
> * @pmic_mode_map: Array indexed by regulator framework mode
> * containing PMIC hardware modes. Must be large
> * enough to index all framework modes supported
> @@ -125,6 +128,8 @@ struct rpmh_vreg_hw_data {
> int n_linear_ranges;
> int n_voltages;
> int hpm_min_load_uA;
> + int pmic_bypass_mode;
> + bool bypass_supported;
> const int *pmic_mode_map;
> unsigned int (*of_map_mode)(unsigned int mode);
> };
> @@ -310,10 +315,13 @@ static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> if (pmic_mode < 0)
> return pmic_mode;
>
> - if (bypassed)
> - cmd.data = PMIC4_BOB_MODE_PASS;
> - else
> + if (bypassed) {
> + if(!vreg->hw_data->bypass_supported)
> + return -EINVAL;
This is redundant, the regulators which don't support bypass should not
be using rpmh_regulator_vrm_bypass_ops.
> + cmd.data = vreg->hw_data->pmic_bypass_mode;
> + } else {
> cmd.data = pmic_mode;
> + }
Can we extend pmic_mode_map to include the value for bypass?
>
> return rpmh_regulator_send_request(vreg, &cmd, true);
> }
> @@ -767,6 +775,8 @@ static const struct rpmh_vreg_hw_data pmic4_bob = {
> },
> .n_linear_ranges = 1,
> .n_voltages = 84,
> + .bypass_supported = true,
> + .pmic_bypass_mode = PMIC4_BOB_MODE_PASS,
> .pmic_mode_map = pmic_mode_map_pmic4_bob,
> .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> };
> @@ -975,6 +985,8 @@ static const struct rpmh_vreg_hw_data pmic5_bob = {
> },
> .n_linear_ranges = 1,
> .n_voltages = 32,
> + .bypass_supported = true,
> + .pmic_bypass_mode = PMIC5_BOB_MODE_PASS,
> .pmic_mode_map = pmic_mode_map_pmic5_bob,
> .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
> };
>
> --
> 2.25.1
>
--
With best wishes
Dmitry
On 10/22/25 12:23 AM, Dmitry Baryshkov wrote: > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote: >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function >> is called to set bypass mode, it sends PMIC4's BOB bypass mode >> value for even if its a PMIC5 BOB. > > The universe will end, the Sun will explode and Ragnarök will be upon > us. Please describe the issue, why sending bypass value is bad. I think you misread, it sends the magic value which corresponds to BYPASS, but one that worked for the previous generation Konrad
On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote: > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote: > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote: > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode > >> value for even if its a PMIC5 BOB. > > > > The universe will end, the Sun will explode and Ragnarök will be upon > > us. Please describe the issue, why sending bypass value is bad. > > I think you misread, it sends the magic value which corresponds > to BYPASS, but one that worked for the previous generation I just wanted to point out that the commit message makes a statement that it sends some value. It should document, why the sent value is bad. -- With best wishes Dmitry
On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote: > On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote: > > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote: > > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote: > > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function > > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode > > >> value for even if its a PMIC5 BOB. > > > The universe will end, the Sun will explode and Ragnarök will be upon > > > us. Please describe the issue, why sending bypass value is bad. > > I think you misread, it sends the magic value which corresponds > > to BYPASS, but one that worked for the previous generation > I just wanted to point out that the commit message makes a statement > that it sends some value. It should document, why the sent value is bad. It seems fairly clear to me from the above that the driver is sending the value for the wrong device type which is something so obviously wrong I'm not sure it requires further explanation.
On Wed, Oct 22, 2025 at 04:15:51PM +0100, Mark Brown wrote: > On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote: > > On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote: > > > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote: > > > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote: > > > > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function > > > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode > > > >> value for even if its a PMIC5 BOB. > > > > > The universe will end, the Sun will explode and Ragnarök will be upon > > > > us. Please describe the issue, why sending bypass value is bad. > > > > I think you misread, it sends the magic value which corresponds > > > to BYPASS, but one that worked for the previous generation > > > I just wanted to point out that the commit message makes a statement > > that it sends some value. It should document, why the sent value is bad. > > It seems fairly clear to me from the above that the driver is sending > the value for the wrong device type which is something so obviously > wrong I'm not sure it requires further explanation. Okay. I'm sorry if I'm overreacting. The bypass_supported field still needs to go away in my opinion. -- With best wishes Dmitry
On Thu, Oct 23, 2025 at 02:37:07PM +0300, Dmitry Baryshkov wrote:
> On Wed, Oct 22, 2025 at 04:15:51PM +0100, Mark Brown wrote:
> > On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote:
> > > > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> > > > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> >
> > > > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> > > > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> > > > >> value for even if its a PMIC5 BOB.
> >
> > > > > The universe will end, the Sun will explode and Ragnarök will be upon
> > > > > us. Please describe the issue, why sending bypass value is bad.
> >
> > > > I think you misread, it sends the magic value which corresponds
> > > > to BYPASS, but one that worked for the previous generation
> >
> > > I just wanted to point out that the commit message makes a statement
> > > that it sends some value. It should document, why the sent value is bad.
> >
> > It seems fairly clear to me from the above that the driver is sending
> > the value for the wrong device type which is something so obviously
> > wrong I'm not sure it requires further explanation.
>
> Okay. I'm sorry if I'm overreacting.
>
> The bypass_supported field still needs to go away in my opinion.
@Dmitry - one way to avoid it is if i re-use `.pmic_bypass_mode` and
keep it `= -EINVAL` for the checking if the bypass mode is not
supported? and drop the `.bypass_supported`.
But do note that currently only BOB type regulator supports bypass
mode, and this above change will also require modifying all of the
existing (and future) configs for regulator types that do not support
bypass control.
In all below 28 structures we will have to define
`.pmic_bypass_mode = -EINVAL`
static const struct rpmh_vreg_hw_data pmic4_pldo = {
static const struct rpmh_vreg_hw_data pmic4_pldo_lv = {
static const struct rpmh_vreg_hw_data pmic4_nldo = {
static const struct rpmh_vreg_hw_data pmic4_hfsmps3 = {
static const struct rpmh_vreg_hw_data pmic4_ftsmps426 = {
static const struct rpmh_vreg_hw_data pmic4_lvs = {
static const struct rpmh_vreg_hw_data pmic5_pldo = {
static const struct rpmh_vreg_hw_data pmic5_pldo_lv = {
static const struct rpmh_vreg_hw_data pmic5_pldo515_mv = {
static const struct rpmh_vreg_hw_data pmic5_pldo502 = {
static const struct rpmh_vreg_hw_data pmic5_pldo502ln = {
static const struct rpmh_vreg_hw_data pmic5_nldo = {
static const struct rpmh_vreg_hw_data pmic5_nldo515 = {
static const struct rpmh_vreg_hw_data pmic5_nldo502 = {
static const struct rpmh_vreg_hw_data pmic5_hfsmps510 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps510 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps520 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps525 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps527 = {
static const struct rpmh_vreg_hw_data pmic5_hfsmps515 = {
static const struct rpmh_vreg_hw_data pmic5_hfsmps515_1 = {
static const struct rpmh_vreg_hw_data pmic5_nldo530 = {
static const struct rpmh_vreg_hw_data pmic5_pldo530_mvp150 = {
static const struct rpmh_vreg_hw_data pmic5_pldo530_mvp300 = {
static const struct rpmh_vreg_hw_data pmic5_pldo530_mvp600 = {
static const struct rpmh_vreg_hw_data pmic5_ftsmps530 = {
while in the current patch i dont need to touch any of these above
structures and just add new property and define it whereever
`bypass_supported` is set to true.
i.e just change these 2 bob nodes only.
static const struct rpmh_vreg_hw_data pmic5_bob = {
static const struct rpmh_vreg_hw_data pmic4_bob = {
Please suggest, if we can do this in a better way.
Regards,
Kamal
On Fri, Oct 24, 2025 at 01:33:58PM +0530, Kamal Wadhwa wrote:
> On Thu, Oct 23, 2025 at 02:37:07PM +0300, Dmitry Baryshkov wrote:
> > On Wed, Oct 22, 2025 at 04:15:51PM +0100, Mark Brown wrote:
> > > On Wed, Oct 22, 2025 at 06:11:46PM +0300, Dmitry Baryshkov wrote:
> > > > On Wed, Oct 22, 2025 at 04:58:15PM +0200, Konrad Dybcio wrote:
> > > > > On 10/22/25 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > On Wed, Oct 22, 2025 at 02:38:53AM +0530, Kamal Wadhwa wrote:
> > >
> > > > > >> Currently, when `rpmh_regulator_set_mode_bypass()` helper function
> > > > > >> is called to set bypass mode, it sends PMIC4's BOB bypass mode
> > > > > >> value for even if its a PMIC5 BOB.
> > >
> > > > > > The universe will end, the Sun will explode and Ragnarök will be upon
> > > > > > us. Please describe the issue, why sending bypass value is bad.
> > >
> > > > > I think you misread, it sends the magic value which corresponds
> > > > > to BYPASS, but one that worked for the previous generation
> > >
> > > > I just wanted to point out that the commit message makes a statement
> > > > that it sends some value. It should document, why the sent value is bad.
> > >
> > > It seems fairly clear to me from the above that the driver is sending
> > > the value for the wrong device type which is something so obviously
> > > wrong I'm not sure it requires further explanation.
> >
> > Okay. I'm sorry if I'm overreacting.
> >
> > The bypass_supported field still needs to go away in my opinion.
>
> @Dmitry - one way to avoid it is if i re-use `.pmic_bypass_mode` and
> keep it `= -EINVAL` for the checking if the bypass mode is not
> supported? and drop the `.bypass_supported`.
You don't need .bypass_supported because the regulators that don't
support bypass don't have .set_bypass callback in their ops and as such
it is impossible to get the vreg->bypassed flag to be set.
> But do note that currently only BOB type regulator supports bypass
> mode, and this above change will also require modifying all of the
> existing (and future) configs for regulator types that do not support
> bypass control.
[...]
>
> while in the current patch i dont need to touch any of these above
> structures and just add new property and define it whereever
> `bypass_supported` is set to true.
>
> i.e just change these 2 bob nodes only.
>
> static const struct rpmh_vreg_hw_data pmic5_bob = {
> static const struct rpmh_vreg_hw_data pmic4_bob = {
>
> Please suggest, if we can do this in a better way.
Don't change any of the nodes, the necessary changes are already in
place. Just fix the value being programmed for PMIC5.
--
With best wishes
Dmitry
© 2016 - 2026 Red Hat, Inc.