[PM-OPP][PATCH 0/4] OMAP: Clean up series for generic opp layer.

[PM-OPP][PATCH 0/4] OMAP: Clean up series for generic opp layer.

[Thara Gopinath]

This patch series does some clean up of the opp layer
including removal of compilation warnings, avoiding wrong inclusioin
of header files, correcting some srror checks and removing the dependency
of opp layer on cpu freq layer.

Apart from all these there is still one more concern i have in this generic
opp layer. This is regarding the separate PMIC opp file opp_twl_tps.c which
today caters to only twl4030 and twl5030 pmic. What is the approach to be
taken if the PMIC changes? I am already facing this issue with OMAP4 where
the PMIC is twl6030 and the formulas for converting vsel into voltage and
vice versa are different. I am against adding another file for twl6030. The
approach is not scalable. We need to keep these vsel to uV and vice versa
convertion in one place or make them functions pointers.

Thara Gopinath (4):
  OMAP: Fix the compilation warning in the opp layer
  OMAP: Correct the return value check after call into find_device_opp
  OMAP: Remove inclusion of PMIC specific header file in generic opp
    layer.
  OMAP: Remove dependency of generic opp layer on cpufreq.

 arch/arm/plat-omap/Makefile           |    4 ++--
 arch/arm/plat-omap/include/plat/opp.h |    4 ++--
 arch/arm/plat-omap/opp.c              |    7 +++----
 3 files changed, 7 insertions(+), 8 deletions(-)

[PM-OPP][PATCH 1/4] OMAP: Fix the compilation warning in the opp layer

[Thara Gopinath]

Fix the following warning from the opp layer during
compilation.
arch/arm/plat-omap/opp.c: In function 'opp_enable':
arch/arm/plat-omap/opp.c:405: warning: unused variable 'dev_opp'

