From: Sung-Chi Li <lschyi@chromium.org>

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@chromium.org>
---
 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 @@
 
 #include <linux/device.h>
 #include <linux/hwmon.h>
+#include <linux/math.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -21,6 +22,9 @@ struct cros_ec_hwmon_priv {
 	struct cros_ec_device *cros_ec;
 	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
 	u8 usable_fans;
+	bool fan_control_supported;
+	u8 manual_fans; /* bits to indicate whether the fan is set to manual */
+	u8 manual_fan_pwm_values[EC_FAN_SPEED_ENTRIES];
 };
 
 static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
@@ -36,6 +40,40 @@ static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index
 	return 0;
 }
 
+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));
+
+	if (ret < 0)
+		return ret;
+
+	*pwm_value = (u8)DIV_ROUND_CLOSEST(le32_to_cpu(resp.percent) * 255, 100);
+	return 0;
+}
+
+static int cros_ec_hwmon_read_pwm_enable(struct cros_ec_device *cros_ec, u8 index,
+					 u8 *control_method)
+{
+	struct ec_params_auto_fan_ctrl_v2 req = {
+		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
+		.fan_idx = index,
+	};
+	struct ec_response_auto_fan_control resp;
+	int ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req, sizeof(req),
+			      &resp, sizeof(resp));
+
+	if (ret < 0)
+		return ret;
+
+	*control_method = resp.is_auto ? 2 : 1;
+	return 0;
+}
+
 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;
 	u8 temp;
 
 	if (type == hwmon_fan) {
@@ -92,6 +132,17 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			if (ret == 0)
 				*val = cros_ec_hwmon_is_error_fan(speed);
 		}
+	} else if (type == hwmon_pwm) {
+		if (attr == hwmon_pwm_enable) {
+			ret = cros_ec_hwmon_read_pwm_enable(priv->cros_ec, channel,
+							    &control_method);
+			if (ret == 0)
+				*val = control_method;
+		} else if (attr == hwmon_pwm_input) {
+			ret = cros_ec_hwmon_read_pwm_value(priv->cros_ec, channel, &pwm_value);
+			if (ret == 0)
+				*val = pwm_value;
+		}
 	} else if (type == hwmon_temp) {
 		if (attr == hwmon_temp_input) {
 			ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
@@ -124,6 +175,72 @@ static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types
 	return -EOPNOTSUPP;
 }
 
+static int cros_ec_hwmon_set_fan_pwm_val(struct cros_ec_device *cros_ec, u8 index, u8 val)
+{
+	int ret;
+	struct ec_params_pwm_set_fan_duty_v1 req = {
+		.fan_idx = index,
+		.percent = DIV_ROUND_CLOSEST((uint32_t)val * 100, 255),
+	};
+
+	ret = cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_DUTY, &req, sizeof(req), NULL, 0);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int cros_ec_hwmon_write_pwm_input(struct cros_ec_device *cros_ec, u8 index, u8 val)
+{
+	u8 control_method;
+	int ret;
+
+	ret = cros_ec_hwmon_read_pwm_enable(cros_ec, index, &control_method);
+	if (ret)
+		return ret;
+	if (control_method != 1)
+		return -EOPNOTSUPP;
+
+	return cros_ec_hwmon_set_fan_pwm_val(cros_ec, index, val);
+}
+
+static int cros_ec_hwmon_write_pwm_enable(struct cros_ec_device *cros_ec, u8 index, u8 val)
+{
+	int ret;
+	struct ec_params_auto_fan_ctrl_v2 req = {
+		.fan_idx = index,
+		.cmd = EC_AUTO_FAN_CONTROL_CMD_SET,
+	};
+
+	/* No CROS EC supports no fan speed control */
+	if (val == 0)
+		return -EOPNOTSUPP;
+
+	req.set_auto = (val != 1) ? true : false;
+	ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req, sizeof(req), NULL, 0);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int cros_ec_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			       int channel, long val)
+{
+	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+
+	if (type == hwmon_pwm) {
+		switch (attr) {
+		case hwmon_pwm_input:
+			return cros_ec_hwmon_write_pwm_input(priv->cros_ec, channel, val);
+		case hwmon_pwm_enable:
+			return cros_ec_hwmon_write_pwm_enable(priv->cros_ec, channel, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
 					u32 attr, int channel)
 {
@@ -132,6 +249,9 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
 	if (type == hwmon_fan) {
 		if (priv->usable_fans & BIT(channel))
 			return 0444;
+	} else if (type == hwmon_pwm) {
+		if (priv->fan_control_supported && priv->usable_fans & BIT(channel))
+			return 0644;
 	} else if (type == hwmon_temp) {
 		if (priv->temp_sensor_names[channel])
 			return 0444;
@@ -147,6 +267,11 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 			   HWMON_F_INPUT | HWMON_F_FAULT,
 			   HWMON_F_INPUT | HWMON_F_FAULT,
 			   HWMON_F_INPUT | HWMON_F_FAULT),
+	HWMON_CHANNEL_INFO(pwm,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
 	HWMON_CHANNEL_INFO(temp,
 			   HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
 			   HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
@@ -178,6 +303,7 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 static const struct hwmon_ops cros_ec_hwmon_ops = {
 	.read = cros_ec_hwmon_read,
 	.read_string = cros_ec_hwmon_read_string,
+	.write = cros_ec_hwmon_write,
 	.is_visible = cros_ec_hwmon_is_visible,
 };
 
@@ -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) &&
+	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_PWM_SET_FAN_DUTY, 1) &&
+	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL, 2);
+}
+
 static int cros_ec_hwmon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -259,13 +400,71 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
 
 	cros_ec_hwmon_probe_temp_sensors(dev, priv, thermal_version);
 	cros_ec_hwmon_probe_fans(priv);
+	priv->fan_control_supported =
+		cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
 							 &cros_ec_hwmon_chip_info, NULL);
+	platform_set_drvdata(pdev, priv);
 
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
+static int cros_ec_hwmon_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct cros_ec_hwmon_priv *priv = platform_get_drvdata(pdev);
+	size_t i;
+	int ret;
+	u8 control_method;
+
+	if (!priv->fan_control_supported)
+		return 0;
+
+	/* EC sets fan control to auto after suspended, store settings before suspending. */
+	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
+		if (!(priv->usable_fans & BIT(i)))
+			continue;
+
+		ret = cros_ec_hwmon_read_pwm_enable(priv->cros_ec, i, &control_method);
+		if (ret)
+			continue;
+
+		if (control_method != 1) {
+			priv->manual_fans &= ~BIT(i);
+			continue;
+		} else {
+			priv->manual_fans |= BIT(i);
+		}
+
+		cros_ec_hwmon_read_pwm_value(priv->cros_ec, i, &priv->manual_fan_pwm_values[i]);
+	}
+
+	return 0;
+}
+
+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]);
+	}
+
+	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,
 	.id_table	= cros_ec_hwmon_id,
 };
 module_platform_driver(cros_ec_hwmon_driver);

