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).
Re: [PATCH] PCAP regulator driver (for 2.6.32).
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 signature.asc Description: Esta é uma parte de mensagem assinada digitalmente
Re: [PATCH] PCAP regulator driver (for 2.6.32).
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.
Re: [PATCH] PCAP regulator driver (for 2.6.32).
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 signature.asc Description: Esta é uma parte de mensagem assinada digitalmente
Re: [PATCH] PCAP regulator driver (for 2.6.32).
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.
Re: [PATCH] PCAP regulator driver (for 2.6.32).
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 signature.asc Description: Esta é uma parte de mensagem assinada digitalmente
Re: [PATCH] PCAP regulator driver (for 2.6.32).
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.
[PATCH] PCAP regulator driver (for 2.6.32).
Add (partial) support for the voltage regulators on the PCAP2 PMIC. Signed-off-by: Daniel Ribeiro --- 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 000..13308cd --- /dev/null +++ b/drivers/regulator/pcap-regulator.c @@ -0,0 +1,336 @@ +/* + * PCAP2 Regulator Driver + * + * Copyright (c) 2009 Daniel Ribeiro + * + * 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 +#include +#include +#include +#include +#include +#include +#include + +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, 1