| Message ID | 1467089591-7631-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016年06月28日 12:53, Douglas Anderson wrote: > The original commit adding support for continuous voltage mode didn't > handle the regulator ramp delay properly. It treated the delay as a > fixed delay in uS despite the property being defined as uV / uS. Let's > adjust it. Luckily there appear to be no users of this ramp delay for > PWM regulators (as per grepping through device trees in linuxnext). > > Note also that the upper bound of usleep_range probably shouldn't be a > full 1 ms longer than the lower bound since I've seen plenty of hardware > with a ramp rate of ~5000 uS / uV and for small jumps the total delays > are in the tens of uS. 1000 is way too much. We'll try to be dynamic > and use 10% I'm agree with the dynamic and use 10%. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Tested-by: Caesar Wang <wxt@rock-chips.com> Tested for my rk3399 board. That's still happy work for my board. .. [ 2891.541958] pwm_regulator_set_voltage: delay=38, min-v=800000, old-v=1024000 [ 2898.188785] pwm_regulator_set_voltage: delay=13, min-v=875000, old-v=800000 [ 2898.211873] pwm_regulator_set_voltage: delay=8, min-v=925000, old-v=877000 [ 2898.312026] pwm_regulator_set_voltage: delay=21, min-v=-800000, old-v=926000 .. > --- > Note that this patch is atop Boris's recent PWM regulator fixes. If > desired it wouldn't be too hard to write it atop the old code, though > quite honestly anyone using a PWM regulator should probably be using his > new code. > > drivers/regulator/pwm-regulator.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c > index fa1c74c77bb0..de94d19f6e1f 100644 > --- a/drivers/regulator/pwm-regulator.c > +++ b/drivers/regulator/pwm-regulator.c > @@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > struct pwm_state pstate; > unsigned int diff_duty; > unsigned int dutycycle; > + int old_uV = pwm_regulator_get_voltage(rdev); > int ret; > > pwm_init_state(drvdata->pwm, &pstate); > @@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > return ret; > } > > - /* Delay required by PWM regulator to settle to the new voltage */ > - usleep_range(ramp_delay, ramp_delay + 1000); > + if (ramp_delay == 0) > + return 0; > + > + /* Ramp delay is in uV/uS. Adjust to uS and delay */ > + ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay); > + usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10)); > > return 0; > }
On Mon, Jun 27, 2016 at 9:53 PM, Douglas Anderson <dianders@chromium.org> wrote: > The original commit adding support for continuous voltage mode didn't > handle the regulator ramp delay properly. It treated the delay as a > fixed delay in uS despite the property being defined as uV / uS. Let's > adjust it. Luckily there appear to be no users of this ramp delay for > PWM regulators (as per grepping through device trees in linuxnext). My grepping agrees, though I'm sure I didn't do a very thorough job. > Note also that the upper bound of usleep_range probably shouldn't be a > full 1 ms longer than the lower bound since I've seen plenty of hardware > with a ramp rate of ~5000 uS / uV and for small jumps the total delays > are in the tens of uS. 1000 is way too much. We'll try to be dynamic > and use 10% > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > Note that this patch is atop Boris's recent PWM regulator fixes. If > desired it wouldn't be too hard to write it atop the old code, though > quite honestly anyone using a PWM regulator should probably be using his > new code. > > drivers/regulator/pwm-regulator.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c > index fa1c74c77bb0..de94d19f6e1f 100644 > --- a/drivers/regulator/pwm-regulator.c > +++ b/drivers/regulator/pwm-regulator.c > @@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > struct pwm_state pstate; > unsigned int diff_duty; > unsigned int dutycycle; > + int old_uV = pwm_regulator_get_voltage(rdev); > int ret; > > pwm_init_state(drvdata->pwm, &pstate); > @@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > return ret; > } > > - /* Delay required by PWM regulator to settle to the new voltage */ > - usleep_range(ramp_delay, ramp_delay + 1000); I was curious about the side effects of the unconditional usleep_range(ramp_delay, ...), even when ramp_delay is 0 (e.g., would we ever sleep here for up to 1ms?), but apparently the implementation optimizes the '0' case to be an unconditional, immediate wake-up. So refactoring this shouldn't have any accidental effects. > + if (ramp_delay == 0) > + return 0; > + > + /* Ramp delay is in uV/uS. Adjust to uS and delay */ > + ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay); > + usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10)); Math checks out to me. > > return 0; > } Reviewed-by: Brian Norris <briannorris@chromium.org>
On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote: > Note also that the upper bound of usleep_range probably shouldn't be a > full 1 ms longer than the lower bound since I've seen plenty of hardware > with a ramp rate of ~5000 uS / uV and for small jumps the total delays > are in the tens of uS. 1000 is way too much. We'll try to be dynamic > and use 10% Surely the upper bound here is just an upper bound and we're essentially just saying that "anything over minimum is fine" here? Though now I look at the implementation it seems it's doing something entirely unehelpful and actually trying to delay for the longest possible time which doesn't seem like what we want or what the usleep_range() API would suggest :(
On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote: > Note that this patch is atop Boris's recent PWM regulator fixes. If > desired it wouldn't be too hard to write it atop the old code, though > quite honestly anyone using a PWM regulator should probably be using his > new code. Oh, and the other thing I meant to say there: is that going anywhere? It seems like the underlying PWM interface is still unclear for some reason and there's been no comment on those bits of v3.
Mark, On Tue, Jun 28, 2016 at 11:52 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote: > >> Note also that the upper bound of usleep_range probably shouldn't be a >> full 1 ms longer than the lower bound since I've seen plenty of hardware >> with a ramp rate of ~5000 uS / uV and for small jumps the total delays >> are in the tens of uS. 1000 is way too much. We'll try to be dynamic >> and use 10% > > Surely the upper bound here is just an upper bound and we're essentially > just saying that "anything over minimum is fine" here? Though now I > look at the implementation it seems it's doing something entirely > unehelpful and actually trying to delay for the longest possible time > which doesn't seem like what we want or what the usleep_range() API > would suggest :( Yeah, you're thinking like someone who wants your computer to perform fast. That's just simply the wrong mentality to have (how dare you?). You need to think about power savings, and a sleeping computer is a computer that doesn't draw much power. Thus, you want to sleep as long as you can. :-P In all seriousness, I think the design for usleep_range() is to try to batch up wakeups to allow longer periods of sleeping and to optimize power. This you want to sleep as long as is allowable and then if you happen to wakeup for some other reason anyway then you process all the things whose minimum has passed. IIRC it was trying to get back to the good old days of only having jiffies where everyone was synchronized on the tick and you could sleep till the next tick after all the work was done. When you think of it this way then sleeping to the max makes sense. ...but it also means that you need to be careful and really not set the max too high. Of course, in practice I've found that often usleep_range() 99% of the time sleeps for the max. That can lead to some very subtle bugs if your min sleep is not long enough (!). -Doug
Hi, On Tue, Jun 28, 2016 at 11:56 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote: > >> Note that this patch is atop Boris's recent PWM regulator fixes. If >> desired it wouldn't be too hard to write it atop the old code, though >> quite honestly anyone using a PWM regulator should probably be using his >> new code. > > Oh, and the other thing I meant to say there: is that going anywhere? > It seems like the underlying PWM interface is still unclear for some > reason and there's been no comment on those bits of v3. As far as I know it is going somewhere, but it's not moving terribly quickly. At some point I think Boris got tired of posting huge patch sets and so split things in two. The first half did eventually land, but the second half is still outstanding and things seem quiet fro the last two weeks. From patchwork we have these outstanding: 9175319 [v3,01/14] pwm: Add an helper to prepare a new PWM state 9175345 [v3,02/14] pwm: Add two helpers to ease relative duty cycle ... 9175361 [v3,03/14] pwm: rockchip: Fix period and duty_cycle approximation 9175341 [v3,04/14] pwm: rockchip: Add support for hardware readout 9175335 [v3,05/14] pwm: rockchip: Avoid glitches on already running PWMs 9175337 [v3,06/14] pwm: rockchip: Add support for atomic update 9175265 [v3,07/14] pwm: sti: Add support for hardware readout 9175317 [v3,08/14] pwm: sti: Avoid glitches on already running PWMs 9175323 [v3,09/14] regulator: pwm: Adjust PWM config at probe time 9175305 [v3,10/14] regulator: pwm: Switch to the atomic PWM API 9175295 [v3,11/14] regulator: pwm: Properly initialize the ->state field 9175267 [v3,12/14] regulator: pwm: Retrieve correct voltage 9175279 [v3,13/14] regulator: pwm: Support extra continuous mode cases 9175273 [v3,14/14] regulator: pwm: Document pwm-dutycycle-unit and ... -Doug
On Tue, Jun 28, 2016 at 12:31:19PM -0700, Doug Anderson wrote: > In all seriousness, I think the design for usleep_range() is to try to > batch up wakeups to allow longer periods of sleeping and to optimize > power. This you want to sleep as long as is allowable and then if you > happen to wakeup for some other reason anyway then you process all the > things whose minimum has passed. IIRC it was trying to get back to > the good old days of only having jiffies where everyone was > synchronized on the tick and you could sleep till the next tick after > all the work was done. When you think of it this way then sleeping to > the max makes sense. ...but it also means that you need to be careful > and really not set the max too high. That's what I *thought* it was doing, but when I looked at the implementation down to the hrtimer level it was explicitly saying it was actively trying to deliver the maximum delay but might do less. I'd have expected that the implementation would check to see if there was a wakeup scheduled in the range, piggybacking on the first one if so, and if there was nothing it'd go for the minimum (perhaps with a bit of rounding). Oh well. > Of course, in practice I've found that often usleep_range() 99% of the > time sleeps for the max. That can lead to some very subtle bugs if > your min sleep is not long enough (!). Yup.
On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote: > Note that this patch is atop Boris's recent PWM regulator fixes. If > desired it wouldn't be too hard to write it atop the old code, though > quite honestly anyone using a PWM regulator should probably be using his > new code. Given that those don't seem likely to go in any time soon and aren't going to go in as a fix can you please respin against current code?
Mark, On Fri, Jul 1, 2016 at 1:06 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote: > >> Note that this patch is atop Boris's recent PWM regulator fixes. If >> desired it wouldn't be too hard to write it atop the old code, though >> quite honestly anyone using a PWM regulator should probably be using his >> new code. > > Given that those don't seem likely to go in any time soon and aren't > going to go in as a fix can you please respin against current code? I went to write this patch and it was pretty easy to write. ...but then I realized that I had no real way to test it. I can compile test it, but that's it. Do you still want it? Specifically without Boris's patches then I don't believe the devices I have access to are bootable when I tell Linux about the PWM regulator, especially when I configure the PWM regulator to use continuous mode (which is where the bug is). As I understand from <https://patchwork.kernel.org/patch/9175279/> the current PWM regulator assumes that 0% duty cycle is the lowest voltage and 100% is the highest voltage. My system is backwards (0% is highest, 100% lowest). Also: last I tried even the non-continuous mode without Boris's patches the regulator would glitch enough at bootup that my system would crash (the PWM regulator affects the memory controller). Anyway, let me know if you want me to post the untested, rebased patch... Personally I'd prefer to wait for Boris's patches and the land the tested version. Boris: any idea for what the next steps on your series are? -Doug
On Fri, Jul 01, 2016 at 01:17:48PM -0700, Doug Anderson wrote: > Anyway, let me know if you want me to post the untested, rebased > patch... Personally I'd prefer to wait for Boris's patches and the > land the tested version. We've got some boards in kernelci.org using PWM regulators I think, or perhaps someone else can test. It does seem like a real fix so...
+Thierry Hi Doug, On Fri, 1 Jul 2016 13:17:48 -0700 Doug Anderson <dianders@chromium.org> wrote: > Mark, > > On Fri, Jul 1, 2016 at 1:06 AM, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 27, 2016 at 09:53:11PM -0700, Douglas Anderson wrote: > > > >> Note that this patch is atop Boris's recent PWM regulator fixes. If > >> desired it wouldn't be too hard to write it atop the old code, though > >> quite honestly anyone using a PWM regulator should probably be using his > >> new code. > > > > Given that those don't seem likely to go in any time soon and aren't > > going to go in as a fix can you please respin against current code? > > I went to write this patch and it was pretty easy to write. ...but > then I realized that I had no real way to test it. I can compile test > it, but that's it. Do you still want it? > > Specifically without Boris's patches then I don't believe the devices > I have access to are bootable when I tell Linux about the PWM > regulator, especially when I configure the PWM regulator to use > continuous mode (which is where the bug is). As I understand from > <https://patchwork.kernel.org/patch/9175279/> the current PWM > regulator assumes that 0% duty cycle is the lowest voltage and 100% is > the highest voltage. My system is backwards (0% is highest, 100% > lowest). Also: last I tried even the non-continuous mode without > Boris's patches the regulator would glitch enough at bootup that my > system would crash (the PWM regulator affects the memory controller). > > Anyway, let me know if you want me to post the untested, rebased > patch... Personally I'd prefer to wait for Boris's patches and the > land the tested version. > > Boris: any idea for what the next steps on your series are? I'm waiting for reviews or acks. AFAIR, I addressed all the comments Thierry made on my v2, so I guess we're good on the PWM side, but that would be good to have a review from Mark now that we agreed on the PWM APIs. That shouldn't prevent you from rebasing this patch on Linus' tree and tag it for stable (I'll rebase my series on top of the regulator tree if needed). Regards, Boris
On Sat, Jul 02, 2016 at 05:47:52PM +0200, Boris Brezillon wrote: > I'm waiting for reviews or acks. AFAIR, I addressed all the comments > Thierry made on my v2, so I guess we're good on the PWM side, but that > would be good to have a review from Mark now that we agreed on the PWM > APIs. I've already have one round of review invalidated by changes in the PWM bits so I've been deprioritising it until it's clear that the churn there has stopped. I understand things are supposed to be quieting down but I also have a bunch of other things to look at...
Hi Mark, On Mon, 4 Jul 2016 10:53:36 +0200 Mark Brown <broonie@kernel.org> wrote: > On Sat, Jul 02, 2016 at 05:47:52PM +0200, Boris Brezillon wrote: > > > I'm waiting for reviews or acks. AFAIR, I addressed all the comments > > Thierry made on my v2, so I guess we're good on the PWM side, but that > > would be good to have a review from Mark now that we agreed on the PWM > > APIs. > > I've already have one round of review invalidated by changes in the PWM > bits so I've been deprioritising it until it's clear that the churn > there has stopped. I understand things are supposed to be quieting down > but I also have a bunch of other things to look at... I understand, but can we still expect to have it in 4.8 (I mean, assuming Thierry is okay taking the first 2 patching for 4.8 too)? Thanks, Boris
On Mon, Jul 04, 2016 at 11:05:04AM +0200, Boris Brezillon wrote: > I understand, but can we still expect to have it in 4.8 (I mean, > assuming Thierry is okay taking the first 2 patching for 4.8 too)? Possibly - we are getting very close to the merge window now, I don't know what Thierry's policies are on applying things as the merge window gets near.
On Mon, 27 Jun 2016 21:53:11 -0700 Douglas Anderson <dianders@chromium.org> wrote: > The original commit adding support for continuous voltage mode didn't > handle the regulator ramp delay properly. It treated the delay as a > fixed delay in uS despite the property being defined as uV / uS. Let's > adjust it. Luckily there appear to be no users of this ramp delay for > PWM regulators (as per grepping through device trees in linuxnext). > > Note also that the upper bound of usleep_range probably shouldn't be a > full 1 ms longer than the lower bound since I've seen plenty of hardware > with a ramp rate of ~5000 uS / uV and for small jumps the total delays > are in the tens of uS. 1000 is way too much. We'll try to be dynamic > and use 10% I realize I may have introduced another bug when adding the ->enable()/disable() methods: we are only waiting for the ramp up delay when changing the voltage, but it should probably be applied when enabling the PWM, and conditionally applied when changing the voltage only if the PWM is enabled. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > Note that this patch is atop Boris's recent PWM regulator fixes. If > desired it wouldn't be too hard to write it atop the old code, though > quite honestly anyone using a PWM regulator should probably be using his > new code. > > drivers/regulator/pwm-regulator.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c > index fa1c74c77bb0..de94d19f6e1f 100644 > --- a/drivers/regulator/pwm-regulator.c > +++ b/drivers/regulator/pwm-regulator.c > @@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > struct pwm_state pstate; > unsigned int diff_duty; > unsigned int dutycycle; > + int old_uV = pwm_regulator_get_voltage(rdev); > int ret; > > pwm_init_state(drvdata->pwm, &pstate); > @@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, > return ret; > } > > - /* Delay required by PWM regulator to settle to the new voltage */ > - usleep_range(ramp_delay, ramp_delay + 1000); > + if (ramp_delay == 0) > + return 0; > + > + /* Ramp delay is in uV/uS. Adjust to uS and delay */ > + ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay); > + usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10)); > > return 0; > }
Mark, On Sat, Jul 2, 2016 at 12:59 AM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Jul 01, 2016 at 01:17:48PM -0700, Doug Anderson wrote: > >> Anyway, let me know if you want me to post the untested, rebased >> patch... Personally I'd prefer to wait for Boris's patches and the >> land the tested version. > > We've got some boards in kernelci.org using PWM regulators I think, or > perhaps someone else can test. It does seem like a real fix so... OK, post coming up then. Sorry for the delay--I was on holiday the last two days. Note: which boards are you thinking would test this? My grep showed zero boards using regulator-ramp-delay with PWM regulator. While this is an important fix for anyone using the ramp delay, I'm not convinced that anyone in mainline is using it. -Doug
Boris, On Wed, Jul 6, 2016 at 5:27 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Mon, 27 Jun 2016 21:53:11 -0700 > Douglas Anderson <dianders@chromium.org> wrote: > >> The original commit adding support for continuous voltage mode didn't >> handle the regulator ramp delay properly. It treated the delay as a >> fixed delay in uS despite the property being defined as uV / uS. Let's >> adjust it. Luckily there appear to be no users of this ramp delay for >> PWM regulators (as per grepping through device trees in linuxnext). >> >> Note also that the upper bound of usleep_range probably shouldn't be a >> full 1 ms longer than the lower bound since I've seen plenty of hardware >> with a ramp rate of ~5000 uS / uV and for small jumps the total delays >> are in the tens of uS. 1000 is way too much. We'll try to be dynamic >> and use 10% > > I realize I may have introduced another bug when adding the > ->enable()/disable() methods: we are only waiting for the ramp up delay > when changing the voltage, but it should probably be applied when > enabling the PWM, and conditionally applied when changing the voltage > only if the PWM is enabled. I'll certainly let Mark comment here, but: * For enabling the PWM, I think you want want "regulator-enable-ramp-delay". * Right, we probably shouldn't be delaying if the regulator is off. I'll add that to my patch. -Doug
On Wed, Jul 06, 2016 at 02:27:19PM +0200, Boris Brezillon wrote: > I realize I may have introduced another bug when adding the > ->enable()/disable() methods: we are only waiting for the ramp up delay > when changing the voltage, but it should probably be applied when > enabling the PWM, and conditionally applied when changing the voltage > only if the PWM is enabled. Yes, it should be applied on enable.
On Wed, Jul 06, 2016 at 11:31:45AM -0700, Doug Anderson wrote: > I'll certainly let Mark comment here, but: > * For enabling the PWM, I think you want want "regulator-enable-ramp-delay". In the absence of that we should just be treating it as a voltage change from zero. Some regulators do have different characteristics on initial ramp up but if we don't know about them we ought to just use the regular one. Note that the core does have standard code to apply delays which may just be working here.
On Wed, Jul 06, 2016 at 11:30:59AM -0700, Doug Anderson wrote: > Note: which boards are you thinking would test this? My grep showed > zero boards using regulator-ramp-delay with PWM regulator. While this > is an important fix for anyone using the ramp delay, I'm not convinced > that anyone in mainline is using it. I just meant there are boards out there using the regulator, not the specific feature.
On Mon, Jul 04, 2016 at 11:13:44AM +0200, Mark Brown wrote: > On Mon, Jul 04, 2016 at 11:05:04AM +0200, Boris Brezillon wrote: > > > I understand, but can we still expect to have it in 4.8 (I mean, > > assuming Thierry is okay taking the first 2 patching for 4.8 too)? > > Possibly - we are getting very close to the merge window now, I don't > know what Thierry's policies are on applying things as the merge window > gets near. I don't have hard deadlines, especially not for new code that has zero potential for regressing. I've now pulled that whole series into the PWM tree. Thierry
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c index fa1c74c77bb0..de94d19f6e1f 100644 --- a/drivers/regulator/pwm-regulator.c +++ b/drivers/regulator/pwm-regulator.c @@ -188,6 +188,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, struct pwm_state pstate; unsigned int diff_duty; unsigned int dutycycle; + int old_uV = pwm_regulator_get_voltage(rdev); int ret; pwm_init_state(drvdata->pwm, &pstate); @@ -219,8 +220,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev, return ret; } - /* Delay required by PWM regulator to settle to the new voltage */ - usleep_range(ramp_delay, ramp_delay + 1000); + if (ramp_delay == 0) + return 0; + + /* Ramp delay is in uV/uS. Adjust to uS and delay */ + ramp_delay = DIV_ROUND_UP(abs(req_min_uV - old_uV), ramp_delay); + usleep_range(ramp_delay, ramp_delay + DIV_ROUND_UP(ramp_delay, 10)); return 0; }
The original commit adding support for continuous voltage mode didn't handle the regulator ramp delay properly. It treated the delay as a fixed delay in uS despite the property being defined as uV / uS. Let's adjust it. Luckily there appear to be no users of this ramp delay for PWM regulators (as per grepping through device trees in linuxnext). Note also that the upper bound of usleep_range probably shouldn't be a full 1 ms longer than the lower bound since I've seen plenty of hardware with a ramp rate of ~5000 uS / uV and for small jumps the total delays are in the tens of uS. 1000 is way too much. We'll try to be dynamic and use 10% Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Note that this patch is atop Boris's recent PWM regulator fixes. If desired it wouldn't be too hard to write it atop the old code, though quite honestly anyone using a PWM regulator should probably be using his new code. drivers/regulator/pwm-regulator.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)