Re: [PATCH] hwmon: (dell-smm) Add cooling device support

From: Guenter Roeck
Date: Sun Apr 10 2022 - 09:43:34 EST

On 4/10/22 03:08, Pali Rohár wrote:
On Saturday 09 April 2022 17:33:48 Armin Wolf wrote:
Am 30.03.22 um 18:33 schrieb Armin Wolf:

Until now, only the temperature sensors where exported thru
the thermal subsystem. Export the fans as "dell-smm-fan[1-3]" too
to make them available as cooling devices.
Also update Documentation.

Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
Documentation/hwmon/dell-smm-hwmon.rst | 7 ++
drivers/hwmon/Kconfig | 1 +
drivers/hwmon/dell-smm-hwmon.c | 94 +++++++++++++++++++++++++-
3 files changed, 99 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index d3323a96665d..41839b7de2c1 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -86,6 +86,13 @@ probe the BIOS on your machine and discover the appropriate codes.

Again, when you find new codes, we'd be happy to have your patches!

+``thermal`` interface
+---------------------------
+
+The driver also exports the fans as thermal cooling devices with
+``type`` set to ``dell-smm-fan[1-3]``. This allows for easy fan control
+using one of the thermal governors.
+
Module parameters
-----------------

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 9ab4e9b3d27b..1175b8e38c45 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -498,6 +498,7 @@ config SENSORS_DS1621
config SENSORS_DELL_SMM
tristate "Dell laptop SMM BIOS hwmon driver"
depends on X86
+ imply THERMAL
help
This hwmon driver adds support for reporting temperature of different
sensors and controls the fans on Dell laptops via System Management
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 84cb1ede7bc0..0c29386f4bd3 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -21,6 +21,7 @@
#include <linux/errno.h>
#include <linux/hwmon.h>
#include <linux/init.h>
+#include <linux/kconfig.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -29,6 +30,7 @@
#include <linux/seq_file.h>
#include <linux/string.h>
#include <linux/smp.h>
+#include <linux/thermal.h>
#include <linux/types.h>
#include <linux/uaccess.h>

@@ -80,6 +82,11 @@ struct dell_smm_data {
int *fan_nominal_speed[DELL_SMM_NO_FANS];
};

+struct dell_smm_cooling_data {
+ u8 fan_num;
+ struct dell_smm_data *data;
+};
+
MODULE_AUTHOR("Massimo Dal Zotto (dz@xxxxxxxxxx)");
MODULE_AUTHOR("Pali Rohár <pali@xxxxxxxxxx>");
MODULE_DESCRIPTION("Dell laptop SMM BIOS hwmon driver");
@@ -638,9 +645,50 @@ static void __init i8k_init_procfs(struct device *dev)

#endif

-/*
- * Hwmon interface
- */
+static int dell_smm_get_max_state(struct thermal_cooling_device *dev, unsigned long *state)
+{
+ struct dell_smm_cooling_data *cdata = dev->devdata;
+
+ *state = cdata->data->i8k_fan_max;
+
+ return 0;
+}
+
+static int dell_smm_get_cur_state(struct thermal_cooling_device *dev, unsigned long *state)
+{
+ struct dell_smm_cooling_data *cdata = dev->devdata;
+ int ret;
+
+ ret = i8k_get_fan_status(cdata->data, cdata->fan_num);
+ if (ret < 0)
+ return ret;
+
+ *state = ret;
+
+ return 0;
+}
+
+static int dell_smm_set_cur_state(struct thermal_cooling_device *dev, unsigned long state)
+{
+ struct dell_smm_cooling_data *cdata = dev->devdata;
+ struct dell_smm_data *data = cdata->data;
+ int ret;
+
+ if (state > data->i8k_fan_max)
+ return -EINVAL;
+
+ mutex_lock(&data->i8k_mutex);
+ ret = i8k_set_fan(data, cdata->fan_num, (int)state);
+ mutex_unlock(&data->i8k_mutex);
+
+ return ret;
+}
+
+static const struct thermal_cooling_device_ops dell_smm_cooling_ops = {
+ .get_max_state = dell_smm_get_max_state,
+ .get_cur_state = dell_smm_get_cur_state,
+ .set_cur_state = dell_smm_set_cur_state,
+};

static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
int channel)
@@ -941,6 +989,37 @@ static const struct hwmon_chip_info dell_smm_chip_info = {
.info = dell_smm_info,
};

+static int __init dell_smm_init_cdev(struct device *dev, u8 fan_num)
+{
+ struct dell_smm_data *data = dev_get_drvdata(dev);
+ struct thermal_cooling_device *cdev;
+ struct dell_smm_cooling_data *cdata;
+ int ret = 0;
+ char *name;
+
+ name = kasprintf(GFP_KERNEL, "dell-smm-fan%u", fan_num + 1);
+ if (!name)
+ return -ENOMEM;
+
+ cdata = devm_kmalloc(dev, sizeof(*cdata), GFP_KERNEL);
+ if (cdata) {
+ cdata->fan_num = fan_num;
+ cdata->data = data;
+ cdev = devm_thermal_of_cooling_device_register(dev, NULL, name, cdata,
+ &dell_smm_cooling_ops);
+ if (IS_ERR(cdev)) {
+ devm_kfree(dev, cdata);
+ ret = PTR_ERR(cdev);
+ }
+ } else {
+ ret = -ENOMEM;
+ }
+
+ kfree(name);
+
+ return ret;
+}
+
static int __init dell_smm_init_hwmon(struct device *dev)
{
struct dell_smm_data *data = dev_get_drvdata(dev);
@@ -967,6 +1046,15 @@ static int __init dell_smm_init_hwmon(struct device *dev)
continue;

data->fan[i] = true;
+
+ /* the cooling device it not critical, ignore failures */
+ if (IS_REACHABLE(CONFIG_THERMAL)) {
+ err = dell_smm_init_cdev(dev, i);
+ if (err < 0)
+ dev_err(dev, "Failed to register cooling device for fan %u\n",
+ i + 1);
+ }
+
data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
sizeof(*data->fan_nominal_speed[i]),
GFP_KERNEL);
--
2.30.2

Any thoughts on this? I tested it on a Dell Inspiron 3505, so i can prove it works.

Hello! If hwmon maintainers are happy with this approach and this new
API then I'm fine with it. Maybe one thing to discuss, should not be
dell_smm_init_cdev() fatal on error?

"the cooling device it not critical, ignore failures"

Besides the misspelling - if it is not fatal, the code should not use
dev_err() but dev_warning(). Other than that I am ok as well.

Guenter