[PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[Luke D. Jones]

Add support for custom fan curves found on some ASUS ROG laptops.

- V1
  + Initial patch work
- V2
  + Don't fail and remove wmi driver if error from
    asus_wmi_evaluate_method_buf() if error is -ENODEV
- V3
  + Store the "default" fan curves
  + Call throttle_thermal_policy_write() if a curve is erased to ensure
    that the factory default for a profile is applied again
- V4
  + Do not apply default curves by default. Testers have found that the
    default curves don't quite match actual no-curve behaviours
  + Add method to enable/disable curves for each profile
- V5
  + Remove an unrequired function left over from previous iterations
  + Ensure default curves are applied if user writes " " to a curve path
  + Rename "active_fan_curve_profiles" to "enabled_fan_curve_profiles" to
    better reflect the behavious of this setting
  + Move throttle_thermal_policy_write_*pu_curves() and rename to
    fan_curve_*pu_write()
  + Merge fan_curve_check_valid() and fan_curve_write()
  + Remove some leftover debug statements
- V6
  + Refactor data structs to store  array or u8 instead of strings.
    This affects the entire patch except the enabled_fan_curves block
  + Use sysfs_match_string in enabled_fan_curve block
  + Add some extra comments to describe things
  + Allow some variation in how fan curve input can be formatted
  + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines per
    fan+profile combo drastically

Luke D. Jones (1):
  asus-wmi: Add support for custom fan curves

 drivers/platform/x86/asus-wmi.c            | 617 ++++++++++++++++++++-
 include/linux/platform_data/x86/asus-wmi.h |   2 +
 2 files changed, 617 insertions(+), 2 deletions(-)

--
2.31.1

[PATCH v6] asus-wmi: Add support for custom fan curves

[Luke D. Jones]

Add support for custom fan curves found on some ASUS ROG laptops.

These laptops have the ability to set a custom curve for the CPU
and GPU fans via an ACPI method call. This patch enables this,
additionally enabling custom fan curves per-profile, where profile
here means each of the 3 levels of "throttle_thermal_policy".

The format for input fan curves is
"30:1,49:2,59:3,69:4,79:31,89:49,99:56,109:58"
where each block of "30:1," is one point on the curve, and 30c, 1%
of fan RPM up to 100%. This follows the internal WMI format. This
format was chosen as it is used heavily by pre-existing utilities
that were based on using the acpi_call module. Further, each pair
can be defined as "T[ c:]P[ %,]" resulting in "30c:1%,", "30c1 "
or similar with a max of two chars between values.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 488 ++++++++++++++++++++-
 include/linux/platform_data/x86/asus-wmi.h |   2 +
 2 files changed, 488 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index cc5811844012..dccdd41917ff 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -106,8 +106,23 @@ module_param(fnlock_default, bool, 0444);
 
 #define WMI_EVENT_MASK			0xFFFF
 
+/* The number of ASUS_THROTTLE_THERMAL_POLICY_* available */
+#define NUM_FAN_CURVE_PROFILES	3
+#define NUM_FAN_CURVE_POINTS	8
+#define NUM_FAN_CURVE_DATA	(NUM_FAN_CURVE_POINTS * 2)
+
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
+/*
+ * The order here matters, index positions reflect
+ * ASUS_THROTTLE_THERMAL_POLICY_<name>
+ */
+static const char * const fan_curve_names[] = {
+	"balanced", "performance", "quiet"
+};
+
+static int throttle_thermal_policy_write(struct asus_wmi *);
+
 static bool ashs_present(void)
 {
 	int i = 0;
@@ -122,7 +137,8 @@ struct bios_args {
 	u32 arg0;
 	u32 arg1;
 	u32 arg2; /* At least TUF Gaming series uses 3 dword input buffer. */
-	u32 arg4;
+	u32 arg3;
+	u32 arg4; /* Some ROG laptops require a full 5 input args */
 	u32 arg5;
 } __packed;
 
@@ -173,6 +189,29 @@ enum fan_type {
 	FAN_TYPE_SPEC83,	/* starting in Spec 8.3, use CPU_FAN_CTRL */
 };
 
+enum fan_device {
+	FAN_DEVICE_CPU = 0,
+	FAN_DEVICE_GPU,
+};
+
+/*
+ * Each temps[n] is paired with percents[n]
+ */
+struct fan_curve {
+	u8 temps[NUM_FAN_CURVE_POINTS];
+	u8 percents[NUM_FAN_CURVE_POINTS];
+};
+
+/*
+ * The related ACPI method for testing availability also returns the factory
+ * default fan curves. We save them here so that a user can reset custom
+ * settings if required.
+ */
+struct fan_curve_data {
+	struct fan_curve custom;
+	struct fan_curve defaults;
+};
+
 struct asus_wmi {
 	int dsts_id;
 	int spec;
@@ -220,6 +259,11 @@ struct asus_wmi {
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
 
+	bool cpu_fan_curve_available;
+	bool gpu_fan_curve_available;
+	bool fan_curves_enabled[NUM_FAN_CURVE_PROFILES];
+	struct fan_curve_data throttle_fan_curves[NUM_FAN_CURVE_PROFILES][2];
+
 	struct platform_profile_handler platform_profile_handler;
 	bool platform_profile_support;
 
@@ -285,6 +329,84 @@ int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval)
 }
 EXPORT_SYMBOL_GPL(asus_wmi_evaluate_method);
 
+static int asus_wmi_evaluate_method5(u32 method_id,
+		u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4, u32 *retval)
+{
+	struct bios_args args = {
+		.arg0 = arg0,
+		.arg1 = arg1,
+		.arg2 = arg2,
+		.arg3 = arg3,
+		.arg4 = arg4,
+	};
+	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+	union acpi_object *obj;
+	u32 tmp = 0;
+
+	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
+				     &input, &output);
+
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	obj = (union acpi_object *)output.pointer;
+	if (obj && obj->type == ACPI_TYPE_INTEGER)
+		tmp = (u32) obj->integer.value;
+
+	if (retval)
+		*retval = tmp;
+
+	kfree(obj);
+
+	if (tmp == ASUS_WMI_UNSUPPORTED_METHOD)
+		return -ENODEV;
+
+	return 0;
+}
+
+/*
+ * Returns as an error if the method output is not a buffer. Typically this
+ * means that the method called is unsupported.
+ */
+static int asus_wmi_evaluate_method_buf(u32 method_id,
+		u32 arg0, u32 arg1, u8 *ret_buffer)
+{
+	struct bios_args args = {
+		.arg0 = arg0,
+		.arg1 = arg1,
+		.arg2 = 0,
+	};
+	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+	union acpi_object *obj;
+	u32 int_tmp = 0;
+
+	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
+				     &input, &output);
+
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	obj = (union acpi_object *)output.pointer;
+
+	if (obj && obj->type == ACPI_TYPE_INTEGER) {
+		int_tmp = (u32) obj->integer.value;
+		if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
+			return -ENODEV;
+		return int_tmp;
+	}
+
+	if (obj && obj->type == ACPI_TYPE_BUFFER)
+		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
+
+	kfree(obj);
+
+	return 0;
+}
+
 static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
 {
 	struct acpi_buffer input;
@@ -2043,6 +2165,330 @@ static ssize_t fan_boost_mode_store(struct device *dev,
 // Fan boost mode: 0 - normal, 1 - overboost, 2 - silent
 static DEVICE_ATTR_RW(fan_boost_mode);
 
+/* Custom fan curves per-profile **********************************************/
+
+static void copy_fan_curve_buf(struct fan_curve *data, u8 *buf)
+{
+	int i;
+
+	for (i = 0; i < NUM_FAN_CURVE_POINTS; i++)
+		data->temps[i] = buf[i];
+
+	for (i = 0; i < NUM_FAN_CURVE_POINTS; i++)
+		data->percents[i] = buf[i + 8];
+}
+
+static void init_fan_curve(struct fan_curve_data *curves,
+			       u8 *buf, u32 dev)
+{
+	copy_fan_curve_buf(&curves->custom, buf);
+	copy_fan_curve_buf(&curves->defaults, buf);
+}
+
+/*
+ * Check if the ability to set fan curves on either fan exists, and store the
+ * defaults for recall later plus to provide users with a starting point.
+ *
+ * "dev" is either CPU_FAN_CURVE or GPU_FAN_CURVE.
+ */
+static int custom_fan_check_present(struct asus_wmi *asus,
+				    bool *available, u32 dev)
+{
+	struct fan_curve_data *curves;
+	u8 buf[NUM_FAN_CURVE_DATA];
+	int fan_idx = 0;
+	int err;
+
+	*available = false;
+	if (dev == ASUS_WMI_DEVID_GPU_FAN_CURVE)
+		fan_idx = 1;
+
+	/* Balanced default */
+	curves =
+	&asus->throttle_fan_curves[ASUS_THROTTLE_THERMAL_POLICY_DEFAULT][fan_idx];
+	err = asus_wmi_evaluate_method_buf(asus->dsts_id, dev, 0, buf);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+	init_fan_curve(curves, buf, dev);
+
+	/*
+	 * Quiet default. The index num for ACPI method does not match the
+	 * throttle_thermal number, same for Performance.
+	 */
+	curves =
+	&asus->throttle_fan_curves[ASUS_THROTTLE_THERMAL_POLICY_SILENT][fan_idx];
+	err = asus_wmi_evaluate_method_buf(asus->dsts_id, dev, 1, buf);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+	init_fan_curve(curves, buf, dev);
+
+	/* Performance default */
+	curves =
+	&asus->throttle_fan_curves[ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST][fan_idx];
+	err = asus_wmi_evaluate_method_buf(asus->dsts_id, dev, 2, buf);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+	init_fan_curve(curves, buf, dev);
+
+	*available = true;
+	return 0;
+}
+
+static ssize_t fan_curve_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	/* index maps to ASUS_THROTTLE_THERMAL_POLICY_DEFAULT */
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	/* dev maps to ASUS_WMI_DEVID_[C/G]PU_FAN_CURVE */
+	int index = to_sensor_dev_attr_2(attr)->index;
+
+	struct fan_curve *dat = &asus->throttle_fan_curves[nr][index].custom;
+	int len = 0;
+	int i = 0;
+
+	for (i = 0; i < NUM_FAN_CURVE_POINTS; i++) {
+		len += sysfs_emit_at(buf, len, "%dc:%d%%",
+					dat->temps[i], dat->percents[i]);
+		if (i < NUM_FAN_CURVE_POINTS - 1)
+			len += sysfs_emit_at(buf, len, ",");
+	}
+	len += sysfs_emit_at(buf, len, "\n");
+	return len;
+}
+
+/*
+ * "dev" is the related WMI method such as ASUS_WMI_DEVID_CPU_FAN_CURVE.
+ */
+static int fan_curve_write(struct asus_wmi *asus, u32 dev,
+					struct fan_curve *curve)
+{
+	int ret;
+	u32 arg1, arg2, arg3, arg4;
+
+	arg1 = curve->temps[0];
+	arg2 = curve->temps[4];
+	arg1 += curve->temps[1] << 8;
+	arg2 += curve->temps[5] << 8;
+	arg1 += curve->temps[2] << 16;
+	arg2 += curve->temps[6] << 16;
+	arg1 += curve->temps[3] << 24;
+	arg2 += curve->temps[7] << 24;
+
+
+	arg3 = curve->percents[0];
+	arg4 = curve->percents[4];
+	arg3 += curve->percents[1] << 8;
+	arg4 += curve->percents[5] << 8;
+	arg3 += curve->percents[2] << 16;
+	arg4 += curve->percents[6] << 16;
+	arg3 += curve->percents[3] << 24;
+	arg4 += curve->percents[7] << 24;
+
+	return asus_wmi_evaluate_method5(ASUS_WMI_METHODID_DEVS, dev,
+					arg1, arg2, arg3, arg4, &ret);
+}
+
+/*
+ * Called only by throttle_thermal_policy_write()
+ */
+static int fan_curve_write_data(struct asus_wmi *asus)
+{
+	struct fan_curve_data *cpu;
+	struct fan_curve_data *gpu;
+	int err, mode;
+
+	mode = asus->throttle_thermal_policy_mode;
+	cpu = &asus->throttle_fan_curves[mode][FAN_DEVICE_CPU];
+	gpu = &asus->throttle_fan_curves[mode][FAN_DEVICE_GPU];
+
+	if (asus->fan_curves_enabled[mode]) {
+		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, &cpu->custom);
+		if (err)
+			return err;
+	}
+
+	if (asus->fan_curves_enabled[mode]) {
+		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, &gpu->custom);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+/*
+ * The expected input is 8 sets of number pairs, where "53c30%" temperacture
+ * and fan RPM percentage. The pair can be of the format "T[ c:]P[ %,]".
+ *
+ */
+static ssize_t fan_curve_store(struct device *dev,
+				struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 temps[NUM_FAN_CURVE_POINTS];
+	u8 percents[NUM_FAN_CURVE_POINTS];
+	/* tmp1 and tmp2 exist only to allow matching to succeed */
+	char *tmp1;
+	char *tmp2;
+	int err;
+
+	/* index maps to ASUS_THROTTLE_THERMAL_POLICY_DEFAULT */
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	/* dev maps to ASUS_WMI_DEVID_[C/G]PU_FAN_CURVE */
+	int index = to_sensor_dev_attr_2(attr)->index;
+	/* Variable format, values must at least be separated by these */
+	char part[] = "%d%2[ c:]%d%2[ %%,\n]%n";
+	u32 prev_percent = 0;
+	u32 prev_temp = 0;
+	u32 percent = 0;
+	u32 temp = 0;
+	int len = 0;
+	int idx = 0;
+	int at = 0;
+
+	struct fan_curve_data *curve = &asus->throttle_fan_curves[nr][index];
+
+	/* Allow a user to write "" or " " to erase a curve setting */
+	if (strlen(buf) <= 1 || strcmp(buf, "\n") == 0) {
+		memcpy(&curve->custom, &curve->defaults, NUM_FAN_CURVE_DATA);
+		err = throttle_thermal_policy_write(asus);
+		if (err)
+			return err;
+		return count;
+	}
+
+	/* parse the buf */
+	while (sscanf(&buf[at], part, &temp, &tmp1, &percent, &tmp2, &len) == 4) {
+		if (temp < prev_temp || percent < prev_percent) {
+			pr_info("Fan curve invalid: a value is sequentially lower");
+			return -EINVAL;
+		}
+
+		if (percent > 100) {
+			pr_info("Fan curve invalid: percentage > 100");
+			return -EINVAL;
+		}
+
+		prev_temp = temp;
+		prev_percent = percent;
+
+		temps[idx] = temp;
+		percents[idx] = percent;
+		at += len;
+		idx += 1;
+	}
+
+	if (idx != NUM_FAN_CURVE_POINTS) {
+		pr_info("Fan curve invalid: incomplete string: %d", idx);
+		return -EINVAL;
+	}
+
+	memcpy(&curve->custom.temps, &temps, NUM_FAN_CURVE_POINTS);
+	memcpy(&curve->custom.percents, &percents, NUM_FAN_CURVE_POINTS);
+
+	/* Maybe activate fan curve if in associated mode */
+	err = throttle_thermal_policy_write(asus);
+	if (err)
+		return err;
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR_2_RW(cpu_fan_curve_balanced, fan_curve,
+				ASUS_THROTTLE_THERMAL_POLICY_DEFAULT,
+				FAN_DEVICE_CPU);
+static SENSOR_DEVICE_ATTR_2_RW(cpu_fan_curve_performance, fan_curve,
+				ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST,
+				FAN_DEVICE_CPU);
+static SENSOR_DEVICE_ATTR_2_RW(cpu_fan_curve_quiet, fan_curve,
+				ASUS_THROTTLE_THERMAL_POLICY_SILENT,
+				FAN_DEVICE_CPU);
+
+static SENSOR_DEVICE_ATTR_2_RW(gpu_fan_curve_balanced, fan_curve,
+				ASUS_THROTTLE_THERMAL_POLICY_DEFAULT,
+				FAN_DEVICE_GPU);
+static SENSOR_DEVICE_ATTR_2_RW(gpu_fan_curve_performance, fan_curve,
+				ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST,
+				FAN_DEVICE_GPU);
+static SENSOR_DEVICE_ATTR_2_RW(gpu_fan_curve_quiet, fan_curve,
+				ASUS_THROTTLE_THERMAL_POLICY_SILENT,
+				FAN_DEVICE_GPU);
+
+/*
+ * Profiles with enabled fan curve setting
+ */
+
+static int enabled_fan_curve_profiles_write(struct asus_wmi *asus,
+					    const char *names)
+{
+	char *buf, *set, *set_end;
+	int err, index;
+
+	buf = set_end = kstrdup(names, GFP_KERNEL);
+
+	/* Reset before checking */
+	asus->fan_curves_enabled[ASUS_THROTTLE_THERMAL_POLICY_DEFAULT] = false;
+	asus->fan_curves_enabled[ASUS_THROTTLE_THERMAL_POLICY_SILENT] = false;
+	asus->fan_curves_enabled[ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST] = false;
+
+	while ((set = strsep(&set_end, " ")) != NULL) {
+		index = sysfs_match_string(fan_curve_names, set);
+		if (index >= 0)
+			asus->fan_curves_enabled[index] = true;
+	}
+
+	err = throttle_thermal_policy_write(asus);
+	if (err)
+		return err;
+
+	kfree(buf);
+
+	return 0;
+}
+
+static ssize_t enabled_fan_curve_profiles_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int i, len = 0;
+
+	for (i = 0; i < NUM_FAN_CURVE_PROFILES; i++) {
+		if (asus->fan_curves_enabled[i]) {
+			len += sysfs_emit_at(buf, len, fan_curve_names[i]);
+			len += sysfs_emit_at(buf, len, " ");
+		}
+	}
+
+	len += sysfs_emit_at(buf, len, "\n");
+	return len;
+}
+
+static ssize_t enabled_fan_curve_profiles_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int err;
+
+	err = enabled_fan_curve_profiles_write(asus, buf);
+	if (err)
+		return err;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(enabled_fan_curve_profiles);
+
 /* Throttle thermal policy ****************************************************/
 
 static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
@@ -2092,6 +2538,12 @@ static int throttle_thermal_policy_write(struct asus_wmi *asus)
 		return -EIO;
 	}
 
