[PATCH 0/3] hwmon: (amc6821) Add cooling device support

[PATCH 0/3] hwmon: (amc6821) Add cooling device support

[João Paulo Gonçalves]

Add support for using the AMC6821 as a cooling device. The AMC6821
registers with the thermal framework only if the `cooling-levels`
property is present in the fan device tree child node. Existing behavior
is unchanged, so the AMC6821 can still be used without the thermal
framework (hwmon only).

Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
---
João Paulo Gonçalves (3):
      dt-bindings: hwmon: amc6821: Add cooling levels
      hwmon: (amc6821) Move reading fan data from OF to a function
      hwmon: (amc6821) Add cooling device support

 .../devicetree/bindings/hwmon/ti,amc6821.yaml      |   6 +
 drivers/hwmon/amc6821.c                            | 147 ++++++++++++++++++---
 2 files changed, 131 insertions(+), 22 deletions(-)
---
base-commit: 7e801aa73daa456c4404fde177d3fc397661abf0
change-id: 20250530-b4-v1-amc6821-cooling-device-support-b4-41efee7492d8

Best regards,
-- 
João Paulo Gonçalves <joao.goncalves@toradex.com>

[PATCH 1/3] dt-bindings: hwmon: amc6821: Add cooling levels

[João Paulo Gonçalves]

From: João Paulo Gonçalves <joao.goncalves@toradex.com>

The fan can be used as a cooling device, add a description of the
`cooling-levels` property and restrict the maximum value to 255, which
is the highest PWM duty cycle supported by the AMC6821 fan controller.

Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
---
 Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
index 9ca7356760a74b1ab5e6c5a4966ba30f050a1eed..eb00756988be158b104642707d96e371930c9fd7 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
@@ -32,6 +32,12 @@ properties:
     $ref: fan-common.yaml#
     unevaluatedProperties: false
 
+    properties:
+      cooling-levels:
+        description: PWM duty cycle values corresponding to thermal cooling states.
+        items:
+          maximum: 255
+
   "#pwm-cells":
     const: 2
     description: |

-- 
2.43.0

[PATCH 2/3] hwmon: (amc6821) Move reading fan data from OF to a function

[João Paulo Gonçalves]

From: João Paulo Gonçalves <joao.goncalves@toradex.com>

Move fan property reading from OF to a separate function. This keeps OF
data handling separate from the code logic and makes it easier to add
features like cooling device support that use the same fan node.

Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
---
 drivers/hwmon/amc6821.c | 58 +++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 13a789cc85d24da282430eb2d4edf0003617fe6b..a969fad803ae1abb05113ce15f2476e83df029d9 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -126,6 +126,7 @@ module_param(init, int, 0444);
 struct amc6821_data {
 	struct regmap *regmap;
 	struct mutex update_lock;
+	enum pwm_polarity of_pwm_polarity;
 };
 
 /*
@@ -848,12 +849,23 @@ static int amc6821_detect(struct i2c_client *client, struct i2c_board_info *info
 	return 0;
 }
 
-static enum pwm_polarity amc6821_pwm_polarity(struct i2c_client *client)
+static void amc6821_of_fan_read_data(struct amc6821_data *data,
+				     struct device_node *fan_np)
 {
-	enum pwm_polarity polarity = PWM_POLARITY_NORMAL;
 	struct of_phandle_args args;
-	struct device_node *fan_np;
 
+	data->of_pwm_polarity = PWM_POLARITY_NORMAL;
+
+	if (!of_parse_phandle_with_args(fan_np, "pwms", "#pwm-cells", 0, &args)) {
+		if (args.args_count == 2 && args.args[1] & PWM_POLARITY_INVERTED)
+			data->of_pwm_polarity = PWM_POLARITY_INVERSED;
+
+		of_node_put(args.np);
+	}
+}
+
+static enum pwm_polarity amc6821_pwm_polarity(struct amc6821_data *data)
+{
 	/*
 	 * For backward compatibility, the pwminv module parameter takes
 	 * always the precedence over any other device description
@@ -863,22 +875,7 @@ static enum pwm_polarity amc6821_pwm_polarity(struct i2c_client *client)
 	if (pwminv > 0)
 		return PWM_POLARITY_INVERSED;
 
-	fan_np = of_get_child_by_name(client->dev.of_node, "fan");
-	if (!fan_np)
-		return PWM_POLARITY_NORMAL;
-
-	if (of_parse_phandle_with_args(fan_np, "pwms", "#pwm-cells", 0, &args))
-		goto out;
-	of_node_put(args.np);
-
-	if (args.args_count != 2)
-		goto out;
-
-	if (args.args[1] & PWM_POLARITY_INVERTED)
-		polarity = PWM_POLARITY_INVERSED;
-out:
-	of_node_put(fan_np);
-	return polarity;
+	return data->of_pwm_polarity;
 }
 
 static int amc6821_init_client(struct i2c_client *client, struct amc6821_data *data)
@@ -902,7 +899,7 @@ static int amc6821_init_client(struct i2c_client *client, struct amc6821_data *d
 			return err;
 
 		regval = AMC6821_CONF1_START;
-		if (amc6821_pwm_polarity(client) == PWM_POLARITY_INVERSED)
+		if (amc6821_pwm_polarity(data) == PWM_POLARITY_INVERSED)
 			regval |= AMC6821_CONF1_PWMINV;
 
 		err = regmap_update_bits(regmap, AMC6821_REG_CONF1,
@@ -938,13 +935,21 @@ static const struct regmap_config amc6821_regmap_config = {
 	.cache_type = REGCACHE_MAPLE,
 };
 
+static void amc6821_of_fan_put(void *data)
+{
+	struct device_node *fan_np = data;
+
+	of_node_put(fan_np);
+}
+
 static int amc6821_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct amc6821_data *data;
 	struct device *hwmon_dev;
 	struct regmap *regmap;
-	int err;
+	struct device_node *fan_np;
+	int err = 0;
 
 	data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
 	if (!data)
@@ -956,6 +961,17 @@ static int amc6821_probe(struct i2c_client *client)
 				     "Failed to initialize regmap\n");
 	data->regmap = regmap;
 
+	fan_np = of_get_child_by_name(dev->of_node, "fan");
+	if (fan_np)
+		err = devm_add_action_or_reset(dev, amc6821_of_fan_put, fan_np);
+
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Failed to add fan node release action\n");
+
+	if (fan_np)
+		amc6821_of_fan_read_data(data, fan_np);
+
 	err = amc6821_init_client(client, data);
 	if (err)
 		return err;

-- 
2.43.0

[PATCH 3/3] hwmon: (amc6821) Add cooling device support

[João Paulo Gonçalves]

From: João Paulo Gonçalves <joao.goncalves@toradex.com>

Add support for using the AMC6821 as a cooling device. The AMC6821
registers with the thermal framework only if the `cooling-levels`
property is present in the fan device tree child node. Existing behavior
is unchanged, so the AMC6821 can still be used without the thermal
framework (hwmon only).

Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
---
 drivers/hwmon/amc6821.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 91 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index a969fad803ae1abb05113ce15f2476e83df029d9..f4c2aa71a0e68c071fa4915567327585c20ab5f5 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -26,6 +26,7 @@
 #include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/thermal.h>
 
 #include <dt-bindings/pwm/pwm.h>
 
@@ -126,6 +127,9 @@ module_param(init, int, 0444);
 struct amc6821_data {
 	struct regmap *regmap;
 	struct mutex update_lock;
+	unsigned long fan_state;
+	unsigned long fan_max_state;
+	unsigned int *fan_cooling_levels;
 	enum pwm_polarity of_pwm_polarity;
 };
 
@@ -805,6 +809,56 @@ static const struct hwmon_chip_info amc6821_chip_info = {
 	.info = amc6821_info,
 };
 
+static int amc6821_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state)
+{
+	struct amc6821_data *data = cdev->devdata;
+
+	if (!data)
+		return -EINVAL;
+
+	*state = data->fan_max_state;
+
+	return 0;
+}
+
+static int amc6821_get_cur_state(struct thermal_cooling_device *cdev, unsigned long *state)
+{
+	struct amc6821_data *data = cdev->devdata;
+
+	if (!data)
+		return -EINVAL;
+
+	*state = data->fan_state;
+
+	return 0;
+}
+
+static int amc6821_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+{
+	struct amc6821_data *data = cdev->devdata;
+	int ret;
+
+	if (!data || state > data->fan_max_state)
+		return -EINVAL;
+
+	ret = regmap_write(data->regmap, AMC6821_REG_DCY,
+			   data->fan_cooling_levels[state]);
+	if (ret)
+		return ret;
+
+	data->fan_state = state;
+
+	/* Change to manual mode (software DCY) */
+	return regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
+				  AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1, 0);
+}
+
+static const struct thermal_cooling_device_ops amc6821_cooling_ops = {
+	.get_max_state = amc6821_get_max_state,
+	.get_cur_state = amc6821_get_cur_state,
+	.set_cur_state = amc6821_set_cur_state,
+};
+
 /* Return 0 if detection is successful, -ENODEV otherwise */
 static int amc6821_detect(struct i2c_client *client, struct i2c_board_info *info)
 {
@@ -849,10 +903,12 @@ static int amc6821_detect(struct i2c_client *client, struct i2c_board_info *info
 	return 0;
 }
 
-static void amc6821_of_fan_read_data(struct amc6821_data *data,
-				     struct device_node *fan_np)
+static int amc6821_of_fan_read_data(struct i2c_client *client,
+				    struct amc6821_data *data,
+				    struct device_node *fan_np)
 {
 	struct of_phandle_args args;
+	int num;
 
 	data->of_pwm_polarity = PWM_POLARITY_NORMAL;
 
@@ -862,6 +918,22 @@ static void amc6821_of_fan_read_data(struct amc6821_data *data,
 
 		of_node_put(args.np);
 	}
+
+	num = of_property_count_u32_elems(fan_np, "cooling-levels");
+	if (num <= 0)
+		return 0;
+
+	data->fan_max_state = num - 1;
+
+	data->fan_cooling_levels = devm_kcalloc(&client->dev, num,
+						sizeof(u32),
+						GFP_KERNEL);
+
+	if (!data->fan_cooling_levels)
+		return -ENOMEM;
+
+	return of_property_read_u32_array(fan_np, "cooling-levels",
+					  data->fan_cooling_levels, num);
 }
 
 static enum pwm_polarity amc6821_pwm_polarity(struct amc6821_data *data)
@@ -970,7 +1042,11 @@ static int amc6821_probe(struct i2c_client *client)
 				     "Failed to add fan node release action\n");
 
 	if (fan_np)
-		amc6821_of_fan_read_data(data, fan_np);
+		err = amc6821_of_fan_read_data(client, data, fan_np);
+
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Failed to read fan device tree data\n");
 
 	err = amc6821_init_client(client, data);
 	if (err)
@@ -986,7 +1062,18 @@ static int amc6821_probe(struct i2c_client *client)
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
 							 data, &amc6821_chip_info,
 							 amc6821_groups);
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	if (IS_ERR(hwmon_dev))
+		return dev_err_probe(dev, PTR_ERR(hwmon_dev),
+				     "Failed to initialize hwmon\n");
+
+	if (IS_ENABLED(CONFIG_THERMAL) && fan_np && data->fan_cooling_levels)
+		return PTR_ERR_OR_ZERO(devm_thermal_of_cooling_device_register(dev,
+									       fan_np,
+									       client->name,
+									       data,
+									       &amc6821_cooling_ops));
+
+	return 0;
 }
 
 static const struct i2c_device_id amc6821_id[] = {

-- 
2.43.0

Re: [PATCH 2/3] hwmon: (amc6821) Move reading fan data from OF to a function

[Quentin Schulz]

Hi João Paulo,

On 5/30/25 7:46 PM, João Paulo Gonçalves wrote:
> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> Move fan property reading from OF to a separate function. This keeps OF
> data handling separate from the code logic and makes it easier to add
> features like cooling device support that use the same fan node.
> 
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> ---
>   drivers/hwmon/amc6821.c | 58 +++++++++++++++++++++++++++++++------------------
>   1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 13a789cc85d24da282430eb2d4edf0003617fe6b..a969fad803ae1abb05113ce15f2476e83df029d9 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -126,6 +126,7 @@ module_param(init, int, 0444);
>   struct amc6821_data {
>   	struct regmap *regmap;
>   	struct mutex update_lock;
> +	enum pwm_polarity of_pwm_polarity;

Do we actually need to keep the information about the OF polarity?

Are you trying to handle runtime modification of the pwminv module 
parameter where we could set it to -1 to force reading from the Device 
Tree again? This seems to be possible, c.f. https://lwn.net/Articles/85443/

Otherwise I would have said we just need to store the "computed" 
polarity, the output of amc6821_pwm_polarity instead of going through 
the logic every time we ask for the polarity.

Justify in the commit log why we want to keep the OF value instead of 
the resolved one (with the module param one).

[...]

> @@ -938,13 +935,21 @@ static const struct regmap_config amc6821_regmap_config = {
>   	.cache_type = REGCACHE_MAPLE,
>   };
>   
> +static void amc6821_of_fan_put(void *data)
> +{
> +	struct device_node *fan_np = data;
> +
> +	of_node_put(fan_np);
> +}
> +
>   static int amc6821_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct amc6821_data *data;
>   	struct device *hwmon_dev;
>   	struct regmap *regmap;
> -	int err;
> +	struct device_node *fan_np;
> +	int err = 0;
>   
>   	data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
>   	if (!data)
> @@ -956,6 +961,17 @@ static int amc6821_probe(struct i2c_client *client)
>   				     "Failed to initialize regmap\n");
>   	data->regmap = regmap;
>   
> +	fan_np = of_get_child_by_name(dev->of_node, "fan");
> +	if (fan_np)
> +		err = devm_add_action_or_reset(dev, amc6821_of_fan_put, fan_np);
> +

This seems a bit overkill to me. If I'm not mistaken, we should be able 
to do something simpler such as:

fan_np = of_get_child_by_name(dev->of_node, "fan");
if (fan_np) {
     amc6821_of_fan_read_data(data, fan_np);
     of_node_put(fan_np);
}

(not tested) what do you think? What made you pick the 
devm_add_action_or_reset here? What am I missing :)

