| Message ID | 20220517142620.1523143-7-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: pwm-fan: switch regulator dynamically | expand |
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. Best regards Uwe
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 ? Thanks, Guenter
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 plus possibly: - regulator disabled, pwm output disabled if pwm=0 Guenter
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. 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
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@pengutronix.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. 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? Best regards Uwe
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. > > 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 >
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/
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
On 5/18/22 00:06, Alexander Stein wrote: > 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, Correct. > 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'? > No. pwmX_mode sets the direct current vs. pulse width. >> 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? > 0 is for disable, not enable, and 1 should match the current implementation for compatibility reasons. Something like 0 -> disable pwm and regulator 1 -> enable pwm; if pwm==0, disable pwm, keep regulator enabled 2 -> enable pwm; if pwm==0, keep pwm and regulator enabled 3 -> enable pwm; if pwm==0, disable pwm and regulator should work. Guenter > 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 > >
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 +pwm1 rw relative speed (0-255), 255=max. speed. +=============== ======= ======================================================= diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index 9ebe958cc908..cb29206ddcdc 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -35,6 +35,7 @@ struct pwm_fan_ctx { struct pwm_device *pwm; struct regulator *reg_en; bool enabled; + bool keep_enabled; int tach_count; struct pwm_fan_tach *tachs; @@ -129,7 +130,8 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx) return 0; pwm_get_state(ctx->pwm, &state); - state.enabled = false; + if (!ctx->keep_enabled) + state.enabled = false; state.duty_cycle = 0; pwm_apply_state(ctx->pwm, &state); @@ -178,20 +180,87 @@ static void pwm_fan_update_state(struct pwm_fan_ctx *ctx, unsigned long pwm) ctx->pwm_fan_state = i; } +static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val) +{ + struct pwm_state old_state; + int ret; + + pwm_get_state(ctx->pwm, &old_state); + + if (val) { + /* + * If PWM is already enabled, nothing will change + * If PWM is disabled, it will enable with inactive level (duty == 0) + */ + ret = pwm_fan_enable_pwm(ctx); + if (ret) { + dev_err(ctx->dev, "failed to apply PWM state\n"); + return ret; + } + + /* Increase regulator reference counter to prevent switching off */ + ret = regulator_enable(ctx->reg_en); + if (ret < 0) { + dev_err(ctx->dev, "failed to enable regulator\n"); + /* Restore old state */ + pwm_apply_state(ctx->pwm, &old_state); + } + } else { + /* Only disable PWM if currently on inactive state */ + if (!ctx->pwm_value) { + struct pwm_state disable_state; + + pwm_get_state(ctx->pwm, &disable_state); + disable_state.duty_cycle = 0; + disable_state.enabled = false; + ret = pwm_apply_state(ctx->pwm, &disable_state); + if (ret) { + dev_err(ctx->dev, "failed to apply PWM state\n"); + return ret; + } + } + /* Decrease regulator reference counter */ + ret = regulator_disable(ctx->reg_en); + if (ret < 0) { + dev_err(ctx->dev, "failed to decrease power supply usage\n"); + /* Restore old state */ + pwm_apply_state(ctx->pwm, &old_state); + } + } + if (ret == 0) + ctx->keep_enabled = val; + + return ret; +} + static int pwm_fan_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, long val) { struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); int ret; - if (val < 0 || val > MAX_PWM) - return -EINVAL; + switch (attr) { + case hwmon_pwm_input: + if (val < 0 || val > MAX_PWM) + return -EINVAL; + ret = __set_pwm(ctx, val); + if (ret) + return ret; + pwm_fan_update_state(ctx, val); + break; + case hwmon_pwm_enable: + mutex_lock(&ctx->lock); + if (val < 0 || val > 1) + ret = -EINVAL; + else if (ctx->keep_enabled != val) + ret = pwm_fan_update_enable(ctx, val); + mutex_unlock(&ctx->lock); - ret = __set_pwm(ctx, val); - if (ret) return ret; + default: + return -EOPNOTSUPP; + } - pwm_fan_update_state(ctx, val); return 0; } @@ -202,9 +271,15 @@ static int pwm_fan_read(struct device *dev, enum hwmon_sensor_types type, switch (type) { case hwmon_pwm: - *val = ctx->pwm_value; - return 0; - + switch (attr) { + case hwmon_pwm_input: + *val = ctx->pwm_value; + return 0; + case hwmon_pwm_enable: + *val = ctx->keep_enabled; + return 0; + } + return -EOPNOTSUPP; case hwmon_fan: *val = ctx->tachs[channel].rpm; return 0; @@ -436,7 +511,7 @@ static int pwm_fan_probe(struct platform_device *pdev) if (!channels) return -ENOMEM; - channels[0] = HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT); + channels[0] = HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE); for (i = 0; i < ctx->tach_count; i++) { struct pwm_fan_tach *tach = &ctx->tachs[i];
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(-)