Re: [PATCHv2 2/5] regulator: omap smps regulator driver

From: Tero Kristo <t-kristo@ti.com>
To: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: linux-omap@vger.kernel.org, "Girdwood, Liam" <lrg@ti.com>,
	"Hilman, Kevin" <khilman@ti.com>
Subject: Re: [PATCHv2 2/5] regulator: omap smps regulator driver
Date: Wed, 13 Jul 2011 18:53:45 +0300	[thread overview]
Message-ID: <1310572425.4331.96.camel@sokoban> (raw)
In-Reply-To: <20110713144019.GA7861@opensource.wolfsonmicro.com>

On Wed, 2011-07-13 at 16:40 +0200, Mark Brown wrote:
> On Wed, Jul 13, 2011 at 05:00:35PM +0300, Tero Kristo wrote:
> 
> > +config REGULATOR_OMAP_SMPS
> > +	tristate "TI OMAP SMPS Power Regulators"
> > +	depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM && TWL4030_CORE
> 
> What is the dependency on the TWL4030?  I see no references to it in the
> code...

Hmm, true... I was mainly thinking about the HW setup where we usually
have a TWL family chip which is controlled by the SMPS regulator driver.
I think that one can actually be dropped as it might be possible to use
some other power IC behind the SMPS channels. I think I'll remove all
the references to TWL4030 / TWL6030 from this patch.

> 
> > +	for (i = 0; i < pdata->num_regulators; i++) {
> > +		initdata = pdata->regulators[i];
> > +
> 
> I do strongly prefer the idiom of just registering all the regulators
> even if they're read only.

Number of available SMPS regulators is kind of board specific issue.
OMAP3 has 2 available, OMAP4 has 3. If we are using some custom powering
solution, we might have even different amounts for these.

> 
> > +		c = &initdata->constraints;
> > +		c->valid_modes_mask &= REGULATOR_MODE_NORMAL;
> > +		c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE;
> > +		c->always_on = true;
> 
> No, this is bad.  We *always* pay attention to the constraints the user
> set even if they're nuts or won't work, the machine driver has the final
> say on what is or isn't allowed on a given board.  The mode setting is
> especially suspect as there's no mode support in the driver.

Just a clarification on this one that I have understood your comment
right... Do you mean that I should be checking the constraints user sets
more thoroughly to see if there is something bogus? I was looking at
some of the other regulator drivers and they seem to be fiddling with
the constraints in similar manner.

-Tero

Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki