[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 at denx.de>
> Cc: Matti Vaittinen <matti.vaittinen at 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 ~~