[PATCH 0/6] [CPUFREQ] S5PV210: Update CPUFREQ

[PATCH 0/6] [CPUFREQ] S5PV210: Update CPUFREQ

[Kukjin Kim]

This patch updates CPUFREQ driver on S5VP210 boards.

[PATCH 1/6] [CPUFREQ] S5PV210: Add additional symantics for "relation" in cpufreq with pm
[PATCH 2/6] [CPUFREQ] S5PV210: Add arm/int voltage control support
[PATCH 3/6] [CPUFREQ] S5PV210: Add pm_notifier to prevent system unstable
[PATCH 4/6] [CPUFREQ] S5PV210: Lock a mutex while changing the cpu frequency
[PATCH 5/6] [CPUFREQ] S5PV210: Adjust udelay prior to voltage scaling down
[PATCH 6/6] [CPUFREQ] S5PV210: Add reboot notifier to prevent system hang

[PATCH 1/6] S5PV210: Add additional symantics for "relation" in cpufreq with pm

[Kukjin Kim]

From: Huisung Kang <hs1218.kang@samsung.com>

Relation has an additional symantics other than standard.
s5pv310_target funtion have below additional relation.
- DISABLE_FURTHER_CPUFREQ : disable further access to target
- ENABLE_FURTHER_CPUFRER : enable access to target

Signed-off-by: Huisung Kang <hs1218.kang@samsung.com>
Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/cpufreq/s5pv210-cpufreq.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index a7cb338..48a4a90 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -30,6 +30,18 @@ static struct cpufreq_freqs freqs;
 #define APLL_VAL_800	((1 << 31) | (100 << 16) | (3 << 8) | 1)
 
 /*
+ * relation has an additional symantics other than the standard of cpufreq
+ * DISALBE_FURTHER_CPUFREQ: disable further access to target
+ * ENABLE_FURTUER_CPUFREQ: enable access to target
+ */
+enum cpufreq_access {
+	DISABLE_FURTHER_CPUFREQ = 0x10,
+	ENABLE_FURTHER_CPUFREQ = 0x20,
+};
+
+static bool no_cpufreq_access;
+
+/*
  * DRAM configurations to calculate refresh counter for changing
  * frequency of memory.
  */
@@ -146,6 +158,22 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 	unsigned int pll_changing = 0;
 	unsigned int bus_speed_changing = 0;
 
+	if (relation & ENABLE_FURTHER_CPUFREQ)
+		no_cpufreq_access = false;
+
+	if (no_cpufreq_access) {
+#ifdef CONFIG_PM_VERBOSE
+		pr_err("%s:%d denied access to %s as it is disabled"
+				"temporarily\n", __FILE__, __LINE__, __func__);
+#endif
+		return -EINVAL;
+	}
+
+	if (relation & DISABLE_FURTHER_CPUFREQ)
+		no_cpufreq_access = true;
+
+	relation &= ~(ENABLE_FURTHER_CPUFREQ | DISABLE_FURTHER_CPUFREQ);
+
 	freqs.old = s5pv210_getspeed(0);
 
 	if (cpufreq_frequency_table_target(policy, s5pv210_freq_table,
-- 
1.7.1

[PATCH 2/6] [CPUFREQ] S5PV210: Add arm/int voltage control support

[Kukjin Kim]

From: Jonghwan Choi <jhbird.choi@samsung.com>

Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/cpufreq/s5pv210-cpufreq.c |   75 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index 48a4a90..d2b3d25 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -16,6 +16,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/cpufreq.h>
+#include <linux/regulator/consumer.h>
 
 #include <mach/map.h>
 #include <mach/regs-clock.h>
@@ -77,6 +78,40 @@ static struct cpufreq_frequency_table s5pv210_freq_table[] = {
 	{0, CPUFREQ_TABLE_END},
 };
 
+static struct regulator *arm_regulator;
+static struct regulator *int_regulator;
+
+struct s5pv210_dvs_conf {
+	int arm_volt;	/* uV */
+	int int_volt;	/* uV */
+};
+
+static const int arm_volt_max = 1350000;
+static const int int_volt_max = 1250000;
+
+static struct s5pv210_dvs_conf dvs_conf[] = {
+	[L0] = {
+		.arm_volt	= 1250000,
+		.int_volt	= 1100000,
+	},
+	[L1] = {
+		.arm_volt	= 1200000,
+		.int_volt	= 1100000,
+	},
+	[L2] = {
+		.arm_volt	= 1050000,
+		.int_volt	= 1100000,
+	},
+	[L3] = {
+		.arm_volt	= 950000,
+		.int_volt	= 1100000,
+	},
+	[L4] = {
+		.arm_volt	= 950000,
+		.int_volt	= 1000000,
+	},
+};
+
 static u32 clkdiv_val[5][11] = {
 	/*
 	 * Clock divider value for following
@@ -157,6 +192,8 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 	unsigned int index, priv_index;
 	unsigned int pll_changing = 0;
 	unsigned int bus_speed_changing = 0;
+	int arm_volt, int_volt;
+	int ret = 0;
 
 	if (relation & ENABLE_FURTHER_CPUFREQ)
 		no_cpufreq_access = false;
@@ -191,12 +228,27 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 					   freqs.old, relation, &priv_index))
 		return -EINVAL;
 
-	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	arm_volt = dvs_conf[index].arm_volt;
+	int_volt = dvs_conf[index].int_volt;
 
 	if (freqs.new > freqs.old) {
 		/* Voltage up: will be implemented */
+		if (!IS_ERR(arm_regulator) &&
+				!IS_ERR(int_regulator)) {
+			ret = regulator_set_voltage(arm_regulator,
+					arm_volt, arm_volt_max);
+			if (ret)
+				return ret;
+
+			ret = regulator_set_voltage(int_regulator,
+					int_volt, int_volt_max);
+			if (ret)
+				return ret;
+		}
 	}
 
+	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
 	/* Check if there need to change PLL */
 	if ((index == L0) || (priv_index == L0))
 		pll_changing = 1;
@@ -409,6 +461,14 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 
 	if (freqs.new < freqs.old) {
 		/* Voltage down: will be implemented */
+		if (!IS_ERR(arm_regulator) &&
+				!IS_ERR(int_regulator)) {
+			regulator_set_voltage(int_regulator,
+					int_volt, int_volt_max);
+
+			regulator_set_voltage(arm_regulator,
+					arm_volt, arm_volt_max);
+		}
 	}
 
 	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
@@ -515,6 +575,19 @@ static struct cpufreq_driver s5pv210_driver = {
 
 static int __init s5pv210_cpufreq_init(void)
 {
+	arm_regulator = regulator_get(NULL, "vddarm");
+	if (IS_ERR(arm_regulator)) {
+		pr_err("failed to get regulator vddarm");
+		return PTR_ERR(arm_regulator);
+	}
+
+	int_regulator = regulator_get(NULL, "vddint");
+	if (IS_ERR(int_regulator)) {
+		pr_err("failed to get regulator vddint");
+		regulator_put(arm_regulator);
+		return PTR_ERR(int_regulator);
+	}
+
 	return cpufreq_register_driver(&s5pv210_driver);
 }
 
-- 
1.7.1

[PATCH 3/6] [CPUFREQ] S5PV210: Add pm_notifier to prevent system unstable

[Kukjin Kim]

From: Huisung Kang <hs1218.kang@samsung.com>

Minimum 800MHz is needed to enter/exit suspend mode due to voltage mismatch.

Signed-off-by: Huisung Kang <hs1218.kang@samsung.com>
Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/cpufreq/s5pv210-cpufreq.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index d2b3d25..ba7f2dc 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/cpufreq.h>
 #include <linux/regulator/consumer.h>
+#include <linux/suspend.h>
 
 #include <mach/map.h>
 #include <mach/regs-clock.h>
@@ -30,6 +31,9 @@ static struct cpufreq_freqs freqs;
 #define APLL_VAL_1000	((1 << 31) | (125 << 16) | (3 << 8) | 1)
 #define APLL_VAL_800	((1 << 31) | (100 << 16) | (3 << 8) | 1)
 
+/* Use 800MHz when entering sleep mode */
+#define SLEEP_FREQ	(800 * 1000)
+
 /*
  * relation has an additional symantics other than the standard of cpufreq
  * DISALBE_FURTHER_CPUFREQ: disable further access to target
@@ -560,6 +564,30 @@ out_dmc0:
 	return ret;
 }
 
+static int s5pv210_cpufreq_notifier_event(struct notifier_block *this,
+					  unsigned long event, void *ptr)
+{
+	int ret;
+
+	switch (event) {
+	case PM_SUSPEND_PREPARE:
+		ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ,
+					    DISABLE_FURTHER_CPUFREQ);
+		if (ret < 0)
+			return NOTIFY_BAD;
+
+		return NOTIFY_OK;
+	case PM_POST_RESTORE:
+	case PM_POST_SUSPEND:
+		cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ,
+				      ENABLE_FURTHER_CPUFREQ);
+
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static struct cpufreq_driver s5pv210_driver = {
 	.flags		= CPUFREQ_STICKY,
 	.verify		= s5pv210_verify_speed,
@@ -573,6 +601,10 @@ static struct cpufreq_driver s5pv210_driver = {
 #endif
 };
 
+static struct notifier_block s5pv210_cpufreq_notifier = {
+	.notifier_call = s5pv210_cpufreq_notifier_event,
+};
+
 static int __init s5pv210_cpufreq_init(void)
 {
 	arm_regulator = regulator_get(NULL, "vddarm");
@@ -588,6 +620,8 @@ static int __init s5pv210_cpufreq_init(void)
 		return PTR_ERR(int_regulator);
 	}
 
+	register_pm_notifier(&s5pv210_cpufreq_notifier);
+
 	return cpufreq_register_driver(&s5pv210_driver);
 }
 
-- 
1.7.1

[PATCH 4/6] S5PV210: Lock a mutex while changing the cpu frequency

[Kukjin Kim]

From: Arve Hjønnevåg <arve@android.com>

Without this lock the call to change the frequency for suspend could
switch to a new frequency while another thread was still changing the
cpu voltage.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/cpufreq/s5pv210-cpufreq.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index ba7f2dc..e9b9b7f 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -26,6 +26,7 @@ static struct clk *cpu_clk;
 static struct clk *dmc0_clk;
 static struct clk *dmc1_clk;
 static struct cpufreq_freqs freqs;
+static DEFINE_MUTEX(set_freq_lock);
 
 /* APLL M,P,S values for 1G/800Mhz */
 #define APLL_VAL_1000	((1 << 31) | (125 << 16) | (3 << 8) | 1)
@@ -199,6 +200,8 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 	int arm_volt, int_volt;
 	int ret = 0;
 
+	mutex_lock(&set_freq_lock);
+
 	if (relation & ENABLE_FURTHER_CPUFREQ)
 		no_cpufreq_access = false;
 
@@ -207,7 +210,8 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 		pr_err("%s:%d denied access to %s as it is disabled"
 				"temporarily\n", __FILE__, __LINE__, __func__);
 #endif
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	if (relation & DISABLE_FURTHER_CPUFREQ)
@@ -218,19 +222,23 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 	freqs.old = s5pv210_getspeed(0);
 
 	if (cpufreq_frequency_table_target(policy, s5pv210_freq_table,
-					   target_freq, relation, &index))
-		return -EINVAL;
+					   target_freq, relation, &index)) {
+		ret = -EINVAL;
+		goto exit;
+	}
 
 	freqs.new = s5pv210_freq_table[index].frequency;
 	freqs.cpu = 0;
 
 	if (freqs.new == freqs.old)
-		return 0;
+		goto exit;
 
 	/* Finding current running level index */
 	if (cpufreq_frequency_table_target(policy, s5pv210_freq_table,
-					   freqs.old, relation, &priv_index))
-		return -EINVAL;
+					   freqs.old, relation, &priv_index)) {
+		ret = -EINVAL;
+		goto exit;
+	}
 
 	arm_volt = dvs_conf[index].arm_volt;
 	int_volt = dvs_conf[index].int_volt;
@@ -242,12 +250,12 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 			ret = regulator_set_voltage(arm_regulator,
 					arm_volt, arm_volt_max);
 			if (ret)
-				return ret;
+				goto exit;
 
 			ret = regulator_set_voltage(int_regulator,
 					int_volt, int_volt_max);
 			if (ret)
-				return ret;
+				goto exit;
 		}
 	}
 
@@ -479,7 +487,9 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 
 	printk(KERN_DEBUG "Perf changed[L%d]\n", index);
 
-	return 0;
+exit:
+	mutex_unlock(&set_freq_lock);
+	return ret;
 }
 
 #ifdef CONFIG_PM
-- 
1.7.1

[PATCH 5/6] [CPUFREQ] S5PV210: Adjust udelay prior to voltage scaling down

[Kukjin Kim]

From: Todd Poynor <toddpoynor@google.com>

Voltage scaling accesses the MAX8998 regulators over bit-banged I2C
with lots of udelays.  In the case of decreasing CPU speed, the
number of loops per us for udelay needs to be adjusted prior to
decreasing voltage to avoid delaying for up to 10X too long.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/cpufreq/s5pv210-cpufreq.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index e9b9b7f..78c88a0 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -471,6 +471,8 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 		}
 	}
 
+	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+
 	if (freqs.new < freqs.old) {
 		/* Voltage down: will be implemented */
 		if (!IS_ERR(arm_regulator) &&
@@ -483,7 +485,6 @@ static int s5pv210_target(struct cpufreq_policy *policy,
 		}
 	}
 
-	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 
 	printk(KERN_DEBUG "Perf changed[L%d]\n", index);
 
-- 
1.7.1

[PATCH 6/6] S5PV210: Add reboot notifier to prevent system hang

[Kukjin Kim]

From: Huisung Kang <hs1218.kang@samsung.com>

When system reboot, the CPUFREQ level should be 800MHz to prevent
system lockup.

Signed-off-by: Huisung Kang <hs1218.kang@samsung.com>
Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
 drivers/cpufreq/s5pv210-cpufreq.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index 78c88a0..082d044 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -16,6 +16,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/cpufreq.h>
+#include <linux/reboot.h>
 #include <linux/regulator/consumer.h>
 #include <linux/suspend.h>
 
@@ -599,6 +600,19 @@ static int s5pv210_cpufreq_notifier_event(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
+static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
+						 unsigned long event, void *ptr)
+{
+	int ret;
+
+	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ,
+			DISABLE_FURTHER_CPUFREQ);
+	if (ret < 0)
+		return NOTIFY_BAD;
+
+	return NOTIFY_DONE;
+}
+
 static struct cpufreq_driver s5pv210_driver = {
 	.flags		= CPUFREQ_STICKY,
 	.verify		= s5pv210_verify_speed,
@@ -616,6 +630,11 @@ static struct notifier_block s5pv210_cpufreq_notifier = {
 	.notifier_call = s5pv210_cpufreq_notifier_event,
 };
 
+static struct notifier_block s5pv210_cpufreq_reboot_notifier = {
+	.notifier_call = s5pv210_cpufreq_reboot_notifier_event,
+};
+
+
 static int __init s5pv210_cpufreq_init(void)
 {
 	arm_regulator = regulator_get(NULL, "vddarm");
@@ -632,6 +651,7 @@ static int __init s5pv210_cpufreq_init(void)
 	}
 
 	register_pm_notifier(&s5pv210_cpufreq_notifier);
+	register_reboot_notifier(&s5pv210_cpufreq_reboot_notifier);
 
 	return cpufreq_register_driver(&s5pv210_driver);
 }
