Re: (EXT) 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: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@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
> > 
> > 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, 
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'?

> 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?

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