[patch 2.6.28-rc6+] regulator: bugfixes and messaging cleanup

[patch 2.6.28-rc6+] regulator: bugfixes and messaging cleanup

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Regulator core bugfixes:
  * Move regulator earlier in link sequence.
       It initializes as a core_initcall() to be available early ...
       but links way late, so a regulator that's at subsys_initcall()
       is unavailable to subsystems which need it to initialize.
  * Prevent registration of duplicate "struct regulator" names.
       They'd be unavailable, and clearly indicate something wrong.
  * Some debug messages were wrongly at the KERN_ERR level
       Callers of regulator_get() have a fault code; messages for
       invalid parameters are thus more than usually superfluous.

And messaging updates;
  * Add debug messages when regulators get named and unnamed.
       Otherwise there's no way to trace what's happening until
       maybe a regulator_get() fails ... MUCH later.
  * Ditto when they get enabled and disabled.
       Same reasons.
  * Make some message used dev_err() not printk(KERN_ERR ...)
       Bare printk isn't as informative.  (Also got rid of two
       copies of one message...)

There are still messaging issues that deserve fixing:  printks outside
the enable/disable paths should also use driver model calls; and more
KERN_ERR messages that should really be dev_dbg().

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Presumably needs to go over the previous enable/disable updates.

 drivers/Makefile         |    4 ++-
 drivers/regulator/core.c |   59 +++++++++++++++++++++++++++++----------------
 2 files changed, 42 insertions(+), 21 deletions(-)

--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -18,6 +18,9 @@ obj-$(CONFIG_ARM_AMBA)		+= amba/
 
 obj-$(CONFIG_XEN)		+= xen/
 
+# regulators early, since some subsystems rely on them to initialize
+obj-$(CONFIG_REGULATOR)		+= regulator/
+
 # char/ comes before serial/ etc so that the VT console is the boot-time
 # default.
 obj-y				+= char/
@@ -101,5 +104,4 @@ obj-$(CONFIG_PPC_PS3)		+= ps3/
 obj-$(CONFIG_OF)		+= of/
 obj-$(CONFIG_SSB)		+= ssb/
 obj-$(CONFIG_VIRTIO)		+= virtio/