-- 
1.7.1

Re: [PATCH 2/6] [CPUFREQ] S5PV210: Add arm/int voltage control support

[Mark Brown]

On Wed, Jun 22, 2011 at 05:07:41PM +0900, Kukjin Kim wrote:

>  	if (freqs.new > freqs.old) {
>  		/* Voltage up: will be implemented */

Presumably this comment is bitrotted?

> +		if (!IS_ERR(arm_regulator) &&
> +				!IS_ERR(int_regulator)) {

You should not do this.  The regulator API stubs itself out and provides
mechanisms for boards to fill out partial regulator configurations, and
clearly these supplies must be physically present in the system.

Plus...

>  static int __init s5pv210_cpufreq_init(void)
>  {
> +	arm_regulator = regulator_get(NULL, "vddarm");
> +	if (IS_ERR(arm_regulator)) {
> +		pr_err("failed to get regulator vddarm");
> +		return PTR_ERR(arm_regulator);
> +	}
> +
> +	int_regulator = regulator_get(NULL, "vddint");
> +	if (IS_ERR(int_regulator)) {
> +		pr_err("failed to get regulator vddint");
> +		regulator_put(arm_regulator);
> +		return PTR_ERR(int_regulator);
> +	}

...the driver won't currently bind if it didn't get the regulators.

Ideally you should also have code which checks to see if the regulators
can support all the operating points.  This will ensure that governors
don't end up spending time trying to go into states that can't be
supported for some reason.  That said this is fairly unusual so may not
be worth bothering.

Re: [PATCH 5/6] S5PV210: Adjust udelay prior to voltage scaling down

[Mark Brown]

On Wed, Jun 22, 2011 at 05:07:44PM +0900, Kukjin Kim wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Voltage scaling accesses the MAX8998 regulators over bit-banged I2C
> with lots of udelays.  In the case of decreasing CPU speed, the
> number of loops per us for udelay needs to be adjusted prior to
> decreasing voltage to avoid delaying for up to 10X too long.

This changelog is very specific to a particular board but actually it's
not at all board specific - we should watch out for this with other
cpufreq drivers as I bet there's more with the same issue.  The S3C64xx
driver is affected for example, I'll send a patch shortly.

> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
>  drivers/cpufreq/s5pv210-cpufreq.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> index e9b9b7f..78c88a0 100644
> --- a/drivers/cpufreq/s5pv210-cpufreq.c
> +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> @@ -471,6 +471,8 @@ static int s5pv210_target(struct cpufreq_policy *policy,
>  		}
>  	}
>  
> +	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +
>  	if (freqs.new < freqs.old) {
>  		/* Voltage down: will be implemented */
>  		if (!IS_ERR(arm_regulator) &&
> @@ -483,7 +485,6 @@ static int s5pv210_target(struct cpufreq_policy *policy,
>  		}
>  	}
>  
> -	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
>  
>  	printk(KERN_DEBUG "Perf changed[L%d]\n", index);
>  
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
"You grabbed my hand and we fell into it, like a daydream - or a fever."

RE: [PATCH 2/6] [CPUFREQ] S5PV210: Add arm/int voltage control support

[Kukjin Kim]

Mark Brown wrote:
> 
> On Wed, Jun 22, 2011 at 05:07:41PM +0900, Kukjin Kim wrote:
> 
> >  	if (freqs.new > freqs.old) {
> >  		/* Voltage up: will be implemented */
> 
> Presumably this comment is bitrotted?
> 
Oops, thanks for your pointing out.

> > +		if (!IS_ERR(arm_regulator) &&
> > +				!IS_ERR(int_regulator)) {
> 
> You should not do this.  The regulator API stubs itself out and provides
> mechanisms for boards to fill out partial regulator configurations, and
> clearly these supplies must be physically present in the system.
> 
Uhm...you're right.

> Plus...
> 
> >  static int __init s5pv210_cpufreq_init(void)
> >  {
> > +	arm_regulator = regulator_get(NULL, "vddarm");
> > +	if (IS_ERR(arm_regulator)) {
> > +		pr_err("failed to get regulator vddarm");
> > +		return PTR_ERR(arm_regulator);
> > +	}
> > +
> > +	int_regulator = regulator_get(NULL, "vddint");
> > +	if (IS_ERR(int_regulator)) {
> > +		pr_err("failed to get regulator vddint");
> > +		regulator_put(arm_regulator);
> > +		return PTR_ERR(int_regulator);
> > +	}
> 
> ...the driver won't currently bind if it didn't get the regulators.
> 
> Ideally you should also have code which checks to see if the regulators
> can support all the operating points.  This will ensure that governors
> don't end up spending time trying to go into states that can't be
> supported for some reason.  That said this is fairly unusual so may not
> be worth bothering.

OK, will address comments from you :)

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.