Message ID | 20220125024652.129568-1-marex@denx.de |
---|---|
State | Accepted |
Commit | 1b7f00eb1f239973ef459a3608ab5335011c6ab5 |
Delegated to: | Stefano Babic |
Headers | show |
Series | regulator: bd718x7: Bypass bogus warnings | expand |
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
> 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
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);
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(-)