Changing voltage might ignore slew rate and cause a current surge.

With current implementation the driver will get the regulator to change
the voltage range used during run time. According to communication I
have had with Texas Instruments, this is not intended, since the
Dynamic Voltage Scaling in the hardware is only designed to work
within a voltage range. The current implementation will therefore
ignore the slew rate that is defined in devicetree when the voltage
range is changed during use.

The current implementation will always select a voltage in the most
accurate range that can reach that voltage even though multiple ranges
are able to reach that voltage. There are 4 Voltage ranges with the
following reach:
0b00: 0.4-0.71875V      (1.25mV step size)
0b01: 0.4-1.0375V       (2.5mV)
0b10: 0.4-1.675V        (5mV)
0b11: 0.8-3.3V          (10mV)
This in practice means that a change from below to above 0.71875V will
use the smallest range(0b00) for the values below and the second
smallest range(0b01) for the voltages above (Up to 1.675V). I have
timed how long it takes to go from below 0.71875V to above. The
increase was 100mV which, with the slew rate set to 1250µV/µs. This
in theory should take 80µs to do. With the current implementation, it
takes 10µs on my hardware. Doing the same test with the slew rate set
to 5000µV/µs, which should take 20µs, also only takes 10µs to do on
my hardware. Not only is this not in line with the technical
specification for the regulator. It also causes a current surge. Which
when calculating the output current, as described in the technical
specification, compared to what I could observe on my hardware the real
output is ~1A higher (~1.2A) than what I calculated it to be(~0.2A).

I tested also transitioning from a bigger to a smaller range, and the
results were the same.

Instead, let's limit the voltage range to a single one, which is in
line with the intended use of the regulator. This is done by looking
up the minimum and maximum requested voltage specified in devicetree.

Signed-off-by: Jonas Andreasson <jonas.andreasson@axis.com>
---
Changes in v2:
- Removed unnecessary goto statement
- Added missing const qualifier in function parameter
- Link to v1: https://lore.kernel.org/r/20250115-tps-fix-v1-1-2bd7b316409d@axis.com
---
 drivers/regulator/tps6287x-regulator.c | 57 ++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/regulator/tps6287x-regulator.c b/drivers/regulator/tps6287x-regulator.c
index 97f5ce138548..c0f5f0a186a3 100644
--- a/drivers/regulator/tps6287x-regulator.c
+++ b/drivers/regulator/tps6287x-regulator.c
@@ -44,10 +44,35 @@ static const unsigned int tps6287x_voltage_range_sel[] = {
 	0x0, 0x1, 0x2, 0x3
 };
 
+static const unsigned int tps6287x_voltage_range_prefix[] = {
+	0x000, 0x100, 0x200, 0x300
+};
+
 static const unsigned int tps6287x_ramp_table[] = {
 	10000, 5000, 1250, 500
 };
 
+struct tps6287x_reg_data {
+	int range;
+};
+
+static int tps6287x_best_range(struct regulator_config *config, const struct regulator_desc *desc)
+{
+	const struct linear_range *r;
+	int i;
+
+	if (!config->init_data->constraints.apply_uV)
+		return -1;
+
+	for (i = 0; i < desc->n_linear_ranges; i++) {
+		r = &desc->linear_ranges[i];
+		if (r->min <= config->init_data->constraints.min_uV &&
+		    config->init_data->constraints.max_uV <= linear_range_get_max_value(r))
+			return i;
+	}
+	return -1;
+}
+
 static int tps6287x_set_mode(struct regulator_dev *rdev, unsigned int mode)
 {
 	unsigned int val;
@@ -91,6 +116,28 @@ static unsigned int tps6287x_of_map_mode(unsigned int mode)
 	}
 }
 
+static int tps6287x_map_voltage(struct regulator_dev *rdev, int min_uV, int max_uV)
+{
+	struct tps6287x_reg_data *data = (struct tps6287x_reg_data *)rdev->reg_data;
+	struct linear_range selected_range;
+	int selector, voltage;
+
+	if (!data || data->range == -1)
+		return regulator_map_voltage_pickable_linear_range(rdev, min_uV, max_uV);
+
+	selected_range = rdev->desc->linear_ranges[data->range];
+	selector = DIV_ROUND_UP(min_uV - selected_range.min, selected_range.step);
+	if (selector < selected_range.min_sel || selector > selected_range.max_sel)
+		return -EINVAL;
+
+	selector |= tps6287x_voltage_range_prefix[data->range];
+	voltage = rdev->desc->ops->list_voltage(rdev, selector);
+	if (voltage < min_uV || voltage > max_uV)
+		return -EINVAL;
+
+	return selector;
+}
+
 static const struct regulator_ops tps6287x_regulator_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -100,6 +147,7 @@ static const struct regulator_ops tps6287x_regulator_ops = {
 	.get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,
 	.set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,
 	.list_voltage = regulator_list_voltage_pickable_linear_range,
+	.map_voltage = tps6287x_map_voltage,
 	.set_ramp_delay = regulator_set_ramp_delay_regmap,
 };
 
@@ -130,8 +178,14 @@ static int tps6287x_i2c_probe(struct i2c_client *i2c)
 {
 	struct device *dev = &i2c->dev;
 	struct regulator_config config = {};
+	struct tps6287x_reg_data *reg_data;
 	struct regulator_dev *rdev;
 
+	reg_data = devm_kzalloc(dev, sizeof(struct tps6287x_reg_data), GFP_KERNEL);
+
+	if (!reg_data)
+		return -ENOMEM;
+
 	config.regmap = devm_regmap_init_i2c(i2c, &tps6287x_regmap_config);
 	if (IS_ERR(config.regmap)) {
 		dev_err(dev, "Failed to init i2c\n");
@@ -143,12 +197,15 @@ static int tps6287x_i2c_probe(struct i2c_client *i2c)
 	config.init_data = of_get_regulator_init_data(dev, dev->of_node,
 						      &tps6287x_reg);
 
+	reg_data->range = tps6287x_best_range(&config, &tps6287x_reg);
+
 	rdev = devm_regulator_register(dev, &tps6287x_reg, &config);
 	if (IS_ERR(rdev)) {
 		dev_err(dev, "Failed to register regulator\n");
 		return PTR_ERR(rdev);
 	}
 
+	rdev->reg_data = (void *)reg_data;
 	dev_dbg(dev, "Probed regulator\n");
 
 	return 0;

---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241212-tps-fix-eacedddb03ca

Best regards,
-- 
Jonas Andreasson <jonas.andreasson@axis.com>

On Tue, 21 Jan 2025 15:37:50 +0100, Jonas Andreasson wrote:
> Changing voltage might ignore slew rate and cause a current surge.
> 
> With current implementation the driver will get the regulator to change
> the voltage range used during run time. According to communication I
> have had with Texas Instruments, this is not intended, since the
> Dynamic Voltage Scaling in the hardware is only designed to work
> within a voltage range. The current implementation will therefore
> ignore the slew rate that is defined in devicetree when the voltage
> range is changed during use.
> 
> [...]

Applied to

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

Thanks!

[1/1] regulator: TPS6287X: Use min/max uV to get VRANGE
      commit: 3028583d1314a70ca273c51e0265f698c0bd5760

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