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

[PATCH v7 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
- V7
  + Further refactor to use pwm1_auto_point1_temp + pwm1_auto_point1_pwm
    format, creating two blocks of attributes for CPU and GPU fans
  + Remove storing of defualt curves and method to reset them. The
    factory defaults are still populated in to structs on module load
    so users have a starting point

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

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

-- 
2.31.1

[PATCH v7] 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".

This patch adds two blocks of attributes to the hwmon sysfs,
1 block each for CPU and GPU fans.

When the user switches profiles the associated curve data for that
profile is then show/store enabled to allow users to rotate through
the profiles and set a fan curve for each profile which then
activates on profile switch if enabled.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index cc5811844012..b594c2475034 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -106,8 +106,18 @@ module_param(fnlock_default, bool, 0444);
 
 #define WMI_EVENT_MASK			0xFFFF
 
+/* The number of ASUS_THROTTLE_THERMAL_POLICY_* available */
+#define FAN_CURVE_PROFILE_NUM	(ASUS_THROTTLE_THERMAL_POLICY_SILENT + 1)
+#define FAN_CURVE_POINTS		8
+#define FAN_CURVE_DEV_CPU		0x00
+#define FAN_CURVE_DEV_GPU		0x01
+/* Mask to determine if setting temperature or percentage */
+#define FAN_CURVE_PWM_MASK		0x04
+
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
+static int throttle_thermal_policy_write(struct asus_wmi *);
+
 static bool ashs_present(void)
 {
 	int i = 0;
@@ -122,7 +132,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 +184,17 @@ enum fan_type {
 	FAN_TYPE_SPEC83,	/* starting in Spec 8.3, use CPU_FAN_CTRL */
 };
 
+/*
+ * 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 {
+	bool enabled;
+	u8 temps[FAN_CURVE_POINTS];
+	u8 percents[FAN_CURVE_POINTS];
+};
+
 struct asus_wmi {
 	int dsts_id;
 	int spec;
@@ -220,6 +242,11 @@ struct asus_wmi {
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
 
+	bool cpu_fan_curve_available;
+	bool gpu_fan_curve_available;
+	/* [throttle modes][fan count] */
+	struct fan_curve_data throttle_fan_curves[FAN_CURVE_PROFILE_NUM][2];
+
 	struct platform_profile_handler platform_profile_handler;
 	bool platform_profile_support;
 
@@ -285,6 +312,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 +2148,440 @@ 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 init_fan_curve(struct fan_curve_data *data, u8 *buf)
+{
+	int i;
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++)
+		data->temps[i] = buf[i];
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++)
+		data->percents[i] = buf[i + 8];
+}
+
+/*
+ * Check if the ability to set fan curves on either fan exists, and populate
+ * with system defaults 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[FAN_CURVE_POINTS * 2];
+	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);
+
+	/*
+	 * 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);
+
+	/* 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);
+
+	*available = true;
+	return 0;
+}
+
+static struct fan_curve_data *fan_curve_attr_data_select(struct device *dev,
+				struct device_attribute *attr)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 mode = asus->throttle_thermal_policy_mode;
+
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	int fan = nr & FAN_CURVE_DEV_GPU;
+
+	return &asus->throttle_fan_curves[mode][fan];
+}
+
+static ssize_t fan_curve_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+	int value;
+
+	int index = to_sensor_dev_attr_2(attr)->index;
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	int pwm = nr & FAN_CURVE_PWM_MASK;
+
+	if (pwm)
+		value = 255 * data->percents[index] / 100;
+	else
+		value = data->temps[index];
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+/*
+ * "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_data *data)
+{
+	int ret, i, shift = 0;
+	u32 arg1, arg2, arg3, arg4;
+
+	arg1 = arg2 = arg3 = arg4 = 0;
+
+	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
+		arg1 += data->temps[i] << shift;
+		arg2 += data->temps[i + 4] << shift;
+		arg3 += data->percents[0] << shift;
+		arg4 += data->percents[i + 4] << shift;
+		shift += 8;
+	}
+
+	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_CURVE_DEV_CPU];
+	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
+
+	if (cpu->enabled) {
+		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
+		if (err)
+			return err;
+	}
+
+	if (gpu->enabled) {
+		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int fan_curve_verify(struct fan_curve_data *data)
+{
+	int i = 0;
+	u8 tmp = 0;
+	u8 prev_tmp = 0;
+
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++) {
+		tmp = data->temps[i];
+		if (tmp < prev_tmp)
+			return -EINVAL;
+		prev_tmp = tmp;
+	}
+
+	tmp = 0;
+	prev_tmp = 0;
+	for (i = 0; i < FAN_CURVE_POINTS; i++) {
+		tmp = data->percents[i];
+		if (tmp < prev_tmp)
+			return -EINVAL;
+		prev_tmp = tmp;
+	}
+
+	return 0;
+}
+
+static ssize_t fan_curve_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+	u8 value, old_value;
+	int err;
+
+	int index = to_sensor_dev_attr_2(attr)->index;
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	int pwm = nr & FAN_CURVE_PWM_MASK;
+
+	err = kstrtou8(buf, 10, &value);
+	if (err < 0)
+		return err;
+
+	if (pwm) {
+		old_value = data->percents[index];
+		data->percents[index] = 100 * value / 255;
+	} else {
+		old_value = data->temps[index];
+		data->temps[index] = value;
+	}
+	/*
+	 * The check here forces writing a curve graph in reverse,
+	 * from highest to lowest.
+	 */
+	err = fan_curve_verify(data);
+	if (err) {
+		if (pwm) {
+			dev_err(dev, "a fan curve percentage was higher than the next in sequence\n");
+			data->percents[index] = old_value;
+		} else {
+			dev_err(dev, "a fan curve temperature was higher than the next in sequence\n");
+			data->temps[index] = old_value;
+		}
+		return err;
+	}
+
+	return count;
+}
+
+static ssize_t fan_curve_enable_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", data->enabled);
+}
+
+static ssize_t fan_curve_enable_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	bool value;
+	int err;
+
+	err = kstrtobool(buf, &value);
+	if (err < 0)
+		return err;
+
+	data->enabled = value;
+	throttle_thermal_policy_write(asus);
+
+	return count;
+}
+
+/* CPU */
+// TODO: enable
+static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable,
+				FAN_CURVE_DEV_CPU);
+// (name, function, fan, point) TODO: need to mask if temp or percent
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 7);
+
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 7);
+
+/* GPU */
+static SENSOR_DEVICE_ATTR_RW(pwm2_enable, fan_curve_enable,
+				FAN_CURVE_DEV_GPU);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point1_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point2_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point3_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point4_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point5_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point6_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point7_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point8_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 7);
+
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point1_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point2_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point3_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point4_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point5_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point6_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point7_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point8_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 7);
+
+static struct attribute *fan_curve_attributes[] = {
+	/* CPU */
+	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point7_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point8_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point6_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point7_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point8_pwm.dev_attr.attr,
+	/* GPU */
+	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point3_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point4_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point5_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point6_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point7_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point8_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point3_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point4_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point5_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point6_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point7_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point8_pwm.dev_attr.attr,
+	NULL
+};
+
+static umode_t fan_curve_sysfs_is_visible(struct kobject *kobj,
+					   struct attribute *attr, int idx)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct asus_wmi *asus = dev_get_drvdata(dev->parent);
+
+	if (attr == &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point7_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point8_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point6_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point7_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point8_pwm.dev_attr.attr) {
+		if (!asus->cpu_fan_curve_available)
+			return 0;
+	}
+
+	if (attr == &sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point3_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point4_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point5_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point6_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point7_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point8_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point3_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point4_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point5_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point6_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point7_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point8_pwm.dev_attr.attr) {
+		if (!asus->gpu_fan_curve_available)
+			return 0;
+	}
+
+	return attr->mode;
+}
+
+static const struct attribute_group fan_curve_attribute_group = {
+	.is_visible = fan_curve_sysfs_is_visible,
+	.attrs = fan_curve_attributes
+};
+__ATTRIBUTE_GROUPS(fan_curve_attribute);
+
+static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
+{
+	struct device *dev = &asus->platform_device->dev;
+	struct device *hwmon;
+
+	hwmon = devm_hwmon_device_register_with_groups(dev, "asus", asus,
+						fan_curve_attribute_groups);
+
+	if (IS_ERR(hwmon)) {
+		pr_err("Could not register asus fan_curve device\n");
+		return PTR_ERR(hwmon);
+	}
+
+	return 0;
+}
+
 /* Throttle thermal policy ****************************************************/
 
 static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
@@ -2053,8 +2592,8 @@ static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
 	asus->throttle_thermal_policy_available = false;
 
 	err = asus_wmi_get_devstate(asus,
-				    ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
-				    &result);
+		ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
+		&result);
 	if (err) {
 		if (err == -ENODEV)
 			return 0;
@@ -2092,6 +2631,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;
 }
 
@@ -2904,7 +3449,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 +3561,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;
@@ -3038,6 +3593,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_hwmon;
 
+	err = asus_wmi_fan_curve_init(asus);
+	if (err)
+		goto fail_custom_fan_curve;
+
 	err = asus_wmi_led_init(asus);
 	if (err)
 		goto fail_leds;
@@ -3109,6 +3668,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 v7] asus-wmi: Add support for custom fan curves

[Barnabás Pőcze]

Hi


2021. augusztus 30., hétfő 13:31 keltezéssel, Luke D. Jones írta:
> 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".
>
> This patch adds two blocks of attributes to the hwmon sysfs,
> 1 block each for CPU and GPU fans.
>
> When the user switches profiles the associated curve data for that
> profile is then show/store enabled to allow users to rotate through
> the profiles and set a fan curve for each profile which then
> activates on profile switch if enabled.
>
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 568 ++++++++++++++++++++-
>  include/linux/platform_data/x86/asus-wmi.h |   2 +
>  2 files changed, 566 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index cc5811844012..b594c2475034 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> [...]
> +/*
> + * Returns as an error if the method output is not a buffer. Typically this

It seems to me it will simply leave the output buffer uninitialized if something
other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and return 0.


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

Is anything known about the possible values? You are later
using it as if it was an errno (e.g. in `custom_fan_check_present()`).

And `obj` is leaked in both of the previous two returns.


> +	}
> +
> +	if (obj && obj->type == ACPI_TYPE_BUFFER)
> +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);

I would suggest you add a "size_t size" argument to this function, and
return -ENOSPC/-ENODATA depending on whether the returned buffer is too
big/small. Maybe return -ENODATA if `obj` is NULL, too.


> +
> +	kfree(obj);
> +
> +	return 0;
> +}
> [...]
> +static ssize_t fan_curve_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +	int value;
> +
> +	int index = to_sensor_dev_attr_2(attr)->index;
> +	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	int pwm = nr & FAN_CURVE_PWM_MASK;
> +
> +	if (pwm)
> +		value = 255 * data->percents[index] / 100;
> +	else
> +		value = data->temps[index];
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", value);

sysfs_emit()


> +}
> +
> +/*
> + * "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_data *data)
> +{
> +	int ret, i, shift = 0;
> +	u32 arg1, arg2, arg3, arg4;
> +
> +	arg1 = arg2 = arg3 = arg4 = 0;
> +
> +	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
> +		arg1 += data->temps[i] << shift;
> +		arg2 += data->temps[i + 4] << shift;
> +		arg3 += data->percents[0] << shift;
> +		arg4 += data->percents[i + 4] << shift;
> +		shift += 8;
> +	}
> +
> +	return asus_wmi_evaluate_method5(ASUS_WMI_METHODID_DEVS, dev,
> +					arg1, arg2, arg3, arg4, &ret);
> +}
> +
> +/*
> + * Called only by throttle_thermal_policy_write()
> + */

