Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/17/22 09:57, Uwe Kleine-König wrote:
Hello,

[dropping Bartlomiej Zolnierkiewicz from Cc as the address bounces]

On Tue, May 17, 2022 at 09:32:24AM -0700, Guenter Roeck wrote:
On 5/17/22 07:53, Uwe Kleine-König wrote:
Hello,

On Tue, May 17, 2022 at 04:26:20PM +0200, 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

Is the pwm1_enable supposed to be set to 0 if that only does the right
thing if the PWM emits low after pwm_disable()? The question I raised in
v2 about "what is the meaning of disable?" hasn't evolved, has it?

I still think it's unfortunate, that "pwm1_enable" has an effect on the
regulator.


Trying to understand. Are you saying that you are ok with affecting the
regulator when setting pwm := 0 (even though that doesn't really mean
"disable pwm output"), but not with making the behavior explicit by
using pwm1_enable ?

Not sure about being ok with affecting the regulator when setting
pwm := 0. I don't know enough about pwm-fans to have a strong opinion
for that.

Some questions to refine the semantics and my opinion:

There are fans without a regulator? (I think: yes)

A fan with a regulator always stops if the regulator is off?
(Hmm, there might be problems with shared regulators that only go off
when all consumers are off? What about always-on regulators, these don't
go off on the last consumer calling disable, right?)

Having said that I think the sane behaviour is:

The intention of pwm := 0 is to stop the fan. So disabling the regulator
(if available) sounds right.


There are fans (eg at least some CPU fans) which never stop, even with
pwm=0. How do you suggest to handle those ?

Guenter

I'm unsure what to reasonably expect from a disabled PWM. I think "stops
to oscillate" can be assumed. So I'd say: If a fan continues to rotate
when the PWM input is constantly active, don't call pwm_disable().

Best regards
Uwe





[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux