* [PATCH v2 0/2] regulator: get voltage & duty table from dts for st-pwm
@ 2014-09-18 7:31 Chris Zhong
2014-09-18 7:31 ` [PATCH v2 1/2] regulator: st-pwm: get voltage and duty table from dts Chris Zhong
2014-09-18 7:31 ` [PATCH v2 2/2] dt-bindings: add devicetree bindings for st-pwm regulator Chris Zhong
0 siblings, 2 replies; 6+ messages in thread
From: Chris Zhong @ 2014-09-18 7:31 UTC (permalink / raw)
To: dianders, heiko
Cc: huangtao, cf, zhangqing, lee.jones, broonie, Chris Zhong,
devicetree, Liam Girdwood, Kumar Gala, linux-kernel,
Grant Likely, Ian Campbell, Rob Herring, Pawel Moll,
Mark Rutland
get voltage & duty table from device tree might be better, other platforms can also use this
driver without any modify.
Tested on a rk3288 sdk board as logic voltage regulator.
Changes in v2:
Adviced by Lee Jones
- rename the documentation
Adviced by Doug Anderson
- update the example
Adviced by Mark Rutland
- remove pwm-reg-period
Chris Zhong (2):
regulator: st-pwm: get voltage and duty table from dts
dt-bindings: add devicetree bindings for st-pwm regulator
.../bindings/regulator/pwm-regulator.txt | 29 +++
drivers/regulator/Kconfig | 8 +-
drivers/regulator/Makefile | 2 +-
drivers/regulator/pwm-regulator.c | 195 ++++++++++++++++++++
drivers/regulator/st-pwm.c | 190 -------------------
5 files changed, 229 insertions(+), 195 deletions(-)
create mode 100644 Documentation/devicetree/bindings/regulator/pwm-regulator.txt
create mode 100644 drivers/regulator/pwm-regulator.c
delete mode 100644 drivers/regulator/st-pwm.c
--
1.7.9.5
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] regulator: st-pwm: get voltage and duty table from dts
2014-09-18 7:31 [PATCH v2 0/2] regulator: get voltage & duty table from dts for st-pwm Chris Zhong
@ 2014-09-18 7:31 ` Chris Zhong
2014-09-18 21:25 ` Doug Anderson
2014-09-18 7:31 ` [PATCH v2 2/2] dt-bindings: add devicetree bindings for st-pwm regulator Chris Zhong
1 sibling, 1 reply; 6+ messages in thread
From: Chris Zhong @ 2014-09-18 7:31 UTC (permalink / raw)
To: dianders, heiko
Cc: huangtao, cf, zhangqing, lee.jones, broonie, Chris Zhong,
Liam Girdwood, Grant Likely, Rob Herring, linux-kernel,
devicetree
Get voltage & duty table from device tree might be better, other platforms can also use this
driver without any modify.
Signed-off-by: Chris Zhong <zyw@rock-chips.com>
eries-changes: 2
Adviced by Lee Jones
- rename the file
- remove all the prefix st_
- add depend on PWM in Kconfig
---
Changes in v2: None
drivers/regulator/Kconfig | 8 +-
drivers/regulator/Makefile | 2 +-
drivers/regulator/pwm-regulator.c | 195 +++++++++++++++++++++++++++++++++++++
drivers/regulator/st-pwm.c | 190 ------------------------------------
4 files changed, 200 insertions(+), 195 deletions(-)
create mode 100644 drivers/regulator/pwm-regulator.c
delete mode 100644 drivers/regulator/st-pwm.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index fb32bab..4b17d75 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -493,11 +493,11 @@ config REGULATOR_S5M8767
via I2C bus. S5M8767A have 9 Bucks and 28 LDOs output and
supports DVS mode with 8bits of output voltage control.
-config REGULATOR_ST_PWM
- tristate "STMicroelectronics PWM voltage regulator"
- depends on ARCH_STI
+config REGULATOR_PWM
+ tristate "PWM voltage regulator"
+ depends on PWM
help
- This driver supports ST's PWM controlled voltage regulators.
+ This driver supports PWM controlled voltage regulators.
config REGULATOR_TI_ABB
tristate "TI Adaptive Body Bias on-chip LDO"
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 236fdbb..3954fbd 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -66,7 +66,7 @@ obj-$(CONFIG_REGULATOR_RK808) += rk808-regulator.o
obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
-obj-$(CONFIG_REGULATOR_ST_PWM) += st-pwm.o
+obj-$(CONFIG_REGULATOR_PWM) += pwm-regulator.o
obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
obj-$(CONFIG_REGULATOR_TI_ABB) += ti-abb-regulator.o
obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
new file mode 100644
index 0000000..ea5b3d4
--- /dev/null
+++ b/drivers/regulator/pwm-regulator.c
@@ -0,0 +1,195 @@
+/*
+ * Regulator driver for PWM Regulators
+ *
+ * Copyright (C) 2014 - STMicroelectronics Inc.
+ *
+ * Author: Lee Jones <lee.jones@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+
+struct pwm_regulator_data {
+ struct regulator_desc *desc;
+ struct pwm_voltages *duty_cycle_table;
+ struct pwm_device *pwm;
+ int pwm_reg_period;
+ bool enabled;
+ int state;
+};
+
+struct pwm_voltages {
+ unsigned int uV;
+ unsigned int dutycycle;
+};
+
+static int pwm_regulator_get_voltage_sel(struct regulator_dev *dev)
+{
+ struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+ return drvdata->state;
+}
+
+static int pwm_regulator_set_voltage_sel(struct regulator_dev *dev,
+ unsigned selector)
+{
+ struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+ int dutycycle;
+ int ret;
+
+ dutycycle = (drvdata->pwm_reg_period / 100) *
+ drvdata->duty_cycle_table[selector].dutycycle;
+
+ ret = pwm_config(drvdata->pwm, dutycycle, drvdata->pwm_reg_period);
+ if (ret) {
+ dev_err(&dev->dev, "Failed to configure PWM\n");
+ return ret;
+ }
+
+ 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;
+ }
+
+ return 0;
+}
+
+static int pwm_regulator_list_voltage(struct regulator_dev *dev,
+ unsigned selector)
+{
+ struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+ if (selector >= dev->desc->n_voltages)
+ return -EINVAL;
+
+ return drvdata->duty_cycle_table[selector].uV;
+}
+
+static struct regulator_ops pwm_regulator_voltage_ops = {
+ .set_voltage_sel = pwm_regulator_set_voltage_sel,
+ .get_voltage_sel = pwm_regulator_get_voltage_sel,
+ .list_voltage = pwm_regulator_list_voltage,
+ .map_voltage = regulator_map_voltage_iterate,
+};
+
+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_probe(struct platform_device *pdev)
+{
+ 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;
+
+ if (!np) {
+ dev_err(&pdev->dev, "Device Tree node missing\n");
+ return -EINVAL;
+ }
+
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ drvdata->desc = &pwm_regulator_desc;
+
+ /* determine the number of voltage-table */
+ prop = of_find_property(np, "voltage-table", &length);
+ if (!prop)
+ return -EINVAL;
+
+ drvdata->desc->n_voltages = length / sizeof(*drvdata->duty_cycle_table);
+
+ /* read voltage table from DT property */
+ if (length > 0) {
+ size_t size = sizeof(*drvdata->duty_cycle_table) *
+ drvdata->desc->n_voltages;
+
+ drvdata->duty_cycle_table = devm_kzalloc(&pdev->dev,
+ size, GFP_KERNEL);
+ if (!drvdata->duty_cycle_table)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_array(np, "voltage-table",
+ (u32 *)drvdata->duty_cycle_table,
+ length / sizeof(u32));
+ if (ret < 0)
+ return ret;
+ }
+
+ config.init_data = of_get_regulator_init_data(&pdev->dev, np);
+ if (!config.init_data)
+ return -ENOMEM;
+
+ config.of_node = np;
+ config.dev = &pdev->dev;
+ config.driver_data = drvdata;
+
+ drvdata->pwm = devm_pwm_get(&pdev->dev, NULL);
+ if (IS_ERR(drvdata->pwm)) {
+ dev_err(&pdev->dev, "Failed to get PWM\n");
+ return PTR_ERR(drvdata->pwm);
+ }
+
+ drvdata->pwm_reg_period = pwm_get_period(drvdata->pwm);
+ if (!drvdata->pwm_reg_period) {
+ dev_err(&pdev->dev, "get pwm peeriod failed\n");
+ return -EINVAL;
+ }
+
+ regulator = devm_regulator_register(&pdev->dev,
+ drvdata->desc, &config);
+ if (IS_ERR(regulator)) {
+ dev_err(&pdev->dev, "Failed to register regulator %s\n",
+ drvdata->desc->name);
+ return PTR_ERR(regulator);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id pwm_of_match[] = {
+ { .compatible = "pwm-regulator" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, pwm_of_match);
+
+static struct platform_driver pwm_regulator_driver = {
+ .driver = {
+ .name = "pwm-regulator",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(pwm_of_match),
+ },
+ .probe = pwm_regulator_probe,
+};
+
+module_platform_driver(pwm_regulator_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Lee Jones <lee.jones@linaro.org>");
+MODULE_DESCRIPTION("PWM Regulator Driver");
+MODULE_ALIAS("platform:pwm-regulator");
diff --git a/drivers/regulator/st-pwm.c b/drivers/regulator/st-pwm.c
deleted file mode 100644
index 5ea78df..0000000
--- a/drivers/regulator/st-pwm.c
+++ /dev/null
@@ -1,190 +0,0 @@
-/*
- * Regulator driver for ST's PWM Regulators
- *
- * Copyright (C) 2014 - STMicroelectronics Inc.
- *
- * Author: Lee Jones <lee.jones@linaro.org>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/err.h>
-#include <linux/regulator/driver.h>
-#include <linux/regulator/machine.h>
-#include <linux/regulator/of_regulator.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
-#include <linux/pwm.h>
-
-#define ST_PWM_REG_PERIOD 8448
-
-struct st_pwm_regulator_pdata {
- const struct regulator_desc *desc;
- struct st_pwm_voltages *duty_cycle_table;
-};
-
-struct st_pwm_regulator_data {
- const struct st_pwm_regulator_pdata *pdata;
- struct pwm_device *pwm;
- bool enabled;
- int state;
-};
-
-struct st_pwm_voltages {
- unsigned int uV;
- unsigned int dutycycle;
-};
-
-static int st_pwm_regulator_get_voltage_sel(struct regulator_dev *dev)
-{
- struct st_pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
-
- return drvdata->state;
-}
-
-static int st_pwm_regulator_set_voltage_sel(struct regulator_dev *dev,
- unsigned selector)
-{
- struct st_pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
- int dutycycle;
- int ret;
-
- dutycycle = (ST_PWM_REG_PERIOD / 100) *
- drvdata->pdata->duty_cycle_table[selector].dutycycle;
-
- ret = pwm_config(drvdata->pwm, dutycycle, ST_PWM_REG_PERIOD);
- if (ret) {
- dev_err(&dev->dev, "Failed to configure PWM\n");
- return ret;
- }
-
- 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;
- }
-
- return 0;
-}
-
-static int st_pwm_regulator_list_voltage(struct regulator_dev *dev,
- unsigned selector)
-{
- struct st_pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
-
- if (selector >= dev->desc->n_voltages)
- return -EINVAL;
-
- return drvdata->pdata->duty_cycle_table[selector].uV;
-}
-
-static struct regulator_ops st_pwm_regulator_voltage_ops = {
- .set_voltage_sel = st_pwm_regulator_set_voltage_sel,
- .get_voltage_sel = st_pwm_regulator_get_voltage_sel,
- .list_voltage = st_pwm_regulator_list_voltage,
- .map_voltage = regulator_map_voltage_iterate,
-};
-
-static struct st_pwm_voltages b2105_duty_cycle_table[] = {
- { .uV = 1114000, .dutycycle = 0, },
- { .uV = 1095000, .dutycycle = 10, },
- { .uV = 1076000, .dutycycle = 20, },
- { .uV = 1056000, .dutycycle = 30, },
- { .uV = 1036000, .dutycycle = 40, },
- { .uV = 1016000, .dutycycle = 50, },
- /* WARNING: Values above 50% duty-cycle cause boot failures. */
-};
-
-static const struct regulator_desc b2105_desc = {
- .name = "b2105-pwm-regulator",
- .ops = &st_pwm_regulator_voltage_ops,
- .type = REGULATOR_VOLTAGE,
- .owner = THIS_MODULE,
- .n_voltages = ARRAY_SIZE(b2105_duty_cycle_table),
- .supply_name = "pwm",
-};
-
-static const struct st_pwm_regulator_pdata b2105_info = {
- .desc = &b2105_desc,
- .duty_cycle_table = b2105_duty_cycle_table,
-};
-
-static const struct of_device_id st_pwm_of_match[] = {
- { .compatible = "st,b2105-pwm-regulator", .data = &b2105_info, },
- { },
-};
-MODULE_DEVICE_TABLE(of, st_pwm_of_match);
-
-static int st_pwm_regulator_probe(struct platform_device *pdev)
-{
- struct st_pwm_regulator_data *drvdata;
- struct regulator_dev *regulator;
- struct regulator_config config = { };
- struct device_node *np = pdev->dev.of_node;
- const struct of_device_id *of_match;
-
- if (!np) {
- dev_err(&pdev->dev, "Device Tree node missing\n");
- return -EINVAL;
- }
-
- drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
- if (!drvdata)
- return -ENOMEM;
-
- of_match = of_match_device(st_pwm_of_match, &pdev->dev);
- if (!of_match) {
- dev_err(&pdev->dev, "failed to match of device\n");
- return -ENODEV;
- }
- drvdata->pdata = of_match->data;
-
- config.init_data = of_get_regulator_init_data(&pdev->dev, np);
- if (!config.init_data)
- return -ENOMEM;
-
- config.of_node = np;
- config.dev = &pdev->dev;
- config.driver_data = drvdata;
-
- drvdata->pwm = devm_pwm_get(&pdev->dev, NULL);
- if (IS_ERR(drvdata->pwm)) {
- dev_err(&pdev->dev, "Failed to get PWM\n");
- return PTR_ERR(drvdata->pwm);
- }
-
- regulator = devm_regulator_register(&pdev->dev,
- drvdata->pdata->desc, &config);
- if (IS_ERR(regulator)) {
- dev_err(&pdev->dev, "Failed to register regulator %s\n",
- drvdata->pdata->desc->name);
- return PTR_ERR(regulator);
- }
-
- return 0;
-}
-
-static struct platform_driver st_pwm_regulator_driver = {
- .driver = {
- .name = "st-pwm-regulator",
- .owner = THIS_MODULE,
- .of_match_table = of_match_ptr(st_pwm_of_match),
- },
- .probe = st_pwm_regulator_probe,
-};
-
-module_platform_driver(st_pwm_regulator_driver);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Lee Jones <lee.jones@linaro.org>");
-MODULE_DESCRIPTION("ST PWM Regulator Driver");
-MODULE_ALIAS("platform:st_pwm-regulator");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] dt-bindings: add devicetree bindings for st-pwm regulator
2014-09-18 7:31 [PATCH v2 0/2] regulator: get voltage & duty table from dts for st-pwm Chris Zhong
2014-09-18 7:31 ` [PATCH v2 1/2] regulator: st-pwm: get voltage and duty table from dts Chris Zhong
@ 2014-09-18 7:31 ` Chris Zhong
2014-09-18 21:27 ` Doug Anderson
1 sibling, 1 reply; 6+ messages in thread
From: Chris Zhong @ 2014-09-18 7:31 UTC (permalink / raw)
To: dianders, heiko
Cc: huangtao, cf, zhangqing, lee.jones, broonie, Chris Zhong,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree, linux-kernel
Document the st-pwm regulator
Signed-off-by: Chris Zhong <zyw@rock-chips.com>
---
Changes in v2:
Adviced by Lee Jones
- rename the documentation
Adviced by Doug Anderson
- update the example
Adviced by Mark Rutland
- remove pwm-reg-period
.../bindings/regulator/pwm-regulator.txt | 29 ++++++++++++++++++++
1 file changed, 29 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/pwm-regulator.txt
diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
new file mode 100644
index 0000000..7c62a30
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
@@ -0,0 +1,29 @@
+pwm regulator bindings
+
+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 merbers in each set of
+ brackets, first one is voltage(unit: uv), the next is duty(unit: percent)
+
+Any property defined as part of the core regulator binding defined in
+regulator.txt can also be used.
+
+Example:
+ pwm_regulator {
+ compatible = "pwm-regulator;
+ pwms = <&pwm1 0 1000000 0>;
+
+ voltage-table = <1114000 0>,
+ <1095000 10>,
+ <1076000 20>,
+ <1056000 30>,
+ <1036000 40>,
+ <1016000 50>;
+
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1016000>;
+ regulator-max-microvolt = <1114000>;
+ regulator-name = "vdd_logic";
+ };
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] regulator: st-pwm: get voltage and duty table from dts
2014-09-18 7:31 ` [PATCH v2 1/2] regulator: st-pwm: get voltage and duty table from dts Chris Zhong
@ 2014-09-18 21:25 ` Doug Anderson
0 siblings, 0 replies; 6+ messages in thread
From: Doug Anderson @ 2014-09-18 21:25 UTC (permalink / raw)
To: Chris Zhong
Cc: Heiko Stübner, Tao Huang, Eddie Cai, zhangqing, Lee Jones,
broonie, Liam Girdwood, Grant Likely, Rob Herring, linux-kernel,
devicetree
Chris,
On Thu, Sep 18, 2014 at 12:31 AM, Chris Zhong <zyw@rock-chips.com> wrote:
> Get voltage & duty table from device tree might be better, other platforms can also use this
> driver without any modify.
>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>
> eries-changes: 2
> Adviced by Lee Jones
> - rename the file
> - remove all the prefix st_
> - add depend on PWM in Kconfig
> ---
>
> Changes in v2: None
>
> drivers/regulator/Kconfig | 8 +-
> drivers/regulator/Makefile | 2 +-
> drivers/regulator/pwm-regulator.c | 195 +++++++++++++++++++++++++++++++++++++
> drivers/regulator/st-pwm.c | 190 ------------------------------------
> 4 files changed, 200 insertions(+), 195 deletions(-)
> create mode 100644 drivers/regulator/pwm-regulator.c
> delete mode 100644 drivers/regulator/st-pwm.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index fb32bab..4b17d75 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -493,11 +493,11 @@ config REGULATOR_S5M8767
> via I2C bus. S5M8767A have 9 Bucks and 28 LDOs output and
> supports DVS mode with 8bits of output voltage control.
>
> -config REGULATOR_ST_PWM
> - tristate "STMicroelectronics PWM voltage regulator"
> - depends on ARCH_STI
> +config REGULATOR_PWM
> + tristate "PWM voltage regulator"
> + depends on PWM
> help
> - This driver supports ST's PWM controlled voltage regulators.
> + This driver supports PWM controlled voltage regulators.
nit: with the rename you probably need to move this up so it's alphabetical.
> config REGULATOR_TI_ABB
> tristate "TI Adaptive Body Bias on-chip LDO"
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 236fdbb..3954fbd 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -66,7 +66,7 @@ obj-$(CONFIG_REGULATOR_RK808) += rk808-regulator.o
> obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
> obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
> obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
> -obj-$(CONFIG_REGULATOR_ST_PWM) += st-pwm.o
> +obj-$(CONFIG_REGULATOR_PWM) += pwm-regulator.o
Same here. Make it alphabetical.
> obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
> obj-$(CONFIG_REGULATOR_TI_ABB) += ti-abb-regulator.o
> obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> new file mode 100644
> index 0000000..ea5b3d4
> --- /dev/null
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -0,0 +1,195 @@
> +/*
> + * Regulator driver for PWM Regulators
> + *
> + * Copyright (C) 2014 - STMicroelectronics Inc.
> + *
> + * Author: Lee Jones <lee.jones@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +
> +struct pwm_regulator_data {
> + struct regulator_desc *desc;
> + struct pwm_voltages *duty_cycle_table;
> + struct pwm_device *pwm;
> + int pwm_reg_period;
pwm_reg_period is "unsigned int".
Technically you could probably leave this out of the structure and
just call pwm_get_period() whenever you need the period.
> + bool enabled;
> + int state;
> +};
> +
> +struct pwm_voltages {
> + unsigned int uV;
> + unsigned int dutycycle;
> +};
> +
> +static int pwm_regulator_get_voltage_sel(struct regulator_dev *dev)
> +{
> + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> +
> + return drvdata->state;
> +}
> +
> +static int pwm_regulator_set_voltage_sel(struct regulator_dev *dev,
> + unsigned selector)
> +{
> + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> + int dutycycle;
> + int ret;
> +
> + dutycycle = (drvdata->pwm_reg_period / 100) *
> + drvdata->duty_cycle_table[selector].dutycycle;
Nit: this is not a new bug for you, but I'd suggest making the above:
(drvdata->pwm_reg_period * drvdata->duty_cycle_table[selector].dutycycle) / 100
Specifically:
(8448 / 100) * 50 = 4200
(8448 * 50) / 100 = 4224
Since the duty cycle was specified as "8448" and not "8400" I'm going
to assume that the second calculation is more accurate.
If you're worried about overflow you could add a cast.
> +
> + ret = pwm_config(drvdata->pwm, dutycycle, drvdata->pwm_reg_period);
> + if (ret) {
> + dev_err(&dev->dev, "Failed to configure PWM\n");
> + return ret;
> + }
> +
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +static int pwm_regulator_list_voltage(struct regulator_dev *dev,
> + unsigned selector)
> +{
> + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> +
> + if (selector >= dev->desc->n_voltages)
> + return -EINVAL;
> +
> + return drvdata->duty_cycle_table[selector].uV;
> +}
> +
> +static struct regulator_ops pwm_regulator_voltage_ops = {
> + .set_voltage_sel = pwm_regulator_set_voltage_sel,
> + .get_voltage_sel = pwm_regulator_get_voltage_sel,
> + .list_voltage = pwm_regulator_list_voltage,
> + .map_voltage = regulator_map_voltage_iterate,
> +};
> +
> +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_probe(struct platform_device *pdev)
> +{
> + 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;
> +
> + if (!np) {
> + dev_err(&pdev->dev, "Device Tree node missing\n");
> + return -EINVAL;
> + }
> +
> + drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->desc = &pwm_regulator_desc;
I think you need to make a copy of pwm_regulator_desc, don't you? You
set "n_voltages" below. If you don't make a copy I think that will
cause problems if you have two PWM regulators in your system with a
different number of voltages, right?
> +
> + /* determine the number of voltage-table */
> + prop = of_find_property(np, "voltage-table", &length);
> + if (!prop)
Check for length < sizeof(*drvdata->duty_cycle_table) too? And maybe
put an error message here?
...could also double check that length %
sizeof(*drvdata->duty_cycle_table) is 0?
> + return -EINVAL;
> +
> + drvdata->desc->n_voltages = length / sizeof(*drvdata->duty_cycle_table);
> +
> + /* read voltage table from DT property */
> + if (length > 0) {
Remove check for length here (since I suggested putting it above)
> + size_t size = sizeof(*drvdata->duty_cycle_table) *
> + drvdata->desc->n_voltages;
Isn't "size" exactly equal to "length" (well, assuming we check that
length % sizeof(*drvdata->duty_cycle_table) is 0 above)? No need to
calculate it again.
> +
> + drvdata->duty_cycle_table = devm_kzalloc(&pdev->dev,
> + size, GFP_KERNEL);
> + if (!drvdata->duty_cycle_table)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32_array(np, "voltage-table",
> + (u32 *)drvdata->duty_cycle_table,
> + length / sizeof(u32));
> + if (ret < 0)
> + return ret;
Error message?
> + }
> +
> + config.init_data = of_get_regulator_init_data(&pdev->dev, np);
> + if (!config.init_data)
> + return -ENOMEM;
> +
> + config.of_node = np;
> + config.dev = &pdev->dev;
> + config.driver_data = drvdata;
> +
> + drvdata->pwm = devm_pwm_get(&pdev->dev, NULL);
> + if (IS_ERR(drvdata->pwm)) {
> + dev_err(&pdev->dev, "Failed to get PWM\n");
> + return PTR_ERR(drvdata->pwm);
> + }
> +
> + drvdata->pwm_reg_period = pwm_get_period(drvdata->pwm);
> + if (!drvdata->pwm_reg_period) {
> + dev_err(&pdev->dev, "get pwm peeriod failed\n");
nit: s/peeriod/period
...actually, I'd personally just skip error checking here. This is a
simple accessor function. If the period is 0 it means someone
specified 0 in the device tree. That will be caught in pwm_config()
and the error there is just as obvious (I think).
> + return -EINVAL;
> + }
> +
> + regulator = devm_regulator_register(&pdev->dev,
> + drvdata->desc, &config);
> + if (IS_ERR(regulator)) {
> + dev_err(&pdev->dev, "Failed to register regulator %s\n",
> + drvdata->desc->name);
> + return PTR_ERR(regulator);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pwm_of_match[] = {
> + { .compatible = "pwm-regulator" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, pwm_of_match);
> +
> +static struct platform_driver pwm_regulator_driver = {
> + .driver = {
> + .name = "pwm-regulator",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(pwm_of_match),
> + },
> + .probe = pwm_regulator_probe,
> +};
> +
> +module_platform_driver(pwm_regulator_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Lee Jones <lee.jones@linaro.org>");
> +MODULE_DESCRIPTION("PWM Regulator Driver");
> +MODULE_ALIAS("platform:pwm-regulator");
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: add devicetree bindings for st-pwm regulator
2014-09-18 7:31 ` [PATCH v2 2/2] dt-bindings: add devicetree bindings for st-pwm regulator Chris Zhong
@ 2014-09-18 21:27 ` Doug Anderson
2014-09-18 22:22 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2014-09-18 21:27 UTC (permalink / raw)
To: Chris Zhong
Cc: Heiko Stübner, Tao Huang, Eddie Cai, zhangqing, Lee Jones,
broonie, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, devicetree, linux-kernel
Chris,
On Thu, Sep 18, 2014 at 12:31 AM, Chris Zhong <zyw@rock-chips.com> wrote:
> Document the st-pwm regulator
>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>
> ---
>
> Changes in v2:
> Adviced by Lee Jones
> - rename the documentation
> Adviced by Doug Anderson
> - update the example
> Adviced by Mark Rutland
> - remove pwm-reg-period
>
> .../bindings/regulator/pwm-regulator.txt | 29 ++++++++++++++++++++
> 1 file changed, 29 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/pwm-regulator.txt
>
> diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> new file mode 100644
> index 0000000..7c62a30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
> @@ -0,0 +1,29 @@
> +pwm regulator bindings
> +
> +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 merbers in each set of
> + brackets, first one is voltage(unit: uv), the next is duty(unit: percent)
> +
I would be tempted to say that you should add a few required properties:
* regulator-boot-on
* regulator-always-on
* regulator-initial-microvolts
>From my understanding of how PWM-based regulators work there is no way
to "disable" them. If you drive them low then they get their biggest
voltage. If you drive them high then they get their smallest voltage.
If you make them floating then it's equivalent to 50% duty cycle
(right?)
Since your current bindings don't have an enable GPIO (which could
possibly be used to turn one of these off if it was hooked up) then
that means that the kernel had better keep the regulator always on.
The initial-microvolts also seems like a wise property to add.
There's no way (that I know of) to query what the voltage was at probe
time. The bootloader (or power on default) of the device might have
left it as an input or might have left it driving high, driving low,
or even PWMing. It seems like we should specify what voltage this
device should start up in. You should make sure to use pinmux in
whatever way is needed (maybe can't use "default") so that you can
make sure that the PWM is configured and driving the initial voltage
before you actually set the pinmux.
All of the above could be done in followup patches since I think they
are new features.
> +Any property defined as part of the core regulator binding defined in
> +regulator.txt can also be used.
> +
> +Example:
> + pwm_regulator {
> + compatible = "pwm-regulator;
> + pwms = <&pwm1 0 1000000 0>;
Since you're using the voltage table from the old st-pwm driver, maybe
you should specify a period that matches? I think that means putting
8448 as your period here.
Wow, so if the ST's PWM regulator needs a period of 8448ns then it's
running at 118kHz? The example you're showing runs at 1kHz, which is
pretty drastically different...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: add devicetree bindings for st-pwm regulator
2014-09-18 21:27 ` Doug Anderson
@ 2014-09-18 22:22 ` Mark Brown
0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2014-09-18 22:22 UTC (permalink / raw)
To: Doug Anderson
Cc: Chris Zhong, Heiko Stübner, Tao Huang, Eddie Cai, zhangqing,
Lee Jones, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 793 bytes --]
On Thu, Sep 18, 2014 at 02:27:59PM -0700, Doug Anderson wrote:
> On Thu, Sep 18, 2014 at 12:31 AM, Chris Zhong <zyw@rock-chips.com> wrote:
> I would be tempted to say that you should add a few required properties:
> * regulator-boot-on
> * regulator-always-on
> * regulator-initial-microvolts
> From my understanding of how PWM-based regulators work there is no way
> to "disable" them. If you drive them low then they get their biggest
> voltage. If you drive them high then they get their smallest voltage.
> If you make them floating then it's equivalent to 50% duty cycle
> (right?)
No, if there is no disable operation then there is no point in
specifying any properties to do with enabling and disabling as they
won't do anything. This is only relevant if the operation is there.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-18 22:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-18 7:31 [PATCH v2 0/2] regulator: get voltage & duty table from dts for st-pwm Chris Zhong
2014-09-18 7:31 ` [PATCH v2 1/2] regulator: st-pwm: get voltage and duty table from dts Chris Zhong
2014-09-18 21:25 ` Doug Anderson
2014-09-18 7:31 ` [PATCH v2 2/2] dt-bindings: add devicetree bindings for st-pwm regulator Chris Zhong
2014-09-18 21:27 ` Doug Anderson
2014-09-18 22:22 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).