-- 
2.49.0.906.g1f30a19c02-goog

On 2025-05-02 13:34:46+0800, Sung-Chi Li via B4 Relay wrote:
> From: Sung-Chi Li <lschyi@chromium.org>
> 
> 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@chromium.org>
> ---
>  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 @@
>  
>  #include <linux/device.h>
>  #include <linux/hwmon.h>
> +#include <linux/math.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -21,6 +22,9 @@ struct cros_ec_hwmon_priv {
>  	struct cros_ec_device *cros_ec;
>  	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
>  	u8 usable_fans;
> +	bool fan_control_supported;
> +	u8 manual_fans; /* bits to indicate whether the fan is set to manual */
> +	u8 manual_fan_pwm_values[EC_FAN_SPEED_ENTRIES];
>  };
>  
>  static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> @@ -36,6 +40,40 @@ static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index
>  	return 0;
>  }
>  
> +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.

> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*pwm_value = (u8)DIV_ROUND_CLOSEST(le32_to_cpu(resp.percent) * 255, 100);
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_read_pwm_enable(struct cros_ec_device *cros_ec, u8 index,
> +					 u8 *control_method)
> +{
> +	struct ec_params_auto_fan_ctrl_v2 req = {
> +		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
> +		.fan_idx = index,
> +	};
> +	struct ec_response_auto_fan_control resp;
> +	int ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req, sizeof(req),
> +			      &resp, sizeof(resp));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*control_method = resp.is_auto ? 2 : 1;
> +	return 0;
> +}
> +
>  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.

