[PM-SR][PATCH 00/12 v2] omap3: sr: janitor series

[PM-SR][PATCH 00/12 v2] omap3: sr: janitor series

[Nishanth Menon]

August revision of pending janitor patches for SR rebased on latest
pm branch. Should be eventually squashed to the relevant SR patch
prior to upstreaming. Note: this series is a merged series posted
http://marc.info/?l=linux-omap&m=127741497415352&w=2 (June 24)
http://marc.info/?l=linux-omap&m=127750241431955&w=2 (June 25)
"omap3: sr: device: add unlikely checks" was dropped from this series
based on Artem's guidance - none of the other patches have recieved
any review comments so far.

Nishanth Menon (12):
  omap3: voltage: cleanup pr_xxxx
  omap3: voltage: make required variables static
  omap3: voltage: expose omap_change_voltscale_method
  omap3: sr: device: cleanup pr_xxx
  omap3: sr: device: check for dev_attr
  omap3: sr: device: fail sr_dev_init should return error
  omap3: sr: device: make omap_sr_latency static
  omap3: sr: cleanup pr_xxx
  omap3: sr: enable/disable sr only if required
  omap3: sr: export sr_dbg_dir
  omap3: sr: sr_exit should be static
  omap3: sr: class3: make class3_data static

 arch/arm/mach-omap2/smartreflex-class3.c      |    2 +-
 arch/arm/mach-omap2/smartreflex.c             |   26 ++++++++++++++++++------
 arch/arm/mach-omap2/sr_device.c               |   16 +++++++++++---
 arch/arm/mach-omap2/voltage.c                 |   13 ++++++-----
 arch/arm/mach-omap2/voltage.h                 |    1 +
 arch/arm/plat-omap/include/plat/smartreflex.h |    5 ++++
 6 files changed, 45 insertions(+), 18 deletions(-)

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Regards,
Nishanth Menon

[PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Nishanth Menon]

Few more pr_xxxx need cleanup for printing the function name and
not using multiline prints when c allows us to do "".

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/voltage.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 148e4d4..3431fa3 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
 		u32 *option = data;
 		*option = val;
 	} else {
-		pr_notice("DEBUG option not enabled!\n	\
-			echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
+		pr_notice("%s: DEBUG option not enabled! "
+			"echo 1 > pm_debug/enable_sr_vp_debug - to enable\n",
+			__func__);
 	}
 	return 0;
 }
@@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
 	u8 vsel;
 
 	vsel = voltage_read_reg(info->vp_offs.voltage_reg);
-	pr_notice("curr_vsel = %x\n", vsel);
+	pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
 	*val = vsel * 12500 + 600000;
 
 	return 0;
-- 
1.6.3.3

[PM-SR][PATCH 02/12] omap3: voltage: make required variables static

[Nishanth Menon]

debugfs voltage_dir - used only by voltage layer and no reason for
others to add data to it, so make it static.
volt_mod have no business being exposed as global. make it static
we dont expose omap3_vp_offs to the world and is __init data,
so make it static.

This fixes sparse warnings:
arch/arm/mach-omap2/voltage.c:42:15: warning: symbol 'voltage_dir' was not declared. Should it be static?
arch/arm/mach-omap2/voltage.c:49:5: warning: symbol 'volt_mod' was not declared. Should it be static?
arch/arm/mach-omap2/voltage.c:130:27: warning: symbol 'omap3_vp_offs' was not declared. Should it be static?

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---

Note: i had initially considered splitting these into three seperate patches,
but these are too trivial.

 arch/arm/mach-omap2/voltage.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 3431fa3..1a3d00d 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -39,14 +39,14 @@
 #define VP_TRANXDONE_TIMEOUT	300
 
 #ifdef CONFIG_PM_DEBUG
-struct dentry *voltage_dir;
+static struct dentry *voltage_dir;
 #endif
 
 /* VP SR debug support */
 u32 enable_sr_vp_debug;
 
 /* PRM voltage module */
-u32 volt_mod;
+static u32 volt_mod;
 
 /* Voltage processor register offsets */
 struct vp_reg_offs {
@@ -127,7 +127,7 @@ static struct omap_vdd_info *vdd_info;
 static int no_scalable_vdd;
 
 /* OMAP3 VP register offsets and other definitions */
-struct __init vp_reg_offs omap3_vp_offs[] = {
+static struct __init vp_reg_offs omap3_vp_offs[] = {
 	/* VP1 */
 	{
 		.vpconfig_reg = OMAP3_PRM_VP1_CONFIG_OFFSET,
-- 
1.6.3.3

[PM-SR][PATCH 03/12] omap3: voltage: expose omap_change_voltscale_method

[Nishanth Menon]

omap_change_voltscale_method is meant to be used for board files
desiring to choose a different scaling technique. so this function
should be exposed to the world.

also fixes sparse warning:
arch/arm/mach-omap2/voltage.c:1047:6: warning: symbol 'omap_change_voltscale_method' was not declared. Should it be static?

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/voltage.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
index a7be515..6dc8ce4 100644
--- a/arch/arm/mach-omap2/voltage.h
+++ b/arch/arm/mach-omap2/voltage.h
@@ -113,6 +113,7 @@ void omap_voltageprocessor_enable(int vp_id);
 void omap_voltageprocessor_disable(int vp_id);
 int omap_voltage_scale(int vdd, unsigned long target_volt);
 void omap_reset_voltage(int vdd);
+void omap_change_voltscale_method(int voltscale_method);
 int omap_get_voltage_table(int vdd, struct omap_volt_data **volt_data);
 struct omap_volt_data *omap_get_volt_data(int vdd, unsigned long volt);
 void omap_voltage_register_pmic(struct omap_volt_pmic_info *pmic_info);
-- 
1.6.3.3

[PM-SR][PATCH 04/12] omap3: sr: device: cleanup pr_xxx

[Nishanth Menon]

Strings in c dont need to be split accross multiple lines with \
. instead they can be put as "abc " "def " and it is equivalent to
"abc def".

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index dbf7603..7d13704 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -151,8 +151,9 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 	sr_dev_data->volts_supported = omap_get_voltage_table(i,
 				&sr_dev_data->volt_data);
 	if (!sr_dev_data->volts_supported) {
-		pr_warning("%s: No Voltage table registerd fo VDD%d.Something \
-				really wrong\n\n", __func__, i + 1);
+		pr_warning("%s: No Voltage table registerd fo VDD%d. "
+				"Something is really wrong\n",
+				__func__, i + 1);
 		i++;
 		kfree(sr_data);
 		return 0;
-- 
1.6.3.3

[PM-SR][PATCH 05/12] omap3: sr: device: check for dev_attr

[Nishanth Menon]

In the unlikely case that hwmod database is messed up, dont crash
report error and attempt to recover.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 7d13704..6f70da6 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -130,6 +130,12 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 	}
 
 	sr_dev_data = (struct omap_sr_dev_data *)oh->dev_attr;
+	if (unlikely(!sr_dev_data)) {
+		pr_err("%s: Bad oh->dev_attr!\n", __func__);
+		kfree(sr_data);
+		return -EINVAL;
+	}
+
 	/*
 	 * OMAP3430 ES3.1 chips by default come with Efuse burnt
 	 * with parameters required for full functionality of
-- 
1.6.3.3

[PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error

[Nishanth Menon]

sr_dev_init should return error on error conditions

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 6f70da6..8fb60d8 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -162,7 +162,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 				__func__, i + 1);
 		i++;
 		kfree(sr_data);
-		return 0;
+		return -ENODATA;
 	}
 	sr_set_nvalues(sr_dev_data, sr_data);
 	od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data),
@@ -172,6 +172,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
 		pr_warning("%s: Could not build omap_device for %s: %s.\n\n",
 			__func__, name, oh->name);
 		kfree(sr_data);
+		return PTR_ERR(od);
 	}
 	i++;
 	return 0;
-- 
1.6.3.3

[PM-SR][PATCH 07/12] omap3: sr: device: make omap_sr_latency static

[Nishanth Menon]

omap_sr_latency definition does not need to be exposed to the world
but we cant make it __init data as the pointer is stored in odev
to be used beyond basic kernel boot.

also fixes sparse warning:
arch/arm/mach-omap2/sr_device.c:32:31: warning: symbol 'omap_sr_latency' was not declared. Should it be static?

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/sr_device.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 8fb60d8..e816666 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -29,7 +29,7 @@
 
 #include "voltage.h"
 
-struct omap_device_pm_latency omap_sr_latency[] = {
+static struct omap_device_pm_latency omap_sr_latency[] = {
 	{
 		.deactivate_func = omap_device_idle_hwmods,
 		.activate_func	 = omap_device_enable_hwmods,
-- 
1.6.3.3

[PM-SR][PATCH 08/12] omap3: sr: cleanup pr_xxx

[Nishanth Menon]

pr_xxx family is not informative for debug unless one decides
to grep the code, instead print the function to help debug
easier on the field.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/smartreflex.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index 57fc9b2..d63691b 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -804,8 +804,9 @@ static int omap_sr_params_store(void *data, u64 val)
 		u32 *option = data;
 		*option = val;
 	} else {
-		pr_notice("DEBUG option not enabled!\n	\
-			echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
+		pr_notice("%s: DEBUG option not enabled! "
+			"echo 1 > pm_debug/enable_sr_vp_debug to enable\n",
+			__func__);
 	}
 	return 0;
 }
-- 
1.6.3.3

[PM-SR][PATCH 09/12] omap3: sr: enable/disable sr only if required

[Nishanth Menon]

We dont need to go down the path of enabling/disabling the SR
if we dont need to. do some sanity check and trigger if needed

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/smartreflex.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index d63691b..9b5a10e 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -778,15 +778,26 @@ static int omap_sr_autocomp_show(void *data, u64 *val)
 static int omap_sr_autocomp_store(void *data, u64 val)
 {
 	struct omap_sr *sr_info = (struct omap_sr *) data;
+	u32 value = (u32) val;
 
 	if (!sr_info) {
 		pr_warning("%s: omap_sr struct for SR not found\n", __func__);
 		return -EINVAL;
 	}
-	if (!val)
-		sr_stop_vddautocomp(sr_info);
-	else
-		sr_start_vddautocomp(sr_info);
+
+	/* Sanity check */
+	if (value && (value != 1)) {
+		pr_err("%s: invalid value %d\n", __func__, value);
+		return -EINVAL;
+	}
+
+	/* change only if needed */
+	if (sr_info->is_autocomp_active ^ value) {
+		if (!val)
+			sr_stop_vddautocomp(sr_info);
+		else
+			sr_start_vddautocomp(sr_info);
+	}
 	return 0;
 }
 
-- 
1.6.3.3

[PM-SR][PATCH 10/12] omap3: sr: export sr_dbg_dir

[Nishanth Menon]

sr_dbg_dir is currently used privately in smartreflex.c, however,
smartreflex class drivers could store their own debugfs entries there
as well.

This also fixes the sparse warning:
arch/arm/mach-omap2/smartreflex.c:44:15: warning: symbol 'sr_dbg_dir' was not declared. Should it be static?

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/plat-omap/include/plat/smartreflex.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/smartreflex.h b/arch/arm/plat-omap/include/plat/smartreflex.h
index 1105db0..df58026 100644
--- a/arch/arm/plat-omap/include/plat/smartreflex.h
+++ b/arch/arm/plat-omap/include/plat/smartreflex.h
@@ -263,6 +263,11 @@ int omap_sr_register_class(struct omap_smartreflex_class_data *class_data);
 
 /* API to register the pmic specific data with the smartreflex driver. */
 void omap_sr_register_pmic(struct omap_smartreflex_pmic_data *pmic_data);
+
+#ifdef CONFIG_PM_DEBUG
+extern struct dentry *sr_dbg_dir;
+#endif
+
 #else
 static inline void omap_smartreflex_enable(int srid) {}
 static inline void omap_smartreflex_disable(int srid) {}
-- 
1.6.3.3

[PM-SR][PATCH 11/12] omap3: sr: sr_exit should be static

[Nishanth Menon]

sr_exit has no business being a public function.
fixes sparse:
arch/arm/mach-omap2/smartreflex.c:959:13: warning: symbol 'sr_exit' was not declared. Should it be static?

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/smartreflex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index 9b5a10e..a723ac7 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -968,7 +968,7 @@ static int __init sr_init(void)
 	return 0;
 }
 
-void __exit sr_exit(void)
+static void __exit sr_exit(void)
 {
 	platform_driver_unregister(&smartreflex_driver);
 }
-- 
1.6.3.3

[PM-SR][PATCH 12/12] omap3: sr: class3: make class3_data static

[Nishanth Menon]

we dont expose the structure to the world, so this should be static
however, since sr core does not take a copy of the same, we cant make
it __init data.

fixes sparse warning:
arch/arm/mach-omap2/smartreflex-class3.c:50:36: warning: symbol 'class3_data' was not declared. Should it be static?

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/smartreflex-class3.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex-class3.c b/arch/arm/mach-omap2/smartreflex-class3.c
index f3b766f..530b2f0 100644
--- a/arch/arm/mach-omap2/smartreflex-class3.c
+++ b/arch/arm/mach-omap2/smartreflex-class3.c
@@ -47,7 +47,7 @@ static int sr_class3_configure(int id)
 }
 
 /* SR class3 structure */
-struct omap_smartreflex_class_data class3_data = {
+static struct omap_smartreflex_class_data class3_data = {
 	.enable = sr_class3_enable,
 	.disable = sr_class3_disable,
 	.configure = sr_class3_configure,
-- 
1.6.3.3

RE: [PM-SR][PATCH 08/12] omap3: sr: cleanup pr_xxx

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 08/12] omap3: sr: cleanup pr_xxx
>>
>>pr_xxx family is not informative for debug unless one decides
>>to grep the code, instead print the function to help debug
>>easier on the field.
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/smartreflex.c |    5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>>index 57fc9b2..d63691b 100644
>>--- a/arch/arm/mach-omap2/smartreflex.c
>>+++ b/arch/arm/mach-omap2/smartreflex.c
>>@@ -804,8 +804,9 @@ static int omap_sr_params_store(void *data, u64 val)
>> 		u32 *option = data;
>> 		*option = val;
>> 	} else {
>>-		pr_notice("DEBUG option not enabled!\n	\
>>-			echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
>>+		pr_notice("%s: DEBUG option not enabled! "
>>+			"echo 1 > pm_debug/enable_sr_vp_debug to enable\n",
>>+			__func__);
>> 	}

Accepted and will be collated with rest of the patches.
>> 	return 0;
>> }
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 09/12] omap3: sr: enable/disable sr only if required

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 09/12] omap3: sr: enable/disable sr only if required
>>
>>We dont need to go down the path of enabling/disabling the SR
>>if we dont need to. do some sanity check and trigger if needed
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/smartreflex.c |   19 +++++++++++++++----
>> 1 files changed, 15 insertions(+), 4 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>>index d63691b..9b5a10e 100644
>>--- a/arch/arm/mach-omap2/smartreflex.c
>>+++ b/arch/arm/mach-omap2/smartreflex.c
>>@@ -778,15 +778,26 @@ static int omap_sr_autocomp_show(void *data, u64 *val)
>> static int omap_sr_autocomp_store(void *data, u64 val)
>> {
>> 	struct omap_sr *sr_info = (struct omap_sr *) data;
>>+	u32 value = (u32) val;
>>
>> 	if (!sr_info) {
>> 		pr_warning("%s: omap_sr struct for SR not found\n", __func__);
>> 		return -EINVAL;
>> 	}
>>-	if (!val)
>>-		sr_stop_vddautocomp(sr_info);
>>-	else
>>-		sr_start_vddautocomp(sr_info);
>>+
>>+	/* Sanity check */
>>+	if (value && (value != 1)) {
>>+		pr_err("%s: invalid value %d\n", __func__, value);
>>+		return -EINVAL;
>>+	}
Will take this in.

>>+
>>+	/* change only if needed */
>>+	if (sr_info->is_autocomp_active ^ value) {

I do not think this is necessary. sr_start_vddautocomp and sr_stop_vddautocomp will take care of
enabling and disabling intelligently.

Regards
Thara

>>+		if (!val)
>>+			sr_stop_vddautocomp(sr_info);
>>+		else
>>+			sr_start_vddautocomp(sr_info);
>>+	}

>> 	return 0;
>> }
>>
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 10/12] omap3: sr: export sr_dbg_dir

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 10/12] omap3: sr: export sr_dbg_dir
>>
>>sr_dbg_dir is currently used privately in smartreflex.c, however,
>>smartreflex class drivers could store their own debugfs entries there
>>as well.
>>
>>This also fixes the sparse warning:
>>arch/arm/mach-omap2/smartreflex.c:44:15: warning: symbol 'sr_dbg_dir' was not declared. Should it be
>>static?
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/plat-omap/include/plat/smartreflex.h |    5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>
>>diff --git a/arch/arm/plat-omap/include/plat/smartreflex.h b/arch/arm/plat-
>>omap/include/plat/smartreflex.h
>>index 1105db0..df58026 100644
>>--- a/arch/arm/plat-omap/include/plat/smartreflex.h
>>+++ b/arch/arm/plat-omap/include/plat/smartreflex.h
>>@@ -263,6 +263,11 @@ int omap_sr_register_class(struct omap_smartreflex_class_data *class_data);
>>
>> /* API to register the pmic specific data with the smartreflex driver. */
>> void omap_sr_register_pmic(struct omap_smartreflex_pmic_data *pmic_data);
>>+
>>+#ifdef CONFIG_PM_DEBUG
>>+extern struct dentry *sr_dbg_dir;
>>+#endif

