[PATCH v3 0/2] platform/x86: dell-ddv: Various driver updates

[PATCH v3 0/2] platform/x86: dell-ddv: Various driver updates

[Armin Wolf]

Thanks to bugreport 216655 on bugzilla, the contents of the
fan/thermal sensor buffers could be almost completely decoded.

The first patch adds support for exposing the sensor values
over a standard hwmon interface, and the second patch enables
asynchronous probing for the driver since the sensor interface
can be quiet slow.

The patch series was tested on a Dell Inspiron 3505, with the hwmon
patch being tested by multiple people over bugzilla and email. Those
who tested the final version of the hwmon patch are credited with
Tested-by tags.
---
Changes in v3:
- remove empty lines before break/return
- invalidate buffer cache on resume
Changes in v2:
- remove already merged patches
- add patch enabling asynchronous probing
- cache sensor buffers
- fix compilation on 32 bit
- add missing break statements
- remove C++ style comments
- rework battery hook and hwmon probing
- rework Kconfig dependencies

Armin Wolf (2):
  platform/x86: dell-ddv: Add hwmon support
  platform/x86: dell-ddv: Prefer asynchronous probing

 drivers/platform/x86/dell/Kconfig        |   8 +-
 drivers/platform/x86/dell/dell-wmi-ddv.c | 492 ++++++++++++++++++++++-
 2 files changed, 495 insertions(+), 5 deletions(-)

--
2.30.2

[PATCH v3 1/2] platform/x86: dell-ddv: Add hwmon support

[Armin Wolf]

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>
---
 drivers/platform/x86/dell/Kconfig        |   8 +-
 drivers/platform/x86/dell/dell-wmi-ddv.c | 490 ++++++++++++++++++++++-
 2 files changed, 493 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..d81dc4dd93e3 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -12,19 +12,27 @@
 #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/pm.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 +71,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 +229,410 @@ 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_invalidate(struct dell_wmi_ddv_sensors *sensors)
+{
+	mutex_lock(&sensors->lock);
+	kfree(sensors->obj);
+	sensors->obj = NULL;
+	mutex_unlock(&sensors->lock);
+}
+
+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,9 +832,34 @@ 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 int dell_wmi_ddv_resume(struct device *dev)
+{
+	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
+
+	/* Force re-reading of all sensors */
+	dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
+	dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(dell_wmi_ddv_dev_pm_ops, NULL, dell_wmi_ddv_resume);
+
 static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
 	{ DELL_DDV_GUID, NULL },
 	{ }
@@ -382,6 +869,7 @@ MODULE_DEVICE_TABLE(wmi, dell_wmi_ddv_id_table);
 static struct wmi_driver dell_wmi_ddv_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
+		.pm = pm_sleep_ptr(&dell_wmi_ddv_dev_pm_ops),
 	},
 	.id_table = dell_wmi_ddv_id_table,
 	.probe = dell_wmi_ddv_probe,
--
2.30.2

[PATCH v3 2/2] platform/x86: dell-ddv: Prefer asynchronous probing

[Armin Wolf]

During probe, both sensor buffers need to be queried to
initialize the hwmon channels. This might be slow on some
machines, causing a unnecessary delay during boot.
Mark the driver with PROBE_PREFER_ASYNCHRONOUS so that it
can be probed asynchronously.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index d81dc4dd93e3..d547c9d09725 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -10,6 +10,7 @@
 #include <linux/acpi.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/device/driver.h>
 #include <linux/dev_printk.h>
 #include <linux/errno.h>
 #include <linux/kconfig.h>
@@ -869,6 +870,7 @@ MODULE_DEVICE_TABLE(wmi, dell_wmi_ddv_id_table);
 static struct wmi_driver dell_wmi_ddv_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 		.pm = pm_sleep_ptr(&dell_wmi_ddv_dev_pm_ops),
 	},
 	.id_table = dell_wmi_ddv_id_table,
--
2.30.2

Re: [PATCH v3 1/2] platform/x86: dell-ddv: Add hwmon support

[Guenter Roeck]

On Thu, Feb 09, 2023 at 10:15:02PM +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. 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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/platform/x86/dell/Kconfig        |   8 +-
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 490 ++++++++++++++++++++++-
>  2 files changed, 493 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..d81dc4dd93e3 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -12,19 +12,27 @@
>  #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/pm.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 +71,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 +229,410 @@ 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_invalidate(struct dell_wmi_ddv_sensors *sensors)
> +{
> +	mutex_lock(&sensors->lock);
> +	kfree(sensors->obj);
> +	sensors->obj = NULL;
> +	mutex_unlock(&sensors->lock);
> +}
> +
> +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,9 +832,34 @@ 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 int dell_wmi_ddv_resume(struct device *dev)
> +{
> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> +
> +	/* Force re-reading of all sensors */
> +	dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
> +	dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(dell_wmi_ddv_dev_pm_ops, NULL, dell_wmi_ddv_resume);
> +
>  static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
>  	{ DELL_DDV_GUID, NULL },
>  	{ }
> @@ -382,6 +869,7 @@ MODULE_DEVICE_TABLE(wmi, dell_wmi_ddv_id_table);
>  static struct wmi_driver dell_wmi_ddv_driver = {
>  	.driver = {
>  		.name = DRIVER_NAME,
> +		.pm = pm_sleep_ptr(&dell_wmi_ddv_dev_pm_ops),
>  	},
>  	.id_table = dell_wmi_ddv_id_table,
>  	.probe = dell_wmi_ddv_probe,
> --
> 2.30.2
> 

Re: [PATCH v3 0/2] platform/x86: dell-ddv: Various driver updates

[Hans de Goede]

Hi Armin,

On 2/9/23 22:15, Armin Wolf wrote:
> Thanks to bugreport 216655 on bugzilla, the contents of the
> fan/thermal sensor buffers could be almost completely decoded.
> 
> The first patch adds support for exposing the sensor values
> over a standard hwmon interface, and the second patch enables
> asynchronous probing for the driver since the sensor interface
> can be quiet slow.
> 
> The patch series was tested on a Dell Inspiron 3505, with the hwmon
> patch being tested by multiple people over bugzilla and email. Those
> who tested the final version of the hwmon patch are credited with
> Tested-by tags.

Thanks this looks nice and clean, much appreciate
the attention to detail.

I've applied the series to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans






> ---
> Changes in v3:
> - remove empty lines before break/return
> - invalidate buffer cache on resume
> Changes in v2:
> - remove already merged patches
> - add patch enabling asynchronous probing
> - cache sensor buffers
> - fix compilation on 32 bit
> - add missing break statements
> - remove C++ style comments
> - rework battery hook and hwmon probing
> - rework Kconfig dependencies
> 
> Armin Wolf (2):
>   platform/x86: dell-ddv: Add hwmon support
>   platform/x86: dell-ddv: Prefer asynchronous probing
> 
>  drivers/platform/x86/dell/Kconfig        |   8 +-
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 492 ++++++++++++++++++++++-
>  2 files changed, 495 insertions(+), 5 deletions(-)
> 
> --
> 2.30.2
>