[PATCH 1/1] regulator: Add new driver for ST's PWM controlled voltage regulators

[PATCH 1/1] regulator: Add new driver for ST's PWM controlled voltage regulators

[Lee Jones]

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

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");
-- 
1.8.3.2

Re: [PATCH 1/1] regulator: Add new driver for ST's PWM controlled voltage regulators

[Josh Cartwright]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/1] regulator: Add new driver for ST's PWM controlled voltage regulators

[Lee Jones]

> > 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?

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

Re: [PATCH 1/1] regulator: Add new driver for ST's PWM controlled voltage regulators

[Josh Cartwright]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] regulator: Add new driver for ST's PWM controlled voltage regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]