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.
>
>> 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.
> 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.
>
>> /* 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?
>> @@ -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.
>> @@ -519,19 +513,16 @@ void init_freq(struct shared_resource *resp)
>> unsigned long freq;
>> resp->no_of_users = 0;
>>
>> - if (!mpu_opps || !dsp_opps)
>> - return;
>> -
> again the initial intent is lost -> to handle cases where the initialization was
> not being done.
Same comment as before.
Thanks,
-Romit
--
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