>  	u8 temp;
>  
>  	if (type == hwmon_fan) {
> @@ -92,6 +132,17 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			if (ret == 0)
>  				*val = cros_ec_hwmon_is_error_fan(speed);
>  		}
> +	} else if (type == hwmon_pwm) {
> +		if (attr == hwmon_pwm_enable) {
> +			ret = cros_ec_hwmon_read_pwm_enable(priv->cros_ec, channel,
> +							    &control_method);

Unnecessary linbreak.

> +			if (ret == 0)
> +				*val = control_method;
> +		} else if (attr == hwmon_pwm_input) {
> +			ret = cros_ec_hwmon_read_pwm_value(priv->cros_ec, channel, &pwm_value);
> +			if (ret == 0)
> +				*val = pwm_value;
> +		}
>  	} else if (type == hwmon_temp) {
>  		if (attr == hwmon_temp_input) {
>  			ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
> @@ -124,6 +175,72 @@ static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types
>  	return -EOPNOTSUPP;
>  }
>  
> +static int cros_ec_hwmon_set_fan_pwm_val(struct cros_ec_device *cros_ec, u8 index, u8 val)
> +{
> +	int ret;
> +	struct ec_params_pwm_set_fan_duty_v1 req = {
> +		.fan_idx = index,
> +		.percent = DIV_ROUND_CLOSEST((uint32_t)val * 100, 255),
> +	};
> +
> +	ret = cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_DUTY, &req, sizeof(req), NULL, 0);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_write_pwm_input(struct cros_ec_device *cros_ec, u8 index, u8 val)
> +{
> +	u8 control_method;
> +	int ret;
> +
> +	ret = cros_ec_hwmon_read_pwm_enable(cros_ec, index, &control_method);
> +	if (ret)
> +		return ret;
> +	if (control_method != 1)
> +		return -EOPNOTSUPP;
> +
> +	return cros_ec_hwmon_set_fan_pwm_val(cros_ec, index, val);
> +}
> +
> +static int cros_ec_hwmon_write_pwm_enable(struct cros_ec_device *cros_ec, u8 index, u8 val)
> +{
> +	int ret;
> +	struct ec_params_auto_fan_ctrl_v2 req = {
> +		.fan_idx = index,
> +		.cmd = EC_AUTO_FAN_CONTROL_CMD_SET,
> +	};
> +
> +	/* No CROS EC supports no fan speed control */
> +	if (val == 0)
> +		return -EOPNOTSUPP;
> +
> +	req.set_auto = (val != 1) ? true : false;
> +	ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req, sizeof(req), NULL, 0);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			       int channel, long val)
> +{
> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_pwm) {
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			return cros_ec_hwmon_write_pwm_input(priv->cros_ec, channel, val);
> +		case hwmon_pwm_enable:
> +			return cros_ec_hwmon_write_pwm_enable(priv->cros_ec, channel, val);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
>  static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
>  					u32 attr, int channel)
>  {
> @@ -132,6 +249,9 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
>  	if (type == hwmon_fan) {
>  		if (priv->usable_fans & BIT(channel))
>  			return 0444;
> +	} else if (type == hwmon_pwm) {
> +		if (priv->fan_control_supported && priv->usable_fans & BIT(channel))
> +			return 0644;
>  	} else if (type == hwmon_temp) {
>  		if (priv->temp_sensor_names[channel])
>  			return 0444;
> @@ -147,6 +267,11 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
>  			   HWMON_F_INPUT | HWMON_F_FAULT,
>  			   HWMON_F_INPUT | HWMON_F_FAULT,
>  			   HWMON_F_INPUT | HWMON_F_FAULT),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
>  	HWMON_CHANNEL_INFO(temp,
>  			   HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
>  			   HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
> @@ -178,6 +303,7 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
>  static const struct hwmon_ops cros_ec_hwmon_ops = {
>  	.read = cros_ec_hwmon_read,
>  	.read_string = cros_ec_hwmon_read_string,
> +	.write = cros_ec_hwmon_write,
>  	.is_visible = cros_ec_hwmon_is_visible,
>  };
>  
> @@ -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.

> +	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_PWM_SET_FAN_DUTY, 1) &&
> +	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL, 2);
> +}
> +
>  static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -259,13 +400,71 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  
>  	cros_ec_hwmon_probe_temp_sensors(dev, priv, thermal_version);
>  	cros_ec_hwmon_probe_fans(priv);
> +	priv->fan_control_supported =
> +		cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);

No need for a linebreak.

>  
>  	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
>  							 &cros_ec_hwmon_chip_info, NULL);
> +	platform_set_drvdata(pdev, priv);
>  
>  	return PTR_ERR_OR_ZERO(hwmon_dev);
>  }
>  
> +static int cros_ec_hwmon_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct cros_ec_hwmon_priv *priv = platform_get_drvdata(pdev);
> +	size_t i;
> +	int ret;
> +	u8 control_method;
> +
> +	if (!priv->fan_control_supported)
> +		return 0;
> +
> +	/* EC sets fan control to auto after suspended, store settings before suspending. */
> +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> +		if (!(priv->usable_fans & BIT(i)))
> +			continue;
> +
> +		ret = cros_ec_hwmon_read_pwm_enable(priv->cros_ec, i, &control_method);
> +		if (ret)
> +			continue;
> +
> +		if (control_method != 1) {
> +			priv->manual_fans &= ~BIT(i);
> +			continue;
> +		} else {
> +			priv->manual_fans |= BIT(i);
> +		}
> +
> +		cros_ec_hwmon_read_pwm_value(priv->cros_ec, i, &priv->manual_fan_pwm_values[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +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?

> +	}
> +
> +	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.

>  	.id_table	= cros_ec_hwmon_id,
>  };
>  module_platform_driver(cros_ec_hwmon_driver);
> 
> -- 
> 2.49.0.906.g1f30a19c02-goog
> 
>

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

On 2025-05-05 10:59:06+0800, Sung-Chi Li wrote:
> 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@chromium.org>

<snip>

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

The ordering was for the whole declaration block.
If you want to keep variables of the same type together, declare them in
a single line (and sort that).

<snip>

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

A warning sounds good here.