[PATCH v3 0/4] platform/x86: system76_acpi: Sync DKMS module changes

[PATCH v3 0/4] platform/x86: system76_acpi: Sync DKMS module changes

[Tim Crawford]

Sync the in-tree system76_acpi module with changes made to the DKMS
module [1]. This introduces/exposes new functionality implemented in the
open source BIOS and EC firmware for System76 laptops.

v3:
- Made use of DEV_ATTR macro for kb_led_color
- Used sysfs_emit() instead of sprintf() for kb_led_color
v2:
- Used input_set_capability() instead of setting bits directly in patch 2/3
- Used sysfs_emit() instead of sprintf() in patch 3/3
- Made use of device_{add,remove}_groups() in patch 3/3

[1]: https://github.com/pop-os/system76-acpi-dkms

Jeremy Soller (2):
  platform/x86: system76_acpi: Report temperature and fan speed
  platform/x86: system76_acpi: Replace Fn+F2 function for OLED models

Tim Crawford (2):
  platform/x86: system76_acpi: Add battery charging thresholds
  platform/x86: system76_acpi: Use DEV_ATTR macro for kb_led_color

 drivers/platform/x86/system76_acpi.c | 382 +++++++++++++++++++++++++--
 1 file changed, 367 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH 1/4] platform/x86: system76_acpi: Report temperature and fan speed

[Tim Crawford]

From: Jeremy Soller <jeremy@system76.com>

Add a hwmon interface to report CPU/GPU temperature and fan speed.
sensors now reports an ACPI interface with the entries:

system76_acpi-acpi-0
Adapter: ACPI interface
CPU fan:        0 RPM
GPU fan:        0 RPM
CPU temp:     +47.0°C
GPU temp:      +0.0°C

Signed-off-by: Jeremy Soller <jeremy@system76.com>
Signed-off-by: Tim Crawford <tcrawford@system76.com>
---
 drivers/platform/x86/system76_acpi.c | 172 ++++++++++++++++++++++++++-
 1 file changed, 171 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index c14fd22ba196..11f0e42386ba 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -10,6 +10,8 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
@@ -24,6 +26,9 @@ struct system76_data {
 	enum led_brightness kb_brightness;
 	enum led_brightness kb_toggle_brightness;
 	int kb_color;
+	struct device *therm;
+	union acpi_object *nfan;
+	union acpi_object *ntmp;
 };
 
 static const struct acpi_device_id device_ids[] = {
@@ -68,6 +73,55 @@ static int system76_get(struct system76_data *data, char *method)
 		return -1;
 }
 
+// Get a System76 ACPI device value by name with index
+static int system76_get_index(struct system76_data *data, char *method, int index)
+{
+	union acpi_object obj;
+	struct acpi_object_list obj_list;
+	acpi_handle handle;
+	acpi_status status;
+	unsigned long long ret = 0;
+
+	obj.type = ACPI_TYPE_INTEGER;
+	obj.integer.value = index;
+	obj_list.count = 1;
+	obj_list.pointer = &obj;
+
+	handle = acpi_device_handle(data->acpi_dev);
+	status = acpi_evaluate_integer(handle, method, &obj_list, &ret);
+	if (ACPI_SUCCESS(status))
+		return (int)ret;
+	return -1;
+}
+
+// Get a System76 ACPI device object by name
+static int system76_get_object(struct system76_data *data, char *method, union acpi_object **obj)
+{
+	acpi_handle handle;
+	acpi_status status;
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+
+	handle = acpi_device_handle(data->acpi_dev);
+	status = acpi_evaluate_object(handle, method, NULL, &buf);
+	if (ACPI_SUCCESS(status)) {
+		*obj = (union acpi_object *)buf.pointer;
+		return 0;
+	}
+
+	return -1;
+}
+
+// Get a name from a System76 ACPI device object
+static char *system76_name(union acpi_object *obj, int index)
+{
+	if (obj && obj->type == ACPI_TYPE_PACKAGE && index <= obj->package.count) {
+		if (obj->package.elements[index].type == ACPI_TYPE_STRING)
+			return obj->package.elements[index].string.pointer;
+	}
+
+	return NULL;
+}
+
 // Set a System76 ACPI device value by name
 static int system76_set(struct system76_data *data, char *method, int value)
 {
@@ -270,6 +324,112 @@ static void kb_led_hotkey_color(struct system76_data *data)
 	kb_led_notify(data);
 }
 
