regulator: bugfixes and messaging cleanup

From: David Brownell <[email protected]>

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().

---
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(-)

@@ -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/

...

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:

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).

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

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.

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.

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

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

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. :)

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

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

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'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.

Well voulunteered :) .

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).

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

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.

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.

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

From: David Brownell <[email protected]>

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.

---
It's not clear "how early" to be; this works.

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

@@ -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/

Applied.

Thanks

Liam

From: David Brownell <[email protected]>

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.

---
 drivers/regulator/core.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

@@ -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;

This bit is good but...

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

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.

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.

This part is fine.

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

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.

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

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.

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.