Re: [External] Re: [PATCH 2/3] ACPI: platform-profile: Add platform profile support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux