[PATCH] regulator: provide dummy supply support

[PATCH] regulator: provide dummy supply support

[Sascha Hauer]

Hi,

The following patch allows it for boards to register a dummy supply for
devices for which no software controllable regulator is available. The
current CONFIG_REGULATOR_DUMMY and (unused) regulator_use_dummy_regulator()
mechanisms only allow to provide a dummy regulator for *all* devices
for which no regulator is found. This would shadow real errors and thus
is not really an option for production kernels.

This patch may eventually superseed the current all-or-nothing approach.

This came to me while looking over Linus' smsc911x regulator support
patch and so I added all people affected by this patch to Cc.

Sascha

[PATCH] regulator: Provide dummy supply support

[Sascha Hauer]

Currently we have CONFIG_REGULATOR_DUMMY which provides a fallback
dummy regulator if none is found. Enabling this option shadows
real regulator_get errors though and can't be used in production.
Also there is regulator_use_dummy_regulator() which has the
same behaviour but can be used during runtime.

This patch allows a board to register dummy supplies for devices
which need a regulator but which is not software controllable
on this board.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/Makefile        |    2 +-
 drivers/regulator/dummy-supply.c  |   88 +++++++++++++++++++++++++++++++++++++
 include/linux/regulator/machine.h |   18 ++++++++
 3 files changed, 107 insertions(+), 1 deletions(-)
 create mode 100644 drivers/regulator/dummy-supply.c

diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 040d5aa..ff618b9 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -3,7 +3,7 @@
 #
 
 
