[PATCH] regulator: bd718x7: Bypass bogus warnings

[PATCH] regulator: bd718x7: Bypass bogus warnings

[Marek Vasut]

When regulator consumer attempts to set enabled DVS regulator voltage,
the driver aborts with "Only DVS bucks can be changed when enabled".
In case the regulator is already set to specified voltage, do nothing
instead of failing outright.

When regulator consumer attempts to set enables regulator which cannot
be controlled because it is already always enabled, the driver aborts
with -EINVAL. Again, do nothing in such case and return 0, because the
request is really fulfilled, the regulator is enabled.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/power/regulator/bd71837.c | 69 ++++++++++++++++---------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/drivers/power/regulator/bd71837.c b/drivers/power/regulator/bd71837.c
index 74011d62985..d4f8da80ad7 100644
--- a/drivers/power/regulator/bd71837.c
+++ b/drivers/power/regulator/bd71837.c
@@ -306,7 +306,7 @@ static int bd71837_set_enable(struct udevice *dev, bool enable)
 	 * reseted to snvs state. Hence we can't set the state here.
 	 */
 	if (plat->enablemask == HW_STATE_CONTROL)
-		return -EINVAL;
+		return enable ? 0 : -EINVAL;
 
 	if (enable)
 		val = plat->enablemask;
@@ -315,6 +315,38 @@ static int bd71837_set_enable(struct udevice *dev, bool enable)
 			       val);
 }
 
+static int bd71837_get_value(struct udevice *dev)
+{
+	unsigned int reg, range;
+	unsigned int tmp;
+	struct bd71837_plat *plat = dev_get_plat(dev);
+	int i;
+
+	reg = pmic_reg_read(dev->parent, plat->volt_reg);
+	if (((int)reg) < 0)
+		return reg;
+
+	range = reg & plat->rangemask;
+
+	reg &= plat->volt_mask;
+	reg >>= ffs(plat->volt_mask) - 1;
+
+	for (i = 0; i < plat->numranges; i++) {
+		struct bd71837_vrange *r = &plat->ranges[i];
+
+		if (plat->rangemask && ((plat->rangemask & range) !=
+		    r->rangeval))
+			continue;
+
+		if (!vrange_find_value(r, reg, &tmp))
+			return tmp;
+	}
+
+	pr_err("Unknown voltage value read from pmic\n");
+
+	return -EINVAL;
+}
+
 static int bd71837_set_value(struct udevice *dev, int uvolt)
 {
 	unsigned int sel;
@@ -330,6 +362,9 @@ static int bd71837_set_value(struct udevice *dev, int uvolt)
 	 */
 	if (!plat->dvs)
 		if (bd71837_get_enable(dev)) {
+			/* If the value is already set, skip the warning. */
+			if (bd71837_get_value(dev) == uvolt)
+				return 0;
 			pr_err("Only DVS bucks can be changed when enabled\n");
 			return -EINVAL;
 		}
@@ -365,38 +400,6 @@ static int bd71837_set_value(struct udevice *dev, int uvolt)
 			       plat->rangemask, sel);
 }
 
