Message-ID: <5f138c01-b644-280b-1d36-94dabe48657d@redhat.com>
Date:   Mon, 11 Oct 2021 17:28:00 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Luke D. Jones" <luke@...nes.dev>, linux-kernel@...r.kernel.org
Cc:     pobrn@...tonmail.com, linux@...ck-us.net,
        platform-driver-x86@...r.kernel.org, hadess@...ess.net
Subject: Re: [PATCH v15] asus-wmi: Add support for custom fan curves

Hi Luke,

On 10/2/21 8:21 AM, Luke D. Jones wrote:
> 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 two ACPI methods.
> 
> This patch adds two pwm<N> attributes to the hwmon sysfs,
> pwm1 for CPU fan, pwm2 for GPU fan. Both are under the hwmon of the
> name `asus_custom_fan_curve`. There is no safety check of the set
> fan curves - this must be done in userspace.
> 
> The fans have settings [1,2,3] under pwm<N>_enable:
> 1. Enable and write settings out
> 2. Disable and use factory fan mode
> 3. Same as 2, additionally restoring default factory curve.
> 
> Use of 2 means that the curve the user has set is still stored and
> won't be erased, but the laptop will be using its default auto-fan
> mode. Re-enabling the manual mode then activates the curves again.
> 
> Notes:
> - pwm<N>_enable = 0 is an invalid setting.
> - pwm is actually a percentage and is scaled on writing to device.
> 
> Signed-off-by: Luke D. Jones <luke@...nes.dev>

Ok, so I've finally been able to make some time to review this
large patch. I've a couple remarks inline below, otherwise this looks
good.

