This patch-set has been rebased on to topic/pwm.
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 (9):
ARM: multi_v7_defconfig: Enable ST's PWM driver
ARM: multi_v7_defconfig: Enable ST's Power Reset driver
ARM: multi_v7_defconfig: Enable support for PWM Regulators
ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family
ARM: STi: STiH407: Add PWM Regulator node
regulator: pwm-regulator: Re-write bindings
regulator: pwm-regulator: Add support for continuous-voltage
regulator: pwm-regulator: Simplify voltage to duty-cycle call
regulator: pwm-regulator: Don't assign structure attributes right away
.../bindings/regulator/pwm-regulator.txt | 68 ++++++++++---
arch/arm/boot/dts/stih407-family.dtsi | 41 ++++++++
arch/arm/boot/dts/stih407.dtsi | 28 ------
arch/arm/configs/multi_v7_defconfig | 3 +
drivers/regulator/pwm-regulator.c | 109 ++++++++++++++++++---
5 files changed, 198 insertions(+), 51 deletions(-)
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 6d83a1b..21c9e12 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -626,6 +626,7 @@ CONFIG_PWM_SAMSUNG=m
CONFIG_PWM_TEGRA=y
CONFIG_PWM_VT8500=y
CONFIG_PHY_HIX5HD2_SATA=y
+CONFIG_PWM_STI=m
CONFIG_OMAP_USB2=y
CONFIG_TI_PIPE3=y
CONFIG_PHY_MIPHY28LP=y
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 21c9e12..4144fa4 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -355,6 +355,7 @@ CONFIG_POWER_RESET_GPIO_RESTART=y
CONFIG_POWER_RESET_KEYSTONE=y
CONFIG_POWER_RESET_SUN6I=y
CONFIG_POWER_RESET_RMOBILE=y
+CONFIG_POWER_RESET_ST=m
CONFIG_SENSORS_LM90=y
CONFIG_SENSORS_LM95245=y
CONFIG_THERMAL=y
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 4144fa4..4629bed 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -405,6 +405,7 @@ CONFIG_REGULATOR_MAX8973=y
CONFIG_REGULATOR_MAX77686=y
CONFIG_REGULATOR_MAX77693=m
CONFIG_REGULATOR_PALMAS=y
+CONFIG_REGULATOR_PWM=m
CONFIG_REGULATOR_S2MPS11=y
CONFIG_REGULATOR_S5M8767=y
CONFIG_REGULATOR_TPS51632=y
--
1.9.1
This also incorporates the STiH410.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/stih407-family.dtsi | 30 ++++++++++++++++++++++++++++++
arch/arm/boot/dts/stih407.dtsi | 28 ----------------------------
2 files changed, 30 insertions(+), 28 deletions(-)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 838b812..64a1a26 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -539,6 +539,7 @@
status = "disabled";
};
+
st_dwc3: dwc3@8f94000 {
compatible = "st,stih407-dwc3";
reg = <0x08f94000 0x1000>, <0x110 0x4>;
@@ -565,5 +566,34 @@
<&phy_port2 PHY_TYPE_USB3>;
};
};
+
+ /* COMMS PWM Module */
+ pwm0: pwm@9810000 {
+ compatible = "st,sti-pwm";
+ status = "okay";
+ #pwm-cells = <2>;
+ reg = <0x9810000 0x68>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pwm0_chan0_default>;
+ clock-names = "pwm";
+ clocks = <&clk_sysin>;
+ st,pwm-num-chan = <1>;
+ };
+
+ /* SBC PWM Module */
+ pwm1: pwm@9510000 {
+ compatible = "st,sti-pwm";
+ status = "okay";
+ #pwm-cells = <2>;
+ reg = <0x9510000 0x68>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pwm1_chan0_default
+ &pinctrl_pwm1_chan1_default
+ &pinctrl_pwm1_chan2_default
+ &pinctrl_pwm1_chan3_default>;
+ clock-names = "pwm";
+ clocks = <&clk_sysin>;
+ st,pwm-num-chan = <4>;
+ };
};
};
diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
index 2c560fc3..3efa3b2 100644
--- a/arch/arm/boot/dts/stih407.dtsi
+++ b/arch/arm/boot/dts/stih407.dtsi
@@ -147,33 +147,5 @@
};
};
};
-
- /* COMMS PWM Module */
- pwm0: pwm@9810000 {
- compatible = "st,sti-pwm";
- status = "disabled";
- #pwm-cells = <2>;
- reg = <0x9810000 0x68>;
- pinctrl-names = "default";
- pinctrl-0 = <&pinctrl_pwm0_chan0_default>;
- clock-names = "pwm";
- clocks = <&clk_sysin>;
- };
-
- /* SBC PWM Module */
- pwm1: pwm@9510000 {
- compatible = "st,sti-pwm";
- status = "disabled";
- #pwm-cells = <2>;
- reg = <0x9510000 0x68>;
- pinctrl-names = "default";
- pinctrl-0 = <&pinctrl_pwm1_chan0_default
- &pinctrl_pwm1_chan1_default
- &pinctrl_pwm1_chan2_default
- &pinctrl_pwm1_chan3_default>;
- clock-names = "pwm";
- clocks = <&clk_sysin>;
- st,pwm-num-chan = <4>;
- };
};
};
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
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 64a1a26..3c1bd5d 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -65,6 +65,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
* 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
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
.../bindings/regulator/pwm-regulator.txt | 68 ++++++++++++++++++----
1 file changed, 56 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
index ce91f61..892b366 100644
--- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
@@ -1,27 +1,71 @@
-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 PWM 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
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 <[email protected]>
---
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 25560fc..dac145d 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)
{
struct pwm_regulator_data *drvdata;
@@ -146,14 +246,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;
config.init_data = of_get_regulator_init_data(&pdev->dev, np,
&pwm_regulator_desc);
--
1.9.1
If we reverse some of the logic and change the formula used,
we can simplify the function greatly.
It is intentional that this function is supplied and then re-worked
within the same patch-set. The submission in the previous patch is
the tried and tested (i.e. in real releases) method written by ST.
This patch contains a simplification provided later. It looks and
performs better, but doesn't have the same time-under-test that the
original method does. The idea is that we keep some history in
order to provide an easy way back i.e. revert.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/regulator/pwm-regulator.c | 27 +++++++--------------------
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index dac145d..d5cb267 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -93,26 +93,13 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
/**
* Continuous voltage call-backs
*/
-static int pwm_voltage_to_duty_cycle(struct regulator_dev *rdev,
- int volt_mV)
+static int pwm_voltage_to_duty_cycle(struct regulator_dev *rdev, int req_uV)
{
- 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;
+ int min_uV = rdev->constraints->min_uV;
+ int max_uV = rdev->constraints->max_uV;
+ int diff = max_uV - min_uV;
+
+ return 100 - ((((req_uV * 100) - (min_uV * 100)) / diff));
}
static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
@@ -131,7 +118,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
int duty_cycle;
int ret;
- duty_cycle = pwm_voltage_to_duty_cycle(rdev, min_uV / 1000);
+ duty_cycle = pwm_voltage_to_duty_cycle(rdev, min_uV);
ret = pwm_config(drvdata->pwm,
(drvdata->pwm->period / 100) * duty_cycle,
--
1.9.1
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 <[email protected]>
---
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 d5cb267..cb48208 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -218,6 +218,7 @@ static int pwm_regulator_init_continuous(struct platform_device *pdev,
static int pwm_regulator_probe(struct platform_device *pdev)
{
+ const struct regulator_init_data *init_data;
struct pwm_regulator_data *drvdata;
struct regulator_dev *regulator;
struct regulator_config config = { };
@@ -240,14 +241,15 @@ static int pwm_regulator_probe(struct platform_device *pdev)
if (ret)
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
The patch
regulator: pwm-regulator: Simplify voltage to duty-cycle call
has been applied to the regulator tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
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
>From cae897dec26a9d81dcb5182b13b08450f38d6bde Mon Sep 17 00:00:00 2001
From: Lee Jones <[email protected]>
Date: Tue, 7 Jul 2015 16:06:52 +0100
Subject: [PATCH] regulator: pwm-regulator: Simplify voltage to duty-cycle call
If we reverse some of the logic and change the formula used,
we can simplify the function greatly.
It is intentional that this function is supplied and then re-worked
within the same patch-set. The submission in the previous patch is
the tried and tested (i.e. in real releases) method written by ST.
This patch contains a simplification provided later. It looks and
performs better, but doesn't have the same time-under-test that the
original method does. The idea is that we keep some history in
order to provide an easy way back i.e. revert.
Signed-off-by: Lee Jones <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/regulator/pwm-regulator.c | 27 +++++++--------------------
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index dac145db305c..d5cb267fa192 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -93,26 +93,13 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
/**
* Continuous voltage call-backs
*/
-static int pwm_voltage_to_duty_cycle(struct regulator_dev *rdev,
- int volt_mV)
+static int pwm_voltage_to_duty_cycle(struct regulator_dev *rdev, int req_uV)
{
- 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;
+ int min_uV = rdev->constraints->min_uV;
+ int max_uV = rdev->constraints->max_uV;
+ int diff = max_uV - min_uV;
+
+ return 100 - ((((req_uV * 100) - (min_uV * 100)) / diff));
}
static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
@@ -131,7 +118,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
int duty_cycle;
int ret;
- duty_cycle = pwm_voltage_to_duty_cycle(rdev, min_uV / 1000);
+ duty_cycle = pwm_voltage_to_duty_cycle(rdev, min_uV);
ret = pwm_config(drvdata->pwm,
(drvdata->pwm->period / 100) * duty_cycle,
--
2.1.4
The patch
regulator: pwm-regulator: Add support for continuous-voltage
has been applied to the regulator tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
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
>From 4773be185a0f7c1c09d8966e100c76f4fa9a3227 Mon Sep 17 00:00:00 2001
From: Lee Jones <[email protected]>
Date: Tue, 7 Jul 2015 16:06:51 +0100
Subject: [PATCH] regulator: pwm-regulator: Add support for continuous-voltage
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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 25560fc519b7..dac145db305c 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)
{
struct pwm_regulator_data *drvdata;
@@ -146,14 +246,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;
config.init_data = of_get_regulator_init_data(&pdev->dev, np,
&pwm_regulator_desc);
--
2.1.4
The patch
regulator: pwm-regulator: Re-write bindings
has been applied to the regulator tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
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
>From 02258b8bcbb98b28064cc829f7062455da398633 Mon Sep 17 00:00:00 2001
From: Lee Jones <[email protected]>
Date: Tue, 7 Jul 2015 16:06:50 +0100
Subject: [PATCH] regulator: pwm-regulator: Re-write bindings
* 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
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
.../bindings/regulator/pwm-regulator.txt | 68 ++++++++++++++++++----
1 file changed, 56 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
index ce91f61feb12..892b36655b3d 100644
--- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
@@ -1,27 +1,71 @@
-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 PWM 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";
};
--
2.1.4
Hi Lee,
I'm interested in this feature (so as Doug is), could you add us in Cc
of your next submission ?
On Tue, 7 Jul 2015 16:06:44 +0100
Lee Jones <[email protected]> wrote:
> This patch-set has been rebased on to topic/pwm.
>
> 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.
Well, I'm not sure this assumption works for all pwm driven regulators.
What if your regulator does not react linearly to the PWM duty-cycle
config ?
How about addressing that by using all the entries of the
voltage<->duty table association and doing the linear interpolation
between the provided points instead of doing it on the min -> max
range ?
Best Regards,
Boris
>
> Lee Jones (9):
> ARM: multi_v7_defconfig: Enable ST's PWM driver
> ARM: multi_v7_defconfig: Enable ST's Power Reset driver
> ARM: multi_v7_defconfig: Enable support for PWM Regulators
> ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family
> ARM: STi: STiH407: Add PWM Regulator node
> regulator: pwm-regulator: Re-write bindings
> regulator: pwm-regulator: Add support for continuous-voltage
> regulator: pwm-regulator: Simplify voltage to duty-cycle call
> regulator: pwm-regulator: Don't assign structure attributes right away
>
> .../bindings/regulator/pwm-regulator.txt | 68 ++++++++++---
> arch/arm/boot/dts/stih407-family.dtsi | 41 ++++++++
> arch/arm/boot/dts/stih407.dtsi | 28 ------
> arch/arm/configs/multi_v7_defconfig | 3 +
> drivers/regulator/pwm-regulator.c | 109 ++++++++++++++++++---
> 5 files changed, 198 insertions(+), 51 deletions(-)
>
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Hi Lee,
On Tue, 7 Jul 2015 16:06:50 +0100
Lee Jones <[email protected]> wrote:
> * 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
>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
> ---
> .../bindings/regulator/pwm-regulator.txt | 68 ++++++++++++++++++----
> 1 file changed, 56 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> index ce91f61..892b366 100644
> --- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> @@ -1,27 +1,71 @@
> -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.
Do we really have to specify a new property to select the mode ?
The existing DT will have to be modified anyway, so maybe we can just
add a new compatible string differentiate those two modes.
Also note that if you're doing linear interpolation between the points
specified in the voltage-table instead of doing it on the min -> max
values, you wouldn't have to modify the binding.
>
> 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 PWM device
Why are you introducing another random unit. What is max-duty-cycle
really encoding (I guess it has to do with the precision you're
expecting, but I'm not sure) ?
The PWM framework is using nanoseconds, the existing pwm-regulator
binding is using percents. Shouldn't we reuse one of them (I
guess you changed that because the percent unit was not precise
enough) ?
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Thu, 09 Jul 2015, Boris Brezillon wrote:
> I'm interested in this feature (so as Doug is), could you add us in Cc
> of your next submission ?
There won't be a subsequent submission, as this has already been
accepted.
> On Tue, 7 Jul 2015 16:06:44 +0100
> Lee Jones <[email protected]> wrote:
>
> > This patch-set has been rebased on to topic/pwm.
> >
> > 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.
>
> Well, I'm not sure this assumption works for all pwm driven regulators.
> What if your regulator does not react linearly to the PWM duty-cycle
> config ?
>
> How about addressing that by using all the entries of the
> voltage<->duty table association and doing the linear interpolation
> between the provided points instead of doing it on the min -> max
> range ?
If you wish to add a 3rd mode, then I'm sure Mark will accept
submissions, but I think what you are suggesting would be pretty
complex and out-of-scope of what this patch-set is trying to achieve.
As a side note, then if the voltage isn't directly proportional to the
duty cycle on a large scale i.e. max => min, then it will not likely
be very accurate between say table entries 1 => 2, or 4 => 5, etc.
What I suggest you do in your case is provide a larger table with all
of the values you find interesting, as it sounds like your PWM
regulator isn't doing what one would normally expect.
> > Lee Jones (9):
> > ARM: multi_v7_defconfig: Enable ST's PWM driver
> > ARM: multi_v7_defconfig: Enable ST's Power Reset driver
> > ARM: multi_v7_defconfig: Enable support for PWM Regulators
> > ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family
> > ARM: STi: STiH407: Add PWM Regulator node
> > regulator: pwm-regulator: Re-write bindings
> > regulator: pwm-regulator: Add support for continuous-voltage
> > regulator: pwm-regulator: Simplify voltage to duty-cycle call
> > regulator: pwm-regulator: Don't assign structure attributes right away
> >
> > .../bindings/regulator/pwm-regulator.txt | 68 ++++++++++---
> > arch/arm/boot/dts/stih407-family.dtsi | 41 ++++++++
> > arch/arm/boot/dts/stih407.dtsi | 28 ------
> > arch/arm/configs/multi_v7_defconfig | 3 +
> > drivers/regulator/pwm-regulator.c | 109 ++++++++++++++++++---
> > 5 files changed, 198 insertions(+), 51 deletions(-)
> >
>
>
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Thu, 9 Jul 2015 12:51:58 +0100
Lee Jones <[email protected]> wrote:
> On Thu, 09 Jul 2015, Boris Brezillon wrote:
> > I'm interested in this feature (so as Doug is), could you add us in Cc
> > of your next submission ?
>
> There won't be a subsequent submission, as this has already been
> accepted.
Oh, I must have overlooked Mark's answer saying that he is accepting it.
>
> > On Tue, 7 Jul 2015 16:06:44 +0100
> > Lee Jones <[email protected]> wrote:
> >
> > > This patch-set has been rebased on to topic/pwm.
> > >
> > > 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.
> >
> > Well, I'm not sure this assumption works for all pwm driven regulators.
> > What if your regulator does not react linearly to the PWM duty-cycle
> > config ?
> >
> > How about addressing that by using all the entries of the
> > voltage<->duty table association and doing the linear interpolation
> > between the provided points instead of doing it on the min -> max
> > range ?
>
> If you wish to add a 3rd mode, then I'm sure Mark will accept
> submissions, but I think what you are suggesting would be pretty
> complex and out-of-scope of what this patch-set is trying to achieve.
Okay, still don't get the need to add a new mode which is almost doing
the same thing when we could have implemented it in a generic way in the
first place. But if your version has already been accepted then I think
I'll have to propose a new mode :-/.
>
> As a side note, then if the voltage isn't directly proportional to the
> duty cycle on a large scale i.e. max => min, then it will not likely
> be very accurate between say table entries 1 => 2, or 4 => 5, etc.
>
> What I suggest you do in your case is provide a larger table with all
> of the values you find interesting, as it sounds like your PWM
> regulator isn't doing what one would normally expect.
Well, I do not exactly agree here. Yes if you want to have a precise
mapping you'll have to add more entries in your voltage table, but
using linear interpolation between two points can be precise enough on
some ranges and prevent one to define a complete voltage table in the
DT.
Doug, could give more details about the regulator used on the veyron
board ?
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
> > > Lee Jones <[email protected]> wrote:
> > >
> > > > This patch-set has been rebased on to topic/pwm.
> > > >
> > > > 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.
> > >
> > > Well, I'm not sure this assumption works for all pwm driven regulators.
> > > What if your regulator does not react linearly to the PWM duty-cycle
> > > config ?
> > >
> > > How about addressing that by using all the entries of the
> > > voltage<->duty table association and doing the linear interpolation
> > > between the provided points instead of doing it on the min -> max
> > > range ?
> >
> > If you wish to add a 3rd mode, then I'm sure Mark will accept
> > submissions, but I think what you are suggesting would be pretty
> > complex and out-of-scope of what this patch-set is trying to achieve.
>
> Okay, still don't get the need to add a new mode which is almost doing
> the same thing when we could have implemented it in a generic way in the
> first place. But if your version has already been accepted then I think
> I'll have to propose a new mode :-/.
This solution is very generic. What you're suggesting is pretty
non-standard I think. This solution specifically doesn't account for
wonky/non-linear PWM regulators -- that's why I made the effort to
write it as an explicit assumption.
FYI, I just sent a patch amending the binding documentation. It
should prevent any further confusion.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi,
On Thu, Jul 9, 2015 at 6:14 AM, Boris Brezillon
<[email protected]> wrote:
>> If you wish to add a 3rd mode, then I'm sure Mark will accept
>> submissions, but I think what you are suggesting would be pretty
>> complex and out-of-scope of what this patch-set is trying to achieve.
>
> Okay, still don't get the need to add a new mode which is almost doing
> the same thing when we could have implemented it in a generic way in the
> first place. But if your version has already been accepted then I think
> I'll have to propose a new mode :-/.
>
>>
>> As a side note, then if the voltage isn't directly proportional to the
>> duty cycle on a large scale i.e. max => min, then it will not likely
>> be very accurate between say table entries 1 => 2, or 4 => 5, etc.
>>
>> What I suggest you do in your case is provide a larger table with all
>> of the values you find interesting, as it sounds like your PWM
>> regulator isn't doing what one would normally expect.
>
> Well, I do not exactly agree here. Yes if you want to have a precise
> mapping you'll have to add more entries in your voltage table, but
> using linear interpolation between two points can be precise enough on
> some ranges and prevent one to define a complete voltage table in the
> DT.
>
> Doug, could give more details about the regulator used on the veyron
> board ?
There's no need for a new mode as far as veyron is concerned. The pwm
regulator on veyron (as far as I understand it) acts the way that Lee
describes. Try doing the math on the values in the table and you
should see that it's as linear as it can be while still using integral
duty cycles.
Originally I only suggested using "linear interpolation" because:
* It meant no bindings change, which is always nice to avoid.
* It meant that old devices got this new mode, which is probably the
right thing anyway (I think). Maybe nobody has published DTS files
with pwm-regulator, so the point is moot.
-Doug
On Thu, 09 Jul 2015, Doug Anderson wrote:
> On Thu, Jul 9, 2015 at 6:14 AM, Boris Brezillon
> <[email protected]> wrote:
> >> If you wish to add a 3rd mode, then I'm sure Mark will accept
> >> submissions, but I think what you are suggesting would be pretty
> >> complex and out-of-scope of what this patch-set is trying to achieve.
> >
> > Okay, still don't get the need to add a new mode which is almost doing
> > the same thing when we could have implemented it in a generic way in the
> > first place. But if your version has already been accepted then I think
> > I'll have to propose a new mode :-/.
> >
> >>
> >> As a side note, then if the voltage isn't directly proportional to the
> >> duty cycle on a large scale i.e. max => min, then it will not likely
> >> be very accurate between say table entries 1 => 2, or 4 => 5, etc.
> >>
> >> What I suggest you do in your case is provide a larger table with all
> >> of the values you find interesting, as it sounds like your PWM
> >> regulator isn't doing what one would normally expect.
> >
> > Well, I do not exactly agree here. Yes if you want to have a precise
> > mapping you'll have to add more entries in your voltage table, but
> > using linear interpolation between two points can be precise enough on
> > some ranges and prevent one to define a complete voltage table in the
> > DT.
> >
> > Doug, could give more details about the regulator used on the veyron
> > board ?
>
> There's no need for a new mode as far as veyron is concerned. The pwm
> regulator on veyron (as far as I understand it) acts the way that Lee
> describes. Try doing the math on the values in the table and you
> should see that it's as linear as it can be while still using integral
> duty cycles.
>
> Originally I only suggested using "linear interpolation" because:
>
> * It meant no bindings change, which is always nice to avoid.
>
> * It meant that old devices got this new mode, which is probably the
> right thing anyway (I think). Maybe nobody has published DTS files
> with pwm-regulator, so the point is moot.
Great news. This means that you provide the voltage the regulator
will provide with 0% duty cycle and 100% duty cycle and you're hot to
trot. No further configuration/bindings required.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog