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@xxxxxxxxxxxxxxxxxxx>
Cc: Thara Gopinath<thara@xxxxxx>
Signed-off-by: Nishanth Menon<nm@xxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