Signed-off-by: Thara Gopinath <thara@ti.com>
---
 arch/arm/plat-omap/opp.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index 0273497..a06b88d 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -402,8 +402,6 @@ int opp_add(const struct omap_opp_def *opp_def)
  */
 int opp_enable(struct omap_opp *opp)
 {
-	struct device_opp *dev_opp;
-
 	if (unlikely(!opp || IS_ERR(opp))) {
 		pr_err("%s: Invalid parameters being passed\n", __func__);
 		return -EINVAL;
-- 
1.7.0.rc1.33.g07cf0f

[PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp

[Thara Gopinath]

Earlier we were checking on !dev_opp where as find_device_opp returns
a error pointer in case of error. So correcting the check as in the earlier
code even if find_device_opp returns an error opp_init_cpufreq_table
was not exiting.

Signed-off-by: Thara Gopinath <thara@ti.com>
---
 arch/arm/plat-omap/opp.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index a06b88d..d88a2e0 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -457,8 +457,10 @@ void opp_init_cpufreq_table(struct device *dev,
 	int i = 0;
 
 	dev_opp = find_device_opp(dev);
-	if (WARN_ON(!dev_opp))
+	if (IS_ERR(dev_opp)) {
+		WARN_ON(1);
 		return;
+	}
 
 	freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) *
 			     (dev_opp->enabled_opp_count + 1), GFP_ATOMIC);
-- 
1.7.0.rc1.33.g07cf0f

[PM-OPP][PATCH 3/4] OMAP: Remove inclusion of PMIC specific header file in generic opp layer.

[Thara Gopinath]

This patch removes the inclusion of plat/opp_twl_tps.h in opp.c.
This header file is included unnecessarily and creats unwanted
dependency between the generic opp layer and TWL4030/5030 PMIC
dependent code.

Signed-off-by: Thara Gopinath <thara@ti.com>
---
 arch/arm/plat-omap/opp.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index d88a2e0..1aea69e 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -19,7 +19,6 @@
 #include <linux/err.h>
 #include <linux/list.h>
 
-#include <plat/opp_twl_tps.h>
 #include <plat/opp.h>
 #include <plat/omap_device.h>
 
-- 
1.7.0.rc1.33.g07cf0f

[PM-OPP][PATCH 4/4] OMAP: Remove dependency of generic opp layer on cpufreq.

[Thara Gopinath]

This patch removes the dependency of the opp layer on cpufreq layer.
OPP layer is now enabled to compile and exist in the system
irrespective of whether cpu freq layer is enabled or not.

Signed-off-by: Thara Gopinath <thara@ti.com>
---
 arch/arm/plat-omap/Makefile           |    4 ++--
 arch/arm/plat-omap/include/plat/opp.h |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
index faf831d..852fa33 100644
--- a/arch/arm/plat-omap/Makefile
+++ b/arch/arm/plat-omap/Makefile
@@ -13,7 +13,7 @@ obj-  :=
 obj-$(CONFIG_ARCH_OMAP16XX) += ocpi.o
 
 # OPP support in (OMAP3+ only at the moment)
-ifdef CONFIG_CPU_FREQ
+ifdef CONFIG_PM
 obj-$(CONFIG_ARCH_OMAP3) += opp.o
 obj-$(CONFIG_TWL4030_CORE) += opp_twl_tps.o
 endif
@@ -37,4 +37,4 @@ obj-y += $(i2c-omap-m) $(i2c-omap-y)
 # OMAP mailbox framework
 obj-$(CONFIG_OMAP_MBOX_FWK) += mailbox.o
 
-obj-$(CONFIG_OMAP_PM_NOOP) += omap-pm-noop.o
\ No newline at end of file
+obj-$(CONFIG_OMAP_PM_NOOP) += omap-pm-noop.o
diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-omap/include/plat/opp.h
index 29e3d03..7d3e0ef 100644
--- a/arch/arm/plat-omap/include/plat/opp.h
+++ b/arch/arm/plat-omap/include/plat/opp.h
@@ -60,7 +60,7 @@ struct omap_opp_def {
 
 struct omap_opp;
 
-#ifdef CONFIG_CPU_FREQ
+#ifdef CONFIG_PM
 
 unsigned long opp_get_voltage(const struct omap_opp *opp);
 
@@ -155,5 +155,5 @@ void opp_init_cpufreq_table(struct omap_opp *opps,
 {
 }
 
-#endif		/* CONFIG_CPU_FREQ */
+#endif		/* CONFIG_PM */
 #endif		/* __ASM_ARM_OMAP_OPP_H */
-- 
1.7.0.rc1.33.g07cf0f

Re: [PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp

[Nishanth Menon]

Gopinath, Thara had written, on 07/13/2010 12:47 AM, the following:
> Earlier we were checking on !dev_opp where as find_device_opp returns
> a error pointer in case of error. So correcting the check as in the earlier
> code even if find_device_opp returns an error opp_init_cpufreq_table
> was not exiting.
> 
> Signed-off-by: Thara Gopinath <thara@ti.com>
> ---
>  arch/arm/plat-omap/opp.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
> index a06b88d..d88a2e0 100644
> --- a/arch/arm/plat-omap/opp.c
> +++ b/arch/arm/plat-omap/opp.c
> @@ -457,8 +457,10 @@ void opp_init_cpufreq_table(struct device *dev,
>  	int i = 0;
>  
>  	dev_opp = find_device_opp(dev);
> -	if (WARN_ON(!dev_opp))
> +	if (IS_ERR(dev_opp)) {
> +		WARN_ON(1);
could we use pr_warning here instead of WARN_ON?

>  		return;
> +	}
>  
>  	freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) *
>  			     (dev_opp->enabled_opp_count + 1), GFP_ATOMIC);


-- 
Regards,
Nishanth Menon

RE: [PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Tuesday, July 13, 2010 5:53 PM
>>To: Gopinath, Thara
>>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com; paul@pwsan.com; tony@atomide.com;
>>Cousson, Benoit; Sawant, Anand; Sripathy, Vishwanath; Premi, Sanjeev
>>Subject: Re: [PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp
>>
>>Gopinath, Thara had written, on 07/13/2010 12:47 AM, the following:
>>> Earlier we were checking on !dev_opp where as find_device_opp returns
>>> a error pointer in case of error. So correcting the check as in the earlier
>>> code even if find_device_opp returns an error opp_init_cpufreq_table
>>> was not exiting.
>>>
>>> Signed-off-by: Thara Gopinath <thara@ti.com>
>>> ---
>>>  arch/arm/plat-omap/opp.c |    4 +++-
>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>>> index a06b88d..d88a2e0 100644
>>> --- a/arch/arm/plat-omap/opp.c
>>> +++ b/arch/arm/plat-omap/opp.c
>>> @@ -457,8 +457,10 @@ void opp_init_cpufreq_table(struct device *dev,
>>>  	int i = 0;
>>>
>>>  	dev_opp = find_device_opp(dev);
>>> -	if (WARN_ON(!dev_opp))
>>> +	if (IS_ERR(dev_opp)) {
>>> +		WARN_ON(1);
>>could we use pr_warning here instead of WARN_ON?
The WARN_ON was already in the code. Added by you or Kevin. I just corrected the error checking.

Regards
Thara
>>
>>>  		return;
>>> +	}
>>>
>>>  	freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) *
>>>  			     (dev_opp->enabled_opp_count + 1), GFP_ATOMIC);
>>
>>
>>--
>>Regards,
>>Nishanth Menon

Re: [PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp

[Nishanth Menon]

Gopinath, Thara had written, on 07/13/2010 09:32 AM, the following:
> 
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Tuesday, July 13, 2010 5:53 PM
>>> To: Gopinath, Thara
>>> Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com; paul@pwsan.com; tony@atomide.com;
>>> Cousson, Benoit; Sawant, Anand; Sripathy, Vishwanath; Premi, Sanjeev
>>> Subject: Re: [PM-OPP][PATCH 2/4] OMAP: Correct the return value check after call into find_device_opp
>>>
>>> Gopinath, Thara had written, on 07/13/2010 12:47 AM, the following:
>>>> Earlier we were checking on !dev_opp where as find_device_opp returns
>>>> a error pointer in case of error. So correcting the check as in the earlier
>>>> code even if find_device_opp returns an error opp_init_cpufreq_table
>>>> was not exiting.
>>>>
>>>> Signed-off-by: Thara Gopinath <thara@ti.com>
>>>> ---
>>>>  arch/arm/plat-omap/opp.c |    4 +++-
>>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>>>> index a06b88d..d88a2e0 100644
>>>> --- a/arch/arm/plat-omap/opp.c
>>>> +++ b/arch/arm/plat-omap/opp.c
>>>> @@ -457,8 +457,10 @@ void opp_init_cpufreq_table(struct device *dev,
>>>>  	int i = 0;
>>>>
>>>>  	dev_opp = find_device_opp(dev);
>>>> -	if (WARN_ON(!dev_opp))
>>>> +	if (IS_ERR(dev_opp)) {
>>>> +		WARN_ON(1);
>>> could we use pr_warning here instead of WARN_ON?
> The WARN_ON was already in the code. Added by you or Kevin. I just corrected the error checking.

:) using a WARN_ON() for an operational code is not a good idea - who 
ever added it (git annotate can help you there). WARN_ON(1) give no 
useful information when debugging in the field - just gives a line 
number and function name which changes btw.. useful info is something like:
pr_warning("%s: failed to find device\n", __func__);

useful when someone posts a log and we need to debug such logic.. esp 
useful after a period of time.

> 
> Regards
> Thara
>>>>  		return;
>>>> +	}
>>>>
>>>>  	freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) *
>>>>  			     (dev_opp->enabled_opp_count + 1), GFP_ATOMIC);
>>>
>>> --
>>> Regards,
>>> Nishanth Menon


-- 
Regards,
Nishanth Menon

Re: [PM-OPP][PATCH 0/4] OMAP: Clean up series for generic opp layer.

[Nishanth Menon]

Thara Gopinath had written, on 07/13/2010 12:47 AM, the following:
> This patch series does some clean up of the opp layer
> including removal of compilation warnings, avoiding wrong inclusioin
> of header files, correcting some srror checks and removing the dependency
> of opp layer on cpu freq layer.
> 
> Apart from all these there is still one more concern i have in this generic
> opp layer. This is regarding the separate PMIC opp file opp_twl_tps.c which
> today caters to only twl4030 and twl5030 pmic. What is the approach to be
> taken if the PMIC changes? I am already facing this issue with OMAP4 where
> the PMIC is twl6030 and the formulas for converting vsel into voltage and
> vice versa are different. I am against adding another file for twl6030. The
> approach is not scalable. We need to keep these vsel to uV and vice versa
> convertion in one place or make them functions pointers.
why is opp_twl_tps.c unable to decide the vsel conversion routines based 
on twl version? it seems to me (only watching l-o - not dug inside 
twl6030 datamanual), that rest of 6030 code seem to share 5030 codebase 
mostly..

on a side note - this is a bigger problem for voltage.c which seem to 
have some ingrained idea that each step is 12.5mV and delays are coded 
in.. but not related to opp layer at all.

> 
> Thara Gopinath (4):
>   OMAP: Fix the compilation warning in the opp layer
>   OMAP: Correct the return value check after call into find_device_opp
>   OMAP: Remove inclusion of PMIC specific header file in generic opp
>     layer.
>   OMAP: Remove dependency of generic opp layer on cpufreq.

This specific series once the minor comment provided is looking good to me.

> 
>  arch/arm/plat-omap/Makefile           |    4 ++--
>  arch/arm/plat-omap/include/plat/opp.h |    4 ++--
>  arch/arm/plat-omap/opp.c              |    7 +++----
>  3 files changed, 7 insertions(+), 8 deletions(-)
> 
> --
> 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


-- 
Regards,
Nishanth Menon

RE: [PM-OPP][PATCH 0/4] OMAP: Clean up series for generic opp layer.

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Tuesday, July 13, 2010 9:23 PM
>>To: Gopinath, Thara
>>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com; paul@pwsan.com; tony@atomide.com;
>>Cousson, Benoit; Sawant, Anand; Sripathy, Vishwanath; Premi, Sanjeev
>>Subject: Re: [PM-OPP][PATCH 0/4] OMAP: Clean up series for generic opp layer.
>>
>>Thara Gopinath had written, on 07/13/2010 12:47 AM, the following:
>>> This patch series does some clean up of the opp layer
>>> including removal of compilation warnings, avoiding wrong inclusioin
>>> of header files, correcting some srror checks and removing the dependency
>>> of opp layer on cpu freq layer.
>>>
>>> Apart from all these there is still one more concern i have in this generic
>>> opp layer. This is regarding the separate PMIC opp file opp_twl_tps.c which
>>> today caters to only twl4030 and twl5030 pmic. What is the approach to be
>>> taken if the PMIC changes? I am already facing this issue with OMAP4 where
>>> the PMIC is twl6030 and the formulas for converting vsel into voltage and
>>> vice versa are different. I am against adding another file for twl6030. The
>>> approach is not scalable. We need to keep these vsel to uV and vice versa
>>> convertion in one place or make them functions pointers.
>>why is opp_twl_tps.c unable to decide the vsel conversion routines based
>>on twl version? it seems to me (only watching l-o - not dug inside
>>twl6030 datamanual), that rest of 6030 code seem to share 5030 codebase
>>mostly..
>>
>>on a side note - this is a bigger problem for voltage.c which seem to
>>have some ingrained idea that each step is 12.5mV and delays are coded
>>in.. but not related to opp layer at all.
Nope voltage.c ideally uses the opp_tpl_twl layer to get the vsel to uV and uV to vsel conversion. I think today there a couple of places where it does a manual calculation but that will be removed in the next version.
All the delays are #defines and can be changed whenever needed. Also I am no sure if u saw the latest implementation where I have a hook to get the step size and slew rate from the PMIC.

Regards
Thara

>>
>>>
>>> Thara Gopinath (4):
>>>   OMAP: Fix the compilation warning in the opp layer
>>>   OMAP: Correct the return value check after call into find_device_opp
>>>   OMAP: Remove inclusion of PMIC specific header file in generic opp
>>>     layer.
>>>   OMAP: Remove dependency of generic opp layer on cpufreq.
>>
>>This specific series once the minor comment provided is looking good to me.
>>
>>>
>>>  arch/arm/plat-omap/Makefile           |    4 ++--
>>>  arch/arm/plat-omap/include/plat/opp.h |    4 ++--
>>>  arch/arm/plat-omap/opp.c              |    7 +++----
>>>  3 files changed, 7 insertions(+), 8 deletions(-)
>>>
>>> --
>>> 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
>>
>>
>>--
>>Regards,
>>Nishanth Menon