[PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Matthias Kaehlcke]

The new function allows consumers to determine if a regulator is
continuous or discrete, and whether the results of
regulator_count_voltages() and regulator_list_voltage() correspond
to the regulator itself or its supply.

Change-Id: I1198cee9fff60dc747a02860e9652034f4d5da33
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- Added regulator_has_continuous_voltage_range()

 drivers/regulator/core.c           | 16 ++++++++++++++++
 include/linux/regulator/consumer.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 53d4fc70dbd0..ce1d02792ef4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2552,6 +2552,22 @@ int regulator_count_voltages(struct regulator *regulator)
 EXPORT_SYMBOL_GPL(regulator_count_voltages);
 
 /**
+ * regulator_has_continuous_voltage_range - is the regulator continuous
+ * or discrete
+ * @regulator: regulator source
+ *
+ * Returns true if the regulator has a continuous voltage range and
+ * false for discrete voltage regulators.
+ */
+int regulator_has_continuous_voltage_range(struct regulator *regulator)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+
+	return rdev->desc->continuous_voltage_range;
+}
+EXPORT_SYMBOL_GPL(regulator_has_continuous_voltage_range);
+
+/**
  * regulator_list_voltage - enumerate supported voltages
  * @regulator: regulator source
  * @selector: identify voltage to list
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index ea0fffa5faeb..19945b5a702e 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -241,6 +241,7 @@ void regulator_bulk_free(int num_consumers,
 			 struct regulator_bulk_data *consumers);
 
 int regulator_count_voltages(struct regulator *regulator);
+int regulator_has_continuous_voltage_range(struct regulator *regulator);
 int regulator_list_voltage(struct regulator *regulator, unsigned selector);
 int regulator_is_supported_voltage(struct regulator *regulator,
 				   int min_uV, int max_uV);
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH v2 2/2] regulator: Add driver for voltage controlled regulators

[Matthias Kaehlcke]

The output voltage of a voltage controlled regulator can be controlled
through the voltage of another regulator. A vctrl regulator can be
continuous or discrete, depending on its control regulator. The current
version of this driver assumes that the output voltage is a linear
function of the control voltage.

Some hardware configurations include overvoltage protection (OVP)
circuitry which can shutdown the regulator on downward voltage changes
that exceed a certain threshold. To prevent this the driver can be
configured to adjust the voltage gradually when a single step would
exceed the threshold.

Change-Id: Ibbf74a9727ff666887cc15e09e8b364aef820063
---
Changes in v2:
- Don't configure control regulator as supply
- Added enable/disable ops to turn control regulator on/off
- Use regulator_has_continuous_voltage_range() to determine if control
  regulator is continuous or discrete
- Use regulator-min/max-microvolt as output voltage range
- Updated and fixed example in DT bindings
- Updated commit message

 .../devicetree/bindings/regulator/vctrl.txt        |  50 ++
 drivers/regulator/Kconfig                          |   7 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/vctrl-regulator.c                | 556 +++++++++++++++++++++
 4 files changed, 614 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/vctrl.txt
 create mode 100644 drivers/regulator/vctrl-regulator.c

diff --git a/Documentation/devicetree/bindings/regulator/vctrl.txt b/Documentation/devicetree/bindings/regulator/vctrl.txt
new file mode 100644
index 000000000000..c7ee9956d62d
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/vctrl.txt
@@ -0,0 +1,50 @@
+Bindings for Voltage controlled regulators
+==========================================
+
+Required properties:
+--------------------
+- compatible		  : must be "vctrl-regulator".
+- regulator-min-microvolt : smallest voltage consumers may set
+- regulator-max-microvolt : largest voltage consumers may set
+- ctrl-regulator:	  : the name of the regulator supplying the control
+			    voltage.
+- ctrl-voltage-range	  : an array of two integer values describing the range
+			    (min/max) of the control voltage. The values specify
+			    the control voltage needed to generate the corresponding
+			    regulator-min/max-microvolt output voltage.
+
+Optional properties:
+--------------------
+- ovp-threshold-percent	: overvoltage protection (OVP) threshold of the
+			  regulator in percent. Some regulators have an OVP
+			  circuitry which shuts down the regulator when the
+			  actual output voltage deviates beyond a certain
+			  margin from the expected value for a given control
+			  voltage. On larger voltage decreases this can occur
+			  undesiredly since the output voltage does not adjust
+			  inmediately to changes in the control voltage. To
+			  avoid this situation the vctrl driver breaks down
+			  larger voltage decreases into multiple steps, where
+			  each step is within the OVP threshold.
+- min-slew-down-rate	: Describes how slowly the regulator voltage will decay
+			  down in the worst case (lightest expected load).
+			  Specified in uV / us (like main regulator ramp rate).
+			  This value is required when ovp-threshold-percent is
+			  specified.
+
+Example:
+
+	vctrl-reg {
+		compatible = "vctrl-regulator";
+		regulator-name = "vctrl_reg";
+
+		ctrl-regulator = "ctrl_reg";
+
+		regulator-min-microvolt = <800000>;
+		regulator-max-microvolt = <1500000>;
+
+		ctrl-voltage-range = <200000 500000>;
+
+		min-slew-down-rate = <225>;
+		ovp-threshold-percent = <16>;
+	};
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index be06eb29c681..9a090e338a19 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -850,6 +850,13 @@ config REGULATOR_TWL4030
 	  This driver supports the voltage regulators provided by
 	  this family of companion chips.
 
+config REGULATOR_VCTRL
+	tristate "Voltage controlled regulators"
+	depends on OF
+	help
+	  This driver provides support for voltage regulators whose output
+	  voltage is controlled by the voltage of another regulator.
+
 config REGULATOR_VEXPRESS
 	tristate "Versatile Express regulators"
 	depends on VEXPRESS_CONFIG
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index ef7725e2592a..891dd287f26b 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -106,6 +106,7 @@ obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
 obj-$(CONFIG_REGULATOR_TPS80031) += tps80031-regulator.o
 obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
+obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o
 obj-$(CONFIG_REGULATOR_VEXPRESS) += vexpress-regulator.o
 obj-$(CONFIG_REGULATOR_WM831X) += wm831x-dcdc.o
 obj-$(CONFIG_REGULATOR_WM831X) += wm831x-isink.o
diff --git a/drivers/regulator/vctrl-regulator.c b/drivers/regulator/vctrl-regulator.c
new file mode 100644
index 000000000000..b80150ae9ea4
--- /dev/null
+++ b/drivers/regulator/vctrl-regulator.c
@@ -0,0 +1,556 @@
+/*
+ * Driver for voltage controller regulators
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/sort.h>
+
+struct vctrl_voltage_range {
+	int min_uV;
+	int max_uV;
+};
+
+struct vctrl_voltage_ranges {
+	struct vctrl_voltage_range ctrl;
+	struct vctrl_voltage_range out;
+};
+
+struct vctrl_voltage_table {
+	int ctrl;
+	int out;
+};
+
+struct vctrl_data {
+	struct regulator_dev *rdev;
+	struct regulator_desc desc;
+	struct regulator *ctrl_reg;
+	bool enabled;
+	unsigned int min_slew_down_rate;
+	unsigned int ovp_threshold;
+	struct vctrl_voltage_ranges vrange;
+	struct vctrl_voltage_table *vtable;
+	unsigned int sel;
+};
+
+static int vctrl_calc_ctrl_voltage(struct vctrl_data *vctrl, int out_uV)
+{
+	struct vctrl_voltage_range *ctrl = &vctrl->vrange.ctrl;
+	struct vctrl_voltage_range *out = &vctrl->vrange.out;
+
+	return ctrl->min_uV +
+		DIV_ROUND_CLOSEST_ULL((s64)(out_uV - out->min_uV) *
+				      (ctrl->max_uV - ctrl->min_uV),
+				      out->max_uV - out->min_uV);
+}
+
+static int vctrl_calc_output_voltage(struct vctrl_data *vctrl, int ctrl_uV)
+{
+	struct vctrl_voltage_range *ctrl = &vctrl->vrange.ctrl;
+	struct vctrl_voltage_range *out = &vctrl->vrange.out;
+
+	if (ctrl_uV < 0) {
+		pr_err("vctrl: failed to get control voltage\n");
+		return ctrl_uV;
+	}
+
+	if (ctrl_uV < ctrl->min_uV)
+		return out->min_uV;
+
+	if (ctrl_uV > ctrl->max_uV)
+		return out->max_uV;
+
+	return out->min_uV +
+		DIV_ROUND_CLOSEST_ULL((s64)(ctrl_uV - ctrl->min_uV) *
+				      (out->max_uV - out->min_uV),
+				      ctrl->max_uV - ctrl->min_uV);
+}
+
+static int vctrl_get_voltage(struct regulator_dev *rdev)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+	int ctrl_uV = regulator_get_voltage(vctrl->ctrl_reg);
+
+	return vctrl_calc_output_voltage(vctrl, ctrl_uV);
+}
+
+static int vctrl_set_voltage(struct regulator_dev *rdev,
+			     int req_min_uV, int req_max_uV,
+			     unsigned int *selector)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+	struct regulator *ctrl_reg = vctrl->ctrl_reg;
+	int orig_ctrl_uV = regulator_get_voltage(ctrl_reg);
+	int uV = vctrl_calc_output_voltage(vctrl, orig_ctrl_uV);
+	int ret;
+
+	if (req_min_uV >= uV || !vctrl->ovp_threshold)
+		/* voltage rising or no OVP */
+		return regulator_set_voltage(
+			ctrl_reg,
+			vctrl_calc_ctrl_voltage(vctrl, req_min_uV),
+			vctrl_calc_ctrl_voltage(vctrl, req_max_uV));
+
+	while (uV > req_min_uV) {
+		int max_drop_uV = (uV * vctrl->ovp_threshold) / 100;
+		int next_uV;
+		int next_ctrl_uV;
+		int delay;
+
+		/* Make sure no infinite loop even in crazy cases */
+		if (max_drop_uV == 0)
+			max_drop_uV = 1;
+
+		next_uV = max_t(int, req_min_uV, uV - max_drop_uV);
+		next_ctrl_uV = vctrl_calc_ctrl_voltage(vctrl, next_uV);
+
+		ret = regulator_set_voltage(ctrl_reg,
+					    next_ctrl_uV,
+					    next_ctrl_uV);
+		if (ret)
+			goto err;
+
+		delay = DIV_ROUND_UP(uV - next_uV, vctrl->min_slew_down_rate);
+		usleep_range(delay, delay + DIV_ROUND_UP(delay, 10));
+
+		uV = next_uV;
+	}
+
+	return 0;
+
+err:
+	/* Try to go back to original voltage */
+	regulator_set_voltage(ctrl_reg, orig_ctrl_uV, orig_ctrl_uV);
+
+	return ret;
+}
+
+static int vctrl_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+
+	return vctrl->sel;
+}
+
+static int vctrl_set_voltage_sel(struct regulator_dev *rdev,
+				 unsigned int selector)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+	struct regulator *ctrl_reg = vctrl->ctrl_reg;
+	unsigned int orig_sel = vctrl->sel;
+	int target_uV;
+	int uV;
+	int ret;
+
+	if (selector >= rdev->desc->n_voltages)
+		return -EINVAL;
+
+	uV = vctrl->vtable[orig_sel].out;
+	target_uV = vctrl->vtable[selector].out;
+
+	if (target_uV >= uV || !vctrl->ovp_threshold) {
+		/* voltage rising or no OVP */
+		ret = regulator_set_voltage(ctrl_reg,
+					    vctrl->vtable[selector].ctrl,
+					    vctrl->vtable[selector].ctrl);
+		if (!ret)
+			vctrl->sel = selector;
+
+		return ret;
+	}
+
+	while (vctrl->sel != selector) {
+		int uV = vctrl->vtable[vctrl->sel].out;
+		int max_drop_uV = (uV * vctrl->ovp_threshold) / 100;
+		int next_uV = max_t(int, target_uV, uV - max_drop_uV);
+		unsigned int next_sel = vctrl->sel;
+		int delay;
+
+		/* Find lowest voltage above the OVP threshold */
+		if (vctrl->vtable[0].out >= next_uV) {
+			next_sel = 0;
+		} else {
+			int i;
+			/*
+			 * Backward traversal should be faster on average for
+			 * gradual changes due to OVP.
+			 */
+			for (i = vctrl->sel - 1; i >= 0; i--) {
+				if (vctrl->vtable[i].out < next_uV) {
+					next_sel = i + 1;
+					break;
+				}
+			}
+		}
+
+		if (next_sel == vctrl->sel) {
+			/*
+			 * The voltage change can not be performed without
+			 * exceeding the OVP threshold.
+			 */
+			dev_err(&rdev->dev,
+				"voltage change would exceed OVP threshold\n");
+			ret = -EINVAL;
+			goto err;
+		}
+
+		ret = regulator_set_voltage(ctrl_reg,
+					    vctrl->vtable[next_sel].ctrl,
+					    vctrl->vtable[next_sel].ctrl);
+		if (ret) {
+			dev_err(&rdev->dev,
+				"failed to set control voltage to %duV\n",
+				vctrl->vtable[next_sel].ctrl);
+			goto err;
+		}
+
+		vctrl->sel = next_sel;
+
+		delay = DIV_ROUND_UP(uV - vctrl->vtable[next_sel].out,
+				     vctrl->min_slew_down_rate);
+		usleep_range(delay, delay + DIV_ROUND_UP(delay, 10));
+	}
+
+	return 0;
+
+err:
+	if (vctrl->sel != orig_sel) {
+		/* Try to go back to original voltage */
+		if (!regulator_set_voltage(ctrl_reg,
+					   vctrl->vtable[orig_sel].ctrl,
+					   vctrl->vtable[orig_sel].ctrl))
+			vctrl->sel = orig_sel;
+		else
+			dev_warn(&rdev->dev,
+				 "failed to restore original voltage\n");
+	}
+
+	return ret;
+}
+
+static int vctrl_list_voltage(struct regulator_dev *rdev,
+			      unsigned int selector)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+
+	if (selector >= rdev->desc->n_voltages)
+		return -EINVAL;
+
+	return vctrl->vtable[selector].out;
+}
+
+static int vctrl_parse_dt(struct platform_device *pdev,
+			  struct vctrl_data *vctrl)
+{
+	int ret;
+	struct device_node *np = pdev->dev.of_node;
+	const char *ctrl_reg;
+	u32 pval;
+	u32 vrange_ctrl[2];
+
+	ret = of_property_read_string(np, "ctrl-regulator", &ctrl_reg);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read ctrl-regulator: %d\n", ret);
+		return ret;
+	}
+
+	vctrl->ctrl_reg = devm_regulator_get(&pdev->dev, ctrl_reg);
+	if (IS_ERR(vctrl->ctrl_reg))
+		return PTR_ERR(vctrl->ctrl_reg);
+
+	ret = of_property_read_u32(np, "ovp-threshold-percent", &pval);
+	if (!ret) {
+		vctrl->ovp_threshold = pval;
+		if (vctrl->ovp_threshold > 100) {
+			dev_err(&pdev->dev,
+				"ovp-threshold-percent (%u) > 100\n",
+				vctrl->ovp_threshold);
+			return -EINVAL;
+		}
+	}
+
+	ret = of_property_read_u32(np, "min-slew-down-rate", &pval);
+	if (!ret) {
+		vctrl->min_slew_down_rate = pval;
+
+		/* We use the value as int and as divider; sanity check */
+		if (vctrl->min_slew_down_rate == 0) {
+			dev_err(&pdev->dev,
+				"min-slew-down-rate must not be 0\n");
+			return -EINVAL;
+		} else if (vctrl->min_slew_down_rate > INT_MAX) {
+			dev_err(&pdev->dev, "min-slew-down-rate (%u) too big\n",
+				vctrl->min_slew_down_rate);
+			return -EINVAL;
+		}
+	}
+
+	if (vctrl->ovp_threshold && !vctrl->min_slew_down_rate) {
+		dev_err(&pdev->dev,
+			"ovp-threshold-percent requires min-slew-down-rate\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(np, "regulator-min-microvolt", &pval);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read regulator-min-microvolt: %d\n", ret);
+		return ret;
+	}
+	vctrl->vrange.out.min_uV = pval;
+
+	ret = of_property_read_u32(np, "regulator-max-microvolt", &pval);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read regulator-max-microvolt: %d\n", ret);
+		return ret;
+	}
+	vctrl->vrange.out.max_uV = pval;
+
+	ret = of_property_read_u32_array(np, "ctrl-voltage-range", vrange_ctrl,
+					 2);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read ctrl-voltage-range: %d\n",
+			ret);
+		return ret;
+	}
+
+	if (vrange_ctrl[0] >= vrange_ctrl[1]) {
+		dev_err(&pdev->dev, "ctrl-voltage-range is invalid: %d-%d\n",
+			vrange_ctrl[0], vrange_ctrl[1]);
+		return -EINVAL;
+	}
+
+	vctrl->vrange.ctrl.min_uV = vrange_ctrl[0];
+	vctrl->vrange.ctrl.max_uV = vrange_ctrl[1];
+
+	return 0;
+}
+
+static int vctrl_cmp_ctrl_uV(const void *a, const void *b)
+{
+	const struct vctrl_voltage_table *at = a;
+	const struct vctrl_voltage_table *bt = b;
+
+	return at->ctrl - bt->ctrl;
+}
+
+static int vctrl_init_vtable(struct platform_device *pdev)
+{
+	struct vctrl_data *vctrl = platform_get_drvdata(pdev);
+	struct regulator_desc *rdesc = &vctrl->desc;
+	struct regulator *ctrl_reg = vctrl->ctrl_reg;
+	struct vctrl_voltage_range *vrange_ctrl = &vctrl->vrange.ctrl;
+	int n_voltages;
+	int ctrl_uV;
+	int i, idx_vt;
+
+	n_voltages = regulator_count_voltages(ctrl_reg);
+
+	rdesc->n_voltages = n_voltages;
+
+	/* determine number of steps within the range of the vctrl regulator */
+	for (i = 0; i < n_voltages; i++) {
+		ctrl_uV = regulator_list_voltage(ctrl_reg, i);
+
+		if (ctrl_uV < vrange_ctrl->min_uV ||
+		    ctrl_uV > vrange_ctrl->max_uV) {
+			rdesc->n_voltages--;
+			continue;
+		}
+	}
+
+	if (rdesc->n_voltages == 0) {
+		dev_err(&pdev->dev, "invalid configuration\n");
+		return -EINVAL;
+	}
+
+	vctrl->vtable = devm_kmalloc_array(
+		&pdev->dev, sizeof(struct vctrl_voltage_table),
+		rdesc->n_voltages, GFP_KERNEL);
+	if (!vctrl->vtable)
+		return -ENOMEM;
+
+	/* create mapping control <=> output voltage */
+	for (i = 0, idx_vt = 0; i < n_voltages; i++) {
+		ctrl_uV = regulator_list_voltage(ctrl_reg, i);
+
+		if (ctrl_uV < vrange_ctrl->min_uV ||
+		    ctrl_uV > vrange_ctrl->max_uV)
+			continue;
+
+		vctrl->vtable[idx_vt].ctrl = ctrl_uV;
+		vctrl->vtable[idx_vt].out =
+			vctrl_calc_output_voltage(vctrl, ctrl_uV);
+		idx_vt++;
+	}
+
+	/* we rely on the table to be ordered by ascending voltage */
+	sort(vctrl->vtable, rdesc->n_voltages,
+	     sizeof(struct vctrl_voltage_table), vctrl_cmp_ctrl_uV,
+	     NULL);
+
+	return 0;
+}
+
+static int vctrl_enable(struct regulator_dev *rdev)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+	int ret = regulator_enable(vctrl->ctrl_reg);
+	if (!ret)
+		vctrl->enabled = true;
+
+	return ret;
+}
+
+static int vctrl_disable(struct regulator_dev *rdev)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+	int ret = regulator_disable(vctrl->ctrl_reg);
+	if (!ret)
+		vctrl->enabled = false;
+
+	return ret;
+}
+
+static int vctrl_is_enabled(struct regulator_dev *rdev)
+{
+	struct vctrl_data *vctrl = rdev_get_drvdata(rdev);
+	return vctrl->enabled;
+}
+
+static const struct regulator_ops vctrl_ops_cont = {
+	.enable		  = vctrl_enable,
+	.disable	  = vctrl_disable,
+	.is_enabled	  = vctrl_is_enabled,
+	.get_voltage	  = vctrl_get_voltage,
+	.set_voltage	  = vctrl_set_voltage,
+};
+
+static const struct regulator_ops vctrl_ops_non_cont = {
+	.enable		  = vctrl_enable,
+	.disable	  = vctrl_disable,
+	.is_enabled	  = vctrl_is_enabled,
+	.set_voltage_sel = vctrl_set_voltage_sel,
+	.get_voltage_sel = vctrl_get_voltage_sel,
+	.list_voltage    = vctrl_list_voltage,
+	.map_voltage     = regulator_map_voltage_iterate,
+};
+
+static int vctrl_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct vctrl_data *vctrl;
+	const struct regulator_init_data *init_data;
+	struct regulator_desc *rdesc;
+	struct regulator_config cfg = { };
+	struct vctrl_voltage_range *vrange_ctrl;
+	int ctrl_uV;
+	int ret;
+
+	vctrl = devm_kzalloc(&pdev->dev, sizeof(struct vctrl_data),
+			     GFP_KERNEL);
+	if (!vctrl)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, vctrl);
+
+	ret = vctrl_parse_dt(pdev, vctrl);
+	if (ret)
+		return ret;
+
+	vrange_ctrl = &vctrl->vrange.ctrl;
+
+	rdesc = &vctrl->desc;
+	rdesc->name = "vctrl";
+	rdesc->type = REGULATOR_VOLTAGE;
+	rdesc->owner = THIS_MODULE;
+
+	if (regulator_has_continuous_voltage_range(vctrl->ctrl_reg)) {
+		rdesc->continuous_voltage_range = true;
+		rdesc->ops = &vctrl_ops_cont;
+	} else {
+		rdesc->ops = &vctrl_ops_non_cont;
+	}
+
+	init_data = of_get_regulator_init_data(&pdev->dev, np, rdesc);
+	if (!init_data)
+		return -ENOMEM;
+
+	cfg.of_node = np;
+	cfg.dev = &pdev->dev;
+	cfg.driver_data = vctrl;
+	cfg.init_data = init_data;
+
+	if (!rdesc->continuous_voltage_range) {
+		ret = vctrl_init_vtable(pdev);
+		if (ret)
+			return ret;
+
+		ctrl_uV = regulator_get_voltage(vctrl->ctrl_reg);
+		if (ctrl_uV < 0) {
+			dev_err(&pdev->dev, "failed to get control voltage\n");
+			return ctrl_uV;
+		}
+
+		/* determine current voltage selector from control voltage */
+		if (ctrl_uV < vrange_ctrl->min_uV) {
+			vctrl->sel = 0;
+		} else if (ctrl_uV > vrange_ctrl->max_uV) {
+			vctrl->sel = rdesc->n_voltages - 1;
+		} else {
+			int i;
+
+			for (i = 0; i < rdesc->n_voltages; i++) {
+				if (ctrl_uV == vctrl->vtable[i].ctrl) {
+					vctrl->sel = i;
+					break;
+				}
+			}
+		}
+	}
+
+	vctrl->rdev = devm_regulator_register(&pdev->dev, rdesc, &cfg);
+	if (IS_ERR(vctrl->rdev)) {
+		ret = PTR_ERR(vctrl->rdev);
+		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id vctrl_of_match[] = {
+	{ .compatible = "vctrl-regulator", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, vctrl_of_match);
+
+static struct platform_driver vctrl_driver = {
+	.probe		= vctrl_probe,
+	.driver		= {
+		.name		= "vctrl-regulator",
+		.of_match_table = of_match_ptr(vctrl_of_match),
+	},
+};
+
+module_platform_driver(vctrl_driver);
+
+MODULE_DESCRIPTION("Voltage Controlled Regulator Driver");
+MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.12.0.246.ga2ecc84866-goog

Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Mark Brown]

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

On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote:

> The new function allows consumers to determine if a regulator is
> continuous or discrete, and whether the results of

Why?  As we discussed at ELC this is an implementation detail of the
regulator and it's to an extent a taste decision if the regulator is
represented as a linear range or a continuous range (in fact given
improvements in the core we could probably just update all continuous
range regulators to linear ones).

> regulator_count_voltages() and regulator_list_voltage() correspond
> to the regulator itself or its supply.

Why?

> Change-Id: I1198cee9fff60dc747a02860e9652034f4d5da33

Don't include noise like this upstream.

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

Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Matthias Kaehlcke]

Hi Mark,

El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit:

> On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote:
> 
> > The new function allows consumers to determine if a regulator is
> > continuous or discrete, and whether the results of
> 
> Why?  As we discussed at ELC this is an implementation detail of the
> regulator and it's to an extent a taste decision if the regulator is
> represented as a linear range or a continuous range (in fact given
> improvements in the core we could probably just update all continuous
> range regulators to linear ones).

The second patch of this series is a driver for voltage controlled
regulators (vctrl), ie the output voltage of a vctrl regulator is
controlled through the voltage of another regulator. The control
regulator can be continuous or discrete and I think it makes sense for
the vctrl regulator to mirror its control regulator in this aspect.
Why should it pretend to have a continuous range when it is actually
discrete due to the control regulator?

Also the vctrl driver allows to break down a voltage change into
multiple steps to prevent overvoltage protection (OVP) circuitry
from shutting down the regulator when a voltage change in a
single step would exceed the OVP threshold.
For continuous control regulators we can simply calculate a "safe"
voltage for the next step and pass it to the control regulator. In
case of a discrete control regulator this calculated voltage may not
be directly available, without knowing the available steps the vctrl
driver has to try different voltage ranges until it finds one that is
accepted by the control regulator. Obviously this can be done but it
adds code complexity and runtime overhead which is not necessary if we
know the available steps (and regulator_list_voltage() is already
there to provide them).

