Re: [PATCH] regulator: Provide dummy supply support

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |