| Message ID | 1308213003-6526-1-git-send-email-myungjoo.ham@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 16, 2011 at 05:30:02PM +0900, MyungJoo Ham wrote: > + adc->vdd = regulator_get(dev, S3C_ADC_REGULATOR_NAME); I'm not convinced that the #define for the name is terribly good style here, especially given that you actually call it vdd in the code. > + if (IS_ERR_OR_NULL(adc->vdd)) { > + dev_dbg(dev, "operating without regulator %s.\n", S3C_ADC_REGULATOR_NAME); > + adc->vdd = NULL; /* Do not control regulator */ > + } > + No, don't do this. Just unconditionally assume the regulator is present if power is essential for use of the device. The regulator API will stub out correctly if it's not in use to allow things to proceed and if vdd is genuinely not hooked up then the driver can't function. > + if (adc->vdd) > + regulator_enable(adc->vdd); You're not checking the return value here or anywhere else after the inital get().
Hello, On Sun, Jun 19, 2011 at 12:06 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Thu, Jun 16, 2011 at 05:30:02PM +0900, MyungJoo Ham wrote: > >> + adc->vdd = regulator_get(dev, S3C_ADC_REGULATOR_NAME); > > I'm not convinced that the #define for the name is terribly good style > here, especially given that you actually call it vdd in the code. Then, would it be fine to use as [ regulator_get(dev, "vdd"); ] ? > >> + if (IS_ERR_OR_NULL(adc->vdd)) { >> + dev_dbg(dev, "operating without regulator %s.\n", S3C_ADC_REGULATOR_NAME); >> + adc->vdd = NULL; /* Do not control regulator */ >> + } >> + > > No, don't do this. Just unconditionally assume the regulator is present > if power is essential for use of the device. The regulator API will > stub out correctly if it's not in use to allow things to proceed and if > vdd is genuinely not hooked up then the driver can't function. This ADC driver is for every ADC from S3C24xx series to Exynos4 (and its successors as well). The regulator (VDD for ADC) is essential for the recent chips (S5PC110, S5PV210, and Exynos4). I was just worried about the old boards using the same ADC driver (mach-s3c2410/mach-*.c, mach-s3c6410/mach-*.c, and so on) without ADC-VDD regulators defined. However, no s3c compliance defconfigs have ever used CONFIG_REGULATOR. Thus, it seems that it's safe to enforce using "vdd" with regulators in plat-samsung's ADC driver. I'll proceed as you have commented. > >> + if (adc->vdd) >> + regulator_enable(adc->vdd); > > You're not checking the return value here or anywhere else after the > inital get(). > Ok. I'll let it handle errors from regulator_enable. Thank you! - MyungJoo.
On Mon, Jun 20, 2011 at 02:16:59PM +0900, MyungJoo Ham wrote: > On Sun, Jun 19, 2011 at 12:06 AM, Mark Brown > > On Thu, Jun 16, 2011 at 05:30:02PM +0900, MyungJoo Ham wrote: > >> + adc->vdd = regulator_get(dev, S3C_ADC_REGULATOR_NAME); > > I'm not convinced that the #define for the name is terribly good style > > here, especially given that you actually call it vdd in the code. > Then, would it be fine to use as [ regulator_get(dev, "vdd"); ] ? Yes. > >> + if (IS_ERR_OR_NULL(adc->vdd)) { > >> + dev_dbg(dev, "operating without regulator %s.\n", S3C_ADC_REGULATOR_NAME); > >> + adc->vdd = NULL; /* Do not control regulator */ > >> + } > >> + > > No, don't do this. Just unconditionally assume the regulator is present > > if power is essential for use of the device. The regulator API will > > stub out correctly if it's not in use to allow things to proceed and if > > vdd is genuinely not hooked up then the driver can't function. > This ADC driver is for every ADC from S3C24xx series to Exynos4 (and > its successors as well). > The regulator (VDD for ADC) is essential for the recent chips > (S5PC110, S5PV210, and Exynos4). > I was just worried about the old boards using the same ADC driver > (mach-s3c2410/mach-*.c, mach-s3c6410/mach-*.c, and so on) without > ADC-VDD regulators defined. If the regulator API is in use on a system it is reasonable to expect it to be set up correctly for the system. > However, no s3c compliance defconfigs have ever used CONFIG_REGULATOR. > Thus, it seems that it's safe to enforce using "vdd" with regulators > in plat-samsung's ADC driver. > I'll proceed as you have commented. Note that SMDK6410 and Cragganmore are both using regulators fairly extensively.
Patch 1/5: Add regulator support in ADC driver. If CONFIG_REGULATOR is enabled, "vdd" regulator for the ADC driver (e.g., "s5p-adc") should exist for the adc driver. Patch 2/5: Channel selection method for S5PC110 and Exynos4 Recent Samsung SoCs have different register addresses for channel selection. Use "s5p-adc" to support such chips. Patch 3/5: Support ADC at Exynos4 Define register addresses and device name for Exynos4 Patch 4/5: Support ADC at S5PC110/S5PV210 Correct ADC device name for S5PC110/S5PV210 Patch 5/5: Header file correction (plat/devs.h) The long-overdue bugfix for compiler errors. ADC for Exynos4 fails to be compiled without this patch. MyungJoo Ham (5): Samsung SoC ADC: use regulator (VDD for ADC). Samsung SoC ADC: Channel selection for S5PV210, S5PC110, and Exynos4 ARM: Exynos4: Support ADC ARM: S5PC110/S5PV210: Support ADC Samsung SoC: header file revised to prevent declaring duplicated. arch/arm/mach-exynos4/Kconfig | 1 + arch/arm/mach-exynos4/cpu.c | 4 ++ arch/arm/mach-exynos4/include/mach/irqs.h | 8 ++++ arch/arm/mach-exynos4/include/mach/map.h | 5 ++ arch/arm/mach-s5pv210/cpu.c | 2 +- arch/arm/plat-samsung/adc.c | 55 +++++++++++++++++++----- arch/arm/plat-samsung/include/plat/devs.h | 5 ++ arch/arm/plat-samsung/include/plat/regs-adc.h | 1 + 8 files changed, 68 insertions(+), 13 deletions(-)
diff --git a/arch/arm/plat-samsung/adc.c b/arch/arm/plat-samsung/adc.c index e8f2be2..a8499d8 100644 --- a/arch/arm/plat-samsung/adc.c +++ b/arch/arm/plat-samsung/adc.c @@ -21,6 +21,7 @@ #include <linux/clk.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/regulator/consumer.h> #include <plat/regs-adc.h> #include <plat/adc.h> @@ -59,6 +60,8 @@ struct s3c_adc_client { unsigned *samples_left); }; +#define S3C_ADC_REGULATOR_NAME "vdd" + struct adc_device { struct platform_device *pdev; struct platform_device *owner; @@ -71,6 +74,7 @@ struct adc_device { unsigned int prescale; int irq; + struct regulator *vdd; }; static struct adc_device *adc_dev; @@ -338,6 +342,12 @@ static int s3c_adc_probe(struct platform_device *pdev) adc->pdev = pdev; adc->prescale = S3C2410_ADCCON_PRSCVL(49); + adc->vdd = regulator_get(dev, S3C_ADC_REGULATOR_NAME); + if (IS_ERR_OR_NULL(adc->vdd)) { + dev_dbg(dev, "operating without regulator %s.\n", S3C_ADC_REGULATOR_NAME); + adc->vdd = NULL; /* Do not control regulator */ + } + adc->irq = platform_get_irq(pdev, 1); if (adc->irq <= 0) { dev_err(dev, "failed to get adc irq\n"); @@ -372,6 +382,8 @@ static int s3c_adc_probe(struct platform_device *pdev) goto err_clk; } + if (adc->vdd) + regulator_enable(adc->vdd); clk_enable(adc->clk); tmp = adc->prescale | S3C2410_ADCCON_PRSCEN; @@ -406,6 +418,8 @@ static int __devexit s3c_adc_remove(struct platform_device *pdev) iounmap(adc->regs); free_irq(adc->irq, adc); clk_disable(adc->clk); + if (adc->vdd) + regulator_disable(adc->vdd); clk_put(adc->clk); kfree(adc); @@ -428,6 +442,8 @@ static int s3c_adc_suspend(struct platform_device *pdev, pm_message_t state) disable_irq(adc->irq); spin_unlock_irqrestore(&adc->lock, flags); clk_disable(adc->clk); + if (adc->vdd) + regulator_disable(adc->vdd); return 0; } @@ -436,6 +452,8 @@ static int s3c_adc_resume(struct platform_device *pdev) { struct adc_device *adc = platform_get_drvdata(pdev); + if (adc->vdd) + regulator_enable(adc->vdd); clk_enable(adc->clk); enable_irq(adc->irq); @@ -485,4 +503,4 @@ static int __init adc_init(void) return ret; } -arch_initcall(adc_init); +module_init(adc_init);
This patch allows the Samsung ADC driver to enable VDD regulator at probe and resume and to disable at exit and suspend. In a platform where ADC's VDD regulator is not "always-on", this control is required although this patch does not provide fine-grained power control (turning on the regulator only when being accessed). However, if VDD regulator ("vdd" for the adc device) is not provided, the regulator control will not be activated because there are platforms that do not provide regulator for ADC device. Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- arch/arm/plat-samsung/adc.c | 20 +++++++++++++++++++- 1 files changed, 19 insertions(+), 1 deletions(-)