> > regulator_count_voltages() and regulator_list_voltage() correspond
> > to the regulator itself or its supply.
> 
> Why?

Please see my explication above on why the vctrl driver needs to know
this.

In general I think the behavior of these APIs can be confusing for
users without intimate knowledge of the regulator core. For me (as a
possibly naive user) it isn't clear why regulator_count_voltages() of
a continuous regulator would return the voltage count of its supply,
instead of a value like 0 or -EINVAL that indicates that it is
continuous. Similar for regulator_list_voltage(). I'm sure there are
reasons for it, but it's not really intuitive.

Above you characterize discrete vs. continuous as an implementation
detail. Aren't we already exposing large parts of it through
regulator_count_voltages() and regulator_list_voltage()?

> > Change-Id: I1198cee9fff60dc747a02860e9652034f4d5da33
> 
> Don't include noise like this upstream.

Sorry, will remove

Matthias

Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Mark Brown]

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

On Thu, Mar 09, 2017 at 11:40:54AM -0800, Matthias Kaehlcke wrote:
> El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit:
> > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote:

> > > The new function allows consumers to determine if a regulator is
> > > continuous or discrete, and whether the results of

> > Why?  As we discussed at ELC this is an implementation detail of the
> > regulator and it's to an extent a taste decision if the regulator is
> > represented as a linear range or a continuous range (in fact given
> > improvements in the core we could probably just update all continuous
> > range regulators to linear ones).

