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 04:29:13PM +0200, Uwe Kleine-König wrote:
> [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims the
> email address doens't exist.]
> 
> Hello Guenter,
> 
> On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote:
> > On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote:
> > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König:
> > > > See
> > > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutronix.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
> 
> I assume with pwm==0 you actually mean duty_cycle == 0?
> 

Correct. The "pwm" attribute sets the duty cycle.

> > 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.
> 
> Disable pwm output == set pwm output to High-Z? Not all PWMs are able to
> provide that.
> 

It is up to us to define whate it means exactly. If you are ok that "set duty
cycle to 0" reflects "set duty cycle to 0, disable pwm, and turn off regulator",
I would hope that you are ok with using the _enable attribute to do the same
and leaving pwm==0 to do what it is supposed to do, ie to keep pwm control
enabled and set the duty cycle to 0.

Thanks,
Guenter

> > 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.
> 
> Are you talking about the PWM framework here, or only the pwm-fan
> driver?
> 
> I'd expect there are better names than pwm1_enable for the intended
> semantic.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |