[PATCH 2/4] regulator: add ab8500 per-regulator startup delay

[PATCH 2/4] regulator: add ab8500 per-regulator startup delay

[Linus Walleij]

From: Bengt Jonsson <bengt.g.jonsson@stericsson.com>

Since some regulators can take some time to come online, we
define a per-regulator millisecond delay value and assign
to the slow TV-out regulator.

Signed-off-by: Bengt Jonsson <bengt.g.jonsson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/ab8500.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 5a77630..4e11980 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/err.h>
+#include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/mfd/ab8500.h>
 #include <linux/mfd/abx500.h>
@@ -38,6 +39,7 @@
  * @voltage_mask: mask to control regulator voltage
  * @voltages: supported voltage table
  * @voltages_len: number of supported voltages for the regulator
+ * @delay: startup delay in ms
  */
 struct ab8500_regulator_info {
 	struct device		*dev;
@@ -55,6 +57,7 @@ struct ab8500_regulator_info {
 	u8 voltage_mask;
 	int const *voltages;
 	int voltages_len;
+	unsigned int delay;
 };
 
 /* voltage tables for the vauxn/vintcore supplies */
@@ -115,6 +118,8 @@ static int ab8500_regulator_enable(struct regulator_dev *rdev)
 		dev_err(rdev_get_dev(rdev),
 			"couldn't set enable bits for regulator\n");
 
+	msleep(info->delay);
+
 	dev_vdbg(rdev_get_dev(rdev),
 		"%s-enable (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
 		info->desc.name, info->update_bank, info->update_reg,
@@ -140,6 +145,8 @@ static int ab8500_regulator_disable(struct regulator_dev *rdev)
 		dev_err(rdev_get_dev(rdev),
 			"couldn't set disable bits for regulator\n");
 
+	msleep(info->delay);
+
 	dev_vdbg(rdev_get_dev(rdev),
 		"%s-disable (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
 		info->desc.name, info->update_bank, info->update_reg,
@@ -281,6 +288,8 @@ static int ab8500_regulator_set_voltage(struct regulator_dev *rdev,
 		dev_err(rdev_get_dev(rdev),
 		"couldn't set voltage reg for regulator\n");
 
+	msleep(info->delay);
+
 	dev_vdbg(rdev_get_dev(rdev),
 		"%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
 		" 0x%x\n",
@@ -426,6 +435,7 @@ static struct ab8500_regulator_info
 			.owner		= THIS_MODULE,
 			.n_voltages	= 1,
 		},
+		.delay			= 10,
 		.fixed_uV		= 2000000,
 		.update_bank		= 0x03,
 		.update_reg		= 0x80,
-- 
1.7.3.2

Re: [PATCH 2/4] regulator: add ab8500 per-regulator startup delay

[Mark Brown]

On Thu, Mar 10, 2011 at 02:43:41PM +0100, Linus Walleij wrote:
> From: Bengt Jonsson <bengt.g.jonsson@stericsson.com>
> 
> Since some regulators can take some time to come online, we
> define a per-regulator millisecond delay value and assign
> to the slow TV-out regulator.

You should be implementing the enable_time() operation for the regulator
for this.  The core will deal with inserting the delays for you (and
will hopefully at some point be able to do something sensible with this
for the bulk_ operations).

Re: [PATCH 2/4] regulator: add ab8500 per-regulator startup delay

[Linus Walleij]

On Thu, Mar 10, 2011 at 2:49 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Mar 10, 2011 at 02:43:41PM +0100, Linus Walleij wrote:
>> From: Bengt Jonsson <bengt.g.jonsson@stericsson.com>
>>
>> Since some regulators can take some time to come online, we
>> define a per-regulator millisecond delay value and assign
>> to the slow TV-out regulator.
>
> You should be implementing the enable_time() operation for the regulator
> for this.

I looked into it, but the core implicitly only does delays after
enable(), and our delays affect disable() and set_voltage()
as well. disable() may look superfluous but I bet there may be
cases where not having a voltage fully disabled before doing
something will cause immesurable harm.

I was thinking about extending the present mechanism in
core by refactoring the signature of the enable_time()
as such:

/* Operations that we want to enumerate to the enable_time() call */
enum regulator_op {
     REGULATOR_ENABLE,
     REGULATOR_DISABLE,
     REGULATOR_SET_VOLTAGE,
     REGULATOR_SET_MODE,
};

/* Time taken to enable the regulator */
int (*enable_time) (struct regulator_dev *, enum regulator_op op);

Then I'll have to break out the delay code and refactor a bit,
altering the few drivers using it I guess.

Sounds like a plan?

Yours,
Linus Walleij

Re: [PATCH 2/4] regulator: add ab8500 per-regulator startup delay

[Mark Brown]

On Thu, Mar 10, 2011 at 04:43:50PM +0100, Linus Walleij wrote:
> On Thu, Mar 10, 2011 at 2:49 PM, Mark Brown

> > You should be implementing the enable_time() operation for the regulator
> > for this.

> I looked into it, but the core implicitly only does delays after
> enable(), and our delays affect disable() and set_voltage()

You should add the appropriate generic operations, then.  This is hardly
unique to your hardware and having the information exposed in the driver
ops will allow the core to do helpfult hings with bulk operations.

> as well. disable() may look superfluous but I bet there may be
> cases where not having a voltage fully disabled before doing
> something will cause immesurable harm.

Given how heavily dependant collapsing the voltage is on the external
circuitry I'm not sure the driver can say too much useful here.  Usually
you're either going to collapse very quickly due to active discharge and
load or you're heavily dependant on the board.

> I was thinking about extending the present mechanism in
> core by refactoring the signature of the enable_time()
> as such:

Just add separate operations for each query - set_voltage() needs more
information to tell you a number anyway as it'll need to know the target
it's going to be asked about.

Re: [PATCH 2/4] regulator: add ab8500 per-regulator startup delay

[Linus Walleij]

On Thu, Mar 10, 2011 at 4:53 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Mar 10, 2011 at 04:43:50PM +0100, Linus Walleij wrote:
>> as well. disable() may look superfluous but I bet there may be
>> cases where not having a voltage fully disabled before doing
>> something will cause immesurable harm.
>
> Given how heavily dependant collapsing the voltage is on the external
> circuitry I'm not sure the driver can say too much useful here.  Usually
> you're either going to collapse very quickly due to active discharge and
> load or you're heavily dependant on the board.

Hm! That's true. So if needed this should probably be a delay setting in
the rail in the struct regulation_constraints if it's really needed?

(I'll verify if it's need before implementing that.)

Linus Walleij