> The second patch of this series is a driver for voltage controlled
> regulators (vctrl), ie the output voltage of a vctrl regulator is
> controlled through the voltage of another regulator. The control
> regulator can be continuous or discrete and I think it makes sense for
> the vctrl regulator to mirror its control regulator in this aspect.
> Why should it pretend to have a continuous range when it is actually
> discrete due to the control regulator?

I don't think we should be providing a consumer facing API which invites
consumers to peer into the implementation details of regulators, or
gives them the idea that these concepts exist.  Consumers can already
enumerate the set of supported voltages via _list_voltage() and so on,
I'd expect consumers to be able to get what they need from those.  You
have an incrediblity specialist use case here but this is just a general
consumer interface that's being added.

> For continuous control regulators we can simply calculate a "safe"
> voltage for the next step and pass it to the control regulator. In
> case of a discrete control regulator this calculated voltage may not
> be directly available, without knowing the available steps the vctrl
> driver has to try different voltage ranges until it finds one that is
> accepted by the control regulator. Obviously this can be done but it
> adds code complexity and runtime overhead which is not necessary if we
> know the available steps (and regulator_list_voltage() is already
> there to provide them).

I'm not seeing how knowing if a regulator is continuous is helpful for
regulators with discrete voltages?

> > > regulator_count_voltages() and regulator_list_voltage() correspond
> > > to the regulator itself or its supply.