Am I correct in thinking that the firmware does not actually
support specifying fan curves for each mode, only a single one,
and the fan curve switching is done by this driver when
the performance mode is changed?


> +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_CURVE_DEV_CPU];
> +	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
> +
> +	if (cpu->enabled) {
> +		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (gpu->enabled) {
> +		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> [...]
> +static ssize_t fan_curve_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +	u8 value, old_value;
> +	int err;
> +
> +	int index = to_sensor_dev_attr_2(attr)->index;
> +	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	int pwm = nr & FAN_CURVE_PWM_MASK;
> +
> +	err = kstrtou8(buf, 10, &value);
> +	if (err < 0)
> +		return err;
> +
> +	if (pwm) {
> +		old_value = data->percents[index];
> +		data->percents[index] = 100 * value / 255;
> +	} else {
> +		old_value = data->temps[index];
> +		data->temps[index] = value;
> +	}
> +	/*
> +	 * The check here forces writing a curve graph in reverse,
> +	 * from highest to lowest.
> +	 */
> +	err = fan_curve_verify(data);
> +	if (err) {
> +		if (pwm) {
> +			dev_err(dev, "a fan curve percentage was higher than the next in sequence\n");
> +			data->percents[index] = old_value;
> +		} else {
> +			dev_err(dev, "a fan curve temperature was higher than the next in sequence\n");
> +			data->temps[index] = old_value;
> +		}
> +		return err;
> +	}

Are such sequences rejected by the firmware itself?
Or is this just an extra layer of protection?


> +
> +	return count;
> +}
> +
> +static ssize_t fan_curve_enable_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", data->enabled);

sysfs_emit()


> +}
> +
> +static ssize_t fan_curve_enable_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	bool value;
> +	int err;
> +
> +	err = kstrtobool(buf, &value);
> +	if (err < 0)
> +		return err;
> +
> +	data->enabled = value;
> +	throttle_thermal_policy_write(asus);
> +
> +	return count;
> +}
> +
> +/* CPU */
> +// TODO: enable
> +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable,
> +				FAN_CURVE_DEV_CPU);

FYI, the pwmX_enable attributes can be created by the hwmon
subsystem itself if you use [devm_]hwmon_device_register_with_info()
with appropriately populated `struct hwmon_chip_info`.


> [...]
> +static const struct attribute_group fan_curve_attribute_group = {
> +	.is_visible = fan_curve_sysfs_is_visible,
> +	.attrs = fan_curve_attributes

Small thing, but it is customary to put commas after non-terminating
entries in initializers / enum definitions.


> +};
> +__ATTRIBUTE_GROUPS(fan_curve_attribute);
> +
> +static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
> +{
> +	struct device *dev = &asus->platform_device->dev;
> +	struct device *hwmon;
> +
> +	hwmon = devm_hwmon_device_register_with_groups(dev, "asus", asus,
> +						fan_curve_attribute_groups);
> +
> +	if (IS_ERR(hwmon)) {
> +		pr_err("Could not register asus fan_curve device\n");

I think `dev_err()` would be better.


> +		return PTR_ERR(hwmon);
> +	}
> +
> +	return 0;
> +}
> [...]
> 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


Best regards,
Barnabás Pőcze

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

[Luke Jones]

Hi

On Mon, Aug 30 2021 at 21:28:18 +0000, Barnabás Pőcze 
<pobrn@protonmail.com> wrote:
> Hi
> 
> 
> 2021. augusztus 30., hétfő 13:31 keltezéssel, Luke D. Jones írta:
>>  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".
>> 
>>  This patch adds two blocks of attributes to the hwmon sysfs,
>>  1 block each for CPU and GPU fans.
>> 
>>  When the user switches profiles the associated curve data for that
>>  profile is then show/store enabled to allow users to rotate through
>>  the profiles and set a fan curve for each profile which then
>>  activates on profile switch if enabled.
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 568 
>> ++++++++++++++++++++-
>>   include/linux/platform_data/x86/asus-wmi.h |   2 +
>>   2 files changed, 566 insertions(+), 4 deletions(-)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index cc5811844012..b594c2475034 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  [...]
>>  +/*
>>  + * Returns as an error if the method output is not a buffer. 
>> Typically this
> 
> It seems to me it will simply leave the output buffer uninitialized 
> if something
> other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and 
> return 0.

Oops, see below inline reply:

> 
> 
>>  + * 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;
> 
> Is anything known about the possible values? You are later
> using it as if it was an errno (e.g. in `custom_fan_check_present()`).
> 
> And `obj` is leaked in both of the previous two returns.

The return for the method we're calling in this patch returns 0 if the 
input arg has no match.

> 
> 
>>  +	}
>>  +
>>  +	if (obj && obj->type == ACPI_TYPE_BUFFER)
>>  +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
> 
> I would suggest you add a "size_t size" argument to this function, and
> return -ENOSPC/-ENODATA depending on whether the returned buffer is 
> too
> big/small. Maybe return -ENODATA if `obj` is NULL, too.

Got it. So something like this would be suitable?

	if (obj && obj->type == ACPI_TYPE_BUFFER)
		if (obj->buffer.length < size)
			err = -ENOSPC;
		if (!obj->buffer.length)
			err = -ENODATA;
		if (err) {
			kfree(obj);
			return err;
		}
		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
	}

	if (obj && obj->type == ACPI_TYPE_INTEGER)
		int_tmp = (u32) obj->integer.value;

	kfree(obj);

	if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
		return -ENODEV;

	/* There is at least one method that returns a 0 with no buffer */
	if (obj == NULL || int_tmp == 0)
		return -ENODATA;

	return 0;

> 
> 
>>  +
>>  +	kfree(obj);
>>  +
>>  +	return 0;
>>  +}
>>  [...]
>>  +static ssize_t fan_curve_show(struct device *dev,
>>  +				struct device_attribute *attr, char *buf)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +	int value;
>>  +
>>  +	int index = to_sensor_dev_attr_2(attr)->index;
>>  +	int nr = to_sensor_dev_attr_2(attr)->nr;
>>  +	int pwm = nr & FAN_CURVE_PWM_MASK;
>>  +
>>  +	if (pwm)
>>  +		value = 255 * data->percents[index] / 100;
>>  +	else
>>  +		value = data->temps[index];
>>  +
>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", value);
> 
> sysfs_emit()

Sorry, I usually go by existing code as examples. I might submit a 
patch after this one that cleans up some of the other parts.

> 
> 
>>  +}
>>  +
>>  +/*
>>  + * "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_data *data)
>>  +{
>>  +	int ret, i, shift = 0;
>>  +	u32 arg1, arg2, arg3, arg4;
>>  +
>>  +	arg1 = arg2 = arg3 = arg4 = 0;
>>  +
>>  +	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
>>  +		arg1 += data->temps[i] << shift;
>>  +		arg2 += data->temps[i + 4] << shift;
>>  +		arg3 += data->percents[0] << shift;
>>  +		arg4 += data->percents[i + 4] << shift;
>>  +		shift += 8;
>>  +	}
>>  +
>>  +	return asus_wmi_evaluate_method5(ASUS_WMI_METHODID_DEVS, dev,
>>  +					arg1, arg2, arg3, arg4, &ret);
>>  +}
>>  +
>>  +/*
>>  + * Called only by throttle_thermal_policy_write()
>>  + */
> 
> Am I correct in thinking that the firmware does not actually
> support specifying fan curves for each mode, only a single one,
> and the fan curve switching is done by this driver when
> the performance mode is changed?

I'm not 100% certain on this. The WMI method 0x00110024 takes an arg 
0,1,2 which then returns some factory stored fan profiles, these fit 
the profiles of ASUS_THROTTLE_THERMAL_POLICY_*, but with 1 and 2 
swapped.

Looking at the SET part, it seems to write to a different location than 
where the GET is fetching information.

Because of the fact there are three sets of curves to get, I thought it 
would be good for users to be able to set per profile. I don't think 
the set is retained in acpi if the profile is switched.

Do you think it would be best to not have the ability to store per 
profile in kernel? How would I choose which profile get to populate the 
initial data with if so?

> 
> 
>>  +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_CURVE_DEV_CPU];
>>  +	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
>>  +
>>  +	if (cpu->enabled) {
>>  +		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
>>  +		if (err)
>>  +			return err;
>>  +	}
>>  +
>>  +	if (gpu->enabled) {
>>  +		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
>>  +		if (err)
>>  +			return err;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  [...]
>>  +static ssize_t fan_curve_store(struct device *dev,
>>  +				struct device_attribute *attr,
>>  +				const char *buf, size_t count)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +	u8 value, old_value;
>>  +	int err;
>>  +
>>  +	int index = to_sensor_dev_attr_2(attr)->index;
>>  +	int nr = to_sensor_dev_attr_2(attr)->nr;
>>  +	int pwm = nr & FAN_CURVE_PWM_MASK;
>>  +
>>  +	err = kstrtou8(buf, 10, &value);
>>  +	if (err < 0)
>>  +		return err;
>>  +
>>  +	if (pwm) {
>>  +		old_value = data->percents[index];
>>  +		data->percents[index] = 100 * value / 255;
>>  +	} else {
>>  +		old_value = data->temps[index];
>>  +		data->temps[index] = value;
>>  +	}
>>  +	/*
>>  +	 * The check here forces writing a curve graph in reverse,
>>  +	 * from highest to lowest.
>>  +	 */
>>  +	err = fan_curve_verify(data);
>>  +	if (err) {
>>  +		if (pwm) {
>>  +			dev_err(dev, "a fan curve percentage was higher than the next 
>> in sequence\n");
>>  +			data->percents[index] = old_value;
>>  +		} else {
>>  +			dev_err(dev, "a fan curve temperature was higher than the next 
>> in sequence\n");
>>  +			data->temps[index] = old_value;
>>  +		}
>>  +		return err;
>>  +	}
> 
> Are such sequences rejected by the firmware itself?
> Or is this just an extra layer of protection?

Not really sure here. The DSDT I have is the following which looks like 
it writes as is. I don't really want to be responsible for allowing a 
reverse curve to be set.

// SET
If ((IIA0 == 0x00110024))
{
    Return (^^PCI0.SBRG.EC0.SUFC (IIA1, IIA2, IIA3, IIA4, 0x40))
}

If ((IIA0 == 0x00110025))
{
    Return (^^PCI0.SBRG.EC0.SUFC (IIA1, IIA2, IIA3, IIA4, 0x44))
}

Method (SUFC, 5, NotSerialized)
{
    Name (DUBF, Buffer (0x10)
    {
        /* 0000 */  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  // 
........
        /* 0008 */  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00   // 
........
    })
    Name (UFC0, Buffer (One)
    {
         0x00                                             // .
    })
    DUBF [Zero] = (Arg0 >> Zero)
    DUBF [One] = (Arg0 >> 0x08)
    DUBF [0x02] = (Arg0 >> 0x10)
    DUBF [0x03] = (Arg0 >> 0x18)
    DUBF [0x04] = (Arg1 >> Zero)
    DUBF [0x05] = (Arg1 >> 0x08)
    DUBF [0x06] = (Arg1 >> 0x10)
    DUBF [0x07] = (Arg1 >> 0x18)
    DUBF [0x08] = (Arg2 >> Zero)
    DUBF [0x09] = (Arg2 >> 0x08)
    DUBF [0x0A] = (Arg2 >> 0x10)
    DUBF [0x0B] = (Arg2 >> 0x18)
    DUBF [0x0C] = (Arg3 >> Zero)
    DUBF [0x0D] = (Arg3 >> 0x08)
    DUBF [0x0E] = (Arg3 >> 0x10)
    DUBF [0x0F] = (Arg3 >> 0x18)
    UFC0 [Zero] = Arg4
    WEBC (0x20, One, UFC0)
    Return (WEBC (0x22, 0x10, DUBF))
}

> 
> 
>>  +
>>  +	return count;
>>  +}
>>  +
>>  +static ssize_t fan_curve_enable_show(struct device *dev,
>>  +				struct device_attribute *attr, char *buf)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +
>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", data->enabled);
> 
> sysfs_emit()

Ack

> 
> 
>>  +}
>>  +
>>  +static ssize_t fan_curve_enable_store(struct device *dev,
>>  +				struct device_attribute *attr,
>>  +				const char *buf, size_t count)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +	struct asus_wmi *asus = dev_get_drvdata(dev);
>>  +	bool value;
>>  +	int err;
>>  +
>>  +	err = kstrtobool(buf, &value);
>>  +	if (err < 0)
>>  +		return err;
>>  +
>>  +	data->enabled = value;
>>  +	throttle_thermal_policy_write(asus);
>>  +
>>  +	return count;
>>  +}
>>  +
>>  +/* CPU */
>>  +// TODO: enable
>>  +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable,
>>  +				FAN_CURVE_DEV_CPU);
> 
> FYI, the pwmX_enable attributes can be created by the hwmon
> subsystem itself if you use [devm_]hwmon_device_register_with_info()
> with appropriately populated `struct hwmon_chip_info`.

