On Sat, May 03, 2025 at 09:36:39AM +0200, Thomas Weißschuh wrote:
> On 2025-05-02 13:34:46+0800, Sung-Chi Li via B4 Relay wrote:
> > From: Sung-Chi Li <lschyi@xxxxxxxxxxxx>
> >
> > Newer EC firmware supports controlling fans through host commands, so
> > adding corresponding implementations for controlling these fans in the
> > driver for other kernel services and userspace to control them.
> >
> > The driver will first probe the supported host command versions (get and
> > set of fan PWM values, get and set of fan control mode) to see if the
> > connected EC fulfills the requirements of controlling the fan, then
> > exposes corresponding sysfs nodes for userspace to control the fan with
> > corresponding read and write implementations.
> > As EC will automatically change the fan mode to auto when the device is
> > suspended, the power management hooks are added as well to keep the fan
> > control mode and fan PWM value consistent during suspend and resume. As
> > we need to access the hwmon device in the power management hook, update
> > the driver by storing the hwmon device in the driver data as well.
> >
> > Signed-off-by: Sung-Chi Li <lschyi@xxxxxxxxxxxx>
> > ---
> > Documentation/hwmon/cros_ec_hwmon.rst | 5 +-
> > drivers/hwmon/cros_ec_hwmon.c | 201 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 205 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
> > index 47ecae983bdbef4bfcafc5dd2fff3de039f77f8e..5b802be120438732529c3d25b1afa8b4ee353305 100644
> > --- a/Documentation/hwmon/cros_ec_hwmon.rst
> > +++ b/Documentation/hwmon/cros_ec_hwmon.rst
> > @@ -23,4 +23,7 @@ ChromeOS embedded controller used in Chromebooks and other devices.
> >
> > The channel labels exposed via hwmon are retrieved from the EC itself.
> >
> > -Fan and temperature readings are supported.
> > +Fan and temperature readings are supported. PWM fan control is also supported if
> > +the EC also supports setting fan PWM values and fan mode. Note that EC will
> > +switch fan control mode back to auto when suspended. This driver will restore
> > +the fan state before suspended.
> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > index 9991c3fa020ac859cbbff29dfb669e53248df885..c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955 100644
> > --- a/drivers/hwmon/cros_ec_hwmon.c
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -7,6 +7,7 @@
> >
> > +static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec, u8 index, u8 *pwm_value)
> > +{
> > + struct ec_params_pwm_get_fan_duty req = {
> > + .fan_idx = index,
> > + };
> > + struct ec_response_pwm_get_fan_duty resp;
> > + int ret = cros_ec_cmd(cros_ec, 0, EC_CMD_PWM_GET_FAN_DUTY, &req, sizeof(req),
> > + &resp, sizeof(resp));
>
> As mentioned before, please split the variable declaration and the
> assignment.
>
Oh sorry, because I saw we use `struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev)`
in the declaration section, I thought it is ok to do so as well for the `ret`.
Will update these assignments in the declaration section.
> > static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
> > {
> > unsigned int offset;
> > @@ -76,6 +114,8 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > int ret = -EOPNOTSUPP;
> > u16 speed;
> > + u8 control_method;
> > + u8 pwm_value;
>
> Ordering.
>
I thought you are talking about only the u8 variables, or do you mean the
ordering should be applied with different types (and the declarations of
different types are mixed)?
> > @@ -233,6 +359,21 @@ static void cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv)
> > }
> > }
> >
> > +static inline bool is_cros_ec_cmd_fulfilled(struct cros_ec_device *cros_ec,
> > + u16 cmd, u8 version)
> > +{
> > + int ret = cros_ec_get_cmd_versions(cros_ec, cmd);
> > +
> > + return ret >= 0 && (ret & EC_VER_MASK(version));
> > +}
> > +
> > +static bool cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_device *cros_ec)
> > +{
> > + return is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_PWM_GET_FAN_DUTY, 0) &&
>
> As mentioned before, the hardcoded version constants are used in
> multiple places. They should use a #define instead.
>
Oh sorry, I thought you are talking about the process of `is_cros_ec_cmd_fulfilled`.
Will make these versions in a #define respectively.
> > +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> > +{
> > + const struct cros_ec_hwmon_priv *priv = platform_get_drvdata(pdev);
> > + size_t i;
> > +
> > + if (!priv->fan_control_supported)
> > + return 0;
> > +
> > + /* EC sets fan control to auto after suspended, restore to settings before suspended. */
> > + for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > + if (!(priv->manual_fans & BIT(i)))
> > + continue;
> > +
> > + /*
> > + * Setting fan PWM value to EC will change the mode to manual for that fan in EC as
> > + * well, so we do not need to issue a separate fan mode to manual call.
> > + */
> > + cros_ec_hwmon_set_fan_pwm_val(priv->cros_ec, i, priv->manual_fan_pwm_values[i]);
>
> Error handling?
>
I removed the error checking in the v2 version because after second thought, I
think even if we failed at the i th fan, we should do our best to restore these
fan settings, thus continuing on the (i+1) th fan and so on rather than stop the
process immediately. Is adding a warning log for the failure sufficient?
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static const struct platform_device_id cros_ec_hwmon_id[] = {
> > { DRV_NAME, 0 },
> > {}
> > @@ -274,6 +473,8 @@ static const struct platform_device_id cros_ec_hwmon_id[] = {
> > static struct platform_driver cros_ec_hwmon_driver = {
> > .driver.name = DRV_NAME,
> > .probe = cros_ec_hwmon_probe,
> > + .suspend = cros_ec_hwmon_suspend,
> > + .resume = cros_ec_hwmon_resume,
>
> I think these should use pm_ptr() to so the functions can be optimized
> away if !CONFIG_PM.
>
Got it, and I think we should also set the `priv->fan_control_supported` to
false as well if !CONFIG_PM. Will update in v3.
> > .id_table = cros_ec_hwmon_id,
> > };
> > module_platform_driver(cros_ec_hwmon_driver);
> >
> > --
> > 2.49.0.906.g1f30a19c02-goog
> >
> >