> > Why?

> Please see my explication above on why the vctrl driver needs to know
> this.

I'm seeing nothing in the above that addresses my question, you don't
even seem to have mentioned supplies.  

> In general I think the behavior of these APIs can be confusing for
> users without intimate knowledge of the regulator core. For me (as a
> possibly naive user) it isn't clear why regulator_count_voltages() of
> a continuous regulator would return the voltage count of its supply,
> instead of a value like 0 or -EINVAL that indicates that it is
> continuous. Similar for regulator_list_voltage(). I'm sure there are
> reasons for it, but it's not really intuitive.

This is happening because continuous regulators are an infrequently used
hack and not every case where they are relevant has been caught.  If the
abstractions are confusing or not working then let's improve them, not
just punch holes in the abstraction layers and make the problem worse.
What you're effectively saying is "I don't really understand what's
going on but this seems to work for me" which is a fairly big warning
sign that the solution isn't great.

The reason we report properties of the parent supply if the child supply
has no control is so that we can pass operations on up to the parent to
implement them there, supporting things like dumb power switches.  We
shouldn't be leaking details of the parent regulator for anything that
does actually regulate.

> Above you characterize discrete vs. continuous as an implementation
> detail. Aren't we already exposing large parts of it through
> regulator_count_voltages() and regulator_list_voltage()?

