RE: Problems while designing TPS65023 regulator driver

2009-04-24 Thread Aggarwal, Anuj 
As you know, some regulator patches were required in linux-OMAP 
tree to submit the TPS65023 patch, in absence of which it won't 
have compiled. Since they are available now, I can submit them 
after doing a refresh.

Moreover, we are working on some restructuring for the different 
TPS devices so that the board-dependent code can be separated 
from the rest of the stuff. It should be closed soon and then 
the new patches would be submitted for review, for both the TPS
devices.

Thanks and Regards,
Anuj Aggarwal

Thanks and Regards,
Anuj Aggarwal
 
Platform Support Products
Texas Instruments Incorporated
PSP Products RSS Feed

> -Original Message-
> From: David Brownell [mailto:davi...@pacbell.net]
> Sent: Friday, April 24, 2009 12:03 PM
> To: Trilok Soni
> Cc: Aggarwal, Anuj; Mark Brown; linux-omap@vger.kernel.org; Tony Lindgren
> Subject: Re: Problems while designing TPS65023 regulator driver
> 
> On Thursday 23 April 2009, Trilok Soni wrote:
> > Thanks but I was requesting tps 6 5 0 2 3 not tps 6 2 3 5 x  :).
> 
> Sorry ... maybe they'll help some other time.  :)
> 
> I was wondering what happened to the tps6235x drivers,
> which seemed to have gotten lost.  I don't recall having
> seen tps65023 code.
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-04-23 Thread David Brownell 
On Thursday 23 April 2009, Trilok Soni wrote:
> Thanks but I was requesting tps 6 5 0 2 3 not tps 6 2 3 5 x  :).

Sorry ... maybe they'll help some other time.  :)

I was wondering what happened to the tps6235x drivers,
which seemed to have gotten lost.  I don't recall having
seen tps65023 code.



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Problems while designing TPS65023 regulator driver

2009-04-23 Thread Aggarwal, Anuj 
Trilok,

Since the new regulator_register API was not there in l-o, I have not 
submitted my patch for TPS65023. But since now it is available, I will 
submit my patch soon.

Thanks and Regards,
Anuj Aggarwal
 
Platform Support Products
Texas Instruments Incorporated
PSP Products RSS Feed
> -Original Message-
> From: Trilok Soni [mailto:soni.tri...@gmail.com]
> Sent: Friday, April 24, 2009 11:32 AM
> To: David Brownell
> Cc: Aggarwal, Anuj; Mark Brown; linux-omap@vger.kernel.org; Tony Lindgren
> Subject: Re: Problems while designing TPS65023 regulator driver
> 
> Hi David,
> 
> On Fri, Apr 24, 2009 at 3:47 AM, David Brownell 
> wrote:
> > On Thursday 23 April 2009, Trilok Soni wrote:
> >> Any updates on tps65023 regulator driver? Could you please submit the
> >> WIP patches to the list?
> >
> > FWIW, here's the last version I saw ... it includes a
> > build hack for the regulator_register() call.  I haven't
> > build-tested it since that API change went to mainline.
> >
> > 
> > From: Manikandan Pillai 
> > Subject: regulator: add support for TPS6235x
> >
> > The patch has been fixed for comments given by David Brownell
> > and Mark Brown for adding TPS6235x support on OMAP3 EVM.
> > Comments fixed include
> > moving Makefile changes to this patch
> > dev_err used
> > removed the extra configuration option from Kconfig
> 
> Thanks but I was requesting tps 6 5 0 2 3 not tps 6 2 3 5 x  :).
> 
> --
> ---Trilok Soni
> http://triloksoni.wordpress.com
> http://www.linkedin.com/in/triloksoni

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-04-23 Thread Trilok Soni 
Hi David,

On Fri, Apr 24, 2009 at 3:47 AM, David Brownell  wrote:
> On Thursday 23 April 2009, Trilok Soni wrote:
>> Any updates on tps65023 regulator driver? Could you please submit the
>> WIP patches to the list?
>
> FWIW, here's the last version I saw ... it includes a
> build hack for the regulator_register() call.  I haven't
> build-tested it since that API change went to mainline.
>
> 
> From: Manikandan Pillai 
> Subject: regulator: add support for TPS6235x
>
> The patch has been fixed for comments given by David Brownell
> and Mark Brown for adding TPS6235x support on OMAP3 EVM.
> Comments fixed include
> moving Makefile changes to this patch
> dev_err used
> removed the extra configuration option from Kconfig

