On 5/18/22 00:06, Alexander Stein wrote:
Am Dienstag, 17. Mai 2022, 19:06:58 CEST schrieb Uwe Kleine-König:
* PGP Signed by an unknown key
Hello,
[dropped Bartlomiej Zolnierkiewicz from Cc:]
On Tue, May 17, 2022 at 09:38:56AM -0700, Guenter Roeck wrote:
On 5/17/22 07:26, Alexander Stein wrote:
This adds the enable attribute which is used to differentiate if PWM
duty
means to switch off regulator and PWM or to keep them enabled but
at inactive PWM output level.
Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
---
Documentation/hwmon/pwm-fan.rst | 10 ++++
drivers/hwmon/pwm-fan.c | 95 +++++++++++++++++++++++++++++----
2 files changed, 95 insertions(+), 10 deletions(-)
diff --git a/Documentation/hwmon/pwm-fan.rst
b/Documentation/hwmon/pwm-fan.rst index 82fe96742fee..0083480068d1
100644
--- a/Documentation/hwmon/pwm-fan.rst
+++ b/Documentation/hwmon/pwm-fan.rst
@@ -18,3 +18,13 @@ the hwmon's sysfs interface.
The fan rotation speed returned via the optional 'fan1_input' is
extrapolated from the sampled interrupts from the tachometer signal
within 1 second.> >
+
+The driver provides the following sensor accesses in sysfs:
+
+=============== =======
=======================================================
+fan1_input ro fan tachometer speed in RPM
+pwm1_enable rw keep enable mode, defines behaviour when
pwm1=0
+ 0=switch off regulator and disable PWM
+ 1=keep regulator enabled and set PWM to
inactive level
Unless I am missing something, I think we have (at least) three
conditions to handle:
- regulator disabled (independent of pwm value)
- regulator enabled, pwm output disabled if pwm=0
- regulator enabled, pwm output enabled and set to 0 (or, if inverted,
to maximum) if pwm=0
What is your expectation for a disabled PWM?
https://lore.kernel.org/linux-pwm/20220517150555.404363-1-u.kleine-koenig@pe
ngutronix.de might be relevant. If you assume that a pwm might output the
active level after disabling, the case "regulator enabled, pwm output
disabled if pwm=0" sounds wrong.
Would "pwm1_disable_on_zero" be a better name than "pwm1_enable"? The
latter is completely unintuitive to me.
I guess Guenter suggested 'pwm1_enable' as it already exists as a predefined,
Correct.
optional attribute, avoiding adding a new custom attribute.
Reading Documentation/hwmon/w83627ehf.rst or Documentation/hwmon/nzxt-
smart2.rst I get the impression their meaning is pretty unrestricted.
If you are concerned by using 'pwm1_enable', what about 'pwm1_mode'?
No. pwmX_mode sets the direct current vs. pulse width.
Maybe go for
0 -> keep pwm and regulator on
1 -> disable pwm, keep regulator on
2 -> keep pwm on, disable regulator
3 -> disable pwm and regulator
(so one bit for pwm and one for regulator), even if 1 is
wrong/unusual/dangerous?
0 is for disable, not enable, and 1 should match the current implementation
for compatibility reasons. Something like
0 -> disable pwm and regulator
1 -> enable pwm; if pwm==0, disable pwm, keep regulator enabled
2 -> enable pwm; if pwm==0, keep pwm and regulator enabled
3 -> enable pwm; if pwm==0, disable pwm and regulator
should work.
Guenter
I tend to like this approach, as it can handle all combinations. You can
decide whether you want to actually shutdown the PWM fan, or keep it enabled
but without providing any PWM. This can mean the fan still runs at the lowest
speed. It also addresses the scenarios where regulator cannot be disabled at
all.
Best regards,
Alexander
