[PATCH 0/3] Export fan control and register fans as cooling devices

[PATCH 0/3] Export fan control and register fans as cooling devices

[Sung-Chi Li via B4 Relay]

This is a continuation of the previous series "Export the target RPM fan
control by ChromeOS EC under hwmon"
(https://lore.kernel.org/lkml/20250313-extend_ec_hwmon_fan-v1-0-5c566776f2c4@chromium.org/T/#t).
There is a change from controlling the target fan RPM value to control
the PWM value.

We anticipate to involve fans connected to EC as thermal cooling
devices, so we can utilize the thermal framework to have further thermal
control strategies.

This series updates the required EC controls definitions, implements the
mechanism for controlling fan PWM values, and registers these fans under
thermal framework as cooling devices.

Adapting comments from the previous series, the driver probes the host
command capability at beginning to see whether a fan is controllable:
  - if command `EC_CMD_PWM_GET_FAN_DUTY` is supported (v0, this is a
    new command).
  - if command `EC_CMD_THERMAL_AUTO_FAN_CTRL` v2 is supported.
  - if command `EC_CMD_PWM_SET_FAN_DUTY` v1 is supported.

This combination is selected as this is the minimum requirement for a
fan to be fully controllable under hwmon framework.

The driver supports changing the fan control mode, and allows to change
the fan PWM value only if the fan is in manual control mode. The power
management hook is implemented as well for keeping the fan control
settings, as EC will automatically restore the control method to auto
when device is suspended.

Change-Id: I4e2fdc8c4bc50778c0d04cfbefeaab7088d3181e
Signed-off-by: Sung-Chi Li <lschyi@google.com>
---
Sung-Chi Li (3):
      platform/chrome: update pwm fan control host commands
      hwmon: (cros_ec) add PWM control over fans
      hwmon: (cros_ec) register fans into thermal framework cooling devices

 Documentation/hwmon/cros_ec_hwmon.rst          |   7 +-
 drivers/hwmon/cros_ec_hwmon.c                  | 309 ++++++++++++++++++++++++-
 include/linux/platform_data/cros_ec_commands.h |  29 ++-
 3 files changed, 339 insertions(+), 6 deletions(-)
---
base-commit: 33035b665157558254b3c21c3f049fd728e72368
change-id: 20250429-cros_ec_fan-da3b64ac9c10

Best regards,
-- 
Sung-Chi Li <lschyi@chromium.org>

[PATCH 1/3] platform/chrome: update pwm fan control host commands

[Sung-Chi Li via B4 Relay]

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

Update cros_ec_commands.h to include definitions for getting PWM fan
duty, getting and setting the fan control mode.

Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
---
 include/linux/platform_data/cros_ec_commands.h | 29 +++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 1f4e4f2b89bb936b4b1c3f4162fec203b196cbc8..2ac1a30f9a3195bfc9dffc72fe5c5b92d83f1ef2 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -1825,6 +1825,16 @@ struct ec_response_pwm_get_duty {
 	uint16_t duty;     /* Duty cycle, EC_PWM_MAX_DUTY = 100% */
 } __ec_align2;
 
+#define EC_CMD_PWM_GET_FAN_DUTY 0x0027
+
+struct ec_params_pwm_get_fan_duty {
+	uint8_t fan_idx;
+} __ec_align1;
+
+struct ec_response_pwm_get_fan_duty {
+	uint32_t percent; /* Percentage of duty cycle, ranging from 0 ~ 100 */
+} __ec_align4;
+
 /*****************************************************************************/
 /*
  * Lightbar commands. This looks worse than it is. Since we only use one HOST
@@ -3105,14 +3115,31 @@ struct ec_params_thermal_set_threshold_v1 {
 
 /****************************************************************************/
 
-/* Toggle automatic fan control */
+/* Set or get fan control mode */
 #define EC_CMD_THERMAL_AUTO_FAN_CTRL 0x0052
 
+enum ec_auto_fan_ctrl_cmd {
+	EC_AUTO_FAN_CONTROL_CMD_SET = 0,
+	EC_AUTO_FAN_CONTROL_CMD_GET,
+};
+
 /* Version 1 of input params */
 struct ec_params_auto_fan_ctrl_v1 {
 	uint8_t fan_idx;
 } __ec_align1;
 
+/* Version 2 of input params */
+struct ec_params_auto_fan_ctrl_v2 {
+	uint8_t fan_idx;
+	uint8_t cmd; /* enum ec_auto_fan_ctrl_cmd */
+	uint8_t set_auto; /* only used with EC_AUTO_FAN_CONTROL_CMD_SET - bool
+			   */
+} __ec_align4;
+
+struct ec_response_auto_fan_control {
+	uint8_t is_auto; /* bool */
+} __ec_align1;
+
 /* Get/Set TMP006 calibration data */
 #define EC_CMD_TMP006_GET_CALIBRATION 0x0053
 #define EC_CMD_TMP006_SET_CALIBRATION 0x0054

-- 
2.49.0.901.g37484f566f-goog

[PATCH 2/3] hwmon: (cros_ec) add PWM control over fans

[Sung-Chi Li via B4 Relay]

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         | 237 +++++++++++++++++++++++++++++++++-
 2 files changed, 237 insertions(+), 5 deletions(-)

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..1139074d3eb003ee72bbe54a954647ced40f6d21 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -17,10 +17,17 @@
 
 #define DRV_NAME	"cros-ec-hwmon"
 
+struct cros_ec_hwmon_platform_priv {
+	struct device *hwmon_dev;
+};
+
 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 +43,51 @@ 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_raw_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)(le32_to_cpu(resp.percent));
+	return 0;
+}
+
+static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec,
+					u8 index, u8 *pwm_value)
+{
+	int ret = cros_ec_hwmon_read_pwm_raw_value(cros_ec, index, pwm_value);
+
+	if (ret == 0)
+		*pwm_value = *pwm_value * 255 / 100;
+	return ret;
+}
+
+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 = {
+		.fan_idx = index,
+		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
+	};
+	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 +128,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 pwm_value;
+	u8 control_method;
 	u8 temp;
 
 	if (type == hwmon_fan) {
@@ -92,6 +146,18 @@ 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 +190,97 @@ static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types
 	return -EOPNOTSUPP;
 }
 
+static int cros_ec_hwmon_write_pwm_value(struct cros_ec_device *cros_ec,
+					     u8 index, u8 val)
+{
+	struct ec_params_pwm_set_fan_duty_v1 req = {
+		.percent = val,
+		.fan_idx = index,
+	};
+	int 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_set_pwm_raw_value(struct cros_ec_hwmon_priv *priv,
+					   u8 index, u8 val)
+{
+	int ret;
+
+	if (!(priv->manual_fans & BIT(index)))
+		return -ECANCELED;
+
+	ret = cros_ec_hwmon_write_pwm_value(priv->cros_ec, index, val);
+	if (ret == 0)
+		priv->manual_fan_pwm_values[index] = val;
+	return ret;
+}
+
+static int cros_ec_hwmon_set_pwm_value(struct cros_ec_hwmon_priv *priv,
+				       u8 index, u8 val)
+{
+	return cros_ec_hwmon_set_pwm_raw_value(priv, index,
+					       (((uint32_t)val) * 100 / 255));
+}
+
+static int cros_ec_hwmon_write_pwm_enable(struct cros_ec_device *cros_ec,
+					  u8 index, u8 val)
+{
+	struct ec_params_auto_fan_ctrl_v2 req = {
+		.fan_idx = index,
+		.cmd = EC_AUTO_FAN_CONTROL_CMD_SET,
+	};
+	int ret;
+
+	/* 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_set_pwm_control_method(struct cros_ec_hwmon_priv *priv,
+						u8 index, u8 val)
+{
+	int ret = cros_ec_hwmon_write_pwm_enable(priv->cros_ec, index, val);
+
+	if (ret == 0) {
+		if (val == 1)
+			priv->manual_fans |= BIT(index);
+		else
+			priv->manual_fans &= ~BIT(index);
+	}
+	return ret;
+}
+
+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_set_pwm_value(priv, channel, val);
+		case hwmon_pwm_enable:
+			return cros_ec_hwmon_set_pwm_control_method(
+				priv, 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 +289,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 && priv->fan_control_supported) {
+		if (priv->usable_fans & BIT(channel))
+			return 0644;
 	} else if (type == hwmon_temp) {
 		if (priv->temp_sensor_names[channel])
 			return 0444;
@@ -147,6 +307,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 +343,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,13 +399,35 @@ static void cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv)
 	}
 }
 
+static void
+cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_hwmon_priv *priv)
+{
+	int ret;
+
+	priv->fan_control_supported = false;
+
+	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_PWM_GET_FAN_DUTY);
+	if (ret < 0 || !(ret & EC_VER_MASK(0)))
+		return;
+
+	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_PWM_SET_FAN_DUTY);
+	if (ret < 0 || !(ret & EC_VER_MASK(1)))
+		return;
+
+	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL);
+	if (ret < 0 || !(ret & EC_VER_MASK(2)))
+		return;
+
+	priv->fan_control_supported = true;
+}
+
 static int cros_ec_hwmon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
+	struct cros_ec_hwmon_platform_priv *platform_priv;
 	struct cros_ec_hwmon_priv *priv;
-	struct device *hwmon_dev;
 	u8 thermal_version;
 	int ret;
 
@@ -251,6 +439,10 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
 	if (thermal_version == 0)
 		return -ENODEV;
 
+	platform_priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!platform_priv)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -259,11 +451,47 @@ 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);
+	cros_ec_hwmon_probe_fan_control_supported(priv);
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
-							 &cros_ec_hwmon_chip_info, NULL);
+	platform_priv->hwmon_dev = devm_hwmon_device_register_with_info(
+		dev, "cros_ec", priv, &cros_ec_hwmon_chip_info, NULL);
+	dev_set_drvdata(dev, platform_priv);
 
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	return PTR_ERR_OR_ZERO(platform_priv->hwmon_dev);
+}
+
+static int cros_ec_hwmon_resume(struct platform_device *pdev)
+{
+	const struct cros_ec_hwmon_platform_priv *platform_priv =
+		dev_get_drvdata(&pdev->dev);
+	const struct cros_ec_hwmon_priv *priv =
+		dev_get_drvdata(platform_priv->hwmon_dev);
+	size_t i;
+	int ret;
+
+	if (!priv->fan_control_supported)
+		return 0;
+
+	/*
+	 * EC sets fan control to auto after suspended, restore settings to
+	 * 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.
+		 */
+		ret = cros_ec_hwmon_write_pwm_value(
+			priv->cros_ec, i, priv->manual_fan_pwm_values[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static const struct platform_device_id cros_ec_hwmon_id[] = {
@@ -274,6 +502,7 @@ 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,
+	.resume		= cros_ec_hwmon_resume,
 	.id_table	= cros_ec_hwmon_id,
 };
 module_platform_driver(cros_ec_hwmon_driver);

-- 
2.49.0.901.g37484f566f-goog

[PATCH 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

[Sung-Chi Li via B4 Relay]

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

Register fans connected under EC as thermal cooling devices as well, so
these fans can then work with the thermal framework.

During the driver probing phase, we will also try to register each fan
as a thermal cooling device based on previous probe result (whether the
there are fans connected on that channel, and whether EC supports fan
control). The basic get max state, get current state, and set current
state methods are then implemented as well.

Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
---
 Documentation/hwmon/cros_ec_hwmon.rst |  2 +
 drivers/hwmon/cros_ec_hwmon.c         | 72 +++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
index 5b802be120438732529c3d25b1afa8b4ee353305..82c75bdaf912a116eaafa3149dc1252b3f7007d2 100644
--- a/Documentation/hwmon/cros_ec_hwmon.rst
+++ b/Documentation/hwmon/cros_ec_hwmon.rst
@@ -27,3 +27,5 @@ 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.
+If a fan is controllable, this driver will register that fan as a cooling device
+in the thermal framework as well.
diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 1139074d3eb003ee72bbe54a954647ced40f6d21..c38c61bba431fe25322793f7dd5db59fcc95daaf 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
+#include <linux/thermal.h>
 #include <linux/types.h>
 #include <linux/units.h>
 
@@ -30,6 +31,11 @@ struct cros_ec_hwmon_priv {
 	u8 manual_fan_pwm_values[EC_FAN_SPEED_ENTRIES];
 };
 
+struct cros_ec_hwmon_cooling_priv {
+	struct cros_ec_hwmon_priv *hwmon_priv;
+	u8 index;
+};
+
 static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
 {
 	int ret;
@@ -340,6 +346,38 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 	NULL
 };
 
+static int
+cros_ec_hwmon_cooling_get_max_state(struct thermal_cooling_device *cdev,
+				    unsigned long *val)
+{
+	*val = 100;
+	return 0;
+}
+
+static int
+cros_ec_hwmon_cooling_get_cur_state(struct thermal_cooling_device *cdev,
+				    unsigned long *val)
+{
+	const struct cros_ec_hwmon_cooling_priv *priv = cdev->devdata;
+	u8 raw_val;
+	int ret = cros_ec_hwmon_read_pwm_raw_value(priv->hwmon_priv->cros_ec,
+						   priv->index, &raw_val);
+
+	if (ret == 0)
+		*val = raw_val;
+	return ret;
+}
+
+static int
+cros_ec_hwmon_cooling_set_cur_state(struct thermal_cooling_device *cdev,
+				    unsigned long val)
+{
+	const struct cros_ec_hwmon_cooling_priv *priv = cdev->devdata;
+
+	return cros_ec_hwmon_set_pwm_raw_value(priv->hwmon_priv, priv->index,
+					       val);
+}
+
 static const struct hwmon_ops cros_ec_hwmon_ops = {
 	.read = cros_ec_hwmon_read,
 	.read_string = cros_ec_hwmon_read_string,
@@ -347,6 +385,12 @@ static const struct hwmon_ops cros_ec_hwmon_ops = {
 	.is_visible = cros_ec_hwmon_is_visible,
 };
 
+static const struct thermal_cooling_device_ops cros_ec_thermal_cooling_ops = {
+	.get_max_state = cros_ec_hwmon_cooling_get_max_state,
+	.get_cur_state = cros_ec_hwmon_cooling_get_cur_state,
+	.set_cur_state = cros_ec_hwmon_cooling_set_cur_state,
+};
+
 static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
 	.ops = &cros_ec_hwmon_ops,
 	.info = cros_ec_hwmon_info,
@@ -421,6 +465,33 @@ cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_hwmon_priv *priv)
 	priv->fan_control_supported = true;
 }
 
+static void
+cros_ec_hwmon_register_fan_cooling_devices(struct device *dev,
+					   struct cros_ec_hwmon_priv *priv)
+{
+	struct cros_ec_hwmon_cooling_priv *cpriv;
+	size_t i;
+
+	if (!priv->fan_control_supported)
+		return;
+
+	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
+		if (!(priv->usable_fans & BIT(i)))
+			continue;
+
+		cpriv = devm_kzalloc(dev, sizeof(*cpriv), GFP_KERNEL);
+		if (!cpriv)
+			return;
+
+		cpriv->hwmon_priv = priv;
+		cpriv->index = i;
+		devm_thermal_of_cooling_device_register(
+			dev, NULL,
+			devm_kasprintf(dev, GFP_KERNEL, "cros-ec-fan%zu", i),
+			cpriv, &cros_ec_thermal_cooling_ops);
+	}
+}
+
 static int cros_ec_hwmon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -452,6 +523,7 @@ 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);
 	cros_ec_hwmon_probe_fan_control_supported(priv);
+	cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
 
 	platform_priv->hwmon_dev = devm_hwmon_device_register_with_info(
 		dev, "cros_ec", priv, &cros_ec_hwmon_chip_info, NULL);

-- 
2.49.0.901.g37484f566f-goog

Re: [PATCH 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

[Thomas Weißschuh]

On 2025-04-29 16:14:23+0800, Sung-Chi Li via B4 Relay wrote:
> From: Sung-Chi Li <lschyi@chromium.org>
> 
> Register fans connected under EC as thermal cooling devices as well, so
> these fans can then work with the thermal framework.
> 
> During the driver probing phase, we will also try to register each fan
> as a thermal cooling device based on previous probe result (whether the
> there are fans connected on that channel, and whether EC supports fan
> control). The basic get max state, get current state, and set current
> state methods are then implemented as well.

There is also HWMON_C_REGISTER_TZ, however it depends on OF.
But this patch looks very generic, so maybe it makes sense to implement
it in the hwmon core.

> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> ---
>  Documentation/hwmon/cros_ec_hwmon.rst |  2 +
>  drivers/hwmon/cros_ec_hwmon.c         | 72 +++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)

<snip>

Re: [PATCH 2/3] hwmon: (cros_ec) add PWM control over fans

[Thomas Weißschuh]

On 2025-04-29 16:14:22+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         | 237 +++++++++++++++++++++++++++++++++-
>  2 files changed, 237 insertions(+), 5 deletions(-)
> 
> 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..1139074d3eb003ee72bbe54a954647ced40f6d21 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -17,10 +17,17 @@
>  
>  #define DRV_NAME	"cros-ec-hwmon"
>  
> +struct cros_ec_hwmon_platform_priv {
> +	struct device *hwmon_dev;
> +};

This indirection is unnecessary and only introduces a bunch of churn.

> +
>  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 +43,51 @@ 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_raw_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)(le32_to_cpu(resp.percent));

Weird choice to store a percentage in a u32.

> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec,
> +					u8 index, u8 *pwm_value)
> +{
> +	int ret = cros_ec_hwmon_read_pwm_raw_value(cros_ec, index, pwm_value);

The _raw_ function is unnecessary.

> +
> +	if (ret == 0)
> +		*pwm_value = *pwm_value * 255 / 100;
> +	return ret;
> +}
> +
> +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 = {
> +		.fan_idx = index,
> +		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
> +	};
> +	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));

