Messages in this thread |  | | Date | Sun, 10 Apr 2022 06:43:22 -0700 | Subject | Re: [PATCH] hwmon: (dell-smm) Add cooling device support | From | Guenter Roeck <> |
| |
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@gmx.de> >>> --- >>> 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@debian.org)"); >>> MODULE_AUTHOR("Pali Rohár <pali@kernel.org>"); >>> 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
|  |