Thanks but I was requesting tps 6 5 0 2 3 not tps 6 2 3 5 x  :).

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-04-23 Thread David Brownell 
On Thursday 23 April 2009, Trilok Soni wrote:
> Any updates on tps65023 regulator driver? Could you please submit the
> WIP patches to the list?

FWIW, here's the last version I saw ... it includes a
build hack for the regulator_register() call.  I haven't
build-tested it since that API change went to mainline.


From: Manikandan Pillai 
Subject: regulator: add support for TPS6235x

The patch has been fixed for comments given by David Brownell
and Mark Brown for adding TPS6235x support on OMAP3 EVM.
Comments fixed include
moving Makefile changes to this patch
dev_err used
removed the extra configuration option from Kconfig

[ dbrown...@users.sourceforge.net: build hack ]
Signed-off-by: Manikandan Pillai 
---
 drivers/regulator/Kconfig  |8 
 drivers/regulator/Makefile |1 
 drivers/regulator/tps6235x-regulator.c |  350 +++
 3 files changed, 359 insertions(+)
 create mode 100644 drivers/regulator/tps6235x-regulator.c

--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -98,4 +98,12 @@ config REGULATOR_PCF50633
 Say Y here to support the voltage regulators and convertors
 on PCF50633
 
+config REGULATOR_TPS6235X
+   tristate "TI TPS6235x Power regulators"
+   depends on I2C
+   help
+ This driver supports TPS6235x voltage regulator chips, for values
+ of "x" from 0 to 6.  These are buck converters which support TI's
+ hardware based "SmartReflex" dynamic voltage scaling.
+
 endif
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -13,5 +13,6 @@ obj-$(CONFIG_REGULATOR_WM8350) += wm8350
 obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
 obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
+obj-$(CONFIG_REGULATOR_TPS6235X)+= tps6235x-regulator.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
--- /dev/null
+++ b/drivers/regulator/tps6235x-regulator.c
@@ -0,0 +1,350 @@
+/*
+ * tps6235x-regulator.c -- support regulators in tps6235x family chips
+ *
+ * Author : Manikandan Pillai
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * These chips are often used in OMAP-based systems.
+ *
+ * This driver implements software-based resource control for various
+ * voltage regulators.  This is usually augmented with state machine
+ * based control.
+ *
+ * For now, all regulator operations apply to VSEL1 (the "ceiling"),
+ * instead of VSEL0 (the "floor") which is used for low power modes.
+ * Also, this *assumes* only software mode control is used...
+*/
+
+#defineTPS6235X_REG_VSEL0  0
+#defineTPS6235X_REG_VSEL1  1
+#defineTPS6235X_REG_CTRL1  2
+#defineTPS6235X_REG_CTRL2  3
+
+/* VSEL bitfields (EN_DCDC is shared) */
+#defineTPS6235X_EN_DCDCBIT(7)
+#defineTPS6235X_LIGHTPFM   BIT(6)
+#defineTPS6235X_VSM_MSK(0x3F)
+
+/* CTRL1 bitfields */
+#define TPS6235X_EN_SYNC   BIT(5)
+#define TPS6235X_HW_nSWBIT(4)
+
+/* CTRL2 bitfields */
+#define TPS6235X_PWR_OK_MSKBIT(5)
+#define TPS6235X_OUT_DIS_MSK   BIT(6)
+#define TPS6235X_GO_MSKBIT(7)
+
+struct tps_info {
+   unsignedmin_uV;
+   unsignedmax_uV;
+   unsignedmult_uV;
+   boolfixed;
+};
+
+struct tps {
+   struct regulator_desc   desc;
+   struct i2c_client   *client;
+   struct regulator_dev*rdev;
+   const struct tps_info   *info;
+};
+
+static inline int tps_6235x_read_reg(struct tps *tps, u8 reg, u8 *val)
+{
+   int status;
+
+   status = i2c_smbus_read_byte_data(tps->client, reg);
+   *val = status;
+   if (status < 0)
+   return status;
+   return 0;
+}
+
+static inline int tps_6235x_write_reg(struct tps *tps, u8 reg, u8 val)
+{
+   return i2c_smbus_write_byte_data(tps->client, reg, val);
+}
+
+static int tps6235x_dcdc_is_enabled(struct regulator_dev *dev)
+{
+   unsigned char vsel1;
+   struct tps *tps = rdev_get_drvdata(dev);
+
+   tps_6235x_read_reg(tps, TPS6235X_REG_VSEL1, &vsel1);
+
+   return !(vsel1 & TPS6235X_EN_DCDC);
+}
+
+static int tps6235x_dcdc_enable(struct regulator_dev *dev)
+{
+   unsigned char vsel1;
+   int ret;
+   struct tps *tps = rdev_get_drvdata(dev);
+
+   ret = tps_6235x_read_reg(tps, TPS6235X_REG_VSEL1, &vsel1);
+
+   if (ret == 0) {
+   vsel1 |= TPS6235X_EN_DCDC;
+   ret = tps_6235x_write_reg(tps, TPS6235X_REG_VSEL1, vsel1);
+   }
+   return ret;
+}
+
+static int tps6235x_

