[PATCH 0/5] hwmon: (cros_ec) Add support for fan curves

[PATCH 0/5] hwmon: (cros_ec) Add support for fan curves

[Thomas Weißschuh]

Allow reading and writing the fan curves used by the embedded
controller.
Only allow the curves to be made more aggressive than the current ones.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (5):
      hwmon: (cros_ec) Implement custom kelvin to celsius conversions
      hwmon: (cros_ec) Prepare the addition of custom groups
      hwmon: (cros_ec) Split out cros_ec_hwmon_get_thermal_config()
      hwmon: (cros_ec) Add support for displaying fan curves
      hwmon: (cros_ec) Allow modification of fan curves

 Documentation/hwmon/cros_ec_hwmon.rst |   3 +
 drivers/hwmon/cros_ec_hwmon.c         | 248 +++++++++++++++++++++++++++++++++-
 2 files changed, 244 insertions(+), 7 deletions(-)
---
base-commit: 18825a69b1bf72ac6e80c6f45f59cac52745927d
change-id: 20260521-cros_ec-hwmon-fan-curve-28530ab3a51e

Best regards,
--  
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/5] hwmon: (cros_ec) Implement custom kelvin to celsius conversions

[Thomas Weißschuh]

The ChromeOS EC APIs use integers representing degrees kelvin for
temperatures. The default conversions from linux/units.h will then
always convert these integer degrees celsius with a 150 millidegree
offset. This is a bit confusing, as it also differs from other CrOS EC
tooling. Internally the EC uses a kelvin to celsius offset of a round
273, so the current conversion is also not entirely accurate.

Implement a custom conversion which preserves round values.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 7c308b0a4b9e..f1b6c9c2d2a3 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -147,9 +147,23 @@ static bool cros_ec_hwmon_is_error_temp(u8 temp)
 	       temp == EC_TEMP_SENSOR_NOT_CALIBRATED;
 }
 
+/* This differs slightly from the variant in units.h to avoid rounding inconsistencies. */
+#define CROS_EC_HWMON_ABSOLUTE_ZERO_MILLICELSIUS (-273000)
+
+static long cros_ec_hwmon_kelvin_to_millicelsius(long t)
+{
+	return t * MILLIDEGREE_PER_DEGREE + CROS_EC_HWMON_ABSOLUTE_ZERO_MILLICELSIUS;
+}
+
+static long cros_ec_hwmon_millicelsius_to_kelvin(long t)
+{
+	return DIV_ROUND_CLOSEST(t - CROS_EC_HWMON_ABSOLUTE_ZERO_MILLICELSIUS,
+				 MILLIDEGREE_PER_DEGREE);
+}
+
 static long cros_ec_hwmon_temp_to_millicelsius(u8 temp)
 {
-	return kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
+	return cros_ec_hwmon_kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
 }
 
 static bool cros_ec_hwmon_attr_is_temp_threshold(u32 attr)
@@ -228,7 +242,7 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 								cros_ec_hwmon_attr_to_thres(attr),
 								&threshold);
 			if (ret == 0)
-				*val = kelvin_to_millicelsius(threshold);
+				*val = cros_ec_hwmon_kelvin_to_millicelsius(threshold);
 		}
 	}
 

-- 
2.54.0

[PATCH 2/5] hwmon: (cros_ec) Prepare the addition of custom groups

[Thomas Weißschuh]

An upcoming change will add a custom sysfs attribute group.

Set up the scaffolding for that.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/hwmon/cros_ec_hwmon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index f1b6c9c2d2a3..932e45caba9c 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -402,6 +402,10 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 	NULL
 };
 
+static const struct attribute_group *cros_ec_hwmon_groups[] = {
+	NULL
+};
+
 static int cros_ec_hwmon_cooling_get_max_state(struct thermal_cooling_device *cdev,
 					       unsigned long *val)
 {
@@ -588,7 +592,8 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
 	cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
-							 &cros_ec_hwmon_chip_info, NULL);
+							 &cros_ec_hwmon_chip_info,
+							 cros_ec_hwmon_groups);
 	platform_set_drvdata(pdev, priv);
 
 	return PTR_ERR_OR_ZERO(hwmon_dev);

-- 
2.54.0

[PATCH 3/5] hwmon: (cros_ec) Split out cros_ec_hwmon_get_thermal_config()

[Thomas Weißschuh]

Some upcoming changes require access to the raw
'struct ec_thermal_config'.

Split out the logic to get it from the EC into a new helper.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/hwmon/cros_ec_hwmon.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 932e45caba9c..a6cc909e56b7 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -117,19 +117,31 @@ static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8
 	return 0;
 }
 
-static int cros_ec_hwmon_read_temp_threshold(struct cros_ec_device *cros_ec, u8 index,
-					     enum ec_temp_thresholds threshold, u32 *temp)
+static int cros_ec_hwmon_get_thermal_config(struct cros_ec_device *cros_ec, u8 index,
+					    struct ec_thermal_config *config)
 {
 	struct ec_params_thermal_get_threshold_v1 req = {};
-	struct ec_thermal_config resp;
 	int ret;
 
 	req.sensor_num = index;
 	ret = cros_ec_cmd(cros_ec, 1, EC_CMD_THERMAL_GET_THRESHOLD,
-			  &req, sizeof(req), &resp, sizeof(resp));
+			  &req, sizeof(req), config, sizeof(*config));
 	if (ret < 0)
 		return ret;
 
+	return 0;
+}
+
+static int cros_ec_hwmon_read_temp_threshold(struct cros_ec_device *cros_ec, u8 index,
+					     enum ec_temp_thresholds threshold, u32 *temp)
+{
+	struct ec_thermal_config resp;
+	int ret;
+
+	ret = cros_ec_hwmon_get_thermal_config(cros_ec, index, &resp);
+	if (ret)
+		return ret;
+
 	*temp = resp.temp_host[threshold];
 	return 0;
 }

-- 
2.54.0

[PATCH 4/5] hwmon: (cros_ec) Add support for displaying fan curves

[Thomas Weißschuh]

The automatic fan control mode of the embedded controller uses fan
curves with two trigger points to calculate the target fan speed.
All temperature sensors affect all fans.

Expose these fan curves through the standard hwmon sysfs ABI.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 Documentation/hwmon/cros_ec_hwmon.rst |   3 +
 drivers/hwmon/cros_ec_hwmon.c         | 139 ++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)

diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
index 9ccab721e7c2..7a8683227252 100644
--- a/Documentation/hwmon/cros_ec_hwmon.rst
+++ b/Documentation/hwmon/cros_ec_hwmon.rst
@@ -46,3 +46,6 @@ PWM fan control
 
     If a fan is controllable, this driver will register that fan as a cooling device
     in the thermal framework as well.
+
+Fan curves:
+   If supported by the EC. Reading only.
diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index a6cc909e56b7..731143f8c6b2 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/hwmon-sysfs.h>
 #include <linux/math.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -17,6 +18,8 @@
 #include <linux/types.h>
 #include <linux/units.h>
 
+#define to_dev_attr(_attr) container_of_const(_attr, struct device_attribute, attr)
+
 #define DRV_NAME	"cros-ec-hwmon"
 
 #define CROS_EC_HWMON_PWM_GET_FAN_DUTY_CMD_VERSION	0
@@ -372,6 +375,141 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
 	return 0;
 }
 
+static bool cros_ec_hwmon_attr_is_temp_fan_off(const struct sensor_device_attribute_2 *attr)
+{
+	return attr->nr == 0;
+}
+
+static ssize_t temp_auto_point_pwm_show(struct device *dev, struct device_attribute *attr,
+					char *buf)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+
+	if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
+		return sysfs_emit(buf, "0\n");
+	else /* temp_fan_max */
+		return sysfs_emit(buf, "255\n");
+}
+
+static ssize_t temp_auto_point_temp_show(struct device *dev, struct device_attribute *attr,
+					 char *buf)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+	struct ec_thermal_config config;
+	u32 temp;
+	int ret;
+
+	scoped_guard(hwmon_lock, dev) {
+		ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
+		if (ret)
+			return ret;
+	}
+
+	if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
+		temp = config.temp_fan_off;
+	else /* temp_fan_max */
+		temp = config.temp_fan_max;
+
+	if (temp == 0)
+		return -ENODATA;
+
+	return sysfs_emit(buf, "%ld\n", cros_ec_hwmon_kelvin_to_millicelsius(temp));
+}
+
+#define CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(_idx)					\
+	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point1_pwm,		\
+				       temp_auto_point_pwm,  0, (_idx) - 1);		\
+	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point2_pwm,		\
+				       temp_auto_point_pwm,  1, (_idx) - 1);		\
+	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point1_temp,		\
+				       temp_auto_point_temp,  0, (_idx) - 1);		\
+	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point2_temp,		\
+				       temp_auto_point_temp,  1, (_idx) - 1)		\
+
+#define CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(_idx)					\
+	&sensor_dev_attr_temp ## _idx ## _auto_point1_pwm.dev_attr.attr,		\
+	&sensor_dev_attr_temp ## _idx ## _auto_point1_temp.dev_attr.attr,		\
+	&sensor_dev_attr_temp ## _idx ## _auto_point2_pwm.dev_attr.attr,		\
+	&sensor_dev_attr_temp ## _idx ## _auto_point2_temp.dev_attr.attr		\
+
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(1);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(2);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(3);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(4);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(5);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(6);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(7);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(8);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(9);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(10);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(11);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(12);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(13);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(14);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(15);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(16);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(17);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(18);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(19);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(20);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(21);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(22);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(23);
+CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(24);
+
+static struct attribute *cros_ec_hwmon_fan_curve_attrs[] = {
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(1),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(2),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(3),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(4),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(5),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(6),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(7),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(8),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(9),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(10),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(11),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(12),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(13),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(14),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(15),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(16),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(17),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(18),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(19),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(20),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(21),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(22),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(23),
+	CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(24),
+	NULL
+};
+
+static_assert(ARRAY_SIZE(cros_ec_hwmon_fan_curve_attrs) ==
+	      ARRAY_SIZE(((struct cros_ec_hwmon_priv *)NULL)->temp_sensor_names) * 4 + 1);
+
+static umode_t cros_ec_hwmon_fan_curve_is_visible(struct kobject *kobj,
+						  struct attribute *attr, int idx)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(to_dev_attr(attr));
+	struct device *dev = kobj_to_dev(kobj);
+	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+
+	if (!priv->temp_threshold_supported)
+		return 0;
+
+	if (!priv->temp_sensor_names[sattr->index])
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group cros_ec_hwmon_fan_curve_group = {
+	.attrs		= cros_ec_hwmon_fan_curve_attrs,
+	.is_visible	= cros_ec_hwmon_fan_curve_is_visible,
+};
+
 static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
 	HWMON_CHANNEL_INFO(fan,
@@ -415,6 +553,7 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 };
 
 static const struct attribute_group *cros_ec_hwmon_groups[] = {
+	&cros_ec_hwmon_fan_curve_group,
 	NULL
 };
 

-- 
2.54.0

[PATCH 5/5] hwmon: (cros_ec) Allow modification of fan curves

[Thomas Weißschuh]

The fan curves used by the embedded controller can be configured.

Expose this through the standard hwmon sysfs ABI.

Only allow the curves to be made more aggressive than the current ones.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 Documentation/hwmon/cros_ec_hwmon.rst |  2 +-
 drivers/hwmon/cros_ec_hwmon.c         | 68 +++++++++++++++++++++++++++++++++--
 2 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
index 7a8683227252..a3d0e43c8833 100644
--- a/Documentation/hwmon/cros_ec_hwmon.rst
+++ b/Documentation/hwmon/cros_ec_hwmon.rst
@@ -48,4 +48,4 @@ PWM fan control
     in the thermal framework as well.
 
 Fan curves:
-   If supported by the EC. Reading only.
+   If supported by the EC. Reading and writing.
diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 731143f8c6b2..fb73a00ebeeb 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -8,9 +8,11 @@
 #include <linux/device.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/kstrtox.h>
 #include <linux/math.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
@@ -135,6 +137,22 @@ static int cros_ec_hwmon_get_thermal_config(struct cros_ec_device *cros_ec, u8 i
 	return 0;
 }
 