Will take this in

Regards
Thara

>>+
>> #else
>> static inline void omap_smartreflex_enable(int srid) {}
>> static inline void omap_smartreflex_disable(int srid) {}
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 11/12] omap3: sr: sr_exit should be static

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 11/12] omap3: sr: sr_exit should be static
>>
>>sr_exit has no business being a public function.
>>fixes sparse:
>>arch/arm/mach-omap2/smartreflex.c:959:13: warning: symbol 'sr_exit' was not declared. Should it be
>>static?
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/smartreflex.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>>index 9b5a10e..a723ac7 100644
>>--- a/arch/arm/mach-omap2/smartreflex.c
>>+++ b/arch/arm/mach-omap2/smartreflex.c
>>@@ -968,7 +968,7 @@ static int __init sr_init(void)
>> 	return 0;
>> }
>>
>>-void __exit sr_exit(void)
>>+static void __exit sr_exit(void)

Will collate this.
>> {
>> 	platform_driver_unregister(&smartreflex_driver);
>> }
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 03/12] omap3: voltage: expose omap_change_voltscale_method

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 03/12] omap3: voltage: expose omap_change_voltscale_method
>>
>>omap_change_voltscale_method is meant to be used for board files
>>desiring to choose a different scaling technique. so this function
>>should be exposed to the world.
>>
>>also fixes sparse warning:
>>arch/arm/mach-omap2/voltage.c:1047:6: warning: symbol 'omap_change_voltscale_method' was not
>>declared. Should it be static?
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/voltage.h |    1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
>>index a7be515..6dc8ce4 100644
>>--- a/arch/arm/mach-omap2/voltage.h
>>+++ b/arch/arm/mach-omap2/voltage.h
>>@@ -113,6 +113,7 @@ void omap_voltageprocessor_enable(int vp_id);
>> void omap_voltageprocessor_disable(int vp_id);
>> int omap_voltage_scale(int vdd, unsigned long target_volt);
>> void omap_reset_voltage(int vdd);
>>+void omap_change_voltscale_method(int voltscale_method);

Collated with rest of the series

Regards
Thara
>> int omap_get_voltage_table(int vdd, struct omap_volt_data **volt_data);
>> struct omap_volt_data *omap_get_volt_data(int vdd, unsigned long volt);
>> void omap_voltage_register_pmic(struct omap_volt_pmic_info *pmic_info);
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 04/12] omap3: sr: device: cleanup pr_xxx

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 04/12] omap3: sr: device: cleanup pr_xxx
>>
>>Strings in c dont need to be split accross multiple lines with \
>>. instead they can be put as "abc " "def " and it is equivalent to
>>"abc def".
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/sr_device.c |    5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
>>index dbf7603..7d13704 100644
>>--- a/arch/arm/mach-omap2/sr_device.c
>>+++ b/arch/arm/mach-omap2/sr_device.c
>>@@ -151,8 +151,9 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>> 	sr_dev_data->volts_supported = omap_get_voltage_table(i,
>> 				&sr_dev_data->volt_data);
>> 	if (!sr_dev_data->volts_supported) {
>>-		pr_warning("%s: No Voltage table registerd fo VDD%d.Something \
>>-				really wrong\n\n", __func__, i + 1);
>>+		pr_warning("%s: No Voltage table registerd fo VDD%d. "
>>+				"Something is really wrong\n",
>>+				__func__, i + 1);

Taken in.

Regards
Thara
>> 		i++;
>> 		kfree(sr_data);
>> 		return 0;
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error
>>
>>sr_dev_init should return error on error conditions
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/sr_device.c |    3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
>>index 6f70da6..8fb60d8 100644
>>--- a/arch/arm/mach-omap2/sr_device.c
>>+++ b/arch/arm/mach-omap2/sr_device.c
>>@@ -162,7 +162,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>> 				__func__, i + 1);
>> 		i++;
>> 		kfree(sr_data);
>>-		return 0;
>>+		return -ENODATA;
>> 	}
>> 	sr_set_nvalues(sr_dev_data, sr_data);
>> 	od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data),
>>@@ -172,6 +172,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>> 		pr_warning("%s: Could not build omap_device for %s: %s.\n\n",
>> 			__func__, name, oh->name);
>> 		kfree(sr_data);
>>+		return PTR_ERR(od);
>> 	}
NAK for this change.
This API is called from omap_hwmod_for_each_by_class for every smartreflex module.
If This API returns an error omap_hwmod_for_each_by_class will exit without looping over
rest of the smartreflex modules. This means a error for one smartreflex module will prevent
rest of the smartreflex modules to be initialized. We do not want this. Hence this API returns 0
for non-availability of data for a smartreflex module.