+static umode_t thermal_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	const struct system76_data *data = drvdata;
+
+	if (type == hwmon_fan || type == hwmon_pwm) {
+		if (system76_name(data->nfan, channel))
+			return 0444;
+	} else if (type == hwmon_temp) {
+		if (system76_name(data->ntmp, channel))
+			return 0444;
+	}
+
+	return 0;
+}
+
+static int thermal_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			int channel, long *val)
+{
+	struct system76_data *data = dev_get_drvdata(dev);
+	int raw;
+
+	if (type == hwmon_fan && attr == hwmon_fan_input) {
+		raw = system76_get_index(data, "GFAN", channel);
+		if (raw >= 0) {
+			*val = (long)((raw >> 8) & 0xFFFF);
+			return 0;
+		}
+	} else if (type == hwmon_pwm && attr == hwmon_pwm_input) {
+		raw = system76_get_index(data, "GFAN", channel);
+		if (raw >= 0) {
+			*val = (long)(raw & 0xFF);
+			return 0;
+		}
+	} else if (type == hwmon_temp && attr == hwmon_temp_input) {
+		raw = system76_get_index(data, "GTMP", channel);
+		if (raw >= 0) {
+			*val = (long)(raw * 1000);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int thermal_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			       int channel, const char **str)
+{
+	struct system76_data *data = dev_get_drvdata(dev);
+
+	if (type == hwmon_fan && attr == hwmon_fan_label) {
+		*str = system76_name(data->nfan, channel);
+		if (*str)
+			return 0;
+	} else if (type == hwmon_temp && attr == hwmon_temp_label) {
+		*str = system76_name(data->ntmp, channel);
+		if (*str)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const struct hwmon_ops thermal_ops = {
+	.is_visible = thermal_is_visible,
+	.read = thermal_read,
+	.read_string = thermal_read_string,
+};
+
+// Allocate up to 8 fans and temperatures
+static const struct hwmon_channel_info *thermal_channel_info[] = {
+	HWMON_CHANNEL_INFO(fan,
+		HWMON_F_INPUT | HWMON_F_LABEL,
+		HWMON_F_INPUT | HWMON_F_LABEL,
+		HWMON_F_INPUT | HWMON_F_LABEL,
+		HWMON_F_INPUT | HWMON_F_LABEL,
+		HWMON_F_INPUT | HWMON_F_LABEL,
+		HWMON_F_INPUT | HWMON_F_LABEL,
+		HWMON_F_INPUT | HWMON_F_LABEL,
+		HWMON_F_INPUT | HWMON_F_LABEL),
+	HWMON_CHANNEL_INFO(pwm,
+		HWMON_PWM_INPUT,
+		HWMON_PWM_INPUT,
+		HWMON_PWM_INPUT,
+		HWMON_PWM_INPUT,
+		HWMON_PWM_INPUT,
+		HWMON_PWM_INPUT,
+		HWMON_PWM_INPUT,
+		HWMON_PWM_INPUT),
+	HWMON_CHANNEL_INFO(temp,
+		HWMON_T_INPUT | HWMON_T_LABEL,
+		HWMON_T_INPUT | HWMON_T_LABEL,
+		HWMON_T_INPUT | HWMON_T_LABEL,
+		HWMON_T_INPUT | HWMON_T_LABEL,
+		HWMON_T_INPUT | HWMON_T_LABEL,
+		HWMON_T_INPUT | HWMON_T_LABEL,
+		HWMON_T_INPUT | HWMON_T_LABEL,
+		HWMON_T_INPUT | HWMON_T_LABEL),
+	NULL
+};
+
+static const struct hwmon_chip_info thermal_chip_info = {
+	.ops = &thermal_ops,
+	.info = thermal_channel_info,
+};
+
 // Handle ACPI notification
 static void system76_notify(struct acpi_device *acpi_dev, u32 event)
 {
@@ -346,6 +506,14 @@ static int system76_add(struct acpi_device *acpi_dev)
 			return err;
 	}
 
+	system76_get_object(data, "NFAN", &data->nfan);
+	system76_get_object(data, "NTMP", &data->ntmp);
+
+	data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev,
+		"system76_acpi", data, &thermal_chip_info, NULL);
+	if (IS_ERR(data->therm))
+		return PTR_ERR(data->therm);
+
 	return 0;
 }
 
@@ -359,9 +527,11 @@ static int system76_remove(struct acpi_device *acpi_dev)
 		device_remove_file(data->kb_led.dev, &kb_led_color_dev_attr);
 
 	devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led);
-
 	devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led);
 
