[PATCH] platform/x86: hp-wmi: add support for omen laptops

[PATCH] platform/x86: hp-wmi: add support for omen laptops

[Enver Balalic]

This patch adds support for HP Omen laptops.
It adds support for most things that can be controlled via the
Windows Omen Command Center application.

 - Fan speed monitoring through hwmon
 - Platform Profile support (cool, balanced, performance)
 - Max fan speed function toggle

This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.

Signed-off-by: Enver Balalic <balalic.enver@gmail.com>
---
 drivers/platform/x86/Kconfig  |   1 +
 drivers/platform/x86/hp-wmi.c | 292 ++++++++++++++++++++++++++++++++--
 2 files changed, 282 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index d12db6c316ea..f0b3d94e182b 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -431,6 +431,7 @@ config HP_WMI
 	tristate "HP WMI extras"
 	depends on ACPI_WMI
 	depends on INPUT
+	depends on HWMON
 	depends on RFKILL || RFKILL = n
 	select INPUT_SPARSEKMAP
 	select ACPI_PLATFORM_PROFILE
diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 027a1467d009..39d26602376d 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -22,6 +22,7 @@
 #include <linux/input/sparse-keymap.h>
 #include <linux/platform_device.h>
 #include <linux/platform_profile.h>
+#include <linux/hwmon.h>
 #include <linux/acpi.h>
 #include <linux/rfkill.h>
 #include <linux/string.h>
@@ -39,6 +40,7 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
 
 #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
 #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
+#define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
 
 enum hp_wmi_radio {
 	HPWMI_WIFI	= 0x0,
@@ -89,10 +91,18 @@ enum hp_wmi_commandtype {
 	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
 };
 
+enum hp_wmi_gm_commandtype {
+	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
+	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
+	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
+	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
+};
+
 enum hp_wmi_command {
 	HPWMI_READ	= 0x01,
 	HPWMI_WRITE	= 0x02,
 	HPWMI_ODM	= 0x03,
+	HPWMI_GM    = 0x20008,
 };
 
 enum hp_wmi_hardware_mask {
@@ -120,6 +130,13 @@ enum hp_wireless2_bits {
 	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
 };
 
+
+enum hp_thermal_profile_omen {
+	HP_OMEN_THERMAL_PROFILE_DEFAULT     = 0x00,
+	HP_OMEN_THERMAL_PROFILE_PERFORMANCE = 0x01,
+	HP_OMEN_THERMAL_PROFILE_COOL        = 0x02,
+};
+
 enum hp_thermal_profile {
 	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
 	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
@@ -279,6 +296,27 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 	return ret;
 }
 
+static int hp_wmi_get_fan_speed(int fan, int *data)
+{
+	int fsh, fsl;
+	char fan_data[4] = { fan, 0, 0, 0 };
+
+	int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
+				       &fan_data, sizeof(fan_data),
+				       sizeof(fan_data));
+
+	if (ret != 0)
+		return -EINVAL;
+
+	fsh = fan_data[2];
+	fsl = fan_data[3];
+
+	// sometimes one of these can be negative
+	*data = ((fsh > 0 ? fsh : 0) << 8) | (fsl > 0 ? fsl : 0);
+
+	return 0;
+}
+
 static int hp_wmi_read_int(int query)
 {
 	int val = 0, ret;
@@ -302,6 +340,61 @@ static int hp_wmi_hw_state(int mask)
 	return !!(state & mask);
 }
 
+static int omen_thermal_profile_set(int mode)
+{
+	char buffer[2] = {0, mode};
+	int ret;
+
+	if (mode < 0 || mode > 2)
+		return -EINVAL;
+
+	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
+				   &buffer, sizeof(buffer), sizeof(buffer));
+
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
+	return mode;
+}
+
+static int hp_wmi_fan_speed_max_set(int enabled)
+{
+	int ret;
+
+	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_SET_QUERY, HPWMI_GM,
+				   &enabled, sizeof(enabled), sizeof(enabled));
+
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
+	return enabled;
+}
+
+static int hp_wmi_fan_speed_max_get(void)
+{
+	int val = 0, ret;
+
+	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
+				   &val, sizeof(val), sizeof(val));
+
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
+	return val;
+}
+
+static int omen_thermal_profile_get(void)
+{
+	u8 data;
+
+	int ret = ec_read(HP_OMEN_EC_THERMAL_PROFILE_OFFSET, &data);
+
+	if (ret)
+		return ret;
+
+	return data;
+}
+
 static int __init hp_wmi_bios_2008_later(void)
 {
 	int state = 0;
@@ -473,6 +566,42 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "0x%x\n", value);
 }
 
