On 2024-03-01 08:51, Alexey Charkov wrote:
On Fri, Mar 1, 2024 at 10:14 AM Dragan Simic <dsimic@xxxxxxxxxxx>
wrote:
On 2024-03-01 06:20, Alexey Charkov wrote:
> On Fri, Mar 1, 2024 at 1:11 AM Dragan Simic <dsimic@xxxxxxxxxxx> wrote:
>> See, I'm not a native English speaker, but I've spent a lot of time
>> and effort improving my English skills. Thus, perhaps these comments
>> may or may not seem like unnecessary nitpicking, depending on how much
>> someone pays attention to writing style in general, but I'll risk to
>> be annoying and state these comments anyway. :)
>>
>> The comment above could be written in a much more condensed form like
>> this, which would also be a bit more accurate:
>>
>>
>> /* IPA threshold, when IPA governor is
>> used */
>>
>> IOW, we're writing all this for someone to read later, but we should
>> (and can) perfectly reasonably expect some already existing background
>> knowledge from the readers. In other words, we should be as concise
>> as possible.
>
> In fact, the power allocation governor code itself doesn't call those
> trips threshold or target as your suggested wording would imply.
> Instead, it calls them "switch on temperature" and "maximum desired
> temperature" [1]. Maybe we can call them that in the comments (and
> also avoid calling the governor IPA, because upstream code only calls
> it a "power allocator").
Hmm, but "IPA" is still mentioned in exactly three places in the files
under drivers/thermal. I think that warrants the use of "IPA", which
is also widely used pretty much everywhere.
Perhaps a win-win would be to have only the very first of the comments
like this, to introduce "IPA" as an acronym:
/* Power allocator (IPA) thermal
governor */
/* switch-on point, when IPA
governor
is used */
Yes, good point, thanks!
I'm glad that you agree. :)
Next, "the target temperature" is mentioned more than a few times in
drivers/thermal/gov_power_allocator.c, which I believe makes the use
of "IPA target" perfectly valid. Actually, let's use "IPA target
temperature", if you agree, to make it self descriptive.
Or perhaps simply "target temperature"? Stepwise governor will also
use this trip as its target, so it's not IPA specific, unlike the
switch-on point.
I also had similar thoughts about the shared nature. I agree, just
"/* target temperature */" would be fine.
Finally, the threshold... Based on
drivers/thermal/gov_power_allocator.c,
I think "IPA switch-on point" would be a good choice, which I already
used above in the proposed opening comment.
Agreed, that sounds good to me, will reflect in the next iteration.
Thanks for bringing it up!
Great, thanks!