Thanks, I was hoping for a method liek this and had found the same last 
night but was unclear on how I could retain the data I need from the 
following?

static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point1_pwm, fan_curve,
			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 0);


> 
> 
>>  [...]
>>  +static const struct attribute_group fan_curve_attribute_group = {
>>  +	.is_visible = fan_curve_sysfs_is_visible,
>>  +	.attrs = fan_curve_attributes
> 
> Small thing, but it is customary to put commas after non-terminating
> entries in initializers / enum definitions.

Ack

> 
> 
>>  +};
>>  +__ATTRIBUTE_GROUPS(fan_curve_attribute);
>>  +
>>  +static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
>>  +{
>>  +	struct device *dev = &asus->platform_device->dev;
>>  +	struct device *hwmon;
>>  +
>>  +	hwmon = devm_hwmon_device_register_with_groups(dev, "asus", asus,
>>  +						fan_curve_attribute_groups);
>>  +
>>  +	if (IS_ERR(hwmon)) {
>>  +		pr_err("Could not register asus fan_curve device\n");
> 
> I think `dev_err()` would be better.

Ack. Sorry, I have been trying to keep using dev_err.

> 
> 
>>  +		return PTR_ERR(hwmon);
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  [...]
>>  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
> 
> 
> Best regards,
> Barnabás Pőcze

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

[Barnabás Pőcze]

Hi


> [...]
> >>  --- a/drivers/platform/x86/asus-wmi.c
> >>  +++ b/drivers/platform/x86/asus-wmi.c
> >>  [...]
> >>  +/*
> >>  + * Returns as an error if the method output is not a buffer.
> >> Typically this
> >
> > It seems to me it will simply leave the output buffer uninitialized
> > if something
> > other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and
> > return 0.
>
> Oops, see below inline reply:
>
> >
> >
> >>  + * 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;
> >
> > Is anything known about the possible values? You are later
> > using it as if it was an errno (e.g. in `custom_fan_check_present()`).
> >
> > And `obj` is leaked in both of the previous two returns.
>
> The return for the method we're calling in this patch returns 0 if the
> input arg has no match.
>
> >
> >
> >>  +	}
> >>  +
> >>  +	if (obj && obj->type == ACPI_TYPE_BUFFER)
> >>  +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
> >
> > I would suggest you add a "size_t size" argument to this function, and
> > return -ENOSPC/-ENODATA depending on whether the returned buffer is
> > too
> > big/small. Maybe return -ENODATA if `obj` is NULL, too.
>
> Got it. So something like this would be suitable?
>
> 	if (obj && obj->type == ACPI_TYPE_BUFFER)
> 		if (obj->buffer.length < size)
> 			err = -ENOSPC;
> 		if (!obj->buffer.length)
> 			err = -ENODATA;
> 		if (err) {
> 			kfree(obj);
> 			return err;
> 		}
> 		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
> 	}
>
> 	if (obj && obj->type == ACPI_TYPE_INTEGER)
> 		int_tmp = (u32) obj->integer.value;
>
> 	kfree(obj);
>
> 	if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
> 		return -ENODEV;
>
> 	/* There is at least one method that returns a 0 with no buffer */
> 	if (obj == NULL || int_tmp == 0)
> 		return -ENODATA;
>
> 	return 0;
>

I had something like the following in mind:

  int err = 0;
  /* ... */
  obj = output.pointer;
  if (!obj)
    return -ENODATA;

  switch (obj->type) {
  case ACPI_TYPE_BUFFER:
    if (obj->buffer.length < size)
      err = -ENODATA;
    else if (obj->buffer.length > size)
      err = -ENOSPC;
    else
      memcpy(ret_buffer, obj->buffer.pointer, size);
    break;
  case ACPI_TYPE_INTEGER:
    switch (obj->integer.value) {
      case ASUS_WMI_UNSUPPORTED_METHOD:
        err = -EOPNOTSUPP;
	break;
      default:
        err = -ENODATA;
	break;
    }
    break;
  default:
    err = -ENODATA;
    break;
  }

  kfree(obj);

  return err;


> >
> >
> >>  +
> >>  +	kfree(obj);
> >>  +
> >>  +	return 0;
> >>  +}
> [...]
> >>  +/*
> >>  + * Called only by throttle_thermal_policy_write()
> >>  + */
> >
> > Am I correct in thinking that the firmware does not actually
> > support specifying fan curves for each mode, only a single one,
> > and the fan curve switching is done by this driver when
> > the performance mode is changed?
>
> I'm not 100% certain on this. The WMI method 0x00110024 takes an arg
> 0,1,2 which then returns some factory stored fan profiles, these fit
> the profiles of ASUS_THROTTLE_THERMAL_POLICY_*, but with 1 and 2
> swapped.
>
> Looking at the SET part, it seems to write to a different location than
> where the GET is fetching information.
>

The, unfortunately, that is not as simple as I initially thought...


> Because of the fact there are three sets of curves to get, I thought it
> would be good for users to be able to set per profile. I don't think
> the set is retained in acpi if the profile is switched.
>
> Do you think it would be best to not have the ability to store per
> profile in kernel?

If there was a method to set a fan curve, and one to retrieve it,
I would suggest just exposing that via the pwmN_auto_pointM_{pwm,temp}
attributes on a hwmon device, and that the profile-dependent switching
be implemented somewhere else. As far as I see, there is already
existing infrastructure for integrating such a feature [0]
(but please correct me if I'm wrong).

This would simplify the kernel code, add no new ABI, and
potentially provide greater control over policy for the
user space.


> How would I choose which profile get to populate the
> initial data with if so?

I assume there isn't a method that can query
the current fan curve (or it is unknown)?


> [...]

[0]: https://gitlab.com/asus-linux/asusctl


Best regards,
Barnabás Pőcze

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

[Luke Jones]

On Wed, Sep 1 2021 at 15:24:40 +0000, Barnabás Pőcze 
<pobrn@protonmail.com> wrote:
> Hi
> 
> 
>>  [...]
>>  >>  --- a/drivers/platform/x86/asus-wmi.c
>>  >>  +++ b/drivers/platform/x86/asus-wmi.c
>>  >>  [...]
>>  >>  +/*
>>  >>  + * Returns as an error if the method output is not a buffer.
>>  >> Typically this
>>  >
>>  > It seems to me it will simply leave the output buffer 
>> uninitialized
>>  > if something
>>  > other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered 
>> and
>>  > return 0.
>> 
>>  Oops, see below inline reply:
>> 
>>  >
>>  >
>>  >>  + * 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;
>>  >
>>  > Is anything known about the possible values? You are later
>>  > using it as if it was an errno (e.g. in 
>> `custom_fan_check_present()`).
>>  >
>>  > And `obj` is leaked in both of the previous two returns.
>> 
>>  The return for the method we're calling in this patch returns 0 if 
>> the
>>  input arg has no match.
>> 
>>  >
>>  >
>>  >>  +	}
>>  >>  +
>>  >>  +	if (obj && obj->type == ACPI_TYPE_BUFFER)
>>  >>  +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
>>  >
>>  > I would suggest you add a "size_t size" argument to this 
>> function, and
>>  > return -ENOSPC/-ENODATA depending on whether the returned buffer 
>> is
>>  > too
>>  > big/small. Maybe return -ENODATA if `obj` is NULL, too.
>> 
>>  Got it. So something like this would be suitable?
>> 
>>  	if (obj && obj->type == ACPI_TYPE_BUFFER)
>>  		if (obj->buffer.length < size)
>>  			err = -ENOSPC;
>>  		if (!obj->buffer.length)
>>  			err = -ENODATA;
>>  		if (err) {
>>  			kfree(obj);
>>  			return err;
>>  		}
>>  		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
>>  	}
>> 
>>  	if (obj && obj->type == ACPI_TYPE_INTEGER)
>>  		int_tmp = (u32) obj->integer.value;
>> 
>>  	kfree(obj);
>> 
>>  	if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
>>  		return -ENODEV;
>> 
>>  	/* There is at least one method that returns a 0 with no buffer */
>>  	if (obj == NULL || int_tmp == 0)
>>  		return -ENODATA;
>> 
>>  	return 0;
>> 
> 
> I had something like the following in mind:
> 
>   int err = 0;
>   /* ... */
>   obj = output.pointer;
>   if (!obj)
>     return -ENODATA;
> 
>   switch (obj->type) {
>   case ACPI_TYPE_BUFFER:
>     if (obj->buffer.length < size)
>       err = -ENODATA;
>     else if (obj->buffer.length > size)
>       err = -ENOSPC;
>     else
>       memcpy(ret_buffer, obj->buffer.pointer, size);
>     break;
>   case ACPI_TYPE_INTEGER:
>     switch (obj->integer.value) {
>       case ASUS_WMI_UNSUPPORTED_METHOD:
>         err = -EOPNOTSUPP;
> 	break;
>       default:
>         err = -ENODATA;
> 	break;
>     }
>     break;
>   default:
>     err = -ENODATA;
>     break;
>   }
> 
>   kfree(obj);
> 
>   return err;
> 

Got it. Sometimes I forget switch/case exists. I'll adjust the v8 patch 
I sent out earlier.

> 
>>  >
>>  >
>>  >>  +
>>  >>  +	kfree(obj);
>>  >>  +
>>  >>  +	return 0;
>>  >>  +}
>>  [...]
>>  >>  +/*
>>  >>  + * Called only by throttle_thermal_policy_write()
>>  >>  + */
>>  >
>>  > Am I correct in thinking that the firmware does not actually
>>  > support specifying fan curves for each mode, only a single one,
>>  > and the fan curve switching is done by this driver when
>>  > the performance mode is changed?
>> 
>>  I'm not 100% certain on this. The WMI method 0x00110024 takes an arg
>>  0,1,2 which then returns some factory stored fan profiles, these fit
>>  the profiles of ASUS_THROTTLE_THERMAL_POLICY_*, but with 1 and 2
>>  swapped.
>> 
>>  Looking at the SET part, it seems to write to a different location 
>> than
>>  where the GET is fetching information.
>> 
> 
> The, unfortunately, that is not as simple as I initially thought...

We can add the fact that a variation exists with a more typical setup 
also. The G713Q has no throttle_thermal and in the dsdt dump looks like 
it possible can read back the curve that is set by the user. This works 
in our favour though.

> 
> 
>>  Because of the fact there are three sets of curves to get, I 
>> thought it
>>  would be good for users to be able to set per profile. I don't think
>>  the set is retained in acpi if the profile is switched.
>> 
>>  Do you think it would be best to not have the ability to store per
>>  profile in kernel?
> 
> If there was a method to set a fan curve, and one to retrieve it,
> I would suggest just exposing that via the pwmN_auto_pointM_{pwm,temp}
> attributes on a hwmon device, and that the profile-dependent switching
> be implemented somewhere else. As far as I see, there is already
> existing infrastructure for integrating such a feature [0]
> (but please correct me if I'm wrong).

There is. I develop asusctl in conjunction with these patches. I'd 
certainly like to find the best way to fit all of this together.

> 
> This would simplify the kernel code, add no new ABI, and
> potentially provide greater control over policy for the
> user space.

I agree.

> 
> 
>>  How would I choose which profile get to populate the
>>  initial data with if so?
> 
> I assume there isn't a method that can query
> the current fan curve (or it is unknown)?

It looks like I need to adjust how pwm[n]_enable works anyway:

`pwm[1-*]_enable`
Fan speed control method:
- 0: no fan speed control (i.e. fan at full speed)
- 1: manual fan speed control enabled (using `pwm[1-*]`)
- 2+: automatic fan speed control enabled

So maybe on "manual" I can make it so the get method does what is in 
fan_curve_check_present() (and change that part also) and fetches the 
"defaults" on enable. This might even give us the fan curve that was 
set (and looks like it will on the machine that has no thermal throttle 
profile, v8 patch) - I'll try this anyway and see what it looks like. 
This seems to be the best approach given how the G713Q works anyway.

The issue I have with the above is that it overwrites any curve set. 
But given that it'll most likely be managed in userspace that's maybe 
*not* an issue. Otherwise would it be sensible to add something like 
`pwm1_reset`? I don't see anything like that in the related docs 
though. `pwm1_reset` would be to re-read the defaults from the acpi 
method.

Central to the above is that we can still read 0, 1, 2 curves from acpi 
- I was thinking to use the throttle_thermal mode to choose which one 
and that would be the only use of it. And won't store them as 
per-profile, which becomes a moot point when userspace is managing it 
anyway.

Many thanks,
Luke.


> 
> 
>>  [...]
> 
> [0]: https://gitlab.com/asus-linux/asusctl
> 
> 
> Best regards,
> Barnabás Pőcze

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

[Hans de Goede]

Hi,

On 9/2/21 12:01 AM, Luke Jones wrote:
> 
> 
> On Wed, Sep 1 2021 at 15:24:40 +0000, Barnabás Pőcze <pobrn@protonmail.com> wrote:
>> Hi
>>
>>
>>>  [...]
>>>  >>  --- a/drivers/platform/x86/asus-wmi.c
>>>  >>  +++ b/drivers/platform/x86/asus-wmi.c
>>>  >>  [...]
>>>  >>  +/*
>>>  >>  + * Returns as an error if the method output is not a buffer.
>>>  >> Typically this
>>>  >
>>>  > It seems to me it will simply leave the output buffer uninitialized
>>>  > if something
>>>  > other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and
>>>  > return 0.
>>>
>>>  Oops, see below inline reply:
>>>
>>>  >
>>>  >
>>>  >>  + * 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;
>>>  >
>>>  > Is anything known about the possible values? You are later
>>>  > using it as if it was an errno (e.g. in `custom_fan_check_present()`).
>>>  >
>>>  > And `obj` is leaked in both of the previous two returns.
>>>
>>>  The return for the method we're calling in this patch returns 0 if the
>>>  input arg has no match.
>>>
>>>  >
>>>  >
>>>  >>  +    }
>>>  >>  +
>>>  >>  +    if (obj && obj->type == ACPI_TYPE_BUFFER)
>>>  >>  +        memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
>>>  >
>>>  > I would suggest you add a "size_t size" argument to this function, and
>>>  > return -ENOSPC/-ENODATA depending on whether the returned buffer is
>>>  > too
>>>  > big/small. Maybe return -ENODATA if `obj` is NULL, too.
>>>
>>>  Got it. So something like this would be suitable?
>>>
>>>      if (obj && obj->type == ACPI_TYPE_BUFFER)
>>>          if (obj->buffer.length < size)
>>>              err = -ENOSPC;
>>>          if (!obj->buffer.length)
>>>              err = -ENODATA;
>>>          if (err) {
>>>              kfree(obj);
>>>              return err;
>>>          }
>>>          memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
>>>      }
>>>
>>>      if (obj && obj->type == ACPI_TYPE_INTEGER)
>>>          int_tmp = (u32) obj->integer.value;
>>>
>>>      kfree(obj);
>>>
>>>      if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
>>>          return -ENODEV;
>>>
>>>      /* There is at least one method that returns a 0 with no buffer */
>>>      if (obj == NULL || int_tmp == 0)
>>>          return -ENODATA;
>>>
>>>      return 0;
>>>
>>
>> I had something like the following in mind:
>>
>>   int err = 0;
>>   /* ... */
>>   obj = output.pointer;
>>   if (!obj)
>>     return -ENODATA;
>>
>>   switch (obj->type) {
>>   case ACPI_TYPE_BUFFER:
>>     if (obj->buffer.length < size)
>>       err = -ENODATA;
>>     else if (obj->buffer.length > size)
>>       err = -ENOSPC;
>>     else
>>       memcpy(ret_buffer, obj->buffer.pointer, size);
>>     break;
>>   case ACPI_TYPE_INTEGER:
>>     switch (obj->integer.value) {
>>       case ASUS_WMI_UNSUPPORTED_METHOD:
>>         err = -EOPNOTSUPP;
>>     break;
>>       default:
>>         err = -ENODATA;
>>     break;
>>     }
>>     break;
>>   default:
>>     err = -ENODATA;
>>     break;
>>   }
>>
>>   kfree(obj);
>>
>>   return err;
>>
> 
> Got it. Sometimes I forget switch/case exists. I'll adjust the v8 patch I sent out earlier.
> 
>>
>>>  >
>>>  >
>>>  >>  +
>>>  >>  +    kfree(obj);
>>>  >>  +
>>>  >>  +    return 0;
>>>  >>  +}
>>>  [...]
>>>  >>  +/*
>>>  >>  + * Called only by throttle_thermal_policy_write()
>>>  >>  + */
>>>  >
>>>  > Am I correct in thinking that the firmware does not actually
>>>  > support specifying fan curves for each mode, only a single one,
>>>  > and the fan curve switching is done by this driver when
>>>  > the performance mode is changed?
>>>
>>>  I'm not 100% certain on this. The WMI method 0x00110024 takes an arg
>>>  0,1,2 which then returns some factory stored fan profiles, these fit
>>>  the profiles of ASUS_THROTTLE_THERMAL_POLICY_*, but with 1 and 2
>>>  swapped.
>>>
>>>  Looking at the SET part, it seems to write to a different location than
>>>  where the GET is fetching information.
>>>
>>
>> The, unfortunately, that is not as simple as I initially thought...
> 
> We can add the fact that a variation exists with a more typical setup also. The G713Q has no throttle_thermal and in the dsdt dump looks like it possible can read back the curve that is set by the user. This works in our favour though.
> 
>>
>>
>>>  Because of the fact there are three sets of curves to get, I thought it
>>>  would be good for users to be able to set per profile. I don't think
>>>  the set is retained in acpi if the profile is switched.
>>>
>>>  Do you think it would be best to not have the ability to store per
>>>  profile in kernel?
>>
>> If there was a method to set a fan curve, and one to retrieve it,
>> I would suggest just exposing that via the pwmN_auto_pointM_{pwm,temp}
>> attributes on a hwmon device, and that the profile-dependent switching
>> be implemented somewhere else. As far as I see, there is already
>> existing infrastructure for integrating such a feature [0]
>> (but please correct me if I'm wrong).
> 
> There is. I develop asusctl in conjunction with these patches. I'd certainly like to find the best way to fit all of this together.
> 
>>
>> This would simplify the kernel code, add no new ABI, and
>> potentially provide greater control over policy for the
>> user space.
> 
> I agree.
> 
>>
>>
>>>  How would I choose which profile get to populate the
>>>  initial data with if so?
>>
>> I assume there isn't a method that can query
>> the current fan curve (or it is unknown)?
> 
> It looks like I need to adjust how pwm[n]_enable works anyway:
> 
> `pwm[1-*]_enable`
> Fan speed control method:
> - 0: no fan speed control (i.e. fan at full speed)
> - 1: manual fan speed control enabled (using `pwm[1-*]`)
> - 2+: automatic fan speed control enabled

Notice the 2+ here, AFAIK the API allows defining extra
settings / custom profiles when the value is higher then 2.

So you could do:

2: automatic fan speed control with factory default profiles
   (with all the pwm1_auto_point#... attributes ro perhaps ?)
3: automatic fan speed control with custom profile

Note you could swap 2 and 3 here, not sure which order makes
the most sense.

Guenter, what do you think about the above?

This will also nicely give power-profiles-daemon a nice way
to check if a custom profile is being used, which Bastien
requested to have.

Regards,

Hans







> 
> So maybe on "manual" I can make it so the get method does what is in fan_curve_check_present() (and change that part also) and fetches the "defaults" on enable. This might even give us the fan curve that was set (and looks like it will on the machine that has no thermal throttle profile, v8 patch) - I'll try this anyway and see what it looks like. This seems to be the best approach given how the G713Q works anyway.
> 
> The issue I have with the above is that it overwrites any curve set. But given that it'll most likely be managed in userspace that's maybe *not* an issue. Otherwise would it be sensible to add something like `pwm1_reset`? I don't see anything like that in the related docs though. `pwm1_reset` would be to re-read the defaults from the acpi method.
> 
> Central to the above is that we can still read 0, 1, 2 curves from acpi - I was thinking to use the throttle_thermal mode to choose which one and that would be the only use of it. And won't store them as per-profile, which becomes a moot point when userspace is managing it anyway.
> 
> Many thanks,
> Luke.
> 
> 
>>
>>
>>>  [...]
>>
>> [0]: https://gitlab.com/asus-linux/asusctl
>>
>>
>> Best regards,
>> Barnabás Pőcze
> 
> 

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

[Luke Jones]

[-- Attachment #1: Type: text/plain, Size: 11191 bytes --]

Hi Barnabás,

I did another refactor using hwmon_device_register_with_info() and 
HWMON_CHANNEL_INFO(). I'm unsure if this is what you were looking for 
so I'm going to attach the patch instead of submitting as a V8 for now.

My main concern as that the use of the above removes the 
pwm1_auto_point1_pwm + pwm1_auto_point1_temp format and gives two 
hwmon<num>, one per cpu/gpu fan with:

device power
fan1_input subsystem
fan2_input temp1_input
fan3_input temp2_input
fan4_input temp3_input
fan5_input temp4_input
fan6_input temp5_input
fan7_input temp6_input
fan8_input temp7_input
in0_enable temp8_input
name uevent

cat -p /sys/devices/platform/asus-nb-wmi/hwmon/hwmon7/name
asus_cpu_fan_custom_curve

I've named the root name of each as descriptive as possible to convey 
exactly what each is

Oh and `sensors` now shows:

asus_cpu_fan_curve-isa-0000
Adapter: ISA adapter
fan1: 8 RPM
fan2: 10 RPM
fan3: 18 RPM
fan4: 20 RPM
fan5: 28 RPM
fan6: 34 RPM
fan7: 44 RPM
fan8: 56 RPM
temp1: +0.0°C
temp2: +0.1°C
temp3: +0.1°C
temp4: +0.1°C
temp5: +0.1°C
temp6: +0.1°C
temp7: +0.1°C
temp8: +0.1°C


 > FYI, the pwmX_enable attributes can be created by the hwmon
 > subsystem itself if you use [devm_]hwmon_device_register_with_info()
 > with appropriately populated `struct hwmon_chip_info`.

So when you say this, did you mean *just* for the pwmX_enable 
attributes?


On Mon, Aug 30 2021 at 21:28:18 +0000, Barnabás Pőcze 
<pobrn@protonmail.com> wrote:
> Hi
> 
> 
> 2021. augusztus 30., hétfő 13:31 keltezéssel, Luke D. Jones írta:
>>  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".
>> 
>>  This patch adds two blocks of attributes to the hwmon sysfs,
>>  1 block each for CPU and GPU fans.
>> 
>>  When the user switches profiles the associated curve data for that
>>  profile is then show/store enabled to allow users to rotate through
>>  the profiles and set a fan curve for each profile which then
>>  activates on profile switch if enabled.
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 568 
>> ++++++++++++++++++++-
>>   include/linux/platform_data/x86/asus-wmi.h |   2 +
>>   2 files changed, 566 insertions(+), 4 deletions(-)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index cc5811844012..b594c2475034 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  [...]
>>  +/*
>>  + * Returns as an error if the method output is not a buffer. 
>> Typically this
> 
> It seems to me it will simply leave the output buffer uninitialized 
> if something
> other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and 
> return 0.
> 
> 
>>  + * 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;
> 
> Is anything known about the possible values? You are later
> using it as if it was an errno (e.g. in `custom_fan_check_present()`).
> 
> And `obj` is leaked in both of the previous two returns.
> 
> 
>>  +	}
>>  +
>>  +	if (obj && obj->type == ACPI_TYPE_BUFFER)
>>  +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
> 
> I would suggest you add a "size_t size" argument to this function, and
> return -ENOSPC/-ENODATA depending on whether the returned buffer is 
> too
> big/small. Maybe return -ENODATA if `obj` is NULL, too.
> 
> 
>>  +
>>  +	kfree(obj);
>>  +
>>  +	return 0;
>>  +}
>>  [...]
>>  +static ssize_t fan_curve_show(struct device *dev,
>>  +				struct device_attribute *attr, char *buf)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +	int value;
>>  +
>>  +	int index = to_sensor_dev_attr_2(attr)->index;
>>  +	int nr = to_sensor_dev_attr_2(attr)->nr;
>>  +	int pwm = nr & FAN_CURVE_PWM_MASK;
>>  +
>>  +	if (pwm)
>>  +		value = 255 * data->percents[index] / 100;
>>  +	else
>>  +		value = data->temps[index];
>>  +
>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", value);
> 
> sysfs_emit()
> 
> 
>>  +}
>>  +
>>  +/*
>>  + * "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_data *data)
>>  +{
>>  +	int ret, i, shift = 0;
>>  +	u32 arg1, arg2, arg3, arg4;
>>  +
>>  +	arg1 = arg2 = arg3 = arg4 = 0;
>>  +
>>  +	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
>>  +		arg1 += data->temps[i] << shift;
>>  +		arg2 += data->temps[i + 4] << shift;
>>  +		arg3 += data->percents[0] << shift;
>>  +		arg4 += data->percents[i + 4] << shift;
>>  +		shift += 8;
>>  +	}
>>  +
>>  +	return asus_wmi_evaluate_method5(ASUS_WMI_METHODID_DEVS, dev,
>>  +					arg1, arg2, arg3, arg4, &ret);
>>  +}
>>  +
>>  +/*
>>  + * Called only by throttle_thermal_policy_write()
>>  + */
> 
> Am I correct in thinking that the firmware does not actually
> support specifying fan curves for each mode, only a single one,
> and the fan curve switching is done by this driver when
> the performance mode is changed?
> 
> 
>>  +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_CURVE_DEV_CPU];
>>  +	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
>>  +
>>  +	if (cpu->enabled) {
>>  +		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
>>  +		if (err)
>>  +			return err;
>>  +	}
>>  +
>>  +	if (gpu->enabled) {
>>  +		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
>>  +		if (err)
>>  +			return err;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  [...]
>>  +static ssize_t fan_curve_store(struct device *dev,
>>  +				struct device_attribute *attr,
>>  +				const char *buf, size_t count)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +	u8 value, old_value;
>>  +	int err;
>>  +
>>  +	int index = to_sensor_dev_attr_2(attr)->index;
>>  +	int nr = to_sensor_dev_attr_2(attr)->nr;
>>  +	int pwm = nr & FAN_CURVE_PWM_MASK;
>>  +
>>  +	err = kstrtou8(buf, 10, &value);
>>  +	if (err < 0)
>>  +		return err;
>>  +
>>  +	if (pwm) {
>>  +		old_value = data->percents[index];
>>  +		data->percents[index] = 100 * value / 255;
>>  +	} else {
>>  +		old_value = data->temps[index];
>>  +		data->temps[index] = value;
>>  +	}
>>  +	/*
>>  +	 * The check here forces writing a curve graph in reverse,
>>  +	 * from highest to lowest.
>>  +	 */
>>  +	err = fan_curve_verify(data);
>>  +	if (err) {
>>  +		if (pwm) {
>>  +			dev_err(dev, "a fan curve percentage was higher than the next 
>> in sequence\n");
>>  +			data->percents[index] = old_value;
>>  +		} else {
>>  +			dev_err(dev, "a fan curve temperature was higher than the next 
>> in sequence\n");
>>  +			data->temps[index] = old_value;
>>  +		}
>>  +		return err;
>>  +	}
> 
> Are such sequences rejected by the firmware itself?
> Or is this just an extra layer of protection?
> 
> 
>>  +
>>  +	return count;
>>  +}
>>  +
>>  +static ssize_t fan_curve_enable_show(struct device *dev,
>>  +				struct device_attribute *attr, char *buf)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +
>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", data->enabled);
> 
> sysfs_emit()
> 
> 
>>  +}
>>  +
>>  +static ssize_t fan_curve_enable_store(struct device *dev,
>>  +				struct device_attribute *attr,
>>  +				const char *buf, size_t count)
>>  +{
>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>> attr);
>>  +	struct asus_wmi *asus = dev_get_drvdata(dev);
>>  +	bool value;
>>  +	int err;
>>  +
>>  +	err = kstrtobool(buf, &value);
>>  +	if (err < 0)
>>  +		return err;
>>  +
>>  +	data->enabled = value;
>>  +	throttle_thermal_policy_write(asus);
>>  +
>>  +	return count;
>>  +}
>>  +
>>  +/* CPU */
>>  +// TODO: enable
>>  +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable,
>>  +				FAN_CURVE_DEV_CPU);
> 
> FYI, the pwmX_enable attributes can be created by the hwmon
> subsystem itself if you use [devm_]hwmon_device_register_with_info()
> with appropriately populated `struct hwmon_chip_info`.
> 
> 
>>  [...]
>>  +static const struct attribute_group fan_curve_attribute_group = {
>>  +	.is_visible = fan_curve_sysfs_is_visible,
>>  +	.attrs = fan_curve_attributes
> 
> Small thing, but it is customary to put commas after non-terminating
> entries in initializers / enum definitions.
> 
> 
>>  +};
>>  +__ATTRIBUTE_GROUPS(fan_curve_attribute);
>>  +
>>  +static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
>>  +{
>>  +	struct device *dev = &asus->platform_device->dev;
>>  +	struct device *hwmon;
>>  +
>>  +	hwmon = devm_hwmon_device_register_with_groups(dev, "asus", asus,
>>  +						fan_curve_attribute_groups);
>>  +
>>  +	if (IS_ERR(hwmon)) {
>>  +		pr_err("Could not register asus fan_curve device\n");
> 
> I think `dev_err()` would be better.
> 
> 
>>  +		return PTR_ERR(hwmon);
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  [...]
>>  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
> 
> 
> Best regards,
> Barnabás Pőcze


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v8-0001-asus-wmi-Add-support-for-custom-fan-curves.patch --]
[-- Type: text/x-patch, Size: 16921 bytes --]

From 728573afa7eda049a57626cc37ec68733035fdef Mon Sep 17 00:00:00 2001
From: "Luke D. Jones" <luke@ljones.dev>
Date: Sun, 29 Aug 2021 13:21:23 +1200
Subject: [PATCH v8 1/1] asus-wmi: Add support for custom fan curves

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".

This patch adds two blocks of attributes to the hwmon sysfs,
1 block each for CPU and GPU fans.

When the user switches profiles the associated curve data for that
profile is then show/store enabled to allow users to rotate through
the profiles and set a fan curve for each profile which then
activates on profile switch if enabled.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index cc5811844012..f83f153d60ba 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -106,8 +106,16 @@ module_param(fnlock_default, bool, 0444);
 
 #define WMI_EVENT_MASK			0xFFFF
 
+/* The number of ASUS_THROTTLE_THERMAL_POLICY_* available */
+#define FAN_CURVE_PROFILE_NUM	(ASUS_THROTTLE_THERMAL_POLICY_SILENT + 1)
+#define FAN_CURVE_POINTS		8
+#define FAN_CURVE_DEV_CPU		0x00
+#define FAN_CURVE_DEV_GPU		0x01
+
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
+static int throttle_thermal_policy_write(struct asus_wmi *);
+
 static bool ashs_present(void)
 {
 	int i = 0;
@@ -122,7 +130,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 +182,17 @@ enum fan_type {
 	FAN_TYPE_SPEC83,	/* starting in Spec 8.3, use CPU_FAN_CTRL */
 };
 
+/*
+ * 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 {
+	u8 enabled;
+	u8 temps[FAN_CURVE_POINTS];
+	u8 percents[FAN_CURVE_POINTS];
+};
+
 struct asus_wmi {
 	int dsts_id;
 	int spec;
@@ -220,6 +240,11 @@ struct asus_wmi {
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
 
+	bool cpu_fan_curve_available;
+	bool gpu_fan_curve_available;
+	/* [throttle modes][fan count] */
+	struct fan_curve_data throttle_fan_curves[FAN_CURVE_PROFILE_NUM][2];
+
 	struct platform_profile_handler platform_profile_handler;
 	bool platform_profile_support;
 
@@ -285,6 +310,105 @@ 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, size_t size)
+{
+	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;
+	int err = 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_BUFFER) {
+		if (obj->buffer.length > size)
+			err = -ENOSPC;
+		if (obj->buffer.length == 0)
+			err = -ENODATA;
+		if (err) {
+			kfree(obj);
+			return err;
+		}
+		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
+	}
+
+	if (obj && obj->type == ACPI_TYPE_INTEGER) {
+		int_tmp = (u32)obj->integer.value;
+
+		kfree(obj);
+		if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
+			return -ENODEV;
+		/*
+		 * At least one method returns a 0 with no buffer if no arg
+		 * is provided, such as ASUS_WMI_DEVID_CPU_FAN_CURVE
+		 */
+		if (int_tmp == 0)
+			return -ENODATA;
+		return int_tmp;
+	}
+
+	kfree(obj);
+
+	if (obj == NULL)
+		return -ENODATA;
+
+	return 0;
+}
+
 static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
 {
 	struct acpi_buffer input;
@@ -2043,6 +2167,357 @@ 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 init_fan_curve(struct fan_curve_data *data, u8 *buf)
+{
+	int i;
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++)
+		data->temps[i] = buf[i];
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++)
+		data->percents[i] = buf[i + 8];
+}
+
+/*
+ * Check if the ability to set fan curves on either fan exists, and populate
+ * with system defaults 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[FAN_CURVE_POINTS * 2];
+	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,
+					FAN_CURVE_POINTS * 2);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+	init_fan_curve(curves, buf);
+
+	/*
+	 * 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,
+					FAN_CURVE_POINTS * 2);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+	init_fan_curve(curves, buf);
+
+	/* 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,
+					FAN_CURVE_POINTS * 2);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+	init_fan_curve(curves, buf);
+
+	*available = true;
+	return 0;
+}
+
+/*
+ * "dev" is the related WMI method such as ASUS_WMI_DEVID_CPU_FAN_CURVE.
+ */
+static int fan_curve_write_arg(struct asus_wmi *asus, u32 dev,
+				struct fan_curve_data *data)
+{
+	u32 arg1 = 0, arg2 = 0, arg3 = 0, arg4 = 0;
+	int ret, i, shift = 0;
+
+	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
+		arg1 += data->temps[i] << shift;
+		arg2 += data->temps[i + 4] << shift;
+		arg3 += data->percents[0] << shift;
+		arg4 += data->percents[i + 4] << shift;
+		shift += 8;
+	}
+
+	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_CURVE_DEV_CPU];
+	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
+
+	if (cpu->enabled) {
+		err = fan_curve_write_arg(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
+		if (err)
+			return err;
+	}
+
+	if (gpu->enabled) {
+		err = fan_curve_write_arg(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int fan_curve_verify(struct device *dev,
+				enum hwmon_sensor_types type, u32 fan_dev)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 mode = asus->throttle_thermal_policy_mode;
+	struct fan_curve_data *data;
+	u8 tmp = 0, prev_tmp = 0;
+	int i;
+
+	switch (type) {
+	case hwmon_temp:
+		data = &asus->throttle_fan_curves[mode][fan_dev];
+		break;
+	case hwmon_fan:
+		data = &asus->throttle_fan_curves[mode][fan_dev];
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++) {
+		tmp = data->temps[i];
+		if (tmp < prev_tmp)
+			return -EINVAL;
+		prev_tmp = tmp;
+	}
+
+	tmp = prev_tmp = 0;
+	for (i = 0; i < FAN_CURVE_POINTS; i++) {
+		tmp = data->percents[i];
+		if (tmp < prev_tmp)
+			return -EINVAL;
+		prev_tmp = tmp;
+	}
+
+	return 0;
+}
+
+/* ret is a pointer to the requested profile->fan->point */
+static int fan_curve_point_select(struct device *dev,
+		enum hwmon_sensor_types type, int point, u32 fan_dev, u8 **ret)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 mode = asus->throttle_thermal_policy_mode;
+
+	switch (type) {
+	case hwmon_temp:
+		*ret = &asus->throttle_fan_curves[mode][fan_dev].temps[point];
+		break;
+	case hwmon_fan:
+		*ret = &asus->throttle_fan_curves[mode][fan_dev].percents[point];
+		break;
+	case hwmon_in:
+		*ret = &asus->throttle_fan_curves[mode][fan_dev].enabled;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int asus_fan_curve_read(struct device *dev, enum hwmon_sensor_types type,
+					int point, long *val, u32 fan_dev)
+{
+	int err;
+	u8 *ret;
+
+	err = fan_curve_point_select(dev, type, point, fan_dev, &ret);
+	if (err)
+		return err;
+	*val = *ret;
+
+	return 0;
+}
+
+static int asus_cpu_fan_curve_read(struct device *dev,
+		enum hwmon_sensor_types type, u32 attr, int point, long *val)
+{
+	return asus_fan_curve_read(dev, type, point, val, FAN_CURVE_DEV_CPU);
+}
+
+static int asus_gpu_fan_curve_read(struct device *dev,
+		enum hwmon_sensor_types type, u32 attr, int point, long *val)
+{
+	return asus_fan_curve_read(dev, type, point, val, FAN_CURVE_DEV_GPU);
+}
+
+
+static int asus_fan_curve_write(struct device *dev,
+				enum hwmon_sensor_types type,
+				u32 attr, int point, long val, u32 fan_dev)
+{
+	u8 value, old_value, *ret;
+	int err;
+
+	err = fan_curve_point_select(dev, type, point, fan_dev, &ret);
+	if (err)
+		return err;
+
+	value = *ret;
+	old_value = value;
+	if (type == hwmon_temp)
+		value = 100 * value / 255;
+
+	/* The check here forces writing a curve graph in reverse for safety */
+	err = fan_curve_verify(dev, type, fan_dev);
+	if (err) {
+		switch (type) {
+		case hwmon_temp:
+			dev_err(dev, "a fan curve temperature was higher than the next in sequence\n");
+			*ret = old_value;
+			break;
+		case hwmon_fan:
+			dev_err(dev, "a fan curve percentage was higher than the next in sequence\n");
+			*ret = old_value;
+			break;
+		default:
+			break;
+		}
+		return err;
+	}
+
+	return 0;
+}
+
+static int asus_cpu_fan_curve_write(struct device *dev,
+		enum hwmon_sensor_types type, u32 attr, int point, long val)
+{
+	return asus_fan_curve_write(dev, type, attr, point, val, FAN_CURVE_DEV_CPU);
+}
+
+static int asus_gpu_fan_curve_write(struct device *dev,
+		enum hwmon_sensor_types type, u32 attr, int point, long val)
+{
+	return asus_fan_curve_write(dev, type, attr, point, val, FAN_CURVE_DEV_GPU);
+}
+
+static umode_t asus_cpu_fan_curve_is_visible(const void *data,
+		enum hwmon_sensor_types type, u32 attr, int point)
+{
+	const struct asus_wmi *asus = data;
+
+	if (asus->cpu_fan_curve_available)
+		return 0644;
+	return 0;
+}
+
+static umode_t asus_gpu_fan_curve_is_visible(const void *data,
+			enum hwmon_sensor_types type, u32 attr, int point)
+{
+	const struct asus_wmi *asus = data;
+
+	if (asus->gpu_fan_curve_available)
+		return 0644;
+	return 0;
+}
+
+static const struct hwmon_ops asus_cpu_fan_curve_ops = {
+	.is_visible = asus_cpu_fan_curve_is_visible,
+	.read = asus_cpu_fan_curve_read,
+	.write = asus_cpu_fan_curve_write,
+};
+
+static const struct hwmon_ops asus_gpu_fan_curve_ops = {
+	.is_visible = asus_gpu_fan_curve_is_visible,
+	.read = asus_gpu_fan_curve_read,
+	.write = asus_gpu_fan_curve_write,
+};
+
+/* Channel number determines CPU or GPU, 0-1 = CPU */
+static const struct hwmon_channel_info *asus_fan_curve_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+		HWMON_T_INPUT, HWMON_T_INPUT, HWMON_T_INPUT, HWMON_T_INPUT,
+		HWMON_T_INPUT, HWMON_T_INPUT, HWMON_T_INPUT, HWMON_T_INPUT),
+	HWMON_CHANNEL_INFO(fan,
+		HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT,
+		HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT),
+	HWMON_CHANNEL_INFO(in, HWMON_I_ENABLE),
+	NULL
+};
+
+static const struct hwmon_chip_info asus_cpu_fan_curve_chip_info = {
+	.ops = &asus_cpu_fan_curve_ops,
+	.info = asus_fan_curve_info,
+};
+
+static const struct hwmon_chip_info asus_gpu_fan_curve_chip_info = {
+	.ops = &asus_gpu_fan_curve_ops,
+	.info = asus_fan_curve_info,
+};
+
+static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
+{
+	struct device *dev = &asus->platform_device->dev;
+	struct device *hwmon;
+	int err;
+
+	err = custom_fan_check_present(asus,
+		&asus->cpu_fan_curve_available, ASUS_WMI_DEVID_CPU_FAN_CURVE);
+	if (err)
+		return err;
+
+	err = custom_fan_check_present(asus,
+		&asus->gpu_fan_curve_available, ASUS_WMI_DEVID_GPU_FAN_CURVE);
+	if (err)
+		return err;
+
+	hwmon = hwmon_device_register_with_info(dev,
+					"asus_cpu_fan_custom_curve", asus,
+					&asus_cpu_fan_curve_chip_info, NULL);
+
+	if (IS_ERR(hwmon)) {
+		dev_err(dev, "Could not register asus_cpu_fan_curve device\n");
+		return PTR_ERR(hwmon);
+	}
+
+	hwmon = hwmon_device_register_with_info(dev,
+					"asus_gpu_fan_custom_curve", asus,
+					&asus_gpu_fan_curve_chip_info, NULL);
+
+	if (IS_ERR(hwmon)) {
+		dev_err(dev, "Could not register asus_cpu_fan_curve device\n");
+		return PTR_ERR(hwmon);
+	}
+
+	return 0;
+}
+
 /* Throttle thermal policy ****************************************************/
 
 static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
@@ -2053,8 +2528,8 @@ static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
 	asus->throttle_thermal_policy_available = false;
 
 	err = asus_wmi_get_devstate(asus,
-				    ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
-				    &result);
+		ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
+		&result);
 	if (err) {
 		if (err == -ENODEV)
 			return 0;
@@ -2092,6 +2567,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;
 }
 
