lkml.org 
[lkml]   [2023]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/2] platform/x86: dell-ddv: Add hwmon support
    From
    Hi Armin,

    On 2/5/23 21:54, 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. Since the WMI interface can be quite slow
    > on some machines, the sensor buffers are cached for 1 second
    > to lessen the performance impact.
    > 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>

    This looks nice and clean to me, thank you:

    Reviewed-by: Hans de Goede <hdegoede@redhat.com>

    I'm going to wait a bit with merging this to see if Guenter
    has any remarks. If there are no remarks by next Monday then
    I'll merge this for the 6.3 merge window.

    Regards,

    Hans



    > ---
    > drivers/platform/x86/dell/Kconfig | 8 +-
    > drivers/platform/x86/dell/dell-wmi-ddv.c | 492 ++++++++++++++++++++++-
    > 2 files changed, 495 insertions(+), 5 deletions(-)
    >
    > diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
    > index d319de8f2132..bdd78076b1d7 100644
    > --- a/drivers/platform/x86/dell/Kconfig
    > +++ b/drivers/platform/x86/dell/Kconfig
    > @@ -192,12 +192,12 @@ config DELL_WMI_DESCRIPTOR
    > config DELL_WMI_DDV
    > tristate "Dell WMI sensors Support"
    > default m
    > - depends on ACPI_BATTERY
    > depends on ACPI_WMI
    > + depends on ACPI_BATTERY || HWMON
    > help
    > - This option adds support for WMI-based sensors like
    > - battery temperature sensors found on some Dell notebooks.
    > - It also supports reading of the battery ePPID.
    > + This option adds support for WMI-based fan and thermal sensors
    > + found on some Dell notebooks. It also supports various WMI-based battery
    > + extras like reading of the battery temperature and ePPID.
    >
    > To compile this drivers as a module, choose M here: the module will
    > be called dell-wmi-ddv.
    > diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
    > index 9695bf493ea6..b7ac483eff12 100644
    > --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
    > +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
    > @@ -12,19 +12,26 @@
    > #include <linux/device.h>
    > #include <linux/dev_printk.h>
    > #include <linux/errno.h>
    > +#include <linux/kconfig.h>
    > #include <linux/kernel.h>
    > +#include <linux/hwmon.h>
    > #include <linux/kstrtox.h>
    > #include <linux/math.h>
    > +#include <linux/math64.h>
    > #include <linux/module.h>
    > +#include <linux/mutex.h>
    > #include <linux/limits.h>
    > #include <linux/power_supply.h>
    > #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,13 +70,63 @@ 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_sensors {
    > + struct mutex lock; /* protect caching */
    > + unsigned long timestamp;
    > + union acpi_object *obj;
    > + u64 entries;
    > +};
    > +
    > struct dell_wmi_ddv_data {
    > struct acpi_battery_hook hook;
    > struct device_attribute temp_attr;
    > struct device_attribute eppid_attr;
    > + struct dell_wmi_ddv_sensors fans;
    > + struct dell_wmi_ddv_sensors temps;
    > 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 +228,427 @@ 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);
    > }
    >
    > +/*
    > + * Needs to be called with lock held, except during initialization.
    > + */
    > +static int dell_wmi_ddv_update_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
    > + struct dell_wmi_ddv_sensors *sensors, size_t entry_size)
    > +{
    > + u64 buffer_size, rem, entries;
    > + union acpi_object *obj;
    > + u8 *buffer;
    > + int ret;
    > +
    > + if (sensors->obj) {
    > + if (time_before(jiffies, sensors->timestamp + HZ))
    > + return 0;
    > +
    > + kfree(sensors->obj);
    > + sensors->obj = NULL;
    > + }
    > +
    > + ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
    > + if (ret < 0)
    > + return ret;
    > +
    > + /* buffer format sanity check */
    > + buffer_size = obj->package.elements[0].integer.value;
    > + buffer = obj->package.elements[1].buffer.pointer;
    > + entries = div64_u64_rem(buffer_size, entry_size, &rem);
    > + if (rem != 1 || buffer[buffer_size - 1] != 0xff) {
    > + ret = -ENOMSG;
    > +
    > + goto err_free;
    > + }
    > +
    > + if (!entries) {
    > + ret = -ENODATA;
    > +
    > + goto err_free;
    > + }
    > +
    > + sensors->obj = obj;
    > + sensors->entries = entries;
    > + sensors->timestamp = jiffies;
    > +
    > + return 0;
    > +
    > +err_free:
    > + kfree(obj);
    > +
    > + return ret;
    > +}
    > +
    > +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;
    > + int ret;
    > +
    > + ret = dell_wmi_ddv_update_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
    > + &data->fans, sizeof(*entry));
    > + if (ret < 0)
    > + return ret;
    > +
    > + if (channel >= data->fans.entries)
    > + return -ENXIO;
    > +
    > + entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
    > + switch (attr) {
    > + case hwmon_fan_input:
    > + *val = get_unaligned_le16(&entry[channel].rpm);
    > +
    > + return 0;
    > + default:
    > + break;
    > + }
    > +
    > + return -EOPNOTSUPP;
    > +}
    > +
    > +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;
    > + int ret;
    > +
    > + ret = dell_wmi_ddv_update_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
    > + &data->temps, sizeof(*entry));
    > + if (ret < 0)
    > + return ret;
    > +
    > + if (channel >= data->temps.entries)
    > + return -ENXIO;
    > +
    > + entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
    > + switch (attr) {
    > + case hwmon_temp_input:
    > + *val = entry[channel].now * 1000;
    > +
    > + return 0;
    > + case hwmon_temp_min:
    > + *val = entry[channel].min * 1000;
    > +
    > + return 0;
    > + case hwmon_temp_max:
    > + *val = entry[channel].max * 1000;
    > +
    > + return 0;
    > + default:
    > + break;
    > + }
    > +
    > + return -EOPNOTSUPP;
    > +}
    > +
    > +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);
    > + int ret;
    > +
    > + switch (type) {
    > + case hwmon_fan:
    > + mutex_lock(&data->fans.lock);
    > + ret = dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
    > + mutex_unlock(&data->fans.lock);
    > +
    > + return ret;
    > + case hwmon_temp:
    > + mutex_lock(&data->temps.lock);
    > + ret = dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
    > + mutex_unlock(&data->temps.lock);
    > +
    > + return ret;
    > + 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;
    > + int ret;
    > + u8 type;
    > +
    > + ret = dell_wmi_ddv_update_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
    > + &data->fans, sizeof(*entry));
    > + if (ret < 0)
    > + return ret;
    > +
    > + if (channel >= data->fans.entries)
    > + return -ENXIO;
    > +
    > + entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
    > + 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;
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
    > + const char **str)
    > +{
    > + struct thermal_sensor_entry *entry;
    > + int ret;
    > +
    > + ret = dell_wmi_ddv_update_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
    > + &data->temps, sizeof(*entry));
    > + if (ret < 0)
    > + return ret;
    > +
    > + if (channel >= data->temps.entries)
    > + return -ENXIO;
    > +
    > + entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
    > + switch (entry[channel].type) {
    > + case 0x00:
    > + *str = "CPU";
    > +
    > + break;
    > + case 0x11:
    > + *str = "Video";
    > +
    > + break;
    > + case 0x22:
    > + *str = "Memory"; /* sometimes called DIMM */
    > +
    > + 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;
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +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);
    > + int ret;
    > +
    > + switch (type) {
    > + case hwmon_fan:
    > + switch (attr) {
    > + case hwmon_fan_label:
    > + mutex_lock(&data->fans.lock);
    > + ret = dell_wmi_ddv_fan_read_string(data, channel, str);
    > + mutex_unlock(&data->fans.lock);
    > +
    > + return ret;
    > + default:
    > + break;
    > + }
    > + break;
    > + case hwmon_temp:
    > + switch (attr) {
    > + case hwmon_temp_label:
    > + mutex_lock(&data->temps.lock);
    > + ret = dell_wmi_ddv_temp_read_string(data, channel, str);
    > + mutex_unlock(&data->temps.lock);
    > +
    > + return ret;
    > + 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 void dell_wmi_ddv_hwmon_cache_destroy(void *data)
    > +{
    > + struct dell_wmi_ddv_sensors *sensors = data;
    > +
    > + mutex_destroy(&sensors->lock);
    > + kfree(sensors->obj);
    > +}
    > +
    > +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
    > + enum dell_ddv_method method,
    > + struct dell_wmi_ddv_sensors *sensors,
    > + size_t entry_size,
    > + enum hwmon_sensor_types type,
    > + u32 config)
    > +{
    > + struct hwmon_channel_info *info;
    > + int ret;
    > +
    > + ret = dell_wmi_ddv_update_sensors(wdev, method, sensors, entry_size);
    > + if (ret < 0)
    > + return ERR_PTR(ret);
    > +
    > + mutex_init(&sensors->lock);
    > +
    > + ret = devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_hwmon_cache_destroy, sensors);
    > + if (ret < 0)
    > + return ERR_PTR(ret);
    > +
    > + info = dell_wmi_ddv_channel_create(&wdev->dev, sensors->entries, type, config);
    > + if (IS_ERR(info))
    > + devm_release_action(&wdev->dev, dell_wmi_ddv_hwmon_cache_destroy, sensors);
    > +
    > + return info;
    > +}
    > +
    > +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 hwmon_channel_info *info;
    > + 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;
    > +
    > + info = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip, HWMON_C_REGISTER_TZ);
    > + if (IS_ERR(info)) {
    > + ret = PTR_ERR(info);
    > +
    > + goto err_release;
    > + }
    > +
    > + cinfo->info[index] = info;
    > + index++;
    > +
    > + info = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION, &data->fans,
    > + sizeof(struct fan_sensor_entry), hwmon_fan,
    > + (HWMON_F_INPUT | HWMON_F_LABEL));
    > + if (!IS_ERR(info)) {
    > + cinfo->info[index] = info;
    > + index++;
    > + }
    > +
    > + info = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION, &data->temps,
    > + sizeof(struct thermal_sensor_entry), hwmon_temp,
    > + (HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
    > + HWMON_T_LABEL));
    > + if (!IS_ERR(info)) {
    > + cinfo->info[index] = info;
    > + index++;
    > + }
    > +
    > + if (index < 2) {
    > + ret = -ENODEV;
    > +
    > + goto err_release;
    > + }
    > +
    > + 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 +848,19 @@ 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);
    > + if (IS_REACHABLE(CONFIG_ACPI_BATTERY)) {
    > + ret = dell_wmi_ddv_battery_add(data);
    > + if (ret < 0 && ret != -ENODEV)
    > + dev_warn(&wdev->dev, "Unable to register ACPI battery hook: %d\n", ret);
    > + }
    > +
    > + if (IS_REACHABLE(CONFIG_HWMON)) {
    > + ret = dell_wmi_ddv_hwmon_add(data);
    > + if (ret < 0 && ret != -ENODEV)
    > + dev_warn(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
    > + }
    > +
    > + return 0;
    > }
    >
    > static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
    > --
    > 2.30.2
    >

    \
     
     \ /
      Last update: 2023-03-27 00:11    [W:2.491 / U:0.616 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site