> ---
>  drivers/platform/x86/asus-wmi.c            | 608 ++++++++++++++++++++-
>  include/linux/platform_data/x86/asus-wmi.h |   2 +
>  2 files changed, 602 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index e14fb5fa7324..8b0ed1969d86 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -106,8 +106,17 @@ module_param(fnlock_default, bool, 0444);
>  
>  #define WMI_EVENT_MASK			0xFFFF
>  
> +#define FAN_CURVE_POINTS		8
> +#define FAN_CURVE_BUF_LEN		(FAN_CURVE_POINTS * 2)
> +#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 +131,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 +183,12 @@ enum fan_type {
>  	FAN_TYPE_SPEC83,	/* starting in Spec 8.3, use CPU_FAN_CTRL */
>  };
>  
> +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 +236,10 @@ struct asus_wmi {
>  	bool throttle_thermal_policy_available;
>  	u8 throttle_thermal_policy_mode;
>  
> +	bool cpu_fan_curve_available;
> +	bool gpu_fan_curve_available;
> +	struct fan_curve_data custom_fan_curves[2];
> +
>  	struct platform_profile_handler platform_profile_handler;
>  	bool platform_profile_support;
>  
> @@ -285,6 +305,103 @@ 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;
> +	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;
> +
> +	switch (obj->type) {
> +	case ACPI_TYPE_BUFFER:
> +		if (obj->buffer.length > size)
> +			err = -ENOSPC;
> +		if (obj->buffer.length == 0)
> +			err = -ENODATA;
> +
> +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
> +		break;
> +	case ACPI_TYPE_INTEGER:
> +		err = (u32)obj->integer.value;
> +
> +		if (err == ASUS_WMI_UNSUPPORTED_METHOD)
> +			err = -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 (err == 0)
> +			err = -ENODATA;
> +		break;
> +	default:
> +		err = -ENODATA;
> +		break;
> +	}
> +
> +	kfree(obj);
> +
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>  static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
>  {
>  	struct acpi_buffer input;
> @@ -1806,6 +1923,13 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	}
>  
>  	asus->fan_pwm_mode = state;
> +
> +	/* Must set to disabled if mode is toggled */
> +	if (asus->cpu_fan_curve_available)
> +		asus->custom_fan_curves[FAN_CURVE_DEV_CPU].enabled = false;
> +	if (asus->gpu_fan_curve_available)
> +		asus->custom_fan_curves[FAN_CURVE_DEV_GPU].enabled = false;
> +
>  	return count;
>  }
>  
> @@ -1953,9 +2077,9 @@ static int fan_boost_mode_check_present(struct asus_wmi *asus)
>  
>  static int fan_boost_mode_write(struct asus_wmi *asus)
>  {
> -	int err;
> -	u8 value;
>  	u32 retval;
> +	u8 value;
> +	int err;
>  
>  	value = asus->fan_boost_mode;
>  
> @@ -2013,10 +2137,10 @@ static ssize_t fan_boost_mode_store(struct device *dev,
>  				    struct device_attribute *attr,
>  				    const char *buf, size_t count)
>  {
> -	int result;
> -	u8 new_mode;
>  	struct asus_wmi *asus = dev_get_drvdata(dev);
>  	u8 mask = asus->fan_boost_mode_mask;
> +	u8 new_mode;
> +	int result;
>  
>  	result = kstrtou8(buf, 10, &new_mode);
>  	if (result < 0) {
> @@ -2043,6 +2167,462 @@ 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 *********************************************************/
> +
> +static void fan_curve_copy_from_buf(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] =
> +			255 * buf[i + FAN_CURVE_POINTS] / 100;
> +	}
> +}
> +
> +static int fan_curve_get_factory_default(struct asus_wmi *asus, u32 fan_dev)
> +{
> +	struct fan_curve_data *curves;
> +	u8 buf[FAN_CURVE_BUF_LEN];
> +	int fan_idx = 0;
> +	u8 mode = 0;
> +	int err;
> +
> +	if (asus->throttle_thermal_policy_available)
> +		mode = asus->throttle_thermal_policy_mode;
> +	/* DEVID_<C/G>PU_FAN_CURVE is switched for OVERBOOST vs SILENT */
> +	if (mode == 2)
> +		mode = 1;
> +	else if (mode == 1)
> +		mode = 2;
> +
> +	if (fan_dev == ASUS_WMI_DEVID_GPU_FAN_CURVE)
> +		fan_idx = FAN_CURVE_DEV_GPU;
> +
> +	curves = &asus->custom_fan_curves[fan_idx];
> +	err = asus_wmi_evaluate_method_buf(asus->dsts_id, fan_dev, mode, buf,
> +					   FAN_CURVE_BUF_LEN);
> +	if (err)
> +		return err;
> +
> +	fan_curve_copy_from_buf(curves, buf);
> +
> +	return 0;
> +}
> +
> +/* Check if capability exists, and populate defaults */
> +static int fan_curve_check_present(struct asus_wmi *asus, bool *available,
> +				   u32 fan_dev)
> +{
> +	int err;
> +
> +	*available = false;
> +
> +	err = fan_curve_get_factory_default(asus, fan_dev);
> +	if (err) {
> +		if (err == -ENODEV)
> +			return 0;
> +		return err;
> +	}
> +
> +	*available = true;
> +	return 0;
> +}
> +
> +/* Determine which fan the attribute is for if SENSOR_ATTR */
> +static struct fan_curve_data *fan_curve_attr_select(struct asus_wmi *asus,
> +					      struct device_attribute *attr)
> +{
> +	int index = to_sensor_dev_attr(attr)->index;
> +
> +	return &asus->custom_fan_curves[index & FAN_CURVE_DEV_GPU];
> +}
> +
> +/* Determine which fan the attribute is for if SENSOR_ATTR_2 */
> +static struct fan_curve_data *fan_curve_attr_2_select(struct asus_wmi *asus,
> +					    struct device_attribute *attr)
> +{
> +	int nr = to_sensor_dev_attr_2(attr)->nr;
> +
> +	return &asus->custom_fan_curves[nr & FAN_CURVE_DEV_GPU];
> +}
> +
> +static ssize_t fan_curve_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute_2 *dev_attr = to_sensor_dev_attr_2(attr);
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	struct fan_curve_data *data;
> +	int value, index, nr;
> +
> +	data = fan_curve_attr_2_select(asus, attr);
> +	index = dev_attr->index;
> +	nr = dev_attr->nr;
> +
> +	if (nr & FAN_CURVE_PWM_MASK)
> +		value = data->percents[index];
> +	else
> +		value = data->temps[index];
> +
> +	return sysfs_emit(buf, "%d\n", value);
> +}
> +
> +/*
> + * "fan_dev" is the related WMI method such as ASUS_WMI_DEVID_CPU_FAN_CURVE.
> + */
> +static int fan_curve_write(struct asus_wmi *asus,
> +			   struct device_attribute *attr, u32 fan_dev)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_2_select(asus, attr);
> +	u32 arg1 = 0, arg2 = 0, arg3 = 0, arg4 = 0;
> +	u8 *percents = data->percents;
> +	u8 *temps = data->temps;
> +	int ret, i, shift = 0;
> +
> +	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
> +		arg1 += (temps[i]) << shift;
> +		arg2 += (temps[i + 4]) << shift;
> +		/* Scale to percentage for device */
> +		arg3 += (100 * percents[i] / 255) << shift;
> +		arg4 += (100 * percents[i + 4] / 255) << shift;
> +		shift += 8;
> +	}
> +
> +	return asus_wmi_evaluate_method5(ASUS_WMI_METHODID_DEVS, fan_dev, arg1,
> +					 arg2, arg3, arg4, &ret);
> +}
> +
> +/*
> + * Called on curve enable/disable. This should be the only way to write out the
> + * fan curves. This avoids potential lockups on write to ACPI for every change.
> + */
> +static int fan_curve_write_data(struct asus_wmi *asus,
> +				struct device_attribute *attr)
> +{
> +	int err;
> +
> +	if (asus->cpu_fan_curve_available) {
> +		err = fan_curve_write(asus, attr, ASUS_WMI_DEVID_CPU_FAN_CURVE);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (asus->gpu_fan_curve_available) {
> +		err = fan_curve_write(asus, attr, ASUS_WMI_DEVID_GPU_FAN_CURVE);
> +		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 sensor_device_attribute_2 *dev_attr = to_sensor_dev_attr_2(attr);
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	struct fan_curve_data *data;
> +	u8 value;
> +	int err;
> +
> +	int pwm = dev_attr->nr & FAN_CURVE_PWM_MASK;
> +	int index = dev_attr->index;
> +
> +	data = fan_curve_attr_2_select(asus, attr);
> +
> +	err = kstrtou8(buf, 10, &value);
> +	if (err < 0)
> +		return err;
> +
> +	if (pwm) {
> +		data->percents[index] = value;
> +	} else {
> +		data->temps[index] = value;
> +	}
> +
> +	/*
> +	 * Mark as disabled so the user has to explicitly enable to apply a
> +	 * changed fan curve. This prevents potential lockups from writing out
> +	 * many changes as one-write-per-change.
> +	 */
> +	data->enabled = false;
> +
> +	return count;
> +}
> +
> +static ssize_t fan_curve_enable_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	struct fan_curve_data *data;
> +	int out = 2;
> +
> +	data = fan_curve_attr_select(asus, attr);
> +
> +	if (data->enabled)
> +		out = 1;
> +
> +	return sysfs_emit(buf, "%d\n", out);
> +}
> +
> +static int fan_curve_set_default(struct asus_wmi *asus)
> +{
> +	int err;
> +
> +	err = fan_curve_get_factory_default(
> +		asus, ASUS_WMI_DEVID_CPU_FAN_CURVE);
> +	if (err)
> +		return err;
> +
> +	err = fan_curve_get_factory_default(
> +		asus, ASUS_WMI_DEVID_GPU_FAN_CURVE);
> +	if (err)
> +		return err;
> +	return 0;
> +}
> +
> +static ssize_t fan_curve_enable_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	struct fan_curve_data *data;
> +	int value, err;
> +
> +	data = fan_curve_attr_select(asus, attr);
> +
> +	err = kstrtoint(buf, 10, &value);
> +	if (err < 0)
> +		return err;
> +
> +	switch (value) {
> +	case 1:
> +		data->enabled = true;
> +		break;
> +	case 2:
> +		data->enabled = false;
> +		break;
> +	/*
> +	 * Auto + reset the fan curve data to defaults. Make it an explicit
> +	 * option so that users don't accidentally overwrite a set fan curve.
> +	 */
> +	case 3:
> +		err = fan_curve_set_default(asus);
> +		if (err)
> +			return err;
> +		data->enabled = false;
> +		break;
> +	default:
> +		return -EINVAL;
> +	};
> +
> +	/*
> +	 * For machines with throttle this is the only way to reset fans to
> +	 * default mode of operation (does not erase curve data).
> +	 */
> +	if (asus->throttle_thermal_policy_available && !data->enabled) {
> +		err = throttle_thermal_policy_write(asus);
> +		if (err)
> +			return err;
> +	}
> +	/* Similar is true for laptops with this fan */
> +	if (asus->fan_type == FAN_TYPE_SPEC83) {

This if seems to miss a "&& !data->enabled" in its condition ?

Also assuming I have this right it might be better to refactor all
this to:

```
	if (data->enabled) {
		err = fan_curve_write_data(asus, attr);
	} else {
		if (asus->throttle_thermal_policy_available) {
			err = throttle_thermal_policy_write(asus);
		} else if (asus->fan_type == FAN_TYPE_SPEC83) {
			err = asus_fan_set_auto(asus);
		} else {
			/*
			 * Machines without either need to write their defaults back always.
			 * This is more of a safeguard against ASUS faulty ACPI tables.
			 */
			err = fan_curve_set_default(asus);
			if (err)
				return err;
			err = fan_curve_write_data(asus, attr);
		}
	}

	if (err)
		return err;
```

At least to me it seems that this more clearly expresses what you are trying
to achieve?


> +		err = asus_fan_set_auto(asus);
> +		if (err)
> +			return err;
> +	}
> +	/*
> +	 * Machines without either need to write their defaults back always.
> +	 * This is more of a safeguard against ASUS faulty ACPI tables.
> +	 */
> +	if (!asus->throttle_thermal_policy_available
> +	    && asus->fan_type != FAN_TYPE_SPEC83 && !data->enabled) {
> +		err = fan_curve_set_default(asus);
> +		if (err)
> +			return err;
> +		err = fan_curve_write_data(asus, attr);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (data->enabled) {
> +		err = fan_curve_write_data(asus, attr);
> +		if (err)
> +			return err;
> +	}
> +
> +	return count;
> +}
> +
> +/* CPU */
> +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable, FAN_CURVE_DEV_CPU);
> +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 *asus_fan_curve_attr[] = {
> +	/* 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 asus_fan_curve_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 (asus->cpu_fan_curve_available)
> +		return 0644;
> +
> +	if (asus->gpu_fan_curve_available)
> +		return 0644;

This seems wrong this essentially does:

	if (asus->cpu_fan_curve_available || asus->gpu_fan_curve_available)
		return 0644;

So if either is available all attributes for both curves will
always show. Shouldn't this check which curve the attribute belongs to
and then decide which _available value to use based on that ?

Since all attributes have their name start with either "pwm1" or "pwm2"
I believe that you can check on that here.

A probably better solution would be to just always use
SENSOR_DEVICE_ATTR_2_RW even for the pwmX_enabled, then you can
just cast the attr here and check nr. and as an added bonus
you can drop the fan_curve_attr_select() helper function.


> +	return 0;
> +}
> +
> +static const struct attribute_group asus_fan_curve_attr_group = {
> +	.is_visible = asus_fan_curve_is_visible,
> +	.attrs = asus_fan_curve_attr,
> +};
> +__ATTRIBUTE_GROUPS(asus_fan_curve_attr);
> +
> +/*
> + * Must be initialised after throttle_thermal_policy_check_present() as
> + * we check the status of throttle_thermal_policy_available during init.
> + */
> +static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
> +{
> +	struct device *dev = &asus->platform_device->dev;
> +	struct device *hwmon;
> +	int err;
> +
> +	err = fan_curve_check_present(asus, &asus->cpu_fan_curve_available,
> +				      ASUS_WMI_DEVID_CPU_FAN_CURVE);
> +	if (err)
> +		return err;
> +
> +	err = fan_curve_check_present(asus, &asus->gpu_fan_curve_available,
> +				      ASUS_WMI_DEVID_GPU_FAN_CURVE);
> +	if (err)
> +		return err;

What if neither are present, should you not do a return 0 then ?

> +
> +	hwmon = devm_hwmon_device_register_with_groups(
> +		dev, "asus_custom_fan_curve", asus, asus_fan_curve_attr_groups);
> +
> +	if (IS_ERR(hwmon)) {
> +		dev_err(dev,
> +			"Could not register asus_custom_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 +2633,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);

Unrelated change ?  Also the old indentation seems better, please drop this.

>  	if (err) {
>  		if (err == -ENODEV)
>  			return 0;
> @@ -2092,6 +2672,12 @@ static int throttle_thermal_policy_write(struct asus_wmi *asus)
>  		return -EIO;
>  	}
>  
> +	/* Must set to disabled if mode is toggled */
> +	if (asus->cpu_fan_curve_available)
> +		asus->custom_fan_curves[FAN_CURVE_DEV_CPU].enabled = false;
> +	if (asus->gpu_fan_curve_available)
> +		asus->custom_fan_curves[FAN_CURVE_DEV_GPU].enabled = false;
> +
>  	return 0;
>  }
>  
> @@ -2901,7 +3487,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,

Unrelated change, please drop.

> @@ -3035,6 +3621,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	if (err)
>  		goto fail_hwmon;
>  
> +	err = asus_wmi_custom_fan_curve_init(asus);
> +	if (err)
> +		goto fail_custom_fan_curve;
> +
>  	err = asus_wmi_led_init(asus);
>  	if (err)
>  		goto fail_leds;
> @@ -3106,6 +3696,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();
> @@ -3131,6 +3722,7 @@ static int asus_wmi_remove(struct platform_device *device)
>  	asus_wmi_debugfs_exit(asus);
>  	asus_wmi_sysfs_exit(asus->platform_device);
>  	asus_fan_set_auto(asus);
> +	throttle_thermal_policy_set_default(asus);
>  	asus_wmi_battery_exit(asus);
>  
>  	if (asus->platform_profile_support)
> 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
> 

Regards,

Hans

Powered by blists - more mailing lists