[PATCH 0/2] regulator: stepping set voltage for regmap users

[PATCH 0/2] regulator: stepping set voltage for regmap users

[Bartosz Golaszewski]

I'm working on a regulator driver for a PMIC whose programming manual
suggests manual ramping of the regulator output voltage one step at a
time (when the regulator is enabled).

Initially I wanted to simply put it into the driver but noticed that
some other modules (e.g. rk808) seem to be doing the same and could
potentially benefit from putting this helper into the regulator core
code.

The first patch makes _regulator_is_enabled() available to regulator
code outside core.c. The second provides the helper described above.

If accepted this will be used in a driver submitted for review after
the 4.21 merge window.

Bartosz Golaszewski (2):
  regulator: make _regulator_is_enabled() available in internal.h
  regulator: provide regulator_set_voltage_sel_regmap_step() helper

 drivers/regulator/core.c         |  3 +-
 drivers/regulator/helpers.c      | 48 ++++++++++++++++++++++++++++++++
 drivers/regulator/internal.h     |  1 +
 include/linux/regulator/driver.h |  2 ++
 4 files changed, 52 insertions(+), 2 deletions(-)

-- 
2.19.1

[PATCH 1/2] regulator: make _regulator_is_enabled() available in internal.h

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We want to use _regulator_is_enabled() in helpers.c. Export it locally
in internal.h.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/regulator/core.c     | 3 +--
 drivers/regulator/internal.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2c66b528aede..3754711530b2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -96,7 +96,6 @@ struct regulator_supply_alias {
 	const char *alias_supply;
 };
 
-static int _regulator_is_enabled(struct regulator_dev *rdev);
 static int _regulator_disable(struct regulator_dev *rdev);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
@@ -2531,7 +2530,7 @@ int regulator_disable_deferred(struct regulator *regulator, int ms)
 }
 EXPORT_SYMBOL_GPL(regulator_disable_deferred);
 
-static int _regulator_is_enabled(struct regulator_dev *rdev)
+int _regulator_is_enabled(struct regulator_dev *rdev)
 {
 	/* A GPIO control always takes precedence */
 	if (rdev->ena_pin)
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 943926a156f2..a8557ed96ae6 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -113,4 +113,5 @@ enum regulator_get_type {
 
 struct regulator *_regulator_get(struct device *dev, const char *id,
 				 enum regulator_get_type get_type);
+int _regulator_is_enabled(struct regulator_dev *rdev);
 #endif
-- 
2.19.1

[PATCH 2/2] regulator: provide regulator_set_voltage_sel_regmap_step() helper

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

On some devices we need to manually ramp the regulators to desired
voltage one step at a time. This patch adds a helper routine for
regmap users that checks if the regulator is enabled and, if so,
iterates over all selectors between the current one and the one
representing the targeted voltage setting.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/regulator/helpers.c      | 48 ++++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |  2 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index 5686a1335bd3..6eb0e147bbc9 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -19,6 +19,8 @@
 #include <linux/regulator/driver.h>
 #include <linux/module.h>
 
+#include "internal.h"
+
 /**
  * regulator_is_enabled_regmap - standard is_enabled() for regmap users
  *
@@ -279,6 +281,52 @@ int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel)
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_sel_regmap);
 
+/**
+ * regulator_set_voltage_sel_regmap_step - stepping set_voltage_sel for regmap
+ *                                         users
+ *
+ * @rdev: regulator to operate on
+ * @sel: Selector to set
+ *
+ * Regulators that use regmap for their register I/O but want to ramp up/down
+ * to the selected voltage one step at a time can use this routine as their
+ * set_voltage_sel operation.
+ */
+int regulator_set_voltage_sel_regmap_step(struct regulator_dev *rdev,
+					  unsigned int sel)
+{
+	int ret, curr, diff, i, end;
+	bool asc;
+
+	/*
+	 * If the regulator is disabled, we can program the desired
+	 * voltage right away.
+	 */
+	if (!_regulator_is_enabled(rdev))
+		return regulator_set_voltage_sel_regmap(rdev, sel);
+
+	curr = regulator_get_voltage_sel_regmap(rdev);
+	if (curr < 0)
+		return curr;
+
+	diff = curr - sel;
+	if (diff == 0)
+		return 0; /* Already there. */
+
+	asc = diff > 0 ? false : true;
+	end = asc ? sel + 1 : sel - 1;
+	asc ? curr++ : curr--;
+
+	for (i = curr; i != end; asc ? i++ : i--) {
+		ret = regulator_set_voltage_sel_regmap(rdev, i);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(regulator_set_voltage_sel_regmap_step);
+
 /**
  * regulator_map_voltage_iterate - map_voltage() based on list_voltage()
  *
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index a9c030192147..eedd8b495900 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -530,6 +530,8 @@ int regulator_set_voltage_sel_pickable_regmap(struct regulator_dev *rdev,
 						unsigned int sel);
 int regulator_get_voltage_sel_regmap(struct regulator_dev *rdev);
 int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel);
+int regulator_set_voltage_sel_regmap_step(struct regulator_dev *rdev,
+					  unsigned int sel);
 int regulator_is_enabled_regmap(struct regulator_dev *rdev);
 int regulator_enable_regmap(struct regulator_dev *rdev);
 int regulator_disable_regmap(struct regulator_dev *rdev);
-- 
2.19.1

Re: [PATCH 2/2] regulator: provide regulator_set_voltage_sel_regmap_step() helper

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Mon, Dec 10, 2018 at 04:10:24PM +0100, Bartosz Golaszewski wrote:

> On some devices we need to manually ramp the regulators to desired
> voltage one step at a time. This patch adds a helper routine for
> regmap users that checks if the regulator is enabled and, if so,

My first question would be why do this at the regmap helper level, why
not do this at the core level by stepping through selectors?  I'd also
expect to be programming the step size here, while some regulators
aren't able to regulate well over larger step changes I'd expect that
they wouldn't need to step through every single possible voltage value,
that's potentially extremely slow.

> +	asc = diff > 0 ? false : true;
> +	end = asc ? sel + 1 : sel - 1;
> +	asc ? curr++ : curr--;

Please just write normal conditional statements so the code is more
readable and hence maintainable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] regulator: provide regulator_set_voltage_sel_regmap_step() helper

[Bartosz Golaszewski]

pon., 10 gru 2018 o 16:41 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Mon, Dec 10, 2018 at 04:10:24PM +0100, Bartosz Golaszewski wrote:
>
> > On some devices we need to manually ramp the regulators to desired
> > voltage one step at a time. This patch adds a helper routine for
> > regmap users that checks if the regulator is enabled and, if so,
>
> My first question would be why do this at the regmap helper level, why
> not do this at the core level by stepping through selectors?  I'd also
> expect to be programming the step size here, while some regulators
> aren't able to regulate well over larger step changes I'd expect that
> they wouldn't need to step through every single possible voltage value,
> that's potentially extremely slow.
>

This sounds like a good improvement to the core regulator code indeed.
I'll give it a try but I'm not sure I'll have something before the
next merge window, so for now I'll put this logic into my driver that
I'd like to get merged for 4.22 and we can factor it out later on once
we agree on the right approach.

> > +     asc = diff > 0 ? false : true;
> > +     end = asc ? sel + 1 : sel - 1;
> > +     asc ? curr++ : curr--;
>
> Please just write normal conditional statements so the code is more
> readable and hence maintainable.

Sure.

Thanks,
Bartosz