Cheers,
Quentin

Re: [PATCH 2/3] hwmon: (amc6821) Move reading fan data from OF to a function

[Francesco Dolcini]

Hello Quentin, João

On Mon, Jun 02, 2025 at 03:21:31PM +0200, Quentin Schulz wrote:
> On 5/30/25 7:46 PM, João Paulo Gonçalves wrote:
> > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > 
> > Move fan property reading from OF to a separate function. This keeps OF
> > data handling separate from the code logic and makes it easier to add
> > features like cooling device support that use the same fan node.
> > 
> > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > ---
> >   drivers/hwmon/amc6821.c | 58 +++++++++++++++++++++++++++++++------------------
> >   1 file changed, 37 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> > index 13a789cc85d24da282430eb2d4edf0003617fe6b..a969fad803ae1abb05113ce15f2476e83df029d9 100644
> > --- a/drivers/hwmon/amc6821.c
> > +++ b/drivers/hwmon/amc6821.c
> > @@ -126,6 +126,7 @@ module_param(init, int, 0444);
> >   struct amc6821_data {
> >   	struct regmap *regmap;
> >   	struct mutex update_lock;
> > +	enum pwm_polarity of_pwm_polarity;
> 
> Do we actually need to keep the information about the OF polarity?

...

> Otherwise I would have said we just need to store the "computed" polarity,
> the output of amc6821_pwm_polarity instead of going through the logic every
> time we ask for the polarity.

I would do this. This pwminv parameter is already weird, given that this
is just a HW configuration, and we have it just for legacy reason.

Francesco