What we should be doing for continuous regulators is allowing people to
list the supported voltages as they would for other regulators.

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

Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Matthias Kaehlcke]

Hi Mark,

El Fri, Mar 17, 2017 at 09:15:47PM +0000 Mark Brown ha dit:

> On Thu, Mar 09, 2017 at 11:40:54AM -0800, Matthias Kaehlcke wrote:
> > El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit:
> > > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote:
> 
> > > > The new function allows consumers to determine if a regulator is
> > > > continuous or discrete, and whether the results of
> 
> > > Why?  As we discussed at ELC this is an implementation detail of the
> > > regulator and it's to an extent a taste decision if the regulator is
> > > represented as a linear range or a continuous range (in fact given
> > > improvements in the core we could probably just update all continuous
> > > range regulators to linear ones).
> 
> > The second patch of this series is a driver for voltage controlled
> > regulators (vctrl), ie the output voltage of a vctrl regulator is
> > controlled through the voltage of another regulator. The control
> > regulator can be continuous or discrete and I think it makes sense for
> > the vctrl regulator to mirror its control regulator in this aspect.
> > Why should it pretend to have a continuous range when it is actually
> > discrete due to the control regulator?
> 
> I don't think we should be providing a consumer facing API which invites
> consumers to peer into the implementation details of regulators, or
> gives them the idea that these concepts exist.  Consumers can already
> enumerate the set of supported voltages via _list_voltage() and so on,
> I'd expect consumers to be able to get what they need from those.  You
> have an incrediblity specialist use case here but this is just a general
> consumer interface that's being added.