-obj-$(CONFIG_REGULATOR) += core.o dummy.o
+obj-$(CONFIG_REGULATOR) += core.o dummy.o dummy-supply.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
diff --git a/drivers/regulator/dummy-supply.c b/drivers/regulator/dummy-supply.c
new file mode 100644
index 0000000..4e62b1f
--- /dev/null
+++ b/drivers/regulator/dummy-supply.c
@@ -0,0 +1,88 @@
+/*
+ * dummy-supply.c
+ *
+ * Copyright 2011 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
+ *
+ * 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.
+ *
+ * This is useful for systems with mixed controllable and
+ * non-controllable regulators, as well as for allowing testing on
+ * systems with no controllable regulators.
+ */
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+struct dummy_regulator {
+	struct regulator_desc desc;
+	struct regulator_init_data initdata;
+	struct regulator_dev *rdev;
+};
+
+static struct regulator_ops dummy_ops;
+
+/**
+ * regulator_register_dummy - register a dummy supply
+ *
+ * Calling this function will register a dummy regulator
+ * for devices for which no software controllable regulator
+ * is available.
+ */
+struct dummy_regulator *regulator_register_dummy(
+		struct regulator_consumer_supply *__supply,
+		int num_supplies)
+{
+	struct dummy_regulator *dummy;
+	struct regulator_consumer_supply *supply;
+
+	dummy = kzalloc(sizeof(*dummy), GFP_KERNEL);
+	if (!dummy)
+		return NULL;
+
+	dummy->desc.name = "dummy";
+	dummy->desc.id = -1;
+	dummy->desc.type = REGULATOR_VOLTAGE;
+	dummy->desc.owner = THIS_MODULE;
+	dummy->desc.ops = &dummy_ops;
+
+	supply = kzalloc(sizeof(*supply) * num_supplies, GFP_KERNEL);
+	if (!supply)
+		goto err_alloc;
+	memcpy(supply, __supply, sizeof(*supply) * num_supplies);
+
+	dummy->initdata.num_consumer_supplies = num_supplies;
+	dummy->initdata.consumer_supplies = supply,
+
+	dummy->rdev = regulator_register(&dummy->desc, NULL,
+						  &dummy->initdata, NULL);
+	if (!dummy->rdev)
+		goto err_register;
+
+	return dummy;
+
+err_register:
+	kfree(supply);
+err_alloc:
+	kfree(dummy);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(regulator_register_dummy);
+
+/**
+ * regulator_unregister_dummy - unregister a dummy supply
+ *
+ * This function unregisters a supply previously registered
+ * with regulator_register_dummy.
+ */
+void regulator_unregister_dummy(struct dummy_regulator *dummy)
+{
+	regulator_unregister(dummy->rdev);
+	kfree(dummy->initdata.consumer_supplies);
+	kfree(dummy);
+}
+EXPORT_SYMBOL_GPL(regulator_unregister_dummy);
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index ce3127a..13245cb 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -186,12 +186,20 @@ struct regulator_init_data {
 	void *driver_data;	/* core does not touch this */
 };
 
+/**
+ * cookie for regulator_register_dummy
+ */
+struct dummy_regulator;
+
 int regulator_suspend_prepare(suspend_state_t state);
 int regulator_suspend_finish(void);
 
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
 void regulator_use_dummy_regulator(void);
+struct dummy_regulator *regulator_register_dummy(
+		struct regulator_consumer_supply *, int);
+void regulator_unregister_dummy(struct dummy_regulator *);
 #else
 static inline void regulator_has_full_constraints(void)
 {
@@ -200,6 +208,16 @@ static inline void regulator_has_full_constraints(void)
 static inline void regulator_use_dummy_regulator(void)
 {
 }
+
+static inline struct dummy_regulator *regulator_register_dummy(
+		struct regulator_consumer_supply *, int)
+{
+	return (void *)-1;
+};
+
+void regulator_unregister_dummy(struct dummy_regulator *)
+{
+}
 #endif
 
 #endif
-- 
1.7.7

Re: [PATCH] regulator: Provide dummy supply support

[Mark Brown]

On Fri, Oct 28, 2011 at 10:26:58PM +0200, Sascha Hauer wrote:

>  drivers/regulator/Makefile        |    2 +-
>  drivers/regulator/dummy-supply.c  |   88 +++++++++++++++++++++++++++++++++++++

We already have a dummy regulator driver and a fixed voltage regulator
driver, we shouldn't be adding a third implementation of the same thing.
Just use the fixed voltage regulator for this.

Re: [PATCH] regulator: Provide dummy supply support

[Sascha Hauer]

On Fri, Oct 28, 2011 at 11:59:31PM +0200, Mark Brown wrote:
> On Fri, Oct 28, 2011 at 10:26:58PM +0200, Sascha Hauer wrote:
> 
> >  drivers/regulator/Makefile        |    2 +-
> >  drivers/regulator/dummy-supply.c  |   88 +++++++++++++++++++++++++++++++++++++
> 
> We already have a dummy regulator driver and a fixed voltage regulator
> driver, we shouldn't be adding a third implementation of the same thing.
> Just use the fixed voltage regulator for this.

I explained in my mail why I think that the current implementation of
the dummy regulator is not suitable for things apart from debugging.

My main concern with the fixed regulator is that it needs quite much
boilerplate code just to say that we have no regulator at all for a
given device. That could also be handled with a helper function which
registers a fixed regulator and only takes the regulator_consumer_supply
as an argument. Would that be ok for you?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] regulator: Provide dummy supply support

[Mike Frysinger]

On Sat, Oct 29, 2011 at 00:47, Sascha Hauer wrote:
> On Fri, Oct 28, 2011 at 11:59:31PM +0200, Mark Brown wrote:
>> On Fri, Oct 28, 2011 at 10:26:58PM +0200, Sascha Hauer wrote:
>>
>> >  drivers/regulator/Makefile        |    2 +-
>> >  drivers/regulator/dummy-supply.c  |   88 +++++++++++++++++++++++++++++++++++++
>>
>> We already have a dummy regulator driver and a fixed voltage regulator
>> driver, we shouldn't be adding a third implementation of the same thing.
>> Just use the fixed voltage regulator for this.
>
> I explained in my mail why I think that the current implementation of
> the dummy regulator is not suitable for things apart from debugging.
>
> My main concern with the fixed regulator is that it needs quite much
> boilerplate code just to say that we have no regulator at all for a
> given device. That could also be handled with a helper function which
> registers a fixed regulator and only takes the regulator_consumer_supply
> as an argument. Would that be ok for you?

i think Mark's point is that we have code in dummy.c already to
provide a dummy regulator.  so your needs sounds like it could be
satisfied with some Kconfig/ifdef massaging and the existing
drivers/regulator/dummy.c rather than introducing a completely
parallel file that is always enabled ?
-mike

Re: [PATCH] regulator: Provide dummy supply support

[Mark Brown]

On Sat, Oct 29, 2011 at 12:47:57AM +0200, Sascha Hauer wrote:

> My main concern with the fixed regulator is that it needs quite much
> boilerplate code just to say that we have no regulator at all for a
> given device. That could also be handled with a helper function which
> registers a fixed regulator and only takes the regulator_consumer_supply
> as an argument. Would that be ok for you?

All you're actually doing in this code is adding a function to register
a new type of regulator which is exactly equivalent to what the existing
regulators provide - there's nothing particularly wrong with the helper
function but defining an entirely new regulator type for it doesn't seem
useful.

Re: [PATCH] regulator: Provide dummy supply support

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1206 bytes --]

