* Re: [PATCH 1/2] platform/x86: system76_acpi: Add hwmon driver
[not found] <aec7591c-80de-4f7b-b8f2-95da5ac1847e@www.fastmail.com>
@ 2020-11-24 12:21 ` Hans de Goede
0 siblings, 0 replies; only message in thread
From: Hans de Goede @ 2020-11-24 12:21 UTC (permalink / raw)
To: Jeremy Soller, platform-driver-x86; +Cc: Björn Lindfors
Hi Jeremy,
On 9/3/20 10:47 PM, Jeremy Soller wrote:
> The hwmon driver provides a kernel interface for accessing fan duty
> cycle, fan speed, and temperature data as defined by firmware. Up to
> eight fans and eight temperature sensors are supported. The actual
> number is determined by querying the firmware.
>
> Signed-off-by: Jeremy Soller <jeremy@system76.com>
> Cc: platform-driver-x86@vger.kernel.org
Quick self intro: I have take over drivers/platform/x86
maintainership from Andy; and I'm working my way through
the backlog of old patches in patchwork:
https://patchwork.kernel.org/project/platform-driver-x86/list/
This patch has some initial review remarks from Barnabás Pőcze
which need addressing. Can you please send a v2 of this series
with Barnabás' remarks to patch 1/2 fixed ?
As I mentioned before switching to a sparse-keymap for 2/2
is not really necessary IMHO, but if you want to do it that
is fine.
Regards,
Hans
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 40219bba6801..d2f9c3dcf4b9 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1158,6 +1158,7 @@ config SONYPI_COMPAT
> config SYSTEM76_ACPI
> tristate "System76 ACPI Driver"
> depends on ACPI
> + depends on HWMON
> select NEW_LEDS
> select LEDS_CLASS
> select LEDS_TRIGGERS
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index c14fd22ba196..aef0ea34829d 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,54 @@ 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;
> + else
> + 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;
> + } else {
> + 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 +323,120 @@ 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 S_IRUGO;
> + } else if (type == hwmon_temp) {
> + if (system76_name(data->ntmp, channel))
> + return S_IRUGO;
> + }
> + 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 +513,17 @@ 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;
> }
>
> @@ -362,6 +540,9 @@ static int system76_remove(struct acpi_device *acpi_dev)
>
> devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led);
>
> + kfree(data->nfan);
> + kfree(data->ntmp);
> +
> system76_get(data, "FINI");
>
> return 0;
>
^ permalink raw reply [flat|nested] only message in thread