Hi
> [...]
> >> +static char *profile_str[] = {
> >
> > Why is it not `const`?
> My mistake. I will fix
> >
> >
> >> + "Low-power",
> >> + "Cool",
> >> + "Quiet",
> >> + "Balance",
> >> + "Performance",
> >> + "Unknown"
> >
> > "unknown" is not documented, yet it may be returned to userspace.
> Ack - I'll look into if it's really needed, but it seemed sensible to
> have it whilst doing the implementation.
I don't advocate for its removal, just that it be documented if it may be
returned to userspace.
> >
> >
> >> +};
> [...]
> >> +#ifndef _PLATFORM_PROFILE_H_
> >> +#define _PLATFORM_PROFILE_H_
> >> +
> >> +/*
> >> + * If more options are added please update profile_str
> >> + * array in platform-profile.c
> >> + */
> >> +
> >> +enum profile_option {
> >> + profile_low,
> >> + profile_cool,
> >> + profile_quiet,
> >> + profile_balance,
> >> + profile_perform,
> >> + profile_unknown /* Must always be last */
> >> +};
> >
> > Shouldn't these be prefixed by `platform_`? And I think it'd be better to have
> > `profile_unknown` as the first value in the enumeration.
> I can add 'platform_'
> I liked having profile_unknown as the last value as it makes scanning
> from 'low' to 'unknown' more future proof if other profiles get added
> (e.g in platform_profile_choices_show).
> Is this something you feel strongly about?
I don't feel strongly about it, just that right now, for all practical purposes
`profile_unknown` feels like a first-class profile option in the current form of
the patch, and it didn't seem right that it can just change. I'd do something like
```
enum performance_profile_option {
performance_profile_unknown,
performance_profile_low,
...
performance_profile_max, /* must be last */
};
```
But I don't have a strong preference for either one of them. Maybe someone
could chime in and tell us which one is more prevalent/preferred.
And as a side note, I think you could put something like
`static_assert(ARRAY_SIZE(profile_str) == performance_profile_max);`
in the code somewhere to make sure there are as many strings in the array as
profile options.
You might actually do the following as well:
```
static const char *profile_str[] = {
[performance_profile_unknown] = "unknown",
[performance_profile_low] = "low",
...
};
```
I realize I might be a bit too paranoid here. :-) But if you do these three things,
or something similar, then the chances of the enum and the array being out of
sync (by accident) will be very slim.
> [...]
Regards,
Barnabás Pőcze