-obj-$(CONFIG_REGULATOR)		+= regulator/
 obj-$(CONFIG_STAGING)		+= staging/
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -775,6 +775,20 @@ static int set_consumer_device_supply(st
 	if (supply == NULL)
 		return -EINVAL;
 
+	list_for_each_entry(node, &regulator_map_list, list) {
+		if (consumer_dev != node->dev)
+			continue;
+		if (strcmp(node->supply, supply) != 0)
+			continue;
+
+		dev_dbg(consumer_dev, "%s/%s is '%s' supply; fail %s/%s\n",
+				dev_name(&node->regulator->dev),
+				node->regulator->desc->name,
+				supply,
+				dev_name(&rdev->dev), rdev->desc->name);
+		return -EBUSY;
+	}
+
 	node = kmalloc(sizeof(struct regulator_map), GFP_KERNEL);
 	if (node == NULL)
 		return -ENOMEM;
@@ -783,6 +797,10 @@ static int set_consumer_device_supply(st
 	node->dev = consumer_dev;
 	node->supply = supply;
 
+	dev_dbg(consumer_dev, "setting up %s/%s as '%s' supply\n",
+			dev_name(&rdev->dev),
+			rdev->desc->name, supply);
+
 	list_add(&node->list, &regulator_map_list);
 	return 0;
 }
@@ -793,8 +811,11 @@ static void unset_consumer_device_supply
 	struct regulator_map *node, *n;
 
 	list_for_each_entry_safe(node, n, &regulator_map_list, list) {
-		if (rdev == node->regulator &&
-			consumer_dev == node->dev) {
+		if (rdev == node->regulator && consumer_dev == node->dev) {
+			dev_dbg(consumer_dev,
+					"removing %s/%s as '%s' supply\n",
+					dev_name(&rdev->dev),
+					rdev->desc->name, node->supply);
 			list_del(&node->list);
 			kfree(node);
 			return;
@@ -894,7 +915,7 @@ struct regulator *regulator_get(struct d
 	struct regulator *regulator = ERR_PTR(-ENODEV);
 
 	if (id == NULL) {
-		printk(KERN_ERR "regulator: get() with no identifier\n");
+		dev_dbg(dev, "regulator_get with no identifier\n");
 		return regulator;
 	}
 
@@ -907,8 +928,7 @@ struct regulator *regulator_get(struct d
 			goto found;
 		}
 	}
-	printk(KERN_ERR "regulator: Unable to get requested regulator: %s\n",
-	       id);
+	dev_dbg(dev, "Unable to find regulator: %s\n", id);
 	mutex_unlock(&regulator_list_mutex);
 	return regulator;
 
@@ -971,19 +991,16 @@ static int _regulator_enable(struct regu
 	int ret = -EINVAL;
 
 	if (!rdev->constraints) {
-		printk(KERN_ERR "%s: %s has no constraints\n",
-		       __func__, rdev->desc->name);
+		dev_err(&rdev->dev, "%s has no constraints\n",
+		       rdev->desc->name);
 		return ret;
 	}
 
 	/* do we need to enable the supply regulator first */
 	if (rdev->supply) {
 		ret = _regulator_enable(rdev->supply);
-		if (ret < 0) {
-			printk(KERN_ERR "%s: failed to enable %s: %d\n",
-			       __func__, rdev->desc->name, ret);
-			return ret;
-		}
+		if (ret < 0)
+			goto fail;
 	}
 
 	/* check voltage and requested load before enabling */
@@ -995,15 +1012,16 @@ static int _regulator_enable(struct regu
 			drms_uA_update(rdev);
 
 		ret = rdev->desc->ops->enable(rdev);
-		if (ret < 0) {
-			printk(KERN_ERR "%s: failed to enable %s: %d\n",
-			       __func__, rdev->desc->name, ret);
-			return ret;
+		if (ret >= 0) {
+			dev_dbg(&rdev->dev, "enabled %s\n", rdev->desc->name);
+			rdev->use_count++;
 		}
-		rdev->use_count++;
-		return ret;
 	}
 
+fail:
+	if (ret < 0)
+		dev_err(&rdev->dev, "can't enable %s: %d\n",
+		       rdev->desc->name, ret);
 	return ret;
 }
 
@@ -1046,10 +1064,11 @@ static int _regulator_disable(struct reg
 		if (rdev->desc->ops->disable) {
 			ret = rdev->desc->ops->disable(rdev);
 			if (ret < 0) {
-				printk(KERN_ERR "%s: failed to disable %s\n",
-				       __func__, rdev->desc->name);
+				dev_err(&rdev->dev, "failed disabling %s: %d\n",
+				       rdev->desc->name, ret);
 				return ret;
 			}
+			dev_dbg(&rdev->dev, "disabled %s\n", rdev->desc->name);
 		}
 
 		/* decrease our supplies ref count and disable if required */

Re: [patch 2.6.28-rc6+] regulator: bugfixes and messaging cleanup

[Mark Brown]

On Mon, Dec 01, 2008 at 01:35:43PM -0800, David Brownell wrote:

> Regulator core bugfixes:

...

> And messaging updates;

This could usefuly be split into a patch series, there's rather a lot
of different changes in here...  On the whole it looks good, a few
things, though:

> @@ -894,7 +915,7 @@ struct regulator *regulator_get(struct d
>  	struct regulator *regulator = ERR_PTR(-ENODEV);
>  
>  	if (id == NULL) {
> -		printk(KERN_ERR "regulator: get() with no identifier\n");
> +		dev_dbg(dev, "regulator_get with no identifier\n");
>  		return regulator;
>  	}

dev may legally be NULL here which is going to cause upset if the
dev_dbg() is hit.  Things like cpufreq which can use regulators don't
use a struct device (currently!) so it's supported.

> @@ -971,19 +991,16 @@ static int _regulator_enable(struct regu
>  	int ret = -EINVAL;
>  
>  	if (!rdev->constraints) {
> -		printk(KERN_ERR "%s: %s has no constraints\n",
> -		       __func__, rdev->desc->name);
> +		dev_err(&rdev->dev, "%s has no constraints\n",
> +		       rdev->desc->name);

Hrm.  You've downgraded most of the diagnostics to dev_dbg() but left
this as KERN_ERR.  It'd be nice to be a bit more consistent.  My
personal preference is to make things in the constraints and machine
definitions display at dev_err() since they should never happen and a
plain text error is often helpful for new users but it's not a very
strong one either way.

>  		ret = rdev->desc->ops->enable(rdev);
> -		if (ret < 0) {
> -			printk(KERN_ERR "%s: failed to enable %s: %d\n",
> -			       __func__, rdev->desc->name, ret);
> -			return ret;
> +		if (ret >= 0) {
> +			dev_dbg(&rdev->dev, "enabled %s\n", rdev->desc->name);
> +			rdev->use_count++;

I have to say I'd be happier keeping the logic as it was here.  The
behaviour does stay the same due to fall through but it's not quite so
clear since it's not consistent with the error handling style for the
rest of the function.

Re: [patch 2.6.28-rc6+] regulator: bugfixes and messaging cleanup

[David Brownell]

On Monday 01 December 2008, Mark Brown wrote:
> On Mon, Dec 01, 2008 at 01:35:43PM -0800, David Brownell wrote:
> 
> > Regulator core bugfixes:
> 
> ...
> 
> > And messaging updates;
> 
> This could usefuly be split into a patch series, there's rather a lot
> of different changes in here...

Could be ... I was mostly just collecting core updates involved in
making sure I could regulator_enable() on one board.  The messaging
in the current code was decidedly un-helpful for that task.

At most I'd see two patches:  one with bugfixes, one with messaging
fixes that aren't strictly "bug" fixes (i.e. messages worked but
violated normal conventions like driver model).


> On the whole it looks good, a few 
> things, though:
> 
> > @@ -894,7 +915,7 @@ struct regulator *regulator_get(struct d
> >  	struct regulator *regulator = ERR_PTR(-ENODEV);
> >  
> >  	if (id == NULL) {
> > -		printk(KERN_ERR "regulator: get() with no identifier\n");
> > +		dev_dbg(dev, "regulator_get with no identifier\n");
> >  		return regulator;
> >  	}
> 
> dev may legally be NULL here

The kerneldoc doesn't mention that at all ... seems like a pretty
major interface artifact.  Hmm, for that matter I notice that if
you were to run kerneldoc, regulator_register() -- and presumably
other functions -- would get lots of errors.

Allowing it to be NULL seems pretty much contrary to the model that
a supply name is associated with a device.  I'd call that a bug and
fix it ... vs calling it a feature and diverging from standard
framework models.


> which is going to cause upset if the 
> dev_dbg() is hit.  Things like cpufreq which can use regulators don't
> use a struct device (currently!) so it's supported.

Which cpufreq driver(s) in mainline use regulator code?

I had commented not long ago that the regulator framework needs
updates to support cpufreq.  For example, trying to enable hardware
support for DVFS (Dynamic Voltage and Frequency Scaling) on several
platforms requires configuring more than one voltage ... floor and
ceiling, switched automatically by a CPU signal, for starters.

It would be good to overhaul cpufreq before trying to make it use
things like the clock and regulator frameworks.  Fitting into the
driver model seems overdue, for one.


> > @@ -971,19 +991,16 @@ static int _regulator_enable(struct regu
> >  	int ret = -EINVAL;
> >  
> >  	if (!rdev->constraints) {
> > -		printk(KERN_ERR "%s: %s has no constraints\n",
> > -		       __func__, rdev->desc->name);
> > +		dev_err(&rdev->dev, "%s has no constraints\n",
> > +		       rdev->desc->name);
> 
> Hrm.  You've downgraded most of the diagnostics to dev_dbg()

No; just ones in the enable/disable paths, and even there just
ones where the caller has control over the illegal parameters.

Recall that rdev->constraints can be nulled for various reasons
that aren't under control of the regulator client.


> but left 
> this as KERN_ERR.  It'd be nice to be a bit more consistent.  My
> personal preference is to make things in the constraints and machine
> definitions display at dev_err() since they should never happen and a
> plain text error is often helpful for new users but it's not a very
> strong one either way.

I think this was as consistent as possible with the basic rule
that when the caller could have prevented it, it's their job to
handle the fault code they get.


> 
> >  		ret = rdev->desc->ops->enable(rdev);
> > -		if (ret < 0) {
> > -			printk(KERN_ERR "%s: failed to enable %s: %d\n",
> > -			       __func__, rdev->desc->name, ret);
> > -			return ret;
> > +		if (ret >= 0) {
> > +			dev_dbg(&rdev->dev, "enabled %s\n", rdev->desc->name);
> > +			rdev->use_count++;
> 
> I have to say I'd be happier keeping the logic as it was here.  The
> behaviour does stay the same due to fall through but it's not quite so
> clear since it's not consistent with the error handling style for the
> rest of the function.

You mean in this case you'd want to see a (needless) GOTO?  I suppose
it could be done.  If you want consistency, I'd get rid of the unique
message for missing constraints, and rely on unique fault codes ...

- Dave

Re: [patch 2.6.28-rc6+] regulator: bugfixes and messaging cleanup

[Mark Brown]

On Mon, Dec 01, 2008 at 03:58:08PM -0800, David Brownell wrote:
> On Monday 01 December 2008, Mark Brown wrote:
> > On Mon, Dec 01, 2008 at 01:35:43PM -0800, David Brownell wrote:

> > This could usefuly be split into a patch series, there's rather a lot
> > of different changes in here...

> At most I'd see two patches:  one with bugfixes, one with messaging
> fixes that aren't strictly "bug" fixes (i.e. messages worked but
> violated normal conventions like driver model).

I'd expect that, for example, reordering the startup sequence could also
be usefully pulled out into a separate patch.

> > >  	if (id == NULL) {
> > > -		printk(KERN_ERR "regulator: get() with no identifier\n");
> > > +		dev_dbg(dev, "regulator_get with no identifier\n");
> > >  		return regulator;
> > >  	}

> > dev may legally be NULL here

> The kerneldoc doesn't mention that at all ... seems like a pretty
> major interface artifact.  Hmm, for that matter I notice that if

Well, it matchs the clock API and the fact that there isn't an explicit
check for a NULL dev along with the check for a NULL id.  :)

> you were to run kerneldoc, regulator_register() -- and presumably
> other functions -- would get lots of errors.

I'd not be surprised, I doubt it's ever been checked.

> Allowing it to be NULL seems pretty much contrary to the model that
> a supply name is associated with a device.  I'd call that a bug and
> fix it ... vs calling it a feature and diverging from standard
> framework models.

Ideally everything should use a device but sadly it's not universal yet.

> > which is going to cause upset if the 
> > dev_dbg() is hit.  Things like cpufreq which can use regulators don't
> > use a struct device (currently!) so it's supported.

> Which cpufreq driver(s) in mainline use regulator code?

None that I am aware of.  However, there's an i.MX31 cpufreq driver
queued on merging of the clock API support required (plus a non-cpufreq
implementation of DVFS for the same CPU which also uses the regulator
API).

> I had commented not long ago that the regulator framework needs
> updates to support cpufreq.  For example, trying to enable hardware
> support for DVFS (Dynamic Voltage and Frequency Scaling) on several
> platforms requires configuring more than one voltage ... floor and
> ceiling, switched automatically by a CPU signal, for starters.

I'd actually given some consideration to such regulators - while it's a
*bit* of an abuse of the API the fact that voltages are specified as
limits could probably be used somewhat idiomatically for the purpose,
especially if the CPU is controlling things autonomously rather than
having it done by software.  It does need thought, though - that idea is
giving me a bit of ick but it does reflect what's going on.

> It would be good to overhaul cpufreq before trying to make it use
> things like the clock and regulator frameworks.  Fitting into the
> driver model seems overdue, for one.

Well voulunteered :) .

> > > @@ -971,19 +991,16 @@ static int _regulator_enable(struct regu
> > >  	int ret = -EINVAL;

> > >  	if (!rdev->constraints) {
> > > -		printk(KERN_ERR "%s: %s has no constraints\n",
> > > -		       __func__, rdev->desc->name);
> > > +		dev_err(&rdev->dev, "%s has no constraints\n",
> > > +		       rdev->desc->name);

> > Hrm.  You've downgraded most of the diagnostics to dev_dbg()

> No; just ones in the enable/disable paths, and even there just
> ones where the caller has control over the illegal parameters.

You've adjusted things in the supply registration interface and get() in
this way as well and at least the change to the "unable to find" message
in get() may not be directly due to the caller (it could be due to a
failure on the part of the board code to register the supply name).

> Recall that rdev->constraints can be nulled for various reasons
> that aren't under control of the regulator client.

Well, clearly.  The client has no direct control over the constraints.

> I think this was as consistent as possible with the basic rule
> that when the caller could have prevented it, it's their job to
> handle the fault code they get.

I don't see handling the error code and explaining why it happend as
being orthogonal here.  My inclination would be to always log at least
constraint and setup errors (since callers are likely to just fail and
it's useful to say why), at least until the API becomes more widely used
since almost everyone is on a learning curve here.  People can enable
DEBUG but it's nice to just give the error.

I can see logging all the enable and disable errors becoming too spammy,
though, since they are much more likely to be retried.

> > >  		ret = rdev->desc->ops->enable(rdev);
> > > -		if (ret < 0) {
> > > -			printk(KERN_ERR "%s: failed to enable %s: %d\n",
> > > -			       __func__, rdev->desc->name, ret);
> > > -			return ret;
> > > +		if (ret >= 0) {

> > I have to say I'd be happier keeping the logic as it was here.  The
> > behaviour does stay the same due to fall through but it's not quite so
> > clear since it's not consistent with the error handling style for the
> > rest of the function.

> You mean in this case you'd want to see a (needless) GOTO?  I suppose

Yes, it's a bit harder to parse wth the reversed test - the change in
idiom raises an eyebrow, especially since we fall through into a case
marked fail for both success and failure.

> it could be done.  If you want consistency, I'd get rid of the unique
> message for missing constraints, and rely on unique fault codes ...

The point here is the patterns in the code - something that looks
different breaks the flow.

[patch 2.6.28-rc7] regulator: init/link earlier

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Move regulator earlier in link sequence.

The regulator core currently initializes as a core_initcall() to be
available early ... but then it links way late, throwing away that
benefit, so regulators available at e.g. subsys_initcall() are not
available to subsystems which need to use them.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
It's not clear "how early" to be; this works.

 drivers/Makefile |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -18,6 +18,9 @@ obj-$(CONFIG_ARM_AMBA)		+= amba/
 
 obj-$(CONFIG_XEN)		+= xen/
 
+# regulators early, since some subsystems rely on them to initialize
+obj-$(CONFIG_REGULATOR)		+= regulator/
+
 # char/ comes before serial/ etc so that the VT console is the boot-time
 # default.
 obj-y				+= char/
@@ -101,5 +104,4 @@ obj-$(CONFIG_PPC_PS3)		+= ps3/
 obj-$(CONFIG_OF)		+= of/
 obj-$(CONFIG_SSB)		+= ssb/
 obj-$(CONFIG_VIRTIO)		+= virtio/
-obj-$(CONFIG_REGULATOR)		+= regulator/
 obj-$(CONFIG_STAGING)		+= staging/

Re: [patch 2.6.28-rc7] regulator: init/link earlier

[Mark Brown]

On Mon, Dec 01, 2008 at 09:50:13PM -0800, David Brownell wrote:

> The regulator core currently initializes as a core_initcall() to be
> available early ... but then it links way late, throwing away that
> benefit, so regulators available at e.g. subsys_initcall() are not
> available to subsystems which need to use them.

> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [patch 2.6.28-rc7] regulator: init/link earlier

[Liam Girdwood]

On Mon, 2008-12-01 at 21:50 -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Move regulator earlier in link sequence.
> 
> The regulator core currently initializes as a core_initcall() to be
> available early ... but then it links way late, throwing away that
> benefit, so regulators available at e.g. subsys_initcall() are not
> available to subsystems which need to use them.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Applied.

Thanks

Liam

[patch 2.6.28-rc7] regulator: catch some registration errors

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Prevent registration of duplicate "struct regulator" names.
They'd be unavailable, and clearly indicate something wrong.

Also make sure the consumer device is provided.  It's nonsensical
to omit these, and not a documented part of the interface.  Since
no code in mainline does such stuff, this is just anti-oops medicine.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/regulator/core.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -772,9 +772,23 @@ static int set_consumer_device_supply(st
 {
 	struct regulator_map *node;
 
-	if (supply == NULL)
+	if (supply == NULL || !consumer_dev)
 		return -EINVAL;
 
+	list_for_each_entry(node, &regulator_map_list, list) {
+		if (consumer_dev != node->dev)
+			continue;
+		if (strcmp(node->supply, supply) != 0)
+			continue;
+
+		dev_dbg(consumer_dev, "%s/%s is '%s' supply; fail %s/%s\n",
+				dev_name(&node->regulator->dev),
+				node->regulator->desc->name,
+				supply,
+				dev_name(&rdev->dev), rdev->desc->name);
+		return -EBUSY;
+	}
+
 	node = kmalloc(sizeof(struct regulator_map), GFP_KERNEL);
 	if (node == NULL)
 		return -ENOMEM;

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[Mark Brown]

On Mon, Dec 01, 2008 at 09:50:35PM -0800, David Brownell wrote:

> Prevent registration of duplicate "struct regulator" names.
> They'd be unavailable, and clearly indicate something wrong.

This bit is good but...

> Also make sure the consumer device is provided.  It's nonsensical
> to omit these, and not a documented part of the interface.  Since
> no code in mainline does such stuff, this is just anti-oops medicine.

...we do still need to cater for cpufreq and other struct deviceless
consumers.  If you can guarantee that no such consumers will ever exist
then great but we're not there yet.

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[David Brownell]

On Tuesday 02 December 2008, Mark Brown wrote:
> > Also make sure the consumer device is provided.  It's nonsensical
> > to omit these, and not a documented part of the interface.  Since
> > no code in mainline does such stuff, this is just anti-oops medicine.
> 
> ...we do still need to cater for cpufreq and other struct deviceless
> consumers.  If you can guarantee that no such consumers will ever exist
> then great but we're not there yet.

Just for the record:  my feeling is that since no such
drivers currently use the regulator framework, it's wrong
to design-in breakage to support them.

When someone writes a cpufreq driver that uses the
regulator framework, they can arrange to provide the
relevant "struct device *" to make that work neatly.

Meanwhile, I still think the regulator framework should
be using driver model messages.  Since the only reason
not to use them is to support those non-existent drivers.

- Dave

p.s. Glad to see at least part of this patch get merged,
   even if related diagnostics are lacking.

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[Mark Brown]

On Mon, Jan 05, 2009 at 03:45:04PM -0800, David Brownell wrote:

> When someone writes a cpufreq driver that uses the
> regulator framework, they can arrange to provide the
> relevant "struct device *" to make that work neatly.

I'm fairly sure it's been mentioned before but there's some already, for
example:

http://opensource.wolfsonmicro.com/cgi-bin/gitweb.cgi?p=linux-2.6-audioplus.git;a=blob;f=arch/arm/mach-mx3/cpufreq.c;h=7a2d5154ada5fb58aeb49b261e6f08cb591b96b2;hb=refs/heads/imx31

Actually, I have some updates to that I should push, needs to be
disentangled from some other stuff first.

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[David Brownell]

On Tuesday 06 January 2009, Mark Brown wrote:
> On Mon, Jan 05, 2009 at 03:45:04PM -0800, David Brownell wrote:
> 
> > When someone writes a cpufreq driver that uses the
> > regulator framework, they can arrange to provide the
> > relevant "struct device *" to make that work neatly.
> 
> I'm fairly sure it's been mentioned before but there's some already, for
> example:
> 
>	[ Long URL for your i.MX3 cpufreq driver elided ] 

Right, and what's keeping you from providing a "struct device *"
for that to use?

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[Mark Brown]

On Tue, Jan 06, 2009 at 02:21:26AM -0800, David Brownell wrote:
> On Tuesday 06 January 2009, Mark Brown wrote:

> >	[ Long URL for your i.MX3 cpufreq driver elided ] 

> Right, and what's keeping you from providing a "struct device *"
> for that to use?

Well, clearly it can be done but I'd expect review issues from having
the architectures provide a half done representation of the CPU in the
device tree that's essentially private to the cpufreq driver (and then
has no tie in with cpufreq itself).

Personally, I'd be much happier to require a device if there were some
agreement about how CPUs should be done in the device model - at the
minute it all seems very ad-hoc where it's done at all.  For the time
being I'd prefer to bodge in a device for doing printouts in the
regulator core rather than forcing users to have a device when we don't
have a clear story on what to use.  Neither approach is particularly nice.

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[Liam Girdwood]

On Mon, 2008-12-01 at 21:50 -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Prevent registration of duplicate "struct regulator" names.
> They'd be unavailable, and clearly indicate something wrong.

This part is fine.

> 
> Also make sure the consumer device is provided.  It's nonsensical
> to omit these, and not a documented part of the interface.  Since
> no code in mainline does such stuff, this is just anti-oops medicine.
> 

I think Mark has probably mentioned the need to support CPUfreq drivers
with no struct device. I had a quick scan of the current in tree ARM
CPUfreq drivers and could not find one that uses struct device (someone
will now find one). 

I wonder if there is another way to approach this as I do agree that
passing NULL is probably not the best solution. Alternatives like
embedding struct device with the CPUfreq core or adding a regulator_get
API call for CPUfreq drivers all involve some work. Fwiw, I'd be happy
enough with the latter option.

Thanks

Liam

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[Mark Brown]

On Wed, Dec 03, 2008 at 09:48:41PM +0000, Liam Girdwood wrote:

> I wonder if there is another way to approach this as I do agree that
> passing NULL is probably not the best solution. Alternatives like
> embedding struct device with the CPUfreq core or adding a regulator_get
> API call for CPUfreq drivers all involve some work. Fwiw, I'd be happy
> enough with the latter option.

We could also accept the NULL but warn when doing so, perhaps also
adding a dummy device from the regulator core for the logging.  That'd
provide some pushback on cpufreq to move to the device model if nobody
is able to convert it right now.

On the bright side, looking at the situation with the clock API there
don't seem to be any other substantial offenders here.  Most of the
other users are part of the platform code and get to peer inside the
clocking structure, so if cpufreq were fixed there shouldn't be much
problem here.

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[David Brownell]

On Thursday 04 December 2008, Mark Brown wrote:
> [ Re Liam's comments about those non-existent cpufreq+regulator drivers ]
>
> On the bright side, looking at the situation with the clock API there
> don't seem to be any other substantial offenders here.  Most of the
> other users are part of the platform code and get to peer inside the
> clocking structure, so if cpufreq were fixed there shouldn't be much
> problem here.

Last time I looked, no cpufreq driver tried to use <linux/clk.h>
calls.  The reason was simple:  the clock framework code sits in
DRAM, so it can't be executed while changing DRAM clocks.

Those bits of cpufreq had to live in on-chip SRAM (there's usually
a few pages of it) and directly update PLL and other clock config
registers ... then wait for things to stabilize before they returned
and the CPU executed from DRAM again.

- Dave
 

Re: [patch 2.6.28-rc7] regulator: catch some registration errors

[Mark Brown]

On Mon, Jan 05, 2009 at 03:45:24PM -0800, David Brownell wrote:

> Last time I looked, no cpufreq driver tried to use <linux/clk.h>

There's at least these ones kicking about at the minute:

	arch/arm/mach-imx/cpufreq.c
	arch/avr32/mach-at32ap/cpufreq.c
	arch/sh/kernel/cpufreq.c

as well as several out of tree ones I'm aware of (the i.MX31 example I
posted, some other i.MX BSPs and one for the s3c24xx).  There's an
opportunity for generalisation here since they're pretty much all very
similar adaption layers with the actual work in the clock framework.

> calls.  The reason was simple:  the clock framework code sits in
> DRAM, so it can't be executed while changing DRAM clocks.

> Those bits of cpufreq had to live in on-chip SRAM (there's usually
> a few pages of it) and directly update PLL and other clock config
> registers ... then wait for things to stabilize before they returned
> and the CPU executed from DRAM again.

That's a fairly system-specific concern, though, and there's no reason
why the code that does that dance has to live in the cpufreq driver
rather than the clock framework.  On some systems the clock framework
will need to know about what the CPU clock is doing anyway.