+static int cros_ec_hwmon_set_thermal_config(struct cros_ec_device *cros_ec, u8 index,
+					    const struct ec_thermal_config *config)
+{
+	struct ec_params_thermal_set_threshold_v1 req = {};
+	int ret;
+
+	req.sensor_num = index;
+	req.cfg = *config;
+	ret = cros_ec_cmd(cros_ec, 1, EC_CMD_THERMAL_SET_THRESHOLD,
+			  &req, sizeof(req), NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int cros_ec_hwmon_read_temp_threshold(struct cros_ec_device *cros_ec, u8 index,
 					     enum ec_temp_thresholds threshold, u32 *temp)
 {
@@ -417,14 +435,60 @@ static ssize_t temp_auto_point_temp_show(struct device *dev, struct device_attri
 	return sysfs_emit(buf, "%ld\n", cros_ec_hwmon_kelvin_to_millicelsius(temp));
 }
 
+static ssize_t temp_auto_point_temp_store(struct device *dev, struct device_attribute *attr,
+					  const char *buf, size_t size)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+	struct ec_thermal_config config;
+	u32 *temp_field;
+	s64 temp;
+	int ret;
+
+	ret = kstrtos64(buf, 10, &temp);
+	if (ret)
+		return ret;
+
+	temp = cros_ec_hwmon_millicelsius_to_kelvin(temp);
+
+	if (overflows_type(temp, config.temp_fan_off))
+		return -ERANGE;
+
+	guard(hwmon_lock)(dev);
+
+	ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
+	if (ret)
+		return ret;
+
+	if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
+		temp_field = &config.temp_fan_off;
+	else /* temp_fan_max */
+		temp_field = &config.temp_fan_max;
+
+	/* Only allow values which are more aggressive than the current ones */
+	if (temp > *temp_field)
+		return -EINVAL;
+
+	*temp_field = temp;
+
+	if (config.temp_fan_off > config.temp_fan_max)
+		return -EINVAL;
+
+	ret = cros_ec_hwmon_set_thermal_config(priv->cros_ec, sattr->index, &config);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
 #define CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(_idx)					\
 	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point1_pwm,		\
 				       temp_auto_point_pwm,  0, (_idx) - 1);		\
 	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point2_pwm,		\
 				       temp_auto_point_pwm,  1, (_idx) - 1);		\
-	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point1_temp,		\
+	static SENSOR_DEVICE_ATTR_2_RW(temp ## _idx ## _auto_point1_temp,		\
 				       temp_auto_point_temp,  0, (_idx) - 1);		\
-	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point2_temp,		\
+	static SENSOR_DEVICE_ATTR_2_RW(temp ## _idx ## _auto_point2_temp,		\
 				       temp_auto_point_temp,  1, (_idx) - 1)		\
 
 #define CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(_idx)					\

-- 
2.54.0

Re: [PATCH 5/5] hwmon: (cros_ec) Allow modification of fan curves

[Armin Wolf]

Am 29.05.26 um 22:31 schrieb Thomas Weißschuh:

> The fan curves used by the embedded controller can be configured.
>
> Expose this through the standard hwmon sysfs ABI.
>
> Only allow the curves to be made more aggressive than the current ones.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   Documentation/hwmon/cros_ec_hwmon.rst |  2 +-
>   drivers/hwmon/cros_ec_hwmon.c         | 68 +++++++++++++++++++++++++++++++++--
>   2 files changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
> index 7a8683227252..a3d0e43c8833 100644
> --- a/Documentation/hwmon/cros_ec_hwmon.rst
> +++ b/Documentation/hwmon/cros_ec_hwmon.rst
> @@ -48,4 +48,4 @@ PWM fan control
>       in the thermal framework as well.
>   
>   Fan curves:
> -   If supported by the EC. Reading only.
> +   If supported by the EC. Reading and writing.
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 731143f8c6b2..fb73a00ebeeb 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -8,9 +8,11 @@
>   #include <linux/device.h>
>   #include <linux/hwmon.h>
>   #include <linux/hwmon-sysfs.h>
> +#include <linux/kstrtox.h>
>   #include <linux/math.h>
>   #include <linux/mod_devicetable.h>
>   #include <linux/module.h>
> +#include <linux/overflow.h>
>   #include <linux/platform_device.h>
>   #include <linux/platform_data/cros_ec_commands.h>
>   #include <linux/platform_data/cros_ec_proto.h>
> @@ -135,6 +137,22 @@ static int cros_ec_hwmon_get_thermal_config(struct cros_ec_device *cros_ec, u8 i
>   	return 0;
>   }
>   
> +static int cros_ec_hwmon_set_thermal_config(struct cros_ec_device *cros_ec, u8 index,
> +					    const struct ec_thermal_config *config)
> +{
> +	struct ec_params_thermal_set_threshold_v1 req = {};
> +	int ret;
> +
> +	req.sensor_num = index;
> +	req.cfg = *config;
> +	ret = cros_ec_cmd(cros_ec, 1, EC_CMD_THERMAL_SET_THRESHOLD,
> +			  &req, sizeof(req), NULL, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   static int cros_ec_hwmon_read_temp_threshold(struct cros_ec_device *cros_ec, u8 index,
>   					     enum ec_temp_thresholds threshold, u32 *temp)
>   {
> @@ -417,14 +435,60 @@ static ssize_t temp_auto_point_temp_show(struct device *dev, struct device_attri
>   	return sysfs_emit(buf, "%ld\n", cros_ec_hwmon_kelvin_to_millicelsius(temp));
>   }
>   
> +static ssize_t temp_auto_point_temp_store(struct device *dev, struct device_attribute *attr,
> +					  const char *buf, size_t size)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +	struct ec_thermal_config config;
> +	u32 *temp_field;
> +	s64 temp;
> +	int ret;
> +
> +	ret = kstrtos64(buf, 10, &temp);
> +	if (ret)
> +		return ret;
> +
> +	temp = cros_ec_hwmon_millicelsius_to_kelvin(temp);
> +
> +	if (overflows_type(temp, config.temp_fan_off))
> +		return -ERANGE;
> +
> +	guard(hwmon_lock)(dev);
> +
> +	ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
> +	if (ret)
> +		return ret;
> +
> +	if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
> +		temp_field = &config.temp_fan_off;
> +	else /* temp_fan_max */
> +		temp_field = &config.temp_fan_max;
> +
> +	/* Only allow values which are more aggressive than the current ones */
> +	if (temp > *temp_field)
> +		return -EINVAL;

Hi,

i think it would be more practical for users to increase and later decrease the fan curve values.
Could the driver copy the original fan curve configuration and use that instead? This would also
require to restore the original fan curve during shutdown and removal.

Thanks,
Armin Wolf

> +
> +	*temp_field = temp;
> +
> +	if (config.temp_fan_off > config.temp_fan_max)
> +		return -EINVAL;
> +
> +	ret = cros_ec_hwmon_set_thermal_config(priv->cros_ec, sattr->index, &config);
> +	if (ret)
> +		return ret;
> +
> +	return size;
> +}
> +
>   #define CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS(_idx)					\
>   	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point1_pwm,		\
>   				       temp_auto_point_pwm,  0, (_idx) - 1);		\
>   	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point2_pwm,		\
>   				       temp_auto_point_pwm,  1, (_idx) - 1);		\
> -	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point1_temp,		\
> +	static SENSOR_DEVICE_ATTR_2_RW(temp ## _idx ## _auto_point1_temp,		\
>   				       temp_auto_point_temp,  0, (_idx) - 1);		\
> -	static SENSOR_DEVICE_ATTR_2_RO(temp ## _idx ## _auto_point2_temp,		\
> +	static SENSOR_DEVICE_ATTR_2_RW(temp ## _idx ## _auto_point2_temp,		\
>   				       temp_auto_point_temp,  1, (_idx) - 1)		\
>   
>   #define CROS_EC_HWMON_TEMP_AUTO_POINT_ATTRS_PTRS(_idx)					\
>

Re: [PATCH 5/5] hwmon: (cros_ec) Allow modification of fan curves

[Thomas Weißschuh]

On 2026-05-30 18:37:32+0200, Armin Wolf wrote:
> Am 29.05.26 um 22:31 schrieb Thomas Weißschuh:

(...)

> > +static ssize_t temp_auto_point_temp_store(struct device *dev, struct device_attribute *attr,
> > +					  const char *buf, size_t size)
> > +{
> > +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> > +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > +	struct ec_thermal_config config;
> > +	u32 *temp_field;
> > +	s64 temp;
> > +	int ret;
> > +
> > +	ret = kstrtos64(buf, 10, &temp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	temp = cros_ec_hwmon_millicelsius_to_kelvin(temp);
> > +
> > +	if (overflows_type(temp, config.temp_fan_off))
> > +		return -ERANGE;
> > +
> > +	guard(hwmon_lock)(dev);
> > +
> > +	ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
> > +		temp_field = &config.temp_fan_off;
> > +	else /* temp_fan_max */
> > +		temp_field = &config.temp_fan_max;
> > +
> > +	/* Only allow values which are more aggressive than the current ones */
> > +	if (temp > *temp_field)
> > +		return -EINVAL;
> 
> i think it would be more practical for users to increase and later decrease the fan curve values.
> Could the driver copy the original fan curve configuration and use that instead? This would also
> require to restore the original fan curve during shutdown and removal.

That would be possible. We would would have to expose these limits
through a new UAPI as otherwise the user has no way to know about them.
Restoring the original on shutdown shouldn't be necessary, as the EC
will reset the curves at shutdown anyways.

I am not so sure that it would be generally useful though. Let's hear
what other people think about it.

> Thanks,
> Armin Wolf
> 
> > +
> > +	*temp_field = temp;
> > +
> > +	if (config.temp_fan_off > config.temp_fan_max)
> > +		return -EINVAL;
> > +
> > +	ret = cros_ec_hwmon_set_thermal_config(priv->cros_ec, sattr->index, &config);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return size;
> > +}

(...)

Re: [PATCH 5/5] hwmon: (cros_ec) Allow modification of fan curves

[Armin Wolf]

Am 04.06.26 um 11:04 schrieb Thomas Weißschuh:
> On 2026-05-30 18:37:32+0200, Armin Wolf wrote:
>> Am 29.05.26 um 22:31 schrieb Thomas Weißschuh:
> 
> (...)
> 
>>> +static ssize_t temp_auto_point_temp_store(struct device *dev, struct device_attribute *attr,
>>> +					  const char *buf, size_t size)
>>> +{
>>> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>>> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
>>> +	struct ec_thermal_config config;
>>> +	u32 *temp_field;
>>> +	s64 temp;
>>> +	int ret;
>>> +
>>> +	ret = kstrtos64(buf, 10, &temp);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	temp = cros_ec_hwmon_millicelsius_to_kelvin(temp);
>>> +
>>> +	if (overflows_type(temp, config.temp_fan_off))
>>> +		return -ERANGE;
>>> +
>>> +	guard(hwmon_lock)(dev);
>>> +
>>> +	ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
>>> +		temp_field = &config.temp_fan_off;
>>> +	else /* temp_fan_max */
>>> +		temp_field = &config.temp_fan_max;
>>> +
>>> +	/* Only allow values which are more aggressive than the current ones */
>>> +	if (temp > *temp_field)
>>> +		return -EINVAL;
>>
>> i think it would be more practical for users to increase and later decrease the fan curve values.
>> Could the driver copy the original fan curve configuration and use that instead? This would also
>> require to restore the original fan curve during shutdown and removal.
> 
> That would be possible. We would would have to expose these limits
> through a new UAPI as otherwise the user has no way to know about them.
> Restoring the original on shutdown shouldn't be necessary, as the EC
> will reset the curves at shutdown anyways.

(And what about kexec?)

Ok, i myself would also interested in having a UAPI for communicating 
fan curve constraints to userspace as i am planning to add a similar 
feature to the uniwill-laptop driver.

I can think of two approaches:

1. Clamp the values into the supported range, userspace will have to 
read back the written value to know the current setting.

2. Introducing a new tempX_auto_pointY_temp_min attribute to communicate 
the constraint to userspace.

Guenter, do you have a preference for one of the approaches? Personally
i would prefer approach number 2.

> 
> I am not so sure that it would be generally useful though. Let's hear
> what other people think about it.

The uniwill-laptop driver will (likely) gain support for a similar 
feature in the future, so having such a UAPI would be beneficial.

Thanks,
Armin Wolf

>> Thanks,
>> Armin Wolf
>>
>>> +
>>> +	*temp_field = temp;
>>> +
>>> +	if (config.temp_fan_off > config.temp_fan_max)
>>> +		return -EINVAL;
>>> +
>>> +	ret = cros_ec_hwmon_set_thermal_config(priv->cros_ec, sattr->index, &config);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return size;
>>> +}
> 
> (...)
> 

Re: [PATCH 5/5] hwmon: (cros_ec) Allow modification of fan curves

[Guenter Roeck]

On 6/4/26 03:33, Armin Wolf wrote:
> Am 04.06.26 um 11:04 schrieb Thomas Weißschuh:
>> On 2026-05-30 18:37:32+0200, Armin Wolf wrote:
>>> Am 29.05.26 um 22:31 schrieb Thomas Weißschuh:
>>
>> (...)
>>
>>>> +static ssize_t temp_auto_point_temp_store(struct device *dev, struct device_attribute *attr,
>>>> +                      const char *buf, size_t size)
>>>> +{
>>>> +    struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>>>> +    struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
>>>> +    struct ec_thermal_config config;
>>>> +    u32 *temp_field;
>>>> +    s64 temp;
>>>> +    int ret;
>>>> +
>>>> +    ret = kstrtos64(buf, 10, &temp);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    temp = cros_ec_hwmon_millicelsius_to_kelvin(temp);
>>>> +
>>>> +    if (overflows_type(temp, config.temp_fan_off))
>>>> +        return -ERANGE;
>>>> +
>>>> +    guard(hwmon_lock)(dev);
>>>> +
>>>> +    ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
>>>> +        temp_field = &config.temp_fan_off;
>>>> +    else /* temp_fan_max */
>>>> +        temp_field = &config.temp_fan_max;
>>>> +
>>>> +    /* Only allow values which are more aggressive than the current ones */
>>>> +    if (temp > *temp_field)
>>>> +        return -EINVAL;
>>>
>>> i think it would be more practical for users to increase and later decrease the fan curve values.
>>> Could the driver copy the original fan curve configuration and use that instead? This would also
>>> require to restore the original fan curve during shutdown and removal.
>>
>> That would be possible. We would would have to expose these limits
>> through a new UAPI as otherwise the user has no way to know about them.
>> Restoring the original on shutdown shouldn't be necessary, as the EC
>> will reset the curves at shutdown anyways.
> 
> (And what about kexec?)
> 
> Ok, i myself would also interested in having a UAPI for communicating fan curve constraints to userspace as i am planning to add a similar feature to the uniwill-laptop driver.
> 
> I can think of two approaches:
> 
> 1. Clamp the values into the supported range, userspace will have to read back the written value to know the current setting.
> 

... which is widely used in hwmon drivers, so it is not special.
We don't usually expect userspace to know the valid attribute range.

> 2. Introducing a new tempX_auto_pointY_temp_min attribute to communicate the constraint to userspace.
> 
> Guenter, do you have a preference for one of the approaches? Personally
> i would prefer approach number 2.
> 

Again, we don't usually provide constraints for limit attributes
to userspace. Otherwise we would need separate _min and _max attributes
for literally every limit attribute. That would add a lot of complexity
for little if any gain.

Worse, almost all attributes do not just have min/max constraints but
step size constraints as well. Hardware does not typically accept values
in millisecond/millivolt etc. but have varying step sizes. How would you
express that ? The hardware won't accept a temperature of, say, 27.123
degrees C. Hwmon drivers are expected to adjust that to the next supported
value. That means userspace has to read the value back to know that value
anyway. Or do you plan to provide the step size to userspace as well ?
What would you do if the step size is non-linear ?

On top of that, _min and _max attributes are already associated with
limits. I would find it confusing if new attributes would redefine that
naming scheme to supported ranges.

>>
>> I am not so sure that it would be generally useful though. Let's hear
>> what other people think about it.
> 
> The uniwill-laptop driver will (likely) gain support for a similar feature in the future, so having such a UAPI would be beneficial.
> 

FWIW, as long as it does not reside in drivers/hwmon/, you can add any attribute
you want, and even violate the documented UAPI. That is one of the "advantages"
of having hwmon drivers reside outside drivers/hwmon/.

Guenter

Re: [PATCH 5/5] hwmon: (cros_ec) Allow modification of fan curves

[Thomas Weißschuh]

On 2026-06-08 06:34:43-0700, Guenter Roeck wrote:
> On 6/4/26 03:33, Armin Wolf wrote:
> > Am 04.06.26 um 11:04 schrieb Thomas Weißschuh:
> > > On 2026-05-30 18:37:32+0200, Armin Wolf wrote:
> > > > Am 29.05.26 um 22:31 schrieb Thomas Weißschuh:
> > > 
> > > (...)
> > > 
> > > > > +static ssize_t temp_auto_point_temp_store(struct device *dev, struct device_attribute *attr,
> > > > > +                      const char *buf, size_t size)
> > > > > +{
> > > > > +    struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> > > > > +    struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > > > > +    struct ec_thermal_config config;
> > > > > +    u32 *temp_field;
> > > > > +    s64 temp;
> > > > > +    int ret;
> > > > > +
> > > > > +    ret = kstrtos64(buf, 10, &temp);
> > > > > +    if (ret)
> > > > > +        return ret;
> > > > > +
> > > > > +    temp = cros_ec_hwmon_millicelsius_to_kelvin(temp);
> > > > > +
> > > > > +    if (overflows_type(temp, config.temp_fan_off))
> > > > > +        return -ERANGE;
> > > > > +
> > > > > +    guard(hwmon_lock)(dev);
> > > > > +
> > > > > +    ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
> > > > > +    if (ret)
> > > > > +        return ret;
> > > > > +
> > > > > +    if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
> > > > > +        temp_field = &config.temp_fan_off;
> > > > > +    else /* temp_fan_max */
> > > > > +        temp_field = &config.temp_fan_max;
> > > > > +
> > > > > +    /* Only allow values which are more aggressive than the current ones */
> > > > > +    if (temp > *temp_field)
> > > > > +        return -EINVAL;
> > > > 
> > > > i think it would be more practical for users to increase and later decrease the fan curve values.
> > > > Could the driver copy the original fan curve configuration and use that instead? This would also
> > > > require to restore the original fan curve during shutdown and removal.
> > > 
> > > That would be possible. We would would have to expose these limits
> > > through a new UAPI as otherwise the user has no way to know about them.
> > > Restoring the original on shutdown shouldn't be necessary, as the EC
> > > will reset the curves at shutdown anyways.
> > 
> > (And what about kexec?)
> > 
> > Ok, i myself would also interested in having a UAPI for communicating fan curve constraints to userspace as i am planning to add a similar feature to the uniwill-laptop driver.
> > 
> > I can think of two approaches:
> > 
> > 1. Clamp the values into the supported range, userspace will have to read back the written value to know the current setting.
> > 
> 
> ... which is widely used in hwmon drivers, so it is not special.
> We don't usually expect userspace to know the valid attribute range.

That works for me.

What should be clamp to, though?
The range active during probe or the currently active one?

> > 2. Introducing a new tempX_auto_pointY_temp_min attribute to communicate the constraint to userspace.
> > 
> > Guenter, do you have a preference for one of the approaches? Personally
> > i would prefer approach number 2.
> > 
> 
> Again, we don't usually provide constraints for limit attributes
> to userspace. Otherwise we would need separate _min and _max attributes
> for literally every limit attribute. That would add a lot of complexity
> for little if any gain.

One constraint that exists today is the range accepted by kstrtol(),
which can return -ERANGE. Not that I want to argue that this is an
issue and requires change, just pointing it out.

> Worse, almost all attributes do not just have min/max constraints but
> step size constraints as well. Hardware does not typically accept values
> in millisecond/millivolt etc. but have varying step sizes. How would you
> express that ? The hardware won't accept a temperature of, say, 27.123
> degrees C. Hwmon drivers are expected to adjust that to the next supported
> value. That means userspace has to read the value back to know that value
> anyway. Or do you plan to provide the step size to userspace as well ?
> What would you do if the step size is non-linear ?

To me the step size argument is slightly different than the range one.
The difference between the values provided by the user and the one
actually used ones should be fairly small, while for the range it may be
much larger. Not that it makes a difference here.

> On top of that, _min and _max attributes are already associated with
> limits. I would find it confusing if new attributes would redefine that
> naming scheme to supported ranges.

Ack.

(...)

Re: [PATCH 1/5] hwmon: (cros_ec) Implement custom kelvin to celsius conversions

[Guenter Roeck]

On Fri, May 29, 2026 at 10:31:52PM +0200, Thomas Weißschuh wrote:
> The ChromeOS EC APIs use integers representing degrees kelvin for
> temperatures. The default conversions from linux/units.h will then
> always convert these integer degrees celsius with a 150 millidegree
> offset. This is a bit confusing, as it also differs from other CrOS EC
> tooling. Internally the EC uses a kelvin to celsius offset of a round
> 273, so the current conversion is also not entirely accurate.
> 
> Implement a custom conversion which preserves round values.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Applied.

Thanks,
Guenter

Re: [PATCH 2/5] hwmon: (cros_ec) Prepare the addition of custom groups

[Guenter Roeck]

On Fri, May 29, 2026 at 10:31:53PM +0200, Thomas Weißschuh wrote:
> An upcoming change will add a custom sysfs attribute group.
> 
> Set up the scaffolding for that.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Applied.

Thanks,
Guenter

Re: [PATCH 3/5] hwmon: (cros_ec) Split out cros_ec_hwmon_get_thermal_config()

[Guenter Roeck]

On Fri, May 29, 2026 at 10:31:54PM +0200, Thomas Weißschuh wrote:
> Some upcoming changes require access to the raw
> 'struct ec_thermal_config'.
> 
> Split out the logic to get it from the EC into a new helper.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Applied.

Thanks,
Guenter

Re: [PATCH 4/5] hwmon: (cros_ec) Add support for displaying fan curves

[Guenter Roeck]

On Fri, May 29, 2026 at 10:31:55PM +0200, Thomas Weißschuh wrote:
> The automatic fan control mode of the embedded controller uses fan
> curves with two trigger points to calculate the target fan speed.
> All temperature sensors affect all fans.
> 
> Expose these fan curves through the standard hwmon sysfs ABI.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Applied.

Thanks,
Guenter