regulator: stepping set voltage for regmap users
cover regulator: stepping set voltage for regmap users
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(-)
Signature · 1 line
2.19.1
PATCH 1/2 regulator: make _regulator_is_enabled() available in internal.h
From: Bartosz Golaszewski <[email protected]>
We want to use _regulator_is_enabled() in helpers.c. Export it locally in internal.h.
--- drivers/regulator/core.c | 3 +-- drivers/regulator/internal.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/regulator/core.c +1 -2
@@ -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)
drivers/regulator/internal.h +1 -0
@@ -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
Signature · 1 line
2.19.1
PATCH 2/2 regulator: provide regulator_set_voltage_sel_regmap_step() helper
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
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.
--- drivers/regulator/helpers.c | 48 ++++++++++++++++++++++++++++++++ include/linux/regulator/driver.h | 2 ++ 2 files changed, 50 insertions(+)
drivers/regulator/helpers.c +48 -0
@@ -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() *
include/linux/regulator/driver.h +2 -0
@@ -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);
Signature · 1 line
2.19.1
Mark Brown
maintainerRe: [PATCH 2/2] regulator: provide regulator_set_voltage_sel_regmap_step() helper
Quoted 3 lines · Bartosz Golaszewski
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.
drivers/regulator/helpers.c int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel) · line 318
Quoted 3 lines
drivers/regulator/helpers.c +3
-0+ 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.
Re: [PATCH 2/2] regulator: provide regulator_set_voltage_sel_regmap_step() helper
pon., 10 gru 2018 o 16:41 Mark Brown <[email protected]> napisał(a):
Quoted 7 lines · Bartosz Golaszewski
Quoted 4 lines
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,
Quoted 7 lines
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.
Quoted 6 lines
drivers/regulator/helpers.c int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel) · line 318
Quoted 3 lines
drivers/regulator/helpers.c +3
-0+ 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