Message-ID: <20090627002035.GA4379@sirena.org.uk>
Date:	Sat, 27 Jun 2009 01:20:36 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Daniel Ribeiro <drwyrm@...il.com>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	openezx-devel <openezx-devel@...ts.openezx.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Pierre Ossman <pierre@...man.eu>
Subject: Re: [PATCH] PCAP regulator driver (for 2.6.32).

On Fri, Jun 26, 2009 at 07:26:55PM -0300, Daniel Ribeiro wrote:
> Em Sex, 2009-06-26 às 16:08 +0100, Mark Brown escreveu:

> > Fundamentally, if your consumer is trying to explicitly force the
> > regulator off then it's not able to cope with the regulator being
> > shared.  I suspect that if someone does add a non-shared API then the

> The consumer (pxamci.c with the logic implemented on mmc/core.c) is not
> trying to explicitly force the regulator off. It is trying to know if
> itself has previously enabled the regulator.

The only previous use case for MMC was the driver was trying to make
sure power was off at startup.  See below...

> The problem is that regulator_is_enabled returns the regulator
> _hardware_ state, and regulator_enable/regulator_disable are used to
> update the use_count. This is an API inconsistency as the consumer
> should keep an internal use_count and _not_ rely on
> regulator_is_enabled.

It's not really an inconsistency - what you're asking for here is for a
boolean result to give a count back.  Things also get confused here as
soon as reference counting from a single consumer comes into play, which
usually means that different bits of the driver are 

Put another way, it's not regulator_was_enabled_by_me().

> I see no point in exporting regulator_is_enabled() as it is now. There
> is no use in a consumer driver to know if the regulator _hardware_ is
> enabled (as it may be shared).

The use cases for this mostly come around boot time - the consumer may
want to initialise the hardware differently if it's already been
powered.  The simplest case is if something like a backlight driver
tries to avoid changing the hardware state as it starts up.

There are also drivers which really can't tolerate shared use and
absolutely do need exclusive use of the regulator - once you have
exclusive use a consumer can, if it never makes use of reference
counting, do what you suggest.

> So, if the regulator framework has no bugs regarding regulators left on
> by the bootloader, then maybe the buggy code is mmc/core.c?

Not quite; the issue here is that the MMC core assumes exclusive use of
the regulator.  My understanding is that there would be actual breakage
if the regulator weren't enabled and disabled when the MMC core requests
it so this isn't just a case of it being buggy, it needs to be sure that
nothing else is changing the regulator state under it.

Since it requires exclusive use it can use the physical state to keep
track of things and in order to ensure that it's boostrapped correctly
it requires that the MMC driver give it a regulator which is disabled
before it starts.

> Anyway, I don't have more time to spend on this issue, so i will just do
> as you request, remove the workaround from pcap_regulator.c and put it
> on pxamci.c, even if I think that this is the ugliest solution so far.

I have mentioned the idea of adding an exclusive use API for a reason
(more properly it'd be the ability to insist on a particular set of
constraints plus an extra bit for exclusive use).
--
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