Messages in this thread | | | Subject | Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support | From | Armin Wolf <> | Date | Thu, 2 Feb 2023 22:12:47 +0100 |
| |
Am 27.01.23 um 17:09 schrieb Armin Wolf:
> Am 27.01.23 um 14:08 schrieb Guenter Roeck: > >> On Thu, Jan 26, 2023 at 08:40:21PM +0100, Armin Wolf wrote: >>> Thanks to bugreport 216655 on bugzilla triggered by the >>> dell-smm-hwmon driver, the contents of the sensor buffers >>> could be almost completely decoded. >>> Add an hwmon interface for exposing the fan and thermal >>> sensor values. The debugfs interface remains in place to >>> aid in reverse-engineering of unknown sensor types >>> and the thermal buffer. >>> >>> Tested-by: Antonín Skala <skala.antonin@gmail.com> >>> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com> >>> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >>> --- >>> drivers/platform/x86/dell/Kconfig | 1 + >>> drivers/platform/x86/dell/dell-wmi-ddv.c | 435 >>> ++++++++++++++++++++++- >>> 2 files changed, 435 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/platform/x86/dell/Kconfig >>> b/drivers/platform/x86/dell/Kconfig >>> index d319de8f2132..21a74b63d9b1 100644 >>> --- a/drivers/platform/x86/dell/Kconfig >>> +++ b/drivers/platform/x86/dell/Kconfig >>> @@ -194,6 +194,7 @@ config DELL_WMI_DDV >>> default m >>> depends on ACPI_BATTERY >>> depends on ACPI_WMI >>> + depends on HWMON >> Not sure if adding such a dependency is a good idea. >> Up to the maintainer to decide. Personally I would prefer >> something like >> depends on HWMON || HWMON=n >> and some conditionals in the code, as it is done with other drivers >> outside the hwmon directory. >> > Good point, i will include this in the next patch revision. > >>> help >>> This option adds support for WMI-based sensors like >>> battery temperature sensors found on some Dell notebooks. >>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c >>> b/drivers/platform/x86/dell/dell-wmi-ddv.c >>> index 9695bf493ea6..5b30bb85199e 100644 >>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c >>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c >>> @@ -13,6 +13,7 @@ >>> #include <linux/dev_printk.h> >>> #include <linux/errno.h> >>> #include <linux/kernel.h> >>> +#include <linux/hwmon.h> >>> #include <linux/kstrtox.h> >>> #include <linux/math.h> >>> #include <linux/module.h> >>> @@ -21,10 +22,13 @@ >>> #include <linux/printk.h> >>> #include <linux/seq_file.h> >>> #include <linux/sysfs.h> >>> +#include <linux/types.h> >>> #include <linux/wmi.h> >>> >>> #include <acpi/battery.h> >>> >>> +#include <asm/unaligned.h> >>> + >>> #define DRIVER_NAME "dell-wmi-ddv" >>> >>> #define DELL_DDV_SUPPORTED_VERSION_MIN 2 >>> @@ -63,6 +67,29 @@ enum dell_ddv_method { >>> DELL_DDV_THERMAL_SENSOR_INFORMATION = 0x22, >>> }; >>> >>> +struct fan_sensor_entry { >>> + u8 type; >>> + __le16 rpm; >>> +} __packed; >>> + >>> +struct thermal_sensor_entry { >>> + u8 type; >>> + s8 now; >>> + s8 min; >>> + s8 max; >>> + u8 unknown; >>> +} __packed; >>> + >>> +struct combined_channel_info { >>> + struct hwmon_channel_info info; >>> + u32 config[]; >>> +}; >>> + >>> +struct combined_chip_info { >>> + struct hwmon_chip_info chip; >>> + const struct hwmon_channel_info *info[]; >>> +}; >>> + >>> struct dell_wmi_ddv_data { >>> struct acpi_battery_hook hook; >>> struct device_attribute temp_attr; >>> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data { >>> struct wmi_device *wdev; >>> }; >>> >>> +static const char * const fan_labels[] = { >>> + "CPU Fan", >>> + "Chassis Motherboard Fan", >>> + "Video Fan", >>> + "Power Supply Fan", >>> + "Chipset Fan", >>> + "Memory Fan", >>> + "PCI Fan", >>> + "HDD Fan", >>> +}; >>> + >>> +static const char * const fan_dock_labels[] = { >>> + "Docking Chassis/Motherboard Fan", >>> + "Docking Video Fan", >>> + "Docking Power Supply Fan", >>> + "Docking Chipset Fan", >>> +}; >>> + >>> static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum >>> dell_ddv_method method, u32 arg, >>> union acpi_object **result, acpi_object_type type) >>> { >>> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct >>> wmi_device *wdev, enum dell_ddv_meth >>> return dell_wmi_ddv_query_type(wdev, method, arg, result, >>> ACPI_TYPE_STRING); >>> } >>> >>> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum >>> dell_ddv_method method, >>> + size_t entry_size, union acpi_object >>> **result, u64 *count) >>> +{ >>> + union acpi_object *obj; >>> + u64 buffer_size; >>> + u8 *buffer; >>> + int ret; >>> + >>> + ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj); >>> + if (ret < 0) >>> + return ret; >>> + >>> + buffer_size = obj->package.elements[0].integer.value; >>> + buffer = obj->package.elements[1].buffer.pointer; >>> + if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != >>> 0xff) { >>> + kfree(obj); >>> + >> Stray empty line >> >>> + return -ENOMSG; >>> + } >>> + >>> + *count = (buffer_size - 1) / entry_size; >>> + *result = obj; >>> + >>> + return 0; >>> +} >>> + >>> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum >>> hwmon_sensor_types type, u32 attr, >>> + int channel) >>> +{ >>> + return 0444; >>> +} >>> + >>> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data >>> *data, u32 attr, int channel, >>> + long *val) >>> +{ >>> + struct fan_sensor_entry *entry; >>> + union acpi_object *obj; >>> + u64 count; >>> + int ret; >>> + >>> + ret = dell_wmi_ddv_query_sensors(data->wdev, >>> DELL_DDV_FAN_SENSOR_INFORMATION, >>> + sizeof(*entry), &obj, &count); >>> + if (ret < 0) >>> + return ret; >>> + >>> + entry = (struct fan_sensor_entry >>> *)obj->package.elements[1].buffer.pointer; >>> + if (count > channel) { >>> + switch (attr) { >>> + case hwmon_fan_input: >>> + *val = get_unaligned_le16(&entry[channel].rpm); >>> + >> Another stray empty line. I see that "empty line before return or break" >> is common. Looks odd to me, and I don't see the point (it confuses >> the code flow for me and lets my brain focus on the empty line instead >> of the code in question), but I guess that is PoV. I won't comment on >> it further and let the maintainer decide. >> >>> + break; >>> + default: >>> + ret = -EOPNOTSUPP; >>> + } >>> + } else { >>> + ret = -ENXIO; >>> + } >> Error handling should come first. >>> + >>> + kfree(obj); >>> + >>> + return ret; >>> +} >>> + >>> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data >>> *data, u32 attr, int channel, >>> + long *val) >>> +{ >>> + struct thermal_sensor_entry *entry; >>> + union acpi_object *obj; >>> + u64 count; >>> + int ret; >>> + >>> + ret = dell_wmi_ddv_query_sensors(data->wdev, >>> DELL_DDV_THERMAL_SENSOR_INFORMATION, >>> + sizeof(*entry), &obj, &count); >>> + if (ret < 0) >>> + return ret; >>> + >>> + entry = (struct thermal_sensor_entry >>> *)obj->package.elements[1].buffer.pointer; >>> + if (count > channel) { >> This is a bit of Joda programming. It is really "channel < count", >> ie the requested channel number is in the range of channels reported >> by the WMI code. PoV, of course, but I find that the above makes the >> code more difficult to read. >>> + switch (attr) { >>> + case hwmon_temp_input: >>> + *val = entry[channel].now * 1000; >>> + >>> + break; >>> + case hwmon_temp_min: >>> + *val = entry[channel].min * 1000; >>> + >>> + break; >>> + case hwmon_temp_max: >>> + *val = entry[channel].max * 1000; >>> + >>> + break; >>> + default: >>> + ret = -EOPNOTSUPP; >> break; missing >> >>> + } >>> + } else { >>> + ret = -ENXIO; >>> + } >> Same as above - error handling should come first. >> >>> + >>> + kfree(obj); >>> + >>> + return ret; >>> +} >>> + >>> +static int dell_wmi_ddv_read(struct device *dev, enum >>> hwmon_sensor_types type, u32 attr, >>> + int channel, long *val) >>> +{ >>> + struct dell_wmi_ddv_data *data = dev_get_drvdata(dev); >>> + >>> + switch (type) { >>> + case hwmon_fan: >>> + return dell_wmi_ddv_fan_read_channel(data, attr, channel, >>> val); >>> + case hwmon_temp: >>> + return dell_wmi_ddv_temp_read_channel(data, attr, channel, >>> val); >>> + default: >>> + break; >>> + } >>> + >>> + return -EOPNOTSUPP; >>> +} >>> + >>> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data >>> *data, int channel, >>> + const char **str) >>> +{ >>> + struct fan_sensor_entry *entry; >>> + union acpi_object *obj; >>> + u64 count; >>> + u8 type; >>> + int ret; >>> + >>> + ret = dell_wmi_ddv_query_sensors(data->wdev, >>> DELL_DDV_FAN_SENSOR_INFORMATION, >>> + sizeof(*entry), &obj, &count); >>> + if (ret < 0) >>> + return ret; >>> + >>> + entry = (struct fan_sensor_entry >>> *)obj->package.elements[1].buffer.pointer; >>> + if (count > channel) { >>> + type = entry[channel].type; >>> + >>> + switch (type) { >>> + case 0x00 ... 0x07: >>> + *str = fan_labels[type]; >>> + >>> + break; >>> + case 0x11 ... 0x14: >>> + *str = fan_dock_labels[type - 0x11]; >>> + >>> + break; >>> + default: >>> + *str = "Unknown Fan"; >> break; missing. >> >>> + } >>> + } else { >>> + ret = -ENXIO; >>> + } >> And again. >> >>> + >>> + kfree(obj); >>> + >>> + return ret; >>> +} >>> + >>> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data >>> *data, int channel, >>> + const char **str) >>> +{ >>> + struct thermal_sensor_entry *entry; >>> + union acpi_object *obj; >>> + u64 count; >>> + int ret; >>> + >>> + ret = dell_wmi_ddv_query_sensors(data->wdev, >>> DELL_DDV_THERMAL_SENSOR_INFORMATION, >>> + sizeof(*entry), &obj, &count); >>> + if (ret < 0) >>> + return ret; >>> + >>> + entry = (struct thermal_sensor_entry >>> *)obj->package.elements[1].buffer.pointer; >>> + if (count > channel) { >>> + switch (entry[channel].type) { >>> + case 0x00: >>> + *str = "CPU"; >>> + >>> + break; >>> + case 0x11: >>> + *str = "Video"; >>> + >>> + break; >>> + case 0x22: >>> + *str = "Memory"; // sometimes called DIMM >> Personally I don't permit C++ style comments in a hwmon driver >> unless _all_ comments are C++ style. Just a remark here. >> >>> + >>> + break; >>> + case 0x33: >>> + *str = "Other"; >>> + >>> + break; >>> + case 0x44: >>> + *str = "Ambient"; // sometimes called SKIN >>> + >>> + break; >>> + case 0x52: >>> + *str = "SODIMM"; >>> + >>> + break; >>> + case 0x55: >>> + *str = "HDD"; >>> + >>> + break; >>> + case 0x62: >>> + *str = "SODIMM 2"; >>> + >>> + break; >>> + case 0x73: >>> + *str = "NB"; >>> + >>> + break; >>> + case 0x83: >>> + *str = "Charger"; >>> + >>> + break; >>> + case 0xbb: >>> + *str = "Memory 3"; >>> + >>> + break; >>> + default: >>> + *str = "Unknown"; >> break; missing >> Ok, I guess this is on purpose. I personally don't permit >> that since it always leaves the question if it was on purpose or not. >> >>> + } >>> + } else { >>> + ret = -ENXIO; >>> + } >>> + >>> + kfree(obj); >>> + >>> + return ret; >>> +} >>> + >>> +static int dell_wmi_ddv_read_string(struct device *dev, enum >>> hwmon_sensor_types type, u32 attr, >>> + int channel, const char **str) >>> +{ >>> + struct dell_wmi_ddv_data *data = dev_get_drvdata(dev); >>> + >>> + switch (type) { >>> + case hwmon_fan: >>> + switch (attr) { >>> + case hwmon_fan_label: >>> + return dell_wmi_ddv_fan_read_string(data, channel, str); >>> + default: >>> + break; >>> + } >>> + break; >>> + case hwmon_temp: >>> + switch (attr) { >>> + case hwmon_temp_label: >>> + return dell_wmi_ddv_temp_read_string(data, channel, str); >>> + default: >>> + break; >>> + } >>> + break; >>> + default: >>> + break; >>> + } >>> + >>> + return -EOPNOTSUPP; >>> +} >>> + >>> +static const struct hwmon_ops dell_wmi_ddv_ops = { >>> + .is_visible = dell_wmi_ddv_is_visible, >>> + .read = dell_wmi_ddv_read, >>> + .read_string = dell_wmi_ddv_read_string, >>> +}; >>> + >>> +static struct hwmon_channel_info >>> *dell_wmi_ddv_channel_create(struct device *dev, u64 count, >>> + enum hwmon_sensor_types type, >>> + u32 config) >>> +{ >>> + struct combined_channel_info *cinfo; >>> + int i; >>> + >>> + cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + >>> 1), GFP_KERNEL); >>> + if (!cinfo) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + cinfo->info.type = type; >>> + cinfo->info.config = cinfo->config; >>> + >>> + for (i = 0; i < count; i++) >>> + cinfo->config[i] = config; >>> + >>> + return &cinfo->info; >>> +} >>> + >>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct >>> wmi_device *wdev, >>> + enum dell_ddv_method method, >>> + size_t entry_size, >>> + enum hwmon_sensor_types type, >>> + u32 config) >>> +{ >>> + union acpi_object *obj; >>> + u64 count; >>> + int ret; >>> + >>> + ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, >>> &obj, &count); >>> + if (ret < 0) >>> + return ERR_PTR(ret); >>> + >>> + kfree(obj); >>> + >>> + if (!count) >>> + return ERR_PTR(-ENODEV); >>> + >>> + return dell_wmi_ddv_channel_create(&wdev->dev, count, type, >>> config); >>> +} >>> + >>> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data) >>> +{ >>> + struct wmi_device *wdev = data->wdev; >>> + struct combined_chip_info *cinfo; >>> + struct device *hdev; >>> + int index = 0; >>> + int ret; >>> + >>> + if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, >>> GFP_KERNEL)) >>> + return -ENOMEM; >>> + >>> + cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), >>> GFP_KERNEL); >>> + if (!cinfo) { >>> + ret = -ENOMEM; >>> + >>> + goto err_release; >>> + } >>> + >>> + cinfo->chip.ops = &dell_wmi_ddv_ops; >>> + cinfo->chip.info = cinfo->info; >>> + >>> + cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, >>> hwmon_chip, >>> + HWMON_C_REGISTER_TZ); >>> + >>> + if (IS_ERR(cinfo->info[index])) { >>> + ret = PTR_ERR(cinfo->info[index]); >>> + >>> + goto err_release; >>> + } >>> + >>> + index++; >>> + >>> + cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, >>> DELL_DDV_FAN_SENSOR_INFORMATION, >>> + sizeof(struct fan_sensor_entry), >>> hwmon_fan, >>> + (HWMON_F_INPUT | HWMON_F_LABEL)); >>> + if (!IS_ERR(cinfo->info[index])) >>> + index++; >>> + >>> + cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, >>> DELL_DDV_THERMAL_SENSOR_INFORMATION, >>> + sizeof(struct thermal_sensor_entry), >>> + hwmon_temp, (HWMON_T_INPUT | >>> HWMON_T_MIN | >>> + HWMON_T_MAX | HWMON_T_LABEL)); >>> + if (!IS_ERR(cinfo->info[index])) >>> + index++; >>> + >>> + if (!index) { >>> + ret = -ENODEV; >>> + >>> + goto err_release; >>> + } >>> + >>> + cinfo->info[index] = NULL; >>> + >>> + hdev = devm_hwmon_device_register_with_info(&wdev->dev, >>> "dell_ddv", data, &cinfo->chip, >>> + NULL); >>> + if (IS_ERR(hdev)) { >>> + ret = PTR_ERR(hdev); >>> + >>> + goto err_release; >>> + } >>> + >>> + devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add); >>> + >>> + return 0; >>> + >>> +err_release: >>> + devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add); >>> + >>> + return ret; >>> +} >>> + >>> static int dell_wmi_ddv_battery_index(struct acpi_device >>> *acpi_dev, u32 *index) >>> { >>> const char *uid_str; >>> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device >>> *wdev, const void *context) >>> >>> dell_wmi_ddv_debugfs_init(wdev); >>> >>> - return dell_wmi_ddv_battery_add(data); >>> + ret = dell_wmi_ddv_hwmon_add(data); >>> + if (ret < 0) >>> + dev_dbg(&wdev->dev, "Unable to register hwmon interface: >>> %d\n", ret); >>> + >>> + ret = dell_wmi_ddv_battery_add(data); >>> + if (ret < 0) >>> + dev_dbg(&wdev->dev, "Unable to register acpi battery hook: >>> %d\n", ret); >>> + >> This used to be an error, but no longer is. Not my call to make >> if this is acceptable, just pointing it out. > > I decided to not treat the battery hook as essential function anymore > because > the battery hook and hwmon functionality are more or less disconnected > from > each other, so having the driver abort probing just because one > functionality > could not be initialized seemed unreasonable to me. > > I already thought about putting the battery hook and hwmon > functionality into > separate drivers, with the main driver registering a MFD device or > something similar. > Because apart from some generic routines, both functions are not > connected in any way. > > Is it acceptable to split the driver for such a thing? > > Armin Wolf > Any thoughts about this? Otherwise i will just use conditionals.
Armin Wolf
>>> + return 0; >>> } >>> >>> static const struct wmi_device_id dell_wmi_ddv_id_table[] = { >>> -- >>> 2.30.2 >>>
| |