+	kfree(data->nfan);
+	kfree(data->ntmp);
+
 	system76_get(data, "FINI");
 
 	return 0;
-- 
2.31.1

[PATCH 2/4] platform/x86: system76_acpi: Replace Fn+F2 function for OLED models

[Tim Crawford]

From: Jeremy Soller <jeremy@system76.com>

System76 laptops models with OLED displays do not support the default
Fn+F2 behavior of turning the embedded display on and off. Some models
instead introduce a new notify event that is used to lock the screen so
the OS will put the display in a low power state.

Signed-off-by: Jeremy Soller <jeremy@system76.com>
Signed-off-by: Tim Crawford <tcrawford@system76.com>
---
 drivers/platform/x86/system76_acpi.c | 30 ++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 11f0e42386ba..e7c86b543930 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -13,6 +13,7 @@
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/init.h>
+#include <linux/input.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -29,6 +30,7 @@ struct system76_data {
 	struct device *therm;
 	union acpi_object *nfan;
 	union acpi_object *ntmp;
+	struct input_dev *input;
 };
 
 static const struct acpi_device_id device_ids[] = {
@@ -430,6 +432,15 @@ static const struct hwmon_chip_info thermal_chip_info = {
 	.info = thermal_channel_info,
 };
 
+static void input_key(struct system76_data *data, unsigned int code)
+{
+	input_report_key(data->input, code, 1);
+	input_sync(data->input);
+
+	input_report_key(data->input, code, 0);
+	input_sync(data->input);
+}
+
 // Handle ACPI notification
 static void system76_notify(struct acpi_device *acpi_dev, u32 event)
 {
@@ -452,6 +463,9 @@ static void system76_notify(struct acpi_device *acpi_dev, u32 event)
 	case 0x84:
 		kb_led_hotkey_color(data);
 		break;
+	case 0x85:
+		input_key(data, KEY_SCREENLOCK);
+		break;
 	}
 }
 
@@ -514,6 +528,22 @@ static int system76_add(struct acpi_device *acpi_dev)
 	if (IS_ERR(data->therm))
 		return PTR_ERR(data->therm);
 
+	data->input = devm_input_allocate_device(&acpi_dev->dev);
+	if (!data->input)
+		return -ENOMEM;
+
+	data->input->name = "System76 ACPI Hotkeys";
+	data->input->phys = "system76_acpi/input0";
+	data->input->id.bustype = BUS_HOST;
+	data->input->dev.parent = &acpi_dev->dev;
+	input_set_capability(data->input, EV_KEY, KEY_SCREENLOCK);
+
+	err = input_register_device(data->input);
+	if (err) {
+		input_free_device(data->input);
+		return err;
+	}
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH 3/4] platform/x86: system76_acpi: Add battery charging thresholds

[Tim Crawford]

System76 laptops running open source EC firmware support configuring
charging thresholds through ACPI methods. Expose this functionality
through the standard sysfs entries charge_control_{start,end}_threshold.

Signed-off-by: Tim Crawford <tcrawford@system76.com>
---
 drivers/platform/x86/system76_acpi.c | 162 +++++++++++++++++++++++++++
 1 file changed, 162 insertions(+)

diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index e7c86b543930..5c525c242211 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -18,8 +18,12 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/pci_ids.h>
+#include <linux/power_supply.h>
+#include <linux/sysfs.h>
 #include <linux/types.h>
 
+#include <acpi/battery.h>
+
 struct system76_data {
 	struct acpi_device *acpi_dev;
 	struct led_classdev ap_led;
@@ -144,6 +148,159 @@ static int system76_set(struct system76_data *data, char *method, int value)
 		return -1;
 }
 
+#define BATTERY_THRESHOLD_INVALID	0xFF
+
+enum {
+	THRESHOLD_START,
+	THRESHOLD_END,
+};
+
+static ssize_t battery_get_threshold(int which, char *buf)
+{
+	struct acpi_object_list input;
+	union acpi_object param;
+	acpi_handle handle;
+	acpi_status status;
+	unsigned long long ret = BATTERY_THRESHOLD_INVALID;
+
+	handle = ec_get_handle();
+	if (!handle)
+		return -ENODEV;
+
+	input.count = 1;
+	input.pointer = &param;
+	// Start/stop selection
+	param.type = ACPI_TYPE_INTEGER;
+	param.integer.value = which;
+
+	status = acpi_evaluate_integer(handle, "GBCT", &input, &ret);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+	if (ret == BATTERY_THRESHOLD_INVALID)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%d\n", (int)ret);
+}
+
+static ssize_t battery_set_threshold(int which, const char *buf, size_t count)
+{
+	struct acpi_object_list input;
+	union acpi_object params[2];
+	acpi_handle handle;
+	acpi_status status;
+	unsigned int value;
+	int ret;
+
+	handle = ec_get_handle();
+	if (!handle)
+		return -ENODEV;
+
+	ret = kstrtouint(buf, 10, &value);
+	if (ret)
+		return ret;
+
+	if (value > 100)
+		return -EINVAL;
+
+	input.count = 2;
+	input.pointer = params;
+	// Start/stop selection
+	params[0].type = ACPI_TYPE_INTEGER;
+	params[0].integer.value = which;
+	// Threshold value
+	params[1].type = ACPI_TYPE_INTEGER;
+	params[1].integer.value = value;
+
+	status = acpi_evaluate_object(handle, "SBCT", &input, NULL);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t charge_control_start_threshold_show(
+	struct device *dev,
+	struct device_attribute *attr,
+	char *buf)
+{
+	return battery_get_threshold(THRESHOLD_START, buf);
+}
+
+static ssize_t charge_control_start_threshold_store(
+	struct device *dev,
+	struct device_attribute *attr,
+	const char *buf,
+	size_t count)
+{
+	return battery_set_threshold(THRESHOLD_START, buf, count);
+}
+
+static DEVICE_ATTR_RW(charge_control_start_threshold);
+
+static ssize_t charge_control_end_threshold_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	return battery_get_threshold(THRESHOLD_END, buf);
+}
+
+static ssize_t charge_control_end_threshold_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	return battery_set_threshold(THRESHOLD_END, buf, count);
+}
+
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+
+static struct attribute *system76_battery_attrs[] = {
+	&dev_attr_charge_control_start_threshold.attr,
+	&dev_attr_charge_control_end_threshold.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(system76_battery);
+
+static int system76_battery_add(struct power_supply *battery)
+{
+	// System76 EC only supports 1 battery
+	if (strcmp(battery->desc->name, "BAT0") != 0)
+		return -ENODEV;
+
+	if (device_add_groups(&battery->dev, system76_battery_groups))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int system76_battery_remove(struct power_supply *battery)
+{
+	device_remove_groups(&battery->dev, system76_battery_groups);
+	return 0;
+}
+
+static struct acpi_battery_hook system76_battery_hook = {
+	.add_battery = system76_battery_add,
+	.remove_battery = system76_battery_remove,
+	.name = "System76 Battery Extension",
+};
+
+static void system76_battery_init(void)
+{
+	acpi_handle handle;
+
+	handle = ec_get_handle();
+	if (handle && acpi_has_method(handle, "GBCT"))
+		battery_hook_register(&system76_battery_hook);
+}
+
+static void system76_battery_exit(void)
+{
+	acpi_handle handle;
+
+	handle = ec_get_handle();
+	if (handle && acpi_has_method(handle, "GBCT"))
+		battery_hook_unregister(&system76_battery_hook);
+}
+
 // Get the airplane mode LED brightness
 static enum led_brightness ap_led_get(struct led_classdev *led)
 {
@@ -544,6 +701,8 @@ static int system76_add(struct acpi_device *acpi_dev)
 		return err;
 	}
 
+	system76_battery_init();
+
 	return 0;
 }
 
@@ -553,6 +712,9 @@ static int system76_remove(struct acpi_device *acpi_dev)
 	struct system76_data *data;
 
 	data = acpi_driver_data(acpi_dev);
+
+	system76_battery_exit();
+
 	if (data->kb_color >= 0)
 		device_remove_file(data->kb_led.dev, &kb_led_color_dev_attr);
 
-- 
2.31.1

[PATCH 4/4] platform/x86: system76_acpi: Use DEV_ATTR macro for kb_led_color

[Tim Crawford]

Update kb_led_color to use the attr macro instead of manually making the
struct. While touching it, also change its show method to use
sysfs_emit() instead of sprintf().

Signed-off-by: Tim Crawford <tcrawford@system76.com>
---
 drivers/platform/x86/system76_acpi.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 5c525c242211..dd00eb2663d6 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -354,7 +354,7 @@ static ssize_t kb_led_color_show(
 
 	led = (struct led_classdev *)dev->driver_data;
 	data = container_of(led, struct system76_data, kb_led);
-	return sprintf(buf, "%06X\n", data->kb_color);
+	return sysfs_emit(buf, "%06X\n", data->kb_color);
 }
 
 // Set the keyboard LED color
@@ -382,14 +382,7 @@ static ssize_t kb_led_color_store(
 	return size;
 }
 
-static const struct device_attribute kb_led_color_dev_attr = {
-	.attr = {
-		.name = "color",
-		.mode = 0644,
-	},
-	.show = kb_led_color_show,
-	.store = kb_led_color_store,
-};
+static DEVICE_ATTR_RW(kb_led_color);
 
 // Notify that the keyboard LED was changed by hardware
 static void kb_led_notify(struct system76_data *data)
@@ -669,10 +662,7 @@ static int system76_add(struct acpi_device *acpi_dev)
 		return err;
 
 	if (data->kb_color >= 0) {
-		err = device_create_file(
-			data->kb_led.dev,
-			&kb_led_color_dev_attr
-		);
+		err = device_create_file(data->kb_led.dev, &dev_attr_kb_led_color);
 		if (err)
 			return err;
 	}
@@ -716,7 +706,7 @@ static int system76_remove(struct acpi_device *acpi_dev)
 	system76_battery_exit();
 
 	if (data->kb_color >= 0)
-		device_remove_file(data->kb_led.dev, &kb_led_color_dev_attr);
+		device_remove_file(data->kb_led.dev, &dev_attr_kb_led_color);
 
 	devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led);
 	devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led);
