Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator

[Guenter Roeck <linux@roeck-us.net>]

On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote:
> Hello Uwe,
> 
> Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König:
> > * PGP Signed by an unknown key
> > 
> > On Fri, May 06, 2022 at 10:35:21AM +0200, Alexander Stein wrote:
> > > Am Freitag, 6. Mai 2022, 10:20:01 CEST schrieb Uwe Kleine-König:
> > > > > Old Signed by an unknown key
> > > > 
> > > > Hello,
> > > > 
> > > > On Fri, May 06, 2022 at 09:15:55AM +0200, Alexander Stein wrote:
> > > > > Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck:
> > > > > > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote:
> > > > > > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > > > > > > 
> > > > > > > A pwm value equal to zero is meant to switch off the pwm
> > > > > > > hence also switching off the fan. Currently the optional
> > > > > > > regulator is always on. When using this driver on boards
> > > > > > > with an inverted pwm signal polarity this can cause running
> > > > > > > the fan at maximum speed when setting pwm to zero.
> > > > > > 
> > > > > > The appropriate solution in this case would be to tell the
> > > > > > software that the pwm is inverted. Turning off the regulator
> > > > > > in that situation is a bad idea since setting the pwm value to
> > > > > > 1 would set it to almost full speed. That does not really make
> > > > > > sense.
> > > > > 
> > > > > The pwm-fan driver is already configured for inverted PWM (ommited
> > > > > some
> > > > > properties for shortness):
> > > > > fan0: pwm-fan {
> > > > > 
> > > > > 	compatible = "pwm-fan";
> > > > > 	fan-supply = <&reg_pwm_fan>;
> > > > > 	pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>;
> > > > > 	cooling-levels = <0 32 64 128 196 240>;
> > > > > 
> > > > > [...]
> > > > > };
> > > > > 
> > > > > The problem here is that the pwm-fan driver currently enables the
> > > > > regulator
> > > > > unconditionally, but the PWM only when the fan is enabled, refer to
> > > > > __set_pwm(). This results in a fan at full speed when pwm-fan is idle,
> > > > > as
> > > > > pwm state is not enabled.
> > > > 
> > > > Which PWM driver are you using?
> > > 
> > > It's pwm-imx27 on a imx8mp based board.
> > 
> > This is one of the known offenders.
> > 
> > > > There is an implicit assumption in some PWM consumers that a disabled
> > > > PWM emits the inactive level. However not all PWMs do this. Is this such
> > > > a case?
> > > 
> > > Oh, I was not aware of that assumption. As far I can tell, this assumption
> > > might actually be violated in pwm-imx27.
> > 
> > The pwm-imx27 driver is a known offender.
> > 
> > IMHO the problem is that there is no properly documented definition what
> > "disabled" means for a PWM. I had some discussions about that in the
> > past with Thierry, but with no agreement. Either we have do define that
> > the output of a PWM is undefined when it's disabled (then the pwm-fan
> > needs fixing) or the output must be the inactive level (then the
> > pwm-imx27 must be fixed to not unset the EN bit when configured for an
> > inverted polarity). In my eyes the first is the sensible thing to do.
> > 
> > See
> > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutroni
> > x.de/ for one of the previous discussions.
> 
> Thanks for the link. I took a look into it. I'm on your side here, IMHO 
> pwm_disable() implies that the PWM perphery is disabled, including any clocks 
> or powerdomain. This is what pwm-imx27 actually does. This might lead to a, 
> probably platform dependent, (undefined?) state of the PWM output pin.
> This implies it is not possible to disable the PWM periphery for inverted 
> signals, if the disabled state is not the inactive level. You know all about 
> it already.
> Then again from pwm-fan side I want be able to disable the FAN, turning of 
> regulator and PWM, so powersaving is possible. That's what this patch is 
> about. This is similar also what pwm_bl is doing.
> Independent of the exact semantics, it makes sense to disable the regulator in 
> pwm-fan as well when the fan shall be disabled.
> 

There are fans which never stop if pwm==0, such as some CPU fans. I don't
think it is a good idea to force those off by turning off their power. The
problem in the driver is that it treats pwm==0 as "disable pwm", not as
"set pwm output to 0", Part of the probem may be that the ABI doesn't have
a good representation for "disable pwm output", which is what is really
wanted/needed here. I think the best solution would be to implement and
use pwmX_enable, and define in the driver documentation that pwm1_enable=0
reflects "disable pwm" and pwm1_enable=1 reflects "emable manual pwm
control:. At the same time, stop associating "pwm==0" with "disable pwm",
but just set the pwm output value to 0.

Guenter

> > > If state->enabled==false then the EN Bit in PWMCR is not set which most
> > > probably renders the output polarity in POUTC as inactive.
> > 
> > It drives the output to 0 which in the inverted polarity case is a 100%
> > relative duty.
> 
> Thanks for confirmation, this matches my observations.
> 
> Best regards,
> Alexander
> 
> 
>