Keep &foo and sizeof(foo) together on the same line please.

> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*control_method = (resp.is_auto) ? 2 : 1;

No need for braces.

> +	return 0;
> +}
> +
>  static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
>  {
>  	unsigned int offset;
> @@ -76,6 +128,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 pwm_value;
> +	u8 control_method;

These lines were sorted.

>  	u8 temp;
>  
>  	if (type == hwmon_fan) {
> @@ -92,6 +146,18 @@ 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 +190,97 @@ static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types
>  	return -EOPNOTSUPP;
>  }
>  
> +static int cros_ec_hwmon_write_pwm_value(struct cros_ec_device *cros_ec,
> +					     u8 index, u8 val)
> +{
> +	struct ec_params_pwm_set_fan_duty_v1 req = {
> +		.percent = val,
> +		.fan_idx = index,
> +	};
> +	int ret = cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_DUTY, &req,
> +			      sizeof(req), NULL, 0);

Declare "int ret" above.

> +
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_set_pwm_raw_value(struct cros_ec_hwmon_priv *priv,
> +					   u8 index, u8 val)
> +{
> +	int ret;
> +
> +	if (!(priv->manual_fans & BIT(index)))
> +		return -ECANCELED;

Weird error code.

> +
> +	ret = cros_ec_hwmon_write_pwm_value(priv->cros_ec, index, val);
> +	if (ret == 0)
> +		priv->manual_fan_pwm_values[index] = val;
> +	return ret;
> +}
> +
> +static int cros_ec_hwmon_set_pwm_value(struct cros_ec_hwmon_priv *priv,
> +				       u8 index, u8 val)
> +{
> +	return cros_ec_hwmon_set_pwm_raw_value(priv, index,
> +					       (((uint32_t)val) * 100 / 255));

Use DIV_ROUND_CLOSEST() for division.

> +}
> +
> +static int cros_ec_hwmon_write_pwm_enable(struct cros_ec_device *cros_ec,
> +					  u8 index, u8 val)
> +{
> +	struct ec_params_auto_fan_ctrl_v2 req = {
> +		.fan_idx = index,
> +		.cmd = EC_AUTO_FAN_CONTROL_CMD_SET,

Swap the two lines above.

> +	};
> +	int ret;
> +
> +	/* 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);

Use a full 100 columns.

> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_set_pwm_control_method(struct cros_ec_hwmon_priv *priv,
> +						u8 index, u8 val)
> +{
> +	int ret = cros_ec_hwmon_write_pwm_enable(priv->cros_ec, index, val);
> +
> +	if (ret == 0) {
> +		if (val == 1)
> +			priv->manual_fans |= BIT(index);
> +		else
> +			priv->manual_fans &= ~BIT(index);
> +	}
> +	return ret;
> +}
> +
> +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_set_pwm_value(priv, channel, val);
> +		case hwmon_pwm_enable:
> +			return cros_ec_hwmon_set_pwm_control_method(
> +				priv, channel, val);

Given that these function directly implement the hwmon ABI the names
should match:
cros_ec_hwmon_write_pwm_input()
cros_ec_hwmon_write_pwn_enable()

Also 100 columns. And everywhere else.

> +		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 +289,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 && priv->fan_control_supported) {
> +		if (priv->usable_fans & BIT(channel))

Move the test for priv->fan_control_supported into the inner if().

> +			return 0644;
>  	} else if (type == hwmon_temp) {
>  		if (priv->temp_sensor_names[channel])
>  			return 0444;
> @@ -147,6 +307,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 +343,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,13 +399,35 @@ static void cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv)
>  	}
>  }
>  
> +static void
> +cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_hwmon_priv *priv)
> +{

Would look nicer by returning the result as bool.

> +	int ret;
> +
> +	priv->fan_control_supported = false;
> +
> +	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_PWM_GET_FAN_DUTY);
> +	if (ret < 0 || !(ret & EC_VER_MASK(0)))

Given that these versions are used in multiple places, a #define would
be good.

> +		return;
> +
> +	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_PWM_SET_FAN_DUTY);
> +	if (ret < 0 || !(ret & EC_VER_MASK(1)))
> +		return;
> +
> +	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL);
> +	if (ret < 0 || !(ret & EC_VER_MASK(2)))
> +		return;
> +
> +	priv->fan_control_supported = true;
> +}
> +
>  static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct cros_ec_hwmon_platform_priv *platform_priv;
>  	struct cros_ec_hwmon_priv *priv;
> -	struct device *hwmon_dev;
>  	u8 thermal_version;
>  	int ret;
>  
> @@ -251,6 +439,10 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  	if (thermal_version == 0)
>  		return -ENODEV;
>  
> +	platform_priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!platform_priv)
> +		return -ENOMEM;
> +
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> @@ -259,11 +451,47 @@ 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);
> +	cros_ec_hwmon_probe_fan_control_supported(priv);
>  
> -	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> -							 &cros_ec_hwmon_chip_info, NULL);
> +	platform_priv->hwmon_dev = devm_hwmon_device_register_with_info(
> +		dev, "cros_ec", priv, &cros_ec_hwmon_chip_info, NULL);
> +	dev_set_drvdata(dev, platform_priv);

platform_set_drvdata()/platform_get_drvdata()

> -	return PTR_ERR_OR_ZERO(hwmon_dev);
> +	return PTR_ERR_OR_ZERO(platform_priv->hwmon_dev);
> +}
> +
> +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> +{
> +	const struct cros_ec_hwmon_platform_priv *platform_priv =
> +		dev_get_drvdata(&pdev->dev);
> +	const struct cros_ec_hwmon_priv *priv =
> +		dev_get_drvdata(platform_priv->hwmon_dev);
> +	size_t i;
> +	int ret;
> +
> +	if (!priv->fan_control_supported)
> +		return 0;
> +
> +	/*
> +	 * EC sets fan control to auto after suspended, restore settings to
> +	 * before suspended.
> +	 */
> +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> +		if (!(priv->manual_fans & BIT(i)))
> +			continue;

Given that we can read the actual state from the EC I'd prefer to read
it back and store it during suspend() instead of storing it during write().

> +
> +		/*
> +		 * 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.
> +		 */
> +		ret = cros_ec_hwmon_write_pwm_value(
> +			priv->cros_ec, i, priv->manual_fan_pwm_values[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static const struct platform_device_id cros_ec_hwmon_id[] = {
> @@ -274,6 +502,7 @@ 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,
> +	.resume		= cros_ec_hwmon_resume,
>  	.id_table	= cros_ec_hwmon_id,
>  };
>  module_platform_driver(cros_ec_hwmon_driver);
> 
> -- 
> 2.49.0.901.g37484f566f-goog
> 
> 

Re: [PATCH 1/3] platform/chrome: update pwm fan control host commands

[Tzung-Bi Shih]

On Tue, Apr 29, 2025 at 04:14:21PM +0800, Sung-Chi Li via B4 Relay wrote:
> From: Sung-Chi Li <lschyi@chromium.org>
> 
> Update cros_ec_commands.h to include definitions for getting PWM fan
> duty, getting and setting the fan control mode.
> 
> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

[Sung-Chi Li]

On Tue, Apr 29, 2025 at 10:45:56PM +0200, Thomas Weißschuh wrote:
> On 2025-04-29 16:14:23+0800, Sung-Chi Li via B4 Relay wrote:
> > From: Sung-Chi Li <lschyi@chromium.org>
> > 
> > Register fans connected under EC as thermal cooling devices as well, so
> > these fans can then work with the thermal framework.
> > 
> > During the driver probing phase, we will also try to register each fan
> > as a thermal cooling device based on previous probe result (whether the
> > there are fans connected on that channel, and whether EC supports fan
> > control). The basic get max state, get current state, and set current
> > state methods are then implemented as well.
> 
> There is also HWMON_C_REGISTER_TZ, however it depends on OF.
> But this patch looks very generic, so maybe it makes sense to implement
> it in the hwmon core.
> 

Hi, the HWMON_C_REGISTER_TZ is for registering a thermal sensor, and here I
registered it as thermal cooling devices, so they are different. I followed
other hwmon drivers:

  - gpio-fan.c
  - aspeed-pwm-tacho.c
  - max6650.c
  - qnap-mcu-hwmon.c
  - ...

. These hwmon drivers also manually registered other cooling devices, and that
makes sense to me, so I think it is good to just register cooling devices rather
than make big changes to hwmon core.

> > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > ---
> >  Documentation/hwmon/cros_ec_hwmon.rst |  2 +
> >  drivers/hwmon/cros_ec_hwmon.c         | 72 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> 
> <snip>

Re: [PATCH 2/3] hwmon: (cros_ec) add PWM control over fans

[Sung-Chi Li]

On Tue, Apr 29, 2025 at 11:20:09PM +0200, Thomas Weißschuh wrote:
> On 2025-04-29 16:14:22+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         | 237 +++++++++++++++++++++++++++++++++-
> >  2 files changed, 237 insertions(+), 5 deletions(-)
> > 
> > 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..1139074d3eb003ee72bbe54a954647ced40f6d21 100644
> > --- a/drivers/hwmon/cros_ec_hwmon.c
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -17,10 +17,17 @@
> >  
> >  #define DRV_NAME	"cros-ec-hwmon"
> >  
> > +struct cros_ec_hwmon_platform_priv {
> > +	struct device *hwmon_dev;
> > +};
> 
> This indirection is unnecessary and only introduces a bunch of churn.
> 

Thank you, after directly storing the `priv` with platform_set_drvdata(), we
do not need this. Will directly store the `priv` with platform_set_drvdata() and
remove these unused code in the v2 patch.

> > +
> >  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 +43,51 @@ 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_raw_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)(le32_to_cpu(resp.percent));
> 
> Weird choice to store a percentage in a u32.
> 

Yes... that is to align with the existing `EC_CMD_PWM_SET_FAN_DUTY`.

> > +	return 0;
> > +}
> > +
> > +static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec,
> > +					u8 index, u8 *pwm_value)
> > +{
> > +	int ret = cros_ec_hwmon_read_pwm_raw_value(cros_ec, index, pwm_value);
> 
> The _raw_ function is unnecessary.
> 

This is to share with the `cros_ec_hwmon_cooling_get_cur_state`, and there is a
unit conversion needed, so extract the same process into a _raw_ function.

> > +
> > +	if (ret == 0)
> > +		*pwm_value = *pwm_value * 255 / 100;
> > +	return ret;
> > +}
> > +
> > +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 = {
> > +		.fan_idx = index,
> > +		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
> > +	};
> > +	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));
> 
> Keep &foo and sizeof(foo) together on the same line please.
> 