Regards
Thara

>> 	i++;
>> 	return 0;
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 07/12] omap3: sr: device: make omap_sr_latency static

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 07/12] omap3: sr: device: make omap_sr_latency static
>>
>>omap_sr_latency definition does not need to be exposed to the world
>>but we cant make it __init data as the pointer is stored in odev
>>to be used beyond basic kernel boot.
>>
>>also fixes sparse warning:
>>arch/arm/mach-omap2/sr_device.c:32:31: warning: symbol 'omap_sr_latency' was not declared. Should it
>>be static?
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/sr_device.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
>>index 8fb60d8..e816666 100644
>>--- a/arch/arm/mach-omap2/sr_device.c
>>+++ b/arch/arm/mach-omap2/sr_device.c
>>@@ -29,7 +29,7 @@
>>
>> #include "voltage.h"
>>
>>-struct omap_device_pm_latency omap_sr_latency[] = {
>>+static struct omap_device_pm_latency omap_sr_latency[] = {
>> 	{
>> 		.deactivate_func = omap_device_idle_hwmods,
>> 		.activate_func	 = omap_device_enable_hwmods,

Taken in

Regards
Thara
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 05/12] omap3: sr: device: check for dev_attr

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 05/12] omap3: sr: device: check for dev_attr
>>
>>In the unlikely case that hwmod database is messed up, dont crash
>>report error and attempt to recover.
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/sr_device.c |    6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
>>index 7d13704..6f70da6 100644
>>--- a/arch/arm/mach-omap2/sr_device.c
>>+++ b/arch/arm/mach-omap2/sr_device.c
>>@@ -130,6 +130,12 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>> 	}
>>
>> 	sr_dev_data = (struct omap_sr_dev_data *)oh->dev_attr;
>>+	if (unlikely(!sr_dev_data)) {
>>+		pr_err("%s: Bad oh->dev_attr!\n", __func__);
>>+		kfree(sr_data);
>>+		return -EINVAL;
>>+	}

Taken in after modifications as per the reply for patch 06/12

Regards
Thara
>>+
>> 	/*
>> 	 * OMAP3430 ES3.1 chips by default come with Efuse burnt
>> 	 * with parameters required for full functionality of
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 12/12] omap3: sr: class3: make class3_data static

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 12/12] omap3: sr: class3: make class3_data static
>>
>>we dont expose the structure to the world, so this should be static
>>however, since sr core does not take a copy of the same, we cant make
>>it __init data.
>>
>>fixes sparse warning:
>>arch/arm/mach-omap2/smartreflex-class3.c:50:36: warning: symbol 'class3_data' was not declared.
>>Should it be static?
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/smartreflex-class3.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/smartreflex-class3.c b/arch/arm/mach-omap2/smartreflex-class3.c
>>index f3b766f..530b2f0 100644
>>--- a/arch/arm/mach-omap2/smartreflex-class3.c
>>+++ b/arch/arm/mach-omap2/smartreflex-class3.c
>>@@ -47,7 +47,7 @@ static int sr_class3_configure(int id)
>> }
>>
>> /* SR class3 structure */
>>-struct omap_smartreflex_class_data class3_data = {
>>+static struct omap_smartreflex_class_data class3_data = {
>> 	.enable = sr_class3_enable,
>> 	.disable = sr_class3_disable,
>> 	.configure = sr_class3_configure,

Taken in 
Regards
Thara
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 02/12] omap3: voltage: make required variables static

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 02/12] omap3: voltage: make required variables static
>>
>>debugfs voltage_dir - used only by voltage layer and no reason for
>>others to add data to it, so make it static.
>>volt_mod have no business being exposed as global. make it static
>>we dont expose omap3_vp_offs to the world and is __init data,
>>so make it static.
>>
>>This fixes sparse warnings:
>>arch/arm/mach-omap2/voltage.c:42:15: warning: symbol 'voltage_dir' was not declared. Should it be
>>static?
>>arch/arm/mach-omap2/voltage.c:49:5: warning: symbol 'volt_mod' was not declared. Should it be static?
>>arch/arm/mach-omap2/voltage.c:130:27: warning: symbol 'omap3_vp_offs' was not declared. Should it be
>>static?
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>>
>>Note: i had initially considered splitting these into three seperate patches,
>>but these are too trivial.
>>
>> arch/arm/mach-omap2/voltage.c |    6 +++---
>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>index 3431fa3..1a3d00d 100644
>>--- a/arch/arm/mach-omap2/voltage.c
>>+++ b/arch/arm/mach-omap2/voltage.c
>>@@ -39,14 +39,14 @@
>> #define VP_TRANXDONE_TIMEOUT	300
>>
>> #ifdef CONFIG_PM_DEBUG
>>-struct dentry *voltage_dir;
>>+static struct dentry *voltage_dir;
>> #endif
>>
>> /* VP SR debug support */
>> u32 enable_sr_vp_debug;
>>
>> /* PRM voltage module */
>>-u32 volt_mod;
>>+static u32 volt_mod;
>>
>> /* Voltage processor register offsets */
>> struct vp_reg_offs {
>>@@ -127,7 +127,7 @@ static struct omap_vdd_info *vdd_info;
>> static int no_scalable_vdd;
>>
>> /* OMAP3 VP register offsets and other definitions */
>>-struct __init vp_reg_offs omap3_vp_offs[] = {
>>+static struct __init vp_reg_offs omap3_vp_offs[] = {

This change is no longer valid after the patch converting vdd id's to
names. Rest of the two changes have been taken in.

Regards,
Thara
>> 	/* VP1 */
>> 	{
>> 		.vpconfig_reg = OMAP3_PRM_VP1_CONFIG_OFFSET,
>>--
>>1.6.3.3

RE: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
>>
>>Few more pr_xxxx need cleanup for printing the function name and
>>not using multiline prints when c allows us to do "".
>>
>>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>>Cc: Thara Gopinath <thara@ti.com>
>>
>>Signed-off-by: Nishanth Menon <nm@ti.com>
>>---
>> arch/arm/mach-omap2/voltage.c |    7 ++++---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>index 148e4d4..3431fa3 100644
>>--- a/arch/arm/mach-omap2/voltage.c
>>+++ b/arch/arm/mach-omap2/voltage.c
>>@@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
>> 		u32 *option = data;
>> 		*option = val;
>> 	} else {
>>-		pr_notice("DEBUG option not enabled!\n	\
>>-			echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
>>+		pr_notice("%s: DEBUG option not enabled! "
>>+			"echo 1 > pm_debug/enable_sr_vp_debug - to enable\n",
>>+			__func__);
>> 	}

I do not think this is needed as these fns get called only from user space.

Regards
Thara

>> 	return 0;
>> }
>>@@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
>> 	u8 vsel;
>>
>> 	vsel = voltage_read_reg(info->vp_offs.voltage_reg);
>>-	pr_notice("curr_vsel = %x\n", vsel);
>>+	pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
>> 	*val = vsel * 12500 + 600000;
>>
>> 	return 0;
>>--
>>1.6.3.3

Re: [PM-SR][PATCH 09/12] omap3: sr: enable/disable sr only if required

[Nishanth Menon]

On 08/05/2010 11:51 PM, Gopinath, Thara wrote:
>
>
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Friday, August 06, 2010 3:54 AM
>>> To: linux-omap
>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>> Subject: [PM-SR][PATCH 09/12] omap3: sr: enable/disable sr only if required
>>>
>>> We dont need to go down the path of enabling/disabling the SR
>>> if we dont need to. do some sanity check and trigger if needed
>>>
>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>> Cc: Thara Gopinath<thara@ti.com>
>>>
>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>> ---
>>> arch/arm/mach-omap2/smartreflex.c |   19 +++++++++++++++----
>>> 1 files changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>>> index d63691b..9b5a10e 100644
>>> --- a/arch/arm/mach-omap2/smartreflex.c
>>> +++ b/arch/arm/mach-omap2/smartreflex.c
>>> @@ -778,15 +778,26 @@ static int omap_sr_autocomp_show(void *data, u64 *val)
>>> static int omap_sr_autocomp_store(void *data, u64 val)
>>> {
>>> 	struct omap_sr *sr_info = (struct omap_sr *) data;
>>> +	u32 value = (u32) val;
>>>
>>> 	if (!sr_info) {
>>> 		pr_warning("%s: omap_sr struct for SR not found\n", __func__);
>>> 		return -EINVAL;
>>> 	}
>>> -	if (!val)
>>> -		sr_stop_vddautocomp(sr_info);
>>> -	else
>>> -		sr_start_vddautocomp(sr_info);
>>> +
>>> +	/* Sanity check */
>>> +	if (value&&  (value != 1)) {
>>> +		pr_err("%s: invalid value %d\n", __func__, value);
>>> +		return -EINVAL;
>>> +	}
> Will take this in.
>
>>> +
>>> +	/* change only if needed */
>>> +	if (sr_info->is_autocomp_active ^ value) {
>
> I do not think this is necessary. sr_start_vddautocomp and sr_stop_vddautocomp will take care of
> enabling and disabling intelligently.

hypothesis 1: helper level code is intelligent:
So, lets see where that intelligence exists:
in start autocomp:
static void sr_start_vddautocomp(struct omap_sr *sr) 

{
         if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) {
                 dev_warn(&sr->pdev->dev,
                         "%s: smartreflex class driver not registered\n",
                         __func__);
                 return;
         }
[NM - Till here we ensured we have an SR class]
         sr->is_autocomp_active = 1;
[NM - aha, we already enabled autocomp]
         if (sr_class->enable(sr->srid))
[NM - this is where the intelligence is -> Class drivers should be now 
intelligent to know if autocomp was previously enabled]
                 sr->is_autocomp_active = 0;
[NM - if we failed we set it then we disable autocomp_active]
}

ok now, lets dig a little further into class3 enable function -> how 
intelligent it really is:
static int sr_class3_enable(int id)
{
         unsigned long volt = 0;

         volt = get_curr_voltage(id);
         if (!volt) {
                 pr_warning("%s: Current voltage unknown.Cannot enable 
SR%d\n",
                                 __func__, id);
                 return -ENODATA;
         }
[NM - so far no intelligence]
         omap_voltageprocessor_enable(id);
[NM - woops we renable voltage processor if we were already enabled]
         return sr_enable(id, volt);
[NM - aha so we are back to Smartreflex core driver for intelligence]
}

digging futher into sr_enable for "intelligent check":
int sr_enable(int srid, unsigned long volt)
{
	u32 nvalue_reciprocal;
	struct omap_volt_data *volt_data;
	struct omap_sr *sr = _sr_lookup(srid);
	int ret;

	if (!sr) {
		pr_warning("%s: omap_sr struct for SR%d not found\n",
			__func__, srid + 1);
		return -EINVAL;
	}

	volt_data = omap_get_volt_data(sr->srid, volt);

	if (IS_ERR(volt_data)) {
		dev_warn(&sr->pdev->dev, "%s: Unable to get voltage table"
			" for nominal voltage %ld\n", __func__, volt);
		return -ENODATA;
	}

	nvalue_reciprocal = volt_data->sr_nvalue;

	if (!nvalue_reciprocal) {
		dev_warn(&sr->pdev->dev, "%s: NVALUE = 0 at voltage %ld\n",
			__func__, volt);
		return -ENODATA;
	}
[NM: So far no intelligence]
	/*
	 * errminlimit is opp dependent and hence linked to voltage
	 * if the debug option is enabled, the user might have over ridden
	 * this parameter so do not get it from voltage table
	 */
	if (!enable_sr_vp_debug)
		sr->err_minlimit = volt_data->sr_errminlimit;

	/* Enable the clocks */
	if (!sr->is_sr_enable) {
		struct omap_sr_data *pdata =
				sr->pdev->dev.platform_data;
		if (pdata->device_enable) {
			ret = pdata->device_enable(sr->pdev);
			if (ret)
				return ret;
		} else {
			dev_warn(&sr->pdev->dev, "%s: Not able to turn on SR"
				"clocks during enable. So returning\n",
				__func__);
			return -EPERM;
		}
		sr->is_sr_enable = 1;
	}
[NM: Dont think this matters too much but yeah, we do set is_sr_enable 
to 1 - the fact that we came to this depth implies something is horribly 
screwed up we are in our normal enable flow!!!]
	/* Check if SR is already enabled. If yes do nothing */
	if (sr_read_reg(sr, SRCONFIG) & SRCONFIG_SRENABLE)
		return 0;

[NM - this is where the real "intelligence" is - since we already have 
sr_enable set in previous operation in Class3, it will detect and return.

HOWEVER, this "intelligence" will fail miserably in the case of class 2 
or class 1.5]


Hypothesis 2: it is a good practice to do verification of parameters in 
helper functions.

as I show in the previous example, there actual verification is 3 
functions deep and not really a good way of "intelligent" check - for as 
far as I think,
a)  I dont even care to have to pay a single function call penalty that 
I can check with a single if statement
b) as a caller of a function, i make every effort to ensure that the 
parameters that I call the function with are correct and I call it only 
when needed

in short, I dont think your analysis is right, we dont have that 
intelligence there, and my suggestion - it is better to do the check 
before calling a helper function.

Regards,
Nishanth Menon

Re: [PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error

[Nishanth Menon]

On 08/06/2010 02:24 AM, Gopinath, Thara wrote:
>
>
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Friday, August 06, 2010 3:54 AM
>>> To: linux-omap
>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>> Subject: [PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error
>>>
>>> sr_dev_init should return error on error conditions
>>>
>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>> Cc: Thara Gopinath<thara@ti.com>
>>>
>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>> ---
>>> arch/arm/mach-omap2/sr_device.c |    3 ++-
>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
>>> index 6f70da6..8fb60d8 100644
>>> --- a/arch/arm/mach-omap2/sr_device.c
>>> +++ b/arch/arm/mach-omap2/sr_device.c
>>> @@ -162,7 +162,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>>> 				__func__, i + 1);
>>> 		i++;
>>> 		kfree(sr_data);
>>> -		return 0;
>>> +		return -ENODATA;
>>> 	}
>>> 	sr_set_nvalues(sr_dev_data, sr_data);
>>> 	od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data),
>>> @@ -172,6 +172,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>>> 		pr_warning("%s: Could not build omap_device for %s: %s.\n\n",
>>> 			__func__, name, oh->name);
>>> 		kfree(sr_data);
>>> +		return PTR_ERR(od);
>>> 	}
> NAK for this change.
> This API is called from omap_hwmod_for_each_by_class for every smartreflex module.
> If This API returns an error omap_hwmod_for_each_by_class will exit without looping over
> rest of the smartreflex modules. This means a error for one smartreflex module will prevent
> rest of the smartreflex modules to be initialized. We do not want this. Hence this API returns 0
> for non-availability of data for a smartreflex module.

why do we want this behavior(aka continue with as many modules as you 
can enable)? h/wmod data structure are NOT meant to be corrupted if they 
are, what guarentee do we have that the rest of the sr module data 
structures have the right hwmods(even if they passed device_build?).. if 
they are, what is the point in enabling SR in half the domains - we 
should flag this as an error to developer and get him to fix it instead 
of encouraging this slipping by half a dozen developers as only sr_l3 
failed or something similar..

Regards,
Nishanth Menon

Re: [PM-SR][PATCH 05/12] omap3: sr: device: check for dev_attr

[Nishanth Menon]

On 08/06/2010 02:27 AM, Gopinath, Thara wrote:
>
>
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Friday, August 06, 2010 3:54 AM
>>> To: linux-omap
>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>> Subject: [PM-SR][PATCH 05/12] omap3: sr: device: check for dev_attr
>>>
>>> In the unlikely case that hwmod database is messed up, dont crash
>>> report error and attempt to recover.
>>>
>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>> Cc: Thara Gopinath<thara@ti.com>
>>>
>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>> ---
>>> arch/arm/mach-omap2/sr_device.c |    6 ++++++
>>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
>>> index 7d13704..6f70da6 100644
>>> --- a/arch/arm/mach-omap2/sr_device.c
>>> +++ b/arch/arm/mach-omap2/sr_device.c
>>> @@ -130,6 +130,12 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>>> 	}
>>>
>>> 	sr_dev_data = (struct omap_sr_dev_data *)oh->dev_attr;
>>> +	if (unlikely(!sr_dev_data)) {
>>> +		pr_err("%s: Bad oh->dev_attr!\n", __func__);
>>> +		kfree(sr_data);
>>> +		return -EINVAL;
>>> +	}
>
> Taken in after modifications as per the reply for patch 06/12

I dont agree to the mod of 06/12. sorry.

Regards,
Nishanth Menon

Re: [PM-SR][PATCH 02/12] omap3: voltage: make required variables static

[Nishanth Menon]

On 08/06/2010 02:39 AM, Gopinath, Thara wrote:
>
>
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Friday, August 06, 2010 3:54 AM
>>> To: linux-omap
>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>> Subject: [PM-SR][PATCH 02/12] omap3: voltage: make required variables static
>>>
>>> debugfs voltage_dir - used only by voltage layer and no reason for
>>> others to add data to it, so make it static.
>>> volt_mod have no business being exposed as global. make it static
>>> we dont expose omap3_vp_offs to the world and is __init data,
>>> so make it static.
>>>
>>> This fixes sparse warnings:
>>> arch/arm/mach-omap2/voltage.c:42:15: warning: symbol 'voltage_dir' was not declared. Should it be
>>> static?
>>> arch/arm/mach-omap2/voltage.c:49:5: warning: symbol 'volt_mod' was not declared. Should it be static?
>>> arch/arm/mach-omap2/voltage.c:130:27: warning: symbol 'omap3_vp_offs' was not declared. Should it be
>>> static?
>>>
>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>> Cc: Thara Gopinath<thara@ti.com>
>>>
>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>> ---
>>>
>>> Note: i had initially considered splitting these into three seperate patches,
>>> but these are too trivial.
>>>
>>> arch/arm/mach-omap2/voltage.c |    6 +++---
>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>> index 3431fa3..1a3d00d 100644
>>> --- a/arch/arm/mach-omap2/voltage.c
>>> +++ b/arch/arm/mach-omap2/voltage.c
>>> @@ -39,14 +39,14 @@
>>> #define VP_TRANXDONE_TIMEOUT	300
>>>
>>> #ifdef CONFIG_PM_DEBUG
>>> -struct dentry *voltage_dir;
>>> +static struct dentry *voltage_dir;
>>> #endif
>>>
>>> /* VP SR debug support */
>>> u32 enable_sr_vp_debug;
>>>
>>> /* PRM voltage module */
>>> -u32 volt_mod;
>>> +static u32 volt_mod;
>>>
>>> /* Voltage processor register offsets */
>>> struct vp_reg_offs {
>>> @@ -127,7 +127,7 @@ static struct omap_vdd_info *vdd_info;
>>> static int no_scalable_vdd;
>>>
>>> /* OMAP3 VP register offsets and other definitions */
>>> -struct __init vp_reg_offs omap3_vp_offs[] = {
>>> +static struct __init vp_reg_offs omap3_vp_offs[] = {
>
> This change is no longer valid after the patch converting vdd id's to
> names. Rest of the two changes have been taken in.

huh where???? kevin's pm branch is what I work on and post to l-o. I am 
not interested if others have a private tree that none in this community 
can see or work with - sorry l-o does not provide me an indication on an 
alternate kernel tree for sr development!

Regards,
Nishanth Menon

Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Nishanth Menon]

On 08/06/2010 02:42 AM, Gopinath, Thara wrote:
>
>
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Friday, August 06, 2010 3:54 AM
>>> To: linux-omap
>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>> Subject: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
>>>
>>> Few more pr_xxxx need cleanup for printing the function name and
>>> not using multiline prints when c allows us to do "".
>>>
>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>> Cc: Thara Gopinath<thara@ti.com>
>>>
>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>> ---
>>> arch/arm/mach-omap2/voltage.c |    7 ++++---
>>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>> index 148e4d4..3431fa3 100644
>>> --- a/arch/arm/mach-omap2/voltage.c
>>> +++ b/arch/arm/mach-omap2/voltage.c
>>> @@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
>>> 		u32 *option = data;
>>> 		*option = val;
>>> 	} else {
>>> -		pr_notice("DEBUG option not enabled!\n	\
>>> -			echo 1>  pm_debug/enable_sr_vp_debug - to enable\n");
>>> +		pr_notice("%s: DEBUG option not enabled! "
>>> +			"echo 1>  pm_debug/enable_sr_vp_debug - to enable\n",
>>> +			__func__);
>>> 	}
>
> I do not think this is needed as these fns get called only from user space.
>

have you tried working on someone else's unfamiliar code and grepping 
for a warning message at the middle of the night because some developer 
forgot to give a "%s:...", __func__ with half a dozen people watching 
behind your back to know it's meaning because the product is hitting the 
shelves the next day? Instead of being able to use cscope and pop the 
function in and go straight to it and understand what is happening?? I 
believe there are few in this list who had the fortune to be in that sitn..


Well that is my motivation here. if driver reports a warning to kernel 
syslog, well, I expect the log to contain the function name for me to 
maintain faster.

> Regards
> Thara
>
>>> 	return 0;
>>> }
>>> @@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
>>> 	u8 vsel;
>>>
>>> 	vsel = voltage_read_reg(info->vp_offs.voltage_reg);
>>> -	pr_notice("curr_vsel = %x\n", vsel);
>>> +	pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
>>> 	*val = vsel * 12500 + 600000;
>>>
>>> 	return 0;
>>> --
>>> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Mark Brown]

