Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

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

On Monday 23 February 2009, Mark Brown wrote:
> On Mon, Feb 23, 2009 at 12:45:44PM -0800, David Brownell wrote:
> 
> > > another with a TWL4030 driver using that API 
> > > and a third patch making the core do something with that data.
> 
> > Best IMO to switch the last two around.  Effectively
> > there'd be one patch "add new features to regulator
> > core", followed by the first of a set of "implement
> > those new features in the driver for regulator X".
> 
> > And in fact that's what I've done with the two patches
> > I'll be sending in a moment.
> 
> The reason I'm suggesting splitting things up the way I am is that it
> separates out the TWL4030 driver (which looks very mergable to me right
> now) from the behaviour changes.  Ordering things that way makes it
> clear what the dependency is.  Another way of splitting it out would be
> to remove the new API from the TWL4030, make that the first patch, then
> have further patches adding the new API and the TWL4030 code to use it.
> 
> I don't see any reason why the TWL4030 regulator support needs to be
> blocked on adding the new API and it makes review easier to keep them
> separate.

I think we're talking past each other.  I agree the twl4030 driver
is very mergeable right now; that's why it was submitted.  You could
do so, and then apply the two patches on top ... very clear what the
dependency is, and as I understand it the result would be more or less
to your liking.

My comment was more along the lines of "avoid adding unused hooks,
just merge the create-hook and use-hook patches".  Having "create"
separate from "use" is often troublesome.


> > Then what would you call constraints by/from the regulator?
> 
> They're subsumed within the constraints supplied by the machine driver
> at the minute.

That is, they are not named.  :)


> > I suggest updating your terminology.  "machine constraints"
> > would be much more clear for what's there now:  they relate
> > to the machine.  Other constraints (regulator, consumer)
> > relate to the other components ... the ones for which they
> > are an adjective.
> 
> Yeah, I kind of agree.  To avoid confusion from changing the names I'd
> be tempted to go for something like "regulator driver constraints" but
> it's not desparately nice.

Hence my suggestion:  {regulator,machine,consumer} constraints,
going from bottom up.  They aren't driver constraints; they
are hardware constraints:  regulators can't supply arbitrary
voltages.


> 	 To be honest I'm not
> 100% clear why this new feature is essential to supporting the TWL4030 -
> I can see that it could be useful but I'm not clear on what makes it
> essential for this driver.

I never said it was "essential".  However it does simplify
the core driver code a lot by moving a lot of error checks
to the init code; the checks need to live someplace.  You're
the one asking for them to be packaged as a new framework
feature.

- Dave