This is automatically formatted by clang-format. I will keep it like this in the
v2 patch. If it is important for readablity, please share with me, and I will
update that in the v2 patch.

> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*control_method = (resp.is_auto) ? 2 : 1;
> 
> No need for braces.
> 

Will remove in the v2 patch.

> > +	return 0;
> > +}
> > +
> >  static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
> >  {
> >  	unsigned int offset;
> > @@ -76,6 +128,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 pwm_value;
> > +	u8 control_method;
> 
> These lines were sorted.
> 

Will sort these u8 variables in the v2 patch.

> >  	u8 temp;
> >  
> >  	if (type == hwmon_fan) {
> > @@ -92,6 +146,18 @@ 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 +190,97 @@ static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types
> >  	return -EOPNOTSUPP;
> >  }
> >  
> > +static int cros_ec_hwmon_write_pwm_value(struct cros_ec_device *cros_ec,
> > +					     u8 index, u8 val)
> > +{
> > +	struct ec_params_pwm_set_fan_duty_v1 req = {
> > +		.percent = val,
> > +		.fan_idx = index,
> > +	};
> > +	int ret = cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_DUTY, &req,
> > +			      sizeof(req), NULL, 0);
> 
> Declare "int ret" above.
> 

Will update in v2 patch.

> > +
> > +	if (ret < 0)
> > +		return ret;
> > +	return 0;
> > +}
> > +
> > +static int cros_ec_hwmon_set_pwm_raw_value(struct cros_ec_hwmon_priv *priv,
> > +					   u8 index, u8 val)
> > +{
> > +	int ret;
> > +
> > +	if (!(priv->manual_fans & BIT(index)))
> > +		return -ECANCELED;
> 
> Weird error code.
> 

Hmm, do you have some error code suggestion? I think the idea here is we will
reject to write the PWM value if fan is not in manual mode, and I am not sure
what error suits for this (is -EPERM the one to use here?).

> > +
> > +	ret = cros_ec_hwmon_write_pwm_value(priv->cros_ec, index, val);
> > +	if (ret == 0)
> > +		priv->manual_fan_pwm_values[index] = val;
> > +	return ret;
> > +}
> > +
> > +static int cros_ec_hwmon_set_pwm_value(struct cros_ec_hwmon_priv *priv,
> > +				       u8 index, u8 val)
> > +{
> > +	return cros_ec_hwmon_set_pwm_raw_value(priv, index,
> > +					       (((uint32_t)val) * 100 / 255));
> 
> Use DIV_ROUND_CLOSEST() for division.
> 

Will update in v2 patch.

> > +}
> > +
> > +static int cros_ec_hwmon_write_pwm_enable(struct cros_ec_device *cros_ec,
> > +					  u8 index, u8 val)
> > +{
> > +	struct ec_params_auto_fan_ctrl_v2 req = {
> > +		.fan_idx = index,
> > +		.cmd = EC_AUTO_FAN_CONTROL_CMD_SET,
> 
> Swap the two lines above.
> 

Will update in v2 patch.

> > +	};
> > +	int ret;
> > +
> > +	/* 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);
> 
> Use a full 100 columns.
> 

Hmm, I found the style guide actually strongly prefer 80:
https://www.kernel.org/doc/html/v6.14/process/coding-style.html#breaking-long-lines-and-strings

> > +	if (ret < 0)
> > +		return ret;
> > +	return 0;
> > +}
> > +
> > +static int cros_ec_hwmon_set_pwm_control_method(struct cros_ec_hwmon_priv *priv,
> > +						u8 index, u8 val)
> > +{
> > +	int ret = cros_ec_hwmon_write_pwm_enable(priv->cros_ec, index, val);
> > +
> > +	if (ret == 0) {
> > +		if (val == 1)
> > +			priv->manual_fans |= BIT(index);
> > +		else
> > +			priv->manual_fans &= ~BIT(index);
> > +	}
> > +	return ret;
> > +}
> > +
> > +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_set_pwm_value(priv, channel, val);
> > +		case hwmon_pwm_enable:
> > +			return cros_ec_hwmon_set_pwm_control_method(
> > +				priv, channel, val);
> 
> Given that these function directly implement the hwmon ABI the names
> should match:
> cros_ec_hwmon_write_pwm_input()
> cros_ec_hwmon_write_pwn_enable()
> 
> Also 100 columns. And everywhere else.
> 
> > +		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 +289,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 && priv->fan_control_supported) {
> > +		if (priv->usable_fans & BIT(channel))
> 
> Move the test for priv->fan_control_supported into the inner if().
> 

Will update in v2 patch.

> > +			return 0644;
> >  	} else if (type == hwmon_temp) {
> >  		if (priv->temp_sensor_names[channel])
> >  			return 0444;
> > @@ -147,6 +307,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 +343,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,13 +399,35 @@ static void cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv)
> >  	}
> >  }
> >  
> > +static void
> > +cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_hwmon_priv *priv)
> > +{
> 
> Would look nicer by returning the result as bool.
> 

Will update in v2 patch.

> > +	int ret;
> > +
> > +	priv->fan_control_supported = false;
> > +
> > +	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_PWM_GET_FAN_DUTY);
> > +	if (ret < 0 || !(ret & EC_VER_MASK(0)))
> 
> Given that these versions are used in multiple places, a #define would
> be good.
> 

Will update in v2 patch.

> > +		return;
> > +
> > +	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_PWM_SET_FAN_DUTY);
> > +	if (ret < 0 || !(ret & EC_VER_MASK(1)))
> > +		return;
> > +
> > +	ret = cros_ec_get_cmd_versions(priv->cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL);
> > +	if (ret < 0 || !(ret & EC_VER_MASK(2)))
> > +		return;
> > +
> > +	priv->fan_control_supported = true;
> > +}
> > +
> >  static int cros_ec_hwmon_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> >  	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> > +	struct cros_ec_hwmon_platform_priv *platform_priv;
> >  	struct cros_ec_hwmon_priv *priv;
> > -	struct device *hwmon_dev;
> >  	u8 thermal_version;
> >  	int ret;
> >  
> > @@ -251,6 +439,10 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
> >  	if (thermal_version == 0)
> >  		return -ENODEV;
> >  
> > +	platform_priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!platform_priv)
> > +		return -ENOMEM;
> > +
> >  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> >  		return -ENOMEM;
> > @@ -259,11 +451,47 @@ 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);
> > +	cros_ec_hwmon_probe_fan_control_supported(priv);
> >  
> > -	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> > -							 &cros_ec_hwmon_chip_info, NULL);
> > +	platform_priv->hwmon_dev = devm_hwmon_device_register_with_info(
> > +		dev, "cros_ec", priv, &cros_ec_hwmon_chip_info, NULL);
> > +	dev_set_drvdata(dev, platform_priv);
> 
> platform_set_drvdata()/platform_get_drvdata()
> 

Will update in v2 patch.

> > -	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +	return PTR_ERR_OR_ZERO(platform_priv->hwmon_dev);
> > +}
> > +
> > +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> > +{
> > +	const struct cros_ec_hwmon_platform_priv *platform_priv =
> > +		dev_get_drvdata(&pdev->dev);
> > +	const struct cros_ec_hwmon_priv *priv =
> > +		dev_get_drvdata(platform_priv->hwmon_dev);
> > +	size_t i;
> > +	int ret;
> > +
> > +	if (!priv->fan_control_supported)
> > +		return 0;
> > +
> > +	/*
> > +	 * EC sets fan control to auto after suspended, restore settings to
> > +	 * before suspended.
> > +	 */
> > +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > +		if (!(priv->manual_fans & BIT(i)))
> > +			continue;
> 
> Given that we can read the actual state from the EC I'd prefer to read
> it back and store it during suspend() instead of storing it during write().
> 

Do you mean reading fan mode and fan PWM value during suspend, or we will keep
updating `manual_fans` while write(), and do not cache the PWM value while
write()? That involves whether we need to send a get fan mode for every write
PWM value.

> > +
> > +		/*
> > +		 * 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.
> > +		 */
> > +		ret = cros_ec_hwmon_write_pwm_value(
> > +			priv->cros_ec, i, priv->manual_fan_pwm_values[i]);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static const struct platform_device_id cros_ec_hwmon_id[] = {
> > @@ -274,6 +502,7 @@ 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,
> > +	.resume		= cros_ec_hwmon_resume,
> >  	.id_table	= cros_ec_hwmon_id,
> >  };
> >  module_platform_driver(cros_ec_hwmon_driver);
> > 
> > -- 
> > 2.49.0.901.g37484f566f-goog
> > 
> > 

Re: [PATCH 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

[Thomas Weißschuh]

On 2025-04-30 09:51:03+0800, Sung-Chi Li wrote:
> On Tue, Apr 29, 2025 at 10:45:56PM +0200, Thomas Weißschuh wrote:
> > On 2025-04-29 16:14:23+0800, Sung-Chi Li via B4 Relay wrote:
> > > From: Sung-Chi Li <lschyi@chromium.org>
> > > 
> > > Register fans connected under EC as thermal cooling devices as well, so
> > > these fans can then work with the thermal framework.
> > > 
> > > During the driver probing phase, we will also try to register each fan
> > > as a thermal cooling device based on previous probe result (whether the
> > > there are fans connected on that channel, and whether EC supports fan
> > > control). The basic get max state, get current state, and set current
> > > state methods are then implemented as well.
> > 
> > There is also HWMON_C_REGISTER_TZ, however it depends on OF.
> > But this patch looks very generic, so maybe it makes sense to implement
> > it in the hwmon core.
> > 
> 
> Hi, the HWMON_C_REGISTER_TZ is for registering a thermal sensor, and here I
> registered it as thermal cooling devices, so they are different. I followed
> other hwmon drivers:
> 
>   - gpio-fan.c
>   - aspeed-pwm-tacho.c
>   - max6650.c
>   - qnap-mcu-hwmon.c
>   - ...

Indeed, sorry.

> . These hwmon drivers also manually registered other cooling devices, and that
> makes sense to me, so I think it is good to just register cooling devices rather
> than make big changes to hwmon core.

The implementation does look like a lot of boilerplate.
If Guenter doesn't chime in, let's stick with the current approach.

> > > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > > ---
> > >  Documentation/hwmon/cros_ec_hwmon.rst |  2 +
> > >  drivers/hwmon/cros_ec_hwmon.c         | 72 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 74 insertions(+)
> > 
> > <snip>

Re: [PATCH 2/3] hwmon: (cros_ec) add PWM control over fans

[Thomas Weißschuh]

On 2025-04-30 15:00:10+0800, Sung-Chi Li wrote:
> On Tue, Apr 29, 2025 at 11:20:09PM +0200, Thomas Weißschuh wrote:
> > On 2025-04-29 16:14:22+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         | 237 +++++++++++++++++++++++++++++++++-
> > >  2 files changed, 237 insertions(+), 5 deletions(-)

<snip>

> > > +static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec,
> > > +					u8 index, u8 *pwm_value)
> > > +{
> > > +	int ret = cros_ec_hwmon_read_pwm_raw_value(cros_ec, index, pwm_value);
> > 
> > The _raw_ function is unnecessary.
> > 
> 
> This is to share with the `cros_ec_hwmon_cooling_get_cur_state`, and there is a
> unit conversion needed, so extract the same process into a _raw_ function.

What's the advantage of scaling the value for the cooling device?
The hwmon core thermal zone implementation also uses the hwmon values
directly.

> > > +
> > > +	if (ret == 0)
> > > +		*pwm_value = *pwm_value * 255 / 100;
> > > +	return ret;
> > > +}
> > > +
> > > +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 = {
> > > +		.fan_idx = index,
> > > +		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
> > > +	};
> > > +	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));
> > 
> > Keep &foo and sizeof(foo) together on the same line please.
> > 
> 
> This is automatically formatted by clang-format. I will keep it like this in the
> v2 patch. If it is important for readablity, please share with me, and I will
> update that in the v2 patch.

