[PATCH v2 1/2] regulator: core: Rename _regulator_enable_delay()

[PATCH v2 1/2] regulator: core: Rename _regulator_enable_delay()

[Brian Norris]

I want to use it in other contexts besides _regulator_do_enable().

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
 * Rename/retain this helper, instead of using fsleep() (see also patch
   2)

 drivers/regulator/core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d2553970a67b..8b188e56fd0b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2511,17 +2511,17 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
 }
 
 /**
- * _regulator_enable_delay - a delay helper function
+ * _regulator_delay_helper - a delay helper function
  * @delay: time to delay in microseconds
  *
  * Delay for the requested amount of time as per the guidelines in:
  *
  *     Documentation/timers/timers-howto.rst
  *
- * The assumption here is that regulators will never be enabled in
+ * The assumption here is that these regulator operations will never used in
  * atomic context and therefore sleeping functions can be used.
  */
-static void _regulator_enable_delay(unsigned int delay)
+static void _regulator_delay_helper(unsigned int delay)
 {
 	unsigned int ms = delay / 1000;
 	unsigned int us = delay % 1000;
@@ -2603,7 +2603,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 		s64 remaining = ktime_us_delta(end, ktime_get());
 
 		if (remaining > 0)
-			_regulator_enable_delay(remaining);
+			_regulator_delay_helper(remaining);
 	}
 
 	if (rdev->ena_pin) {
@@ -2630,14 +2630,14 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 	/* If poll_enabled_time is set, poll upto the delay calculated
 	 * above, delaying poll_enabled_time uS to check if the regulator
 	 * actually got enabled.
-	 * If the regulator isn't enabled after enable_delay has
-	 * expired, return -ETIMEDOUT.
+	 * If the regulator isn't enabled after our delay helper has expired,
+	 * return -ETIMEDOUT.
 	 */
 	if (rdev->desc->poll_enabled_time) {
 		unsigned int time_remaining = delay;
 
 		while (time_remaining > 0) {
-			_regulator_enable_delay(rdev->desc->poll_enabled_time);
+			_regulator_delay_helper(rdev->desc->poll_enabled_time);
 
 			if (rdev->desc->ops->get_status) {
 				ret = _regulator_check_status_enabled(rdev);
@@ -2656,7 +2656,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 			return -ETIMEDOUT;
 		}
 	} else {
-		_regulator_enable_delay(delay);
+		_regulator_delay_helper(delay);
 	}
 
 	trace_regulator_enable_complete(rdev_get_name(rdev));
-- 
2.36.0.rc0.470.gd361397f0d-goog

[PATCH v2 2/2] regulator: core: Sleep (not delay) in set_voltage()

[Brian Norris]

These delays can be relatively large (e.g., hundreds of microseconds to
several milliseconds on RK3399 Gru systems). Per
Documentation/timers/timers-howto.rst, that should usually use a
sleeping delay. Let's use the existing regulator delay helper to handle
both large and small delays appropriately. This avoids burning a bunch
of CPU time and hurting scheduling latencies when hitting regulators a
lot (e.g., during cpufreq).

The sleep vs. delay issue choice has been made differently over time --
early versions of RK3399 Gru PWM-regulator support used usleep_range()
in pwm-regulator.c. More of this got moved into the regulator core,
in commits like:

73e705bf81ce regulator: core: Add set_voltage_time op

At the same time, the sleep turned into a delay.

It's OK to sleep in _regulator_do_set_voltage(), as we aren't in an
atomic context. (All our callers grab various mutexes already.)

I avoid using fsleep() because it uses a usleep_range() of [N to N*2],
and usleep_range() very commonly biases to the high end of the range. We
don't want to double the expected delay, especially for long delays.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---

Changes in v2:
 * Add Matthias's "Reviewed-by"
 * Use regulator helper instead of fsleep()
 * Reorder patch 1&2

 drivers/regulator/core.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8b188e56fd0b..0279ff687e1d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3548,12 +3548,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	}
 
 	/* Insert any necessary delays */
-	if (delay >= 1000) {
-		mdelay(delay / 1000);
-		udelay(delay % 1000);
-	} else if (delay) {
-		udelay(delay);
-	}
+	_regulator_delay_helper(delay);
 
 	if (best_val >= 0) {
 		unsigned long data = best_val;
-- 
2.36.0.rc0.470.gd361397f0d-goog

Re: [PATCH v2 1/2] regulator: core: Rename _regulator_enable_delay()

[Mark Brown]

On Wed, 20 Apr 2022 14:22:12 -0700, Brian Norris wrote:
> I want to use it in other contexts besides _regulator_do_enable().
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/2] regulator: core: Rename _regulator_enable_delay()
      commit: a38dce4cb1f1bcc4f6ef7f11e54b6507a4043ebe
[2/2] regulator: core: Sleep (not delay) in set_voltage()
      commit: 062920d2464715ef5cbba52a8573ba12cc882b8f

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark