Nishanth Menon wrote: > 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? Yes, because they are going to be SoC specific. > >>>> Signed-off-by: Romit Dasgupta <ro...@ti.com> >>>> --- >>>> >>>> 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.
OK >> >> > 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. As I said before one needs to make changes in the kernel by knowing what they are doing. > > 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. The right approach should be to take out the loop in pm34xx.c for now and explicitly call the opp_init_list function after passing OPP_MPU, OPP_L3, OPP_DSP in any order. So pm34xx.c needs to change not opp.[ch]. What do you think? >>>> */ >>>> -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. What is the readability issue? Why cant we declare something like enum opp_t opp_t? >>> 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? omap3_pm_init_opp_table > > 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. > How on earth?? I have removed mpu_opps, l3_opps, dsp_opps from the code so how do you think I should preserve the checking of the variables when they don't exist. -- 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