* [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 [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
* [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 [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
all inboxes | Powered by JetHome®