On Friday 28 October 2011 18:47:57 Sascha Hauer wrote:
> On Fri, Oct 28, 2011 at 11:59:31PM +0200, Mark Brown wrote:
> > On Fri, Oct 28, 2011 at 10:26:58PM +0200, Sascha Hauer wrote:
> > >  drivers/regulator/Makefile        |    2 +-
> > >  drivers/regulator/dummy-supply.c  |   88
> > >  +++++++++++++++++++++++++++++++++++++
> > 
> > We already have a dummy regulator driver and a fixed voltage regulator
> > driver, we shouldn't be adding a third implementation of the same thing.
> > Just use the fixed voltage regulator for this.
> 
> I explained in my mail why I think that the current implementation of
> the dummy regulator is not suitable for things apart from debugging.

your complaints seem to be specific to how the dummy regulator gets hooked in 
and not in the specific regulator implementation.  so it seems like the right 
thing would be to split the kconfig knobs:

 config REGULATOR_DUMMY
-	bool "Provide a dummy regulator if regulator lookups fail"
+	bool "Provide a dummy regulator"
+config REGULATOR_DUMMY_FALLBACK
+	bool "Fallback to dummy regulator if lookup fails"
+	depends on REGULATOR_DUMMY

(and then obviously update the .c files accordingly)
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: Provide dummy supply support

[Mark Brown]

On Tue, Nov 01, 2011 at 01:50:14PM -0400, Mike Frysinger wrote:
> On Friday 28 October 2011 18:47:57 Sascha Hauer wrote:

> > > We already have a dummy regulator driver and a fixed voltage regulator
> > > driver, we shouldn't be adding a third implementation of the same thing.
> > > Just use the fixed voltage regulator for this.

> > I explained in my mail why I think that the current implementation of
> > the dummy regulator is not suitable for things apart from debugging.

> your complaints seem to be specific to how the dummy regulator gets hooked in 
> and not in the specific regulator implementation.  so it seems like the right 
> thing would be to split the kconfig knobs:

Quite.  Sascha, your mail doesn't refer to the implementation of the
regulator itself at all.  Nothing in your changelog even mentions that
you're introducing a new regulator driver.

I think there's a big abstraction understanding failure here, reading
your changelog I'm not sure you understand the existing mechainsms that
are in place.  You say:

| This patch allows a board to register dummy supplies for devices
| which need a regulator but which is not software controllable
| on this board.

but this is exactly the use case the fixed voltage regulator is there
for.

>  config REGULATOR_DUMMY
> -	bool "Provide a dummy regulator if regulator lookups fail"
> +	bool "Provide a dummy regulator"
> +config REGULATOR_DUMMY_FALLBACK
> +	bool "Fallback to dummy regulator if lookup fails"
> +	depends on REGULATOR_DUMMY

As I think I said earlier I'd use the fixed regulator for this, all
Sascha's actually doing here is adding a wrapper to simplify
registration of that.

