On 17/12/2020 08:36, Rafael J. Wysocki wrote:
> On Wed, Dec 16, 2020 at 10:36 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
>>
>>
>> On 16/12/2020 14:50, Rafael J. Wysocki wrote:
>>> On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
>>>>> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>>>>>>
>>>>>>> On Fri, Dec 11, 2020 at 3:15 AM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
>>>>>>>>
>>>>>>>> This is the initial implementation of the platform-profile feature.
>>>>>>>> It provides the details discussed and outlined in the
>>>>>>>> sysfs-platform_profile document.
>>>>>>>>
>>>>>>>> Many modern systems have the ability to modify the operating profile to
>>>>>>>> control aspects like fan speed, temperature and power levels. This
>>>>>>>> module provides a common sysfs interface that platform modules can register
>>>>>>>> against to control their individual profile options.
>>>>>>>>
>>>>>>>> Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx>
>>>>>>> [...]
>>>>>>>> +enum platform_profile_option {
>>>>>>>> + PLATFORM_PROFILE_LOW,
>>>>>>>> + PLATFORM_PROFILE_COOL,
>>>>>>>> + PLATFORM_PROFILE_QUIET,
>>>>>>>> + PLATFORM_PROFILE_BALANCED,
>>>>>>>> + PLATFORM_PROFILE_PERFORM,
>>>>>>>> + PLATFORM_PROFILE_LAST, /*must always be last */
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +struct platform_profile_handler {
>>>>>>>> + unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>>>>>>> + int (*profile_get)(enum platform_profile_option *profile);
>>>>>>>
>>>>>>> I'm not sure why this callback is necessary and, provided that there
>>>>>>> is a good enough reason, why it cannot return an enum
>>>>>>> platform_profile_option value.
>>>>>>>
>>>>>>> In principle, if ->profile_set() returns 0, the requested profile can
>>>>>>> be saved in a static var and then returned by subsequent "read"
>>>>>>> operations.
>>>>>>>
>>>>>>
>>>>>> It is possible that the platform profile can be changed with (e.g.) a dedicated
>>>>>> button (commonly found on laptops), in which case there needs to be a mechanism
>>>>>> to retrieve the new profile, which would not be possible without introducing
>>>>>> something else in place of that getter - unless I'm missing something obvious.
>>>>>
>>>>> Fair enough.
>>>>>
>>>>> The other question remains, then.
>>>>>
>>>> My thinking here that I shouldn't make assumptions for future platform
>>>> implementations - there may be valid cases in the future where being
>>>> able to return an error condition if there was an error would be useful.
>>>>
>>>> Just trying to keep this somewhat future proof. Returning an error
>>>> condition seemed a useful thing to have available.
>>>
>>> You can still return an error while returning a platform_profile_option value.
>>>
>>> Just add a special value representing an error to that set.
>>>
>> I'd like to understand what is better about that approach than having an
>> error and a returnable parameter?
>>
>> I'm probably missing something obvious but if I add an extra
>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
>> error case (and return just an enum platform_profile_option) it seems I
>> lose the granularity of being able to return a specific error condition.
>> It doesn't feel like an improvement.
>
> And what's the user expected to do about the different error codes
> that can be returned?
>
>From my experiences when something fails I would rather have more
information than less. I'm probably over thinking it though.
Looking at it again, from my Lenovo platform profile implementation
perspective I have no objections. It's hard to argue a point without
having a hard requirement or example :) I'll go ahead with your
suggestion of just returning the profile.
Thank you for the review
Mark