-static int bd71837_get_value(struct udevice *dev)
-{
-	unsigned int reg, range;
-	unsigned int tmp;
-	struct bd71837_plat *plat = dev_get_plat(dev);
-	int i;
-
-	reg = pmic_reg_read(dev->parent, plat->volt_reg);
-	if (((int)reg) < 0)
-		return reg;
-
-	range = reg & plat->rangemask;
-
-	reg &= plat->volt_mask;
-	reg >>= ffs(plat->volt_mask) - 1;
-
-	for (i = 0; i < plat->numranges; i++) {
-		struct bd71837_vrange *r = &plat->ranges[i];
-
-		if (plat->rangemask && ((plat->rangemask & range) !=
-		    r->rangeval))
-			continue;
-
-		if (!vrange_find_value(r, reg, &tmp))
-			return tmp;
-	}
-
-	pr_err("Unknown voltage value read from pmic\n");
-
-	return -EINVAL;
-}
-
 static int bd71837_regulator_probe(struct udevice *dev)
 {
 	struct bd71837_plat *plat = dev_get_plat(dev);
-- 
2.34.1

Re: [PATCH] regulator: bd718x7: Bypass bogus warnings

[Vaittinen, Matti]

Hi dee Ho peeps!

On 1/25/22 04:46, Marek Vasut wrote:
> When regulator consumer attempts to set enabled DVS regulator voltage,
> the driver aborts with "Only DVS bucks can be changed when enabled".
> In case the regulator is already set to specified voltage, do nothing
> instead of failing outright.
> 
> When regulator consumer attempts to set enables regulator which cannot
> be controlled because it is already always enabled, the driver aborts
> with -EINVAL. Again, do nothing in such case and return 0, because the
> request is really fulfilled, the regulator is enabled.
>

I think this change makes sense. But at the same time there are risks.

I originally wrote this driver for executing some simple tests with the 
PMIC w/o loading an OS to control it. As a result, this driver uses 
static regulator configuration and assumes the run-states and things 
like whether to enable SW control for regulator ON/OFF.

What I am somewhat worried about is potential use together with full OS. 
For example at the current Linux driver for BD71837/BD71847 these 
assumptions are no longer enforced - whether to keep hardware-control 
for regulators or if the voltages are actually scaled by external 
feed-back connections can now be defined for OS via device-tree.

I am not sure what would be the best approach. We could add all the 
functionality and device-tree parsing that is done by the Linux driver - 
or we could remove all constraints/assumptions and leave sanity checks 
for users - which for sure can risk the SOC to not boot unless power to 
PMIC is cut (in practice this can mean removing/completely draining a 
battery).

To tell the truth - I have no idea where this driver is nowadays used - 
or if it is - so I can't really say what would be the sane thing to do :)


> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>   drivers/power/regulator/bd71837.c | 69 ++++++++++++++++---------------
>   1 file changed, 36 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/power/regulator/bd71837.c b/drivers/power/regulator/bd71837.c
> index 74011d62985..d4f8da80ad7 100644
> --- a/drivers/power/regulator/bd71837.c
> +++ b/drivers/power/regulator/bd71837.c
> @@ -306,7 +306,7 @@ static int bd71837_set_enable(struct udevice *dev, bool enable)
>   	 * reseted to snvs state. Hence we can't set the state here.
>   	 */
>   	if (plat->enablemask == HW_STATE_CONTROL)

The HW_STATE_CONTROL flag is hard-coded based on assumed use-case for 
the power-rails and assumed reset-target. (Eg, it's not required to 
reset to SNVS state - in which case any of the power-rails can be under 
SW control. OTOH, it may be leaving power rail to the HW control is 
requested for any power rail regardless of the SNVS / boot-criticality 
in order to force certain output state at STBY. So simple hard-coding 
the HW_STATE_CONTROL may not work well with the OS configuration.

> -		return -EINVAL;
> +		return enable ? 0 : -EINVAL;
>   
>   	if (enable)
>   		val = plat->enablemask;
> @@ -315,6 +315,38 @@ static int bd71837_set_enable(struct udevice *dev, bool enable)
>   			       val);
>   }
>   
> +static int bd71837_get_value(struct udevice *dev)
> +{
> +	unsigned int reg, range;
> +	unsigned int tmp;
> +	struct bd71837_plat *plat = dev_get_plat(dev);
> +	int i;
> +
> +	reg = pmic_reg_read(dev->parent, plat->volt_reg);
> +	if (((int)reg) < 0)
> +		return reg;
> +
> +	range = reg & plat->rangemask;
> +
> +	reg &= plat->volt_mask;
> +	reg >>= ffs(plat->volt_mask) - 1;
> +
> +	for (i = 0; i < plat->numranges; i++) {
> +		struct bd71837_vrange *r = &plat->ranges[i];
> +
> +		if (plat->rangemask && ((plat->rangemask & range) !=
> +		    r->rangeval))
> +			continue;
> +
> +		if (!vrange_find_value(r, reg, &tmp))
> +			return tmp;
> +	}
> +
> +	pr_err("Unknown voltage value read from pmic\n");
> +
> +	return -EINVAL;
> +}
> +
>   static int bd71837_set_value(struct udevice *dev, int uvolt)
>   {
>   	unsigned int sel;
> @@ -330,6 +362,9 @@ static int bd71837_set_value(struct udevice *dev, int uvolt)
>   	 */
>   	if (!plat->dvs)
>   		if (bd71837_get_enable(dev)) {
> +			/* If the value is already set, skip the warning. */
> +			if (bd71837_get_value(dev) == uvolt)
> +				return 0;

I believe this fix is always correct - thanks Marek!

As a summary - no objections to the changes, just a word of warning for 
users of this driver that there might be more than this to do...

Best Regards
	-- Matti

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

[PATCH] regulator: bd718x7: Bypass bogus warnings

[sbabic]

> When regulator consumer attempts to set enabled DVS regulator voltage,
> the driver aborts with "Only DVS bucks can be changed when enabled".
> In case the regulator is already set to specified voltage, do nothing
> instead of failing outright.
> When regulator consumer attempts to set enables regulator which cannot
> be controlled because it is already always enabled, the driver aborts
> with -EINVAL. Again, do nothing in such case and return 0, because the
> request is really fulfilled, the regulator is enabled.
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================