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

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

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
> 
> 
>