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         | 66 +++++++++++++++++++++++++++++++++++
 2 files changed, 68 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 c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955..abfcf44fb7505189124e78c651b0eb1e0533b4e8 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -13,6 +13,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>
 
@@ -27,6 +28,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;
@@ -300,6 +306,34 @@ 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 = 255;
+	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 read_val;
+	int ret = cros_ec_hwmon_read_pwm_value(priv->hwmon_priv->cros_ec, priv->index, &read_val);
+
+	if (ret == 0)
+		*val = read_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_write_pwm_input(priv->hwmon_priv->cros_ec, priv->index, val);
+}
+
 static const struct hwmon_ops cros_ec_hwmon_ops = {
 	.read = cros_ec_hwmon_read,
 	.read_string = cros_ec_hwmon_read_string,
@@ -307,6 +341,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,
@@ -374,6 +414,31 @@ static bool cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_device *cro
 	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL, 2);
 }
 
+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;
@@ -402,6 +467,7 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
 	cros_ec_hwmon_probe_fans(priv);
 	priv->fan_control_supported =
 		cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
+	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);

-- 
2.49.0.906.g1f30a19c02-goog

On 2025-05-02 13:34:47+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.
> 
> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> ---
>  Documentation/hwmon/cros_ec_hwmon.rst |  2 ++
>  drivers/hwmon/cros_ec_hwmon.c         | 66 +++++++++++++++++++++++++++++++++++
>  2 files changed, 68 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 c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955..abfcf44fb7505189124e78c651b0eb1e0533b4e8 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -13,6 +13,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>

Needs a dependency on CONFIG_THERMAL.

>  #include <linux/types.h>
>  #include <linux/units.h>
>  
> @@ -27,6 +28,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;
> @@ -300,6 +306,34 @@ 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 = 255;
> +	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 read_val;
> +	int ret = cros_ec_hwmon_read_pwm_value(priv->hwmon_priv->cros_ec, priv->index, &read_val);

Split declaration and assignment.

> +
> +	if (ret == 0)
> +		*val = read_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_write_pwm_input(priv->hwmon_priv->cros_ec, priv->index, val);
> +}
> +
>  static const struct hwmon_ops cros_ec_hwmon_ops = {
>  	.read = cros_ec_hwmon_read,
>  	.read_string = cros_ec_hwmon_read_string,
> @@ -307,6 +341,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,
> +};

Move this directly after the definition of the functions.

> +
>  static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
>  	.ops = &cros_ec_hwmon_ops,
>  	.info = cros_ec_hwmon_info,
> @@ -374,6 +414,31 @@ static bool cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_device *cro
>  	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL, 2);
>  }
>  
> +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 (!IS_ENABLED(CONFIG_THERMAL))
	return;

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

What happens for multiple/chained ECs? If both provide sensors the
thermal device names will collide.

Error handling for devm_kasprintf() is missing.

> +			&cros_ec_thermal_cooling_ops);

Error handling for devm_thermal_of_cooling_device_register() is missing.

> +	}
> +}
> +
>  static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -402,6 +467,7 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  	cros_ec_hwmon_probe_fans(priv);
>  	priv->fan_control_supported =
>  		cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
> +	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);
> 
> -- 
> 2.49.0.906.g1f30a19c02-goog
> 
>

On Sat, May 03, 2025 at 09:27:18AM +0200, Thomas Weißschuh wrote:
> On 2025-05-02 13:34:47+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.
> > 
> > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > ---
> >  Documentation/hwmon/cros_ec_hwmon.rst |  2 ++
> >  drivers/hwmon/cros_ec_hwmon.c         | 66 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 68 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 c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955..abfcf44fb7505189124e78c651b0eb1e0533b4e8 100644
> > --- a/drivers/hwmon/cros_ec_hwmon.c
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -13,6 +13,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>
> 
> Needs a dependency on CONFIG_THERMAL.
> 

I think adding the `if (!IS_ENABLED(CONFIG_THERMAL))` you suggested is
sufficient, and turning on or off CONFIG_THERMAL both can compile, so I'll only
add the guarding statement in the `cros_ec_hwmon_register_fan_cooling_devices`.

> > +
> > +	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,
> 
> What happens for multiple/chained ECs? If both provide sensors the
> thermal device names will collide.
> 

How about changing the "cros-ec-fan%zu" to "%s-fan%zu", which prefixes the
`dev_name()`? Here is an example from a device: cros-ec-hwmon.12.auto-fan0.

> Error handling for devm_kasprintf() is missing.
> 

Thank you for catching this, I will skip registering that device if the
devm_kasprintf() fails.

> > +			&cros_ec_thermal_cooling_ops);
> 
> Error handling for devm_thermal_of_cooling_device_register() is missing.
> 

I think we should continue registering other fans, so maybe we add a warning
here if the registration fails?

> > +	}
> > +}
> > +
> >  static int cros_ec_hwmon_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -402,6 +467,7 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
> >  	cros_ec_hwmon_probe_fans(priv);
> >  	priv->fan_control_supported =
> >  		cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
> > +	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);
> > 
> > -- 
> > 2.49.0.906.g1f30a19c02-goog
> > 
> > 

On 2025-05-06 10:49:37+0800, Sung-Chi Li wrote:
> On Sat, May 03, 2025 at 09:27:18AM +0200, Thomas Weißschuh wrote:
> > On 2025-05-02 13:34:47+0800, Sung-Chi Li via B4 Relay wrote:
> > > From: Sung-Chi Li <lschyi@chromium.org>

<snip>

> > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > > index c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955..abfcf44fb7505189124e78c651b0eb1e0533b4e8 100644
> > > --- a/drivers/hwmon/cros_ec_hwmon.c
> > > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > > @@ -13,6 +13,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>
> > 
> > Needs a dependency on CONFIG_THERMAL.
> > 
> 
> I think adding the `if (!IS_ENABLED(CONFIG_THERMAL))` you suggested is
> sufficient, and turning on or off CONFIG_THERMAL both can compile, so I'll only
> add the guarding statement in the `cros_ec_hwmon_register_fan_cooling_devices`.

Agreed.

> > > +
> > > +	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,
> > 
> > What happens for multiple/chained ECs? If both provide sensors the
> > thermal device names will collide.
> > 
> 
> How about changing the "cros-ec-fan%zu" to "%s-fan%zu", which prefixes the
> `dev_name()`? Here is an example from a device: cros-ec-hwmon.12.auto-fan0.

Sounds good in general.
It should match what a potential future HWMON_C_REGISTER_TCD would do.
Which in turn should mirror HWMON_C_REGISTER_TZ, if that has a specific
naming scheme.

> > Error handling for devm_kasprintf() is missing.
> > 
> 
> Thank you for catching this, I will skip registering that device if the
> devm_kasprintf() fails.

This should also mirror what HWMON_C_REGISTER_TZ is doing on errors.
If I read the code correctly, probing is aborted there.

> 
> > > +			&cros_ec_thermal_cooling_ops);
> > 
> > Error handling for devm_thermal_of_cooling_device_register() is missing.
> > 
> 
> I think we should continue registering other fans, so maybe we add a warning
> here if the registration fails?

Same as above.

<snip>

On 5/3/25 00:27, Thomas Weißschuh wrote:
> On 2025-05-02 13:34:47+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.
>>
>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
>> ---
>>   Documentation/hwmon/cros_ec_hwmon.rst |  2 ++
>>   drivers/hwmon/cros_ec_hwmon.c         | 66 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 68 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 c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955..abfcf44fb7505189124e78c651b0eb1e0533b4e8 100644
>> --- a/drivers/hwmon/cros_ec_hwmon.c
>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>> @@ -13,6 +13,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>
> 
> Needs a dependency on CONFIG_THERMAL.
> 
>>   #include <linux/types.h>
>>   #include <linux/units.h>
>>   
>> @@ -27,6 +28,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;
>> @@ -300,6 +306,34 @@ 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 = 255;
>> +	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 read_val;
>> +	int ret = cros_ec_hwmon_read_pwm_value(priv->hwmon_priv->cros_ec, priv->index, &read_val);
> 
> Split declaration and assignment.
> 
>> +
>> +	if (ret == 0)
>> +		*val = read_val;
>> +	return ret;

Also, error handling always comes first.

	if (ret)
		return ret;

	*val = read_val;
	return 0;

>> +}
>> +
>> +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_write_pwm_input(priv->hwmon_priv->cros_ec, priv->index, val);
>> +}
>> +
>>   static const struct hwmon_ops cros_ec_hwmon_ops = {
>>   	.read = cros_ec_hwmon_read,
>>   	.read_string = cros_ec_hwmon_read_string,
>> @@ -307,6 +341,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,
>> +};
> 
> Move this directly after the definition of the functions.
> 
>> +
>>   static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
>>   	.ops = &cros_ec_hwmon_ops,
>>   	.info = cros_ec_hwmon_info,
>> @@ -374,6 +414,31 @@ static bool cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_device *cro
>>   	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL, 2);
>>   }
>>   
>> +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 (!IS_ENABLED(CONFIG_THERMAL))
> 	return;
> 
>> +
>> +	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,
> 
> What happens for multiple/chained ECs? If both provide sensors the
> thermal device names will collide.
> 
> Error handling for devm_kasprintf() is missing.
> 
>> +			&cros_ec_thermal_cooling_ops);
> 
> Error handling for devm_thermal_of_cooling_device_register() is missing.
> 
>> +	}
>> +}
>> +
>>   static int cros_ec_hwmon_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -402,6 +467,7 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>>   	cros_ec_hwmon_probe_fans(priv);
>>   	priv->fan_control_supported =
>>   		cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
>> +	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);
>>
>> -- 
>> 2.49.0.906.g1f30a19c02-goog
>>
>>