Romit Dasgupta said the following on 01/13/2010 04:41 AM:
Menon, Nishanth wrote:
General comment: might be good to state the enum types you are introducing
for OMAP3 in the commit message
Actually the introduction of enum type itself is the heart of the patch. The
details are irrelevant.
could you be a little more verbose as to what is irrelevant? the OMAP3
enums descriptions in commit message?
Signed-off-by: Romit Dasgupta <romit@xxxxxx>
---
omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
omap34xx_opp_def_list;
- for (i = 0; i < ARRAY_SIZE(omap3_rate_tables); i++) {
- *omap3_rate_tables[i] = opp_init_list(omap3_opp_def_list[i]);
+ entries = cpu_is_omap3630() ? ARRAY_SIZE(omap34xx_opp_def_list) :
+ ARRAY_SIZE(omap36xx_opp_def_list);
+ for (i = 1; i <= entries; i++) {
+ ret = opp_init_list(i, omap3_opp_def_list[i - 1]);
a) if you remove OPP_NONE, i-1 is not needed (same everywhere in the patch)
Frankly, I did not want to introduce OPP_NONE but did so as you are checking all
parameters passed to the OPP APIs.
Lets remove it then.
b) if we modify the ENUMS or the sequence of definitions in opp_t the logic
here becomes fault. it might be good to retain an equivalent of
omap3_rate_table with enum equivalents and register by indexing off that.
You are right but this is a kernel level API and user level code is not going to
use this. Having said this there is no scope for a programmer to introduce new
sequences without understanding the consequences.
definition of enum and the implicit usage of enums are in two different
files. there is a distinct possibility of some one modifying the header
without actually knowing that .c depends on the exact values of the enum
definition.
pm34xx.c has no right to depend on opp.h definition values -> if it does
it ties it down and a constraint for future flexibility. please change.
/* We dont want half configured system at the moment */
- BUG_ON(IS_ERR(omap3_rate_tables[i]));
+ BUG_ON(ret);
}
}
diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
index 157b38e..38c44ee 100644
--- a/arch/arm/mach-omap2/resource34xx.c
+++ b/arch/arm/mach-omap2/resource34xx.c
@@ -161,7 +161,7 @@ static DEFINE_MUTEX(dvfs_mutex);
/**
* opp_to_freq - convert OPPID to frequency (DEPRECATED)
* @freq: return frequency back to caller
- * @opps: opp list
+ * @opp_t: OPP type where we need to look.
* @opp_id: OPP ID we are searching for
*
* return 0 and freq is populated if we find the opp_id, else,
@@ -169,14 +169,14 @@ static DEFINE_MUTEX(dvfs_mutex);
*
* NOTE: this function is a standin for the timebeing as opp_id is deprecated
*/
-static int __deprecated opp_to_freq(unsigned long *freq,
- const struct omap_opp *opps, u8 opp_id)
+static int __deprecated opp_to_freq(unsigned long *freq, enum opp_t opp_t,
Enum type and variable have the same name :( mebbe a rename of variable is
appropriate
Not sure why you say this. Did you see the compiler throwing up any warning?
The usage later in the code is opp_t -> this is a readability issue not
a compiler warning.
@@ -188,20 +188,20 @@ static int __deprecated opp_to_freq(unsigned long *freq,
-static int __deprecated freq_to_opp(u8 *opp_id, struct omap_opp *opps,
+static int __deprecated freq_to_opp(u8 *opp_id, enum opp_t opp_t,
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
unsigned long freq)
{
struct omap_opp *opp;
- BUG_ON(!opp_id || !opps);
- opp = opp_find_freq_ceil(opps, &freq);
+ BUG_ON(opp_t == OPP_NONE || opp_t > OPP_TYPES);
+ opp = opp_find_freq_ceil(opp_t, &freq);
if (IS_ERR(opp))
return -EINVAL;
*opp_id = opp_get_opp_id(opp);
@@ -218,9 +218,6 @@ void init_opp(struct shared_resource *resp)
u8 opp_id;
resp->no_of_users = 0;
- if (!mpu_opps || !dsp_opps || !l3_opps)
- return;
-
the original intent of this check is lost here - if the initializations did not
take place, we will not proceed. An equivalent check might be good to maintain
at this point.
You are partially correct. I took off the checks because we have a BUG_ON() call
in the beginning of the boot code right after we initialize the OPP tables. So
we should not hit this check.
hmm.. interesting.. can you elaborate with exact functions?
if you are removing this check, please do a follow up patch and maintain
equivalent one for this so that the patch does exactly what the commit
message says "introduce enums" - not do something more.
[...]
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