In principle I totally agree with you that consumers should be able
to enumerate the supported voltages with the existing functions. And
they can, as long as they already know (or assume) that the regulator
they are using actually has discrete steps, otherwise they might get
unexpected results.

You are right that my case is very specialist, however I think it is
a general problem that a consumer can't know whether the results of
_list_voltage(), etc correspond to the regulator itself or to its
supplies. E.g. a consumer might have a continuous reg which is
supplied by a discrete reg, in this case _list_voltage() would return
the steps of the supply reg, which is probably not what most consumers
expect.

> > For continuous control regulators we can simply calculate a "safe"
> > voltage for the next step and pass it to the control regulator. In
> > case of a discrete control regulator this calculated voltage may not
> > be directly available, without knowing the available steps the vctrl
> > driver has to try different voltage ranges until it finds one that is
> > accepted by the control regulator. Obviously this can be done but it
> > adds code complexity and runtime overhead which is not necessary if we
> > know the available steps (and regulator_list_voltage() is already
> > there to provide them).
> 
> I'm not seeing how knowing if a regulator is continuous is helpful for
> regulators with discrete voltages?

The main purpose of the function for vctrl is: Do the values reported
by _count_voltages()/_list_voltage() actually describe this regulator
or do I ignore them because they come from the regulators' supply?

> > > > regulator_count_voltages() and regulator_list_voltage() correspond
> > > > to the regulator itself or its supply.
> 
> > > Why?
> 
> > Please see my explication above on why the vctrl driver needs to know
> > this.
> 
> I'm seeing nothing in the above that addresses my question, you don't
> even seem to have mentioned supplies.  

Sorry, I really didn't try to evade your question. Does it make more
sense with the example above?

> > In general I think the behavior of these APIs can be confusing for
> > users without intimate knowledge of the regulator core. For me (as a
> > possibly naive user) it isn't clear why regulator_count_voltages() of
> > a continuous regulator would return the voltage count of its supply,
> > instead of a value like 0 or -EINVAL that indicates that it is
> > continuous. Similar for regulator_list_voltage(). I'm sure there are
> > reasons for it, but it's not really intuitive.
> 
> This is happening because continuous regulators are an infrequently used
> hack and not every case where they are relevant has been caught.  If the
> abstractions are confusing or not working then let's improve them, not
> just punch holes in the abstraction layers and make the problem worse.
> What you're effectively saying is "I don't really understand what's
> going on but this seems to work for me" which is a fairly big warning
> sign that the solution isn't great.

I'm not happy with this function either and agree that the (perceived)
need for it could be an indication of an issue in the abstraction. The
function was useful to unblock me on certain aspects of the vctrl
driver, please interpret it more as an RFC than 'I need exactly this'.

> The reason we report properties of the parent supply if the child supply
> has no control is so that we can pass operations on up to the parent to
> implement them there, supporting things like dumb power switches.  We
> shouldn't be leaking details of the parent regulator for anything that
> does actually regulate.

I see, for dumb power switches it seems indeed reasonable to 'forward'
the details of the parent regulator. In most other cases we don't want
that for the sake of encapsulation and to give consumers values that
describe the regulator they are inquiring about (even if that just
means 'no values'). So we probably want a flag or some other mechanism
to indicate whether to 'forward' the parents' details or not.

With regulator_count_voltages() returning 0 or -EINVAL there would be
no need for regulator_has_continuous_voltage_range(), and I would be
more than happy to live without it.

> > Above you characterize discrete vs. continuous as an implementation
> > detail. Aren't we already exposing large parts of it through
> > regulator_count_voltages() and regulator_list_voltage()?
> 
> What we should be doing for continuous regulators is allowing people to
> list the supported voltages as they would for other regulators.

In the overall regulator context this may make sense, at this point I
don't really have enough background on the subsystem to have an
informed opinion.

>From the vctrl perspective I wouldn't be overly happy, since it
wouldn't allow to distinguish between continuous and discrete
supplies, and I still think that handling discrete supplies
differently is simpler/more efficient. This doesn't mean I argue
against your proposal if it is deemed the right thing from a subsystem
perspective.

Thanks for the constructive discussion!

Matthias

Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Mark Brown]

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

On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote:

> In principle I totally agree with you that consumers should be able
> to enumerate the supported voltages with the existing functions. And
> they can, as long as they already know (or assume) that the regulator
> they are using actually has discrete steps, otherwise they might get
> unexpected results.