+	if (asus->cpu_fan_curve_available || asus->gpu_fan_curve_available) {
+		err = fan_curve_write_data(asus);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -2711,6 +3163,13 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
 	&dev_attr_throttle_thermal_policy.attr,
+	&sensor_dev_attr_cpu_fan_curve_balanced.dev_attr.attr,
+	&sensor_dev_attr_cpu_fan_curve_performance.dev_attr.attr,
+	&sensor_dev_attr_cpu_fan_curve_quiet.dev_attr.attr,
+	&sensor_dev_attr_gpu_fan_curve_balanced.dev_attr.attr,
+	&sensor_dev_attr_gpu_fan_curve_performance.dev_attr.attr,
+	&sensor_dev_attr_gpu_fan_curve_quiet.dev_attr.attr,
+	&dev_attr_enabled_fan_curve_profiles.attr,
 	&dev_attr_panel_od.attr,
 	NULL
 };
@@ -2741,6 +3200,20 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
 		ok = asus->throttle_thermal_policy_available;
+	else if (attr == &sensor_dev_attr_cpu_fan_curve_balanced.dev_attr.attr)
+		ok = asus->cpu_fan_curve_available;
+	else if (attr == &sensor_dev_attr_cpu_fan_curve_performance.dev_attr.attr)
+		ok = asus->cpu_fan_curve_available;
+	else if (attr == &sensor_dev_attr_cpu_fan_curve_quiet.dev_attr.attr)
+		ok = asus->cpu_fan_curve_available;
+	else if (attr == &sensor_dev_attr_gpu_fan_curve_balanced.dev_attr.attr)
+		ok = asus->gpu_fan_curve_available;
+	else if (attr == &sensor_dev_attr_gpu_fan_curve_performance.dev_attr.attr)
+		ok = asus->gpu_fan_curve_available;
+	else if (attr == &sensor_dev_attr_gpu_fan_curve_quiet.dev_attr.attr)
+		ok = asus->gpu_fan_curve_available;
+	else if (attr == &dev_attr_enabled_fan_curve_profiles.attr)
+		ok = asus->cpu_fan_curve_available || asus->gpu_fan_curve_available;
 	else if (attr == &dev_attr_panel_od.attr)
 		ok = asus->panel_overdrive_available;
 
@@ -2904,7 +3377,7 @@ static int show_call(struct seq_file *m, void *data)
 	if (ACPI_FAILURE(status))
 		return -EIO;
 
-	obj = (union acpi_object *)output.pointer;
+	obj = output.pointer;
 	if (obj && obj->type == ACPI_TYPE_INTEGER)
 		seq_printf(m, "%#x(%#x, %#x) = %#x\n", asus->debug.method_id,
 			   asus->debug.dev_id, asus->debug.ctrl_param,
@@ -3016,6 +3489,16 @@ static int asus_wmi_add(struct platform_device *pdev)
 	else
 		throttle_thermal_policy_set_default(asus);
 
+	err = custom_fan_check_present(asus, &asus->cpu_fan_curve_available,
+			ASUS_WMI_DEVID_CPU_FAN_CURVE);
+	if (err)
+		goto fail_custom_fan_curve;
+
+	err = custom_fan_check_present(asus, &asus->gpu_fan_curve_available,
+			ASUS_WMI_DEVID_GPU_FAN_CURVE);
+	if (err)
+		goto fail_custom_fan_curve;
+
 	err = platform_profile_setup(asus);
 	if (err)
 		goto fail_platform_profile_setup;
@@ -3109,6 +3592,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus_wmi_sysfs_exit(asus->platform_device);
 fail_sysfs:
 fail_throttle_thermal_policy:
+fail_custom_fan_curve:
 fail_platform_profile_setup:
 	if (asus->platform_profile_support)
 		platform_profile_remove();
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 17dc5cb6f3f2..a571b47ff362 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -77,6 +77,8 @@
 #define ASUS_WMI_DEVID_THERMAL_CTRL	0x00110011
 #define ASUS_WMI_DEVID_FAN_CTRL		0x00110012 /* deprecated */
 #define ASUS_WMI_DEVID_CPU_FAN_CTRL	0x00110013
+#define ASUS_WMI_DEVID_CPU_FAN_CURVE	0x00110024
+#define ASUS_WMI_DEVID_GPU_FAN_CURVE	0x00110025
 
 /* Power */
 #define ASUS_WMI_DEVID_PROCESSOR_STATE	0x00120012
-- 
2.31.1

Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[Hans de Goede]

Hi Luke,

On 8/29/21 9:14 AM, Luke D. Jones wrote:
> Add support for custom fan curves found on some ASUS ROG laptops.
> 
> - V1
>   + Initial patch work
> - V2
>   + Don't fail and remove wmi driver if error from
>     asus_wmi_evaluate_method_buf() if error is -ENODEV
> - V3
>   + Store the "default" fan curves
>   + Call throttle_thermal_policy_write() if a curve is erased to ensure
>     that the factory default for a profile is applied again
> - V4
>   + Do not apply default curves by default. Testers have found that the
>     default curves don't quite match actual no-curve behaviours
>   + Add method to enable/disable curves for each profile
> - V5
>   + Remove an unrequired function left over from previous iterations
>   + Ensure default curves are applied if user writes " " to a curve path
>   + Rename "active_fan_curve_profiles" to "enabled_fan_curve_profiles" to
>     better reflect the behavious of this setting
>   + Move throttle_thermal_policy_write_*pu_curves() and rename to
>     fan_curve_*pu_write()
>   + Merge fan_curve_check_valid() and fan_curve_write()
>   + Remove some leftover debug statements
> - V6
>   + Refactor data structs to store  array or u8 instead of strings.
>     This affects the entire patch except the enabled_fan_curves block
>   + Use sysfs_match_string in enabled_fan_curve block
>   + Add some extra comments to describe things
>   + Allow some variation in how fan curve input can be formatted
>   + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines per
>     fan+profile combo drastically

Thank you for your continued work on this. I read in the discussin of v5
that you discussed using the standard auto_point#_pwm, auto_point#_temp
pairs. I see here that you have decided to not go that route, may I ask
why ?

Regards,

Hans

Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[Luke Jones]

On Sun, Aug 29 2021 at 11:57:55 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi Luke,
> 
> On 8/29/21 9:14 AM, Luke D. Jones wrote:
>>  Add support for custom fan curves found on some ASUS ROG laptops.
>> 
>>  - V1
>>    + Initial patch work
>>  - V2
>>    + Don't fail and remove wmi driver if error from
>>      asus_wmi_evaluate_method_buf() if error is -ENODEV
>>  - V3
>>    + Store the "default" fan curves
>>    + Call throttle_thermal_policy_write() if a curve is erased to 
>> ensure
>>      that the factory default for a profile is applied again
>>  - V4
>>    + Do not apply default curves by default. Testers have found that 
>> the
>>      default curves don't quite match actual no-curve behaviours
>>    + Add method to enable/disable curves for each profile
>>  - V5
>>    + Remove an unrequired function left over from previous iterations
>>    + Ensure default curves are applied if user writes " " to a curve 
>> path
>>    + Rename "active_fan_curve_profiles" to 
>> "enabled_fan_curve_profiles" to
>>      better reflect the behavious of this setting
>>    + Move throttle_thermal_policy_write_*pu_curves() and rename to
>>      fan_curve_*pu_write()
>>    + Merge fan_curve_check_valid() and fan_curve_write()
>>    + Remove some leftover debug statements
>>  - V6
>>    + Refactor data structs to store  array or u8 instead of strings.
>>      This affects the entire patch except the enabled_fan_curves 
>> block
>>    + Use sysfs_match_string in enabled_fan_curve block
>>    + Add some extra comments to describe things
>>    + Allow some variation in how fan curve input can be formatted
>>    + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines per
>>      fan+profile combo drastically
> 
> Thank you for your continued work on this. I read in the discussin of 
> v5
> that you discussed using the standard auto_point#_pwm, 
> auto_point#_temp
> pairs. I see here that you have decided to not go that route, may I 
> ask
> why ?

Sure, primary reason is because the RPM for the fans is in percentage 
so it didn't really make sense to me to use that format.

Also if the max for that is 255 then I'd need to introduce scaling to 
make match what the ACPI method expects (max 100). But yeah, 
auto_point#_pwm changes the meaning.

> 
> Regards,
> 
> Hans
> 

Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[Guenter Roeck]

On 8/29/21 3:03 AM, Luke Jones wrote:
> 
> 
> On Sun, Aug 29 2021 at 11:57:55 +0200, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Luke,
>>
>> On 8/29/21 9:14 AM, Luke D. Jones wrote:
>>>  Add support for custom fan curves found on some ASUS ROG laptops.
>>>
>>>  - V1
>>>    + Initial patch work
>>>  - V2
>>>    + Don't fail and remove wmi driver if error from
>>>      asus_wmi_evaluate_method_buf() if error is -ENODEV
>>>  - V3
>>>    + Store the "default" fan curves
>>>    + Call throttle_thermal_policy_write() if a curve is erased to ensure
>>>      that the factory default for a profile is applied again
>>>  - V4
>>>    + Do not apply default curves by default. Testers have found that the
>>>      default curves don't quite match actual no-curve behaviours
>>>    + Add method to enable/disable curves for each profile
>>>  - V5
>>>    + Remove an unrequired function left over from previous iterations
>>>    + Ensure default curves are applied if user writes " " to a curve path
>>>    + Rename "active_fan_curve_profiles" to "enabled_fan_curve_profiles" to
>>>      better reflect the behavious of this setting
>>>    + Move throttle_thermal_policy_write_*pu_curves() and rename to
>>>      fan_curve_*pu_write()
>>>    + Merge fan_curve_check_valid() and fan_curve_write()
>>>    + Remove some leftover debug statements
>>>  - V6
>>>    + Refactor data structs to store  array or u8 instead of strings.
>>>      This affects the entire patch except the enabled_fan_curves block
>>>    + Use sysfs_match_string in enabled_fan_curve block
>>>    + Add some extra comments to describe things
>>>    + Allow some variation in how fan curve input can be formatted
>>>    + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines per
>>>      fan+profile combo drastically
>>
>> Thank you for your continued work on this. I read in the discussin of v5
>> that you discussed using the standard auto_point#_pwm, auto_point#_temp
>> pairs. I see here that you have decided to not go that route, may I ask
>> why ?
> 
> Sure, primary reason is because the RPM for the fans is in percentage so it didn't really make sense to me to use that format.
> 
> Also if the max for that is 255 then I'd need to introduce scaling to make match what the ACPI method expects (max 100). But yeah, auto_point#_pwm changes the meaning.
> 

Bad argument. That is true for other controllers as well. You could
just scale it up and down as needed.

The whole point of an ABI is that it is standardized.
If others would [be permitted to] follow your line of argument,
we would not have a useful ABI because "my chip provides/needs
data in some other format".

Guenter

Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[Luke Jones]

On Sun, Aug 29 2021 at 08:18:01 -0700, Guenter Roeck 
<linux@roeck-us.net> wrote:
> On 8/29/21 3:03 AM, Luke Jones wrote:
>> 
>> 
>> On Sun, Aug 29 2021 at 11:57:55 +0200, Hans de Goede 
>> <hdegoede@redhat.com> wrote:
>>> Hi Luke,
>>> 
>>> On 8/29/21 9:14 AM, Luke D. Jones wrote:
>>>>  Add support for custom fan curves found on some ASUS ROG laptops.
>>>> 
>>>>  - V1
>>>>    + Initial patch work
>>>>  - V2
>>>>    + Don't fail and remove wmi driver if error from
>>>>      asus_wmi_evaluate_method_buf() if error is -ENODEV
>>>>  - V3
>>>>    + Store the "default" fan curves
>>>>    + Call throttle_thermal_policy_write() if a curve is erased to 
>>>> ensure
>>>>      that the factory default for a profile is applied again
>>>>  - V4
>>>>    + Do not apply default curves by default. Testers have found 
>>>> that the
>>>>      default curves don't quite match actual no-curve behaviours
>>>>    + Add method to enable/disable curves for each profile
>>>>  - V5
>>>>    + Remove an unrequired function left over from previous 
>>>> iterations
>>>>    + Ensure default curves are applied if user writes " " to a 
>>>> curve path
>>>>    + Rename "active_fan_curve_profiles" to 
>>>> "enabled_fan_curve_profiles" to
>>>>      better reflect the behavious of this setting
>>>>    + Move throttle_thermal_policy_write_*pu_curves() and rename to
>>>>      fan_curve_*pu_write()
>>>>    + Merge fan_curve_check_valid() and fan_curve_write()
>>>>    + Remove some leftover debug statements
>>>>  - V6
>>>>    + Refactor data structs to store  array or u8 instead of 
>>>> strings.
>>>>      This affects the entire patch except the enabled_fan_curves 
>>>> block
>>>>    + Use sysfs_match_string in enabled_fan_curve block
>>>>    + Add some extra comments to describe things
>>>>    + Allow some variation in how fan curve input can be formatted
>>>>    + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines 
>>>> per
>>>>      fan+profile combo drastically
>>> 
>>> Thank you for your continued work on this. I read in the discussin 
>>> of v5
>>> that you discussed using the standard auto_point#_pwm, 
>>> auto_point#_temp
>>> pairs. I see here that you have decided to not go that route, may I 
>>> ask
>>> why ?
>> 
>> Sure, primary reason is because the RPM for the fans is in 
>> percentage so it didn't really make sense to me to use that format.
>> 
>> Also if the max for that is 255 then I'd need to introduce scaling 
>> to make match what the ACPI method expects (max 100). But yeah, 
>> auto_point#_pwm changes the meaning.
>> 
> 
> Bad argument. That is true for other controllers as well. You could
> just scale it up and down as needed.
> 
> The whole point of an ABI is that it is standardized.
> If others would [be permitted to] follow your line of argument,
> we would not have a useful ABI because "my chip provides/needs
> data in some other format".
> 
> Guenter

Understood. But lets see if I understand fully:

The key part is "pwmX_auto_pointN_temp and pwmX_auto_pointN_pwm", with 
X being a profile, and N the point in the curve graph. If I use this 
format I have:

- 3 profiles
- each profile has two fans
- each fan has 8 points on it
- each point has 2 integers

so that makes for a total of 96 individual sysfs paths. Is that really 
okay? And where would the new paths god?
- Under /sys/devices/platform/asus-nb-wmi/ still, or
- /sys/devices/platform/asus-nb-wmi/hwmon/ ?

I'm currently using SENSOR_DEVICE_ATTR_2_RW with index = profile, nr = 
fan. If there weren't profiles involved then I could see it being 
easily achieved with that.. Maybe I could use the index(profile) with a 
mask to get the fan number.

I've done all the groundwork for it at least, so it can certainly be 
done. My only worry is that because of the sheer number of sysfs paths 
being added (96) it could become unwieldy to use.

Could I use the existing method + the above?

Many thanks,
Luke.

Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[Luke Jones]

On Mon, Aug 30 2021 at 08:55:17 +1200, Luke Jones <luke@ljones.dev> 
wrote:
> 
> 
> On Sun, Aug 29 2021 at 08:18:01 -0700, Guenter Roeck 
> <linux@roeck-us.net> wrote:
>> On 8/29/21 3:03 AM, Luke Jones wrote:
>>> 
>>> 
>>> On Sun, Aug 29 2021 at 11:57:55 +0200, Hans de Goede 
>>> \x7f\x7f<hdegoede@redhat.com> wrote:
>>>> Hi Luke,
>>>> 
>>>> On 8/29/21 9:14 AM, Luke D. Jones wrote:
>>>>>  Add support for custom fan curves found on some ASUS ROG laptops.
>>>>> 
>>>>>  - V1
>>>>>    + Initial patch work
>>>>>  - V2
>>>>>    + Don't fail and remove wmi driver if error from
>>>>>      asus_wmi_evaluate_method_buf() if error is -ENODEV
>>>>>  - V3
>>>>>    + Store the "default" fan curves
>>>>>    + Call throttle_thermal_policy_write() if a curve is erased to 
>>>>> \x7f\x7f\x7f\x7fensure
>>>>>      that the factory default for a profile is applied again
>>>>>  - V4
>>>>>    + Do not apply default curves by default. Testers have found 
>>>>> \x7f\x7f\x7f\x7fthat the
>>>>>      default curves don't quite match actual no-curve behaviours
>>>>>    + Add method to enable/disable curves for each profile
>>>>>  - V5
>>>>>    + Remove an unrequired function left over from previous 
>>>>> \x7f\x7f\x7f\x7fiterations
>>>>>    + Ensure default curves are applied if user writes " " to a 
>>>>> \x7f\x7f\x7f\x7fcurve path
>>>>>    + Rename "active_fan_curve_profiles" to 
>>>>> \x7f\x7f\x7f\x7f"enabled_fan_curve_profiles" to
>>>>>      better reflect the behavious of this setting
>>>>>    + Move throttle_thermal_policy_write_*pu_curves() and rename to
>>>>>      fan_curve_*pu_write()
>>>>>    + Merge fan_curve_check_valid() and fan_curve_write()
>>>>>    + Remove some leftover debug statements
>>>>>  - V6
>>>>>    + Refactor data structs to store  array or u8 instead of 
>>>>> \x7f\x7f\x7f\x7fstrings.
>>>>>      This affects the entire patch except the enabled_fan_curves 
>>>>> \x7f\x7f\x7f\x7fblock
>>>>>    + Use sysfs_match_string in enabled_fan_curve block
>>>>>    + Add some extra comments to describe things
>>>>>    + Allow some variation in how fan curve input can be formatted
>>>>>    + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines 
>>>>> \x7f\x7f\x7f\x7fper
>>>>>      fan+profile combo drastically
>>>> 
>>>> Thank you for your continued work on this. I read in the discussin 
>>>> \x7f\x7f\x7fof v5
>>>> that you discussed using the standard auto_point#_pwm, 
>>>> \x7f\x7f\x7fauto_point#_temp
>>>> pairs. I see here that you have decided to not go that route, may 
>>>> I \x7f\x7f\x7fask
>>>> why ?
>>> 
>>> Sure, primary reason is because the RPM for the fans is in 
>>> \x7f\x7fpercentage so it didn't really make sense to me to use that 
>>> format.
>>> 
>>> Also if the max for that is 255 then I'd need to introduce scaling 
>>> \x7f\x7fto make match what the ACPI method expects (max 100). But yeah, 
>>> \x7f\x7fauto_point#_pwm changes the meaning.
>>> 
>> 
>> Bad argument. That is true for other controllers as well. You could
>> just scale it up and down as needed.
>> 
>> The whole point of an ABI is that it is standardized.
>> If others would [be permitted to] follow your line of argument,
>> we would not have a useful ABI because "my chip provides/needs
>> data in some other format".
>> 
>> Guenter
> 
> Understood. But lets see if I understand fully:
> 
> The key part is "pwmX_auto_pointN_temp and pwmX_auto_pointN_pwm", 
> with X being a profile, and N the point in the curve graph. If I use 
> this format I have:
> 
> - 3 profiles
> - each profile has two fans
> - each fan has 8 points on it
> - each point has 2 integers
> 
> so that makes for a total of 96 individual sysfs paths. Is that 
> really okay? And where would the new paths god?
> - Under /sys/devices/platform/asus-nb-wmi/ still, or
> - /sys/devices/platform/asus-nb-wmi/hwmon/ ?
> 
> I'm currently using SENSOR_DEVICE_ATTR_2_RW with index = profile, nr 
> = fan. If there weren't profiles involved then I could see it being 
> easily achieved with that.. Maybe I could use the index(profile) with 
> a mask to get the fan number.
> 
> I've done all the groundwork for it at least, so it can certainly be 
> done. My only worry is that because of the sheer number of sysfs 
> paths being added (96) it could become unwieldy to use.
> 
> Could I use the existing method + the above?

I've had a bit of a think after morning coffee and I think there is 
another way to do this:

- CPU Fan = pwm1_auto_pointN_pwm + pwm1_auto_pointN_temp
- GPU Fan = pwm2_auto_pointN_pwm + pwm2_auto_pointN_temp
for example. So we're not breaking the meaning of anything or making 
things obtuse and complex.

Ending up with:
/* CPU */
// (name, function, fan, point)
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_temp, fan_curve, 1, 0);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_temp, fan_curve, 1, 1);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_temp, fan_curve, 1, 2);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_temp, fan_curve, 1, 3);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_temp, fan_curve, 1, 4);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_temp, fan_curve, 1, 5);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_temp, fan_curve, 1, 6);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_temp, fan_curve, 1, 7);

