Am Freitag, 6. Mai 2022, 20:31:24 CEST schrieb Guenter Roeck: > 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@pe > > > > > ngutronix.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. Just to make sure to be on the same side and summarize a bit. What you mean is to add a new sysfs attribute to pwm-fan driver which controls what pwm_duty==0 implies. I would suggest to name is 'keep_pwm_enabled' (but I am open for other suggestions) with the following meaning: 1 - pwm_duty==0 just means that. Set PWM duty to 0 and keep PWM (and fan regulator) enabled. 0 - pwm_duty==0 means that the PWM duty is set to 0, PWM is disabled and any PWM fan regulator is disabled as well. For pwm_duty!=0 this setting is irrelevant. Having the default to be '1' seems sensible in order to not brake boards as regulator will be kept enabled. PWM duty and/or PWM disable is irrelevant as PWM inversion is not yet supported properly anyway. IMHO this should address all the mentioned issues. With 'keep_pwm_enabled=1' only the duty is set and the regulators are not forced to be disabled. E.g. the CPU fans mentioned by Guenter. This is also the case for hardware where the regulator is shared or not switchable at all. On hardware where it is safe to disable the regulator and PWM keep_pwm_enabled=0' allows the system to poweroff PWM and powersupply for the fan to improve powersavings. With this it is up to the administrator to provide the correct setting for this attribute as it highly depends on the actual hardware and/or usage. Best regards, Alexander
