Re: [PATCH v3] hwmon: (cros_ec) Add set and get target fan RPM function

["Thomas Weißschuh" <thomas@weissschuh.net>]

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@chromium.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.

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