Re: [PATCH] PM / devfreq: add relation of recommended frequency.

[MyungJoo Ham]

2012/3/1 Rafael J. Wysocki <rjw@xxxxxxx>:
> Hi,
>
> Sorry, I seem to have overlooked this patch.
>
> On Friday, February 10, 2012, MyungJoo Ham wrote:
>> The semantics of "target frequency" given to devfreq driver from
>> devfreq framework has always been interpretted as "at least" or GLB
>> (greatest lower bound). However, the framework might want the
>> device driver to limit its max frequency (LUB: least upper bound),
>> especially if it is given by thermal framework (it's too hot).
>>
>> Thus, the target fuction should have another parameter to express
>> whether the framework wants GLB or LUB. And, the additional parameter,
>> "u32 options", does it.
>
> I'd call that "flags" (which usually is the case in the kernel for things
> handled with the help of bitwise operators).

Hmm.. I agree. "Flag" looks better.

[]
>> -     if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
>> +     if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>>               freq = devfreq->qos_min_freq;
>> -     if (devfreq->max_freq && freq > devfreq->max_freq)
>> +             options &= ~(1 << 0);
>
> What exactly is the (1 << 0) for?
>
> Whatever the case is I'd just use one flag name, e.g.
> DEVFREQ_LEAST_UPPER_BOUND (equal to (1U << 0), and assume that the other
> option is to be chosen when this flag is not set.
>
>
>> +             options |= DEVFREQ_OPTION_FREQ_LUB;
>
> So the above would become:
>
> +       if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>                freq = devfreq->qos_min_freq;
> -       if (devfreq->max_freq && freq > devfreq->max_freq)
> +               options |= DEVFREQ_LEAST_UPPER_BOUND;
> +       }
>
>> +     }
>> +     if (devfreq->max_freq && freq > devfreq->max_freq) {
>>               freq = devfreq->max_freq;
>> -     if (devfreq->min_freq && freq < devfreq->min_freq)
>> +             options &= ~(1 << 0);
>> +             options |= DEVFREQ_OPTION_FREQ_GLB;
>> +     }
>
> and this one would become:
>
> +       if (devfreq->max_freq && freq > devfreq->max_freq) {
>                freq = devfreq->max_freq;
> -       if (devfreq->min_freq && freq < devfreq->min_freq)
> +               options &= ~DEVFREQ_LEAST_UPPER_BOUND;
> +       }
>
> Then, the code would be much easier to follow.
>

Ok, I'll remove one of the two. It appears pretty without either one of the two.

>> @@ -771,14 +777,30 @@ module_exit(devfreq_exit);
>>   *                        freq value given to target callback.
>>   * @dev              The devfreq user device. (parent of devfreq)
>>   * @freq     The frequency given to target function
>> + * @floor    false: find LUB first and use GLB if LUB not available.
>> + *           true:  find GLB first and use LUB if GLB not available.
>> + *
>> + * LUB: least upper bound (at least this freq or above, but the least)
>> + * GLB: greatest lower bound (at most this freq or below, but the most)
>>   *
>>   */
>> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
>> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
>> +                                 bool floor)
>
> Why don't you use "u32 flags" here too?
>

Reading this again, it doesn't matter. That'd use u32 flags, too.

>>  {
>> -     struct opp *opp = opp_find_freq_ceil(dev, freq);
>> +     struct opp *opp;
>>
>> -     if (opp == ERR_PTR(-ENODEV))
>> +     if (floor) {
>
> That would become
>
> +       if (options & DEVFREQ_LEAST_UPPER_BOUND) {
>
> which is a bit more meaningful IMO.
>

Yes.. I agree.

[]
>>
>> +/*
>> + * target callback, which is to provide additional information to the
>> + * devfreq driver.
>> + */
>> +
>> +/* The resulting frequency should be at least this. (least upper bound) */
>> +#define DEVFREQ_OPTION_FREQ_LUB      0x0
>> +/* The resulting frequency should be at most this. (greatest lower bound) */
>> +#define DEVFREQ_OPTION_FREQ_GLB 0x1
>
> As I said, I'd use one option name and call it DEVFREQ_LEAST_UPPER_BOUND.
> It is not necessary to give a name to the alternative. :-)

Yes. ;)



Thank you.



Cheers!
MyungJoo.


-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/