Message-ID: <20090223220359.GA3601@sirena.org.uk>
Date:	Mon, 23 Feb 2009 22:04:06 +0000
From:	Mark Brown <broonie@...ena.org.uk>
To:	David Brownell <david-b@...bell.net>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>,
	lkml <linux-kernel@...r.kernel.org>,
	OMAP <linux-omap@...r.kernel.org>
Subject: Re: [patch 2.6.29-rc3-git 1/2] regulator: twl4030 regulators

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.

> > This 
> > would result in something which maintains consistent behaviour over all
> > regulator drivers,

> It's already consistent, even without such patches!!

> There is no driver which pays attention to regulator(_dev)
> constraints that does it any differently than this one.

That's because nobody's doing it at all; once we add a custom
implementation in one driver we then need to police each driver
individually for consistency with the new interface.  If the logic is
factored out then that problem doesn't exist and drivers don't need to
reimplement any of it.

> > > > Your current patch does also set constraints for the voltages if they
> > > > weren't there previously

> > > I was thinking that boards which don't need constraints
> > > shouldn't need to create any ... they could just pass on
> > > the capabilities of the underlying regulator.

> > For safety the regulator API won't do anything without being explicitly
> > told that it's OK to do so - if we were to do this we'd need to have an
> > explicit flag in the constraints which says that this is what to do.
> > Constraints are also permission grants.

> I like to avoid flags unless they're absolutely required.
> In this case my initial reaction is to say that hooking up
> the components in the first place was the permission grant.

The trouble is we don't know what's connected to the regulator without
being told - even if some of the components hanging off the supply are
visible to software that's no guarantee that all of them are.  Keeping
the responsibility in the hands of the machine driver helps ensure that
users have made a concious decision that their settings are OK for their
design.

> > Indeed.  Like I say, a very large proportion of the development of the
> > regulator API has been done on reference designs which are built in this
> > fashion, including both pluggable PMIC boards and other daughtercards.

> That doesn't seem to have escaped its development cage yet.  ;)

There's a couple on their way to mainline right now, should make it in
the next merge window hopefully.  Unfortunately they're for systems that
aren't very completely supported in mainline right now which makes them
less useful as examples than they might be.  There's also none of them
where there's any immediate prospect of more than one of the PMIC board
options going into mainline.

> > Normally the primary PMIC cards are handled with conditional code in the
> > machine file (either compile time or run time) or by registering drivers

> Fair enough.  I'd de-emphasize "conditional", since that sounds
> way too much like #ifdeffing.  The source code should just call
> something like is_pmic_cardX(), and not care how that works.

It's normally a combination of the two - normally you don't get any form
of board ID readback and you get things like mutually exclusive options
for I2C or SPI control due to shared multi function pin setup that can't
be autoprobed entirely safely, for example.  Once you've decided on the
control bus it's normally safe to do autoprobing, though.

> Then what would you call constraints by/from the regulator?

They're subsumed within the constraints supplied by the machine driver
at the minute.

> 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.

> I don't mind moving it ... later, after the regulator
> core has proper support for decoupling machine-specific
> constraints from regulator-specific ones.  I view that
> code as a workaround for a current limitation of that
> core, so like all such workarounds it should vanish
> when it's no longer relevant.

I'd strongly suggest that if you're adding workarounds like this it's
worth at least calling them out in the changelog.  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists