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 <ma...@denx.de> > Cc: Matti Vaittinen <matti.vaitti...@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 ~~