* [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs @ 2021-09-03 14:24 W_Armin 2021-09-03 14:49 ` Pali Rohár 0 siblings, 1 reply; 5+ messages in thread From: W_Armin @ 2021-09-03 14:24 UTC (permalink / raw) To: pali; +Cc: linux, jdelvare, linux-hwmon From: Armin Wolf <W_Armin@gmx.de> For allowing tools like i8kutils to query the fan state without having to rely on the deprecated /proc/i8k interface, they need to scale the pwm values (0 - 255) back to the real hardware values (0 - 2/3). Show fan_max in sysfs to allow for such a scenario. Tested on a Dell Latitude C600. Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/hwmon/dell-smm-hwmon.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c index 774c1b0715d9..6d3fd4f0f99d 100644 --- a/drivers/hwmon/dell-smm-hwmon.c +++ b/drivers/hwmon/dell-smm-hwmon.c @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0); MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)"); static uint fan_max; -module_param(fan_max, uint, 0); +module_param(fan_max, uint, 0444); MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)"); struct smm_regs { @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct platform_device *pdev) fan_max = conf->fan_max; } - data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be 0 */ + if (!fan_max) /* Must not be 0*/ + fan_max = I8K_FAN_HIGH; + + data->i8k_fan_max = fan_max; data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max); fan_control = dmi_first_match(i8k_whitelist_fan_control); -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs 2021-09-03 14:24 [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs W_Armin @ 2021-09-03 14:49 ` Pali Rohár 2021-09-03 14:59 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Pali Rohár @ 2021-09-03 14:49 UTC (permalink / raw) To: linux, W_Armin; +Cc: jdelvare, linux-hwmon On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote: > From: Armin Wolf <W_Armin@gmx.de> > > For allowing tools like i8kutils to query the fan state > without having to rely on the deprecated /proc/i8k interface, > they need to scale the pwm values (0 - 255) back to the > real hardware values (0 - 2/3). > Show fan_max in sysfs to allow for such a scenario. Guenter, I think that this is general problem and not specific to dell-smm-hwmon.c driver and i8kutils. All other hwmon tools should have similar problem. If e.g. GUI tool has slider for controlling pwm then such tool needs to know number of steps. Otherwise usage of such slider would be not very user friendly... Currently in hwmon sysfs API there is not attribute which could export this kind of information. What about e.g. introducing 'pwm_steps' attribute which would provide this information? > Tested on a Dell Latitude C600. > > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/hwmon/dell-smm-hwmon.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c > index 774c1b0715d9..6d3fd4f0f99d 100644 > --- a/drivers/hwmon/dell-smm-hwmon.c > +++ b/drivers/hwmon/dell-smm-hwmon.c > @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0); > MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)"); > > static uint fan_max; > -module_param(fan_max, uint, 0); > +module_param(fan_max, uint, 0444); > MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)"); > > struct smm_regs { > @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct platform_device *pdev) > fan_max = conf->fan_max; > } > > - data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be 0 */ > + if (!fan_max) /* Must not be 0*/ > + fan_max = I8K_FAN_HIGH; > + > + data->i8k_fan_max = fan_max; > data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max); > > fan_control = dmi_first_match(i8k_whitelist_fan_control); > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs 2021-09-03 14:49 ` Pali Rohár @ 2021-09-03 14:59 ` Guenter Roeck 2021-09-03 15:40 ` Armin Wolf 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2021-09-03 14:59 UTC (permalink / raw) To: Pali Rohár, W_Armin; +Cc: jdelvare, linux-hwmon On 9/3/21 7:49 AM, Pali Rohár wrote: > On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote: >> From: Armin Wolf <W_Armin@gmx.de> >> >> For allowing tools like i8kutils to query the fan state >> without having to rely on the deprecated /proc/i8k interface, >> they need to scale the pwm values (0 - 255) back to the >> real hardware values (0 - 2/3). >> Show fan_max in sysfs to allow for such a scenario. > > Guenter, I think that this is general problem and not specific to > dell-smm-hwmon.c driver and i8kutils. > > All other hwmon tools should have similar problem. If e.g. GUI tool has > slider for controlling pwm then such tool needs to know number of steps. > Otherwise usage of such slider would be not very user friendly... > > Currently in hwmon sysfs API there is not attribute which could export > this kind of information. > > What about e.g. introducing 'pwm_steps' attribute which would provide > this information? > It isn't really a problem. The problem is that the tool writers insist not following the ABI. All they would have to do is to use a scale of 0..255, read back any written pwm value, and adjust the displayed value based on the returned value. I do not see why this would be "not user friendly". An attribute such as pwm_steps would not work because the step size can be non-linear. Guenter >> Tested on a Dell Latitude C600. >> >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >> --- >> drivers/hwmon/dell-smm-hwmon.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c >> index 774c1b0715d9..6d3fd4f0f99d 100644 >> --- a/drivers/hwmon/dell-smm-hwmon.c >> +++ b/drivers/hwmon/dell-smm-hwmon.c >> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0); >> MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)"); >> >> static uint fan_max; >> -module_param(fan_max, uint, 0); >> +module_param(fan_max, uint, 0444); >> MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)"); >> >> struct smm_regs { >> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct platform_device *pdev) >> fan_max = conf->fan_max; >> } >> >> - data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be 0 */ >> + if (!fan_max) /* Must not be 0*/ >> + fan_max = I8K_FAN_HIGH; >> + >> + data->i8k_fan_max = fan_max; >> data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max); >> >> fan_control = dmi_first_match(i8k_whitelist_fan_control); >> -- >> 2.20.1 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs 2021-09-03 14:59 ` Guenter Roeck @ 2021-09-03 15:40 ` Armin Wolf 2021-09-03 17:05 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Armin Wolf @ 2021-09-03 15:40 UTC (permalink / raw) To: Guenter Roeck, Pali Rohár; +Cc: jdelvare, linux-hwmon Am 03.09.21 um 16:59 schrieb Guenter Roeck: > On 9/3/21 7:49 AM, Pali Rohár wrote: >> On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote: >>> From: Armin Wolf <W_Armin@gmx.de> >>> >>> For allowing tools like i8kutils to query the fan state >>> without having to rely on the deprecated /proc/i8k interface, >>> they need to scale the pwm values (0 - 255) back to the >>> real hardware values (0 - 2/3). >>> Show fan_max in sysfs to allow for such a scenario. >> >> Guenter, I think that this is general problem and not specific to >> dell-smm-hwmon.c driver and i8kutils. >> >> All other hwmon tools should have similar problem. If e.g. GUI tool has >> slider for controlling pwm then such tool needs to know number of steps. >> Otherwise usage of such slider would be not very user friendly... >> >> Currently in hwmon sysfs API there is not attribute which could export >> this kind of information. >> >> What about e.g. introducing 'pwm_steps' attribute which would provide >> this information? >> > > It isn't really a problem. The problem is that the tool writers insist > not following the ABI. All they would have to do is to use a scale of > 0..255, > read back any written pwm value, and adjust the displayed value based > on the returned value. I do not see why this would be "not user > friendly". > > An attribute such as pwm_steps would not work because the step size > can be > non-linear. > > Guenter > i8kutils uses values from 0 to 2/3 for historic reasons, as the deprecated /proc/i8k interface provided such values. In order to not break backwards compatibility, they cannot simply switch to standard pwm values (ithink). I believe that showing fan_max via sysfs would allow i8kutils to finally use the standard interface, since when this happens, we maybe can remove the /proc/i8k interface. >>> Tested on a Dell Latitude C600. >>> >>> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >>> --- >>> drivers/hwmon/dell-smm-hwmon.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwmon/dell-smm-hwmon.c >>> b/drivers/hwmon/dell-smm-hwmon.c >>> index 774c1b0715d9..6d3fd4f0f99d 100644 >>> --- a/drivers/hwmon/dell-smm-hwmon.c >>> +++ b/drivers/hwmon/dell-smm-hwmon.c >>> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0); >>> MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with >>> (default: autodetect)"); >>> >>> static uint fan_max; >>> -module_param(fan_max, uint, 0); >>> +module_param(fan_max, uint, 0444); >>> MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed >>> (default: autodetect)"); >>> >>> struct smm_regs { >>> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct >>> platform_device *pdev) >>> fan_max = conf->fan_max; >>> } >>> >>> - data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be >>> 0 */ >>> + if (!fan_max) /* Must not be 0*/ >>> + fan_max = I8K_FAN_HIGH; >>> + >>> + data->i8k_fan_max = fan_max; >>> data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max); >>> >>> fan_control = dmi_first_match(i8k_whitelist_fan_control); >>> -- >>> 2.20.1 >>> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs 2021-09-03 15:40 ` Armin Wolf @ 2021-09-03 17:05 ` Guenter Roeck 0 siblings, 0 replies; 5+ messages in thread From: Guenter Roeck @ 2021-09-03 17:05 UTC (permalink / raw) To: Armin Wolf, Pali Rohár; +Cc: jdelvare, linux-hwmon On 9/3/21 8:40 AM, Armin Wolf wrote: > Am 03.09.21 um 16:59 schrieb Guenter Roeck: >> On 9/3/21 7:49 AM, Pali Rohár wrote: >>> On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote: >>>> From: Armin Wolf <W_Armin@gmx.de> >>>> >>>> For allowing tools like i8kutils to query the fan state >>>> without having to rely on the deprecated /proc/i8k interface, >>>> they need to scale the pwm values (0 - 255) back to the >>>> real hardware values (0 - 2/3). >>>> Show fan_max in sysfs to allow for such a scenario. >>> >>> Guenter, I think that this is general problem and not specific to >>> dell-smm-hwmon.c driver and i8kutils. >>> >>> All other hwmon tools should have similar problem. If e.g. GUI tool has >>> slider for controlling pwm then such tool needs to know number of steps. >>> Otherwise usage of such slider would be not very user friendly... >>> >>> Currently in hwmon sysfs API there is not attribute which could export >>> this kind of information. >>> >>> What about e.g. introducing 'pwm_steps' attribute which would provide >>> this information? >>> >> >> It isn't really a problem. The problem is that the tool writers insist >> not following the ABI. All they would have to do is to use a scale of >> 0..255, >> read back any written pwm value, and adjust the displayed value based >> on the returned value. I do not see why this would be "not user >> friendly". >> >> An attribute such as pwm_steps would not work because the step size >> can be >> non-linear. >> >> Guenter >> > i8kutils uses values from 0 to 2/3 for historic reasons, as the > deprecated /proc/i8k interface > provided such values. In order to not break backwards compatibility, > they cannot simply > switch to standard pwm values (ithink). > I believe that showing fan_max via sysfs would allow i8kutils to finally > use the standard interface, > since when this happens, we maybe can remove the /proc/i8k interface. No, we can't, because that is an ABI, and we can not remove it because someone might insist using an old version of i8kutils with a new kernel. Also, fan_max is _not_ a 'standard' interface. If i8kutils wants to use fan_max, it can get it from the existing proc interface. Besides, fan_max is repurposed here: So far it is a module parameter. With the proposed changes in place, it would no longer be a module parameter but a sysfs attribute disguised as module parameter that is abused to report a value (one changed by the driver) to userspace. Guenter >>>> Tested on a Dell Latitude C600. >>>> >>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >>>> --- >>>> drivers/hwmon/dell-smm-hwmon.c | 7 +++++-- >>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c >>>> b/drivers/hwmon/dell-smm-hwmon.c >>>> index 774c1b0715d9..6d3fd4f0f99d 100644 >>>> --- a/drivers/hwmon/dell-smm-hwmon.c >>>> +++ b/drivers/hwmon/dell-smm-hwmon.c >>>> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0); >>>> MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with >>>> (default: autodetect)"); >>>> >>>> static uint fan_max; >>>> -module_param(fan_max, uint, 0); >>>> +module_param(fan_max, uint, 0444); >>>> MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed >>>> (default: autodetect)"); >>>> >>>> struct smm_regs { >>>> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct >>>> platform_device *pdev) >>>> fan_max = conf->fan_max; >>>> } >>>> >>>> - data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be >>>> 0 */ >>>> + if (!fan_max) /* Must not be 0*/ >>>> + fan_max = I8K_FAN_HIGH; >>>> + >>>> + data->i8k_fan_max = fan_max; >>>> data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max); >>>> >>>> fan_control = dmi_first_match(i8k_whitelist_fan_control); >>>> -- >>>> 2.20.1 >>>> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-03 17:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-03 14:24 [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs W_Armin 2021-09-03 14:49 ` Pali Rohár 2021-09-03 14:59 ` Guenter Roeck 2021-09-03 15:40 ` Armin Wolf 2021-09-03 17:05 ` 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.