Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
"Gopinath, Thara" writes: >>>-Original Message- >>>From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >>>Sent: Friday, October 22, 2010 10:22 PM >>>To: Gopinath, Thara >>>Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Sripathy, >>>Vishwanath; Sawant, Anand >>>Subject: Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and >>>Smartreflex drivers >>> >>>"Gopinath, Thara" writes: >>> >>>>>>Thara Gopinath writes: >>>>>> >>>>>>> This patch adds debug support to the voltage and smartreflex drivers. >>>>>>> This means a whole bunch of voltage processor and smartreflex >>>>>>> parameters are now visible through the pm debugfs. By default >>>>>>> only a read of these parameters are permitted. If you need to >>>>>>> write into them then >>>>>>> echo 1 > /pm_debug/enable_sr_vp_debug >>>>>> >>>>>>Why a read-only interface by default? As a debug interface it seems >>>>>>redundant to have to enable it. >>>>>> >>>> >>>> Read-only interface by default so that we can read these values from >>>> user space even if we do not want to manipulate it from user-side. >>>> >>> >>>If we do not want to manipulate it from the user-side, then simply don't >>>write to it. Remember, this is a debug interface, not a primary >>>interface. >>> >>>I think the enable_sr_vp_debug flag should disappear, and it should be a >>>read/write interface. >>> >>>If the values are changed via debugfs, then set some per-SR instance >>>flag that can be checked. >>> >>>Basically, the current code is confusing because you're using the the >>>flag called 'enable' to determine whether the user *might have* written >>>the values. >>> >>>[...] >>> >>>>>>> + /* >>>>>>> +* Getting vp errorgain based on the voltage. If the debug option >>>is >>>>>>> +* enabled allow the override of errorgain from user side. >>>>>>> +*/ >>>>>> >>>>>>As suggested in earlier comment, please use a specific flag that this >>>>>>has been overridden instead of the 'debug enabled' flag (which should >>>>>>disappear, IMO) >>>> >>>> What do you mean by a separate flag. You want a flag to be maintained >>>> for just this purpose ? >>> >>>Yes. I want a flag to be maintained *specifically* for this purpose, >>>instead of using a much more general flag that only means a user *might* >>>have overridden the values, use one that specifically means a user *has* >>>overridden the values. > > Hello Kevin, > > I tried this. Couple of questions/concerns I have. > 1. If you take a look at the definition of these debugfs parameters, the > omap_vdd_info struct is not passed as an argument. The actual variables are > the parameters. I am not sure how to extract omap_vdd_info from this. Maybe > container_of will help, but then it will be clumsy. Same concern for > smartreflex err_minlimit variable. There is no way to get the sr instance > except use container of which I am not sure will work or not > 2.Also in voltage layer we export out eight parameters tat can be over-ridden > from the user side. I do not think we should be maintaining one flag per > variable. The design will be too very clumsy. I didn't mean to suggest one flag per varialble. Just one flag to indicate that the user *has* overridden the defaults. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
Hi Thara,
On 9/22/2010 4:45 PM, Gopinath, Thara wrote:
This patch adds debug support to the voltage and smartreflex drivers.
This means a whole bunch of voltage processor and smartreflex
parameters are now visible through the pm debugfs. By default
only a read of these parameters are permitted. If you need to
write into them then
echo 1> /pm_debug/enable_sr_vp_debug
The voltage parameters can be viewed at
/pm_debug/voltage/vdd_/
and the smartreflex parameters can be viewed at
/pm_debug/smartreflex/sr_/
Can we start getting rid of this pm_debug miscellaneous directory?
Like powerdomain and clockdomain debug stuff, I think that voltage
domain layer deserve its own entry in the debugfs top level directory.
I do not see the need to add a extra level a directory for that.
Moreover smartreflex should be under voltage entry and not in the same
level as voltage.
/sys/kernel/debug/voltage/vdd_/smartreflex/
Regards,
Benoit
where is mpu or core for OMAP3.
Signed-off-by: Thara Gopinath
---
arch/arm/mach-omap2/pm-debug.c| 15 ++
arch/arm/mach-omap2/smartreflex.c | 40 +-
arch/arm/mach-omap2/voltage.c | 210 -
arch/arm/plat-omap/include/plat/smartreflex.h |1 -
arch/arm/plat-omap/include/plat/voltage.h |5 +
5 files changed, 264 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index 390f1c6..879efb2 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include "prm.h"
#include "cm.h"
@@ -586,6 +587,18 @@ static int option_set(void *data, u64 val)
omap3_pm_off_mode_enable(val);
}
+ if (option ==&enable_sr_vp_debug&& val)
+ pr_notice("Beware that enabling this option will allow user "
+ "to override the system defined vp and sr parameters "
+ "All the updated parameters will take effect next "
+ "time smartreflex is enabled. Also this option "
+ "disables the automatic vp errorgain and sr errormin "
+ "limit changes as per the voltage. Users will have "
+ "to explicitly write values into the debug fs "
+ "entries corresponding to these if they want to see "
+ "them changing according to the VDD voltage\n");
+
+
return 0;
}
@@ -642,6 +655,8 @@ static int __init pm_dbg_init(void)
(void) debugfs_create_file("wakeup_timer_milliseconds",
S_IRUGO | S_IWUGO, d,&wakeup_timer_milliseconds,
&pm_dbg_option_fops);
+ (void) debugfs_create_file("enable_sr_vp_debug", S_IRUGO | S_IWUGO, d,
+ &enable_sr_vp_debug,&pm_dbg_option_fops);
pm_dbg_main_dir = d;
pm_dbg_init_done = 1;
diff --git a/arch/arm/mach-omap2/smartreflex.c
b/arch/arm/mach-omap2/smartreflex.c
index 7fc3138..b5a7878 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -558,8 +558,13 @@ int sr_enable(struct voltagedomain *voltdm, unsigned long
volt)
return -ENODATA;
}
- /* errminlimit is opp dependent and hence linked to voltage */
- sr->err_minlimit = volt_data->sr_errminlimit;
+ /*
+* 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->sr_enable) {
@@ -811,9 +816,34 @@ static int omap_sr_autocomp_store(void *data, u64 val)
return 0;
}
+static int omap_sr_params_show(void *data, u64 *val)
+{
+ u32 *param = data;
+
+ *val = *param;
+ return 0;
+}
+
+static int omap_sr_params_store(void *data, u64 val)
+{
+ if (enable_sr_vp_debug) {
+ u32 *option = data;
+ *option = val;
+ } else {
+ pr_notice("%s: DEBUG option not enabled!\n \
+ echo 1> pm_debug/enable_sr_vp_debug - to enable\n",
+ __func__);
+ }
+
+ return 0;
+}
+
DEFINE_SIMPLE_ATTRIBUTE(pm_sr_fops, omap_sr_autocomp_show,
omap_sr_autocomp_store, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(sr_params_fops, omap_sr_params_show,
+ omap_sr_params_store, "%llu\n");
+
static int __init omap_smartreflex_probe(struct platform_device *pdev)
{
struct omap_sr *sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL);
@@ -907,6 +937,12 @@ static int __init omap_smartreflex_probe(struct
platform_device *pdev)
(void) debugfs_create_file("autocomp
RE: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
>>-Original Message- >>From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >>Sent: Friday, October 22, 2010 10:22 PM >>To: Gopinath, Thara >>Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Sripathy, >>Vishwanath; Sawant, Anand >>Subject: Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and >>Smartreflex drivers >> >>"Gopinath, Thara" writes: >> >>>>>Thara Gopinath writes: >>>>> >>>>>> This patch adds debug support to the voltage and smartreflex drivers. >>>>>> This means a whole bunch of voltage processor and smartreflex >>>>>> parameters are now visible through the pm debugfs. By default >>>>>> only a read of these parameters are permitted. If you need to >>>>>> write into them then >>>>>> echo 1 > /pm_debug/enable_sr_vp_debug >>>>> >>>>>Why a read-only interface by default? As a debug interface it seems >>>>>redundant to have to enable it. >>>>> >>> >>> Read-only interface by default so that we can read these values from >>> user space even if we do not want to manipulate it from user-side. >>> >> >>If we do not want to manipulate it from the user-side, then simply don't >>write to it. Remember, this is a debug interface, not a primary >>interface. >> >>I think the enable_sr_vp_debug flag should disappear, and it should be a >>read/write interface. >> >>If the values are changed via debugfs, then set some per-SR instance >>flag that can be checked. >> >>Basically, the current code is confusing because you're using the the >>flag called 'enable' to determine whether the user *might have* written >>the values. >> >>[...] >> >>>>>> + /* >>>>>> +* Getting vp errorgain based on the voltage. If the debug option >>is >>>>>> +* enabled allow the override of errorgain from user side. >>>>>> +*/ >>>>> >>>>>As suggested in earlier comment, please use a specific flag that this >>>>>has been overridden instead of the 'debug enabled' flag (which should >>>>>disappear, IMO) >>> >>> What do you mean by a separate flag. You want a flag to be maintained >>> for just this purpose ? >> >>Yes. I want a flag to be maintained *specifically* for this purpose, >>instead of using a much more general flag that only means a user *might* >>have overridden the values, use one that specifically means a user *has* >>overridden the values. Hello Kevin, I tried this. Couple of questions/concerns I have. 1. If you take a look at the definition of these debugfs parameters, the omap_vdd_info struct is not passed as an argument. The actual variables are the parameters. I am not sure how to extract omap_vdd_info from this. Maybe container_of will help, but then it will be clumsy. Same concern for smartreflex err_minlimit variable. There is no way to get the sr instance except use container of which I am not sure will work or not 2.Also in voltage layer we export out eight parameters tat can be over-ridden from the user side. I do not think we should be maintaining one flag per variable. The design will be too very clumsy. Regards Thara >> >>Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
"Gopinath, Thara" writes: >>>Thara Gopinath writes: >>> This patch adds debug support to the voltage and smartreflex drivers. This means a whole bunch of voltage processor and smartreflex parameters are now visible through the pm debugfs. By default only a read of these parameters are permitted. If you need to write into them then echo 1 > /pm_debug/enable_sr_vp_debug >>> >>>Why a read-only interface by default? As a debug interface it seems >>>redundant to have to enable it. >>> > > Read-only interface by default so that we can read these values from > user space even if we do not want to manipulate it from user-side. > If we do not want to manipulate it from the user-side, then simply don't write to it. Remember, this is a debug interface, not a primary interface. I think the enable_sr_vp_debug flag should disappear, and it should be a read/write interface. If the values are changed via debugfs, then set some per-SR instance flag that can be checked. Basically, the current code is confusing because you're using the the flag called 'enable' to determine whether the user *might have* written the values. [...] + /* +* Getting vp errorgain based on the voltage. If the debug option is +* enabled allow the override of errorgain from user side. +*/ >>> >>>As suggested in earlier comment, please use a specific flag that this >>>has been overridden instead of the 'debug enabled' flag (which should >>>disappear, IMO) > > What do you mean by a separate flag. You want a flag to be maintained > for just this purpose ? Yes. I want a flag to be maintained *specifically* for this purpose, instead of using a much more general flag that only means a user *might* have overridden the values, use one that specifically means a user *has* overridden the values. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
>>-Original Message- >>From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- >>ow...@vger.kernel.org] On Behalf Of Kevin Hilman >>Sent: Friday, October 15, 2010 12:51 AM >>To: Gopinath, Thara >>Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Sripathy, >>Vishwanath; Sawant, Anand >>Subject: Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and >>Smartreflex drivers >> >>On Wed, 2010-09-22 at 20:15 +0530, Thara Gopinath wrote: >>> --- a/arch/arm/mach-omap2/smartreflex.c >>> +++ b/arch/arm/mach-omap2/smartreflex.c >>> @@ -558,8 +558,13 @@ int sr_enable(struct voltagedomain *voltdm, >>> unsigned long volt) >>> return -ENODATA; >>> } >>> >>> - /* errminlimit is opp dependent and hence linked to voltage */ >>> - sr->err_minlimit = volt_data->sr_errminlimit; >>> + /* >>> +* 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 >>> +*/ >> >>this comment needs better punctuation to be clearer Will do >> >>> + if (!enable_sr_vp_debug) >>> + sr->err_minlimit = volt_data->sr_errminlimit; >> >>Rather than checking if debug is enabled, you should create a flag >>specific to whether this value was overwritten by the user, and use that >>flag instead. mmm.. See my comment regarding flag and enable_sr_vp_debug in the other patch Regards Thara >> >>Kevin >> >> >>-- >>To unsubscribe from this list: send the line "unsubscribe linux-omap" in >>the body of a message to majord...@vger.kernel.org >>More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
>>-Original Message-
>>From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
>>Sent: Friday, October 15, 2010 5:16 AM
>>To: Gopinath, Thara
>>Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Sripathy,
>>Vishwanath; Sawant, Anand
>>Subject: Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and
>>Smartreflex drivers
>>
>>Thara Gopinath writes:
>>
>>> This patch adds debug support to the voltage and smartreflex drivers.
>>> This means a whole bunch of voltage processor and smartreflex
>>> parameters are now visible through the pm debugfs. By default
>>> only a read of these parameters are permitted. If you need to
>>> write into them then
>>> echo 1 > /pm_debug/enable_sr_vp_debug
>>
>>Why a read-only interface by default? As a debug interface it seems
>>redundant to have to enable it.
Read-only interface by default so that we can read these values from user space
even if we do not want to manipulate it from user-side.
>>
>>> The voltage parameters can be viewed at
>>> /pm_debug/voltage/vdd_/
>>> and the smartreflex parameters can be viewed at
>>> /pm_debug/smartreflex/sr_/
>>>
>>> where is mpu or core for OMAP3.
>>>
>>> Signed-off-by: Thara Gopinath
>>> ---
>>> arch/arm/mach-omap2/pm-debug.c| 15 ++
>>> arch/arm/mach-omap2/smartreflex.c | 40 +-
>>> arch/arm/mach-omap2/voltage.c | 210
>>-
>>> arch/arm/plat-omap/include/plat/smartreflex.h |1 -
>>> arch/arm/plat-omap/include/plat/voltage.h |5 +
>>> 5 files changed, 264 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-
>>debug.c
>>> index 390f1c6..879efb2 100644
>>> --- a/arch/arm/mach-omap2/pm-debug.c
>>> +++ b/arch/arm/mach-omap2/pm-debug.c
>>> @@ -32,6 +32,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> #include "prm.h"
>>> #include "cm.h"
>>> @@ -586,6 +587,18 @@ static int option_set(void *data, u64 val)
>>> omap3_pm_off_mode_enable(val);
>>> }
>>>
>>> + if (option == &enable_sr_vp_debug && val)
>>> + pr_notice("Beware that enabling this option will allow user "
>>> + "to override the system defined vp and sr parameters "
>>> + "All the updated parameters will take effect next "
>>> + "time smartreflex is enabled. Also this option "
>>> + "disables the automatic vp errorgain and sr errormin "
>>> + "limit changes as per the voltage. Users will have "
>>> + "to explicitly write values into the debug fs "
>>> + "entries corresponding to these if they want to see "
>>> + "them changing according to the VDD voltage\n");
>>> +
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -642,6 +655,8 @@ static int __init pm_dbg_init(void)
>>> (void) debugfs_create_file("wakeup_timer_milliseconds",
>>> S_IRUGO | S_IWUGO, d, &wakeup_timer_milliseconds,
>>> &pm_dbg_option_fops);
>>> + (void) debugfs_create_file("enable_sr_vp_debug", S_IRUGO | S_IWUGO, d,
>>> + &enable_sr_vp_debug, &pm_dbg_option_fops);
>>>
>>> pm_dbg_main_dir = d;
>>> pm_dbg_init_done = 1;
>>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
>>omap2/smartreflex.c
>>> index 7fc3138..b5a7878 100644
>>> --- a/arch/arm/mach-omap2/smartreflex.c
>>> +++ b/arch/arm/mach-omap2/smartreflex.c
>>> @@ -558,8 +558,13 @@ int sr_enable(struct voltagedomain *voltdm, unsigned
>>long volt)
>>> return -ENODATA;
>>> }
>>>
>>> - /* errminlimit is opp dependent and hence linked to voltage */
>>> - sr->err_minlimit = volt_data->sr_errminlimit;
>>> + /*
>>> +* 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
>>&
Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
Thara Gopinath writes:
> This patch adds debug support to the voltage and smartreflex drivers.
> This means a whole bunch of voltage processor and smartreflex
> parameters are now visible through the pm debugfs. By default
> only a read of these parameters are permitted. If you need to
> write into them then
> echo 1 > /pm_debug/enable_sr_vp_debug
Why a read-only interface by default? As a debug interface it seems
redundant to have to enable it.
> The voltage parameters can be viewed at
> /pm_debug/voltage/vdd_/
> and the smartreflex parameters can be viewed at
> /pm_debug/smartreflex/sr_/
>
> where is mpu or core for OMAP3.
>
> Signed-off-by: Thara Gopinath
> ---
> arch/arm/mach-omap2/pm-debug.c| 15 ++
> arch/arm/mach-omap2/smartreflex.c | 40 +-
> arch/arm/mach-omap2/voltage.c | 210
> -
> arch/arm/plat-omap/include/plat/smartreflex.h |1 -
> arch/arm/plat-omap/include/plat/voltage.h |5 +
> 5 files changed, 264 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index 390f1c6..879efb2 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #include "prm.h"
> #include "cm.h"
> @@ -586,6 +587,18 @@ static int option_set(void *data, u64 val)
> omap3_pm_off_mode_enable(val);
> }
>
> + if (option == &enable_sr_vp_debug && val)
> + pr_notice("Beware that enabling this option will allow user "
> + "to override the system defined vp and sr parameters "
> + "All the updated parameters will take effect next "
> + "time smartreflex is enabled. Also this option "
> + "disables the automatic vp errorgain and sr errormin "
> + "limit changes as per the voltage. Users will have "
> + "to explicitly write values into the debug fs "
> + "entries corresponding to these if they want to see "
> + "them changing according to the VDD voltage\n");
> +
> +
> return 0;
> }
>
> @@ -642,6 +655,8 @@ static int __init pm_dbg_init(void)
> (void) debugfs_create_file("wakeup_timer_milliseconds",
> S_IRUGO | S_IWUGO, d, &wakeup_timer_milliseconds,
> &pm_dbg_option_fops);
> + (void) debugfs_create_file("enable_sr_vp_debug", S_IRUGO | S_IWUGO, d,
> + &enable_sr_vp_debug, &pm_dbg_option_fops);
>
> pm_dbg_main_dir = d;
> pm_dbg_init_done = 1;
> diff --git a/arch/arm/mach-omap2/smartreflex.c
> b/arch/arm/mach-omap2/smartreflex.c
> index 7fc3138..b5a7878 100644
> --- a/arch/arm/mach-omap2/smartreflex.c
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -558,8 +558,13 @@ int sr_enable(struct voltagedomain *voltdm, unsigned
> long volt)
> return -ENODATA;
> }
>
> - /* errminlimit is opp dependent and hence linked to voltage */
> - sr->err_minlimit = volt_data->sr_errminlimit;
> + /*
> + * 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->sr_enable) {
> @@ -811,9 +816,34 @@ static int omap_sr_autocomp_store(void *data, u64 val)
> return 0;
> }
>
> +static int omap_sr_params_show(void *data, u64 *val)
> +{
> + u32 *param = data;
> +
> + *val = *param;
> + return 0;
> +}
> +
> +static int omap_sr_params_store(void *data, u64 val)
> +{
> + if (enable_sr_vp_debug) {
> + u32 *option = data;
insert blank line
> + *option = val;
> + } else {
> + pr_notice("%s: DEBUG option not enabled!\n \
you don't need a '\' to continue strings onto new lines. Just end the
string, and start another on the next line, as you've done elsewhere in
the patch.
> + echo 1 > pm_debug/enable_sr_vp_debug - to enable\n",
> + __func__);
> + }
> +
> + return 0;
> +}
> +
> DEFINE_SIMPLE_ATTRIBUTE(pm_sr_fops, omap_sr_autocomp_show,
> omap_sr_autocomp_store, "%llu\n");
>
> +DEFINE_SIMPLE_ATTRIBUTE(sr_params_fops, omap_sr_params_show,
> + omap_sr_params_store, "%llu\n");
> +
> static int __init omap_smartreflex_probe(struct platform_device *pdev)
> {
> struct omap_sr *sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL);
> @@ -907,6 +937,12 @@ static int __init omap_smartreflex_probe(struct
> platform_device *pdev)
>
> (void) debugfs_create_file("autocomp", S_IRUGO | S_IWUGO, dbg_dir,
>
Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
On Wed, 2010-09-22 at 20:15 +0530, Thara Gopinath wrote: > --- a/arch/arm/mach-omap2/smartreflex.c > +++ b/arch/arm/mach-omap2/smartreflex.c > @@ -558,8 +558,13 @@ int sr_enable(struct voltagedomain *voltdm, > unsigned long volt) > return -ENODATA; > } > > - /* errminlimit is opp dependent and hence linked to voltage */ > - sr->err_minlimit = volt_data->sr_errminlimit; > + /* > +* 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 > +*/ this comment needs better punctuation to be clearer > + if (!enable_sr_vp_debug) > + sr->err_minlimit = volt_data->sr_errminlimit; Rather than checking if debug is enabled, you should create a flag specific to whether this value was overwritten by the user, and use that flag instead. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
>>-Original Message- >>From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >>Sent: Thursday, September 30, 2010 4:50 AM >>To: Gopinath, Thara >>Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Sripathy, >>Vishwanath; Sawant, Anand >>Subject: Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and >>Smartreflex drivers >> >>Thara Gopinath writes: >> >>[...] >> >>> diff --git a/arch/arm/plat-omap/include/plat/voltage.h b/arch/arm/plat- >>omap/include/plat/voltage.h >>> index 99836a1..8978d33 100644 >>> --- a/arch/arm/plat-omap/include/plat/voltage.h >>> +++ b/arch/arm/plat-omap/include/plat/voltage.h >>> @@ -14,6 +14,11 @@ >>> #ifndef __ARCH_ARM_MACH_OMAP2_VOLTAGE_H >>> #define __ARCH_ARM_MACH_OMAP2_VOLTAGE_H >>> >>> +extern u32 enable_sr_vp_debug; >>> +#ifdef CONFIG_PM_DEBUG >>> +extern struct dentry *pm_dbg_main_dir; >>> +#endif >>> + >>> #define VOLTSCALE_VPFORCEUPDATE1 >>> #define VOLTSCALE_VCBYPASS 2 >> >>This belongs in PATCH 1/11 and belongs in pm.h, not voltage.h as it's a >>feature of the PM debug layer, not the voltage layer. Yes indeed!! Will fix this in v4 Regards Thara -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and Smartreflex drivers
Thara Gopinath writes: [...] > diff --git a/arch/arm/plat-omap/include/plat/voltage.h > b/arch/arm/plat-omap/include/plat/voltage.h > index 99836a1..8978d33 100644 > --- a/arch/arm/plat-omap/include/plat/voltage.h > +++ b/arch/arm/plat-omap/include/plat/voltage.h > @@ -14,6 +14,11 @@ > #ifndef __ARCH_ARM_MACH_OMAP2_VOLTAGE_H > #define __ARCH_ARM_MACH_OMAP2_VOLTAGE_H > > +extern u32 enable_sr_vp_debug; > +#ifdef CONFIG_PM_DEBUG > +extern struct dentry *pm_dbg_main_dir; > +#endif > + > #define VOLTSCALE_VPFORCEUPDATE 1 > #define VOLTSCALE_VCBYPASS 2 This belongs in PATCH 1/11 and belongs in pm.h, not voltage.h as it's a feature of the PM debug layer, not the voltage layer. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