Re: [PATCH] regulator: Provide dummy supply support

[Sascha Hauer]

On Tue, Nov 01, 2011 at 06:27:21PM +0000, Mark Brown wrote:
> On Tue, Nov 01, 2011 at 01:50:14PM -0400, Mike Frysinger wrote:
> > On Friday 28 October 2011 18:47:57 Sascha Hauer wrote:
> 
> > > > We already have a dummy regulator driver and a fixed voltage regulator
> > > > driver, we shouldn't be adding a third implementation of the same thing.
> > > > Just use the fixed voltage regulator for this.
> 
> > > I explained in my mail why I think that the current implementation of
> > > the dummy regulator is not suitable for things apart from debugging.
> 
> > your complaints seem to be specific to how the dummy regulator gets hooked in 
> > and not in the specific regulator implementation.  so it seems like the right 
> > thing would be to split the kconfig knobs:
> 
> Quite.  Sascha, your mail doesn't refer to the implementation of the
> regulator itself at all.  Nothing in your changelog even mentions that
> you're introducing a new regulator driver.
> 
> I think there's a big abstraction understanding failure here, reading
> your changelog I'm not sure you understand the existing mechainsms that
> are in place.  You say:
> 
> | This patch allows a board to register dummy supplies for devices
> | which need a regulator but which is not software controllable
> | on this board.
> 
> but this is exactly the use case the fixed voltage regulator is there
> for.
> 
> >  config REGULATOR_DUMMY
> > -	bool "Provide a dummy regulator if regulator lookups fail"
> > +	bool "Provide a dummy regulator"
> > +config REGULATOR_DUMMY_FALLBACK
> > +	bool "Fallback to dummy regulator if lookup fails"
> > +	depends on REGULATOR_DUMMY
> 
> As I think I said earlier I'd use the fixed regulator for this, all
> Sascha's actually doing here is adding a wrapper to simplify
> registration of that.

There's one difference between the fixed and the dummy regulator though:
The fixed regulator has a voltage. The same dummy regulator instance can
be used for all devices which do not have a software controllable
regulator. I think the same can be done with the fixed regulator aswell,
but the bogus voltage showing up in the sysfs entry might be confusing
to users.
That's the reason I implemented a (second) dummy regulator driver.
Indeed it's not nice to have two of them.

Another approach to this topic would be to allow a board to explicitely
bind to the existing dummy regulator, like the following (error path
should of course be implemented before applying this)

Sascha

8<------------------------------------

[PATCH] regulator: allow boards to bind to the dummy regulator

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c          |   19 +++++++++++++++++++
 include/linux/regulator/machine.h |    8 ++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d8e6a42..a7a38ba 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1046,6 +1046,25 @@ static void unset_regulator_supplies(struct regulator_dev *rdev)
 	}
 }
 
