Am Dienstag, 17. Mai 2022, 19:32:11 CEST schrieb Guenter Roeck: > 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. In my case (422J fan) just supplying voltage with inactive PWM results in a minimum rotation speed. So these two settings are coupled here. > > 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?) IMHO this is something the system integrator shall manage. Is it possible to disable the regulator? No for shared/always-on ones. In this case stopping the fan needs to be done by setting PWM to inactive level and keep regulator on (pwm1_enable=1). If this results in a minimum rotation speed as it would using a 422J, it's pretty much impossible to actually stop the fan in such a system. > > 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 ? If it's impossible to stop the fan from the hardware side (e.g. always-on regulators), then software only can do this much. Maybe adding those 4 states Uwe mentioned in [1] is not a bad idea. This way it's possible to configure the exact behavior one would want/need. This also can handle the cases where a disabled PWM has no/wrong defined state. Best regards Alexander [1] https://lore.kernel.org/all/ 20220517170658.u3dpe6gglsihh6n6@xxxxxxxxxxxxxx/
