Hi
+CC Guenter Roeck for hwmon
2021. október 1., péntek 18:08 keltezéssel, Tim Crawford írta:
> From: Jeremy Soller <jeremy@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Tim Crawford <tcrawford@xxxxxxxxxxxx>
> ---
> 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