On Fri, Aug 06, 2010 at 06:08:03AM -0500, Nishanth Menon wrote:

> Well that is my motivation here. if driver reports a warning to kernel  
> syslog, well, I expect the log to contain the function name for me to  
> maintain faster.

That's really not the expectation for Linux log messages - the general
approach to find the source of a message is generally to just grep for
the message text which is usually very effective.

Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Nishanth Menon]

Mark Brown had written, on 08/06/2010 07:18 AM, the following:
> On Fri, Aug 06, 2010 at 06:08:03AM -0500, Nishanth Menon wrote:
> 
>> Well that is my motivation here. if driver reports a warning to kernel  
>> syslog, well, I expect the log to contain the function name for me to  
>> maintain faster.
> 
> That's really not the expectation for Linux log messages - the general
> approach to find the source of a message is generally to just grep for
> the message text which is usually very effective.

taking a small sample set of pr_xyz(); (pr which spans a single line):

$ git grep pr_ drivers/|grep ")"|grep __func__|wc -l
589
$ git grep pr_ drivers/|grep ")"|grep -v __func__|wc -l
5373
$ git grep pr_fmt drivers/|wc -l
138

Reading Documentation/dynamic-debug-howto.txt, I see that you are in a 
one way right. I can get fine grained control over the log by enabling 
CONFIG_DYNAMIC_DEBUG.

At the same time, I have wondered on the usage of pr_fmt() in many of 
the drivers. in a way, if i wanted to be that verbose in a driver, I 
could in theory do:
#define pr_fmt(fmt) "%s:" fmt, __func__ and get the benefit throughout 
the file..

but overall, I still disagree that we dont need to have the function 
name in log is a speed booster for maintenance folks.
a) when the strings are split up when they go multiple lines:
E.g.:
	"abcd "
	"efg"
print comes out abcd efg
i) you do a git grep "abcd efg" will return nada
ii) if you do a git grep of "abcd", you will probably see half a dozen 
prints there, then open each file to see where the "real" message is, 
and you find the file searching a bit


b) when there are couple more usage in cases of commonly used error 
message- (e.g. invalid parameters),  you end up getting multiple hits, 
and you are left guessing where it came from

in this example: lets see: (on l-o pm branch):
git grep "DEBUG option not enabled" .
arch/arm/mach-omap2/smartreflex.c:              pr_notice("DEBUG option 
not enabled!\n  \
arch/arm/mach-omap2/voltage.c:          pr_notice("DEBUG option not 
enabled!\n  \

now open up both the files to find exactly what you are looking for.

c) time required:
$ time git grep "DEBUG option not enabled" .
arch/arm/mach-omap2/smartreflex.c:              pr_notice("DEBUG option 
not enabled!\n  \
arch/arm/mach-omap2/voltage.c:          pr_notice("DEBUG option not 
enabled!\n  \

real	1m34.722s
user	0m0.440s
sys	0m1.820s

Vs cscope or ctags where it is rather instantaneous if you know the 
function name..
-- 
Regards,
Nishanth Menon

Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Mark Brown]

On Fri, Aug 06, 2010 at 08:10:44AM -0500, Nishanth Menon wrote:

> a) when the strings are split up when they go multiple lines:
> E.g.:
> 	"abcd "
> 	"efg"
> print comes out abcd efg

That's generally considered bad practice for precisely this reason.

> c) time required:
> $ time git grep "DEBUG option not enabled" .
> arch/arm/mach-omap2/smartreflex.c:              pr_notice("DEBUG
> option not enabled!\n  \
> arch/arm/mach-omap2/voltage.c:          pr_notice("DEBUG option not
> enabled!\n  \

> real	1m34.722s
> user	0m0.440s
> sys	0m1.820s

> Vs cscope or ctags where it is rather instantaneous if you know the
> function name..

Interesting, I see better performance than that, especially hot cache,
and of course an educated guess as to where to look helps greatly.

Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Nishanth Menon]

Mark Brown had written, on 08/06/2010 08:32 AM, the following:
> On Fri, Aug 06, 2010 at 08:10:44AM -0500, Nishanth Menon wrote:
> 
>> a) when the strings are split up when they go multiple lines:
>> E.g.:
>> 	"abcd "
>> 	"efg"
>> print comes out abcd efg
> 
> That's generally considered bad practice for precisely this reason.

I agree, but we dont normally have an cleaner option other than:
a) reduce the size of the print message
b) reduce the nesting to give us more space to print ;)

but things get a little more worse now that I think of my personal 
experience,
e.g.:
pr_info("%s is not active\n", my_struct->name);

if name is "mpu", then you get a print of:
mpu is not active, and a git grep does not match up either...


> 
>> c) time required:
>> $ time git grep "DEBUG option not enabled" .
>> arch/arm/mach-omap2/smartreflex.c:              pr_notice("DEBUG
>> option not enabled!\n  \
>> arch/arm/mach-omap2/voltage.c:          pr_notice("DEBUG option not
>> enabled!\n  \
> 
>> real	1m34.722s
>> user	0m0.440s
>> sys	0m1.820s
> 
>> Vs cscope or ctags where it is rather instantaneous if you know the
>> function name..
> 
> Interesting, I see better performance than that, especially hot cache,
> and of course an educated guess as to where to look helps greatly.
tell me about it.. ;) mebbe I can use this thread to get a machine 
upgrade budget :P.. but I guess there are a lot more folks stuck like me..
just for curiosity sake, this data was on a dual core
model name	: Intel(R) Pentium(R) 4 CPU 3.40GHz
bogomips	: 6800.29

with a 7200RPM SATA 500GB drive :(..

-- 
Regards,
Nishanth Menon

Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Mark Brown]

On Fri, Aug 06, 2010 at 08:37:52AM -0500, Nishanth Menon wrote:

> >Interesting, I see better performance than that, especially hot cache,
> >and of course an educated guess as to where to look helps greatly.

> tell me about it.. ;) mebbe I can use this thread to get a machine
> upgrade budget :P.. but I guess there are a lot more folks stuck
> like me..
> just for curiosity sake, this data was on a dual core
> model name	: Intel(R) Pentium(R) 4 CPU 3.40GHz
> bogomips	: 6800.29

> with a 7200RPM SATA 500GB drive :(..

That's actually better in most respects than the laptop I tried for
comparison.

RE: [PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error

[Gopinath, Thara]

>>-----Original Message-----
>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
>>Nishanth Menon
>>Sent: Friday, August 06, 2010 4:30 PM
>>To: Gopinath, Thara
>>Cc: Menon, Nishanth; linux-omap; Kevin Hilman
>>Subject: Re: [PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error
>>
>>On 08/06/2010 02:24 AM, Gopinath, Thara wrote:
>>>
>>>
>>>>> -----Original Message-----
>>>>> From: Menon, Nishanth
>>>>> Sent: Friday, August 06, 2010 3:54 AM
>>>>> To: linux-omap
>>>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>>>> Subject: [PM-SR][PATCH 06/12] omap3: sr: device: fail sr_dev_init should return error
>>>>>
>>>>> sr_dev_init should return error on error conditions
>>>>>
>>>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>>>> Cc: Thara Gopinath<thara@ti.com>
>>>>>
>>>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>>>> ---
>>>>> arch/arm/mach-omap2/sr_device.c |    3 ++-
>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
>>>>> index 6f70da6..8fb60d8 100644
>>>>> --- a/arch/arm/mach-omap2/sr_device.c
>>>>> +++ b/arch/arm/mach-omap2/sr_device.c
>>>>> @@ -162,7 +162,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>>>>> 				__func__, i + 1);
>>>>> 		i++;
>>>>> 		kfree(sr_data);
>>>>> -		return 0;
>>>>> +		return -ENODATA;
>>>>> 	}
>>>>> 	sr_set_nvalues(sr_dev_data, sr_data);
>>>>> 	od = omap_device_build(name, i, oh, sr_data, sizeof(*sr_data),
>>>>> @@ -172,6 +172,7 @@ static int sr_dev_init(struct omap_hwmod *oh, void *user)
>>>>> 		pr_warning("%s: Could not build omap_device for %s: %s.\n\n",
>>>>> 			__func__, name, oh->name);
>>>>> 		kfree(sr_data);
>>>>> +		return PTR_ERR(od);
>>>>> 	}
>>> NAK for this change.
>>> This API is called from omap_hwmod_for_each_by_class for every smartreflex module.
>>> If This API returns an error omap_hwmod_for_each_by_class will exit without looping over
>>> rest of the smartreflex modules. This means a error for one smartreflex module will prevent
>>> rest of the smartreflex modules to be initialized. We do not want this. Hence this API returns 0
>>> for non-availability of data for a smartreflex module.
>>
>>why do we want this behavior(aka continue with as many modules as you
>>can enable)? h/wmod data structure are NOT meant to be corrupted if they
>>are, what guarentee do we have that the rest of the sr module data
>>structures have the right hwmods(even if they passed device_build?).. if
>>they are, what is the point in enabling SR in half the domains - we
>>should flag this as an error to developer and get him to fix it instead
>>of encouraging this slipping by half a dozen developers as only sr_l3
>>failed or something similar..

It could be a typo that is causing one hwmod to fail. Also I disagree with the point that we
cannot enable sr for one voltage domain alone. There are two or three separate voltage domains in
the system so that they can be controlled and configured independent of each other.
Also if one of the sr modules fail registration we do flag the error in today's code plus if the
debug fs is enabled the entry for the failed sr module will be missing. I think that is enough
of error handling and flagging. 

Regards
Thara

>>
>>Regards,
>>Nishanth Menon
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

[Gopinath, Thara]

>>-----Original Message-----
>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
>>Nishanth Menon
>>Sent: Friday, August 06, 2010 4:38 PM
>>To: Gopinath, Thara
>>Cc: Menon, Nishanth; linux-omap; Kevin Hilman
>>Subject: Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
>>
>>On 08/06/2010 02:42 AM, Gopinath, Thara wrote:
>>>
>>>
>>>>> -----Original Message-----
>>>>> From: Menon, Nishanth
>>>>> Sent: Friday, August 06, 2010 3:54 AM
>>>>> To: linux-omap
>>>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>>>> Subject: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
>>>>>
>>>>> Few more pr_xxxx need cleanup for printing the function name and
>>>>> not using multiline prints when c allows us to do "".
>>>>>
>>>>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
>>>>> Cc: Thara Gopinath<thara@ti.com>
>>>>>
>>>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>>>> ---
>>>>> arch/arm/mach-omap2/voltage.c |    7 ++++---
>>>>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>>>> index 148e4d4..3431fa3 100644
>>>>> --- a/arch/arm/mach-omap2/voltage.c
>>>>> +++ b/arch/arm/mach-omap2/voltage.c
>>>>> @@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
>>>>> 		u32 *option = data;
>>>>> 		*option = val;
>>>>> 	} else {
>>>>> -		pr_notice("DEBUG option not enabled!\n	\
>>>>> -			echo 1>  pm_debug/enable_sr_vp_debug - to enable\n");
>>>>> +		pr_notice("%s: DEBUG option not enabled! "
>>>>> +			"echo 1>  pm_debug/enable_sr_vp_debug - to enable\n",
>>>>> +			__func__);
>>>>> 	}
>>>
>>> I do not think this is needed as these fns get called only from user space.
>>>
>>
>>have you tried working on someone else's unfamiliar code and grepping
>>for a warning message at the middle of the night because some developer
>>forgot to give a "%s:...", __func__ with half a dozen people watching
>>behind your back to know it's meaning because the product is hitting the
>>shelves the next day? Instead of being able to use cscope and pop the
>>function in and go straight to it and understand what is happening?? I
>>believe there are few in this list who had the fortune to be in that sitn..
>>
>>
>>Well that is my motivation here. if driver reports a warning to kernel
>>syslog, well, I expect the log to contain the function name for me to
>>maintain faster.

Bingo!! Kudos on managing to prove that you are a cut above the rest. Poor moi
wont understand at all as moi do not work at all!!
But unless there are others in the community who feel that this is absolutely
necessary I am not pulling this in. For me you do a 
cat /debug/pm_debg<some entry> and get an error it is easy to figure out in the kernel
with the limited experience I have working on linux kernel.

Regards
Thara

>>
>>> Regards
>>> Thara
>>>
>>>>> 	return 0;
>>>>> }
>>>>> @@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
>>>>> 	u8 vsel;
>>>>>
>>>>> 	vsel = voltage_read_reg(info->vp_offs.voltage_reg);
>>>>> -	pr_notice("curr_vsel = %x\n", vsel);
>>>>> +	pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
>>>>> 	*val = vsel * 12500 + 600000;
>>>>>
>>>>> 	return 0;
>>>>> --
>>>>> 1.6.3.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html