It's not that important. But unfortunate that clang-format will make the
formatting worse.

<snip>

> > > +static int cros_ec_hwmon_set_pwm_raw_value(struct cros_ec_hwmon_priv *priv,
> > > +					   u8 index, u8 val)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!(priv->manual_fans & BIT(index)))
> > > +		return -ECANCELED;
> > 
> > Weird error code.
> > 
> 
> Hmm, do you have some error code suggestion? I think the idea here is we will
> reject to write the PWM value if fan is not in manual mode, and I am not sure
> what error suits for this (is -EPERM the one to use here?).

EOPNOTSUPP maybe.

> 
> > > +
> > > +	ret = cros_ec_hwmon_write_pwm_value(priv->cros_ec, index, val);
> > > +	if (ret == 0)
> > > +		priv->manual_fan_pwm_values[index] = val;
> > > +	return ret;
> > > +}

<snip>

> > > +static int cros_ec_hwmon_write_pwm_enable(struct cros_ec_device *cros_ec,
> > > +					  u8 index, u8 val)
> > > +{
> > > +	struct ec_params_auto_fan_ctrl_v2 req = {
> > > +		.fan_idx = index,
> > > +		.cmd = EC_AUTO_FAN_CONTROL_CMD_SET,
> > 
> > Swap the two lines above.
> > 
> 
> Will update in v2 patch.
> 
> > > +	};
> > > +	int ret;
> > > +
> > > +	/* 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);
> > 
> > Use a full 100 columns.
> > 
> 
> Hmm, I found the style guide actually strongly prefer 80:
> https://www.kernel.org/doc/html/v6.14/process/coding-style.html#breaking-long-lines-and-strings

I don't think it is a strong recommendation anymore.
The hwmon core also doesn't seem to be religious about it.

> > > +	if (ret < 0)
> > > +		return ret;
> > > +	return 0;
> > > +}
> > > +

<snip>

> > > +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> > > +{
> > > +	const struct cros_ec_hwmon_platform_priv *platform_priv =
> > > +		dev_get_drvdata(&pdev->dev);
> > > +	const struct cros_ec_hwmon_priv *priv =
> > > +		dev_get_drvdata(platform_priv->hwmon_dev);
> > > +	size_t i;
> > > +	int ret;
> > > +
> > > +	if (!priv->fan_control_supported)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * EC sets fan control to auto after suspended, restore settings to
> > > +	 * before suspended.
> > > +	 */
> > > +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > > +		if (!(priv->manual_fans & BIT(i)))
> > > +			continue;
> > 
> > Given that we can read the actual state from the EC I'd prefer to read
> > it back and store it during suspend() instead of storing it during write().
> > 
> 
> Do you mean reading fan mode and fan PWM value during suspend, or we will keep
> updating `manual_fans` while write(), and do not cache the PWM value while
> write()? That involves whether we need to send a get fan mode for every write
> PWM value.

This one:
"reading fan mode and fan PWM value during suspend"

> > > +
> > > +		/*
> > > +		 * 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.
> > > +		 */
> > > +		ret = cros_ec_hwmon_write_pwm_value(
> > > +			priv->cros_ec, i, priv->manual_fan_pwm_values[i]);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > >  }

