| Message ID | 1395324654-8235-1-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hey Lee- A few minor things below. On Thu, Mar 20, 2014 at 02:10:54PM +0000, Lee Jones wrote: > On some STMicroelectronics hardware reside regulators consisting > partly of a PWM input connected to the feedback loop. As the PWM > duty-cycle is varied the output voltage adapts. This driver > allows us to vary the output voltage by adapting the PWM input > duty-cycle. > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- /dev/null > +++ b/drivers/regulator/st-pwm.c > +static int st_pwm_regulator_set_voltage(struct regulator_dev *dev, > + int min_uV, int max_uV, > + unsigned *selector) > +{ > + struct st_pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > + int dutycycle, best_val = INT_MAX; > + int sel, ret; > + > + for (sel = 0; sel < dev->desc->n_voltages; sel++) { > + if (drvdata->duty_cycle_table[sel].uV < best_val && > + drvdata->duty_cycle_table[sel].uV >= min_uV && > + drvdata->duty_cycle_table[sel].uV <= max_uV) { > + best_val = drvdata->duty_cycle_table[sel].uV; > + if (selector) > + *selector = sel; > + } > + } If you implement .set_voltage_sel() instead and set map_voltage to regulator_map_voltage_iterate, the core can take care of this. > + > + if (best_val == INT_MAX) > + return -EINVAL; > + > + dutycycle = (ST_PWM_REG_PERIOD / 100) * > + drvdata->duty_cycle_table[sel].dutycycle; Considering (ST_PWM_REG_PERIOD / 100) is constant, could you get away with dropping this calculation by just putting the already-adjusted values into your duty cycle table? > + > + 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 = sel; > + > + 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->duty_cycle_table[selector].uV; > +} > + > +static struct regulator_ops st_pwm_regulator_voltage_ops = { > + .get_voltage = st_pwm_regulator_get_voltage, > + .set_voltage = st_pwm_regulator_set_voltage, > + .list_voltage = st_pwm_regulator_list_voltage, > +}; > + > +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 = 996000, .dutycycle = 50, }, > + /* WARNING: Values above 50% duty-cycle cause boot failures. */ > +}; > + > +static 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 struct st_pwm_regulator_data b2105_info = { > + .desc = &b2105_desc, > + .duty_cycle_table = b2105_duty_cycle_table, > +}; > + > +static struct of_device_id st_pwm_of_match[] = { const. At least the regulator_desc and duty cycle table should be const as well. (see my comments below about b2105_info). > + { .compatible = "st,b2105-pwm-regulator", .data = &b2105_info, }, > + { }, > +}; You may want a MODULE_DEVICE_TABLE(of, ...); here if you want to be able to be autoloaded. > + > +static int st_pwm_regulator_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct st_pwm_regulator_data *drvdata; > + const struct of_device_id *of_match; > + struct regulator_dev *regulator; > + struct regulator_config config = { }; > + > + if (!np) { > + dev_err(&pdev->dev, "Device Tree node missing\n"); > + return -EINVAL; > + } > + > + 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 = (struct st_pwm_regulator_data *) of_match->data; Hrm, I typed "cast not necessary here", but then I realized it is necessary since you using it to cast away constness. Are you safe assuming that there will only be one of these devices in a system? It doesn't seem like much a burden to just allocate a new object and use it instead of a statically allocated one.
> > On some STMicroelectronics hardware reside regulators consisting > > partly of a PWM input connected to the feedback loop. As the PWM > > duty-cycle is varied the output voltage adapts. This driver > > allows us to vary the output voltage by adapting the PWM input > > duty-cycle. > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- /dev/null > > +++ b/drivers/regulator/st-pwm.c > > +static int st_pwm_regulator_set_voltage(struct regulator_dev *dev, > > + int min_uV, int max_uV, > > + unsigned *selector) > > +{ > > + struct st_pwm_regulator_data *drvdata = rdev_get_drvdata(dev); > > + int dutycycle, best_val = INT_MAX; > > + int sel, ret; > > + > > + for (sel = 0; sel < dev->desc->n_voltages; sel++) { > > + if (drvdata->duty_cycle_table[sel].uV < best_val && > > + drvdata->duty_cycle_table[sel].uV >= min_uV && > > + drvdata->duty_cycle_table[sel].uV <= max_uV) { > > + best_val = drvdata->duty_cycle_table[sel].uV; > > + if (selector) > > + *selector = sel; > > + } > > + } > > If you implement .set_voltage_sel() instead and set map_voltage to > regulator_map_voltage_iterate, the core can take care of this. I'll do that, thanks. > > + > > + if (best_val == INT_MAX) > > + return -EINVAL; > > + > > + dutycycle = (ST_PWM_REG_PERIOD / 100) * > > + drvdata->duty_cycle_table[sel].dutycycle; > > Considering (ST_PWM_REG_PERIOD / 100) is constant, could you get away > with dropping this calculation by just putting the already-adjusted > values into your duty cycle table? I thought about this, but I settled on this way for clarity. Also, this is only a constant if no one decides to change the period, so the calculation needs to be done somewhere. Did you have something better in mind? [...] > > +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 = 996000, .dutycycle = 50, }, > > + /* WARNING: Values above 50% duty-cycle cause boot failures. */ > > +}; > > + > > +static 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 struct st_pwm_regulator_data b2105_info = { > > + .desc = &b2105_desc, > > + .duty_cycle_table = b2105_duty_cycle_table, > > +}; > > + > > +static struct of_device_id st_pwm_of_match[] = { > > const. At least the regulator_desc and duty cycle table should be const > as well. (see my comments below about b2105_info). > > > + { .compatible = "st,b2105-pwm-regulator", .data = &b2105_info, }, > > + { }, > > +}; > > You may want a MODULE_DEVICE_TABLE(of, ...); here if you want to be able > to be autoloaded. Right, good catch. > > +static int st_pwm_regulator_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct st_pwm_regulator_data *drvdata; > > + const struct of_device_id *of_match; > > + struct regulator_dev *regulator; > > + struct regulator_config config = { }; > > + > > + if (!np) { > > + dev_err(&pdev->dev, "Device Tree node missing\n"); > > + return -EINVAL; > > + } > > + > > + 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 = (struct st_pwm_regulator_data *) of_match->data; > > Hrm, I typed "cast not necessary here", but then I realized it is > necessary since you using it to cast away constness. This is remnant from when I was doing something unessersariy complcated in a previous (unpublished/personal) revision. I'll take a look at this too, thanks. > Are you safe assuming that there will only be one of these devices in a > system? It doesn't seem like much a burden to just allocate a new > object and use it instead of a statically allocated one. I have written this driver to be expandable. We have new systems coming out which contain more than one of these regulators. Unless I'm missing your meaning?
On Thu, Mar 20, 2014 at 03:48:27PM +0000, Lee Jones wrote: [..] > > > + dutycycle = (ST_PWM_REG_PERIOD / 100) * > > > + drvdata->duty_cycle_table[sel].dutycycle; > > > > Considering (ST_PWM_REG_PERIOD / 100) is constant, could you get away > > with dropping this calculation by just putting the already-adjusted > > values into your duty cycle table? > > I thought about this, but I settled on this way for clarity. Also, > this is only a constant if no one decides to change the period, so the > calculation needs to be done somewhere. Did you have something better > in mind? I was merely suggesting something like: #define T(uV, _duty) { uV, (ST_PWM_REG_PERIOD / 100) * _duty } static const struct st_pwm_voltages b2105_duty_cycle_table[] = { T(1114000, 0), T(1095000, 10), T(1076000, 20), T(1056000, 30), T(1036000, 40), T( 996000, 50), }; But, meh, it's marginally more complicated and doesn't save much, so I'll leave it up to you to decide whether or not you'd want to change it. > > > +static int st_pwm_regulator_probe(struct platform_device *pdev) > > > +{ > > > + struct device_node *np = pdev->dev.of_node; > > > + struct st_pwm_regulator_data *drvdata; > > > + const struct of_device_id *of_match; > > > + struct regulator_dev *regulator; > > > + struct regulator_config config = { }; > > > + > > > + if (!np) { > > > + dev_err(&pdev->dev, "Device Tree node missing\n"); > > > + return -EINVAL; > > > + } > > > + > > > + 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 = (struct st_pwm_regulator_data *) of_match->data; > > > > Hrm, I typed "cast not necessary here", but then I realized it is > > necessary since you using it to cast away constness. > > This is remnant from when I was doing something unessersariy > complcated in a previous (unpublished/personal) revision. I'll take a > look at this too, thanks. > > > Are you safe assuming that there will only be one of these devices in a > > system? It doesn't seem like much a burden to just allocate a new > > object and use it instead of a statically allocated one. > > I have written this driver to be expandable. We have new systems > coming out which contain more than one of these regulators. Unless I'm > missing your meaning? Sorry I was unclear. I'm really asking about feasibility of multiple instances of the same device (in this case the b2105). The way the driver is currently written, this wouldn't work because the instance shared in the static b2105_info object. As long as you've at least thought about this case, then I'm satisfied. Thanks, Josh
On Thu, Mar 20, 2014 at 02:10:54PM +0000, Lee Jones wrote:
> + dev_info(&pdev->dev, "ST PWM Regulator registered\n");
Remove this, it's not adding anything really (the core is pretty chatty
already). Otherwise this all seems fine modulo the issues Josh
mentioned.
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 6a79328..a6b29cb 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -432,6 +432,12 @@ 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 + help + This driver supports ST's PWM controlled voltage regulators. + config REGULATOR_TI_ABB tristate "TI Adaptive Body Bias on-chip LDO" depends on ARCH_OMAP diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 979f9dd..d0c2bb8 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -59,6 +59,7 @@ obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o obj-$(CONFIG_REGULATOR_RC5T583) += rc5t583-regulator.o obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o +obj-$(CONFIG_REGULATOR_ST_PWM) += st-pwm.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/st-pwm.c b/drivers/regulator/st-pwm.c new file mode 100644 index 0000000..4630465 --- /dev/null +++ b/drivers/regulator/st-pwm.c @@ -0,0 +1,195 @@ +/* + * 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_data { + struct pwm_device *pwm; + struct regulator_desc *desc; + struct st_pwm_voltages *duty_cycle_table; + bool enabled; + int state; +}; + +struct st_pwm_voltages { + unsigned int uV; + unsigned int dutycycle; +}; + +static int st_pwm_regulator_get_voltage(struct regulator_dev *dev) +{ + struct st_pwm_regulator_data *drvdata = rdev_get_drvdata(dev); + + return drvdata->duty_cycle_table[drvdata->state].uV; +} + +static int st_pwm_regulator_set_voltage(struct regulator_dev *dev, + int min_uV, int max_uV, + unsigned *selector) +{ + struct st_pwm_regulator_data *drvdata = rdev_get_drvdata(dev); + int dutycycle, best_val = INT_MAX; + int sel, ret; + + for (sel = 0; sel < dev->desc->n_voltages; sel++) { + if (drvdata->duty_cycle_table[sel].uV < best_val && + drvdata->duty_cycle_table[sel].uV >= min_uV && + drvdata->duty_cycle_table[sel].uV <= max_uV) { + best_val = drvdata->duty_cycle_table[sel].uV; + if (selector) + *selector = sel; + } + } + + if (best_val == INT_MAX) + return -EINVAL; + + dutycycle = (ST_PWM_REG_PERIOD / 100) * + drvdata->duty_cycle_table[sel].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 = sel; + + 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->duty_cycle_table[selector].uV; +} + +static struct regulator_ops st_pwm_regulator_voltage_ops = { + .get_voltage = st_pwm_regulator_get_voltage, + .set_voltage = st_pwm_regulator_set_voltage, + .list_voltage = st_pwm_regulator_list_voltage, +}; + +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 = 996000, .dutycycle = 50, }, + /* WARNING: Values above 50% duty-cycle cause boot failures. */ +}; + +static 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 struct st_pwm_regulator_data b2105_info = { + .desc = &b2105_desc, + .duty_cycle_table = b2105_duty_cycle_table, +}; + +static struct of_device_id st_pwm_of_match[] = { + { .compatible = "st,b2105-pwm-regulator", .data = &b2105_info, }, + { }, +}; + +static int st_pwm_regulator_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct st_pwm_regulator_data *drvdata; + const struct of_device_id *of_match; + struct regulator_dev *regulator; + struct regulator_config config = { }; + + if (!np) { + dev_err(&pdev->dev, "Device Tree node missing\n"); + return -EINVAL; + } + + 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 = (struct st_pwm_regulator_data *) 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->desc, &config); + if (IS_ERR(regulator)) { + dev_err(&pdev->dev, "Failed to register regulator %s\n", + drvdata->desc->name); + return PTR_ERR(regulator); + } + + dev_info(&pdev->dev, "ST PWM Regulator registered\n"); + + 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");
On some STMicroelectronics hardware reside regulators consisting partly of a PWM input connected to the feedback loop. As the PWM duty-cycle is varied the output voltage adapts. This driver allows us to vary the output voltage by adapting the PWM input duty-cycle. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/regulator/Kconfig | 6 ++ drivers/regulator/Makefile | 1 + drivers/regulator/st-pwm.c | 195 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+) create mode 100644 drivers/regulator/st-pwm.c