On 5/9/22 00:39, Alexander Stein wrote:
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.
No, I am not suggesting that. I am suggesting to add support for the existing
standard attribute pwm1_enable, and define what its values mean in the driver
documentation. There is no need to provide a non-standard attribute.
Guenter
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