-- 
2.31.1

Re: [PATCH 4/4] platform/x86: system76_acpi: Use DEV_ATTR macro for kb_led_color

[Barnabás Pőcze]

Hi


2021. október 1., péntek 18:08 keltezéssel, Tim Crawford írta:

> Update kb_led_color to use the attr macro instead of manually making the
> struct. While touching it, also change its show method to use
> sysfs_emit() instead of sprintf().
>

If you're already touching this part of the code, you should probably create
an attribute group and set the `groups` field of the led_classdev struct
instead of manually adding the attribute.


Regards,
Barnabás Pőcze


> Signed-off-by: Tim Crawford <tcrawford@system76.com>
> ---
>  drivers/platform/x86/system76_acpi.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index 5c525c242211..dd00eb2663d6 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -354,7 +354,7 @@ static ssize_t kb_led_color_show(
>
>  	led = (struct led_classdev *)dev->driver_data;
>  	data = container_of(led, struct system76_data, kb_led);
> -	return sprintf(buf, "%06X\n", data->kb_color);
> +	return sysfs_emit(buf, "%06X\n", data->kb_color);
>  }
>
>  // Set the keyboard LED color
> @@ -382,14 +382,7 @@ static ssize_t kb_led_color_store(
>  	return size;
>  }
>
> -static const struct device_attribute kb_led_color_dev_attr = {
> -	.attr = {
> -		.name = "color",
> -		.mode = 0644,
> -	},
> -	.show = kb_led_color_show,
> -	.store = kb_led_color_store,
> -};
> +static DEVICE_ATTR_RW(kb_led_color);
>
>  // Notify that the keyboard LED was changed by hardware
>  static void kb_led_notify(struct system76_data *data)
> @@ -669,10 +662,7 @@ static int system76_add(struct acpi_device *acpi_dev)
>  		return err;
>
>  	if (data->kb_color >= 0) {
> -		err = device_create_file(
> -			data->kb_led.dev,
> -			&kb_led_color_dev_attr
> -		);
> +		err = device_create_file(data->kb_led.dev, &dev_attr_kb_led_color);
>  		if (err)
>  			return err;
>  	}
> @@ -716,7 +706,7 @@ static int system76_remove(struct acpi_device *acpi_dev)
>  	system76_battery_exit();
>
>  	if (data->kb_color >= 0)
> -		device_remove_file(data->kb_led.dev, &kb_led_color_dev_attr);
> +		device_remove_file(data->kb_led.dev, &dev_attr_kb_led_color);
>
>  	devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led);
>  	devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led);
> --
> 2.31.1

