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.
Let me know what I'm missing.
Thanks
Mark
