Re: [PATCH v3 1/1] asus-wmi: Add support for platform_profile

From: Luke Jones
Date: Sat Aug 14 2021 - 08:47:46 EST


A very quick question: should I be enabling platform_profile by default if asus_wmi is enabled in kconfig?

On Sat, Aug 14 2021 at 23:46:06 +1200, Luke Jones <luke@xxxxxxxxxx> wrote:
Hi Andy, thanks for the feedback. All is addressed, and some inline comment/reply. New version incoming pending pr_info() vs dev_info()

On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
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?


I wasn't sure what the best return was here. On your suggestion I've changed to ENODATA

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_*()?

That seemed to be the convention? I see there is also dev_info(), so I've switched to that as it seems more appropriate.


+ } 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;

Thanks, I think I was transliterating thought process to code as I went along.
All updated now.


+ }


--
With Best Regards,
Andy Shevchenko