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. 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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
