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?
