Add (partial) support for the voltage regulators on the PCAP2 PMIC.
Signed-off-by: Daniel Ribeiro <[email protected]>
---
drivers/regulator/Kconfig | 7 +
drivers/regulator/Makefile | 1 +
drivers/regulator/pcap-regulator.c | 336 ++++++++++++++++++++++++++++++++++++
3 files changed, 344 insertions(+), 0 deletions(-)
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index f431779..db1cc08 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -117,4 +117,11 @@ config REGULATOR_LP3971
Say Y here to support the voltage regulators and convertors
on National Semiconductors LP3971 PMIC
+config REGULATOR_PCAP
+ tristate "PCAP2 regulator driver"
+ depends on EZX_PCAP
+ help
+ This driver provides support for the voltage regulators of the
+ PCAP2 PMIC.
+
endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 4d762c4..3a9748f 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -16,5 +16,6 @@ obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
+obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o
ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/pcap-regulator.c b/drivers/regulator/pcap-regulator.c
new file mode 100644
index 0000000..13308cd
--- /dev/null
+++ b/drivers/regulator/pcap-regulator.c
@@ -0,0 +1,336 @@
+/*
+ * PCAP2 Regulator Driver
+ *
+ * Copyright (c) 2009 Daniel Ribeiro <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/mfd/ezx-pcap.h>
+
+static const u16 V1_table[] = {
+ 2775, 1275, 1600, 1725, 1825, 1925, 2075, 2275,
+};
+
+static const u16 V2_table[] = {
+ 2500, 2775,
+};
+
+static const u16 V3_table[] = {
+ 1075, 1275, 1550, 1725, 1876, 1950, 2075, 2275,
+};
+
+static const u16 V4_table[] = {
+ 1275, 1550, 1725, 1875, 1950, 2075, 2275, 2775,
+};
+
+static const u16 V5_table[] = {
+ 1875, 2275, 2475, 2775,
+};
+
+static const u16 V6_table[] = {
+ 2475, 2775,
+};
+
+static const u16 V7_table[] = {
+ 1875, 2775,
+};
+
+#define V8_table V4_table
+
+static const u16 V9_table[] = {
+ 1575, 1875, 2475, 2775,
+};
+
+static const u16 V10_table[] = {
+ 5000,
+};
+
+static const u16 VAUX1_table[] = {
+ 1875, 2475, 2775, 3000,
+};
+
+#define VAUX2_table VAUX1_table
+
+static const u16 VAUX3_table[] = {
+ 1200, 1200, 1200, 1200, 1400, 1600, 1800, 2000,
+ 2200, 2400, 2600, 2800, 3000, 3200, 3400, 3600,
+};
+
+static const u16 VAUX4_table[] = {
+ 1800, 1800, 3000, 5000,
+};
+
+static const u16 VSIM_table[] = {
+ 1875, 3000,
+};
+
+static const u16 VSIM2_table[] = {
+ 1875,
+};
+
+static const u16 VVIB_table[] = {
+ 1300, 1800, 2000, 3000,
+};
+
+static const u16 SW1_table[] = {
+ 900, 950, 1000, 1050, 1100, 1150, 1200, 1250,
+ 1300, 1350, 1400, 1450, 1500, 1600, 1875, 2250,
+};
+
+#define SW2_table SW1_table
+
+static const u16 SW3_table[] = {
+ 4000, 4500, 5000, 5500,
+};
+
+struct pcap_regulator {
+ const u8 reg;
+ const u8 en;
+ const u8 index;
+ const u8 stby;
+ const u8 lowpwr;
+ const u8 n_voltages;
+ const u16 *voltage_table;
+};
+
+#define NA 0xff
+
+#define VREG_INFO(_vreg, _reg, _en, _index, _stby, _lowpwr) \
+ [_vreg] = { \
+ .reg = _reg, \
+ .en = _en, \
+ .index = _index, \
+ .stby = _stby, \
+ .lowpwr = _lowpwr, \
+ .n_voltages = ARRAY_SIZE(_vreg##_table), \
+ .voltage_table = _vreg##_table, \
+ }
+
+static struct pcap_regulator vreg_table[] = {
+ VREG_INFO(V1, PCAP_REG_VREG1, 1, 2, 18, 0),
+ VREG_INFO(V2, PCAP_REG_VREG1, 5, 6, 19, 22),
+ VREG_INFO(V3, PCAP_REG_VREG1, 7, 8, 20, 23),
+ VREG_INFO(V4, PCAP_REG_VREG1, 11, 12, 21, 24),
+ /* V5 STBY and LOWPWR are on PCAP_REG_VREG2 */
+ VREG_INFO(V5, PCAP_REG_VREG1, 15, 16, 12, 19),
+
+ VREG_INFO(V6, PCAP_REG_VREG2, 1, 2, 14, 20),
+ VREG_INFO(V7, PCAP_REG_VREG2, 3, 4, 15, 21),
+ VREG_INFO(V8, PCAP_REG_VREG2, 5, 6, 16, 22),
+ VREG_INFO(V9, PCAP_REG_VREG2, 9, 10, 17, 23),
+ VREG_INFO(V10, PCAP_REG_VREG2, 10, NA, 18, 24),
+
+ VREG_INFO(VAUX1, PCAP_REG_AUXVREG, 1, 2, 22, 23),
+ /* VAUX2 ... VSIM2 STBY and LOWPWR are on PCAP_REG_LOWPWR */
+ VREG_INFO(VAUX2, PCAP_REG_AUXVREG, 4, 5, 0, 1),
+ VREG_INFO(VAUX3, PCAP_REG_AUXVREG, 7, 8, 2, 3),
+ VREG_INFO(VAUX4, PCAP_REG_AUXVREG, 12, 13, 4, 5),
+ VREG_INFO(VSIM, PCAP_REG_AUXVREG, 17, 18, NA, 6),
+ VREG_INFO(VSIM2, PCAP_REG_AUXVREG, 16, NA, NA, 7),
+ VREG_INFO(VVIB, PCAP_REG_AUXVREG, 19, 20, NA, NA),
+
+ VREG_INFO(SW1, PCAP_REG_SWCTRL, 1, 2, NA, NA),
+ VREG_INFO(SW2, PCAP_REG_SWCTRL, 6, 7, NA, NA),
+ /* SW3 STBY is on PCAP_REG_AUXVREG */
+ VREG_INFO(SW3, PCAP_REG_SWCTRL, 11, 12, 24, NA),
+
+ /* SWxS used to control SWx voltage on standby */
+/* VREG_INFO(SW1S, PCAP_REG_LOWPWR, NA, 12, NA, NA),
+ VREG_INFO(SW2S, PCAP_REG_LOWPWR, NA, 20, NA, NA), */
+};
+
+static int pcap_regulator_set_voltage(struct regulator_dev *rdev,
+ int min_uV, int max_uV)
+{
+ struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)];
+ void *pcap = rdev_get_drvdata(rdev);
+ int uV;
+ u32 tmp;
+ u8 i;
+
+ if (vreg->n_voltages == 1)
+ return -EINVAL;
+
+ for (i = 0; i < vreg->n_voltages; i++) {
+ /* For V1 the first is not the best match */
+ if (i == 0 && rdev_get_id(rdev) == V1)
+ i = 1;
+ else if (i + 1 == vreg->n_voltages && rdev_get_id(rdev) == V1)
+ i = 0;
+
+ uV = vreg->voltage_table[i] * 1000;
+ if (min_uV <= uV && uV <= max_uV) {
+ ezx_pcap_read(pcap, vreg->reg, &tmp);
+ tmp &= ~((vreg->n_voltages - 1) << vreg->index);
+ tmp |= i << vreg->index;
+ ezx_pcap_write(pcap, vreg->reg, tmp);
+ return 0;
+ }
+ if (i == 0 && rdev_get_id(rdev) == V1)
+ i = vreg->n_voltages - 1;
+ }
+
+ return -EINVAL;
+}
+
+static int pcap_regulator_get_voltage(struct regulator_dev *rdev)
+{
+ struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)];
+ void *pcap = rdev_get_drvdata(rdev);
+ u32 tmp;
+ int mV;
+
+ if (vreg->n_voltages == 1)
+ return vreg->voltage_table[0] * 1000;
+
+ ezx_pcap_read(pcap, vreg->reg, &tmp);
+ tmp = ((tmp >> vreg->index) & (vreg->n_voltages - 1));
+ mV = vreg->voltage_table[tmp];
+
+ return mV * 1000;
+}
+
+static int pcap_regulator_enable(struct regulator_dev *rdev)
+{
+ struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)];
+ void *pcap = rdev_get_drvdata(rdev);
+ u32 tmp;
+
+ if (vreg->en == NA)
+ return -EINVAL;
+
+ ezx_pcap_read(pcap, vreg->reg, &tmp);
+ tmp |= (1 << vreg->en);
+ ezx_pcap_write(pcap, vreg->reg, tmp);
+
+ return 0;
+}
+
+static int pcap_regulator_disable(struct regulator_dev *rdev)
+{
+ struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)];
+ void *pcap = rdev_get_drvdata(rdev);
+ u32 tmp;
+
+ if (vreg->en == NA)
+ return -EINVAL;
+
+ ezx_pcap_read(pcap, vreg->reg, &tmp);
+ tmp &= ~(1 << vreg->en);
+ ezx_pcap_write(pcap, vreg->reg, tmp);
+
+ return 0;
+}
+
+static int pcap_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)];
+ void *pcap = rdev_get_drvdata(rdev);
+ u32 tmp;
+
+ if (vreg->en == NA)
+ return -EINVAL;
+
+ ezx_pcap_read(pcap, vreg->reg, &tmp);
+ return (tmp >> vreg->en) & 1;
+}
+
+static int pcap_regulator_list_voltage(struct regulator_dev *rdev,
+ unsigned int index)
+{
+ struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)];
+
+ return vreg->voltage_table[index] * 1000;
+}
+
+static struct regulator_ops pcap_regulator_ops = {
+ .list_voltage = pcap_regulator_list_voltage,
+ .set_voltage = pcap_regulator_set_voltage,
+ .get_voltage = pcap_regulator_get_voltage,
+ .enable = pcap_regulator_enable,
+ .disable = pcap_regulator_disable,
+ .is_enabled = pcap_regulator_is_enabled,
+};
+
+#define VREG(_vreg) \
+ [_vreg] = { \
+ .name = #_vreg, \
+ .id = _vreg, \
+ .n_voltages = ARRAY_SIZE(_vreg##_table), \
+ .ops = &pcap_regulator_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ }
+
+static struct regulator_desc pcap_regulators[] = {
+ VREG(V1), VREG(V2), VREG(V3), VREG(V4), VREG(V5), VREG(V6), VREG(V7),
+ VREG(V8), VREG(V9), VREG(V10), VREG(VAUX1), VREG(VAUX2), VREG(VAUX3),
+ VREG(VAUX4), VREG(VSIM), VREG(VSIM2), VREG(VVIB), VREG(SW1), VREG(SW2),
+};
+
+static int __devinit pcap_regulator_probe(struct platform_device *pdev)
+{
+ struct regulator_dev *rdev;
+ void *pcap = platform_get_drvdata(pdev);
+
+ rdev = regulator_register(&pcap_regulators[pdev->id], &pdev->dev,
+ pdev->dev.platform_data, pcap);
+ if (IS_ERR(rdev))
+ return PTR_ERR(rdev);
+
+ platform_set_drvdata(pdev, rdev);
+
+ /*
+ * The regulator framework doesn't like regulators which default
+ * to ON at boot time, so we just disable it here (when it is safe).
+ */
+ if (pdev->id == VAUX2 || pdev->id == VAUX3)
+ pcap_regulator_disable(rdev);
+
+ return 0;
+}
+
+static int __devexit pcap_regulator_remove(struct platform_device *pdev)
+{
+ struct regulator_dev *rdev = platform_get_drvdata(pdev);
+
+ regulator_unregister(rdev);
+
+ return 0;
+}
+
+static struct platform_driver pcap_regulator_driver = {
+ .driver = {
+ .name = "pcap-regulator",
+ },
+ .probe = pcap_regulator_probe,
+ .remove = __devexit_p(pcap_regulator_remove),
+};
+
+static int __init pcap_regulator_init(void)
+{
+ return platform_driver_register(&pcap_regulator_driver);
+}
+
+static void __exit pcap_regulator_exit(void)
+{
+ platform_driver_unregister(&pcap_regulator_driver);
+}
+
+module_init(pcap_regulator_init);
+module_exit(pcap_regulator_exit);
+
+MODULE_AUTHOR("Daniel Ribeiro <[email protected]>");
+MODULE_DESCRIPTION("PCAP2 Regulator Driver");
+MODULE_LICENSE("GPL");
--
tg: (69e76aa..) ezx/pcap_regulator (depends on: ezx/local/pcap)
--
Daniel Ribeiro
On Thu, Jun 25, 2009 at 05:29:53PM -0300, Daniel Ribeiro wrote:
> + /*
> + * The regulator framework doesn't like regulators which default
> + * to ON at boot time, so we just disable it here (when it is safe).
> + */
> + if (pdev->id == VAUX2 || pdev->id == VAUX3)
> + pcap_regulator_disable(rdev);
No need to do this - the regulator framework is perfectly happy with
regulators that are enabled at boot time and turning regulators that are
actively being used off is likely to cause issues. Regulator drivers
should just leave everything as they find it and leave it up to the core
and machine drivers to make any changes.
Other than that this looks good; I'm assuming the PCAP core has a
sensible way of getting the platform data to the devices.
Em Sex, 2009-06-26 às 00:37 +0100, Mark Brown escreveu:
> On Thu, Jun 25, 2009 at 05:29:53PM -0300, Daniel Ribeiro wrote:
> > + /*
> > + * The regulator framework doesn't like regulators which default
> > + * to ON at boot time, so we just disable it here (when it is safe).
> > + */
> > + if (pdev->id == VAUX2 || pdev->id == VAUX3)
> > + pcap_regulator_disable(rdev);
>
> No need to do this - the regulator framework is perfectly happy with
> regulators that are enabled at boot time and turning regulators that are
> actively being used off is likely to cause issues. Regulator drivers
> should just leave everything as they find it and leave it up to the core
> and machine drivers to make any changes.
Humm, I still see:
if (WARN(rdev->use_count <= 0,
"unbalanced disables for %s\n",
rdev->desc->name))
return -EIO;
So, the regulator is enabled at boot, but it can't be disabled because
use_count is 0. This is the same issue as twl4030-mmc, but instead of a
enable/disable pair on pxamci.c i opted to disable it at pcap's
regulator probe(). VAUX2 and VAUX3 are only used for MMC on all the
hardware that I know of, so it is safe.
Also, on regulator_init_complete() the regulator core disables all
regulators which have a use_count == 0. So, its kind of funny that if I
don't disable the regulator at boot(so mmc_core does enable() it), the
timing causes regulator_init_complete() to auto-disable the regulator
right when pxamci.c is probing the card.
I can move this hack to pxamci.c if you want me to (as David did for
twl4030), but this issue really should be fixed on regulator/core.c:
With the current code if you use a regulator which can be disabled as
the supplier for the CPU core, then the regulator framework will power
off the CPU at boot.
Maybe you should simply allow disable() if use_count is 0, and not
auto-disable() on regulator_init_complete() ?
--
Daniel Ribeiro
On Fri, Jun 26, 2009 at 03:04:23AM -0300, Daniel Ribeiro wrote:
> Em Sex, 2009-06-26 ?s 00:37 +0100, Mark Brown escreveu:
> > actively being used off is likely to cause issues. Regulator drivers
> > should just leave everything as they find it and leave it up to the core
> > and machine drivers to make any changes.
> So, the regulator is enabled at boot, but it can't be disabled because
> use_count is 0. This is the same issue as twl4030-mmc, but instead of a
> enable/disable pair on pxamci.c i opted to disable it at pcap's
At the minute you need to use the enable/disable pair since (as we've
previously discussed) the API needs to support regulators which are
shared between multiple users (potentially including multiple users from
the same consumer).
> regulator probe(). VAUX2 and VAUX3 are only used for MMC on all the
> hardware that I know of, so it is safe.
I've heard that one before :)
> I can move this hack to pxamci.c if you want me to (as David did for
> twl4030), but this issue really should be fixed on regulator/core.c:
You need to either do that or add an API allowing consumers to claim a
regulator for exclusive use. If the regulator is claimed for exclusive
use then other consumers wouldn't be able to access it and there would
be no issue with interfering with other users.
> With the current code if you use a regulator which can be disabled as
> the supplier for the CPU core, then the regulator framework will power
> off the CPU at boot.
This will only be done if the board has said that it has fully specified
the regulator configuration - if the board hasn't done that then the
state of the regulators will stay unchanged. The regulator API is very
dependant on boards supplying good constraints, if bad constraints are
provided the consequences could include even more serious things like
lasting hardware damage. This is just an example of what can go wrong
with a bad board setup.
> Maybe you should simply allow disable() if use_count is 0, and not
> auto-disable() on regulator_init_complete() ?
That doesn't fulfil the same need since it requires that there be a
consumer for the regulator to actively sit there and disable it. It's
not intended to help if a consumer actively needs to have the regulator
disabled right now, it's there to disable the regulator if nothing wants
to have it on which isn't quite the same thing.
Em Sex, 2009-06-26 às 11:55 +0100, Mark Brown escreveu:
> On Fri, Jun 26, 2009 at 03:04:23AM -0300, Daniel Ribeiro wrote:
> > So, the regulator is enabled at boot, but it can't be disabled because
> > use_count is 0. This is the same issue as twl4030-mmc, but instead of a
> > enable/disable pair on pxamci.c i opted to disable it at pcap's
>
> At the minute you need to use the enable/disable pair since (as we've
> previously discussed) the API needs to support regulators which are
> shared between multiple users (potentially including multiple users from
> the same consumer).
> You need to either do that or add an API allowing consumers to claim a
> regulator for exclusive use. If the regulator is claimed for exclusive
> use then other consumers wouldn't be able to access it and there would
> be no issue with interfering with other users.
I'm not proposing an API for exclusive use. Allowing the enable bit to
be turn off case use_count is 0 shouldn't break regulator sharing for
other consumers, as far as the regulator framework is concerned there
are no other consumers.
What about increasing use_count at regulator_get() if the regulator is
already on and use_count == 0? If a consumer requests a regulator that
was previously ON, then there is no reason for it to enable/disable it.
If it is requested, and its already ON, then the regulator framework can
assume it is already being used.
If the above is not possible, then regulator_is_enabled() doesn't match
regulator_enable()/regulator_disable() pair. Maybe the API should make
this clear?
--
Daniel Ribeiro
On Fri, Jun 26, 2009 at 09:26:32AM -0300, Daniel Ribeiro wrote:
> Em Sex, 2009-06-26 ?s 11:55 +0100, Mark Brown escreveu:
> > You need to either do that or add an API allowing consumers to claim a
> > regulator for exclusive use. If the regulator is claimed for exclusive
> > use then other consumers wouldn't be able to access it and there would
> > be no issue with interfering with other users.
> I'm not proposing an API for exclusive use. Allowing the enable bit to
> be turn off case use_count is 0 shouldn't break regulator sharing for
> other consumers, as far as the regulator framework is concerned there
> are no other consumers.
What breaks things for non-exclusive use is that the driver is doing a
disable when it didn't do the corresponding enable. This means that if
whatever did the enable still cares about the device being enabled then
it will get upset. The reason the regulator API complains here is to
help catch problems in drivers rather than because it itself will fall
over.
> What about increasing use_count at regulator_get() if the regulator is
> already on and use_count == 0? If a consumer requests a regulator that
> was previously ON, then there is no reason for it to enable/disable it.
> If it is requested, and its already ON, then the regulator framework can
> assume it is already being used.
Think about the shared use case here. If the regulator is enabled when
a consumer starts then the consumer can't tell if this was because the
regulator happened to be left on at system startup or if it was because
some other user of the regulator has enabled it. This means that none
of the consumers can ever drop that use count so it's stuck there and
the regulator can never actually be disabled.
> If the above is not possible, then regulator_is_enabled() doesn't match
> regulator_enable()/regulator_disable() pair. Maybe the API should make
> this clear?
Frankly I'm not sure how much any documentation is going to help here.
There's already a note about the fact that the regulator might've been
enabled elsewhere, it could be strengthened a bit but it still relies on
people reading it.
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
problem will go away, half the problem is with consumers thinking they
have exclusive use of the regulator.
Em Sex, 2009-06-26 às 16:08 +0100, Mark Brown escreveu:
> > If the above is not possible, then regulator_is_enabled() doesn't match
> > regulator_enable()/regulator_disable() pair. Maybe the API should make
> > this clear?
>
> Frankly I'm not sure how much any documentation is going to help here.
> There's already a note about the fact that the regulator might've been
> enabled elsewhere, it could be strengthened a bit but it still relies on
> people reading it.
I wasn't talking about documentation.
> 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
> problem will go away, half the problem is with consumers thinking they
> have exclusive use of the regulator.
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 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.
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).
So, if the regulator framework has no bugs regarding regulators left on
by the bootloader, then maybe the buggy code is mmc/core.c?
int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
{
...
int enabled;
enabled = regulator_is_enabled(supply);
...
if (vdd_bit) {
...
if (result == 0 && !enabled)
result = regulator_enable(supply);
} else if (enabled) {
result = regulator_disable(supply);
}
return result;
}
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.
--
Daniel Ribeiro
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).