@@ -2904,7 +3385,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,
@@ -3038,6 +3519,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_hwmon;
 
+	err = asus_wmi_fan_curve_init(asus);
+	if (err)
+		goto fail_custom_fan_curve;
+
 	err = asus_wmi_led_init(asus);
 	if (err)
 		goto fail_leds;
@@ -3109,6 +3594,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 v7] asus-wmi: Add support for custom fan curves

[Luke Jones]

[-- Attachment #1: Type: text/plain, Size: 12054 bytes --]

Additionally to the above I've taken in to account feedback for v7 
patch, and cleaned up plus tidied a few things.

I'll again attach for quick look over before I submit next patch as 
full review. I am thinking that this version (v9 here) is the proper 
and expected outcome.

Cheers,
Luke

On Tue, Aug 31 2021 at 20:58:49 +1200, Luke Jones <luke@ljones.dev> 
wrote:
> Hi Barnabás,
> 
> I did another refactor using hwmon_device_register_with_info() and 
> HWMON_CHANNEL_INFO(). I'm unsure if this is what you were looking for 
> so I'm going to attach the patch instead of submitting as a V8 for 
> now.
> 
> My main concern as that the use of the above removes the 
> pwm1_auto_point1_pwm + pwm1_auto_point1_temp format and gives two 
> hwmon<num>, one per cpu/gpu fan with:
> 
> device power
> fan1_input subsystem
> fan2_input temp1_input
> fan3_input temp2_input
> fan4_input temp3_input
> fan5_input temp4_input
> fan6_input temp5_input
> fan7_input temp6_input
> fan8_input temp7_input
> in0_enable temp8_input
> name uevent
> 
> cat -p /sys/devices/platform/asus-nb-wmi/hwmon/hwmon7/name
> asus_cpu_fan_custom_curve
> 
> I've named the root name of each as descriptive as possible to convey 
> exactly what each is
> 
> Oh and `sensors` now shows:
> 
> asus_cpu_fan_curve-isa-0000
> Adapter: ISA adapter
> fan1: 8 RPM
> fan2: 10 RPM
> fan3: 18 RPM
> fan4: 20 RPM
> fan5: 28 RPM
> fan6: 34 RPM
> fan7: 44 RPM
> fan8: 56 RPM
> temp1: +0.0°C
> temp2: +0.1°C
> temp3: +0.1°C
> temp4: +0.1°C
> temp5: +0.1°C
> temp6: +0.1°C
> temp7: +0.1°C
> temp8: +0.1°C
> 
> 
> > FYI, the pwmX_enable attributes can be created by the hwmon
> > subsystem itself if you use [devm_]hwmon_device_register_with_info()
> > with appropriately populated `struct hwmon_chip_info`.
> 
> So when you say this, did you mean *just* for the pwmX_enable 
> attributes?
> 
> 
> On Mon, Aug 30 2021 at 21:28:18 +0000, Barnabás Pőcze 
> <pobrn@protonmail.com> wrote:
>> Hi
>> 
>> 
>> 2021. augusztus 30., hétfő 13:31 keltezéssel, Luke D. Jones írta:
>>>  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".
>>> 
>>>  This patch adds two blocks of attributes to the hwmon sysfs,
>>>  1 block each for CPU and GPU fans.
>>> 
>>>  When the user switches profiles the associated curve data for that
>>>  profile is then show/store enabled to allow users to rotate through
>>>  the profiles and set a fan curve for each profile which then
>>>  activates on profile switch if enabled.
>>> 
>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>  ---
>>>   drivers/platform/x86/asus-wmi.c            | 568 
>>> \x7f\x7f++++++++++++++++++++-
>>>   include/linux/platform_data/x86/asus-wmi.h |   2 +
>>>   2 files changed, 566 insertions(+), 4 deletions(-)
>>> 
>>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>>> \x7f\x7fb/drivers/platform/x86/asus-wmi.c
>>>  index cc5811844012..b594c2475034 100644
>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>  [...]
>>>  +/*
>>>  + * Returns as an error if the method output is not a buffer. 
>>> \x7f\x7fTypically this
>> 
>> It seems to me it will simply leave the output buffer uninitialized 
>> \x7fif something
>> other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and 
>> \x7freturn 0.
>> 
>> 
>>>  + * 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;
>> 
>> Is anything known about the possible values? You are later
>> using it as if it was an errno (e.g. in 
>> `custom_fan_check_present()`).
>> 
>> And `obj` is leaked in both of the previous two returns.
>> 
>> 
>>>  +	}
>>>  +
>>>  +	if (obj && obj->type == ACPI_TYPE_BUFFER)
>>>  +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
>> 
>> I would suggest you add a "size_t size" argument to this function, 
>> and
>> return -ENOSPC/-ENODATA depending on whether the returned buffer is 
>> \x7ftoo
>> big/small. Maybe return -ENODATA if `obj` is NULL, too.
>> 
>> 
>>>  +
>>>  +	kfree(obj);
>>>  +
>>>  +	return 0;
>>>  +}
>>>  [...]
>>>  +static ssize_t fan_curve_show(struct device *dev,
>>>  +				struct device_attribute *attr, char *buf)
>>>  +{
>>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>>> \x7f\x7fattr);
>>>  +	int value;
>>>  +
>>>  +	int index = to_sensor_dev_attr_2(attr)->index;
>>>  +	int nr = to_sensor_dev_attr_2(attr)->nr;
>>>  +	int pwm = nr & FAN_CURVE_PWM_MASK;
>>>  +
>>>  +	if (pwm)
>>>  +		value = 255 * data->percents[index] / 100;
>>>  +	else
>>>  +		value = data->temps[index];
>>>  +
>>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", value);
>> 
>> sysfs_emit()
>> 
>> 
>>>  +}
>>>  +
>>>  +/*
>>>  + * "dev" is the related WMI method such as 
>>> \x7f\x7fASUS_WMI_DEVID_CPU_FAN_CURVE.
>>>  + */
>>>  +static int fan_curve_write(struct asus_wmi *asus, u32 dev,
>>>  +					struct fan_curve_data *data)
>>>  +{
>>>  +	int ret, i, shift = 0;
>>>  +	u32 arg1, arg2, arg3, arg4;
>>>  +
>>>  +	arg1 = arg2 = arg3 = arg4 = 0;
>>>  +
>>>  +	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
>>>  +		arg1 += data->temps[i] << shift;
>>>  +		arg2 += data->temps[i + 4] << shift;
>>>  +		arg3 += data->percents[0] << shift;
>>>  +		arg4 += data->percents[i + 4] << shift;
>>>  +		shift += 8;
>>>  +	}
>>>  +
>>>  +	return asus_wmi_evaluate_method5(ASUS_WMI_METHODID_DEVS, dev,
>>>  +					arg1, arg2, arg3, arg4, &ret);
>>>  +}
>>>  +
>>>  +/*
>>>  + * Called only by throttle_thermal_policy_write()
>>>  + */
>> 
>> Am I correct in thinking that the firmware does not actually
>> support specifying fan curves for each mode, only a single one,
>> and the fan curve switching is done by this driver when
>> the performance mode is changed?
>> 
>> 
>>>  +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_CURVE_DEV_CPU];
>>>  +	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
>>>  +
>>>  +	if (cpu->enabled) {
>>>  +		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
>>>  +		if (err)
>>>  +			return err;
>>>  +	}
>>>  +
>>>  +	if (gpu->enabled) {
>>>  +		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
>>>  +		if (err)
>>>  +			return err;
>>>  +	}
>>>  +
>>>  +	return 0;
>>>  +}
>>>  [...]
>>>  +static ssize_t fan_curve_store(struct device *dev,
>>>  +				struct device_attribute *attr,
>>>  +				const char *buf, size_t count)
>>>  +{
>>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>>> \x7f\x7fattr);
>>>  +	u8 value, old_value;
>>>  +	int err;
>>>  +
>>>  +	int index = to_sensor_dev_attr_2(attr)->index;
>>>  +	int nr = to_sensor_dev_attr_2(attr)->nr;
>>>  +	int pwm = nr & FAN_CURVE_PWM_MASK;
>>>  +
>>>  +	err = kstrtou8(buf, 10, &value);
>>>  +	if (err < 0)
>>>  +		return err;
>>>  +
>>>  +	if (pwm) {
>>>  +		old_value = data->percents[index];
>>>  +		data->percents[index] = 100 * value / 255;
>>>  +	} else {
>>>  +		old_value = data->temps[index];
>>>  +		data->temps[index] = value;
>>>  +	}
>>>  +	/*
>>>  +	 * The check here forces writing a curve graph in reverse,
>>>  +	 * from highest to lowest.
>>>  +	 */
>>>  +	err = fan_curve_verify(data);
>>>  +	if (err) {
>>>  +		if (pwm) {
>>>  +			dev_err(dev, "a fan curve percentage was higher than the next 
>>> \x7f\x7fin sequence\n");
>>>  +			data->percents[index] = old_value;
>>>  +		} else {
>>>  +			dev_err(dev, "a fan curve temperature was higher than the next 
>>> \x7f\x7fin sequence\n");
>>>  +			data->temps[index] = old_value;
>>>  +		}
>>>  +		return err;
>>>  +	}
>> 
>> Are such sequences rejected by the firmware itself?
>> Or is this just an extra layer of protection?
>> 
>> 
>>>  +
>>>  +	return count;
>>>  +}
>>>  +
>>>  +static ssize_t fan_curve_enable_show(struct device *dev,
>>>  +				struct device_attribute *attr, char *buf)
>>>  +{
>>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>>> \x7f\x7fattr);
>>>  +
>>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", data->enabled);
>> 
>> sysfs_emit()
>> 
>> 
>>>  +}
>>>  +
>>>  +static ssize_t fan_curve_enable_store(struct device *dev,
>>>  +				struct device_attribute *attr,
>>>  +				const char *buf, size_t count)
>>>  +{
>>>  +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, 
>>> \x7f\x7fattr);
>>>  +	struct asus_wmi *asus = dev_get_drvdata(dev);
>>>  +	bool value;
>>>  +	int err;
>>>  +
>>>  +	err = kstrtobool(buf, &value);
>>>  +	if (err < 0)
>>>  +		return err;
>>>  +
>>>  +	data->enabled = value;
>>>  +	throttle_thermal_policy_write(asus);
>>>  +
>>>  +	return count;
>>>  +}
>>>  +
>>>  +/* CPU */
>>>  +// TODO: enable
>>>  +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable,
>>>  +				FAN_CURVE_DEV_CPU);
>> 
>> FYI, the pwmX_enable attributes can be created by the hwmon
>> subsystem itself if you use [devm_]hwmon_device_register_with_info()
>> with appropriately populated `struct hwmon_chip_info`.
>> 
>> 
>>>  [...]
>>>  +static const struct attribute_group fan_curve_attribute_group = {
>>>  +	.is_visible = fan_curve_sysfs_is_visible,
>>>  +	.attrs = fan_curve_attributes
>> 
>> Small thing, but it is customary to put commas after non-terminating
>> entries in initializers / enum definitions.
>> 
>> 
>>>  +};
>>>  +__ATTRIBUTE_GROUPS(fan_curve_attribute);
>>>  +
>>>  +static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
>>>  +{
>>>  +	struct device *dev = &asus->platform_device->dev;
>>>  +	struct device *hwmon;
>>>  +
>>>  +	hwmon = devm_hwmon_device_register_with_groups(dev, "asus", asus,
>>>  +						fan_curve_attribute_groups);
>>>  +
>>>  +	if (IS_ERR(hwmon)) {
>>>  +		pr_err("Could not register asus fan_curve device\n");
>> 
>> I think `dev_err()` would be better.
>> 
>> 
>>>  +		return PTR_ERR(hwmon);
>>>  +	}
>>>  +
>>>  +	return 0;
>>>  +}
>>>  [...]
>>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>>> \x7f\x7fb/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
>> 
>> 
>> Best regards,
>> Barnabás Pőcze
> 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v9-0001-asus-wmi-Add-support-for-custom-fan-curves.patch --]
[-- Type: text/x-patch, Size: 22724 bytes --]

From 05f7f9c4990e097b8a71a2a39a36aa1d5d39742a Mon Sep 17 00:00:00 2001
From: "Luke D. Jones" <luke@ljones.dev>
Date: Sun, 29 Aug 2021 13:21:23 +1200
Subject: [PATCH v9] asus-wmi: Add support for custom fan curves

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".

This patch adds two blocks of attributes to the hwmon sysfs,
1 block each for CPU and GPU fans.

When the user switches profiles the associated curve data for that
profile is then show/store enabled to allow users to rotate through
the profiles and set a fan curve for each profile which then
activates on profile switch if enabled.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index cc5811844012..b594c2475034 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -106,8 +106,18 @@ module_param(fnlock_default, bool, 0444);
 
 #define WMI_EVENT_MASK			0xFFFF
 
+/* The number of ASUS_THROTTLE_THERMAL_POLICY_* available */
+#define FAN_CURVE_PROFILE_NUM	(ASUS_THROTTLE_THERMAL_POLICY_SILENT + 1)
+#define FAN_CURVE_POINTS		8
+#define FAN_CURVE_DEV_CPU		0x00
+#define FAN_CURVE_DEV_GPU		0x01
+/* Mask to determine if setting temperature or percentage */
+#define FAN_CURVE_PWM_MASK		0x04
+
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
+static int throttle_thermal_policy_write(struct asus_wmi *);
+
 static bool ashs_present(void)
 {
 	int i = 0;
@@ -122,7 +132,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 +184,17 @@ enum fan_type {
 	FAN_TYPE_SPEC83,	/* starting in Spec 8.3, use CPU_FAN_CTRL */
 };
 
+/*
+ * 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 {
+	bool enabled;
+	u8 temps[FAN_CURVE_POINTS];
+	u8 percents[FAN_CURVE_POINTS];
+};
+
 struct asus_wmi {
 	int dsts_id;
 	int spec;
@@ -220,6 +242,11 @@ struct asus_wmi {
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
 
+	bool cpu_fan_curve_available;
+	bool gpu_fan_curve_available;
+	/* [throttle modes][fan count] */
+	struct fan_curve_data throttle_fan_curves[FAN_CURVE_PROFILE_NUM][2];
+
 	struct platform_profile_handler platform_profile_handler;
 	bool platform_profile_support;
 
@@ -285,6 +312,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 +2148,440 @@ 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 init_fan_curve(struct fan_curve_data *data, u8 *buf)
+{
+	int i;
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++)
+		data->temps[i] = buf[i];
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++)
+		data->percents[i] = buf[i + 8];
+}
+
+/*
+ * Check if the ability to set fan curves on either fan exists, and populate
+ * with system defaults 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[FAN_CURVE_POINTS * 2];
+	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);
+
+	/*
+	 * 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);
+
+	/* 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);
+
+	*available = true;
+	return 0;
+}
+
+static struct fan_curve_data *fan_curve_attr_data_select(struct device *dev,
+				struct device_attribute *attr)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 mode = asus->throttle_thermal_policy_mode;
+
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	int fan = nr & FAN_CURVE_DEV_GPU;
+
+	return &asus->throttle_fan_curves[mode][fan];
+}
+
+static ssize_t fan_curve_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+	int value;
+
+	int index = to_sensor_dev_attr_2(attr)->index;
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	int pwm = nr & FAN_CURVE_PWM_MASK;
+
+	if (pwm)
+		value = 255 * data->percents[index] / 100;
+	else
+		value = data->temps[index];
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+/*
+ * "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_data *data)
+{
+	int ret, i, shift = 0;
+	u32 arg1, arg2, arg3, arg4;
+
+	arg1 = arg2 = arg3 = arg4 = 0;
+
+	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
+		arg1 += data->temps[i] << shift;
+		arg2 += data->temps[i + 4] << shift;
+		arg3 += data->percents[0] << shift;
+		arg4 += data->percents[i + 4] << shift;
+		shift += 8;
+	}
+
+	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_CURVE_DEV_CPU];
+	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
+
+	if (cpu->enabled) {
+		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
+		if (err)
+			return err;
+	}
+
+	if (gpu->enabled) {
+		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int fan_curve_verify(struct fan_curve_data *data)
+{
+	int i = 0;
+	u8 tmp = 0;
+	u8 prev_tmp = 0;
+
+
+	for (i = 0; i < FAN_CURVE_POINTS; i++) {
+		tmp = data->temps[i];
+		if (tmp < prev_tmp)
+			return -EINVAL;
+		prev_tmp = tmp;
+	}
+
+	tmp = 0;
+	prev_tmp = 0;
+	for (i = 0; i < FAN_CURVE_POINTS; i++) {
+		tmp = data->percents[i];
+		if (tmp < prev_tmp)
+			return -EINVAL;
+		prev_tmp = tmp;
+	}
+
+	return 0;
+}
+
+static ssize_t fan_curve_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+	u8 value, old_value;
+	int err;
+
+	int index = to_sensor_dev_attr_2(attr)->index;
+	int nr = to_sensor_dev_attr_2(attr)->nr;
+	int pwm = nr & FAN_CURVE_PWM_MASK;
+
+	err = kstrtou8(buf, 10, &value);
+	if (err < 0)
+		return err;
+
+	if (pwm) {
+		old_value = data->percents[index];
+		data->percents[index] = 100 * value / 255;
+	} else {
+		old_value = data->temps[index];
+		data->temps[index] = value;
+	}
+	/*
+	 * The check here forces writing a curve graph in reverse,
+	 * from highest to lowest.
+	 */
+	err = fan_curve_verify(data);
+	if (err) {
+		if (pwm) {
+			dev_err(dev, "a fan curve percentage was higher than the next in sequence\n");
+			data->percents[index] = old_value;
+		} else {
+			dev_err(dev, "a fan curve temperature was higher than the next in sequence\n");
+			data->temps[index] = old_value;
+		}
+		return err;
+	}
+
+	return count;
+}
+
+static ssize_t fan_curve_enable_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", data->enabled);
+}
+
+static ssize_t fan_curve_enable_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	bool value;
+	int err;
+
+	err = kstrtobool(buf, &value);
+	if (err < 0)
+		return err;
+
+	data->enabled = value;
+	throttle_thermal_policy_write(asus);
+
+	return count;
+}
+
+/* CPU */
+// TODO: enable
+static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable,
+				FAN_CURVE_DEV_CPU);
+// (name, function, fan, point) TODO: need to mask if temp or percent
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_temp, fan_curve,
+				FAN_CURVE_DEV_CPU, 7);
+
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point1_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point2_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point3_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point4_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point5_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point6_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point7_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm1_auto_point8_pwm, fan_curve,
+			FAN_CURVE_DEV_CPU | FAN_CURVE_PWM_MASK, 7);
+
+/* GPU */
+static SENSOR_DEVICE_ATTR_RW(pwm2_enable, fan_curve_enable,
+				FAN_CURVE_DEV_GPU);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point1_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point2_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point3_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point4_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point5_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point6_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point7_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point8_temp, fan_curve,
+				FAN_CURVE_DEV_GPU, 7);
+
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point1_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point2_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point3_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point4_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point5_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point6_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point7_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point8_pwm, fan_curve,
+			FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 7);
+
+static struct attribute *fan_curve_attributes[] = {
+	/* CPU */
+	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point7_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point8_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point6_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point7_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point8_pwm.dev_attr.attr,
+	/* GPU */
+	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point3_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point4_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point5_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point6_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point7_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point8_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point3_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point4_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point5_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point6_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point7_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point8_pwm.dev_attr.attr,
+	NULL
+};
+
+static umode_t fan_curve_sysfs_is_visible(struct kobject *kobj,
+					   struct attribute *attr, int idx)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct asus_wmi *asus = dev_get_drvdata(dev->parent);
+
+	if (attr == &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point7_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point8_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point6_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point7_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm1_auto_point8_pwm.dev_attr.attr) {
+		if (!asus->cpu_fan_curve_available)
+			return 0;
+	}
+
+	if (attr == &sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point3_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point4_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point5_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point6_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point7_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point8_temp.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point3_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point4_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point5_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point6_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point7_pwm.dev_attr.attr
+	|| attr == &sensor_dev_attr_pwm2_auto_point8_pwm.dev_attr.attr) {
+		if (!asus->gpu_fan_curve_available)
+			return 0;
+	}
+
+	return attr->mode;
+}
+
+static const struct attribute_group fan_curve_attribute_group = {
+	.is_visible = fan_curve_sysfs_is_visible,
+	.attrs = fan_curve_attributes
+};
+__ATTRIBUTE_GROUPS(fan_curve_attribute);
+
+static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
+{
+	struct device *dev = &asus->platform_device->dev;
+	struct device *hwmon;
+
+	hwmon = devm_hwmon_device_register_with_groups(dev, "asus", asus,
+						fan_curve_attribute_groups);
+
+	if (IS_ERR(hwmon)) {
+		pr_err("Could not register asus fan_curve device\n");
+		return PTR_ERR(hwmon);
+	}
+
+	return 0;
+}
+
 /* Throttle thermal policy ****************************************************/
 
 static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