+static ssize_t max_fan_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	int value = hp_wmi_fan_speed_max_get();
+
+	if (value < 0)
+		return value;
+	return sprintf(buf, "%d\n", value);
+}
+
+static ssize_t fan1_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	int fan_speed;
+
+	int ret = hp_wmi_get_fan_speed(0, &fan_speed);
+
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", fan_speed);
+}
+
+static ssize_t fan2_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	int fan_speed;
+
+	int ret = hp_wmi_get_fan_speed(1, &fan_speed);
+
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", fan_speed);
+}
+
 static ssize_t als_store(struct device *dev, struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
@@ -514,12 +643,33 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t max_fan_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	u32 tmp;
+	int ret;
+
+	ret = kstrtou32(buf, 10, &tmp);
+	if (ret)
+		return ret;
+
+	ret = hp_wmi_fan_speed_max_set(tmp);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
 static DEVICE_ATTR_RO(display);
 static DEVICE_ATTR_RO(hddtemp);
 static DEVICE_ATTR_RW(als);
 static DEVICE_ATTR_RO(dock);
 static DEVICE_ATTR_RO(tablet);
 static DEVICE_ATTR_RW(postcode);
+static DEVICE_ATTR_RW(max_fan);
+static DEVICE_ATTR_RO(fan1);
+static DEVICE_ATTR_RO(fan2);
 
 static struct attribute *hp_wmi_attrs[] = {
 	&dev_attr_display.attr,
@@ -528,6 +678,7 @@ static struct attribute *hp_wmi_attrs[] = {
 	&dev_attr_dock.attr,
 	&dev_attr_tablet.attr,
 	&dev_attr_postcode.attr,
+	&dev_attr_max_fan.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(hp_wmi);
@@ -878,6 +1029,58 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 	return err;
 }
 
+static int platform_profile_omen_get(struct platform_profile_handler *pprof,
+				enum platform_profile_option *profile)
+{
+	int tp;
+
+	tp = omen_thermal_profile_get();
+	if (tp < 0)
+		return tp;
+
+	switch (tp) {
+	case HP_OMEN_THERMAL_PROFILE_PERFORMANCE:
+		*profile = PLATFORM_PROFILE_PERFORMANCE;
+		break;
+	case HP_OMEN_THERMAL_PROFILE_DEFAULT:
+		*profile = PLATFORM_PROFILE_BALANCED;
+		break;
+	case HP_OMEN_THERMAL_PROFILE_COOL:
+		*profile = PLATFORM_PROFILE_COOL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int platform_profile_omen_set(struct platform_profile_handler *pprof,
+				enum platform_profile_option profile)
+{
+	int err, tp;
+
+	switch (profile) {
+	case PLATFORM_PROFILE_PERFORMANCE:
+		tp = HP_OMEN_THERMAL_PROFILE_PERFORMANCE;
+		break;
+	case PLATFORM_PROFILE_BALANCED:
+		tp = HP_OMEN_THERMAL_PROFILE_DEFAULT;
+		break;
+	case PLATFORM_PROFILE_COOL:
+		tp = HP_OMEN_THERMAL_PROFILE_COOL;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = omen_thermal_profile_set(tp);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static int thermal_profile_get(void)
 {
 	return hp_wmi_read_int(HPWMI_THERMAL_PROFILE_QUERY);
@@ -946,19 +1149,34 @@ static int thermal_profile_setup(void)
 	int err, tp;
 
 	tp = thermal_profile_get();
-	if (tp < 0)
-		return tp;
+	if (tp >= 0) {
+		/*
+		* call thermal profile write command to ensure that the firmware correctly
+		* sets the OEM variables for the DPTF
+		*/
+		err = thermal_profile_set(tp);
+		if (err)
+			return err;
 
-	/*
-	 * call thermal profile write command to ensure that the firmware correctly
-	 * sets the OEM variables for the DPTF
-	 */
-	err = thermal_profile_set(tp);
-	if (err)
-		return err;
+		platform_profile_handler.profile_get = platform_profile_get;
+		platform_profile_handler.profile_set = platform_profile_set;
+	}
+
+	tp = omen_thermal_profile_get();
+	if (tp >= 0) {
+		/*
+		* call thermal profile write command to ensure that the firmware correctly
+		* sets the OEM variables for the DPTF
+		*/
+		err = omen_thermal_profile_set(tp);
+		if (err < 0)
+			return err;
 
-	platform_profile_handler.profile_get = platform_profile_get,
-	platform_profile_handler.profile_set = platform_profile_set,
+		platform_profile_handler.profile_get = platform_profile_omen_get;
+		platform_profile_handler.profile_set = platform_profile_omen_set;
+	} else {
+		return tp;
+	}
 
 	set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
 	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
@@ -973,6 +1191,8 @@ static int thermal_profile_setup(void)
 	return 0;
 }
 
+static int hp_wmi_hwmon_init(void);
+
 static int __init hp_wmi_bios_setup(struct platform_device *device)
 {
 	/* clear detected rfkill devices */
@@ -984,6 +1204,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 	if (hp_wmi_rfkill_setup(device))
 		hp_wmi_rfkill2_setup(device);
 
+	hp_wmi_hwmon_init();
+
 	thermal_profile_setup();
 
 	return 0;
@@ -1068,6 +1290,54 @@ static struct platform_driver hp_wmi_driver = {
 	.remove = __exit_p(hp_wmi_bios_remove),
 };
 
+static struct attribute *hwmon_attributes[] = {
+	&dev_attr_fan1.attr,
+	&dev_attr_fan2.attr,
+	NULL
+};
+
+static umode_t hp_wmi_hwmon_sysfs_is_visible(struct kobject *kobj,
+					  struct attribute *attr, int idx)
+{
+	int fan_speed;
+
+	if (attr == &dev_attr_fan1.attr) {
+		int ret = hp_wmi_get_fan_speed(0, &fan_speed);
+
+		if (ret < 0)
+			return 0;
+	} else if (attr == &dev_attr_fan2.attr) {
+		int ret = hp_wmi_get_fan_speed(1, &fan_speed);
+
+		if (ret < 0)
+			return 0;
+	}
+
+	return attr->mode;
+}
+
+static const struct attribute_group hwmon_attribute_group = {
+	.is_visible = hp_wmi_hwmon_sysfs_is_visible,
+	.attrs = hwmon_attributes
+};
+__ATTRIBUTE_GROUPS(hwmon_attribute);
+
+static int hp_wmi_hwmon_init(void)
+{
+	struct device *dev = &hp_wmi_platform_dev->dev;
+	struct device *hwmon;
+
+	hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
+			hwmon_attribute_groups);
+
+	if (IS_ERR(hwmon)) {
+		pr_err("Could not register hp hwmon device\n");
+		return PTR_ERR(hwmon);
+	}
+
+	return 0;
+}
+
 static int __init hp_wmi_init(void)
 {
 	int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
-- 
2.33.0

Re: [PATCH] platform/x86: hp-wmi: add support for omen laptops

[Barnabás Pőcze]

Hi


+CC Guenter Roeck and Jean Delvare for hwmon parts


2021. augusztus 22., vasárnap 18:31 keltezéssel, Enver Balalic írta:
> This patch adds support for HP Omen laptops.
> It adds support for most things that can be controlled via the
> Windows Omen Command Center application.
>
>  - Fan speed monitoring through hwmon
>  - Platform Profile support (cool, balanced, performance)
>  - Max fan speed function toggle
>
> This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.
>
> Signed-off-by: Enver Balalic <balalic.enver@gmail.com>
> ---
>  drivers/platform/x86/Kconfig  |   1 +
>  drivers/platform/x86/hp-wmi.c | 292 ++++++++++++++++++++++++++++++++--
>  2 files changed, 282 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index d12db6c316ea..f0b3d94e182b 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -431,6 +431,7 @@ config HP_WMI
>  	tristate "HP WMI extras"
>  	depends on ACPI_WMI
>  	depends on INPUT
> +	depends on HWMON
>  	depends on RFKILL || RFKILL = n
>  	select INPUT_SPARSEKMAP
>  	select ACPI_PLATFORM_PROFILE
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 027a1467d009..39d26602376d 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -22,6 +22,7 @@
>  #include <linux/input/sparse-keymap.h>
>  #include <linux/platform_device.h>
>  #include <linux/platform_profile.h>
> +#include <linux/hwmon.h>
>  #include <linux/acpi.h>
>  #include <linux/rfkill.h>
>  #include <linux/string.h>
> @@ -39,6 +40,7 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
>
>  #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
>  #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> +#define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>
>  enum hp_wmi_radio {
>  	HPWMI_WIFI	= 0x0,
> @@ -89,10 +91,18 @@ enum hp_wmi_commandtype {
>  	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
>  };
>
> +enum hp_wmi_gm_commandtype {
> +	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> +	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> +	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
> +	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> +};
> +
>  enum hp_wmi_command {
>  	HPWMI_READ	= 0x01,
>  	HPWMI_WRITE	= 0x02,
>  	HPWMI_ODM	= 0x03,
> +	HPWMI_GM    = 0x20008,
>  };
>
>  enum hp_wmi_hardware_mask {
> @@ -120,6 +130,13 @@ enum hp_wireless2_bits {
>  	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
>  };
>
> +
> +enum hp_thermal_profile_omen {
> +	HP_OMEN_THERMAL_PROFILE_DEFAULT     = 0x00,
> +	HP_OMEN_THERMAL_PROFILE_PERFORMANCE = 0x01,
> +	HP_OMEN_THERMAL_PROFILE_COOL        = 0x02,
> +};
> +
>  enum hp_thermal_profile {
>  	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
>  	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
> @@ -279,6 +296,27 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  	return ret;
>  }
>
> +static int hp_wmi_get_fan_speed(int fan, int *data)
> +{
> +	int fsh, fsl;
> +	char fan_data[4] = { fan, 0, 0, 0 };
> +
> +	int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
> +				       &fan_data, sizeof(fan_data),
> +				       sizeof(fan_data));
> +
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	fsh = fan_data[2];
> +	fsl = fan_data[3];
> +
> +	// sometimes one of these can be negative
> +	*data = ((fsh > 0 ? fsh : 0) << 8) | (fsl > 0 ? fsl : 0);
> +
> +	return 0;
> +}
> +
>  static int hp_wmi_read_int(int query)
>  {
>  	int val = 0, ret;
> @@ -302,6 +340,61 @@ static int hp_wmi_hw_state(int mask)
>  	return !!(state & mask);
>  }
>
> +static int omen_thermal_profile_set(int mode)
> +{
> +	char buffer[2] = {0, mode};
> +	int ret;
> +
> +	if (mode < 0 || mode > 2)
> +		return -EINVAL;
> +
> +	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
> +				   &buffer, sizeof(buffer), sizeof(buffer));
> +
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
> +	return mode;
> +}
> +
> +static int hp_wmi_fan_speed_max_set(int enabled)
> +{
> +	int ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_SET_QUERY, HPWMI_GM,
> +				   &enabled, sizeof(enabled), sizeof(enabled));
> +
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
> +	return enabled;
> +}
> +
> +static int hp_wmi_fan_speed_max_get(void)
> +{
> +	int val = 0, ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
> +				   &val, sizeof(val), sizeof(val));
> +
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
> +	return val;
> +}
> +
> +static int omen_thermal_profile_get(void)
> +{
> +	u8 data;
> +
> +	int ret = ec_read(HP_OMEN_EC_THERMAL_PROFILE_OFFSET, &data);
> +
> +	if (ret)
> +		return ret;
> +
> +	return data;
> +}
> +
>  static int __init hp_wmi_bios_2008_later(void)
>  {
>  	int state = 0;
> @@ -473,6 +566,42 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
>  	return sprintf(buf, "0x%x\n", value);
>  }
>
> +static ssize_t max_fan_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	int value = hp_wmi_fan_speed_max_get();
> +
> +	if (value < 0)
> +		return value;
> +	return sprintf(buf, "%d\n", value);
> +}
> +
> +static ssize_t fan1_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	int fan_speed;
> +
> +	int ret = hp_wmi_get_fan_speed(0, &fan_speed);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", fan_speed);
> +}
> +
> +static ssize_t fan2_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	int fan_speed;
> +
> +	int ret = hp_wmi_get_fan_speed(1, &fan_speed);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", fan_speed);
> +}
> +
>  static ssize_t als_store(struct device *dev, struct device_attribute *attr,
>  			 const char *buf, size_t count)
>  {
> @@ -514,12 +643,33 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>
> +static ssize_t max_fan_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	u32 tmp;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 10, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	ret = hp_wmi_fan_speed_max_set(tmp);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
>  static DEVICE_ATTR_RO(display);
>  static DEVICE_ATTR_RO(hddtemp);
>  static DEVICE_ATTR_RW(als);
>  static DEVICE_ATTR_RO(dock);
>  static DEVICE_ATTR_RO(tablet);
>  static DEVICE_ATTR_RW(postcode);
> +static DEVICE_ATTR_RW(max_fan);
> +static DEVICE_ATTR_RO(fan1);
> +static DEVICE_ATTR_RO(fan2);
>
>  static struct attribute *hp_wmi_attrs[] = {
>  	&dev_attr_display.attr,
> @@ -528,6 +678,7 @@ static struct attribute *hp_wmi_attrs[] = {
>  	&dev_attr_dock.attr,
>  	&dev_attr_tablet.attr,
>  	&dev_attr_postcode.attr,
> +	&dev_attr_max_fan.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(hp_wmi);
> @@ -878,6 +1029,58 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>  	return err;
>  }
>
> +static int platform_profile_omen_get(struct platform_profile_handler *pprof,
> +				enum platform_profile_option *profile)
> +{
> +	int tp;
> +
> +	tp = omen_thermal_profile_get();
> +	if (tp < 0)
> +		return tp;
> +
> +	switch (tp) {
> +	case HP_OMEN_THERMAL_PROFILE_PERFORMANCE:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case HP_OMEN_THERMAL_PROFILE_DEFAULT:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case HP_OMEN_THERMAL_PROFILE_COOL:
> +		*profile = PLATFORM_PROFILE_COOL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int platform_profile_omen_set(struct platform_profile_handler *pprof,
> +				enum platform_profile_option profile)
> +{
> +	int err, tp;
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		tp = HP_OMEN_THERMAL_PROFILE_PERFORMANCE;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		tp = HP_OMEN_THERMAL_PROFILE_DEFAULT;
> +		break;
> +	case PLATFORM_PROFILE_COOL:
> +		tp = HP_OMEN_THERMAL_PROFILE_COOL;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = omen_thermal_profile_set(tp);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
>  static int thermal_profile_get(void)
>  {
>  	return hp_wmi_read_int(HPWMI_THERMAL_PROFILE_QUERY);
> @@ -946,19 +1149,34 @@ static int thermal_profile_setup(void)
>  	int err, tp;
>
>  	tp = thermal_profile_get();
> -	if (tp < 0)
> -		return tp;
> +	if (tp >= 0) {
> +		/*
> +		* call thermal profile write command to ensure that the firmware correctly
> +		* sets the OEM variables for the DPTF
> +		*/
> +		err = thermal_profile_set(tp);
> +		if (err)
> +			return err;
>
> -	/*
> -	 * call thermal profile write command to ensure that the firmware correctly
> -	 * sets the OEM variables for the DPTF
> -	 */
> -	err = thermal_profile_set(tp);
> -	if (err)
> -		return err;
> +		platform_profile_handler.profile_get = platform_profile_get;
> +		platform_profile_handler.profile_set = platform_profile_set;
> +	}
> +
> +	tp = omen_thermal_profile_get();
> +	if (tp >= 0) {
> +		/*
> +		* call thermal profile write command to ensure that the firmware correctly
> +		* sets the OEM variables for the DPTF
> +		*/
> +		err = omen_thermal_profile_set(tp);
> +		if (err < 0)
> +			return err;
>
> -	platform_profile_handler.profile_get = platform_profile_get,
> -	platform_profile_handler.profile_set = platform_profile_set,
> +		platform_profile_handler.profile_get = platform_profile_omen_get;
> +		platform_profile_handler.profile_set = platform_profile_omen_set;
> +	} else {
> +		return tp;
> +	}
>
>  	set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
>  	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
> @@ -973,6 +1191,8 @@ static int thermal_profile_setup(void)
>  	return 0;
>  }
>
> +static int hp_wmi_hwmon_init(void);
> +
>  static int __init hp_wmi_bios_setup(struct platform_device *device)
>  {
>  	/* clear detected rfkill devices */
> @@ -984,6 +1204,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  	if (hp_wmi_rfkill_setup(device))
>  		hp_wmi_rfkill2_setup(device);
>
> +	hp_wmi_hwmon_init();
> +
>  	thermal_profile_setup();
>
>  	return 0;
> @@ -1068,6 +1290,54 @@ static struct platform_driver hp_wmi_driver = {
>  	.remove = __exit_p(hp_wmi_bios_remove),
>  };
>
> +static struct attribute *hwmon_attributes[] = {
> +	&dev_attr_fan1.attr,
> +	&dev_attr_fan2.attr,
> +	NULL
> +};
> +
> +static umode_t hp_wmi_hwmon_sysfs_is_visible(struct kobject *kobj,
> +					  struct attribute *attr, int idx)
> +{
> +	int fan_speed;
> +
> +	if (attr == &dev_attr_fan1.attr) {
> +		int ret = hp_wmi_get_fan_speed(0, &fan_speed);
> +
> +		if (ret < 0)
> +			return 0;
> +	} else if (attr == &dev_attr_fan2.attr) {
> +		int ret = hp_wmi_get_fan_speed(1, &fan_speed);
> +
> +		if (ret < 0)
> +			return 0;
> +	}
> +
> +	return attr->mode;
> +}
> +
> +static const struct attribute_group hwmon_attribute_group = {
> +	.is_visible = hp_wmi_hwmon_sysfs_is_visible,
> +	.attrs = hwmon_attributes
> +};
> +__ATTRIBUTE_GROUPS(hwmon_attribute);
> +
> +static int hp_wmi_hwmon_init(void)
> +{
> +	struct device *dev = &hp_wmi_platform_dev->dev;
> +	struct device *hwmon;
> +
> +	hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
> +			hwmon_attribute_groups);

I think you should use

  [devm_]hwmon_device_register_with_info()

as it creates sysfs attributes for you, etc. You wouldn't need to manually
create device attributes, and you wouldn't need fan{1,2}_show() at all.


> +
> +	if (IS_ERR(hwmon)) {
> +		pr_err("Could not register hp hwmon device\n");
> +		return PTR_ERR(hwmon);
> +	}
> +
> +	return 0;
> +}
> +
>  static int __init hp_wmi_init(void)
>  {
>  	int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
> --
> 2.33.0


Best regards,
Barnabás Pőcze

Re: [PATCH] platform/x86: hp-wmi: add support for omen laptops

[Guenter Roeck]

On 8/22/21 10:04 AM, Barnabás Pőcze wrote:
> Hi
> 
> 
> +CC Guenter Roeck and Jean Delvare for hwmon parts
> 
> 
> 2021. augusztus 22., vasárnap 18:31 keltezéssel, Enver Balalic írta:
>> This patch adds support for HP Omen laptops.
>> It adds support for most things that can be controlled via the
>> Windows Omen Command Center application.
>>
>>   - Fan speed monitoring through hwmon
>>   - Platform Profile support (cool, balanced, performance)
>>   - Max fan speed function toggle
>>
>> This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.
>>
>> Signed-off-by: Enver Balalic <balalic.enver@gmail.com>
>> ---
>>   drivers/platform/x86/Kconfig  |   1 +
>>   drivers/platform/x86/hp-wmi.c | 292 ++++++++++++++++++++++++++++++++--
>>   2 files changed, 282 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index d12db6c316ea..f0b3d94e182b 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -431,6 +431,7 @@ config HP_WMI
>>   	tristate "HP WMI extras"
>>   	depends on ACPI_WMI
>>   	depends on INPUT
>> +	depends on HWMON
>>   	depends on RFKILL || RFKILL = n
>>   	select INPUT_SPARSEKMAP
>>   	select ACPI_PLATFORM_PROFILE
>> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
>> index 027a1467d009..39d26602376d 100644
>> --- a/drivers/platform/x86/hp-wmi.c
>> +++ b/drivers/platform/x86/hp-wmi.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/input/sparse-keymap.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/platform_profile.h>
>> +#include <linux/hwmon.h>
>>   #include <linux/acpi.h>
>>   #include <linux/rfkill.h>
>>   #include <linux/string.h>
>> @@ -39,6 +40,7 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
>>
>>   #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
>>   #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
>> +#define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>>
>>   enum hp_wmi_radio {
>>   	HPWMI_WIFI	= 0x0,
>> @@ -89,10 +91,18 @@ enum hp_wmi_commandtype {
>>   	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
>>   };
>>
>> +enum hp_wmi_gm_commandtype {
>> +	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
>> +	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
>> +	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
>> +	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
>> +};
>> +
>>   enum hp_wmi_command {
>>   	HPWMI_READ	= 0x01,
>>   	HPWMI_WRITE	= 0x02,
>>   	HPWMI_ODM	= 0x03,
>> +	HPWMI_GM    = 0x20008,
>>   };
>>
>>   enum hp_wmi_hardware_mask {
>> @@ -120,6 +130,13 @@ enum hp_wireless2_bits {
>>   	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
>>   };
>>
>> +
>> +enum hp_thermal_profile_omen {
>> +	HP_OMEN_THERMAL_PROFILE_DEFAULT     = 0x00,
>> +	HP_OMEN_THERMAL_PROFILE_PERFORMANCE = 0x01,
>> +	HP_OMEN_THERMAL_PROFILE_COOL        = 0x02,
>> +};
>> +
>>   enum hp_thermal_profile {
>>   	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
>>   	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
>> @@ -279,6 +296,27 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>   	return ret;
>>   }
>>
>> +static int hp_wmi_get_fan_speed(int fan, int *data)
>> +{
>> +	int fsh, fsl;
>> +	char fan_data[4] = { fan, 0, 0, 0 };
>> +
>> +	int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
>> +				       &fan_data, sizeof(fan_data),
>> +				       sizeof(fan_data));
>> +
>> +	if (ret != 0)
>> +		return -EINVAL;
>> +
>> +	fsh = fan_data[2];
>> +	fsl = fan_data[3];
>> +
>> +	// sometimes one of these can be negative
>> +	*data = ((fsh > 0 ? fsh : 0) << 8) | (fsl > 0 ? fsl : 0);
>> +
>> +	return 0;
>> +}
>> +
>>   static int hp_wmi_read_int(int query)
>>   {
>>   	int val = 0, ret;
>> @@ -302,6 +340,61 @@ static int hp_wmi_hw_state(int mask)
>>   	return !!(state & mask);
>>   }
>>
>> +static int omen_thermal_profile_set(int mode)
>> +{
>> +	char buffer[2] = {0, mode};
>> +	int ret;
>> +
>> +	if (mode < 0 || mode > 2)
>> +		return -EINVAL;
>> +
>> +	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
>> +				   &buffer, sizeof(buffer), sizeof(buffer));
>> +
>> +	if (ret)
>> +		return ret < 0 ? ret : -EINVAL;
>> +
>> +	return mode;
>> +}
>> +
>> +static int hp_wmi_fan_speed_max_set(int enabled)
>> +{
>> +	int ret;
>> +
>> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_SET_QUERY, HPWMI_GM,
>> +				   &enabled, sizeof(enabled), sizeof(enabled));
>> +
>> +	if (ret)
>> +		return ret < 0 ? ret : -EINVAL;
>> +
>> +	return enabled;
>> +}
>> +
>> +static int hp_wmi_fan_speed_max_get(void)
>> +{
>> +	int val = 0, ret;
>> +
>> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
>> +				   &val, sizeof(val), sizeof(val));
>> +
>> +	if (ret)
>> +		return ret < 0 ? ret : -EINVAL;
>> +
>> +	return val;
>> +}
>> +
>> +static int omen_thermal_profile_get(void)
>> +{
>> +	u8 data;
>> +
>> +	int ret = ec_read(HP_OMEN_EC_THERMAL_PROFILE_OFFSET, &data);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	return data;
>> +}
>> +
>>   static int __init hp_wmi_bios_2008_later(void)
>>   {
>>   	int state = 0;
>> @@ -473,6 +566,42 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
>>   	return sprintf(buf, "0x%x\n", value);
>>   }
>>
>> +static ssize_t max_fan_show(struct device *dev, struct device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	int value = hp_wmi_fan_speed_max_get();
>> +
>> +	if (value < 0)
>> +		return value;
>> +	return sprintf(buf, "%d\n", value);
>> +}
>> +
>> +static ssize_t fan1_show(struct device *dev, struct device_attribute *attr,
>> +			char *buf)
>> +{
>> +	int fan_speed;
>> +
>> +	int ret = hp_wmi_get_fan_speed(0, &fan_speed);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return sprintf(buf, "%d\n", fan_speed);
>> +}
>> +
>> +static ssize_t fan2_show(struct device *dev, struct device_attribute *attr,
>> +			char *buf)
>> +{
>> +	int fan_speed;
>> +
>> +	int ret = hp_wmi_get_fan_speed(1, &fan_speed);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return sprintf(buf, "%d\n", fan_speed);
>> +}
>> +
>>   static ssize_t als_store(struct device *dev, struct device_attribute *attr,
>>   			 const char *buf, size_t count)
>>   {
>> @@ -514,12 +643,33 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>>   	return count;
>>   }
>>
>> +static ssize_t max_fan_store(struct device *dev, struct device_attribute *attr,
>> +			     const char *buf, size_t count)
>> +{
>> +	u32 tmp;
>> +	int ret;
>> +
>> +	ret = kstrtou32(buf, 10, &tmp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = hp_wmi_fan_speed_max_set(tmp);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>>   static DEVICE_ATTR_RO(display);
>>   static DEVICE_ATTR_RO(hddtemp);
>>   static DEVICE_ATTR_RW(als);
>>   static DEVICE_ATTR_RO(dock);
>>   static DEVICE_ATTR_RO(tablet);
>>   static DEVICE_ATTR_RW(postcode);
>> +static DEVICE_ATTR_RW(max_fan);
>> +static DEVICE_ATTR_RO(fan1);
>> +static DEVICE_ATTR_RO(fan2);
>>
>>   static struct attribute *hp_wmi_attrs[] = {
>>   	&dev_attr_display.attr,
>> @@ -528,6 +678,7 @@ static struct attribute *hp_wmi_attrs[] = {
>>   	&dev_attr_dock.attr,
>>   	&dev_attr_tablet.attr,
>>   	&dev_attr_postcode.attr,
>> +	&dev_attr_max_fan.attr,
>>   	NULL,
>>   };
>>   ATTRIBUTE_GROUPS(hp_wmi);
>> @@ -878,6 +1029,58 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>>   	return err;
>>   }
>>
>> +static int platform_profile_omen_get(struct platform_profile_handler *pprof,
>> +				enum platform_profile_option *profile)
>> +{
>> +	int tp;
>> +
>> +	tp = omen_thermal_profile_get();
>> +	if (tp < 0)
>> +		return tp;
>> +
>> +	switch (tp) {
>> +	case HP_OMEN_THERMAL_PROFILE_PERFORMANCE:
>> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
>> +		break;
>> +	case HP_OMEN_THERMAL_PROFILE_DEFAULT:
>> +		*profile = PLATFORM_PROFILE_BALANCED;
>> +		break;
>> +	case HP_OMEN_THERMAL_PROFILE_COOL:
>> +		*profile = PLATFORM_PROFILE_COOL;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int platform_profile_omen_set(struct platform_profile_handler *pprof,
>> +				enum platform_profile_option profile)
>> +{
>> +	int err, tp;
>> +
>> +	switch (profile) {
>> +	case PLATFORM_PROFILE_PERFORMANCE:
>> +		tp = HP_OMEN_THERMAL_PROFILE_PERFORMANCE;
>> +		break;
>> +	case PLATFORM_PROFILE_BALANCED:
>> +		tp = HP_OMEN_THERMAL_PROFILE_DEFAULT;
>> +		break;
>> +	case PLATFORM_PROFILE_COOL:
>> +		tp = HP_OMEN_THERMAL_PROFILE_COOL;
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	err = omen_thermal_profile_set(tp);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	return 0;
>> +}
>> +
>>   static int thermal_profile_get(void)
>>   {
>>   	return hp_wmi_read_int(HPWMI_THERMAL_PROFILE_QUERY);
>> @@ -946,19 +1149,34 @@ static int thermal_profile_setup(void)
>>   	int err, tp;
>>
>>   	tp = thermal_profile_get();
>> -	if (tp < 0)
>> -		return tp;
>> +	if (tp >= 0) {
>> +		/*
>> +		* call thermal profile write command to ensure that the firmware correctly
>> +		* sets the OEM variables for the DPTF
>> +		*/
>> +		err = thermal_profile_set(tp);
>> +		if (err)
>> +			return err;
>>
>> -	/*
>> -	 * call thermal profile write command to ensure that the firmware correctly
>> -	 * sets the OEM variables for the DPTF
>> -	 */
>> -	err = thermal_profile_set(tp);
>> -	if (err)
>> -		return err;
>> +		platform_profile_handler.profile_get = platform_profile_get;
>> +		platform_profile_handler.profile_set = platform_profile_set;
>> +	}
>> +
>> +	tp = omen_thermal_profile_get();
>> +	if (tp >= 0) {
>> +		/*
>> +		* call thermal profile write command to ensure that the firmware correctly
>> +		* sets the OEM variables for the DPTF
>> +		*/
>> +		err = omen_thermal_profile_set(tp);
>> +		if (err < 0)
>> +			return err;
>>
>> -	platform_profile_handler.profile_get = platform_profile_get,
>> -	platform_profile_handler.profile_set = platform_profile_set,
>> +		platform_profile_handler.profile_get = platform_profile_omen_get;
>> +		platform_profile_handler.profile_set = platform_profile_omen_set;
>> +	} else {
>> +		return tp;
>> +	}
>>
>>   	set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
>>   	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
>> @@ -973,6 +1191,8 @@ static int thermal_profile_setup(void)
>>   	return 0;
>>   }
>>
>> +static int hp_wmi_hwmon_init(void);
>> +
>>   static int __init hp_wmi_bios_setup(struct platform_device *device)
>>   {
>>   	/* clear detected rfkill devices */
>> @@ -984,6 +1204,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>>   	if (hp_wmi_rfkill_setup(device))
>>   		hp_wmi_rfkill2_setup(device);
>>
>> +	hp_wmi_hwmon_init();
>> +
>>   	thermal_profile_setup();
>>
>>   	return 0;
>> @@ -1068,6 +1290,54 @@ static struct platform_driver hp_wmi_driver = {
>>   	.remove = __exit_p(hp_wmi_bios_remove),
>>   };
>>
>> +static struct attribute *hwmon_attributes[] = {
>> +	&dev_attr_fan1.attr,
>> +	&dev_attr_fan2.attr,
>> +	NULL
>> +};
>> +
>> +static umode_t hp_wmi_hwmon_sysfs_is_visible(struct kobject *kobj,
>> +					  struct attribute *attr, int idx)
>> +{
>> +	int fan_speed;
>> +
>> +	if (attr == &dev_attr_fan1.attr) {
>> +		int ret = hp_wmi_get_fan_speed(0, &fan_speed);
>> +
>> +		if (ret < 0)
>> +			return 0;
>> +	} else if (attr == &dev_attr_fan2.attr) {
>> +		int ret = hp_wmi_get_fan_speed(1, &fan_speed);
>> +
>> +		if (ret < 0)
>> +			return 0;
>> +	}
>> +
>> +	return attr->mode;
>> +}
>> +
>> +static const struct attribute_group hwmon_attribute_group = {
>> +	.is_visible = hp_wmi_hwmon_sysfs_is_visible,
>> +	.attrs = hwmon_attributes
>> +};
>> +__ATTRIBUTE_GROUPS(hwmon_attribute);
>> +
>> +static int hp_wmi_hwmon_init(void)
>> +{
>> +	struct device *dev = &hp_wmi_platform_dev->dev;
>> +	struct device *hwmon;
>> +
>> +	hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
>> +			hwmon_attribute_groups);
> 
> I think you should use
> 
>    [devm_]hwmon_device_register_with_info()
> 
> as it creates sysfs attributes for you, etc. You wouldn't need to manually
> create device attributes, and you wouldn't need fan{1,2}_show() at all.
> 

Correct. Also, the code as written doesn't really make sense as hwmon driver
because all its attributes are non-standard. The "sensors" command will show
nothing.

If you don't want to use standard hwmon ABI attributes, please don't register
a hwmon driver. The non-standard attributes can reside in the platform
sysfs directory. There is already the non-standard "hddtemp", so that would
not make much of a difference anyway. On top of that, "max_fan" is added as
non-hwmon attribute, making the hwmon registration even more pointless.

Guenter

> 
>> +
>> +	if (IS_ERR(hwmon)) {
>> +		pr_err("Could not register hp hwmon device\n");
>> +		return PTR_ERR(hwmon);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int __init hp_wmi_init(void)
>>   {
>>   	int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
>> --
>> 2.33.0
> 
> 
> Best regards,
> Barnabás Pőcze
> 

Re: [PATCH] platform/x86: hp-wmi: add support for omen laptops

[Enver Balalic]

On Sun, Aug 22, 2021 at 10:21:15AM -0700, Guenter Roeck wrote:
> On 8/22/21 10:04 AM, Barnabás Pőcze wrote:
> > Hi
> > 
> > 
> > +CC Guenter Roeck and Jean Delvare for hwmon parts
> > 
> > 
> > 2021. augusztus 22., vasárnap 18:31 keltezéssel, Enver Balalic írta:
> > > This patch adds support for HP Omen laptops.
> > > It adds support for most things that can be controlled via the
> > > Windows Omen Command Center application.
> > > 
> > >   - Fan speed monitoring through hwmon
> > >   - Platform Profile support (cool, balanced, performance)
> > >   - Max fan speed function toggle
> > > 
> > > This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.
> > > 
> > > Signed-off-by: Enver Balalic <balalic.enver@gmail.com>
> > > ---
> > >   drivers/platform/x86/Kconfig  |   1 +
> > >   drivers/platform/x86/hp-wmi.c | 292 ++++++++++++++++++++++++++++++++--
> > >   2 files changed, 282 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index d12db6c316ea..f0b3d94e182b 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -431,6 +431,7 @@ config HP_WMI
> > >   	tristate "HP WMI extras"
> > >   	depends on ACPI_WMI
> > >   	depends on INPUT
> > > +	depends on HWMON
> > >   	depends on RFKILL || RFKILL = n
> > >   	select INPUT_SPARSEKMAP
> > >   	select ACPI_PLATFORM_PROFILE
> > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> > > index 027a1467d009..39d26602376d 100644
> > > --- a/drivers/platform/x86/hp-wmi.c
> > > +++ b/drivers/platform/x86/hp-wmi.c
> > > @@ -22,6 +22,7 @@
> > >   #include <linux/input/sparse-keymap.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/platform_profile.h>
> > > +#include <linux/hwmon.h>
> > >   #include <linux/acpi.h>
> > >   #include <linux/rfkill.h>
> > >   #include <linux/string.h>
> > > @@ -39,6 +40,7 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
> > > 
> > >   #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
> > >   #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> > > +#define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> > > 
> > >   enum hp_wmi_radio {
> > >   	HPWMI_WIFI	= 0x0,
> > > @@ -89,10 +91,18 @@ enum hp_wmi_commandtype {
> > >   	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
> > >   };
> > > 
> > > +enum hp_wmi_gm_commandtype {
> > > +	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> > > +	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> > > +	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
> > > +	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> > > +};
> > > +
> > >   enum hp_wmi_command {
> > >   	HPWMI_READ	= 0x01,
> > >   	HPWMI_WRITE	= 0x02,
> > >   	HPWMI_ODM	= 0x03,
> > > +	HPWMI_GM    = 0x20008,
> > >   };
> > > 
> > >   enum hp_wmi_hardware_mask {
> > > @@ -120,6 +130,13 @@ enum hp_wireless2_bits {
> > >   	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
> > >   };
> > > 
> > > +
> > > +enum hp_thermal_profile_omen {
> > > +	HP_OMEN_THERMAL_PROFILE_DEFAULT     = 0x00,
> > > +	HP_OMEN_THERMAL_PROFILE_PERFORMANCE = 0x01,
> > > +	HP_OMEN_THERMAL_PROFILE_COOL        = 0x02,
> > > +};
> > > +
> > >   enum hp_thermal_profile {
> > >   	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
> > >   	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
> > > @@ -279,6 +296,27 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
> > >   	return ret;
> > >   }
> > > 
> > > +static int hp_wmi_get_fan_speed(int fan, int *data)
> > > +{
> > > +	int fsh, fsl;
> > > +	char fan_data[4] = { fan, 0, 0, 0 };
> > > +
> > > +	int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
> > > +				       &fan_data, sizeof(fan_data),
> > > +				       sizeof(fan_data));
> > > +
> > > +	if (ret != 0)
> > > +		return -EINVAL;
> > > +
> > > +	fsh = fan_data[2];
> > > +	fsl = fan_data[3];
> > > +
> > > +	// sometimes one of these can be negative
> > > +	*data = ((fsh > 0 ? fsh : 0) << 8) | (fsl > 0 ? fsl : 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int hp_wmi_read_int(int query)
> > >   {
> > >   	int val = 0, ret;
> > > @@ -302,6 +340,61 @@ static int hp_wmi_hw_state(int mask)
> > >   	return !!(state & mask);
> > >   }
> > > 
> > > +static int omen_thermal_profile_set(int mode)
> > > +{
> > > +	char buffer[2] = {0, mode};
> > > +	int ret;
> > > +
> > > +	if (mode < 0 || mode > 2)
> > > +		return -EINVAL;
> > > +
> > > +	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
> > > +				   &buffer, sizeof(buffer), sizeof(buffer));
> > > +
> > > +	if (ret)
> > > +		return ret < 0 ? ret : -EINVAL;
> > > +
> > > +	return mode;
> > > +}
> > > +
> > > +static int hp_wmi_fan_speed_max_set(int enabled)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_SET_QUERY, HPWMI_GM,
> > > +				   &enabled, sizeof(enabled), sizeof(enabled));
> > > +
> > > +	if (ret)
> > > +		return ret < 0 ? ret : -EINVAL;
> > > +
> > > +	return enabled;
> > > +}
> > > +
> > > +static int hp_wmi_fan_speed_max_get(void)
> > > +{
> > > +	int val = 0, ret;
> > > +
> > > +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
> > > +				   &val, sizeof(val), sizeof(val));
> > > +
> > > +	if (ret)
> > > +		return ret < 0 ? ret : -EINVAL;
> > > +
> > > +	return val;
> > > +}
> > > +
> > > +static int omen_thermal_profile_get(void)
> > > +{
> > > +	u8 data;
> > > +
> > > +	int ret = ec_read(HP_OMEN_EC_THERMAL_PROFILE_OFFSET, &data);
> > > +
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return data;
> > > +}
> > > +
> > >   static int __init hp_wmi_bios_2008_later(void)
> > >   {
> > >   	int state = 0;
> > > @@ -473,6 +566,42 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
> > >   	return sprintf(buf, "0x%x\n", value);
> > >   }
> > > 
> > > +static ssize_t max_fan_show(struct device *dev, struct device_attribute *attr,
> > > +			    char *buf)
> > > +{
> > > +	int value = hp_wmi_fan_speed_max_get();
> > > +
> > > +	if (value < 0)
> > > +		return value;
> > > +	return sprintf(buf, "%d\n", value);
> > > +}
> > > +
> > > +static ssize_t fan1_show(struct device *dev, struct device_attribute *attr,
> > > +			char *buf)
> > > +{
> > > +	int fan_speed;
> > > +
> > > +	int ret = hp_wmi_get_fan_speed(0, &fan_speed);
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return sprintf(buf, "%d\n", fan_speed);
> > > +}
> > > +
> > > +static ssize_t fan2_show(struct device *dev, struct device_attribute *attr,
> > > +			char *buf)
> > > +{
> > > +	int fan_speed;
> > > +
> > > +	int ret = hp_wmi_get_fan_speed(1, &fan_speed);
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return sprintf(buf, "%d\n", fan_speed);
> > > +}
> > > +
> > >   static ssize_t als_store(struct device *dev, struct device_attribute *attr,
> > >   			 const char *buf, size_t count)
> > >   {
> > > @@ -514,12 +643,33 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
> > >   	return count;
> > >   }
> > > 
> > > +static ssize_t max_fan_store(struct device *dev, struct device_attribute *attr,
> > > +			     const char *buf, size_t count)
> > > +{
> > > +	u32 tmp;
> > > +	int ret;
> > > +
> > > +	ret = kstrtou32(buf, 10, &tmp);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = hp_wmi_fan_speed_max_set(tmp);
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return count;
> > > +}
> > > +
> > >   static DEVICE_ATTR_RO(display);
> > >   static DEVICE_ATTR_RO(hddtemp);
> > >   static DEVICE_ATTR_RW(als);
> > >   static DEVICE_ATTR_RO(dock);
> > >   static DEVICE_ATTR_RO(tablet);
> > >   static DEVICE_ATTR_RW(postcode);
> > > +static DEVICE_ATTR_RW(max_fan);
> > > +static DEVICE_ATTR_RO(fan1);
> > > +static DEVICE_ATTR_RO(fan2);
> > > 
> > >   static struct attribute *hp_wmi_attrs[] = {
> > >   	&dev_attr_display.attr,
> > > @@ -528,6 +678,7 @@ static struct attribute *hp_wmi_attrs[] = {
> > >   	&dev_attr_dock.attr,
> > >   	&dev_attr_tablet.attr,
> > >   	&dev_attr_postcode.attr,
> > > +	&dev_attr_max_fan.attr,
> > >   	NULL,
> > >   };
> > >   ATTRIBUTE_GROUPS(hp_wmi);
> > > @@ -878,6 +1029,58 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
> > >   	return err;
> > >   }
> > > 
> > > +static int platform_profile_omen_get(struct platform_profile_handler *pprof,
> > > +				enum platform_profile_option *profile)
> > > +{
> > > +	int tp;
> > > +
> > > +	tp = omen_thermal_profile_get();
> > > +	if (tp < 0)
> > > +		return tp;
> > > +
> > > +	switch (tp) {
> > > +	case HP_OMEN_THERMAL_PROFILE_PERFORMANCE:
> > > +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> > > +		break;
> > > +	case HP_OMEN_THERMAL_PROFILE_DEFAULT:
> > > +		*profile = PLATFORM_PROFILE_BALANCED;
> > > +		break;
> > > +	case HP_OMEN_THERMAL_PROFILE_COOL:
> > > +		*profile = PLATFORM_PROFILE_COOL;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int platform_profile_omen_set(struct platform_profile_handler *pprof,
> > > +				enum platform_profile_option profile)
> > > +{
> > > +	int err, tp;
> > > +
> > > +	switch (profile) {
> > > +	case PLATFORM_PROFILE_PERFORMANCE:
> > > +		tp = HP_OMEN_THERMAL_PROFILE_PERFORMANCE;
> > > +		break;
> > > +	case PLATFORM_PROFILE_BALANCED:
> > > +		tp = HP_OMEN_THERMAL_PROFILE_DEFAULT;
> > > +		break;
> > > +	case PLATFORM_PROFILE_COOL:
> > > +		tp = HP_OMEN_THERMAL_PROFILE_COOL;
> > > +		break;
> > > +	default:
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > > +	err = omen_thermal_profile_set(tp);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int thermal_profile_get(void)
> > >   {
> > >   	return hp_wmi_read_int(HPWMI_THERMAL_PROFILE_QUERY);
> > > @@ -946,19 +1149,34 @@ static int thermal_profile_setup(void)
> > >   	int err, tp;
> > > 
> > >   	tp = thermal_profile_get();
> > > -	if (tp < 0)
> > > -		return tp;
> > > +	if (tp >= 0) {
> > > +		/*
> > > +		* call thermal profile write command to ensure that the firmware correctly
> > > +		* sets the OEM variables for the DPTF
> > > +		*/
> > > +		err = thermal_profile_set(tp);
> > > +		if (err)
> > > +			return err;
> > > 
> > > -	/*
> > > -	 * call thermal profile write command to ensure that the firmware correctly
> > > -	 * sets the OEM variables for the DPTF
> > > -	 */
> > > -	err = thermal_profile_set(tp);
> > > -	if (err)
> > > -		return err;
> > > +		platform_profile_handler.profile_get = platform_profile_get;
> > > +		platform_profile_handler.profile_set = platform_profile_set;
> > > +	}
> > > +
> > > +	tp = omen_thermal_profile_get();
> > > +	if (tp >= 0) {
> > > +		/*
> > > +		* call thermal profile write command to ensure that the firmware correctly
> > > +		* sets the OEM variables for the DPTF
> > > +		*/
> > > +		err = omen_thermal_profile_set(tp);
> > > +		if (err < 0)
> > > +			return err;
> > > 
> > > -	platform_profile_handler.profile_get = platform_profile_get,
> > > -	platform_profile_handler.profile_set = platform_profile_set,
> > > +		platform_profile_handler.profile_get = platform_profile_omen_get;
> > > +		platform_profile_handler.profile_set = platform_profile_omen_set;
> > > +	} else {
> > > +		return tp;
> > > +	}
> > > 
> > >   	set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
> > >   	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
> > > @@ -973,6 +1191,8 @@ static int thermal_profile_setup(void)
> > >   	return 0;
> > >   }
> > > 
> > > +static int hp_wmi_hwmon_init(void);
> > > +
> > >   static int __init hp_wmi_bios_setup(struct platform_device *device)
> > >   {
> > >   	/* clear detected rfkill devices */
> > > @@ -984,6 +1204,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
> > >   	if (hp_wmi_rfkill_setup(device))
> > >   		hp_wmi_rfkill2_setup(device);
> > > 
> > > +	hp_wmi_hwmon_init();
> > > +
> > >   	thermal_profile_setup();
> > > 
> > >   	return 0;
> > > @@ -1068,6 +1290,54 @@ static struct platform_driver hp_wmi_driver = {
> > >   	.remove = __exit_p(hp_wmi_bios_remove),
> > >   };
> > > 
> > > +static struct attribute *hwmon_attributes[] = {
> > > +	&dev_attr_fan1.attr,
> > > +	&dev_attr_fan2.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static umode_t hp_wmi_hwmon_sysfs_is_visible(struct kobject *kobj,
> > > +					  struct attribute *attr, int idx)
> > > +{
> > > +	int fan_speed;
> > > +
> > > +	if (attr == &dev_attr_fan1.attr) {
> > > +		int ret = hp_wmi_get_fan_speed(0, &fan_speed);
> > > +
> > > +		if (ret < 0)
> > > +			return 0;
> > > +	} else if (attr == &dev_attr_fan2.attr) {
> > > +		int ret = hp_wmi_get_fan_speed(1, &fan_speed);
> > > +
> > > +		if (ret < 0)
> > > +			return 0;
> > > +	}
> > > +
> > > +	return attr->mode;
> > > +}
> > > +
> > > +static const struct attribute_group hwmon_attribute_group = {
> > > +	.is_visible = hp_wmi_hwmon_sysfs_is_visible,
> > > +	.attrs = hwmon_attributes
> > > +};
> > > +__ATTRIBUTE_GROUPS(hwmon_attribute);
> > > +
> > > +static int hp_wmi_hwmon_init(void)
> > > +{
> > > +	struct device *dev = &hp_wmi_platform_dev->dev;
> > > +	struct device *hwmon;
> > > +
> > > +	hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
> > > +			hwmon_attribute_groups);
> > 
> > I think you should use
> > 
> >    [devm_]hwmon_device_register_with_info()
> > 
> > as it creates sysfs attributes for you, etc. You wouldn't need to manually
> > create device attributes, and you wouldn't need fan{1,2}_show() at all.
> > 
> 
> Correct. Also, the code as written doesn't really make sense as hwmon driver
> because all its attributes are non-standard. The "sensors" command will show
> nothing.
> 

I failed to take a look at the documentation instead i just looked at a
different laptop's WMI driver as a reference. My bad.

> If you don't want to use standard hwmon ABI attributes, please don't register
> a hwmon driver. The non-standard attributes can reside in the platform
> sysfs directory. There is already the non-standard "hddtemp", so that would
> not make much of a difference anyway. On top of that, "max_fan" is added as
> non-hwmon attribute, making the hwmon registration even more pointless.
> 
> Guenter

I would like to expose these fan speeds properly, and hwmon seemed like
the way to do it. I'll redo this part to match how it should be done in the
documentation. 

The questions I have are:

The only value I have is the current fan speed in RPM, I don't have
the rest of the values like min,max,pulses,target. Is it ok to implement this
in hwmon if I don't have those values? Can I use default values in place of
the ones I don't have, or should i omit those fields entirely.
I can assume min speed to be 0 since it does turn off the fans at light load
but in the case of the max speed property, I don't know what it is. Do I omit 
that field entirely or ?

The "max_fan" that I added is not a RPM value, it's a simple toggle in WMI
that either sets the fans to blow at their max speed (1), or it sets them 
to auto (0), this matches what the windows omen command center does. 
I don't know of a way to expose this "properly" so i just added a simple 
attribute. Is there a proper way to expose this behaviour ?

Thanks,
Enver

> 
> > 
> > > +
> > > +	if (IS_ERR(hwmon)) {
> > > +		pr_err("Could not register hp hwmon device\n");
> > > +		return PTR_ERR(hwmon);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int __init hp_wmi_init(void)
> > >   {
> > >   	int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
> > > --
> > > 2.33.0
> > 
> > 
> > Best regards,
> > Barnabás Pőcze
> > 
> 

Re: [PATCH] platform/x86: hp-wmi: add support for omen laptops

[Guenter Roeck]

On 8/22/21 11:10 AM, Enver Balalic wrote:
> On Sun, Aug 22, 2021 at 10:21:15AM -0700, Guenter Roeck wrote:
>> On 8/22/21 10:04 AM, Barnabás Pőcze wrote:
>>> Hi
>>>
>>>
>>> +CC Guenter Roeck and Jean Delvare for hwmon parts
>>>
>>>
>>> 2021. augusztus 22., vasárnap 18:31 keltezéssel, Enver Balalic írta:
>>>> This patch adds support for HP Omen laptops.
>>>> It adds support for most things that can be controlled via the
>>>> Windows Omen Command Center application.
>>>>
>>>>    - Fan speed monitoring through hwmon
>>>>    - Platform Profile support (cool, balanced, performance)
>>>>    - Max fan speed function toggle
>>>>
>>>> This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.
>>>>
>>>> Signed-off-by: Enver Balalic <balalic.enver@gmail.com>
>>>> ---
>>>>    drivers/platform/x86/Kconfig  |   1 +
>>>>    drivers/platform/x86/hp-wmi.c | 292 ++++++++++++++++++++++++++++++++--
>>>>    2 files changed, 282 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>> index d12db6c316ea..f0b3d94e182b 100644
>>>> --- a/drivers/platform/x86/Kconfig
>>>> +++ b/drivers/platform/x86/Kconfig
>>>> @@ -431,6 +431,7 @@ config HP_WMI
>>>>    	tristate "HP WMI extras"
>>>>    	depends on ACPI_WMI
>>>>    	depends on INPUT
>>>> +	depends on HWMON
>>>>    	depends on RFKILL || RFKILL = n
>>>>    	select INPUT_SPARSEKMAP
>>>>    	select ACPI_PLATFORM_PROFILE
>>>> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
>>>> index 027a1467d009..39d26602376d 100644
>>>> --- a/drivers/platform/x86/hp-wmi.c
>>>> +++ b/drivers/platform/x86/hp-wmi.c
>>>> @@ -22,6 +22,7 @@
>>>>    #include <linux/input/sparse-keymap.h>
>>>>    #include <linux/platform_device.h>
>>>>    #include <linux/platform_profile.h>
>>>> +#include <linux/hwmon.h>
>>>>    #include <linux/acpi.h>
>>>>    #include <linux/rfkill.h>
>>>>    #include <linux/string.h>
>>>> @@ -39,6 +40,7 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
>>>>
>>>>    #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
>>>>    #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
>>>> +#define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>>>>
>>>>    enum hp_wmi_radio {
>>>>    	HPWMI_WIFI	= 0x0,
>>>> @@ -89,10 +91,18 @@ enum hp_wmi_commandtype {
>>>>    	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
>>>>    };
>>>>
>>>> +enum hp_wmi_gm_commandtype {
>>>> +	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
>>>> +	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
>>>> +	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
>>>> +	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
>>>> +};
>>>> +
>>>>    enum hp_wmi_command {
>>>>    	HPWMI_READ	= 0x01,
>>>>    	HPWMI_WRITE	= 0x02,
>>>>    	HPWMI_ODM	= 0x03,
>>>> +	HPWMI_GM    = 0x20008,
>>>>    };
>>>>
>>>>    enum hp_wmi_hardware_mask {
>>>> @@ -120,6 +130,13 @@ enum hp_wireless2_bits {
>>>>    	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
>>>>    };
>>>>
>>>> +
>>>> +enum hp_thermal_profile_omen {
>>>> +	HP_OMEN_THERMAL_PROFILE_DEFAULT     = 0x00,
>>>> +	HP_OMEN_THERMAL_PROFILE_PERFORMANCE = 0x01,
>>>> +	HP_OMEN_THERMAL_PROFILE_COOL        = 0x02,
>>>> +};
>>>> +
>>>>    enum hp_thermal_profile {
>>>>    	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
>>>>    	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
>>>> @@ -279,6 +296,27 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>>>    	return ret;
>>>>    }
>>>>
>>>> +static int hp_wmi_get_fan_speed(int fan, int *data)
>>>> +{
>>>> +	int fsh, fsl;
>>>> +	char fan_data[4] = { fan, 0, 0, 0 };
>>>> +
>>>> +	int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
>>>> +				       &fan_data, sizeof(fan_data),
>>>> +				       sizeof(fan_data));
>>>> +
>>>> +	if (ret != 0)
>>>> +		return -EINVAL;
>>>> +
>>>> +	fsh = fan_data[2];
>>>> +	fsl = fan_data[3];
>>>> +
>>>> +	// sometimes one of these can be negative
>>>> +	*data = ((fsh > 0 ? fsh : 0) << 8) | (fsl > 0 ? fsl : 0);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int hp_wmi_read_int(int query)
>>>>    {
>>>>    	int val = 0, ret;
>>>> @@ -302,6 +340,61 @@ static int hp_wmi_hw_state(int mask)
>>>>    	return !!(state & mask);
>>>>    }
>>>>
>>>> +static int omen_thermal_profile_set(int mode)
>>>> +{
>>>> +	char buffer[2] = {0, mode};
>>>> +	int ret;
>>>> +
>>>> +	if (mode < 0 || mode > 2)
>>>> +		return -EINVAL;
>>>> +
>>>> +	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
>>>> +				   &buffer, sizeof(buffer), sizeof(buffer));
>>>> +
>>>> +	if (ret)
>>>> +		return ret < 0 ? ret : -EINVAL;
>>>> +
>>>> +	return mode;
>>>> +}
>>>> +
>>>> +static int hp_wmi_fan_speed_max_set(int enabled)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_SET_QUERY, HPWMI_GM,
>>>> +				   &enabled, sizeof(enabled), sizeof(enabled));
>>>> +
>>>> +	if (ret)
>>>> +		return ret < 0 ? ret : -EINVAL;
>>>> +
>>>> +	return enabled;
>>>> +}
>>>> +
>>>> +static int hp_wmi_fan_speed_max_get(void)
>>>> +{
>>>> +	int val = 0, ret;
>>>> +
>>>> +	ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
>>>> +				   &val, sizeof(val), sizeof(val));
>>>> +
>>>> +	if (ret)
>>>> +		return ret < 0 ? ret : -EINVAL;
>>>> +
>>>> +	return val;
>>>> +}
>>>> +
>>>> +static int omen_thermal_profile_get(void)
>>>> +{
>>>> +	u8 data;
>>>> +
>>>> +	int ret = ec_read(HP_OMEN_EC_THERMAL_PROFILE_OFFSET, &data);
>>>> +
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return data;
>>>> +}
>>>> +
>>>>    static int __init hp_wmi_bios_2008_later(void)
>>>>    {
>>>>    	int state = 0;
>>>> @@ -473,6 +566,42 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
>>>>    	return sprintf(buf, "0x%x\n", value);
>>>>    }
>>>>
>>>> +static ssize_t max_fan_show(struct device *dev, struct device_attribute *attr,
>>>> +			    char *buf)
>>>> +{
>>>> +	int value = hp_wmi_fan_speed_max_get();
>>>> +
>>>> +	if (value < 0)
>>>> +		return value;
>>>> +	return sprintf(buf, "%d\n", value);
>>>> +}
>>>> +
>>>> +static ssize_t fan1_show(struct device *dev, struct device_attribute *attr,
>>>> +			char *buf)
>>>> +{
>>>> +	int fan_speed;
>>>> +
>>>> +	int ret = hp_wmi_get_fan_speed(0, &fan_speed);
>>>> +
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	return sprintf(buf, "%d\n", fan_speed);
>>>> +}
>>>> +
>>>> +static ssize_t fan2_show(struct device *dev, struct device_attribute *attr,
>>>> +			char *buf)
>>>> +{
>>>> +	int fan_speed;
>>>> +
>>>> +	int ret = hp_wmi_get_fan_speed(1, &fan_speed);
>>>> +
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	return sprintf(buf, "%d\n", fan_speed);
>>>> +}
>>>> +
>>>>    static ssize_t als_store(struct device *dev, struct device_attribute *attr,
>>>>    			 const char *buf, size_t count)
>>>>    {
>>>> @@ -514,12 +643,33 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>>>>    	return count;
>>>>    }
>>>>
>>>> +static ssize_t max_fan_store(struct device *dev, struct device_attribute *attr,
>>>> +			     const char *buf, size_t count)
>>>> +{
>>>> +	u32 tmp;
>>>> +	int ret;
>>>> +
>>>> +	ret = kstrtou32(buf, 10, &tmp);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = hp_wmi_fan_speed_max_set(tmp);
>>>> +
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	return count;
>>>> +}
>>>> +
>>>>    static DEVICE_ATTR_RO(display);
>>>>    static DEVICE_ATTR_RO(hddtemp);
>>>>    static DEVICE_ATTR_RW(als);
>>>>    static DEVICE_ATTR_RO(dock);
>>>>    static DEVICE_ATTR_RO(tablet);
>>>>    static DEVICE_ATTR_RW(postcode);
>>>> +static DEVICE_ATTR_RW(max_fan);
>>>> +static DEVICE_ATTR_RO(fan1);
>>>> +static DEVICE_ATTR_RO(fan2);
>>>>
>>>>    static struct attribute *hp_wmi_attrs[] = {
>>>>    	&dev_attr_display.attr,
>>>> @@ -528,6 +678,7 @@ static struct attribute *hp_wmi_attrs[] = {
>>>>    	&dev_attr_dock.attr,
>>>>    	&dev_attr_tablet.attr,
>>>>    	&dev_attr_postcode.attr,
>>>> +	&dev_attr_max_fan.attr,
>>>>    	NULL,
>>>>    };
>>>>    ATTRIBUTE_GROUPS(hp_wmi);
>>>> @@ -878,6 +1029,58 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>>>>    	return err;
>>>>    }
>>>>
>>>> +static int platform_profile_omen_get(struct platform_profile_handler *pprof,
>>>> +				enum platform_profile_option *profile)
>>>> +{
>>>> +	int tp;
>>>> +
>>>> +	tp = omen_thermal_profile_get();
>>>> +	if (tp < 0)
>>>> +		return tp;
>>>> +
>>>> +	switch (tp) {
>>>> +	case HP_OMEN_THERMAL_PROFILE_PERFORMANCE:
>>>> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
>>>> +		break;
>>>> +	case HP_OMEN_THERMAL_PROFILE_DEFAULT:
>>>> +		*profile = PLATFORM_PROFILE_BALANCED;
>>>> +		break;
>>>> +	case HP_OMEN_THERMAL_PROFILE_COOL:
>>>> +		*profile = PLATFORM_PROFILE_COOL;
>>>> +		break;
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int platform_profile_omen_set(struct platform_profile_handler *pprof,
>>>> +				enum platform_profile_option profile)
>>>> +{
>>>> +	int err, tp;
>>>> +
>>>> +	switch (profile) {
>>>> +	case PLATFORM_PROFILE_PERFORMANCE:
>>>> +		tp = HP_OMEN_THERMAL_PROFILE_PERFORMANCE;
>>>> +		break;
>>>> +	case PLATFORM_PROFILE_BALANCED:
>>>> +		tp = HP_OMEN_THERMAL_PROFILE_DEFAULT;
>>>> +		break;
>>>> +	case PLATFORM_PROFILE_COOL:
>>>> +		tp = HP_OMEN_THERMAL_PROFILE_COOL;
>>>> +		break;
>>>> +	default:
>>>> +		return -EOPNOTSUPP;
>>>> +	}
>>>> +
>>>> +	err = omen_thermal_profile_set(tp);
>>>> +	if (err < 0)
>>>> +		return err;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int thermal_profile_get(void)
>>>>    {
>>>>    	return hp_wmi_read_int(HPWMI_THERMAL_PROFILE_QUERY);
>>>> @@ -946,19 +1149,34 @@ static int thermal_profile_setup(void)
>>>>    	int err, tp;
>>>>
>>>>    	tp = thermal_profile_get();
>>>> -	if (tp < 0)
>>>> -		return tp;
>>>> +	if (tp >= 0) {
>>>> +		/*
>>>> +		* call thermal profile write command to ensure that the firmware correctly
>>>> +		* sets the OEM variables for the DPTF
>>>> +		*/
>>>> +		err = thermal_profile_set(tp);
>>>> +		if (err)
>>>> +			return err;
>>>>
>>>> -	/*
>>>> -	 * call thermal profile write command to ensure that the firmware correctly
>>>> -	 * sets the OEM variables for the DPTF
>>>> -	 */
>>>> -	err = thermal_profile_set(tp);
>>>> -	if (err)
>>>> -		return err;
>>>> +		platform_profile_handler.profile_get = platform_profile_get;
>>>> +		platform_profile_handler.profile_set = platform_profile_set;
>>>> +	}
>>>> +
>>>> +	tp = omen_thermal_profile_get();
>>>> +	if (tp >= 0) {
>>>> +		/*
>>>> +		* call thermal profile write command to ensure that the firmware correctly
>>>> +		* sets the OEM variables for the DPTF
>>>> +		*/
>>>> +		err = omen_thermal_profile_set(tp);
>>>> +		if (err < 0)
>>>> +			return err;
>>>>
>>>> -	platform_profile_handler.profile_get = platform_profile_get,
>>>> -	platform_profile_handler.profile_set = platform_profile_set,
>>>> +		platform_profile_handler.profile_get = platform_profile_omen_get;
>>>> +		platform_profile_handler.profile_set = platform_profile_omen_set;
>>>> +	} else {
>>>> +		return tp;
>>>> +	}
>>>>
>>>>    	set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
>>>>    	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
>>>> @@ -973,6 +1191,8 @@ static int thermal_profile_setup(void)
>>>>    	return 0;
>>>>    }
>>>>
>>>> +static int hp_wmi_hwmon_init(void);
>>>> +
>>>>    static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>    {
>>>>    	/* clear detected rfkill devices */
>>>> @@ -984,6 +1204,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>    	if (hp_wmi_rfkill_setup(device))
>>>>    		hp_wmi_rfkill2_setup(device);
>>>>
>>>> +	hp_wmi_hwmon_init();
>>>> +
>>>>    	thermal_profile_setup();
>>>>
>>>>    	return 0;
>>>> @@ -1068,6 +1290,54 @@ static struct platform_driver hp_wmi_driver = {
>>>>    	.remove = __exit_p(hp_wmi_bios_remove),
>>>>    };
>>>>
>>>> +static struct attribute *hwmon_attributes[] = {
>>>> +	&dev_attr_fan1.attr,
>>>> +	&dev_attr_fan2.attr,
>>>> +	NULL
>>>> +};
>>>> +
>>>> +static umode_t hp_wmi_hwmon_sysfs_is_visible(struct kobject *kobj,
>>>> +					  struct attribute *attr, int idx)
>>>> +{
>>>> +	int fan_speed;
>>>> +
>>>> +	if (attr == &dev_attr_fan1.attr) {
>>>> +		int ret = hp_wmi_get_fan_speed(0, &fan_speed);
>>>> +
>>>> +		if (ret < 0)
>>>> +			return 0;
>>>> +	} else if (attr == &dev_attr_fan2.attr) {
>>>> +		int ret = hp_wmi_get_fan_speed(1, &fan_speed);
>>>> +
>>>> +		if (ret < 0)
>>>> +			return 0;
>>>> +	}
>>>> +
>>>> +	return attr->mode;
>>>> +}
>>>> +
>>>> +static const struct attribute_group hwmon_attribute_group = {
>>>> +	.is_visible = hp_wmi_hwmon_sysfs_is_visible,
>>>> +	.attrs = hwmon_attributes
>>>> +};
>>>> +__ATTRIBUTE_GROUPS(hwmon_attribute);
>>>> +
>>>> +static int hp_wmi_hwmon_init(void)
>>>> +{
>>>> +	struct device *dev = &hp_wmi_platform_dev->dev;
>>>> +	struct device *hwmon;
>>>> +
>>>> +	hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
>>>> +			hwmon_attribute_groups);
>>>
>>> I think you should use
>>>
>>>     [devm_]hwmon_device_register_with_info()
>>>
>>> as it creates sysfs attributes for you, etc. You wouldn't need to manually
>>> create device attributes, and you wouldn't need fan{1,2}_show() at all.
>>>
>>
>> Correct. Also, the code as written doesn't really make sense as hwmon driver
>> because all its attributes are non-standard. The "sensors" command will show
>> nothing.
>>
> 
> I failed to take a look at the documentation instead i just looked at a
> different laptop's WMI driver as a reference. My bad.
> 
>> If you don't want to use standard hwmon ABI attributes, please don't register
>> a hwmon driver. The non-standard attributes can reside in the platform
>> sysfs directory. There is already the non-standard "hddtemp", so that would
>> not make much of a difference anyway. On top of that, "max_fan" is added as
>> non-hwmon attribute, making the hwmon registration even more pointless.
>>
>> Guenter
> 
> I would like to expose these fan speeds properly, and hwmon seemed like
> the way to do it. I'll redo this part to match how it should be done in the
> documentation.
> 
> The questions I have are:
> 
> The only value I have is the current fan speed in RPM, I don't have
> the rest of the values like min,max,pulses,target. Is it ok to implement this
> in hwmon if I don't have those values? Can I use default values in place of
> the ones I don't have, or should i omit those fields entirely.
> I can assume min speed to be 0 since it does turn off the fans at light load
> but in the case of the max speed property, I don't know what it is. Do I omit
> that field entirely or ?
> 

Documentation/hwmon/sysfs-interface.rst:

"All entries (except name) are optional, and should only be created in a
  given driver if the chip has the feature."

should answer that question.

> The "max_fan" that I added is not a RPM value, it's a simple toggle in WMI
> that either sets the fans to blow at their max speed (1), or it sets them
> to auto (0), this matches what the windows omen command center does.
> I don't know of a way to expose this "properly" so i just added a simple
> attribute. Is there a proper way to expose this behaviour ?
> 

The closest match would be:

`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

                 Check individual chip documentation files for automatic mode
                 details.

                 RW

with values 0 (full speed) and 2 (automatic).

Guenter

Re: [PATCH] platform/x86: hp-wmi: add support for omen laptops

[Barnabás Pőcze]

Hi


2021. augusztus 22., vasárnap 20:10 keltezéssel, Enver Balalic írta:
> [...]
> > > > +static int hp_wmi_hwmon_init(void)
> > > > +{
> > > > +	struct device *dev = &hp_wmi_platform_dev->dev;
> > > > +	struct device *hwmon;
> > > > +
> > > > +	hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
> > > > +			hwmon_attribute_groups);
> > >
> > > I think you should use
> > >
> > >    [devm_]hwmon_device_register_with_info()
> > >
> > > as it creates sysfs attributes for you, etc. You wouldn't need to manually
> > > create device attributes, and you wouldn't need fan{1,2}_show() at all.
> > >
> >
> > Correct. Also, the code as written doesn't really make sense as hwmon driver
> > because all its attributes are non-standard. The "sensors" command will show
> > nothing.
> >
>
> I failed to take a look at the documentation instead i just looked at a
> different laptop's WMI driver as a reference. My bad.
>
> > If you don't want to use standard hwmon ABI attributes, please don't register
> > a hwmon driver. The non-standard attributes can reside in the platform
> > sysfs directory. There is already the non-standard "hddtemp", so that would
> > not make much of a difference anyway. On top of that, "max_fan" is added as
> > non-hwmon attribute, making the hwmon registration even more pointless.
> >
> > Guenter
>
> I would like to expose these fan speeds properly, and hwmon seemed like
> the way to do it. I'll redo this part to match how it should be done in the
> documentation.
>
> The questions I have are:
>
> The only value I have is the current fan speed in RPM, I don't have
> the rest of the values like min,max,pulses,target. Is it ok to implement this
> in hwmon if I don't have those values? Can I use default values in place of
> the ones I don't have, or should i omit those fields entirely.
> I can assume min speed to be 0 since it does turn off the fans at light load
> but in the case of the max speed property, I don't know what it is. Do I omit
> that field entirely or ?
>

I think you can simply omit what you don't have. In your case,
I imagine you'll have something like this:

  static const struct hwmon_channel_info *info[] = {
    HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT),
    HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
    NULL
  };

  static const struct hwmon_ops ops = {
    .is_visible = ...,
    .read       = ...,
    .write      = ...,
  };

  static const struct hwmon_chip_info chip_info = {
    .ops  = &ops,
    .info = info,
  };


> The "max_fan" that I added is not a RPM value, it's a simple toggle in WMI
> that either sets the fans to blow at their max speed (1), or it sets them
> to auto (0), this matches what the windows omen command center does.
> I don't know of a way to expose this "properly" so i just added a simple
> attribute. Is there a proper way to expose this behaviour ?
>

I believe the proper way to expose that is via a pwmN_enable attribute on the
hwmon device, this should be created by the hwmon subsystem when it sees the

  HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),

channel in the info struct.

If both fans are controlled by one switch, I think it's fine to just have a
single pwmN_enable attribute. But Guenter can probably tell you more.

For this attribute the value 2 means automatic, 1 means manual, 0 means full speed
as far as I'm aware (at least this is what thinkpad_acpi does). If manual fan
control is not available, returning -EOPNOTSUPP is probably fine.


> [...]


Best regards,
Barnabás Pőcze

Re: [PATCH] platform/x86: hp-wmi: add support for omen laptops

[Guenter Roeck]

On 8/22/21 11:53 AM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2021. augusztus 22., vasárnap 20:10 keltezéssel, Enver Balalic írta:
>> [...]
>>>>> +static int hp_wmi_hwmon_init(void)
>>>>> +{
>>>>> +	struct device *dev = &hp_wmi_platform_dev->dev;
>>>>> +	struct device *hwmon;
>>>>> +
>>>>> +	hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
>>>>> +			hwmon_attribute_groups);
>>>>
>>>> I think you should use
>>>>
>>>>     [devm_]hwmon_device_register_with_info()
>>>>
>>>> as it creates sysfs attributes for you, etc. You wouldn't need to manually
>>>> create device attributes, and you wouldn't need fan{1,2}_show() at all.
>>>>
>>>
>>> Correct. Also, the code as written doesn't really make sense as hwmon driver
>>> because all its attributes are non-standard. The "sensors" command will show
>>> nothing.
>>>
>>
>> I failed to take a look at the documentation instead i just looked at a
>> different laptop's WMI driver as a reference. My bad.
>>
>>> If you don't want to use standard hwmon ABI attributes, please don't register
>>> a hwmon driver. The non-standard attributes can reside in the platform
>>> sysfs directory. There is already the non-standard "hddtemp", so that would
>>> not make much of a difference anyway. On top of that, "max_fan" is added as
>>> non-hwmon attribute, making the hwmon registration even more pointless.
>>>
>>> Guenter
>>
>> I would like to expose these fan speeds properly, and hwmon seemed like
>> the way to do it. I'll redo this part to match how it should be done in the
>> documentation.
>>
>> The questions I have are:
>>
>> The only value I have is the current fan speed in RPM, I don't have
>> the rest of the values like min,max,pulses,target. Is it ok to implement this
>> in hwmon if I don't have those values? Can I use default values in place of
>> the ones I don't have, or should i omit those fields entirely.
>> I can assume min speed to be 0 since it does turn off the fans at light load
>> but in the case of the max speed property, I don't know what it is. Do I omit
>> that field entirely or ?
>>
> 
> I think you can simply omit what you don't have. In your case,
> I imagine you'll have something like this:
> 
>    static const struct hwmon_channel_info *info[] = {
>      HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT),
>      HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
>      NULL
>    };
> 
>    static const struct hwmon_ops ops = {
>      .is_visible = ...,
>      .read       = ...,
>      .write      = ...,
>    };
> 
>    static const struct hwmon_chip_info chip_info = {
>      .ops  = &ops,
>      .info = info,
>    };
> 
> 
>> The "max_fan" that I added is not a RPM value, it's a simple toggle in WMI
>> that either sets the fans to blow at their max speed (1), or it sets them
>> to auto (0), this matches what the windows omen command center does.
>> I don't know of a way to expose this "properly" so i just added a simple
>> attribute. Is there a proper way to expose this behaviour ?
>>
> 
> I believe the proper way to expose that is via a pwmN_enable attribute on the
> hwmon device, this should be created by the hwmon subsystem when it sees the
> 
>    HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
> 
> channel in the info struct.
> 
> If both fans are controlled by one switch, I think it's fine to just have a
> single pwmN_enable attribute. But Guenter can probably tell you more.
> 
> For this attribute the value 2 means automatic, 1 means manual, 0 means full speed
> as far as I'm aware (at least this is what thinkpad_acpi does). If manual fan
> control is not available, returning -EOPNOTSUPP is probably fine.
> 
That, or -EINVAL.

I would also suggest to create temp1_input for the HD temperature. We can not drop
the existing attribute for backwards compatibility reasons, but it would make sense
to expose the HD temperature as standard attribute (possibly with an associated
_name attribute).

Guenter