+int regulator_add_dummy_supply(struct regulator_consumer_supply *supply,
+		int num_supplies)
+{
+	int i, ret;
+
+	for (i = 0; i < num_supplies; i++) {
+		ret = set_consumer_device_supply(dummy_regulator_rdev, NULL,
+				supply[i].dev_name, supply[i].supply);
+		if (ret)
+			goto err_out;
+	}
+
+	return 0;
+err_out:
+	/* FIXME: unset device supply */
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_add_dummy_supply);
+
 #define REG_STR_SIZE	64
 
 static struct regulator *create_regulator(struct regulator_dev *rdev,
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index ce3127a..89089cd 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -192,6 +192,8 @@ int regulator_suspend_finish(void);
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
 void regulator_use_dummy_regulator(void);
+int regulator_add_dummy_supply(struct regulator_consumer_supply *supply,
+		int num_supplies);
 #else
 static inline void regulator_has_full_constraints(void)
 {
@@ -200,6 +202,12 @@ static inline void regulator_has_full_constraints(void)
 static inline void regulator_use_dummy_regulator(void)
 {
 }
+
+static inline int regulator_add_dummy_supply(struct regulator_consumer_supply *supply,
+		int num_supplies)
+{
+	return 0;
+}
 #endif
 
 #endif
-- 
1.7.7

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] regulator: Provide dummy supply support

[Mark Brown]

On Wed, Nov 02, 2011 at 11:03:56AM +0100, Sascha Hauer wrote:
> On Tue, Nov 01, 2011 at 06:27:21PM +0000, Mark Brown wrote:

> > As I think I said earlier I'd use the fixed regulator for this, all
> > Sascha's actually doing here is adding a wrapper to simplify
> > registration of that.

> There's one difference between the fixed and the dummy regulator though:
> The fixed regulator has a voltage. The same dummy regulator instance can

No, the voltage is optional.

> be used for all devices which do not have a software controllable
> regulator. I think the same can be done with the fixed regulator aswell,
> but the bogus voltage showing up in the sysfs entry might be confusing
> to users.

I don't think that's a meaningful issue, any in any case it'd be better
practice to fill it in so devices can use the information if they want
to (which really shouldn't be hard

> Another approach to this topic would be to allow a board to explicitely
> bind to the existing dummy regulator, like the following (error path
> should of course be implemented before applying this)

If you know you've got a fixed voltage supply I don't understand why you
wouldn't want to set one up.  There clearly is an actual supply in the
system...

Re: [PATCH] regulator: Provide dummy supply support

[Sascha Hauer]

On Wed, Nov 02, 2011 at 10:41:27AM +0000, Mark Brown wrote:
> On Wed, Nov 02, 2011 at 11:03:56AM +0100, Sascha Hauer wrote:
> > On Tue, Nov 01, 2011 at 06:27:21PM +0000, Mark Brown wrote:
> 
> > > As I think I said earlier I'd use the fixed regulator for this, all
> > > Sascha's actually doing here is adding a wrapper to simplify
> > > registration of that.
> 
> > There's one difference between the fixed and the dummy regulator though:
> > The fixed regulator has a voltage. The same dummy regulator instance can
> 
> No, the voltage is optional.

Ok, didn't know this.

> 
> > be used for all devices which do not have a software controllable
> > regulator. I think the same can be done with the fixed regulator aswell,
> > but the bogus voltage showing up in the sysfs entry might be confusing
> > to users.
> 
> I don't think that's a meaningful issue, any in any case it'd be better
> practice to fill it in so devices can use the information if they want
> to (which really shouldn't be hard
> 
> > Another approach to this topic would be to allow a board to explicitely
> > bind to the existing dummy regulator, like the following (error path
> > should of course be implemented before applying this)
> 
> If you know you've got a fixed voltage supply I don't understand why you
> wouldn't want to set one up.  There clearly is an actual supply in the
> system...

It's just that it's simpler for a board to provide all dummy supplies in
a single array, no matter if the real voltage is 1.8V or 3.3V. Anyway,
as the voltage is optional in a fixed regulator this is no problem. I'll
update my patch to use the fixed regulator then.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] regulator: provide dummy supply support

[Mark Brown]

On Fri, Oct 28, 2011 at 10:26:57PM +0200, Sascha Hauer wrote:

> The following patch allows it for boards to register a dummy supply for
> devices for which no software controllable regulator is available. The
> current CONFIG_REGULATOR_DUMMY and (unused) regulator_use_dummy_regulator()

Please don't send cover letters for single patch serieses, if there's
content it should probably be in the changelog...

Re: [PATCH] regulator: provide dummy supply support

[Mike Frysinger]

On Fri, Oct 28, 2011 at 23:57, Mark Brown wrote:
> On Fri, Oct 28, 2011 at 10:26:57PM +0200, Sascha Hauer wrote:
>> The following patch allows it for boards to register a dummy supply for
>> devices for which no software controllable regulator is available. The
>> current CONFIG_REGULATOR_DUMMY and (unused) regulator_use_dummy_regulator()
>
> Please don't send cover letters for single patch serieses, if there's
> content it should probably be in the changelog...

sorry for the additional noise, but +1
-mike