On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@xxxxxxxxxx> wrote:
>
> Add initial support for platform_profile where the support is
> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>
> Because throttle_thermal_policy is used by platform_profile and is
> writeable separately to platform_profile any userspace changes to
> throttle_thermal_policy need to notify platform_profile.
>
> In future throttle_thermal_policy sysfs should be removed so that
> only one method controls the laptop power profile.
Some comments below.
...
> + /*
> + * Ensure that platform_profile updates userspace with the change to ensure
> + * that platform_profile and throttle_thermal_policy_mode are in sync
Missed period here and in other multi-line comments.
> + */
...
> + /* All possible toggles like throttle_thermal_policy here */
> + if (asus->throttle_thermal_policy_available) {
> + tp = asus->throttle_thermal_policy_mode;
> + } else {
> + return -1;
> + }
> +
> + if (tp < 0)
> + return tp;
This will be better in a form
if (!..._available)
return -ENODATA; // what -1 means?
tp = ...;
if (tp < 0)
return tp;
...
> + /* All possible toggles like throttle_thermal_policy here */
> + if (!asus->throttle_thermal_policy_available) {
> + return -1;
See above.
> + }
...
> + if (asus->throttle_thermal_policy_available) {
> + pr_info("Using throttle_thermal_policy for platform_profile support\n");
Why pr_*()?
> + } else {
> + /*
> + * Not an error if a component platform_profile relies on is unavailable
> + * so early return, skipping the setup of platform_profile.
> + */
> + return 0;
Do it other way around,
/*
* Comment
*/
if (!...)
return 0;
> + }
--
With Best Regards,
Andy Shevchenko