static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_pwm, fan_curve, 1, 0);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_pwm, fan_curve, 1, 1);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_pwm, fan_curve, 1, 2);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_pwm, fan_curve, 1, 3);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_pwm, fan_curve, 1, 4);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_pwm, fan_curve, 1, 5);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_pwm, fan_curve, 1, 6);
static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_pwm, fan_curve, 1, 7);
/* and the same for GPU fan */

But because we still have 3 profiles to consider, I would propose that 
the settings be show/store dependant on the profile that the machine is 
in, e.g, internally show/store to correct profile via checking current 
profile number active.

I do need some suggestions on what I see as an issue though:
(1)
Given that it now becomes difficult to write all the settings at once, 
at what point should I attempt to write the "block" to the device? 
Write out for every change?
(2)
Also given the above, how do I reasonably check the user isn't trying 
to create an invalid graph? E.g, lower fan speeds for higher 
temperature? Check that a point isn't higher/lower than neighbouring 
points and expect users to write the points in reverse?

I could maybe also have pwm1_enable and pwm2_enable. Perhaps set this 
to false if a change is made, then only write out the full block if it 
is then set to enabled. Further to this, if the user changes profiles 
and the curves have been previously set and enabled, then that curve is 
written out per usual.

Looking forward to some guidance on this. I'll try making a start on 
what I've proposed above for now.


