Re: [patch 2.6.29-rc6+misc] MMC: regulator utilities

[David Brownell <david-b@pacbell.net>]

On Monday 02 March 2009, Pierre Ossman wrote:
> On Thu, 26 Feb 2009 13:48:30 -0800
> David Brownell <david-b@pacbell.net> wrote:
> 
> > From: David Brownell <dbrownell@users.sourceforge.net>
> > 
> > Add optional glue between MMC and regulator stacks, using a new
> > regulator interface to learn what voltages are available.
> > 
> > This is intended to be selected and driven by MMC host adapters.
> > It only handles reusable parts of the regulator-to-MMC glue; the
> > adapter drivers will have access to details that affect how this
> > is used.  Examples include when to use multiple voltage rails or
> > configure (internal or external) level shifters.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> > ---
> > Changes from previous version:  adapter must select this, and
> > callers now pass in the regulator.  mmc_regulator_set_ocr()
> > is still not tested, mmc_regulator_get_ocrmask() passed sanity
> > testing.
> > 
> > Pierre:  Mark may have a need for this soonish.  The omap_hsmmc
> > code will want it at some point.
> > 
> 
> I have no insight into the regulator stuff, so I'm going to have to
> trust you on this. :)

Works for me.  ;)


> Some nitpicking though:
> 
> > --- a/drivers/mmc/core/Kconfig
> > +++ b/drivers/mmc/core/Kconfig
> > @@ -14,3 +14,10 @@ config MMC_UNSAFE_RESUME
> >  	  This option is usually just for embedded systems which use
> >  	  a MMC/SD card for rootfs. Most people should say N here.
> >  
> > +config MMC_REGULATOR
> > +	bool
> > +	depends on REGULATOR
> > +	help
> > +	  Select this if your MMC host adapter driver wants helper
> > +	  utilities for accessing power rails.
> > +
> 
> Is there a need for a special Kconfig for this? Can't we just build
> these two whenever REGULATOR is defined? Or always, provided the
> regulator API is present even when the code isn't.

The first patch had a "default y" there, nobody commented.
I'll simplify that, and use #ifdef CONFIG_REGULATOR instead.

 
> > +/**
> > + * mmc_regulator_set_ocr - set regulator to match host->ios voltage
> > + * @host: mmc host whose supply voltage will be changed
> > + * @supply: regulator to use
> > + *
> > + * MMC host drivers may use this to enable or disable a regulator using
> > + * a particular supply voltage.  This would normally be called from the
> > + * set_ios() method.
> > + */
> > +int mmc_regulator_set_ocr(struct mmc_host *host, struct regulator *supply)
> > +{
> 
> Why not pass the vdd directly? Saves a few dereferences if nothing else.

This call syntax is simpler, which is usually a win.
Passing a third parameter would create fault paths
of the "pass *wrong* parameter" flavor.

In terms of object code, when I've looked at such things
the dereferences generally cost the same as a ref to a
parameter, but passing an extra parameter isn't free.

- Dave