Re: Problems while designing TPS65023 regulator driver

2009-04-23 Thread Trilok Soni 
Hi Anuj,

On Sat, Apr 4, 2009 at 5:35 AM, Tony Lindgren  wrote:
> * Mark Brown  [090403 01:53]:
>> On Fri, Apr 03, 2009 at 01:33:58PM +0530, Aggarwal, Anuj wrote:
>>
>> > I could not find the commit in linux-OMAP git where the init data is
>> > passed as a parameter to the regulator_register(). I am dependent
>> > on this commit for my TPS65023 regulator driver and could not push
>> > my patch without the commit being in l-o.
>>
>> > Any idea when this would be available in l-o?
>>
>> I can't speak for the OMAP tree but the patch should appear in
>> 2.6.30-rc1 so presumably it'll get merged into OMAP some time after that
>> is released.
>
> Sounds good to me.

Any updates on tps65023 regulator driver? Could you please submit the
WIP patches to the list?

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-04-03 Thread Tony Lindgren 
* Mark Brown  [090403 01:53]:
> On Fri, Apr 03, 2009 at 01:33:58PM +0530, Aggarwal, Anuj wrote:
> 
> > I could not find the commit in linux-OMAP git where the init data is 
> > passed as a parameter to the regulator_register(). I am dependent 
> > on this commit for my TPS65023 regulator driver and could not push 
> > my patch without the commit being in l-o.
> 
> > Any idea when this would be available in l-o?
> 
> I can't speak for the OMAP tree but the patch should appear in
> 2.6.30-rc1 so presumably it'll get merged into OMAP some time after that
> is released.

Sounds good to me.

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-04-03 Thread Mark Brown 
On Fri, Apr 03, 2009 at 01:33:58PM +0530, Aggarwal, Anuj wrote:

> I could not find the commit in linux-OMAP git where the init data is 
> passed as a parameter to the regulator_register(). I am dependent 
> on this commit for my TPS65023 regulator driver and could not push 
> my patch without the commit being in l-o.

> Any idea when this would be available in l-o?

I can't speak for the OMAP tree but the patch should appear in
2.6.30-rc1 so presumably it'll get merged into OMAP some time after that
is released.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Problems while designing TPS65023 regulator driver

2009-04-03 Thread Aggarwal, Anuj 
Mark,

I could not find the commit in linux-OMAP git where the init data is 
passed as a parameter to the regulator_register(). I am dependent 
on this commit for my TPS65023 regulator driver and could not push 
my patch without the commit being in l-o.

Any idea when this would be available in l-o?

Thanks and Regards,
Anuj Aggarwal

Thanks and Regards,
Anuj Aggarwal
 
Platform Support Products
Texas Instruments Inc
Ph: +91-80-2509-9542
TI IP Ph: 509-9542
PSP Products RSS Feed PSP Product Announcements
 

> -Original Message-
> From: Mark Brown [mailto:broo...@sirena.org.uk]
> Sent: Thursday, February 26, 2009 7:35 PM
> To: Aggarwal, Anuj
> Cc: linux-omap@vger.kernel.org
> Subject: Re: Problems while designing TPS65023 regulator driver
> 
> On Thu, Feb 26, 2009 at 02:41:54PM +0530, Aggarwal, Anuj wrote:
> 
> > Since all the five regulators can be controlled using a single i2c
> > device, I made a single i2c_board_info structure in my platform
> > specific file and put all the regulator_init_data information there:
> 
> This is very common - most of the devices that have multiple regulators
> also have some other subsystems on them (eg, an RTC or a watchdog) and
> use a core driver in drivers/mfd with the individual functions of the
> device as child platform drivers so this hasn't come up much.
> 
> > Now, the problem is in the tps_65023_probe function. Since it will be
> > called only once as there is only one i2c device, I have to register
> > all the regulators in that only. But I am not able to communicate the
> > same to the regulator core layer. Inside the regulator_register(),
> > variable init_data, which equals to dev->platform_data, is always
> > pointing to the first array member, which is coming from the evm
> > specific file. And it fails to register my second regulator instance,
> > set_consumer_device_supply() specifically failing for the second
> > iteration. Because of this, the probe function fails.
> 
> > How should I handle this scenario? Am I missing something in my 
> > implementation?
> 
> Use -next or the regulator git at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6
> 
> There the init data is passed as a parameter to regulator_register()
> rather than being read from the platform data so the problem goes away.
> The relevant commit is 8ec143c801ff0514ce92e69aa2f7bd48e73b9baa.
> 
> [Please fix your mail client to wrap at 80 columns - currently you have
> no line breaks in paragraphs which makes your mails a bit hard to read
> and reply to.]

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-03-10 Thread Mark Brown 
On Mon, Mar 09, 2009 at 04:45:26PM -0800, David Brownell wrote:
> On Sunday 08 March 2009, Mark Brown wrote:

> > >  - Regulators not marked as "boot_on" or "always_on" won't
> > >be active (and usecount will be 0) on return from setup.

> > This breaks the idea that we don't do anything unless explictly told to
> > do so.

> I'm not sure where you're drawing this "explicitly" line, but
> clearly it's not where I would draw it!  The board init code
> explicitly said "here's a regulator, with these settings for
> the boot_on and always_on flags".  If neither was set, they
> are obviously clear ... so the regulator shouldn't be enabled.

At the minute a zero initialised set of constraints means "don't touch
anything" - it doesn't grant any permissions to do anything, all that
can actually be done is inspect the state.  Some of the drivers use this
currently, having a block of regulator constraint data in a larger block
of platform data and registering all regulators on the chip
unconditionally.  Requiring boot_on or always_on to be set would mean
that these drivers would start powering everything off once the change
is merged unless the drivers are changed first.

If we are going to make this change it might be best to first spend a
release printing a big fat warning so it's harder for people to get
surprised by it, especially with stuff getting merged via platform
trees.

> > I did actually still consider adding code to power off the 
> > regulator but thought that there may also be situations where the state
> > really is unknown (eg, it depends on what the system booted from) and
> > it'd be useful to be able to punt to the consumers to figure it out.

The other use case I should've mentioned is for people who are reverse
engineering systems and initially want to fire things up and inspect the
state they get left with before they go figuring out what (if anything)
they want to do with it.  Even if you do know the design this can be
quite handy for testing that everything came up as expected, the kernel
provides a fairly convenient UI.

> The core problem with that thought is that if you try doing that,
> then consumers have exactly zero ways to fix the issue.  It's the
> scenario I listed above:  regulator is enabled, but refcount is
> zero.  So they're not allowed to disable.

It can do it by enabling (which is a noop) and then disabling - it's not
nice and wasn't really intentional but it gets the job done.

> That's in addition to the fact that "unknown" states are
> extremely error prone.  The state after initialization should
> fully known, without having to play such guessing games.

Yes, doing it via constriants is clearly better - I'm more thinking
about this in terms of "if you really want to do it this is how" than as
something I'd recommend people use.

> defined values.  Adding a second "always_on" flag makes for
> some confusion, since it only defines a third state, not a
> pair of states (it's not orthogonal).

We should just be able to remove always_on; it's equivalent to setting
boot_on and not enabling REGULATOR_CHANGE_STATUS.  I'll look into that
but it's got cross tree issues too.

> always have the consumer ops delegate to the real regulator.
> And have that real regulator's usecount set to one when it's
> enabled at boot time, so regulator_disable() will work then.

Clearly.  I'm wondering how that plays with multiple consumers, though.
Consumers will be able to disable regulators that were left on but
they'll need something to let them figure out why the device was left
on.  Or just not worry about supporting such users too strongly suggest
that they should be using something that gets added to the constraints.

Fancy kicking off a couple of new discussions on lkml?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-03-09 Thread David Brownell 
On Sunday 08 March 2009, Mark Brown wrote:
> On Sun, Mar 08, 2009 at 12:54:35PM -0800, David Brownell wrote:
> 
> > but the bootloader turned the regulator on, then drivers
> > can't disable the regulator (on penalty of a stackdump!)
> > unless they issue a spurious/pointless/undesirable enable()
> > beforehand ...
> 
> We can't easily have both reference counting and unbalanced disables,
> sadly.

The patch I sent had an "easy" solution though:  on return
from initialization (according to the board constraints),
refcount is zero if disable() was called, one if enable()
was called instead.

(There's a glitch associated with using multiple refcounts
though; see below.)


> >  - Regulators not marked as "boot_on" or "always_on" won't
> >be active (and usecount will be 0) on return from setup.
> 
> This breaks the idea that we don't do anything unless explictly told to
> do so.

I'm not sure where you're drawing this "explicitly" line, but
clearly it's not where I would draw it!  The board init code
explicitly said "here's a regulator, with these settings for
the boot_on and always_on flags".  If neither was set, they
are obviously clear ... so the regulator shouldn't be enabled.


> I did actually still consider adding code to power off the 
> regulator but thought that there may also be situations where the state
> really is unknown (eg, it depends on what the system booted from) and
> it'd be useful to be able to punt to the consumers to figure it out.

The core problem with that thought is that if you try doing that,
then consumers have exactly zero ways to fix the issue.  It's the
scenario I listed above:  regulator is enabled, but refcount is
zero.  So they're not allowed to disable.

That's in addition to the fact that "unknown" states are
extremely error prone.  The state after initialization should
fully known, without having to play such guessing games.


> I'm a bit ambivalent on this one, though - avoiding a sprawl of options
> is certainly neater.  An enum for the initial power state has an appeal
> here.

A boolean "boot_on" enum value seems sufficient.  Two clearly
defined values.  Adding a second "always_on" flag makes for
some confusion, since it only defines a third state, not a
pair of states (it's not orthogonal).

 
> > -   /* are we enabled at boot time by firmware / bootloader */
> > -   if (rdev->constraints->boot_on)
> > -   rdev->use_count = 1;
> > -
> 
> That's not there with the current regulator tree (this was the bug with
> not being able to disable boot_on regulators, there's no way to drop
> that use count later on).

Other than regulator_disable()?  I don't follow.   There was a
real mess of convoluted logic later on, true.  And I see it was
somewhat simplified by 38db9f31d6dc6147b87692b3b5a8a32de1a6cbe6.

Are you referring to the fact that the refcounting is oddly split
between the consumer handle ("struct regulator") and the real
regulator ("struct regulator_dev")?  If so, the fix is easy:
always have the consumer ops delegate to the real regulator.
And have that real regulator's usecount set to one when it's
enabled at boot time, so regulator_disable() will work then.


> Much of the rest of your patch will fail to apply due to similar
> changes; the logic that's there now is roughly the same as what you have
> here except we don't bother to check is_enabled() any more (no harm
> adding that back, it'd be useful if enable() can't be called for an
> already enabled regulator) and we don't disable the regulator.

 and, the most important bit in terms of being able to
use the regulator calls in some of these cases, disabling
regulators that aren't supposed to be enabled.

Updated version below.  It preserves the existing refcount
bug (noted above), and has only been build-tested.

- Dave


=== CUT HERE
From: David Brownell 

Make the regulator setup code more consistent:  unless a
regulator is marked as "boot_on" or "always_on", it will
always be configured as inactive on return from setup.

Note that there's still a mess with respect to refcounting,
which is shared unequally between consumer and provider handles
("struct regulator" and "struct regulator_dev" respectively).
Only the "inactive after setup" case works cleanly.

Signed-off-by: David Brownell 
---
 drivers/regulator/core.c |   29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -801,15 +801,28 @@ static int set_machine_constraints(struc
}
 
/* If the constraints say the regulator should be on at this point
-* and we have control then make sure it is enabled.
+* and we have control then make sure it is enabled.  Else, it's
+* supposed to be disabled ... be sure of that, instead.
 */
-   if ((constraints->always_on || constraints->boot_on) && ops->enable) {
-   ret = ops->enable(rdev);
-   if (ret < 0) {
-

Re: Problems while designing TPS65023 regulator driver

2009-03-08 Thread Mark Brown 
On Sun, Mar 08, 2009 at 12:54:35PM -0800, David Brownell wrote:

> but the bootloader turned the regulator on, then drivers
> can't disable the regulator (on penalty of a stackdump!)
> unless they issue a spurious/pointless/undesirable enable()
> beforehand ...

We can't easily have both reference counting and unbalanced disables,
sadly.

>  - Regulators not marked as "boot_on" or "always_on" won't
>be active (and usecount will be 0) on return from setup.

This breaks the idea that we don't do anything unless explictly told to
do so.  I did actually still consider adding code to power off the
regulator but thought that there may also be situations where the state
really is unknown (eg, it depends on what the system booted from) and
it'd be useful to be able to punt to the consumers to figure it out.

I'm a bit ambivalent on this one, though - avoiding a sprawl of options
is certainly neater.  An enum for the initial power state has an appeal
here.

> - /* are we enabled at boot time by firmware / bootloader */
> - if (rdev->constraints->boot_on)
> - rdev->use_count = 1;
> -

That's not there with the current regulator tree (this was the bug with
not being able to disable boot_on regulators, there's no way to drop
that use count later on).

Much of the rest of your patch will fail to apply due to similar
changes; the logic that's there now is roughly the same as what you have
here except we don't bother to check is_enabled() any more (no harm
adding that back, it'd be useful if enable() can't be called for an
already enabled regulator) and we don't disable the regulator.

> + } else if (is_enabled < 0) {
> + pr_warning("%s: hoping regulator '%s' is %sd...\n",
> +__func__, name, "enable");
> + }

I'm really not loving this %s for the enabled - yes, it'll save a small
amount of memory but it hurts gepability.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-03-08 Thread David Brownell 
On Saturday 07 March 2009, Mark Brown wrote:
> What's happening here is that the kernel is making sure that the
> information it was given about the state of the regulator is actually
> true in case it was important ...

If that's a goal, then I suggest merging the appended patch,
which addresses a similar case:  both boot_on and always_on
are clear, so the regulator should not be enabled.

This *can* be important, as e.g. if those flags are clear
but the bootloader turned the regulator on, then drivers
can't disable the regulator (on penalty of a stackdump!)
unless they issue a spurious/pointless/undesirable enable()
beforehand ...

- Dave

== CUT HERE
From: David Brownell 

Make the regulator setup code simpler and more consistent:

 - The only difference between "boot_on" and "always_on" is
   that an "always_on" regulator won't be disabled.  Both will
   be active (and usecount will be 1) on return from setup.

 - Regulators not marked as "boot_on" or "always_on" won't
   be active (and usecount will be 0) on return from setup.

The exception to that simple policy is when there's a non-Linux
interface to the regulator ... e.g. if either a DSP or the CPU
running Linux can enable the regulator, and the DSP needs it to
be on, then it will be on.

Signed-off-by: David Brownell 
---
 drivers/regulator/core.c |   62 ++---
 1 file changed, 47 insertions(+), 15 deletions(-)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -711,6 +711,8 @@ static int set_machine_constraints(struc
int ret = 0;
const char *name;
struct regulator_ops *ops = rdev->desc->ops;
+   int enable = 0;
+   int is_enabled = -ENOSYS;
 
if (constraints->name)
name = constraints->name;
@@ -799,10 +801,6 @@ static int set_machine_constraints(struc
}
}
 
-   /* are we enabled at boot time by firmware / bootloader */
-   if (rdev->constraints->boot_on)
-   rdev->use_count = 1;
-
/* do we need to setup our suspend state */
if (constraints->initial_state) {
ret = suspend_prepare(rdev, constraints->initial_state);
@@ -814,17 +812,51 @@ static int set_machine_constraints(struc
}
}
 
-   /* if always_on is set then turn the regulator on if it's not
-* already on. */
-   if (constraints->always_on && ops->enable &&
-   ((ops->is_enabled && !ops->is_enabled(rdev)) ||
-(!ops->is_enabled && !constraints->boot_on))) {
-   ret = ops->enable(rdev);
-   if (ret < 0) {
-   printk(KERN_ERR "%s: failed to enable %s\n",
-  __func__, name);
-   rdev->constraints = NULL;
-   goto out;
+   /* Should this be enabled when we return from here?  The difference
+* between "boot_on" and "always_on" is that "always_on" regulators
+* won't ever be disabled.
+*/
+   if (constraints->boot_on || constraints->always_on)
+   enable = 1;
+
+   /* Make sure the regulator isn't wrongly enabled or disabled.
+* Bootloaders are often sloppy about leaving things on; and
+* sometimes Linux wants to use a different model.
+*/
+   if (ops->is_enabled)
+   is_enabled = ops->is_enabled(rdev);
+   if (enable) {
+   if (ops->enable) {
+   /* forcibly enable if it's off or we can't tell */
+   if (is_enabled <= 0) {
+   ret = ops->enable(rdev);
+   pr_warning("%s: %s '%s' --> %d\n",
+  __func__, "enable", name, ret);
+   if (ret < 0) {
+   rdev->constraints = NULL;
+   goto out;
+   }
+   }
+   } else if (is_enabled < 0) {
+   pr_warning("%s: hoping regulator '%s' is %sd...\n",
+  __func__, name, "enable");
+   }
+   rdev->use_count = 1;
+   } else {
+   if (ops->disable) {
+   /* forcibly disable if it's on or we can't tell */
+   if (is_enabled != 0) {
+   ret = ops->disable(rdev);
+   pr_warning("%s: %s '%s' --> %d\n",
+  __func__, "disable", name, ret);
+   if (ret < 0) {
+   rdev->constraints = NULL;
+   goto out;
+   }
+   }
+   } else if (is_enabled < 0) {
+   pr_warning("%s: hoping regulator '%s' is %sd...\n",
+

Re: Problems while designing TPS65023 regulator driver

2009-03-07 Thread Mark Brown 
On Fri, Mar 06, 2009 at 04:07:16PM -0800, David Brownell wrote:

> The boot_on semantics are kind of odd then ...

> What I thought they meant:  Bootloader turned this on.

That's still roughly the case, though in practice there's no actual need
to do this for the vast majority of regulators since it is possible for
the kernel to just read the status of the regulator back at runtime
which is obviously more reliable.

> What you describe above:Kernel turn this on during startup.

What's happening here is that the kernel is making sure that the
information it was given about the state of the regulator is actually
true in case it was important (things could drift if the bootloader or
hardware are improved to boot up with a better default configuration,
for example).  The kernel could warn here but we'd need to be clear why
the constraints tell us that the regulator is on.

This also has the side effect of allowing the constraints to turn the
regulator on at startup, perhaps to aid early boot or perhaps because
not all the drivers in the system that need it have regulator support
yet, but that wasn't the primary purpose.

> Versus normal behavior: Consumer turns it on, as needed.

That now works as normal.  Originally using boot_on would've had the
effect of setting an always_on constraint which really wasn't desirable
since we already have that.

> I wouldn't have thought there would be a need for that
> second case, since the board-specific init code can
> just define a consumer that turns it on if that's what
> it needs.

Yes, it could - or it could define an always_on constraint if that were
what's needed.  On the other hand, it's the sort of thing that more than
one board is going to need to do so it does make sense to factor it out
a bit.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-03-06 Thread David Brownell 
On Friday 06 March 2009, Mark Brown wrote:
> > Later, I found out that in set_machine_constraints(),ops->enable() is
> > being called if the boot_on flag is set. What is the purpose of doing
> > this? Since the regulator is already enabled, why we are calling the
> > ops->enable() to do the same again? In my opinion, regulator_enable()
> 
> This ensures that the regulator is actually turned on.  Previously
> boot_on was equivalent to always_on and there was no way for a machine
> driver to turn a regulator on at startup so the semantics of boot_on
> were changed slightly to be usable to switch a regulator on at boot.

The boot_on semantics are kind of odd then ...

What I thought they meant:  Bootloader turned this on.
What you describe above:Kernel turn this on during startup.
Versus normal behavior: Consumer turns it on, as needed.

I wouldn't have thought there would be a need for that
second case, since the board-specific init code can
just define a consumer that turns it on if that's what
it needs.

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-03-06 Thread Mark Brown 
On Thu, Mar 05, 2009 at 05:33:55PM +0530, Aggarwal, Anuj wrote:

[Please fix your mail client to wrap lines at ~80 columns - not doing so
makes your mails much harder to read and reply to.]

> But still when I call regulator_disable() after doing a _get() on it,
> the call fails saying " unbalanced disables for supply". Then I checked
> the same repository again and found commit
> 38db9f31d6dc6147b87692b3b5a8a32de1a6cbe6 (regulator: Allow boot_on
> regulators to be disabled by clients). But still, it is not allowing me
> to disable the regulator as soon as I do a get on it. 

You'll need to do an enable followed by a disable for the benefit of the
reference counting that is done for the consumer usage.  What is the
consumer driver here?

> Later, I found out that in set_machine_constraints(),ops->enable() is
> being called if the boot_on flag is set. What is the purpose of doing
> this? Since the regulator is already enabled, why we are calling the
> ops->enable() to do the same again? In my opinion, regulator_enable()

This ensures that the regulator is actually turned on.  Previously
boot_on was equivalent to always_on and there was no way for a machine
driver to turn a regulator on at startup so the semantics of boot_on
were changed slightly to be usable to switch a regulator on at boot.

We could check to see if the regulator is already enabled but it didn't
really seem worth it - if it's a problem a check could be added to query
to see if the regulator is enabled before applying the boot/always on
constraints.

> should have been called to let the framework increase its usage count so
> that the user can disable the same as and when required.

This wouldn't do what you want - the regulator reference counts are two
level, they're counted in the consumer and then the regulator counts the
number of consumers which enable it.  If the core uses regulator_enable
that means it has a consumer allocated for the regulator and that
consumer will end up forcing the regulator to be always on (this was
essentially what the previous boot_on implementation ended up doing).

Consumers need to enable regulators they want to use even if they are
already enabled since otherwise the core may decide to disable the
regulator due to the action of some other consumer which is sharing the
supply.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Problems while designing TPS65023 regulator driver

2009-03-05 Thread Aggarwal, Anuj 
Mark,

Thanks for the patch, it worked fine for me.

I am facing one more problem now. I am setting boot_on flag in the constraints 
structure for all my regulators as they are enabled when the system is powered 
on.
But still when I call regulator_disable() after doing a _get() on it, the call 
fails saying " unbalanced disables for supply". Then I checked the same 
repository again and found commit 38db9f31d6dc6147b87692b3b5a8a32de1a6cbe6 
(regulator: Allow boot_on regulators to be disabled by clients). But still, it 
is not allowing me to disable the regulator as soon as I do a get on it.


Later, I found out that in set_machine_constraints(),ops->enable() is being 
called if the boot_on flag is set. What is the purpose of doing this? Since the 
regulator is already enabled, why we are calling the ops->enable() to do the 
same again? In my opinion, regulator_enable() should have been called to let 
the framework increase its usage count so that the user can disable the same as 
and when required.


Thanks and Regards,
Anuj Aggarwal
 

> -Original Message-
> From: Mark Brown [mailto:broo...@sirena.org.uk]
> Sent: Thursday, February 26, 2009 7:35 PM
> To: Aggarwal, Anuj
> Cc: linux-omap@vger.kernel.org
> Subject: Re: Problems while designing TPS65023 regulator driver
> 
> On Thu, Feb 26, 2009 at 02:41:54PM +0530, Aggarwal, Anuj wrote:
> 
> > Since all the five regulators can be controlled using a single i2c
> > device, I made a single i2c_board_info structure in my platform
> > specific file and put all the regulator_init_data information there:
> 
> This is very common - most of the devices that have multiple regulators
> also have some other subsystems on them (eg, an RTC or a watchdog) and
> use a core driver in drivers/mfd with the individual functions of the
> device as child platform drivers so this hasn't come up much.
> 
> > Now, the problem is in the tps_65023_probe function. Since it will be
> > called only once as there is only one i2c device, I have to register
> > all the regulators in that only. But I am not able to communicate the
> > same to the regulator core layer. Inside the regulator_register(),
> > variable init_data, which equals to dev->platform_data, is always
> > pointing to the first array member, which is coming from the evm
> > specific file. And it fails to register my second regulator instance,
> > set_consumer_device_supply() specifically failing for the second
> > iteration. Because of this, the probe function fails.
> 
> > How should I handle this scenario? Am I missing something in my 
> > implementation?
> 
> Use -next or the regulator git at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6
> 
> There the init data is passed as a parameter to regulator_register()
> rather than being read from the platform data so the problem goes away.
> The relevant commit is 8ec143c801ff0514ce92e69aa2f7bd48e73b9baa.
> 
> [Please fix your mail client to wrap at 80 columns - currently you have
> no line breaks in paragraphs which makes your mails a bit hard to read
> and reply to.]

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems while designing TPS65023 regulator driver

2009-02-26 Thread Mark Brown 
On Thu, Feb 26, 2009 at 02:41:54PM +0530, Aggarwal, Anuj wrote:

> Since all the five regulators can be controlled using a single i2c
> device, I made a single i2c_board_info structure in my platform
> specific file and put all the regulator_init_data information there:

This is very common - most of the devices that have multiple regulators
also have some other subsystems on them (eg, an RTC or a watchdog) and
use a core driver in drivers/mfd with the individual functions of the
device as child platform drivers so this hasn't come up much.

> Now, the problem is in the tps_65023_probe function. Since it will be
> called only once as there is only one i2c device, I have to register
> all the regulators in that only. But I am not able to communicate the
> same to the regulator core layer. Inside the regulator_register(),
> variable init_data, which equals to dev->platform_data, is always
> pointing to the first array member, which is coming from the evm
> specific file. And it fails to register my second regulator instance,
> set_consumer_device_supply() specifically failing for the second
> iteration. Because of this, the probe function fails.

> How should I handle this scenario? Am I missing something in my 
> implementation?

Use -next or the regulator git at:

git://git.kernel.org/pub/scm/linux/kernel/git/lrg/voltage-2.6

There the init data is passed as a parameter to regulator_register()
rather than being read from the platform data so the problem goes away.
The relevant commit is 8ec143c801ff0514ce92e69aa2f7bd48e73b9baa.

[Please fix your mail client to wrap at 80 columns - currently you have
no line breaks in paragraphs which makes your mails a bit hard to read
and reply to.]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html