Re: [PATCH 1/4] platform/x86: system76_acpi: Report temperature and fan speed

[Barnabás Pőcze]

Hi


+CC Guenter Roeck for hwmon


2021. október 1., péntek 18:08 keltezéssel, Tim Crawford írta:

> From: Jeremy Soller <jeremy@system76.com>
>
> Add a hwmon interface to report CPU/GPU temperature and fan speed.
> sensors now reports an ACPI interface with the entries:
>
> system76_acpi-acpi-0
> Adapter: ACPI interface
> CPU fan:        0 RPM
> GPU fan:        0 RPM
> CPU temp:     +47.0°C
> GPU temp:      +0.0°C
>
> Signed-off-by: Jeremy Soller <jeremy@system76.com>
> Signed-off-by: Tim Crawford <tcrawford@system76.com>
> ---
>  drivers/platform/x86/system76_acpi.c | 172 ++++++++++++++++++++++++++-
>  1 file changed, 171 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index c14fd22ba196..11f0e42386ba 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -10,6 +10,8 @@
>   */
>
>  #include <linux/acpi.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
> @@ -24,6 +26,9 @@ struct system76_data {
>  	enum led_brightness kb_brightness;
>  	enum led_brightness kb_toggle_brightness;
>  	int kb_color;
> +	struct device *therm;
> +	union acpi_object *nfan;
> +	union acpi_object *ntmp;
>  };
>
>  static const struct acpi_device_id device_ids[] = {
> @@ -68,6 +73,55 @@ static int system76_get(struct system76_data *data, char *method)
>  		return -1;
>  }
>
> +// Get a System76 ACPI device value by name with index
> +static int system76_get_index(struct system76_data *data, char *method, int index)
> +{
> +	union acpi_object obj;
> +	struct acpi_object_list obj_list;
> +	acpi_handle handle;
> +	acpi_status status;
> +	unsigned long long ret = 0;
> +
> +	obj.type = ACPI_TYPE_INTEGER;
> +	obj.integer.value = index;
> +	obj_list.count = 1;
> +	obj_list.pointer = &obj;
> +
> +	handle = acpi_device_handle(data->acpi_dev);
> +	status = acpi_evaluate_integer(handle, method, &obj_list, &ret);
> +	if (ACPI_SUCCESS(status))
> +		return (int)ret;

(The cast is unnecessary.)


> +	return -1;

This should probably be a relevant errno.


> +}
> +
> +// Get a System76 ACPI device object by name
> +static int system76_get_object(struct system76_data *data, char *method, union acpi_object **obj)
> +{
> +	acpi_handle handle;
> +	acpi_status status;
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> +
> +	handle = acpi_device_handle(data->acpi_dev);
> +	status = acpi_evaluate_object(handle, method, NULL, &buf);
> +	if (ACPI_SUCCESS(status)) {
> +		*obj = (union acpi_object *)buf.pointer;

(The cast is unnecessary.)


> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +
> +// Get a name from a System76 ACPI device object
> +static char *system76_name(union acpi_object *obj, int index)
> +{
> +	if (obj && obj->type == ACPI_TYPE_PACKAGE && index <= obj->package.count) {
> +		if (obj->package.elements[index].type == ACPI_TYPE_STRING)
> +			return obj->package.elements[index].string.pointer;
> +	}
> +
> +	return NULL;
> +}
> +
>  // Set a System76 ACPI device value by name
>  static int system76_set(struct system76_data *data, char *method, int value)
>  {
> @@ -270,6 +324,112 @@ static void kb_led_hotkey_color(struct system76_data *data)
>  	kb_led_notify(data);
>  }
>
> +static umode_t thermal_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	const struct system76_data *data = drvdata;
> +
> +	if (type == hwmon_fan || type == hwmon_pwm) {
> +		if (system76_name(data->nfan, channel))
> +			return 0444;
> +	} else if (type == hwmon_temp) {
> +		if (system76_name(data->ntmp, channel))
> +			return 0444;
> +	}
> +
> +	return 0;
> +}
> +
> +static int thermal_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			int channel, long *val)
> +{
> +	struct system76_data *data = dev_get_drvdata(dev);
> +	int raw;
> +
> +	if (type == hwmon_fan && attr == hwmon_fan_input) {
> +		raw = system76_get_index(data, "GFAN", channel);
> +		if (raw >= 0) {
> +			*val = (long)((raw >> 8) & 0xFFFF);
> +			return 0;
> +		}
> +	} else if (type == hwmon_pwm && attr == hwmon_pwm_input) {
> +		raw = system76_get_index(data, "GFAN", channel);
> +		if (raw >= 0) {
> +			*val = (long)(raw & 0xFF);
> +			return 0;
> +		}
> +	} else if (type == hwmon_temp && attr == hwmon_temp_input) {
> +		raw = system76_get_index(data, "GTMP", channel);
> +		if (raw >= 0) {
> +			*val = (long)(raw * 1000);

(The cast is unnecessary.)


> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;

I am not sure if EINVAL is the best error to return in case of any errors here.
Usually hwmon drivers return EOPNOTSUPP when an impossible (type, attr)
tuple is encountered as far as I know, and a relevant errno in case of an error.
I think returning EIO in case of an error would be somewhat better than EINVAL.


> +}
> +
> +static int thermal_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			       int channel, const char **str)
> +{
> +	struct system76_data *data = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_fan && attr == hwmon_fan_label) {
> +		*str = system76_name(data->nfan, channel);
> +		if (*str)
> +			return 0;
> +	} else if (type == hwmon_temp && attr == hwmon_temp_label) {
> +		*str = system76_name(data->ntmp, channel);
> +		if (*str)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct hwmon_ops thermal_ops = {
> +	.is_visible = thermal_is_visible,
> +	.read = thermal_read,
> +	.read_string = thermal_read_string,
> +};
> +
> +// Allocate up to 8 fans and temperatures
> +static const struct hwmon_channel_info *thermal_channel_info[] = {
> +	HWMON_CHANNEL_INFO(fan,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL),
> +	HWMON_CHANNEL_INFO(pwm,
> +		HWMON_PWM_INPUT,
> +		HWMON_PWM_INPUT,
> +		HWMON_PWM_INPUT,
> +		HWMON_PWM_INPUT,
> +		HWMON_PWM_INPUT,
> +		HWMON_PWM_INPUT,
> +		HWMON_PWM_INPUT,
> +		HWMON_PWM_INPUT),
> +	HWMON_CHANNEL_INFO(temp,
> +		HWMON_T_INPUT | HWMON_T_LABEL,
> +		HWMON_T_INPUT | HWMON_T_LABEL,
> +		HWMON_T_INPUT | HWMON_T_LABEL,
> +		HWMON_T_INPUT | HWMON_T_LABEL,
> +		HWMON_T_INPUT | HWMON_T_LABEL,
> +		HWMON_T_INPUT | HWMON_T_LABEL,
> +		HWMON_T_INPUT | HWMON_T_LABEL,
> +		HWMON_T_INPUT | HWMON_T_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info thermal_chip_info = {
> +	.ops = &thermal_ops,
> +	.info = thermal_channel_info,
> +};
> +
>  // Handle ACPI notification
>  static void system76_notify(struct acpi_device *acpi_dev, u32 event)
>  {
> @@ -346,6 +506,14 @@ static int system76_add(struct acpi_device *acpi_dev)
>  			return err;
>  	}
>
> +	system76_get_object(data, "NFAN", &data->nfan);
> +	system76_get_object(data, "NTMP", &data->ntmp);

I believe some error handling would be useful.


> +
> +	data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev,
> +		"system76_acpi", data, &thermal_chip_info, NULL);
> +	if (IS_ERR(data->therm))
> +		return PTR_ERR(data->therm);

This is mostly theoretical, but `data->nfan` and `data->ntmp` are potentially leaked
in the error path. I think using `devm_kmemdup()` _might_ provide a convenient
solution.


> +
>  	return 0;
>  }
>
> @@ -359,9 +527,11 @@ static int system76_remove(struct acpi_device *acpi_dev)
>  		device_remove_file(data->kb_led.dev, &kb_led_color_dev_attr);
>
>  	devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led);
> -
>  	devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led);
>
> +	kfree(data->nfan);
> +	kfree(data->ntmp);
> +
>  	system76_get(data, "FINI");
>
>  	return 0;
> --
> 2.31.1


Regards,
Barnabás Pőcze