@@ -2053,8 +2592,8 @@ static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
 	asus->throttle_thermal_policy_available = false;
 
 	err = asus_wmi_get_devstate(asus,
-				    ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
-				    &result);
+		ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
+		&result);
 	if (err) {
 		if (err == -ENODEV)
 			return 0;
@@ -2092,6 +2631,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;
 }
 
@@ -2904,7 +3449,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 +3561,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;
@@ -3038,6 +3593,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_hwmon;
 
+	err = asus_wmi_fan_curve_init(asus);
+	if (err)
+		goto fail_custom_fan_curve;
+
 	err = asus_wmi_led_init(asus);
 	if (err)
 		goto fail_leds;
@@ -3109,6 +3668,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 v7] asus-wmi: Add support for custom fan curves

[Barnabás Pőcze]

> Hi Barnabás,
>
> I did another refactor using hwmon_device_register_with_info() and
> HWMON_CHANNEL_INFO(). I'm unsure if this is what you were looking for
> so I'm going to attach the patch instead of submitting as a V8 for now.
>
> My main concern as that the use of the above removes the
> pwm1_auto_point1_pwm + pwm1_auto_point1_temp format and gives two
> hwmon<num>, one per cpu/gpu fan with:
>
> device power
> fan1_input subsystem
> fan2_input temp1_input
> fan3_input temp2_input
> fan4_input temp3_input
> fan5_input temp4_input
> fan6_input temp5_input
> fan7_input temp6_input
> fan8_input temp7_input
> in0_enable temp8_input
> name uevent
>
> cat -p /sys/devices/platform/asus-nb-wmi/hwmon/hwmon7/name
> asus_cpu_fan_custom_curve
>
> I've named the root name of each as descriptive as possible to convey
> exactly what each is
>
> Oh and `sensors` now shows:
>
> asus_cpu_fan_curve-isa-0000
> Adapter: ISA adapter
> fan1: 8 RPM
> fan2: 10 RPM
> fan3: 18 RPM
> fan4: 20 RPM
> fan5: 28 RPM
> fan6: 34 RPM
> fan7: 44 RPM
> fan8: 56 RPM
> temp1: +0.0°C
> temp2: +0.1°C
> temp3: +0.1°C
> temp4: +0.1°C
> temp5: +0.1°C
> temp6: +0.1°C
> temp7: +0.1°C
> temp8: +0.1°C
>
>
>  > FYI, the pwmX_enable attributes can be created by the hwmon
>  > subsystem itself if you use [devm_]hwmon_device_register_with_info()
>  > with appropriately populated `struct hwmon_chip_info`.
>
> So when you say this, did you mean *just* for the pwmX_enable
> attributes?
> [...]

Yes, that's right.


Best regards,
Barnabás Pőcze