<snip>

Re: [PATCH 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

[Guenter Roeck]

On 4/30/25 07:36, Thomas Weißschuh wrote:
> On 2025-04-30 09:51:03+0800, Sung-Chi Li wrote:
>> On Tue, Apr 29, 2025 at 10:45:56PM +0200, Thomas Weißschuh wrote:
>>> On 2025-04-29 16:14:23+0800, Sung-Chi Li via B4 Relay wrote:
>>>> From: Sung-Chi Li <lschyi@chromium.org>
>>>>
>>>> Register fans connected under EC as thermal cooling devices as well, so
>>>> these fans can then work with the thermal framework.
>>>>
>>>> During the driver probing phase, we will also try to register each fan
>>>> as a thermal cooling device based on previous probe result (whether the
>>>> there are fans connected on that channel, and whether EC supports fan
>>>> control). The basic get max state, get current state, and set current
>>>> state methods are then implemented as well.
>>>
>>> There is also HWMON_C_REGISTER_TZ, however it depends on OF.
>>> But this patch looks very generic, so maybe it makes sense to implement
>>> it in the hwmon core.
>>>
>>
>> Hi, the HWMON_C_REGISTER_TZ is for registering a thermal sensor, and here I
>> registered it as thermal cooling devices, so they are different. I followed
>> other hwmon drivers:
>>
>>    - gpio-fan.c
>>    - aspeed-pwm-tacho.c
>>    - max6650.c
>>    - qnap-mcu-hwmon.c
>>    - ...
> 
> Indeed, sorry.
> 
>> . These hwmon drivers also manually registered other cooling devices, and that
>> makes sense to me, so I think it is good to just register cooling devices rather
>> than make big changes to hwmon core.
> 
> The implementation does look like a lot of boilerplate.
> If Guenter doesn't chime in, let's stick with the current approach.
> 

Someone could make the necessary improvements to the hwmon core and clean up the drivers
implementing that boilerplate today, but that should be a separate patch series, and this
series should not depend on it.

Patches welcome ...

Thanks,
Guenter

Re: [PATCH 2/3] hwmon: (cros_ec) add PWM control over fans

[Sung-Chi Li]

On Wed, Apr 30, 2025 at 04:48:15PM +0200, Thomas Weißschuh wrote:
> On 2025-04-30 15:00:10+0800, Sung-Chi Li wrote:
> > On Tue, Apr 29, 2025 at 11:20:09PM +0200, Thomas Weißschuh wrote:
> > > On 2025-04-29 16:14:22+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         | 237 +++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 237 insertions(+), 5 deletions(-)
> 
> <snip>
> 
> > > > +static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec,
> > > > +					u8 index, u8 *pwm_value)
> > > > +{
> > > > +	int ret = cros_ec_hwmon_read_pwm_raw_value(cros_ec, index, pwm_value);
> > > 
> > > The _raw_ function is unnecessary.
> > > 
> > 
> > This is to share with the `cros_ec_hwmon_cooling_get_cur_state`, and there is a
> > unit conversion needed, so extract the same process into a _raw_ function.
> 
> What's the advantage of scaling the value for the cooling device?
> The hwmon core thermal zone implementation also uses the hwmon values
> directly.
> 

After rethinking about this, I think using the same unit between hwmon and
thermal cooling devices is easier. As a result, will remove the _raw_ functions,
and update logics in the cooling device commit.

> > > > +
> > > > +	if (ret == 0)
> > > > +		*pwm_value = *pwm_value * 255 / 100;
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +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 = {
> > > > +		.fan_idx = index,
> > > > +		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
> > > > +	};
> > > > +	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));
> > > 
> > > Keep &foo and sizeof(foo) together on the same line please.
> > > 
> > 
> > This is automatically formatted by clang-format. I will keep it like this in the
> > v2 patch. If it is important for readablity, please share with me, and I will
> > update that in the v2 patch.
> 
> It's not that important. But unfortunate that clang-format will make the
> formatting worse.
> 

Ok, I will keep them in the same row.

<snip>

> > > > +	};
> > > > +	int ret;
> > > > +
> > > > +	/* 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);
> > > 
> > > Use a full 100 columns.
> > > 
> > 
> > Hmm, I found the style guide actually strongly prefer 80:
> > https://www.kernel.org/doc/html/v6.14/process/coding-style.html#breaking-long-lines-and-strings
> 
> I don't think it is a strong recommendation anymore.
> The hwmon core also doesn't seem to be religious about it.
> 

Ok, I will update the series with 100 columns.

<snip>

> > > > +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> > > > +{
> > > > +	const struct cros_ec_hwmon_platform_priv *platform_priv =
> > > > +		dev_get_drvdata(&pdev->dev);
> > > > +	const struct cros_ec_hwmon_priv *priv =
> > > > +		dev_get_drvdata(platform_priv->hwmon_dev);
> > > > +	size_t i;
> > > > +	int ret;
> > > > +
> > > > +	if (!priv->fan_control_supported)
> > > > +		return 0;
> > > > +
> > > > +	/*
> > > > +	 * EC sets fan control to auto after suspended, restore settings to
> > > > +	 * before suspended.
> > > > +	 */
> > > > +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > > > +		if (!(priv->manual_fans & BIT(i)))
> > > > +			continue;
> > > 
> > > Given that we can read the actual state from the EC I'd prefer to read
> > > it back and store it during suspend() instead of storing it during write().
> > > 
> > 
> > Do you mean reading fan mode and fan PWM value during suspend, or we will keep
> > updating `manual_fans` while write(), and do not cache the PWM value while
> > write()? That involves whether we need to send a get fan mode for every write
> > PWM value.
> 
> This one:
> "reading fan mode and fan PWM value during suspend"
> 

Sounds good, update the logic to only caching values when suspending, and remove
the behavior of caching values when writing. The write of PWM values is then
changed to fetch the fan control method first.

Re: [PATCH 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

[Sung-Chi Li]

On Wed, Apr 30, 2025 at 08:38:06AM -0700, Guenter Roeck wrote:
> On 4/30/25 07:36, Thomas Weißschuh wrote:
> > On 2025-04-30 09:51:03+0800, Sung-Chi Li wrote:
> > > On Tue, Apr 29, 2025 at 10:45:56PM +0200, Thomas Weißschuh wrote:
> > > > On 2025-04-29 16:14:23+0800, Sung-Chi Li via B4 Relay wrote:
> > > > > From: Sung-Chi Li <lschyi@chromium.org>
> > > > > 
> > > > > Register fans connected under EC as thermal cooling devices as well, so
> > > > > these fans can then work with the thermal framework.
> > > > > 
> > > > > During the driver probing phase, we will also try to register each fan
> > > > > as a thermal cooling device based on previous probe result (whether the
> > > > > there are fans connected on that channel, and whether EC supports fan
> > > > > control). The basic get max state, get current state, and set current
> > > > > state methods are then implemented as well.
> > > > 
> > > > There is also HWMON_C_REGISTER_TZ, however it depends on OF.
> > > > But this patch looks very generic, so maybe it makes sense to implement
> > > > it in the hwmon core.
> > > > 
> > > 
> > > Hi, the HWMON_C_REGISTER_TZ is for registering a thermal sensor, and here I
> > > registered it as thermal cooling devices, so they are different. I followed
> > > other hwmon drivers:
> > > 
> > >    - gpio-fan.c
> > >    - aspeed-pwm-tacho.c
> > >    - max6650.c
> > >    - qnap-mcu-hwmon.c
> > >    - ...
> > 
> > Indeed, sorry.
> > 
> > > . These hwmon drivers also manually registered other cooling devices, and that
> > > makes sense to me, so I think it is good to just register cooling devices rather
> > > than make big changes to hwmon core.
> > 
> > The implementation does look like a lot of boilerplate.
> > If Guenter doesn't chime in, let's stick with the current approach.
> > 
> 
> Someone could make the necessary improvements to the hwmon core and clean up the drivers
> implementing that boilerplate today, but that should be a separate patch series, and this
> series should not depend on it.
> 
> Patches welcome ...
> 
> Thanks,
> Guenter
> 

Thank you, I will first complete this series, and see how to improve hwmon core
afterwards.