[PATCH 0/8] regulator: pwm-regulator: Introduce continuous-mode

[PATCH 0/8] regulator: pwm-regulator: Introduce continuous-mode

[Lee Jones]

Continuous mode uses the PWM regulator's maximum and minimum supplied
voltages specified in the regulator-{min,max}-microvolt properties to
calculate appropriate duty-cycle values.  This allows for a much more
fine grained solution when compared with voltage-table mode, which
this driver already supports.  This solution does make an assumption
that a %50 duty-cycle value will cause the regulator voltage to run
at half way between the supplied max_uV and min_uV values.

Lee Jones (8):
  ARM: STi: STiH407: Add PWM Regulator node
  regulator: pwm-regulator: Remove unnecessary descriptor attribute from ddata
  regulator: pwm-regulator: Remove superfluous is_enabled check
  regulator: pwm-regulator: Diffientiate between dev (device) and rdev (regulator_dev)
  regulator: pwm-regulator: Don't assign structure attributes right away
  regulator: pwm-regulator: Separate voltage-table initialisation
  regulator: pwm-regulator: Add support for continuous-voltage
  regulator: pwm-regulator: Re-write bindings

 .../bindings/regulator/pwm-regulator.txt           |  66 ++++--
 arch/arm/boot/dts/stih407-family.dtsi              |  11 +
 drivers/regulator/pwm-regulator.c                  | 222 +++++++++++++++------
 3 files changed, 229 insertions(+), 70 deletions(-)

-- 
1.9.1

[PATCH 1/8] ARM: STi: STiH407: Add PWM Regulator node

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 3c626099..a7389a3 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -82,6 +82,17 @@
 		interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
+	pwm_regulator: pwm-regulator {
+		compatible = "pwm-regulator";
+		pwms = <&pwm1 3 8448>;
+		regulator-name = "CPU_1V0_AVS";
+		regulator-min-microvolt = <784000>;
+		regulator-max-microvolt = <1299000>;
+		regulator-always-on;
+		max-duty-cycle = <255>;
+		status = "okay";
+	};
+
 	soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.9.1

[PATCH 2/8] regulator: pwm-regulator: Remove unnecessary descriptor attribute from ddata

[Lee Jones]

The Regulator Device keeps a full copy of it's own, which can be easily accessed.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/pwm-regulator.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index cf2a39b..4a071b6 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -21,7 +21,6 @@
 #include <linux/pwm.h>
 
 struct pwm_regulator_data {
-	struct regulator_desc desc;
 	struct pwm_voltages *duty_cycle_table;
 	struct pwm_device *pwm;
 	bool enabled;
@@ -78,7 +77,7 @@ static int pwm_regulator_list_voltage(struct regulator_dev *dev,
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
 
-	if (selector >= drvdata->desc.n_voltages)
+	if (selector >= dev->desc->n_voltages)
 		return -EINVAL;
 
 	return drvdata->duty_cycle_table[selector].uV;
@@ -91,7 +90,7 @@ static struct regulator_ops pwm_regulator_voltage_ops = {
 	.map_voltage     = regulator_map_voltage_iterate,
 };
 
-static const struct regulator_desc pwm_regulator_desc = {
+static struct regulator_desc pwm_regulator_desc = {
 	.name		= "pwm-regulator",
 	.ops		= &pwm_regulator_voltage_ops,
 	.type		= REGULATOR_VOLTAGE,
@@ -117,8 +116,6 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
-	memcpy(&drvdata->desc, &pwm_regulator_desc, sizeof(pwm_regulator_desc));
-
 	/* determine the number of voltage-table */
 	prop = of_find_property(np, "voltage-table", &length);
 	if (!prop) {
@@ -133,7 +130,7 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	drvdata->desc.n_voltages = length / sizeof(*drvdata->duty_cycle_table);
+	pwm_regulator_desc.n_voltages = length / sizeof(*drvdata->duty_cycle_table);
 
 	drvdata->duty_cycle_table = devm_kzalloc(&pdev->dev,
 						 length, GFP_KERNEL);
@@ -150,7 +147,7 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	}
 
 	config.init_data = of_get_regulator_init_data(&pdev->dev, np,
-						      &drvdata->desc);
+						      &pwm_regulator_desc);
 	if (!config.init_data)
 		return -ENOMEM;
 
@@ -165,10 +162,10 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	}
 
 	regulator = devm_regulator_register(&pdev->dev,
-					    &drvdata->desc, &config);
+					    &pwm_regulator_desc, &config);
 	if (IS_ERR(regulator)) {
 		dev_err(&pdev->dev, "Failed to register regulator %s\n",
-			drvdata->desc.name);
+			pwm_regulator_desc.name);
 		return PTR_ERR(regulator);
 	}
 
-- 
1.9.1

[PATCH 3/8] regulator: pwm-regulator: Remove superfluous is_enabled check

[Lee Jones]

The core framework already takes care of this.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/pwm-regulator.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 4a071b6..36f9684 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -23,7 +23,6 @@
 struct pwm_regulator_data {
 	struct pwm_voltages *duty_cycle_table;
 	struct pwm_device *pwm;
-	bool enabled;
 	int state;
 };
 
@@ -60,13 +59,10 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *dev,
 
 	drvdata->state = selector;
 
-	if (!drvdata->enabled) {
-		ret = pwm_enable(drvdata->pwm);
-		if (ret) {
-			dev_err(&dev->dev, "Failed to enable PWM\n");
-			return ret;
-		}
-		drvdata->enabled = true;
+	ret = pwm_enable(drvdata->pwm);
+	if (ret) {
+		dev_err(&dev->dev, "Failed to enable PWM\n");
+		return ret;
 	}
 
 	return 0;
-- 
1.9.1

[PATCH 4/8] regulator: pwm-regulator: Diffientiate between dev (device) and rdev (regulator_dev)

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/pwm-regulator.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 36f9684..ae32086 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -31,17 +31,17 @@ struct pwm_voltages {
 	unsigned int dutycycle;
 };
 
-static int pwm_regulator_get_voltage_sel(struct regulator_dev *dev)
+static int pwm_regulator_get_voltage_sel(struct regulator_dev *rdev)
 {
-	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
 
 	return drvdata->state;
 }
 
-static int pwm_regulator_set_voltage_sel(struct regulator_dev *dev,
+static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
 					 unsigned selector)
 {
-	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
 	unsigned int pwm_reg_period;
 	int dutycycle;
 	int ret;
@@ -53,7 +53,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *dev,
 
 	ret = pwm_config(drvdata->pwm, dutycycle, pwm_reg_period);
 	if (ret) {
-		dev_err(&dev->dev, "Failed to configure PWM\n");
+		dev_err(&rdev->dev, "Failed to configure PWM\n");
 		return ret;
 	}
 
@@ -61,19 +61,19 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *dev,
 
 	ret = pwm_enable(drvdata->pwm);
 	if (ret) {
-		dev_err(&dev->dev, "Failed to enable PWM\n");
+		dev_err(&rdev->dev, "Failed to enable PWM\n");
 		return ret;
 	}
 
 	return 0;
 }
 
-static int pwm_regulator_list_voltage(struct regulator_dev *dev,
+static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
 				      unsigned selector)
 {
-	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
 
-	if (selector >= dev->desc->n_voltages)
+	if (selector >= rdev->desc->n_voltages)
 		return -EINVAL;
 
 	return drvdata->duty_cycle_table[selector].uV;
-- 
1.9.1

[PATCH 5/8] regulator: pwm-regulator: Don't assign structure attributes right away

[Lee Jones]

Perhaps this is just personal preference, but ...

This patch introduces a new local variable to receive and test regulator
initialisation data.  It simplifies and cleans up the code making it
that little bit easier to read and maintain.  The local value is assigned
to the structure attribute when all the others are.  This is the way we
usually do things.

Prevents this kind of nonsense:

	this->is->just.silly = fetch_silly_value(&pointer);
	if (!this->is->just.silly) {
		printk("Silly value failed: %d\n", this->is->just.silly);
		return this->is->just.silly;
	}

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/pwm-regulator.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index ae32086..779ecf9 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -96,6 +96,7 @@ static struct regulator_desc pwm_regulator_desc = {
 
 static int pwm_regulator_probe(struct platform_device *pdev)
 {
+	const struct regulator_init_data *init_data;
 	struct pwm_regulator_data *drvdata;
 	struct property *prop;
 	struct regulator_dev *regulator;
@@ -142,14 +143,15 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	config.init_data = of_get_regulator_init_data(&pdev->dev, np,
-						      &pwm_regulator_desc);
-	if (!config.init_data)
+	init_data = of_get_regulator_init_data(&pdev->dev, np,
+					       &pwm_regulator_desc);
+	if (init_data)
 		return -ENOMEM;
 
 	config.of_node = np;
 	config.dev = &pdev->dev;
 	config.driver_data = drvdata;
+	config.init_data = init_data;
 
 	drvdata->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(drvdata->pwm)) {
-- 
1.9.1

[PATCH 6/8] regulator: pwm-regulator: Separate voltage-table initialisation

[Lee Jones]

Take this out of the main .probe() routine in order to facilitate the
introduction of different ways to obtain 'duty cycle' information.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/pwm-regulator.c | 77 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 779ecf9..5a18437 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -78,8 +78,7 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
 
 	return drvdata->duty_cycle_table[selector].uV;
 }
-
-static struct regulator_ops pwm_regulator_voltage_ops = {
+static struct regulator_ops pwm_regulator_voltage_table_ops = {
 	.set_voltage_sel = pwm_regulator_set_voltage_sel,
 	.get_voltage_sel = pwm_regulator_get_voltage_sel,
 	.list_voltage    = pwm_regulator_list_voltage,
@@ -88,21 +87,56 @@ static struct regulator_ops pwm_regulator_voltage_ops = {
 
 static struct regulator_desc pwm_regulator_desc = {
 	.name		= "pwm-regulator",
-	.ops		= &pwm_regulator_voltage_ops,
 	.type		= REGULATOR_VOLTAGE,
 	.owner		= THIS_MODULE,
 	.supply_name    = "pwm",
 };
 
+static int pwm_regulator_init_table(struct platform_device *pdev,
+				    struct pwm_regulator_data *drvdata)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct pwm_voltages *duty_cycle_table;
+	int length;
+	int ret;
+
+	of_find_property(np, "voltage-table", &length);
+
+	if ((length < sizeof(*duty_cycle_table)) ||
+	    (length % sizeof(*duty_cycle_table))) {
+		dev_err(&pdev->dev,
+			"voltage-table length(%d) is invalid\n",
+			length);
+		return -EINVAL;
+	}
+
+	duty_cycle_table = devm_kzalloc(&pdev->dev, length, GFP_KERNEL);
+	if (!duty_cycle_table)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(np, "voltage-table",
+					 (u32 *)duty_cycle_table,
+					 length / sizeof(u32));
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read voltage-table\n");
+		return ret;
+	}
+
+	drvdata->duty_cycle_table	= duty_cycle_table;
+	pwm_regulator_desc.ops		= &pwm_regulator_voltage_table_ops;
+	pwm_regulator_desc.n_voltages	= length / sizeof(*duty_cycle_table);
+
+	return 0;
+}
+
 static int pwm_regulator_probe(struct platform_device *pdev)
 {
 	const struct regulator_init_data *init_data;
 	struct pwm_regulator_data *drvdata;
-	struct property *prop;
 	struct regulator_dev *regulator;
 	struct regulator_config config = { };
 	struct device_node *np = pdev->dev.of_node;
-	int length, ret;
+	int ret;
 
 	if (!np) {
 		dev_err(&pdev->dev, "Device Tree node missing\n");
@@ -113,36 +147,15 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
-	/* determine the number of voltage-table */
-	prop = of_find_property(np, "voltage-table", &length);
-	if (!prop) {
-		dev_err(&pdev->dev, "No voltage-table\n");
-		return -EINVAL;
-	}
-
-	if ((length < sizeof(*drvdata->duty_cycle_table)) ||
-	    (length % sizeof(*drvdata->duty_cycle_table))) {
-		dev_err(&pdev->dev, "voltage-table length(%d) is invalid\n",
-			length);
+	if (of_find_property(np, "voltage-table", NULL)) {
+		ret = pwm_regulator_init_table(pdev, drvdata);
+		if (ret)
+			return ret;
+	} else {
+		dev_err(&pdev->dev, "No \"voltage-table\" supplied\n");
 		return -EINVAL;
 	}
 
-	pwm_regulator_desc.n_voltages = length / sizeof(*drvdata->duty_cycle_table);
-
-	drvdata->duty_cycle_table = devm_kzalloc(&pdev->dev,
-						 length, GFP_KERNEL);
-	if (!drvdata->duty_cycle_table)
-		return -ENOMEM;
-
-	/* read voltage table from DT property */
-	ret = of_property_read_u32_array(np, "voltage-table",
-					 (u32 *)drvdata->duty_cycle_table,
-					 length / sizeof(u32));
-	if (ret < 0) {
-		dev_err(&pdev->dev, "read voltage-table failed\n");
-		return ret;
-	}
-
 	init_data = of_get_regulator_init_data(&pdev->dev, np,
 					       &pwm_regulator_desc);
 	if (init_data)
-- 
1.9.1

[PATCH 7/8] regulator: pwm-regulator: Add support for continuous-voltage

[Lee Jones]

The current version of PWM regulator only supports a static table
approach, where pre-calculated values are supplied by the vendor and
obtained via DT.  The continuous-voltage method takes min_uV and
max_uV, and divides the difference between them up into a number of
slices.  The number of slices depend on how large the duty cycle
register is.  This information is provided by a DT property.

As the name alludes, this provides values for a continuous voltage
range between min_uV and max_uV, which has obvious benefits over
either limited voltage possibilities, or the requirement to provide
a large voltage-table.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/regulator/pwm-regulator.c | 114 +++++++++++++++++++++++++++++++++++---
 1 file changed, 106 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 5a18437..85a9795 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -10,6 +10,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/err.h>
@@ -21,9 +22,16 @@
 #include <linux/pwm.h>
 
 struct pwm_regulator_data {
-	struct pwm_voltages *duty_cycle_table;
+	/*  Shared */
 	struct pwm_device *pwm;
+
+	/* Voltage table */
+	struct pwm_voltages *duty_cycle_table;
 	int state;
+
+	/* Continuous voltage */
+	u32 max_duty_cycle;
+	int volt_uV;
 };
 
 struct pwm_voltages {
@@ -31,6 +39,9 @@ struct pwm_voltages {
 	unsigned int dutycycle;
 };
 
+/**
+ * Voltage table call-backs
+ */
 static int pwm_regulator_get_voltage_sel(struct regulator_dev *rdev)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
@@ -78,6 +89,71 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
 
 	return drvdata->duty_cycle_table[selector].uV;
 }
+
+/**
+ * Continuous voltage call-backs
+ */
+static int pwm_voltage_to_duty_cycle(struct regulator_dev *rdev,
+					int volt_mV)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+	int min_mV = rdev->constraints->min_uV / 1000;
+	int max_mV = rdev->constraints->max_uV / 1000;
+	int max_duty_cycle = drvdata->max_duty_cycle;
+	int vdiff = min_mV - max_mV;
+	int pwm_code;
+	int tmp;
+
+	tmp = max_duty_cycle - min_mV * max_duty_cycle / vdiff;
+	pwm_code = tmp + max_duty_cycle * volt_mV / vdiff;
+
+	if (pwm_code < 0)
+		pwm_code = 0;
+	if (pwm_code > max_duty_cycle)
+		pwm_code = max_duty_cycle;
+
+	return pwm_code * 100 / max_duty_cycle;
+}
+
+static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+
+	return drvdata->volt_uV;
+}
+
+static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
+					int min_uV, int max_uV,
+					unsigned *selector)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+	unsigned int ramp_delay = rdev->constraints->ramp_delay;
+	int duty_cycle;
+	int ret;
+
+	duty_cycle = pwm_voltage_to_duty_cycle(rdev, min_uV / 1000);
+
+	ret = pwm_config(drvdata->pwm,
+			 (drvdata->pwm->period / 100) * duty_cycle,
+			 drvdata->pwm->period);
+	if (ret) {
+		dev_err(&rdev->dev, "Failed to configure PWM\n");
+		return ret;
+	}
+
+	ret = pwm_enable(drvdata->pwm);
+	if (ret) {
+		dev_err(&rdev->dev, "Failed to enable PWM\n");
+		return ret;
+	}
+	drvdata->volt_uV = min_uV;
+
+	/* Delay required by PWM regulator to settle to the new voltage */
+	usleep_range(ramp_delay, ramp_delay + 1000);
+
+	return 0;
+}
+
 static struct regulator_ops pwm_regulator_voltage_table_ops = {
 	.set_voltage_sel = pwm_regulator_set_voltage_sel,
 	.get_voltage_sel = pwm_regulator_get_voltage_sel,
@@ -85,6 +161,11 @@ static struct regulator_ops pwm_regulator_voltage_table_ops = {
 	.map_voltage     = regulator_map_voltage_iterate,
 };
 
+static struct regulator_ops pwm_regulator_voltage_continuous_ops = {
+	.get_voltage = pwm_regulator_get_voltage,
+	.set_voltage = pwm_regulator_set_voltage,
+};
+
 static struct regulator_desc pwm_regulator_desc = {
 	.name		= "pwm-regulator",
 	.type		= REGULATOR_VOLTAGE,
@@ -129,6 +210,25 @@ static int pwm_regulator_init_table(struct platform_device *pdev,
 	return 0;
 }
 
+static int pwm_regulator_init_continuous(struct platform_device *pdev,
+					 struct pwm_regulator_data *drvdata)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	ret = of_property_read_u32(np, "max-duty-cycle",
+				   &drvdata->max_duty_cycle);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read \"pwm-max-value\"\n");
+		return ret;
+	}
+
+	pwm_regulator_desc.ops = &pwm_regulator_voltage_continuous_ops;
+	pwm_regulator_desc.continuous_voltage_range = true;
+
+	return 0;
+}
+
 static int pwm_regulator_probe(struct platform_device *pdev)
 {
 	const struct regulator_init_data *init_data;
@@ -147,14 +247,12 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
-	if (of_find_property(np, "voltage-table", NULL)) {
+	if (of_find_property(np, "voltage-table", NULL))
 		ret = pwm_regulator_init_table(pdev, drvdata);
-		if (ret)
-			return ret;
-	} else {
-		dev_err(&pdev->dev, "No \"voltage-table\" supplied\n");
-		return -EINVAL;
-	}
+	else
+		ret = pwm_regulator_init_continuous(pdev, drvdata);
+	if (ret)
+		return ret;
 
 	init_data = of_get_regulator_init_data(&pdev->dev, np,
 					       &pwm_regulator_desc);
-- 
1.9.1

[PATCH 8/8] dt: regulator: pwm-regulator: Re-write bindings

[Lee Jones]

* Add support for continuous-voltage mode
* Put more meat on the bones with regards to voltage-table mode
* Sort out formatting for ease of consumption

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../bindings/regulator/pwm-regulator.txt           | 66 ++++++++++++++++++----
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
index ce91f61..9a8269f 100644
--- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
@@ -1,27 +1,69 @@
-pwm regulator bindings
+Bindings for the Generic PWM Regulator
+======================================
+
+Currently supports 2 modes of operation:
+
+voltage-table:		When in this mode, a voltage table (See below) of predefined
+			voltage <=> duty-cycle values must be provided via DT.
+			Limitations are that the regulator can only operate at the
+			voltages supplied in the table.  Intermediary duty-cycle
+			values which would normally allow finer grained voltage
+			selection are ignored and rendered useless.  Although more
+			control is given to the user if the assumptions made in
+			continuous-voltage mode do not reign true.
+
+continuous-voltage:	This mode uses the regulator's maximum and minimum supplied
+			voltages specified in the regulator-{min,max}-microvolt
+			properties to calculate appropriate duty-cycle values.  This
+			allows for a much more fine grained solution when compared
+			with voltage-table mode above.  This solution does make an
+			assumption that a %50 duty-cycle value will cause the
+			regulator voltage to run at half way between the supplied
+			max_uV and min_uV values.
 
 Required properties:
-- compatible: Should be "pwm-regulator"
-- pwms: OF device-tree PWM specification (see PWM binding pwm.txt)
-- voltage-table: voltage and duty table, include 2 members in each set of
-  brackets, first one is voltage(unit: uv), the next is duty(unit: percent)
+--------------------
+- compatible:		Should be "pwm-regulator"
+
+- pwms:			PWM specification (See: ../pwm/pwm.txt)
+
+One of these must be provided:
+- voltage-table: 	Voltage and Duty-Cycle table consisting of 2 cells
+			    First cell is voltage in microvolts (uV)
+			    Second cell is duty-cycle in percent (%)
+
+- max-duty-cycle:	Maximum Duty-Cycle value -- this will normally be 255 (0xff)
+			for an 8 bit PMU device
 
-Any property defined as part of the core regulator binding defined in
-regulator.txt can also be used.
+If both are provided, the current default is voltage-table mode.
 
-Example:
+Any property defined as part of the core regulator binding can also be used.
+(See: ../regulator/regulator.txt)
+
+Continuous Voltage Example:
 	pwm_regulator {
 		compatible = "pwm-regulator;
 		pwms = <&pwm1 0 8448 0>;
+		regulator-min-microvolt = <1016000>;
+		regulator-max-microvolt = <1114000>;
+		regulator-name = "vdd_logic";
+
+		max-duty-cycle = <255>; /* 8bit PWM */
+	};
 
+Voltage Table Example:
+	pwm_regulator {
+		compatible = "pwm-regulator;
+		pwms = <&pwm1 0 8448 0>;
+		regulator-min-microvolt = <1016000>;
+		regulator-max-microvolt = <1114000>;
+		regulator-name = "vdd_logic";
+
+			      /* Voltage Duty-Cycle */
 		voltage-table = <1114000 0>,
 				<1095000 10>,
 				<1076000 20>,
 				<1056000 30>,
 				<1036000 40>,
 				<1016000 50>;
-
-		regulator-min-microvolt = <1016000>;
-		regulator-max-microvolt = <1114000>;
-		regulator-name = "vdd_logic";
 	};
-- 
1.9.1

[PATCH 8/8] dt: regulator: pwm-regulator: Re-write bindings

[Lee Jones]

Whoops, nearly forgot the DT folks.

Sorry chaps.

> * Add support for continuous-voltage mode
> * Put more meat on the bones with regards to voltage-table mode
> * Sort out formatting for ease of consumption
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../bindings/regulator/pwm-regulator.txt           | 66 ++++++++++++++++++----
>  1 file changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> index ce91f61..9a8269f 100644
> --- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> @@ -1,27 +1,69 @@
> -pwm regulator bindings
> +Bindings for the Generic PWM Regulator
> +======================================
> +
> +Currently supports 2 modes of operation:
> +
> +voltage-table:		When in this mode, a voltage table (See below) of predefined
> +			voltage <=> duty-cycle values must be provided via DT.
> +			Limitations are that the regulator can only operate at the
> +			voltages supplied in the table.  Intermediary duty-cycle
> +			values which would normally allow finer grained voltage
> +			selection are ignored and rendered useless.  Although more
> +			control is given to the user if the assumptions made in
> +			continuous-voltage mode do not reign true.
> +
> +continuous-voltage:	This mode uses the regulator's maximum and minimum supplied
> +			voltages specified in the regulator-{min,max}-microvolt
> +			properties to calculate appropriate duty-cycle values.  This
> +			allows for a much more fine grained solution when compared
> +			with voltage-table mode above.  This solution does make an
> +			assumption that a %50 duty-cycle value will cause the
> +			regulator voltage to run at half way between the supplied
> +			max_uV and min_uV values.
>  
>  Required properties:
> -- compatible: Should be "pwm-regulator"
> -- pwms: OF device-tree PWM specification (see PWM binding pwm.txt)
> -- voltage-table: voltage and duty table, include 2 members in each set of
> -  brackets, first one is voltage(unit: uv), the next is duty(unit: percent)
> +--------------------
> +- compatible:		Should be "pwm-regulator"
> +
> +- pwms:			PWM specification (See: ../pwm/pwm.txt)
> +
> +One of these must be provided:
> +- voltage-table: 	Voltage and Duty-Cycle table consisting of 2 cells
> +			    First cell is voltage in microvolts (uV)
> +			    Second cell is duty-cycle in percent (%)
> +
> +- max-duty-cycle:	Maximum Duty-Cycle value -- this will normally be 255 (0xff)
> +			for an 8 bit PMU device
>  
> -Any property defined as part of the core regulator binding defined in
> -regulator.txt can also be used.
> +If both are provided, the current default is voltage-table mode.
>  
> -Example:
> +Any property defined as part of the core regulator binding can also be used.
> +(See: ../regulator/regulator.txt)
> +
> +Continuous Voltage Example:
>  	pwm_regulator {
>  		compatible = "pwm-regulator;
>  		pwms = <&pwm1 0 8448 0>;
> +		regulator-min-microvolt = <1016000>;
> +		regulator-max-microvolt = <1114000>;
> +		regulator-name = "vdd_logic";
> +
> +		max-duty-cycle = <255>; /* 8bit PWM */
> +	};
>  
> +Voltage Table Example:
> +	pwm_regulator {
> +		compatible = "pwm-regulator;
> +		pwms = <&pwm1 0 8448 0>;
> +		regulator-min-microvolt = <1016000>;
> +		regulator-max-microvolt = <1114000>;
> +		regulator-name = "vdd_logic";
> +
> +			      /* Voltage Duty-Cycle */
>  		voltage-table = <1114000 0>,
>  				<1095000 10>,
>  				<1076000 20>,
>  				<1056000 30>,
>  				<1036000 40>,
>  				<1016000 50>;
> -
> -		regulator-min-microvolt = <1016000>;
> -		regulator-max-microvolt = <1114000>;
> -		regulator-name = "vdd_logic";
>  	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 5/8] regulator: pwm-regulator: Don't assign structure attributes right away

[Mark Brown]

On Fri, Jun 05, 2015 at 07:42:48PM +0100, Lee Jones wrote:
> Perhaps this is just personal preference, but ...

Possibly...

> Prevents this kind of nonsense:

> 	this->is->just.silly = fetch_silly_value(&pointer);
> 	if (!this->is->just.silly) {
> 		printk("Silly value failed: %d\n", this->is->just.silly);
> 		return this->is->just.silly;
> 	}

But we don't have any of that code?  Well, one if statement where we
check config.init_data but that's it.  A temporary does help with things
like the above but we're not doing that in this driver are we?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150608/50ddba59/attachment.sig>

[PATCH 2/8] regulator: pwm-regulator: Remove unnecessary descriptor attribute from ddata

[Mark Brown]

On Fri, Jun 05, 2015 at 07:42:45PM +0100, Lee Jones wrote:
> The Regulator Device keeps a full copy of it's own, which can be easily accessed.

Applied, thanks.  Please keep your commit logs under 80 columns.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150608/59cef120/attachment.sig>

[PATCH 3/8] regulator: pwm-regulator: Remove superfluous is_enabled check

[Mark Brown]

On Fri, Jun 05, 2015 at 07:42:46PM +0100, Lee Jones wrote:
> The core framework already takes care of this.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150608/f5defc90/attachment.sig>

[PATCH 4/8] regulator: pwm-regulator: Diffientiate between dev (device) and rdev (regulator_dev)

[Mark Brown]

On Fri, Jun 05, 2015 at 07:42:47PM +0100, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150608/2286998d/attachment.sig>

[PATCH 5/8] regulator: pwm-regulator: Don't assign structure attributes right away

[Lee Jones]

On Mon, 08 Jun 2015, Mark Brown wrote:

> On Fri, Jun 05, 2015 at 07:42:48PM +0100, Lee Jones wrote:
> > Perhaps this is just personal preference, but ...
> 
> Possibly...
> 
> > Prevents this kind of nonsense:
> 
> > 	this->is->just.silly = fetch_silly_value(&pointer);
> > 	if (!this->is->just.silly) {
> > 		printk("Silly value failed: %d\n", this->is->just.silly);
> > 		return this->is->just.silly;
> > 	}
> 
> But we don't have any of that code?  Well, one if statement where we
> check config.init_data but that's it.  A temporary does help with things
> like the above but we're not doing that in this driver are we?

Admittedly this is an extreme example, but I do consider:

	init_data = of_get_regulator_init_data(<blah>);
	if (init_data)
        	return -ENOMEM;

... neater than:

	config.init_data = of_get_regulator_init_data(<blah>);
	if (!config.init_data)
        	return -ENOMEM;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 5/8] regulator: pwm-regulator: Don't assign structure attributes right away

[Mark Brown]

On Tue, Jun 09, 2015 at 08:03:47AM +0100, Lee Jones wrote:

> Admittedly this is an extreme example, but I do consider:

> 	init_data = of_get_regulator_init_data(<blah>);
> 	if (init_data)
>         	return -ENOMEM;

> ... neater than:

> 	config.init_data = of_get_regulator_init_data(<blah>);
> 	if (!config.init_data)
>         	return -ENOMEM;

Oh, I see.  I pretty much see things the other way for things where the
temporary has no other users.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150609/f9d5ce95/attachment.sig>

[PATCH 5/8] regulator: pwm-regulator: Don't assign structure attributes right away

[Lee Jones]

On Tue, 09 Jun 2015, Mark Brown wrote:

> On Tue, Jun 09, 2015 at 08:03:47AM +0100, Lee Jones wrote:
> 
> > Admittedly this is an extreme example, but I do consider:
> 
> > 	init_data = of_get_regulator_init_data(<blah>);
> > 	if (init_data)
> >         	return -ENOMEM;
> 
> > ... neater than:
> 
> > 	config.init_data = of_get_regulator_init_data(<blah>);
> > 	if (!config.init_data)
> >         	return -ENOMEM;
> 
> Oh, I see.  I pretty much see things the other way for things where the
> temporary has no other users.

I don't feel passionate enough about it to contest.

Skip this patch then.  Are you okay to continue the review?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 6/8] regulator: pwm-regulator: Separate voltage-table initialisation

[Mark Brown]

On Fri, Jun 05, 2015 at 07:42:49PM +0100, Lee Jones wrote:
> Take this out of the main .probe() routine in order to facilitate the
> introduction of different ways to obtain 'duty cycle' information.

This and the rest of the patches are fine but seem to depend on patch 4
- can you please rebase and resend?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150609/b04d1fad/attachment.sig>

[PATCH 6/8] regulator: pwm-regulator: Separate voltage-table initialisation

[Lee Jones]

On Tue, 09 Jun 2015, Mark Brown wrote:
> On Fri, Jun 05, 2015 at 07:42:49PM +0100, Lee Jones wrote:
> > Take this out of the main .probe() routine in order to facilitate the
> > introduction of different ways to obtain 'duty cycle' information.
> 
> This and the rest of the patches are fine but seem to depend on patch 4
> - can you please rebase and resend?

Done.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog