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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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@ew.tq-group.com>
> >>>> ---
> >>>> 
> >>>>    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@pengutronix.de/