Message-ID: <7b5c4001-1a5b-4ea0-aca9-e0c88ec7a98b@roeck-us.net>
Date: Sun, 23 Mar 2025 09:22:07 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Weißschuh <thomas@...ssschuh.net>
Cc: Sung-Chi Li <lschyi@...omium.org>, Jean Delvare <jdelvare@...e.com>,
 Benson Leung <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org>,
 chrome-platform@...ts.linux.dev, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] hwmon: (cros_ec) Add set and get target fan RPM
 function

On 3/23/25 09:05, Thomas Weißschuh wrote:
> On 2025-03-22 09:10:40-0700, Guenter Roeck wrote:
>> On 3/22/25 08:23, Thomas Weißschuh wrote:
>>> On 2025-03-22 07:12:48-0700, Guenter Roeck wrote:
>>>> On 3/22/25 06:55, Thomas Weißschuh wrote:
>>>>> On 2025-03-18 15:45:23+0800, Sung-Chi Li wrote:
>>>>>> The ChromeOS embedded controller (EC) supports closed loop fan speed
>>>>>> control, so add the fan target attribute under hwmon framework, such
>>>>>> that kernel can expose reading and specifying the desired fan RPM for
>>>>>> fans connected to the EC.
>>>>>>
>>>>>> When probing the cros_ec hwmon module, we also check the supported
>>>>>> command version of setting target fan RPM. This commit implements the
>>>>>> version 0 of getting the target fan RPM, which can only read the target
>>>>>> RPM of the first fan. This commit also implements the version 1 of
>>>>>> setting the target fan RPM to each fan respectively.
>>>>>>
>>>>>> Signed-off-by: Sung-Chi Li <lschyi@...omium.org>
>>>>>> ---
>>>>>> ChromeOS embedded controller (EC) supports closed-loop fan control. We
>>>>>> anticipate to have the fan related control from the kernel side, so this
>>>>>> series register the HWMON_F_TARGET attribute, and implement the read and
>>>>>> write function for setting/reading the target fan RPM from the EC side.
>>>>>
>>>>> Should it be possible to switch back to automatic control?
>>>>> I can't find anything in the hwmon ABI about it.
>>>>> And neither in the CrOS EC source.
>>>>>
>>>>> Am I missing something?
>>>>>
>>>>
>>>> Not sure I understand the context, but the fan control method is normally
>>>> selected with pwmX_enable, which is defined as
>>>>
>>>>                   Fan speed control method:
>>>>
>>>>                   - 0: no fan speed control (i.e. fan at full speed)
>>>>                   - 1: manual fan speed control enabled (using `pwmY`)
>>>>                   - 2+: automatic fan speed control enabled
>>>
>>> So far I associated pwmY_enable = 1 with the pwmY attribute.
>>> Also controlling it through fanY_target does make sense though.
>>> It could be clearer from the docs IMHO.
>>>
>>> That also means that the patch under discussion needs to implement the
>>> pwmY_enable attribute.
>>>
>>
>> Does it ? Does setting the target fan speed automatically change the fan
>> control method ? Normally that is orthogonal: One does not necessarily
>> want to enable a specific fan control method after setting a single related
>> attribute, but do that only after all attributes have been set.
> 
> With the currently proposed patch setting fanY_target automatically
> switches to manual control. And then there is no way to go back to
> automatic control.
> 

The driver should not do that.

>> For example, there could be another attribute specifying how fast the fan
>> speed should be adjusted, or there could be minimum and/or maximum permitted
>> pwm values. I am not saying that this is the case here, but setting a target
>> fan speed should not automatically change the fan control method.
> 
> Makes sense.
> This goes back to my original question: If pwmY_enable is *not*
> implemented, how would it be possible to switch between the different
> mechanisms?

It isn't. However, as I said above, the driver should not change the fan control
method when writing fanX_target. If that happens in the EC, the driver would need
to check first if it is in manual mode and only permit setting fanX_target if
that is the case.

Guenter

Powered by blists - more mailing lists