> Many thanks,
> Luke.
> 

Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves

[Luke Jones]

On Mon, Aug 30 2021 at 10:20:57 +1200, Luke Jones <luke@ljones.dev> 
wrote:
> 
> 
> On Mon, Aug 30 2021 at 08:55:17 +1200, Luke Jones <luke@ljones.dev> 
> wrote:
>> 
>> 
>> On Sun, Aug 29 2021 at 08:18:01 -0700, Guenter Roeck 
>> \x7f<linux@roeck-us.net> wrote:
>>> On 8/29/21 3:03 AM, Luke Jones wrote:
>>>> 
>>>> 
>>>> On Sun, Aug 29 2021 at 11:57:55 +0200, Hans de Goede 
>>>> \x7f\x7f\x7f\x7f\x7f<hdegoede@redhat.com> wrote:
>>>>> Hi Luke,
>>>>> 
>>>>> On 8/29/21 9:14 AM, Luke D. Jones wrote:
>>>>>>  Add support for custom fan curves found on some ASUS ROG 
>>>>>> laptops.
>>>>>> 
>>>>>>  - V1
>>>>>>    + Initial patch work
>>>>>>  - V2
>>>>>>    + Don't fail and remove wmi driver if error from
>>>>>>      asus_wmi_evaluate_method_buf() if error is -ENODEV
>>>>>>  - V3
>>>>>>    + Store the "default" fan curves
>>>>>>    + Call throttle_thermal_policy_write() if a curve is erased 
>>>>>> to \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fensure
>>>>>>      that the factory default for a profile is applied again
>>>>>>  - V4
>>>>>>    + Do not apply default curves by default. Testers have found 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fthat the
>>>>>>      default curves don't quite match actual no-curve behaviours
>>>>>>    + Add method to enable/disable curves for each profile
>>>>>>  - V5
>>>>>>    + Remove an unrequired function left over from previous 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fiterations
>>>>>>    + Ensure default curves are applied if user writes " " to a 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fcurve path
>>>>>>    + Rename "active_fan_curve_profiles" to 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f"enabled_fan_curve_profiles" to
>>>>>>      better reflect the behavious of this setting
>>>>>>    + Move throttle_thermal_policy_write_*pu_curves() and rename 
>>>>>> to
>>>>>>      fan_curve_*pu_write()
>>>>>>    + Merge fan_curve_check_valid() and fan_curve_write()
>>>>>>    + Remove some leftover debug statements
>>>>>>  - V6
>>>>>>    + Refactor data structs to store  array or u8 instead of 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fstrings.
>>>>>>      This affects the entire patch except the enabled_fan_curves 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fblock
>>>>>>    + Use sysfs_match_string in enabled_fan_curve block
>>>>>>    + Add some extra comments to describe things
>>>>>>    + Allow some variation in how fan curve input can be formatted
>>>>>>    + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fper
>>>>>>      fan+profile combo drastically
>>>>> 
>>>>> Thank you for your continued work on this. I read in the 
>>>>> discussin \x7f\x7f\x7f\x7f\x7f\x7f\x7fof v5
>>>>> that you discussed using the standard auto_point#_pwm, 
>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7fauto_point#_temp
>>>>> pairs. I see here that you have decided to not go that route, may 
>>>>> \x7f\x7f\x7f\x7fI \x7f\x7f\x7fask
>>>>> why ?
>>>> 
>>>> Sure, primary reason is because the RPM for the fans is in 
>>>> \x7f\x7f\x7f\x7f\x7fpercentage so it didn't really make sense to me to use that 
>>>> \x7f\x7f\x7fformat.
>>>> 
>>>> Also if the max for that is 255 then I'd need to introduce scaling 
>>>> \x7f\x7f\x7f\x7f\x7fto make match what the ACPI method expects (max 100). But 
>>>> yeah, \x7f\x7f\x7f\x7f\x7fauto_point#_pwm changes the meaning.
>>>> 
>>> 
>>> Bad argument. That is true for other controllers as well. You could
>>> just scale it up and down as needed.
>>> 
>>> The whole point of an ABI is that it is standardized.
>>> If others would [be permitted to] follow your line of argument,
>>> we would not have a useful ABI because "my chip provides/needs
>>> data in some other format".
>>> 
>>> Guenter
>> 
>> Understood. But lets see if I understand fully:
>> 
>> The key part is "pwmX_auto_pointN_temp and pwmX_auto_pointN_pwm", 
>> \x7fwith X being a profile, and N the point in the curve graph. If I 
>> use \x7fthis format I have:
>> 
>> - 3 profiles
>> - each profile has two fans
>> - each fan has 8 points on it
>> - each point has 2 integers
>> 
>> so that makes for a total of 96 individual sysfs paths. Is that 
>> \x7freally okay? And where would the new paths god?
>> - Under /sys/devices/platform/asus-nb-wmi/ still, or
>> - /sys/devices/platform/asus-nb-wmi/hwmon/ ?
>> 
>> I'm currently using SENSOR_DEVICE_ATTR_2_RW with index = profile, nr 
>> \x7f= fan. If there weren't profiles involved then I could see it being 
>> \x7feasily achieved with that.. Maybe I could use the index(profile) 
>> with \x7fa mask to get the fan number.
>> 
>> I've done all the groundwork for it at least, so it can certainly be 
>> \x7fdone. My only worry is that because of the sheer number of sysfs 
>> \x7fpaths being added (96) it could become unwieldy to use.
>> 
>> Could I use the existing method + the above?
> 
> I've had a bit of a think after morning coffee and I think there is 
> another way to do this:
> 
> - CPU Fan = pwm1_auto_pointN_pwm + pwm1_auto_pointN_temp
> - GPU Fan = pwm2_auto_pointN_pwm + pwm2_auto_pointN_temp
> for example. So we're not breaking the meaning of anything or making 
> things obtuse and complex.
> 
> Ending up with:
> /* CPU */
> // (name, function, fan, point)
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_temp, fan_curve, 1, 
> 0);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_temp, fan_curve, 1, 
> 1);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_temp, fan_curve, 1, 
> 2);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_temp, fan_curve, 1, 
> 3);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_temp, fan_curve, 1, 
> 4);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_temp, fan_curve, 1, 
> 5);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_temp, fan_curve, 1, 
> 6);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_temp, fan_curve, 1, 
> 7);
> 
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_pwm, fan_curve, 1, 0);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_pwm, fan_curve, 1, 1);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_pwm, fan_curve, 1, 2);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_pwm, fan_curve, 1, 3);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_pwm, fan_curve, 1, 4);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_pwm, fan_curve, 1, 5);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_pwm, fan_curve, 1, 6);
> static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_pwm, fan_curve, 1, 7);
> /* and the same for GPU fan */
> 
> But because we still have 3 profiles to consider, I would propose 
> that the settings be show/store dependant on the profile that the 
> machine is in, e.g, internally show/store to correct profile via 
> checking current profile number active.
> 
> I do need some suggestions on what I see as an issue though:
> (1)
> Given that it now becomes difficult to write all the settings at 
> once, at what point should I attempt to write the "block" to the 
> device? Write out for every change?
> (2)
> Also given the above, how do I reasonably check the user isn't trying 
> to create an invalid graph? E.g, lower fan speeds for higher 
> temperature? Check that a point isn't higher/lower than neighbouring 
> points and expect users to write the points in reverse?
> 
> I could maybe also have pwm1_enable and pwm2_enable. Perhaps set this 
> to false if a change is made, then only write out the full block if 
> it is then set to enabled. Further to this, if the user changes 
> profiles and the curves have been previously set and enabled, then 
> that curve is written out per usual.
> 
> Looking forward to some guidance on this. I'll try making a start on 
> what I've proposed above for now.
> 
> 
>> Many thanks,
>> Luke.
>> 
> 

I have completed the above now. So I'll now complete testing and then 
submit v7. What a journey, learning a lot.

Cheers!