* [PATCH v3 0/2] hwmon: (dell-smm) Support additional attributes
@ 2021-09-26 22:10 W_Armin
2021-09-26 22:10 ` [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target W_Armin
2021-09-26 22:10 ` [PATCH v3 2/2] hwmon: (dell-smm) Remove unnecessary includes W_Armin
0 siblings, 2 replies; 16+ messages in thread
From: W_Armin @ 2021-09-26 22:10 UTC (permalink / raw)
To: pali; +Cc: linux, jdelvare, linux-hwmon
From: Armin Wolf <W_Armin@gmx.de>
This patch series adds support for fanX_min, fanX_max and fanX_target.
A second patch also removes some unnecessary includes.
Both patches where tested on a Dell Inspiron 3505 and
a Dell Latitude C600.
Changes in v3:
- improve fanX_min/_max/_target detection logic
Changes in v2:
- update documentation
- prevent out-of-bounds read/write when module is loaded with
custom fan_max value
Armin Wolf (2):
hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
hwmon: (dell-smm) Remove unnecessary includes
Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
drivers/hwmon/dell-smm-hwmon.c | 63 ++++++++++++++++++++++----
2 files changed, 58 insertions(+), 8 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-09-26 22:10 [PATCH v3 0/2] hwmon: (dell-smm) Support additional attributes W_Armin
@ 2021-09-26 22:10 ` W_Armin
2021-10-10 21:18 ` Armin Wolf
` (3 more replies)
2021-09-26 22:10 ` [PATCH v3 2/2] hwmon: (dell-smm) Remove unnecessary includes W_Armin
1 sibling, 4 replies; 16+ messages in thread
From: W_Armin @ 2021-09-26 22:10 UTC (permalink / raw)
To: pali; +Cc: linux, jdelvare, linux-hwmon
From: Armin Wolf <W_Armin@gmx.de>
The nominal speed of each fan can be obtained with
i8k_get_fan_nominal_speed(), however the result is not available
from userspace.
Change that by adding fanX_min, fanX_max and fanX_target attributes.
All are RO since fan control happens over pwm.
Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
2 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index 3bf77a5df995..beec88491171 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -34,6 +34,9 @@ Name Perm Description
=============================== ======= =======================================
fan[1-3]_input RO Fan speed in RPM.
fan[1-3]_label RO Fan label.
+fan[1-3]_min RO Minimal Fan speed in RPM
+fan[1-3]_max RO Maximal Fan speed in RPM
+fan[1-3]_target RO Expected Fan speed in RPM
pwm[1-3] RW Control the fan PWM duty-cycle.
pwm1_enable WO Enable or disable automatic BIOS fan
control (not supported on all laptops,
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 774c1b0715d9..476f2a74bd8c 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -76,6 +76,7 @@ struct dell_smm_data {
int temp_type[DELL_SMM_NO_TEMP];
bool fan[DELL_SMM_NO_FANS];
int fan_type[DELL_SMM_NO_FANS];
+ int *fan_nominal_speed[DELL_SMM_NO_FANS];
};
MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
@@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
if (data->fan[channel] && !data->disallow_fan_type_call)
return 0444;
+ break;
+ case hwmon_fan_min:
+ case hwmon_fan_max:
+ case hwmon_fan_target:
+ if (data->fan_nominal_speed[channel])
+ return 0444;
+
break;
default:
break;
@@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
*val = ret;
+ return 0;
+ case hwmon_fan_min:
+ *val = data->fan_nominal_speed[channel][0];
+
+ return 0;
+ case hwmon_fan_max:
+ *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
+
+ return 0;
+ case hwmon_fan_target:
+ ret = i8k_get_fan_status(data, channel);
+ if (ret < 0)
+ return ret;
+
+ if (ret > data->i8k_fan_max)
+ ret = data->i8k_fan_max;
+
+ *val = data->fan_nominal_speed[channel][ret];
+
return 0;
default:
break;
@@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
HWMON_T_INPUT | HWMON_T_LABEL
),
HWMON_CHANNEL_INFO(fan,
- HWMON_F_INPUT | HWMON_F_LABEL,
- HWMON_F_INPUT | HWMON_F_LABEL,
- HWMON_F_INPUT | HWMON_F_LABEL
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
+ HWMON_F_TARGET,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
+ HWMON_F_TARGET,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
+ HWMON_F_TARGET
),
HWMON_CHANNEL_INFO(pwm,
HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
@@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
{
struct dell_smm_data *data = dev_get_drvdata(dev);
struct device *dell_smm_hwmon_dev;
- int i, err;
+ int i, state, err;
for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
data->temp_type[i] = i8k_get_temp_type(i);
@@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
err = i8k_get_fan_status(data, i);
if (err < 0)
err = i8k_get_fan_type(data, i);
- if (err >= 0)
- data->fan[i] = true;
+
+ if (err < 0)
+ continue;
+
+ data->fan[i] = true;
+ data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
+ sizeof(*data->fan_nominal_speed[i]),
+ GFP_KERNEL);
+ if (!data->fan_nominal_speed[i])
+ continue;
+
+ for (state = 0; state <= data->i8k_fan_max; state++) {
+ err = i8k_get_fan_nominal_speed(data, i, state);
+ if (err < 0) {
+ /* Mark nominal speed table as invalid in case of error */
+ devm_kfree(dev, data->fan_nominal_speed[i]);
+ data->fan_nominal_speed[i] = NULL;
+ break;
+ }
+ data->fan_nominal_speed[i][state] = err;
+ }
}
dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
--
2.20.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] hwmon: (dell-smm) Remove unnecessary includes
2021-09-26 22:10 [PATCH v3 0/2] hwmon: (dell-smm) Support additional attributes W_Armin
2021-09-26 22:10 ` [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target W_Armin
@ 2021-09-26 22:10 ` W_Armin
2021-10-08 21:07 ` Guenter Roeck
1 sibling, 1 reply; 16+ messages in thread
From: W_Armin @ 2021-09-26 22:10 UTC (permalink / raw)
To: pali; +Cc: linux, jdelvare, linux-hwmon
From: Armin Wolf <W_Armin@gmx.de>
sched.h and io.h are not used anywhere in dell-smm-hwmon.c.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/dell-smm-hwmon.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 476f2a74bd8c..af0d0d2b6e99 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -26,8 +26,6 @@
#include <linux/mutex.h>
#include <linux/hwmon.h>
#include <linux/uaccess.h>
-#include <linux/io.h>
-#include <linux/sched.h>
#include <linux/ctype.h>
#include <linux/smp.h>
--
2.20.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/2] hwmon: (dell-smm) Remove unnecessary includes
2021-09-26 22:10 ` [PATCH v3 2/2] hwmon: (dell-smm) Remove unnecessary includes W_Armin
@ 2021-10-08 21:07 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-10-08 21:07 UTC (permalink / raw)
To: W_Armin; +Cc: pali, jdelvare, linux-hwmon
On Mon, Sep 27, 2021 at 12:10:44AM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> sched.h and io.h are not used anywhere in dell-smm-hwmon.c.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Applied.
Thanks,
Guenter
> ---
> drivers/hwmon/dell-smm-hwmon.c | 2 --
> 1 file changed, 2 deletions(-)
>
> --
> 2.20.1
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 476f2a74bd8c..af0d0d2b6e99 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -26,8 +26,6 @@
> #include <linux/mutex.h>
> #include <linux/hwmon.h>
> #include <linux/uaccess.h>
> -#include <linux/io.h>
> -#include <linux/sched.h>
> #include <linux/ctype.h>
> #include <linux/smp.h>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-09-26 22:10 ` [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target W_Armin
@ 2021-10-10 21:18 ` Armin Wolf
2021-10-11 14:27 ` Guenter Roeck
2021-10-11 16:11 ` Pali Rohár
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Armin Wolf @ 2021-10-10 21:18 UTC (permalink / raw)
To: pali; +Cc: linux, jdelvare, linux-hwmon
Am 27.09.21 um 00:10 schrieb W_Armin@gmx.de:
> From: Armin Wolf <W_Armin@gmx.de>
>
> The nominal speed of each fan can be obtained with
> i8k_get_fan_nominal_speed(), however the result is not available
> from userspace.
> Change that by adding fanX_min, fanX_max and fanX_target attributes.
> All are RO since fan control happens over pwm.
>
> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
> drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
> 2 files changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index 3bf77a5df995..beec88491171 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -34,6 +34,9 @@ Name Perm Description
> =============================== ======= =======================================
> fan[1-3]_input RO Fan speed in RPM.
> fan[1-3]_label RO Fan label.
> +fan[1-3]_min RO Minimal Fan speed in RPM
> +fan[1-3]_max RO Maximal Fan speed in RPM
> +fan[1-3]_target RO Expected Fan speed in RPM
> pwm[1-3] RW Control the fan PWM duty-cycle.
> pwm1_enable WO Enable or disable automatic BIOS fan
> control (not supported on all laptops,
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 774c1b0715d9..476f2a74bd8c 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -76,6 +76,7 @@ struct dell_smm_data {
> int temp_type[DELL_SMM_NO_TEMP];
> bool fan[DELL_SMM_NO_FANS];
> int fan_type[DELL_SMM_NO_FANS];
> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
> };
>
> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> if (data->fan[channel] && !data->disallow_fan_type_call)
> return 0444;
>
> + break;
> + case hwmon_fan_min:
> + case hwmon_fan_max:
> + case hwmon_fan_target:
> + if (data->fan_nominal_speed[channel])
> + return 0444;
> +
> break;
> default:
> break;
> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>
> *val = ret;
>
> + return 0;
> + case hwmon_fan_min:
> + *val = data->fan_nominal_speed[channel][0];
> +
> + return 0;
> + case hwmon_fan_max:
> + *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
> +
> + return 0;
> + case hwmon_fan_target:
> + ret = i8k_get_fan_status(data, channel);
> + if (ret < 0)
> + return ret;
> +
> + if (ret > data->i8k_fan_max)
> + ret = data->i8k_fan_max;
> +
> + *val = data->fan_nominal_speed[channel][ret];
> +
> return 0;
> default:
> break;
> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
> HWMON_T_INPUT | HWMON_T_LABEL
> ),
> HWMON_CHANNEL_INFO(fan,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET
> ),
> HWMON_CHANNEL_INFO(pwm,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> {
> struct dell_smm_data *data = dev_get_drvdata(dev);
> struct device *dell_smm_hwmon_dev;
> - int i, err;
> + int i, state, err;
>
> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
> data->temp_type[i] = i8k_get_temp_type(i);
> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> err = i8k_get_fan_status(data, i);
> if (err < 0)
> err = i8k_get_fan_type(data, i);
> - if (err >= 0)
> - data->fan[i] = true;
> +
> + if (err < 0)
> + continue;
> +
> + data->fan[i] = true;
> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
> + sizeof(*data->fan_nominal_speed[i]),
> + GFP_KERNEL);
> + if (!data->fan_nominal_speed[i])
> + continue;
> +
> + for (state = 0; state <= data->i8k_fan_max; state++) {
> + err = i8k_get_fan_nominal_speed(data, i, state);
> + if (err < 0) {
> + /* Mark nominal speed table as invalid in case of error */
> + devm_kfree(dev, data->fan_nominal_speed[i]);
> + data->fan_nominal_speed[i] = NULL;
> + break;
> + }
> + data->fan_nominal_speed[i][state] = err;
> + }
> }
>
> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
> --
> 2.20.1
>
Are there any problems with this patch? I already tested it on two
models, is there something
else i should verify/change?
Armin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-10 21:18 ` Armin Wolf
@ 2021-10-11 14:27 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-10-11 14:27 UTC (permalink / raw)
To: Armin Wolf, pali; +Cc: jdelvare, linux-hwmon
On 10/10/21 2:18 PM, Armin Wolf wrote:
> Am 27.09.21 um 00:10 schrieb W_Armin@gmx.de:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> The nominal speed of each fan can be obtained with
>> i8k_get_fan_nominal_speed(), however the result is not available
>> from userspace.
>> Change that by adding fanX_min, fanX_max and fanX_target attributes.
>> All are RO since fan control happens over pwm.
>>
>> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
>> drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
>> 2 files changed, 58 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>> index 3bf77a5df995..beec88491171 100644
>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>> @@ -34,6 +34,9 @@ Name Perm Description
>> =============================== ======= =======================================
>> fan[1-3]_input RO Fan speed in RPM.
>> fan[1-3]_label RO Fan label.
>> +fan[1-3]_min RO Minimal Fan speed in RPM
>> +fan[1-3]_max RO Maximal Fan speed in RPM
>> +fan[1-3]_target RO Expected Fan speed in RPM
>> pwm[1-3] RW Control the fan PWM duty-cycle.
>> pwm1_enable WO Enable or disable automatic BIOS fan
>> control (not supported on all laptops,
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 774c1b0715d9..476f2a74bd8c 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -76,6 +76,7 @@ struct dell_smm_data {
>> int temp_type[DELL_SMM_NO_TEMP];
>> bool fan[DELL_SMM_NO_FANS];
>> int fan_type[DELL_SMM_NO_FANS];
>> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
>> };
>>
>> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>> if (data->fan[channel] && !data->disallow_fan_type_call)
>> return 0444;
>>
>> + break;
>> + case hwmon_fan_min:
>> + case hwmon_fan_max:
>> + case hwmon_fan_target:
>> + if (data->fan_nominal_speed[channel])
>> + return 0444;
>> +
>> break;
>> default:
>> break;
>> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>>
>> *val = ret;
>>
>> + return 0;
>> + case hwmon_fan_min:
>> + *val = data->fan_nominal_speed[channel][0];
>> +
>> + return 0;
>> + case hwmon_fan_max:
>> + *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
>> +
>> + return 0;
>> + case hwmon_fan_target:
>> + ret = i8k_get_fan_status(data, channel);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (ret > data->i8k_fan_max)
>> + ret = data->i8k_fan_max;
>> +
>> + *val = data->fan_nominal_speed[channel][ret];
>> +
>> return 0;
>> default:
>> break;
>> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
>> HWMON_T_INPUT | HWMON_T_LABEL
>> ),
>> HWMON_CHANNEL_INFO(fan,
>> - HWMON_F_INPUT | HWMON_F_LABEL,
>> - HWMON_F_INPUT | HWMON_F_LABEL,
>> - HWMON_F_INPUT | HWMON_F_LABEL
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET,
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET,
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET
>> ),
>> HWMON_CHANNEL_INFO(pwm,
>> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>> {
>> struct dell_smm_data *data = dev_get_drvdata(dev);
>> struct device *dell_smm_hwmon_dev;
>> - int i, err;
>> + int i, state, err;
>>
>> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
>> data->temp_type[i] = i8k_get_temp_type(i);
>> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>> err = i8k_get_fan_status(data, i);
>> if (err < 0)
>> err = i8k_get_fan_type(data, i);
>> - if (err >= 0)
>> - data->fan[i] = true;
>> +
>> + if (err < 0)
>> + continue;
>> +
>> + data->fan[i] = true;
>> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
>> + sizeof(*data->fan_nominal_speed[i]),
>> + GFP_KERNEL);
>> + if (!data->fan_nominal_speed[i])
>> + continue;
>> +
>> + for (state = 0; state <= data->i8k_fan_max; state++) {
>> + err = i8k_get_fan_nominal_speed(data, i, state);
>> + if (err < 0) {
>> + /* Mark nominal speed table as invalid in case of error */
>> + devm_kfree(dev, data->fan_nominal_speed[i]);
>> + data->fan_nominal_speed[i] = NULL;
>> + break;
>> + }
>> + data->fan_nominal_speed[i][state] = err;
>> + }
>> }
>>
>> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
>> --
>> 2.20.1
>>
> Are there any problems with this patch? I already tested it on two
> models, is there something
> else i should verify/change?
>
I am waiting for an Acked-by:/Reviewed-by: from Pali.
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-09-26 22:10 ` [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target W_Armin
2021-10-10 21:18 ` Armin Wolf
@ 2021-10-11 16:11 ` Pali Rohár
2021-10-11 18:48 ` Guenter Roeck
2021-10-13 14:38 ` Pali Rohár
2021-10-13 18:51 ` Guenter Roeck
3 siblings, 1 reply; 16+ messages in thread
From: Pali Rohár @ 2021-10-11 16:11 UTC (permalink / raw)
To: W_Armin; +Cc: linux, jdelvare, linux-hwmon
On Monday 27 September 2021 00:10:43 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> The nominal speed of each fan can be obtained with
> i8k_get_fan_nominal_speed(), however the result is not available
> from userspace.
> Change that by adding fanX_min, fanX_max and fanX_target attributes.
> All are RO since fan control happens over pwm.
>
> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
> drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
> 2 files changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index 3bf77a5df995..beec88491171 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -34,6 +34,9 @@ Name Perm Description
> =============================== ======= =======================================
> fan[1-3]_input RO Fan speed in RPM.
> fan[1-3]_label RO Fan label.
> +fan[1-3]_min RO Minimal Fan speed in RPM
> +fan[1-3]_max RO Maximal Fan speed in RPM
> +fan[1-3]_target RO Expected Fan speed in RPM
Hello!
I do not know API / meaning of these 3 properties, so I looked into
hwmon documentation at:
https://www.kernel.org/doc/html/latest/hwmon/sysfs-interface.html#fans
And in documentation is written that both 3 properties (min, max and
target) are RW.
I'm somehow lost as I have not fully understood what is the original
meaning of these 3 properties. Guenter could you help?
After reading I understood it as that these properties are for HW which
supports controlling directly fan speed via RPM (and not PWM). And so
user can set lower and upper limit of fan speed (via _min and _max) or
can set exact RPM value (via _target).
> pwm[1-3] RW Control the fan PWM duty-cycle.
> pwm1_enable WO Enable or disable automatic BIOS fan
> control (not supported on all laptops,
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 774c1b0715d9..476f2a74bd8c 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -76,6 +76,7 @@ struct dell_smm_data {
> int temp_type[DELL_SMM_NO_TEMP];
> bool fan[DELL_SMM_NO_FANS];
> int fan_type[DELL_SMM_NO_FANS];
> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
> };
>
> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> if (data->fan[channel] && !data->disallow_fan_type_call)
> return 0444;
>
> + break;
> + case hwmon_fan_min:
> + case hwmon_fan_max:
> + case hwmon_fan_target:
> + if (data->fan_nominal_speed[channel])
> + return 0444;
> +
> break;
> default:
> break;
> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>
> *val = ret;
>
> + return 0;
> + case hwmon_fan_min:
> + *val = data->fan_nominal_speed[channel][0];
When fan is turned off then it has speed 0 RPM. So should not be minimal
fan speed always hardcoded to zero?
> +
> + return 0;
> + case hwmon_fan_max:
> + *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
> +
> + return 0;
> + case hwmon_fan_target:
> + ret = i8k_get_fan_status(data, channel);
> + if (ret < 0)
> + return ret;
> +
> + if (ret > data->i8k_fan_max)
> + ret = data->i8k_fan_max;
> +
> + *val = data->fan_nominal_speed[channel][ret];
> +
> return 0;
> default:
> break;
> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
> HWMON_T_INPUT | HWMON_T_LABEL
> ),
> HWMON_CHANNEL_INFO(fan,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET
> ),
> HWMON_CHANNEL_INFO(pwm,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> {
> struct dell_smm_data *data = dev_get_drvdata(dev);
> struct device *dell_smm_hwmon_dev;
> - int i, err;
> + int i, state, err;
>
> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
> data->temp_type[i] = i8k_get_temp_type(i);
> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> err = i8k_get_fan_status(data, i);
> if (err < 0)
> err = i8k_get_fan_type(data, i);
> - if (err >= 0)
> - data->fan[i] = true;
> +
> + if (err < 0)
> + continue;
> +
> + data->fan[i] = true;
> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
> + sizeof(*data->fan_nominal_speed[i]),
> + GFP_KERNEL);
> + if (!data->fan_nominal_speed[i])
> + continue;
> +
> + for (state = 0; state <= data->i8k_fan_max; state++) {
> + err = i8k_get_fan_nominal_speed(data, i, state);
> + if (err < 0) {
> + /* Mark nominal speed table as invalid in case of error */
> + devm_kfree(dev, data->fan_nominal_speed[i]);
> + data->fan_nominal_speed[i] = NULL;
> + break;
> + }
> + data->fan_nominal_speed[i][state] = err;
> + }
> }
>
> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-11 16:11 ` Pali Rohár
@ 2021-10-11 18:48 ` Guenter Roeck
2021-10-11 19:14 ` Armin Wolf
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2021-10-11 18:48 UTC (permalink / raw)
To: Pali Rohár, W_Armin; +Cc: jdelvare, linux-hwmon
On 10/11/21 9:11 AM, Pali Rohár wrote:
> On Monday 27 September 2021 00:10:43 W_Armin@gmx.de wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> The nominal speed of each fan can be obtained with
>> i8k_get_fan_nominal_speed(), however the result is not available
>> from userspace.
>> Change that by adding fanX_min, fanX_max and fanX_target attributes.
>> All are RO since fan control happens over pwm.
>>
>> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
>> drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
>> 2 files changed, 58 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>> index 3bf77a5df995..beec88491171 100644
>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>> @@ -34,6 +34,9 @@ Name Perm Description
>> =============================== ======= =======================================
>> fan[1-3]_input RO Fan speed in RPM.
>> fan[1-3]_label RO Fan label.
>> +fan[1-3]_min RO Minimal Fan speed in RPM
>> +fan[1-3]_max RO Maximal Fan speed in RPM
>> +fan[1-3]_target RO Expected Fan speed in RPM
>
> Hello!
>
> I do not know API / meaning of these 3 properties, so I looked into
> hwmon documentation at:
> https://www.kernel.org/doc/html/latest/hwmon/sysfs-interface.html#fans
>
> And in documentation is written that both 3 properties (min, max and
> target) are RW.
>
That is the expectation. That doesn't mean they _have_ to be RW.
It doesn't make sense to categorically demand that attributes are RW
if the hardware does not support it.
> I'm somehow lost as I have not fully understood what is the original
> meaning of these 3 properties. Guenter could you help?
>
min: lower fan speed results in warning/error.
max: higher fan speed results in error
target: target fan speed. The controller should set the fan speed
to this level.
> After reading I understood it as that these properties are for HW which
> supports controlling directly fan speed via RPM (and not PWM). And so
> user can set lower and upper limit of fan speed (via _min and _max) or
> can set exact RPM value (via _target).
>
Not really. The controller can try to modify a pwm value to reach the
configured target fan speed. min/max values apply to both pwm and rpm
controlled fans.
>> pwm[1-3] RW Control the fan PWM duty-cycle.
>> pwm1_enable WO Enable or disable automatic BIOS fan
>> control (not supported on all laptops,
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 774c1b0715d9..476f2a74bd8c 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -76,6 +76,7 @@ struct dell_smm_data {
>> int temp_type[DELL_SMM_NO_TEMP];
>> bool fan[DELL_SMM_NO_FANS];
>> int fan_type[DELL_SMM_NO_FANS];
>> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
>> };
>>
>> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>> if (data->fan[channel] && !data->disallow_fan_type_call)
>> return 0444;
>>
>> + break;
>> + case hwmon_fan_min:
>> + case hwmon_fan_max:
>> + case hwmon_fan_target:
>> + if (data->fan_nominal_speed[channel])
>> + return 0444;
>> +
>> break;
>> default:
>> break;
>> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>>
>> *val = ret;
>>
>> + return 0;
>> + case hwmon_fan_min:
>> + *val = data->fan_nominal_speed[channel][0];
>
> When fan is turned off then it has speed 0 RPM. So should not be minimal
> fan speed always hardcoded to zero?
>
No. Fans can be turned off on most systems/controllers. This means the
"minimum" fan speed would always be 0, and the attribute would be
worthless. In other words, "fan turned off" is always an exception.
>> +
>> + return 0;
>> + case hwmon_fan_max:
>> + *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
>> +
>> + return 0;
>> + case hwmon_fan_target:
>> + ret = i8k_get_fan_status(data, channel);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (ret > data->i8k_fan_max)
>> + ret = data->i8k_fan_max;
>> +
>> + *val = data->fan_nominal_speed[channel][ret];
>> +
>> return 0;
>> default:
>> break;
>> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
>> HWMON_T_INPUT | HWMON_T_LABEL
>> ),
>> HWMON_CHANNEL_INFO(fan,
>> - HWMON_F_INPUT | HWMON_F_LABEL,
>> - HWMON_F_INPUT | HWMON_F_LABEL,
>> - HWMON_F_INPUT | HWMON_F_LABEL
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET,
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET,
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET
>> ),
>> HWMON_CHANNEL_INFO(pwm,
>> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>> {
>> struct dell_smm_data *data = dev_get_drvdata(dev);
>> struct device *dell_smm_hwmon_dev;
>> - int i, err;
>> + int i, state, err;
>>
>> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
>> data->temp_type[i] = i8k_get_temp_type(i);
>> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>> err = i8k_get_fan_status(data, i);
>> if (err < 0)
>> err = i8k_get_fan_type(data, i);
>> - if (err >= 0)
>> - data->fan[i] = true;
>> +
>> + if (err < 0)
>> + continue;
>> +
>> + data->fan[i] = true;
>> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
>> + sizeof(*data->fan_nominal_speed[i]),
>> + GFP_KERNEL);
>> + if (!data->fan_nominal_speed[i])
>> + continue;
>> +
>> + for (state = 0; state <= data->i8k_fan_max; state++) {
>> + err = i8k_get_fan_nominal_speed(data, i, state);
>> + if (err < 0) {
>> + /* Mark nominal speed table as invalid in case of error */
>> + devm_kfree(dev, data->fan_nominal_speed[i]);
>> + data->fan_nominal_speed[i] = NULL;
>> + break;
>> + }
>> + data->fan_nominal_speed[i][state] = err;
>> + }
>> }
>>
>> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
>> --
>> 2.20.1
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-11 18:48 ` Guenter Roeck
@ 2021-10-11 19:14 ` Armin Wolf
2021-10-13 14:38 ` Pali Rohár
0 siblings, 1 reply; 16+ messages in thread
From: Armin Wolf @ 2021-10-11 19:14 UTC (permalink / raw)
To: Guenter Roeck, Pali Rohár; +Cc: jdelvare, linux-hwmon
Am 11.10.21 um 20:48 schrieb Guenter Roeck:
> On 10/11/21 9:11 AM, Pali Rohár wrote:
>> On Monday 27 September 2021 00:10:43 W_Armin@gmx.de wrote:
>>> From: Armin Wolf <W_Armin@gmx.de>
>>>
>>> The nominal speed of each fan can be obtained with
>>> i8k_get_fan_nominal_speed(), however the result is not available
>>> from userspace.
>>> Change that by adding fanX_min, fanX_max and fanX_target attributes.
>>> All are RO since fan control happens over pwm.
>>>
>>> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
>>> drivers/hwmon/dell-smm-hwmon.c | 61
>>> +++++++++++++++++++++++---
>>> 2 files changed, 58 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst
>>> b/Documentation/hwmon/dell-smm-hwmon.rst
>>> index 3bf77a5df995..beec88491171 100644
>>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>>> @@ -34,6 +34,9 @@ Name Perm Description
>>> =============================== =======
>>> =======================================
>>> fan[1-3]_input RO Fan speed in RPM.
>>> fan[1-3]_label RO Fan label.
>>> +fan[1-3]_min RO Minimal Fan speed in RPM
>>> +fan[1-3]_max RO Maximal Fan speed in RPM
>>> +fan[1-3]_target RO Expected Fan speed in RPM
>>
>> Hello!
>>
>> I do not know API / meaning of these 3 properties, so I looked into
>> hwmon documentation at:
>> https://www.kernel.org/doc/html/latest/hwmon/sysfs-interface.html#fans
>>
>> And in documentation is written that both 3 properties (min, max and
>> target) are RW.
>>
>
> That is the expectation. That doesn't mean they _have_ to be RW.
> It doesn't make sense to categorically demand that attributes are RW
> if the hardware does not support it.
>
>> I'm somehow lost as I have not fully understood what is the original
>> meaning of these 3 properties. Guenter could you help?
>>
> min: lower fan speed results in warning/error.
> max: higher fan speed results in error
> target: target fan speed. The controller should set the fan speed
> to this level.
>
>> After reading I understood it as that these properties are for HW which
>> supports controlling directly fan speed via RPM (and not PWM). And so
>> user can set lower and upper limit of fan speed (via _min and _max) or
>> can set exact RPM value (via _target).
>>
>
> Not really. The controller can try to modify a pwm value to reach the
> configured target fan speed. min/max values apply to both pwm and rpm
> controlled fans.
>
The reason i made fanX_target RO is that the SMM interface does not
directly support
setting the fan speed in rpm. Since the attributes should reflect the
hardware implementation,
making fanX_target RW would make no sense (at least to me).
>>> pwm[1-3] RW Control the fan PWM duty-cycle.
>>> pwm1_enable WO Enable or disable
>>> automatic BIOS fan
>>> control (not supported on
>>> all laptops,
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>> b/drivers/hwmon/dell-smm-hwmon.c
>>> index 774c1b0715d9..476f2a74bd8c 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -76,6 +76,7 @@ struct dell_smm_data {
>>> int temp_type[DELL_SMM_NO_TEMP];
>>> bool fan[DELL_SMM_NO_FANS];
>>> int fan_type[DELL_SMM_NO_FANS];
>>> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
>>> };
>>>
>>> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>>> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void
>>> *drvdata, enum hwmon_sensor_types
>>> if (data->fan[channel] && !data->disallow_fan_type_call)
>>> return 0444;
>>>
>>> + break;
>>> + case hwmon_fan_min:
>>> + case hwmon_fan_max:
>>> + case hwmon_fan_target:
>>> + if (data->fan_nominal_speed[channel])
>>> + return 0444;
>>> +
>>> break;
>>> default:
>>> break;
>>> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev,
>>> enum hwmon_sensor_types type, u32 a
>>>
>>> *val = ret;
>>>
>>> + return 0;
>>> + case hwmon_fan_min:
>>> + *val = data->fan_nominal_speed[channel][0];
>>
>> When fan is turned off then it has speed 0 RPM. So should not be minimal
>> fan speed always hardcoded to zero?
>>
>
> No. Fans can be turned off on most systems/controllers. This means the
> "minimum" fan speed would always be 0, and the attribute would be
> worthless. In other words, "fan turned off" is always an exception.
>
I am not sure if we can assume that the fans will have 0 rpm when turned
off.
Maybe some notebook model defines 100 rpm as "turned off"?
In this case we should trust the nominal speed for "off" rather than assume
its 0 rpm.
>>> +
>>> + return 0;
>>> + case hwmon_fan_max:
>>> + *val =
>>> data->fan_nominal_speed[channel][data->i8k_fan_max];
>>> +
>>> + return 0;
>>> + case hwmon_fan_target:
>>> + ret = i8k_get_fan_status(data, channel);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (ret > data->i8k_fan_max)
>>> + ret = data->i8k_fan_max;
>>> +
>>> + *val = data->fan_nominal_speed[channel][ret];
>>> +
>>> return 0;
>>> default:
>>> break;
>>> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info
>>> *dell_smm_info[] = {
>>> HWMON_T_INPUT | HWMON_T_LABEL
>>> ),
>>> HWMON_CHANNEL_INFO(fan,
>>> - HWMON_F_INPUT | HWMON_F_LABEL,
>>> - HWMON_F_INPUT | HWMON_F_LABEL,
>>> - HWMON_F_INPUT | HWMON_F_LABEL
>>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN |
>>> HWMON_F_MAX |
>>> + HWMON_F_TARGET,
>>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN |
>>> HWMON_F_MAX |
>>> + HWMON_F_TARGET,
>>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN |
>>> HWMON_F_MAX |
>>> + HWMON_F_TARGET
>>> ),
>>> HWMON_CHANNEL_INFO(pwm,
>>> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>>> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct
>>> device *dev)
>>> {
>>> struct dell_smm_data *data = dev_get_drvdata(dev);
>>> struct device *dell_smm_hwmon_dev;
>>> - int i, err;
>>> + int i, state, err;
>>>
>>> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
>>> data->temp_type[i] = i8k_get_temp_type(i);
>>> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct
>>> device *dev)
>>> err = i8k_get_fan_status(data, i);
>>> if (err < 0)
>>> err = i8k_get_fan_type(data, i);
>>> - if (err >= 0)
>>> - data->fan[i] = true;
>>> +
>>> + if (err < 0)
>>> + continue;
>>> +
>>> + data->fan[i] = true;
>>> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev,
>>> data->i8k_fan_max + 1,
>>> + sizeof(*data->fan_nominal_speed[i]),
>>> + GFP_KERNEL);
>>> + if (!data->fan_nominal_speed[i])
>>> + continue;
>>> +
>>> + for (state = 0; state <= data->i8k_fan_max; state++) {
>>> + err = i8k_get_fan_nominal_speed(data, i, state);
>>> + if (err < 0) {
>>> + /* Mark nominal speed table as invalid in case of
>>> error */
>>> + devm_kfree(dev, data->fan_nominal_speed[i]);
>>> + data->fan_nominal_speed[i] = NULL;
>>> + break;
>>> + }
>>> + data->fan_nominal_speed[i][state] = err;
>>> + }
>>> }
>>>
>>> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev,
>>> "dell_smm", data,
>>> --
>>> 2.20.1
>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-11 19:14 ` Armin Wolf
@ 2021-10-13 14:38 ` Pali Rohár
0 siblings, 0 replies; 16+ messages in thread
From: Pali Rohár @ 2021-10-13 14:38 UTC (permalink / raw)
To: Armin Wolf; +Cc: Guenter Roeck, jdelvare, linux-hwmon
On Monday 11 October 2021 21:14:36 Armin Wolf wrote:
> Am 11.10.21 um 20:48 schrieb Guenter Roeck:
> > On 10/11/21 9:11 AM, Pali Rohár wrote:
> > > On Monday 27 September 2021 00:10:43 W_Armin@gmx.de wrote:
> > > > From: Armin Wolf <W_Armin@gmx.de>
> > > >
> > > > The nominal speed of each fan can be obtained with
> > > > i8k_get_fan_nominal_speed(), however the result is not available
> > > > from userspace.
> > > > Change that by adding fanX_min, fanX_max and fanX_target attributes.
> > > > All are RO since fan control happens over pwm.
> > > >
> > > > Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
> > > >
> > > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > > ---
> > > > Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
> > > > drivers/hwmon/dell-smm-hwmon.c | 61
> > > > +++++++++++++++++++++++---
> > > > 2 files changed, 58 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/Documentation/hwmon/dell-smm-hwmon.rst
> > > > b/Documentation/hwmon/dell-smm-hwmon.rst
> > > > index 3bf77a5df995..beec88491171 100644
> > > > --- a/Documentation/hwmon/dell-smm-hwmon.rst
> > > > +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> > > > @@ -34,6 +34,9 @@ Name Perm Description
> > > > =============================== =======
> > > > =======================================
> > > > fan[1-3]_input RO Fan speed in RPM.
> > > > fan[1-3]_label RO Fan label.
> > > > +fan[1-3]_min RO Minimal Fan speed in RPM
> > > > +fan[1-3]_max RO Maximal Fan speed in RPM
> > > > +fan[1-3]_target RO Expected Fan speed in RPM
> > >
> > > Hello!
> > >
> > > I do not know API / meaning of these 3 properties, so I looked into
> > > hwmon documentation at:
> > > https://www.kernel.org/doc/html/latest/hwmon/sysfs-interface.html#fans
> > >
> > > And in documentation is written that both 3 properties (min, max and
> > > target) are RW.
> > >
> >
> > That is the expectation. That doesn't mean they _have_ to be RW.
> > It doesn't make sense to categorically demand that attributes are RW
> > if the hardware does not support it.
> >
> > > I'm somehow lost as I have not fully understood what is the original
> > > meaning of these 3 properties. Guenter could you help?
> > >
> > min: lower fan speed results in warning/error.
> > max: higher fan speed results in error
> > target: target fan speed. The controller should set the fan speed
> > to this level.
> >
> > > After reading I understood it as that these properties are for HW which
> > > supports controlling directly fan speed via RPM (and not PWM). And so
> > > user can set lower and upper limit of fan speed (via _min and _max) or
> > > can set exact RPM value (via _target).
> > >
> >
> > Not really. The controller can try to modify a pwm value to reach the
> > configured target fan speed. min/max values apply to both pwm and rpm
> > controlled fans.
> >
> The reason i made fanX_target RO is that the SMM interface does not directly
> support
> setting the fan speed in rpm. Since the attributes should reflect the
> hardware implementation,
> making fanX_target RW would make no sense (at least to me).
Ok. Then patch seems to be fine.
> > > > pwm[1-3] RW Control the fan PWM duty-cycle.
> > > > pwm1_enable WO Enable or disable
> > > > automatic BIOS fan
> > > > control (not supported
> > > > on all laptops,
> > > > diff --git a/drivers/hwmon/dell-smm-hwmon.c
> > > > b/drivers/hwmon/dell-smm-hwmon.c
> > > > index 774c1b0715d9..476f2a74bd8c 100644
> > > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > > @@ -76,6 +76,7 @@ struct dell_smm_data {
> > > > int temp_type[DELL_SMM_NO_TEMP];
> > > > bool fan[DELL_SMM_NO_FANS];
> > > > int fan_type[DELL_SMM_NO_FANS];
> > > > + int *fan_nominal_speed[DELL_SMM_NO_FANS];
> > > > };
> > > >
> > > > MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
> > > > @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const
> > > > void *drvdata, enum hwmon_sensor_types
> > > > if (data->fan[channel] && !data->disallow_fan_type_call)
> > > > return 0444;
> > > >
> > > > + break;
> > > > + case hwmon_fan_min:
> > > > + case hwmon_fan_max:
> > > > + case hwmon_fan_target:
> > > > + if (data->fan_nominal_speed[channel])
> > > > + return 0444;
> > > > +
> > > > break;
> > > > default:
> > > > break;
> > > > @@ -740,6 +748,25 @@ static int dell_smm_read(struct device
> > > > *dev, enum hwmon_sensor_types type, u32 a
> > > >
> > > > *val = ret;
> > > >
> > > > + return 0;
> > > > + case hwmon_fan_min:
> > > > + *val = data->fan_nominal_speed[channel][0];
> > >
> > > When fan is turned off then it has speed 0 RPM. So should not be minimal
> > > fan speed always hardcoded to zero?
> > >
> >
> > No. Fans can be turned off on most systems/controllers. This means the
> > "minimum" fan speed would always be 0, and the attribute would be
> > worthless. In other words, "fan turned off" is always an exception.
Ok!
> I am not sure if we can assume that the fans will have 0 rpm when turned
> off.
E6440 reports 0 rpm when fan is turned off.
> Maybe some notebook model defines 100 rpm as "turned off"?
I really do not know.
> In this case we should trust the nominal speed for "off" rather than assume
> its 0 rpm.
> > > > +
> > > > + return 0;
> > > > + case hwmon_fan_max:
> > > > + *val =
> > > > data->fan_nominal_speed[channel][data->i8k_fan_max];
> > > > +
> > > > + return 0;
> > > > + case hwmon_fan_target:
> > > > + ret = i8k_get_fan_status(data, channel);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + if (ret > data->i8k_fan_max)
> > > > + ret = data->i8k_fan_max;
> > > > +
> > > > + *val = data->fan_nominal_speed[channel][ret];
> > > > +
> > > > return 0;
> > > > default:
> > > > break;
> > > > @@ -889,9 +916,12 @@ static const struct hwmon_channel_info
> > > > *dell_smm_info[] = {
> > > > HWMON_T_INPUT | HWMON_T_LABEL
> > > > ),
> > > > HWMON_CHANNEL_INFO(fan,
> > > > - HWMON_F_INPUT | HWMON_F_LABEL,
> > > > - HWMON_F_INPUT | HWMON_F_LABEL,
> > > > - HWMON_F_INPUT | HWMON_F_LABEL
> > > > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN |
> > > > HWMON_F_MAX |
> > > > + HWMON_F_TARGET,
> > > > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN |
> > > > HWMON_F_MAX |
> > > > + HWMON_F_TARGET,
> > > > + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN |
> > > > HWMON_F_MAX |
> > > > + HWMON_F_TARGET
> > > > ),
> > > > HWMON_CHANNEL_INFO(pwm,
> > > > HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> > > > @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct
> > > > device *dev)
> > > > {
> > > > struct dell_smm_data *data = dev_get_drvdata(dev);
> > > > struct device *dell_smm_hwmon_dev;
> > > > - int i, err;
> > > > + int i, state, err;
> > > >
> > > > for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
> > > > data->temp_type[i] = i8k_get_temp_type(i);
> > > > @@ -926,8 +956,27 @@ static int __init
> > > > dell_smm_init_hwmon(struct device *dev)
> > > > err = i8k_get_fan_status(data, i);
> > > > if (err < 0)
> > > > err = i8k_get_fan_type(data, i);
> > > > - if (err >= 0)
> > > > - data->fan[i] = true;
> > > > +
> > > > + if (err < 0)
> > > > + continue;
> > > > +
> > > > + data->fan[i] = true;
> > > > + data->fan_nominal_speed[i] = devm_kmalloc_array(dev,
> > > > data->i8k_fan_max + 1,
> > > > + sizeof(*data->fan_nominal_speed[i]),
> > > > + GFP_KERNEL);
> > > > + if (!data->fan_nominal_speed[i])
> > > > + continue;
> > > > +
> > > > + for (state = 0; state <= data->i8k_fan_max; state++) {
> > > > + err = i8k_get_fan_nominal_speed(data, i, state);
> > > > + if (err < 0) {
> > > > + /* Mark nominal speed table as invalid in case
> > > > of error */
> > > > + devm_kfree(dev, data->fan_nominal_speed[i]);
> > > > + data->fan_nominal_speed[i] = NULL;
> > > > + break;
> > > > + }
> > > > + data->fan_nominal_speed[i][state] = err;
> > > > + }
> > > > }
> > > >
> > > > dell_smm_hwmon_dev =
> > > > devm_hwmon_device_register_with_info(dev, "dell_smm", data,
> > > > --
> > > > 2.20.1
> > > >
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-09-26 22:10 ` [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target W_Armin
2021-10-10 21:18 ` Armin Wolf
2021-10-11 16:11 ` Pali Rohár
@ 2021-10-13 14:38 ` Pali Rohár
2021-10-13 18:51 ` Guenter Roeck
3 siblings, 0 replies; 16+ messages in thread
From: Pali Rohár @ 2021-10-13 14:38 UTC (permalink / raw)
To: W_Armin; +Cc: linux, jdelvare, linux-hwmon
On Monday 27 September 2021 00:10:43 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> The nominal speed of each fan can be obtained with
> i8k_get_fan_nominal_speed(), however the result is not available
> from userspace.
> Change that by adding fanX_min, fanX_max and fanX_target attributes.
> All are RO since fan control happens over pwm.
>
> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Reviewed-by: Pali Rohár <pali@kernel.org>
> ---
> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
> drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
> 2 files changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index 3bf77a5df995..beec88491171 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -34,6 +34,9 @@ Name Perm Description
> =============================== ======= =======================================
> fan[1-3]_input RO Fan speed in RPM.
> fan[1-3]_label RO Fan label.
> +fan[1-3]_min RO Minimal Fan speed in RPM
> +fan[1-3]_max RO Maximal Fan speed in RPM
> +fan[1-3]_target RO Expected Fan speed in RPM
> pwm[1-3] RW Control the fan PWM duty-cycle.
> pwm1_enable WO Enable or disable automatic BIOS fan
> control (not supported on all laptops,
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 774c1b0715d9..476f2a74bd8c 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -76,6 +76,7 @@ struct dell_smm_data {
> int temp_type[DELL_SMM_NO_TEMP];
> bool fan[DELL_SMM_NO_FANS];
> int fan_type[DELL_SMM_NO_FANS];
> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
> };
>
> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> if (data->fan[channel] && !data->disallow_fan_type_call)
> return 0444;
>
> + break;
> + case hwmon_fan_min:
> + case hwmon_fan_max:
> + case hwmon_fan_target:
> + if (data->fan_nominal_speed[channel])
> + return 0444;
> +
> break;
> default:
> break;
> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>
> *val = ret;
>
> + return 0;
> + case hwmon_fan_min:
> + *val = data->fan_nominal_speed[channel][0];
> +
> + return 0;
> + case hwmon_fan_max:
> + *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
> +
> + return 0;
> + case hwmon_fan_target:
> + ret = i8k_get_fan_status(data, channel);
> + if (ret < 0)
> + return ret;
> +
> + if (ret > data->i8k_fan_max)
> + ret = data->i8k_fan_max;
> +
> + *val = data->fan_nominal_speed[channel][ret];
> +
> return 0;
> default:
> break;
> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
> HWMON_T_INPUT | HWMON_T_LABEL
> ),
> HWMON_CHANNEL_INFO(fan,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET
> ),
> HWMON_CHANNEL_INFO(pwm,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> {
> struct dell_smm_data *data = dev_get_drvdata(dev);
> struct device *dell_smm_hwmon_dev;
> - int i, err;
> + int i, state, err;
>
> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
> data->temp_type[i] = i8k_get_temp_type(i);
> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> err = i8k_get_fan_status(data, i);
> if (err < 0)
> err = i8k_get_fan_type(data, i);
> - if (err >= 0)
> - data->fan[i] = true;
> +
> + if (err < 0)
> + continue;
> +
> + data->fan[i] = true;
> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
> + sizeof(*data->fan_nominal_speed[i]),
> + GFP_KERNEL);
> + if (!data->fan_nominal_speed[i])
> + continue;
> +
> + for (state = 0; state <= data->i8k_fan_max; state++) {
> + err = i8k_get_fan_nominal_speed(data, i, state);
> + if (err < 0) {
> + /* Mark nominal speed table as invalid in case of error */
> + devm_kfree(dev, data->fan_nominal_speed[i]);
> + data->fan_nominal_speed[i] = NULL;
> + break;
> + }
> + data->fan_nominal_speed[i][state] = err;
> + }
> }
>
> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-09-26 22:10 ` [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target W_Armin
` (2 preceding siblings ...)
2021-10-13 14:38 ` Pali Rohár
@ 2021-10-13 18:51 ` Guenter Roeck
2021-10-16 21:25 ` Armin Wolf
3 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2021-10-13 18:51 UTC (permalink / raw)
To: W_Armin; +Cc: pali, jdelvare, linux-hwmon
On Mon, Sep 27, 2021 at 12:10:43AM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> The nominal speed of each fan can be obtained with
> i8k_get_fan_nominal_speed(), however the result is not available
> from userspace.
> Change that by adding fanX_min, fanX_max and fanX_target attributes.
> All are RO since fan control happens over pwm.
>
> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Reviewed-by: Pali Rohár <pali@kernel.org>
Applied.
Thanks,
Guenter
> ---
> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
> drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
> 2 files changed, 58 insertions(+), 6 deletions(-)
>
> --
> 2.20.1
>
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index 3bf77a5df995..beec88491171 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -34,6 +34,9 @@ Name Perm Description
> =============================== ======= =======================================
> fan[1-3]_input RO Fan speed in RPM.
> fan[1-3]_label RO Fan label.
> +fan[1-3]_min RO Minimal Fan speed in RPM
> +fan[1-3]_max RO Maximal Fan speed in RPM
> +fan[1-3]_target RO Expected Fan speed in RPM
> pwm[1-3] RW Control the fan PWM duty-cycle.
> pwm1_enable WO Enable or disable automatic BIOS fan
> control (not supported on all laptops,
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 774c1b0715d9..476f2a74bd8c 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -76,6 +76,7 @@ struct dell_smm_data {
> int temp_type[DELL_SMM_NO_TEMP];
> bool fan[DELL_SMM_NO_FANS];
> int fan_type[DELL_SMM_NO_FANS];
> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
> };
>
> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> if (data->fan[channel] && !data->disallow_fan_type_call)
> return 0444;
>
> + break;
> + case hwmon_fan_min:
> + case hwmon_fan_max:
> + case hwmon_fan_target:
> + if (data->fan_nominal_speed[channel])
> + return 0444;
> +
> break;
> default:
> break;
> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>
> *val = ret;
>
> + return 0;
> + case hwmon_fan_min:
> + *val = data->fan_nominal_speed[channel][0];
> +
> + return 0;
> + case hwmon_fan_max:
> + *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
> +
> + return 0;
> + case hwmon_fan_target:
> + ret = i8k_get_fan_status(data, channel);
> + if (ret < 0)
> + return ret;
> +
> + if (ret > data->i8k_fan_max)
> + ret = data->i8k_fan_max;
> +
> + *val = data->fan_nominal_speed[channel][ret];
> +
> return 0;
> default:
> break;
> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
> HWMON_T_INPUT | HWMON_T_LABEL
> ),
> HWMON_CHANNEL_INFO(fan,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL,
> - HWMON_F_INPUT | HWMON_F_LABEL
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET,
> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
> + HWMON_F_TARGET
> ),
> HWMON_CHANNEL_INFO(pwm,
> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> {
> struct dell_smm_data *data = dev_get_drvdata(dev);
> struct device *dell_smm_hwmon_dev;
> - int i, err;
> + int i, state, err;
>
> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
> data->temp_type[i] = i8k_get_temp_type(i);
> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> err = i8k_get_fan_status(data, i);
> if (err < 0)
> err = i8k_get_fan_type(data, i);
> - if (err >= 0)
> - data->fan[i] = true;
> +
> + if (err < 0)
> + continue;
> +
> + data->fan[i] = true;
> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
> + sizeof(*data->fan_nominal_speed[i]),
> + GFP_KERNEL);
> + if (!data->fan_nominal_speed[i])
> + continue;
> +
> + for (state = 0; state <= data->i8k_fan_max; state++) {
> + err = i8k_get_fan_nominal_speed(data, i, state);
> + if (err < 0) {
> + /* Mark nominal speed table as invalid in case of error */
> + devm_kfree(dev, data->fan_nominal_speed[i]);
> + data->fan_nominal_speed[i] = NULL;
> + break;
> + }
> + data->fan_nominal_speed[i][state] = err;
> + }
> }
>
> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-13 18:51 ` Guenter Roeck
@ 2021-10-16 21:25 ` Armin Wolf
2021-10-16 22:00 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Armin Wolf @ 2021-10-16 21:25 UTC (permalink / raw)
To: Guenter Roeck; +Cc: pali, jdelvare, linux-hwmon
Am 13.10.21 um 20:51 schrieb Guenter Roeck:
> On Mon, Sep 27, 2021 at 12:10:43AM +0200, W_Armin@gmx.de wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> The nominal speed of each fan can be obtained with
>> i8k_get_fan_nominal_speed(), however the result is not available
>> from userspace.
>> Change that by adding fanX_min, fanX_max and fanX_target attributes.
>> All are RO since fan control happens over pwm.
>>
>> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> Reviewed-by: Pali Rohár <pali@kernel.org>
> Applied.
>
> Thanks,
> Guenter
>
>> ---
>> Documentation/hwmon/dell-smm-hwmon.rst | 3 ++
>> drivers/hwmon/dell-smm-hwmon.c | 61 +++++++++++++++++++++++---
>> 2 files changed, 58 insertions(+), 6 deletions(-)
>>
>> --
>> 2.20.1
>>
>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>> index 3bf77a5df995..beec88491171 100644
>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>> @@ -34,6 +34,9 @@ Name Perm Description
>> =============================== ======= =======================================
>> fan[1-3]_input RO Fan speed in RPM.
>> fan[1-3]_label RO Fan label.
>> +fan[1-3]_min RO Minimal Fan speed in RPM
>> +fan[1-3]_max RO Maximal Fan speed in RPM
>> +fan[1-3]_target RO Expected Fan speed in RPM
>> pwm[1-3] RW Control the fan PWM duty-cycle.
>> pwm1_enable WO Enable or disable automatic BIOS fan
>> control (not supported on all laptops,
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 774c1b0715d9..476f2a74bd8c 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -76,6 +76,7 @@ struct dell_smm_data {
>> int temp_type[DELL_SMM_NO_TEMP];
>> bool fan[DELL_SMM_NO_FANS];
>> int fan_type[DELL_SMM_NO_FANS];
>> + int *fan_nominal_speed[DELL_SMM_NO_FANS];
>> };
>>
>> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>> @@ -673,6 +674,13 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>> if (data->fan[channel] && !data->disallow_fan_type_call)
>> return 0444;
>>
>> + break;
>> + case hwmon_fan_min:
>> + case hwmon_fan_max:
>> + case hwmon_fan_target:
>> + if (data->fan_nominal_speed[channel])
>> + return 0444;
>> +
>> break;
>> default:
>> break;
>> @@ -740,6 +748,25 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>>
>> *val = ret;
>>
>> + return 0;
>> + case hwmon_fan_min:
>> + *val = data->fan_nominal_speed[channel][0];
>> +
>> + return 0;
>> + case hwmon_fan_max:
>> + *val = data->fan_nominal_speed[channel][data->i8k_fan_max];
>> +
>> + return 0;
>> + case hwmon_fan_target:
>> + ret = i8k_get_fan_status(data, channel);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (ret > data->i8k_fan_max)
>> + ret = data->i8k_fan_max;
>> +
>> + *val = data->fan_nominal_speed[channel][ret];
>> +
>> return 0;
>> default:
>> break;
>> @@ -889,9 +916,12 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
>> HWMON_T_INPUT | HWMON_T_LABEL
>> ),
>> HWMON_CHANNEL_INFO(fan,
>> - HWMON_F_INPUT | HWMON_F_LABEL,
>> - HWMON_F_INPUT | HWMON_F_LABEL,
>> - HWMON_F_INPUT | HWMON_F_LABEL
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET,
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET,
>> + HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MIN | HWMON_F_MAX |
>> + HWMON_F_TARGET
>> ),
>> HWMON_CHANNEL_INFO(pwm,
>> HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>> @@ -910,7 +940,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>> {
>> struct dell_smm_data *data = dev_get_drvdata(dev);
>> struct device *dell_smm_hwmon_dev;
>> - int i, err;
>> + int i, state, err;
>>
>> for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
>> data->temp_type[i] = i8k_get_temp_type(i);
>> @@ -926,8 +956,27 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>> err = i8k_get_fan_status(data, i);
>> if (err < 0)
>> err = i8k_get_fan_type(data, i);
>> - if (err >= 0)
>> - data->fan[i] = true;
>> +
>> + if (err < 0)
>> + continue;
>> +
>> + data->fan[i] = true;
>> + data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
>> + sizeof(*data->fan_nominal_speed[i]),
>> + GFP_KERNEL);
>> + if (!data->fan_nominal_speed[i])
>> + continue;
>> +
>> + for (state = 0; state <= data->i8k_fan_max; state++) {
>> + err = i8k_get_fan_nominal_speed(data, i, state);
>> + if (err < 0) {
>> + /* Mark nominal speed table as invalid in case of error */
>> + devm_kfree(dev, data->fan_nominal_speed[i]);
>> + data->fan_nominal_speed[i] = NULL;
>> + break;
>> + }
>> + data->fan_nominal_speed[i][state] = err;
>> + }
>> }
>>
>> dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
After looking at hwmon-next today, i noticed that this patch was
apparently removed.
Is there a issue with this patch causing the revert?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-16 21:25 ` Armin Wolf
@ 2021-10-16 22:00 ` Guenter Roeck
2021-10-16 22:09 ` Armin Wolf
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2021-10-16 22:00 UTC (permalink / raw)
To: Armin Wolf; +Cc: pali, jdelvare, linux-hwmon
On 10/16/21 2:25 PM, Armin Wolf wrote:
> Am 13.10.21 um 20:51 schrieb Guenter Roeck:
>> On Mon, Sep 27, 2021 at 12:10:43AM +0200, W_Armin@gmx.de wrote:
>>> From: Armin Wolf <W_Armin@gmx.de>
>>>
>>> The nominal speed of each fan can be obtained with
>>> i8k_get_fan_nominal_speed(), however the result is not available
>>> from userspace.
>>> Change that by adding fanX_min, fanX_max and fanX_target attributes.
>>> All are RO since fan control happens over pwm.
>>>
>>> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>> Applied.
>>
[ ... ]
> After looking at hwmon-next today, i noticed that this patch was
> apparently removed.
> Is there a issue with this patch causing the revert?
Not that I recall. Looks like I messed up. Sorry, and thanks for noticing.
I added it back in.
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-16 22:00 ` Guenter Roeck
@ 2021-10-16 22:09 ` Armin Wolf
2021-10-17 5:48 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Armin Wolf @ 2021-10-16 22:09 UTC (permalink / raw)
To: Guenter Roeck; +Cc: pali, jdelvare, linux-hwmon
Am 17.10.21 um 00:00 schrieb Guenter Roeck:
> On 10/16/21 2:25 PM, Armin Wolf wrote:
>> Am 13.10.21 um 20:51 schrieb Guenter Roeck:
>>> On Mon, Sep 27, 2021 at 12:10:43AM +0200, W_Armin@gmx.de wrote:
>>>> From: Armin Wolf <W_Armin@gmx.de>
>>>>
>>>> The nominal speed of each fan can be obtained with
>>>> i8k_get_fan_nominal_speed(), however the result is not available
>>>> from userspace.
>>>> Change that by adding fanX_min, fanX_max and fanX_target attributes.
>>>> All are RO since fan control happens over pwm.
>>>>
>>>> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>> Applied.
>>>
>
> [ ... ]
>
>> After looking at hwmon-next today, i noticed that this patch was
>> apparently removed.
>> Is there a issue with this patch causing the revert?
>
> Not that I recall. Looks like I messed up. Sorry, and thanks for
> noticing.
> I added it back in.
>
> Guenter
It seems its not the only patch missing, i cant find the following
patches anymore:
[PATCH v2] hwmon: acpi_power_meter: Use acpi_bus_get_acpi_device()
[PATCH v2 04/20 hwmon: max31722: Warn about failure to put device in
stand-by in .remove()
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target
2021-10-16 22:09 ` Armin Wolf
@ 2021-10-17 5:48 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-10-17 5:48 UTC (permalink / raw)
To: Armin Wolf; +Cc: pali, jdelvare, linux-hwmon
On 10/16/21 3:09 PM, Armin Wolf wrote:
> Am 17.10.21 um 00:00 schrieb Guenter Roeck:
>> On 10/16/21 2:25 PM, Armin Wolf wrote:
>>> Am 13.10.21 um 20:51 schrieb Guenter Roeck:
>>>> On Mon, Sep 27, 2021 at 12:10:43AM +0200, W_Armin@gmx.de wrote:
>>>>> From: Armin Wolf <W_Armin@gmx.de>
>>>>>
>>>>> The nominal speed of each fan can be obtained with
>>>>> i8k_get_fan_nominal_speed(), however the result is not available
>>>>> from userspace.
>>>>> Change that by adding fanX_min, fanX_max and fanX_target attributes.
>>>>> All are RO since fan control happens over pwm.
>>>>>
>>>>> Tested on a Dell Inspiron 3505 and a Dell Latitude C600.
>>>>>
>>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>>> Applied.
>>>>
>>
>> [ ... ]
>>
>>> After looking at hwmon-next today, i noticed that this patch was
>>> apparently removed.
>>> Is there a issue with this patch causing the revert?
>>
>> Not that I recall. Looks like I messed up. Sorry, and thanks for
>> noticing.
>> I added it back in.
>>
>> Guenter
> It seems its not the only patch missing, i cant find the following
> patches anymore:
>
> [PATCH v2] hwmon: acpi_power_meter: Use acpi_bus_get_acpi_device()
> [PATCH v2 04/20 hwmon: max31722: Warn about failure to put device in
> stand-by in .remove()
>
You are right. I added them back in. Must have had a blackout or something :-(.
Thanks a lot for checking!
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-10-17 5:48 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-26 22:10 [PATCH v3 0/2] hwmon: (dell-smm) Support additional attributes W_Armin
2021-09-26 22:10 ` [PATCH v3 1/2] hwmon: (dell-smm) Add support for fanX_min, fanX_max and fanX_target W_Armin
2021-10-10 21:18 ` Armin Wolf
2021-10-11 14:27 ` Guenter Roeck
2021-10-11 16:11 ` Pali Rohár
2021-10-11 18:48 ` Guenter Roeck
2021-10-11 19:14 ` Armin Wolf
2021-10-13 14:38 ` Pali Rohár
2021-10-13 14:38 ` Pali Rohár
2021-10-13 18:51 ` Guenter Roeck
2021-10-16 21:25 ` Armin Wolf
2021-10-16 22:00 ` Guenter Roeck
2021-10-16 22:09 ` Armin Wolf
2021-10-17 5:48 ` Guenter Roeck
2021-09-26 22:10 ` [PATCH v3 2/2] hwmon: (dell-smm) Remove unnecessary includes W_Armin
2021-10-08 21:07 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.