Given the limits of number representation continuous regulators also
have discrete steps, they just have a lot of them (but so do some
regulators we currently say aren't continuous so...).

> You are right that my case is very specialist, however I think it is
> a general problem that a consumer can't know whether the results of
> _list_voltage(), etc correspond to the regulator itself or to its
> supplies. E.g. a consumer might have a continuous reg which is
> supplied by a discrete reg, in this case _list_voltage() would return
> the steps of the supply reg, which is probably not what most consumers
> expect.

No, this is doesn't make much sense!  Why should we be reporting
properties of the parent regulator when the child regulator is
regulating away all visibility of those properties?

> > > Please see my explication above on why the vctrl driver needs to know
> > > this.

> > I'm seeing nothing in the above that addresses my question, you don't
> > even seem to have mentioned supplies.  

> Sorry, I really didn't try to evade your question. Does it make more
> sense with the example above?

No, not at all.

> > What we should be doing for continuous regulators is allowing people to
> > list the supported voltages as they would for other regulators.

> In the overall regulator context this may make sense, at this point I
> don't really have enough background on the subsystem to have an
> informed opinion.

> From the vctrl perspective I wouldn't be overly happy, since it
> wouldn't allow to distinguish between continuous and discrete
> supplies, and I still think that handling discrete supplies
> differently is simpler/more efficient. This doesn't mean I argue
> against your proposal if it is deemed the right thing from a subsystem
> perspective.

If it helps think of a continuous regulator as a discrete regulator with
a base voltage of 0 and steps of 1uV.

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

Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Matthias Kaehlcke]

El Mon, Mar 20, 2017 at 12:06:15PM +0000 Mark Brown ha dit:

> On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote:
> 
> > In principle I totally agree with you that consumers should be able
> > to enumerate the supported voltages with the existing functions. And
> > they can, as long as they already know (or assume) that the regulator
> > they are using actually has discrete steps, otherwise they might get
> > unexpected results.
> 
> Given the limits of number representation continuous regulators also
> have discrete steps, they just have a lot of them (but so do some
> regulators we currently say aren't continuous so...).
> 
> > You are right that my case is very specialist, however I think it is
> > a general problem that a consumer can't know whether the results of
> > _list_voltage(), etc correspond to the regulator itself or to its
> > supplies. E.g. a consumer might have a continuous reg which is
> > supplied by a discrete reg, in this case _list_voltage() would return
> > the steps of the supply reg, which is probably not what most consumers
> > expect.
> 
> No, this is doesn't make much sense!  Why should we be reporting
> properties of the parent regulator when the child regulator is
> regulating away all visibility of those properties?

I am confused whether you are confirming that the current behavior
makes no sense or if you think that what I'm saying is nonsense.

> > > > Please see my explication above on why the vctrl driver needs to know
> > > > this.
> 
> > > I'm seeing nothing in the above that addresses my question, you don't
> > > even seem to have mentioned supplies.  
> 
> > Sorry, I really didn't try to evade your question. Does it make more
> > sense with the example above?
> 
> No, not at all.

I take this as an indication that you don't think my description above
is correct.

Let's use a real world example then, tested with actual software and
hardware.

Our regulator is 'ppvar_bigcpu':

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/release-R58-9334.B-chromeos-4.4/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi#177

For our test we change the supply to 'pp1200_lpddr' to make sure the
supply has at least one voltage and this voltage is within the
constraints of 'ppvar_bigcpu'.

'ppvar_bigcpu' is continuous, however _count_voltages(ppvar_bigcpu)
returns 1 and _list_voltage(ppvar_bigcpu, 0) returns 1200000, which is
precisely the configuration of 'pp1200_lpddr'. I verified this with a
4.10ish kernel (no major changes, zero changes in regulators).

> > > What we should be doing for continuous regulators is allowing people to
> > > list the supported voltages as they would for other regulators.
> 
> > In the overall regulator context this may make sense, at this point I
> > don't really have enough background on the subsystem to have an
> > informed opinion.
> 
> > From the vctrl perspective I wouldn't be overly happy, since it
> > wouldn't allow to distinguish between continuous and discrete
> > supplies, and I still think that handling discrete supplies
> > differently is simpler/more efficient. This doesn't mean I argue
> > against your proposal if it is deemed the right thing from a subsystem
> > perspective.
> 
> If it helps think of a continuous regulator as a discrete regulator with
> a base voltage of 0 and steps of 1uV.

Thanks, I understood that. What I didn't realize initially is that we
can avoid iterating through all the voltages if the regulator has
linear steps, which we can determine with regulator_get_linear_step().
With that in mind I don't see concerns from the vctrl perspective.

Matthias

Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()

[Mark Brown]

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

On Thu, Mar 23, 2017 at 02:40:16PM -0700, Matthias Kaehlcke wrote:
> El Mon, Mar 20, 2017 at 12:06:15PM +0000 Mark Brown ha dit:
> > On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote:

> > > You are right that my case is very specialist, however I think it is
> > > a general problem that a consumer can't know whether the results of
> > > _list_voltage(), etc correspond to the regulator itself or to its
> > > supplies. E.g. a consumer might have a continuous reg which is
> > > supplied by a discrete reg, in this case _list_voltage() would return
> > > the steps of the supply reg, which is probably not what most consumers
> > > expect.

> > No, this is doesn't make much sense!  Why should we be reporting
> > properties of the parent regulator when the child regulator is
> > regulating away all visibility of those properties?

> I am confused whether you are confirming that the current behavior
> makes no sense or if you think that what I'm saying is nonsense.

I'm saying that the current behaviour isn't good and that the
incoherence of what you're proposing should make this clear to you.

> > If it helps think of a continuous regulator as a discrete regulator with
> > a base voltage of 0 and steps of 1uV.

> Thanks, I understood that. What I didn't realize initially is that we
> can avoid iterating through all the voltages if the regulator has
> linear steps, which we can determine with regulator_get_linear_step().
> With that in mind I don't see concerns from the vctrl perspective.

OK, good.  I keep meaning to look into retooling all the continuous
regulators to actually be linear regulators to simplify things.

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