[PATCH v2] hwmon: (tc654) Add thermal_cooling device support

[PATCH v2] hwmon: (tc654) Add thermal_cooling device support

[Christian Lamparter]

Adds thermal_cooling device support to the tc654/tc655
driver. This make it possible to integrate it into a
device-tree supported thermal-zone node as a
cooling device.

I have been using this patch as part of the Netgear WNDR4700
Centria NAS Router support within OpenWrt since 2016.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
v1 -> v2: - Drop imply THERMAL
	  - aligned ( so . everything is lining up
---
 drivers/hwmon/tc654.c | 94 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
index cf2a3acd5c91..c8f511a60769 100644
--- a/drivers/hwmon/tc654.c
+++ b/drivers/hwmon/tc654.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/thermal.h>
 #include <linux/util_macros.h>
 
 enum tc654_regs {
@@ -367,36 +368,30 @@ static ssize_t pwm_mode_store(struct device *dev, struct device_attribute *da,
 static const int tc654_pwm_map[16] = { 77,  88, 102, 112, 124, 136, 148, 160,
 				      172, 184, 196, 207, 219, 231, 243, 255};
 
+static int get_pwm(struct tc654_data *data)
+{
+	if (data->config & TC654_REG_CONFIG_SDM)
+		return 0;
+	else
+		return tc654_pwm_map[data->duty_cycle];
+}
+
 static ssize_t pwm_show(struct device *dev, struct device_attribute *da,
 			char *buf)
 {
 	struct tc654_data *data = tc654_update_client(dev);
-	int pwm;
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
-	if (data->config & TC654_REG_CONFIG_SDM)
-		pwm = 0;
-	else
-		pwm = tc654_pwm_map[data->duty_cycle];
-
-	return sprintf(buf, "%d\n", pwm);
+	return sprintf(buf, "%d\n", get_pwm(data));
 }
 
-static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
-			 const char *buf, size_t count)
+static int _set_pwm(struct tc654_data *data, unsigned long val)
 {
-	struct tc654_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	unsigned long val;
 	int ret;
 
-	if (kstrtoul(buf, 10, &val))
-		return -EINVAL;
-	if (val > 255)
-		return -EINVAL;
-
 	mutex_lock(&data->update_lock);
 
 	if (val == 0)
@@ -416,6 +411,22 @@ static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
 
 out:
 	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
+			 const char *buf, size_t count)
+{
+	struct tc654_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val > 255)
+		return -EINVAL;
+
+	ret = _set_pwm(data, val);
 	return ret < 0 ? ret : count;
 }
 
@@ -447,6 +458,44 @@ static struct attribute *tc654_attrs[] = {
 
 ATTRIBUTE_GROUPS(tc654);
 
+/* cooling device */
+
+static int tc654_get_max_state(struct thermal_cooling_device *cdev,
+			       unsigned long *state)
+{
+	*state = 255;
+	return 0;
+}
+
+static int tc654_get_cur_state(struct thermal_cooling_device *cdev,
+			       unsigned long *state)
+{
+	struct tc654_data *data = tc654_update_client(cdev->devdata);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	*state = get_pwm(data);
+	return 0;
+}
+
+static int tc654_set_cur_state(struct thermal_cooling_device *cdev,
+			       unsigned long state)
+{
+	struct tc654_data *data = tc654_update_client(cdev->devdata);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	return _set_pwm(data, clamp_val(state, 0, 255));
+}
+
+static const struct thermal_cooling_device_ops tc654_fan_cool_ops = {
+	.get_max_state = tc654_get_max_state,
+	.get_cur_state = tc654_get_cur_state,
+	.set_cur_state = tc654_set_cur_state,
+};
+
 /*
  * device probe and removal
  */
@@ -477,7 +526,18 @@ static int tc654_probe(struct i2c_client *client)
 	hwmon_dev =
 	    devm_hwmon_device_register_with_groups(dev, client->name, data,
 						   tc654_groups);
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	if (IS_ENABLED(CONFIG_THERMAL)) {
+		struct thermal_cooling_device *cdev;
+
+		cdev = devm_thermal_of_cooling_device_register(dev, dev->of_node, client->name,
+							       hwmon_dev, &tc654_fan_cool_ops);
+		return PTR_ERR_OR_ZERO(cdev);
+	}
+
+	return 0;
 }
 
 static const struct i2c_device_id tc654_id[] = {
-- 
2.34.1

Re: [PATCH v2] hwmon: (tc654) Add thermal_cooling device support

[Guenter Roeck]

On 2/6/22 11:05, Christian Lamparter wrote:
> Adds thermal_cooling device support to the tc654/tc655
> driver. This make it possible to integrate it into a
> device-tree supported thermal-zone node as a
> cooling device.
> 
> I have been using this patch as part of the Netgear WNDR4700
> Centria NAS Router support within OpenWrt since 2016.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> v1 -> v2: - Drop imply THERMAL
> 	  - aligned ( so . everything is lining up
> ---
>   drivers/hwmon/tc654.c | 94 +++++++++++++++++++++++++++++++++++--------
>   1 file changed, 77 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
> index cf2a3acd5c91..c8f511a60769 100644
> --- a/drivers/hwmon/tc654.c
> +++ b/drivers/hwmon/tc654.c
> @@ -15,6 +15,7 @@
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/slab.h>
> +#include <linux/thermal.h>
>   #include <linux/util_macros.h>
>   
>   enum tc654_regs {
> @@ -367,36 +368,30 @@ static ssize_t pwm_mode_store(struct device *dev, struct device_attribute *da,
>   static const int tc654_pwm_map[16] = { 77,  88, 102, 112, 124, 136, 148, 160,
>   				      172, 184, 196, 207, 219, 231, 243, 255};
>   
> +static int get_pwm(struct tc654_data *data)
> +{
> +	if (data->config & TC654_REG_CONFIG_SDM)
> +		return 0;
> +	else

Nit: else after return is unnecessary. Sorry for not noticing before.

> +		return tc654_pwm_map[data->duty_cycle];
> +}
> +
>   static ssize_t pwm_show(struct device *dev, struct device_attribute *da,
>   			char *buf)
>   {
>   	struct tc654_data *data = tc654_update_client(dev);
> -	int pwm;
>   
>   	if (IS_ERR(data))
>   		return PTR_ERR(data);
>   
> -	if (data->config & TC654_REG_CONFIG_SDM)
> -		pwm = 0;
> -	else
> -		pwm = tc654_pwm_map[data->duty_cycle];
> -
> -	return sprintf(buf, "%d\n", pwm);
> +	return sprintf(buf, "%d\n", get_pwm(data));
>   }
>   
> -static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
> -			 const char *buf, size_t count)
> +static int _set_pwm(struct tc654_data *data, unsigned long val)
>   {
> -	struct tc654_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	unsigned long val;
>   	int ret;
>   
> -	if (kstrtoul(buf, 10, &val))
> -		return -EINVAL;
> -	if (val > 255)
> -		return -EINVAL;
> -
>   	mutex_lock(&data->update_lock);
>   
>   	if (val == 0)
> @@ -416,6 +411,22 @@ static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
>   
>   out:
>   	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
> +			 const char *buf, size_t count)
> +{
> +	struct tc654_data *data = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +	if (val > 255)
> +		return -EINVAL;
> +
> +	ret = _set_pwm(data, val);
>   	return ret < 0 ? ret : count;
>   }
>   
> @@ -447,6 +458,44 @@ static struct attribute *tc654_attrs[] = {
>   
>   ATTRIBUTE_GROUPS(tc654);
>   
> +/* cooling device */
> +
> +static int tc654_get_max_state(struct thermal_cooling_device *cdev,
> +			       unsigned long *state)
> +{
> +	*state = 255;
> +	return 0;
> +}
> +
> +static int tc654_get_cur_state(struct thermal_cooling_device *cdev,
> +			       unsigned long *state)
> +{
> +	struct tc654_data *data = tc654_update_client(cdev->devdata);
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	*state = get_pwm(data);
> +	return 0;
> +}
> +
> +static int tc654_set_cur_state(struct thermal_cooling_device *cdev,
> +			       unsigned long state)
> +{
> +	struct tc654_data *data = tc654_update_client(cdev->devdata);
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	return _set_pwm(data, clamp_val(state, 0, 255));
> +}

Looking into this further, wouldn't it be more appropriate to limit the
cooling states to [0 .. 15], or in other words use data->duty_cycle
directly ? I don't know how the thermal subsystem reacts if it requests
to set the state to a certain value and the actual state is set to
something completely different. Also, it doesn't seem to make much sense
to bother the thermal subsystem with 256 states if the chip really only
supports 16 states.

Thanks,
Guenter

Re: [PATCH v2] hwmon: (tc654) Add thermal_cooling device support

[Christian Lamparter]

On 07/02/2022 09:47, Guenter Roeck wrote:
> On 2/6/22 11:05, Christian Lamparter wrote:
>> Adds thermal_cooling device support to the tc654/tc655
>> driver. This make it possible to integrate it into a
>> device-tree supported thermal-zone node as a
>> cooling device.
>>
>> I have been using this patch as part of the Netgear WNDR4700
>> Centria NAS Router support within OpenWrt since 2016.
>>
>> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
>> ---
>> v1 -> v2: - Drop imply THERMAL
>>       - aligned ( so . everything is lining up
>> ---
>>   drivers/hwmon/tc654.c | 94 +++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 77 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
>> --- a/drivers/hwmon/tc654.c
>> +++ b/drivers/hwmon/tc654.c
>> @@ -367,36 +368,30 @@ static ssize_t pwm_mode_store(struct device *dev, struct device_attribute *da,
>>   static const int tc654_pwm_map[16] = { 77,  88, 102, 112, 124, 136, 148, 160,
>>                         172, 184, 196, 207, 219, 231, 243, 255};
>> +static int get_pwm(struct tc654_data *data)
>> +{
>> +    if (data->config & TC654_REG_CONFIG_SDM)
>> +        return 0;
>> +    else
> 
> Nit: else after return is unnecessary. Sorry for not noticing before.
> 
Noted.

>> @@ -447,6 +458,44 @@ static struct attribute *tc654_attrs[] = {
>>   ATTRIBUTE_GROUPS(tc654);
>> +/* cooling device */
>> +
>> +static int tc654_get_max_state(struct thermal_cooling_device *cdev,
>> +                   unsigned long *state)
>> +{
>> +    *state = 255;
>> +    return 0;
>> +}
>> +
>> +static int tc654_get_cur_state(struct thermal_cooling_device *cdev,
>> +                   unsigned long *state)
>> +{
>> +    struct tc654_data *data = tc654_update_client(cdev->devdata);
>> +
>> +    if (IS_ERR(data))
>> +        return PTR_ERR(data);
>> +
>> +    *state = get_pwm(data);
>> +    return 0;
>> +}
>> +
>> +static int tc654_set_cur_state(struct thermal_cooling_device *cdev,
>> +                   unsigned long state)
>> +{
>> +    struct tc654_data *data = tc654_update_client(cdev->devdata);
>> +
>> +    if (IS_ERR(data))
>> +        return PTR_ERR(data);
>> +
>> +    return _set_pwm(data, clamp_val(state, 0, 255));
>> +}
> 
> Looking into this further, wouldn't it be more appropriate to limit the
> cooling states to [0 .. 15], or in other words use data->duty_cycle
> directly ? I don't know how the thermal subsystem reacts if it requests
> to set the state to a certain value and the actual state is set to
> something completely different. Also, it doesn't seem to make much sense
> to bother the thermal subsystem with 256 states if the chip really only
> supports 16 states.

So there's more: Yes, the chip has 16 PWM states (from Duty Cycle 0=30%
to Duty Cycle 15=100% - each step has a linear 4,66...% increment).
The chip also has a "shutdown state/mode". where the FAN(s) are all turned
off (only the chip's I2C interface remains active for the wake-up signal).

This is why even the current pwm_show() looks so funny.
<https://github.com/torvalds/linux/blob/master/drivers/hwmon/tc654.c#L365-L380>

|static ssize_t pwm_show(struct device *dev, struct device_attribute *da,
|			char *buf)
|{
|	struct tc654_data *data = tc654_update_client(dev);
|	int pwm;
|
|	if (IS_ERR(data))
|		return PTR_ERR(data);
|---- from here ----
|	if (data->config & TC654_REG_CONFIG_SDM)
|		pwm = 0;
|	else
|		pwm = tc654_pwm_map[data->duty_cycle];
|---- to here ----
|	return sprintf(buf, "%d\n", pwm);
|}

If the chip is in that TC654_REG_CONFIG_SDM (SDM=Shutdown Mode), the PWM is 0%
and the fan is off. If it's not in TC654_REG_CONFIG_SDM, then that duty_cycle=0
gives you a PWM with 30%.

The same goes for the pwm_store. And yes, the current behavior through the
hwmon sysfs interface is a bit extreme:

pwm_store with val=0 => SDM engaged - Fans turn off.
pwm_store with val=1 => Fans are on 30%. (There's a steep cliff / steep climb)
...
pwm_store with val=~78 > Fans are on 34%
...
pwm_store with val=255 = Fans are on 100%


So, I would like to keep that shutdown state in there. The Fan on the
WNDR4700 is annoying and only needs to run from time to time.

For now, I fiddled around by adding +1 and -1 here and there. Another
option would be to extend the tc654_pwm_map. But this has the
consequence that it changes the existing behavior on the hwmon interface
as well.

I've inlined a preliminary git diff patch, if you want to take a peek
and maybe already have comments.

I want to test this on the hardware, before sending out a version...
Which I only can do on weekends (so it will be a week, hope that's ok).

Thanks,
Christian
---
diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
index cf2a3acd5c91..f6b6375ffeaf 100644
--- a/drivers/hwmon/tc654.c
+++ b/drivers/hwmon/tc654.c
@@ -15,6 +15,7 @@
  #include <linux/module.h>
  #include <linux/mutex.h>
  #include <linux/slab.h>
+#include <linux/thermal.h>
  #include <linux/util_macros.h>

  enum tc654_regs {
@@ -384,28 +385,19 @@ static ssize_t pwm_show(struct device *dev, struct device_attribute *da,
  	return sprintf(buf, "%d\n", pwm);
  }

-static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
-			 const char *buf, size_t count)
+static int _set_pwm(struct tc654_data *data, unsigned long val)
  {
-	struct tc654_data *data = dev_get_drvdata(dev);
  	struct i2c_client *client = data->client;
-	unsigned long val;
  	int ret;

-	if (kstrtoul(buf, 10, &val))
-		return -EINVAL;
-	if (val > 255)
-		return -EINVAL;
-
  	mutex_lock(&data->update_lock);

-	if (val == 0)
+	if (val == 0) {
  		data->config |= TC654_REG_CONFIG_SDM;
-	else
+	} else {
  		data->config &= ~TC654_REG_CONFIG_SDM;
-
-	data->duty_cycle = find_closest(val, tc654_pwm_map,
-					ARRAY_SIZE(tc654_pwm_map));
+		data->duty_cycle = val - 1;
+	}

  	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
  	if (ret < 0)
@@ -416,6 +408,24 @@ static ssize_t pwm_store(struct device *dev, struct device_attribute *da,

  out:
  	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
+			 const char *buf, size_t count)
+{
+	struct tc654_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val > 255)
+		return -EINVAL;
+	if (val > 0)
+		val = find_closest(val, tc654_pwm_map, ARRAY_SIZE(tc654_pwm_map)) + 1;
+
+	ret = _set_pwm(data, val);
  	return ret < 0 ? ret : count;
  }

@@ -447,6 +457,56 @@ static struct attribute *tc654_attrs[] = {

  ATTRIBUTE_GROUPS(tc654);

+/* thermal cooling device functions
+ *
+ * Account for the "ShutDown Mode (SDM)" state by offseting
+ * the 16 PWM duty cycle states by 1.
+ *
+ * State  0 =   0% PWM | Shutdown - Fan(s) are off
+ * State  1 =  30% PWM | duty_cycle =  0
+ * State  2 = ~35% PWM | duty_cycle =  1
+ * [...]
+ * State 15 = ~95% PWM | duty_cycle = 14
+ * State 16 = 100% PWM | duty_cycle = 15
+ */
+#define TC654_MAX_COOLING_STATES	16
+
+static int tc654_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state)
+{
+	return TC654_MAX_COOLING_STATES;
+}
+
+static int tc654_get_cur_state(struct thermal_cooling_device *cdev, unsigned long *state)
+{
+	struct tc654_data *data = tc654_update_client(cdev->devdata);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (data->config & TC654_REG_CONFIG_SDM)
+		*state = 0;	/* FAN is off */
+	else
+		*state = data->duty_cycle + 1;	/* offset PWM States by 1 */
+
+	return 0;
+}
+
+static int tc654_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+{
+	struct tc654_data *data = tc654_update_client(cdev->devdata);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	return _set_pwm(data, clamp_val(state, 0, TC654_MAX_COOLING_STATES));
+}
+
+static const struct thermal_cooling_device_ops tc654_fan_cool_ops = {
+	.get_max_state = tc654_get_max_state,
+	.get_cur_state = tc654_get_cur_state,
+	.set_cur_state = tc654_set_cur_state,
+};
+
  /*
   * device probe and removal
   */
@@ -477,7 +537,18 @@ static int tc654_probe(struct i2c_client *client)
  	hwmon_dev =
  	    devm_hwmon_device_register_with_groups(dev, client->name, data,
  						   tc654_groups);
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	if (IS_ENABLED(CONFIG_THERMAL)) {
+		struct thermal_cooling_device *cdev;
+
+		cdev = devm_thermal_of_cooling_device_register(dev, dev->of_node, client->name,
+							       hwmon_dev, &tc654_fan_cool_ops);
+		return PTR_ERR_OR_ZERO(cdev);
+	}
+
+	return 0;
  }

  static const struct i2c_device_id tc654_id[] = {
---

Re: [PATCH v2] hwmon: (tc654) Add thermal_cooling device support

[Guenter Roeck]

On 2/7/22 03:10, Christian Lamparter wrote:
> On 07/02/2022 09:47, Guenter Roeck wrote:
>> On 2/6/22 11:05, Christian Lamparter wrote:
>>> Adds thermal_cooling device support to the tc654/tc655
>>> driver. This make it possible to integrate it into a
>>> device-tree supported thermal-zone node as a
>>> cooling device.
>>>
>>> I have been using this patch as part of the Netgear WNDR4700
>>> Centria NAS Router support within OpenWrt since 2016.
>>>
>>> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
>>> ---
>>> v1 -> v2: - Drop imply THERMAL
>>>       - aligned ( so . everything is lining up
>>> ---
>>>   drivers/hwmon/tc654.c | 94 +++++++++++++++++++++++++++++++++++--------
>>>   1 file changed, 77 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
>>> --- a/drivers/hwmon/tc654.c
>>> +++ b/drivers/hwmon/tc654.c
>>> @@ -367,36 +368,30 @@ static ssize_t pwm_mode_store(struct device *dev, struct device_attribute *da,
>>>   static const int tc654_pwm_map[16] = { 77,  88, 102, 112, 124, 136, 148, 160,
>>>                         172, 184, 196, 207, 219, 231, 243, 255};
>>> +static int get_pwm(struct tc654_data *data)
>>> +{
>>> +    if (data->config & TC654_REG_CONFIG_SDM)
>>> +        return 0;
>>> +    else
>>
>> Nit: else after return is unnecessary. Sorry for not noticing before.
>>
> Noted.
> 
>>> @@ -447,6 +458,44 @@ static struct attribute *tc654_attrs[] = {
>>>   ATTRIBUTE_GROUPS(tc654);
>>> +/* cooling device */
>>> +
>>> +static int tc654_get_max_state(struct thermal_cooling_device *cdev,
>>> +                   unsigned long *state)
>>> +{
>>> +    *state = 255;
>>> +    return 0;
>>> +}
>>> +
>>> +static int tc654_get_cur_state(struct thermal_cooling_device *cdev,
>>> +                   unsigned long *state)
>>> +{
>>> +    struct tc654_data *data = tc654_update_client(cdev->devdata);
>>> +
>>> +    if (IS_ERR(data))
>>> +        return PTR_ERR(data);
>>> +
>>> +    *state = get_pwm(data);
>>> +    return 0;
>>> +}
>>> +
>>> +static int tc654_set_cur_state(struct thermal_cooling_device *cdev,
>>> +                   unsigned long state)
>>> +{
>>> +    struct tc654_data *data = tc654_update_client(cdev->devdata);
>>> +
>>> +    if (IS_ERR(data))
>>> +        return PTR_ERR(data);
>>> +
>>> +    return _set_pwm(data, clamp_val(state, 0, 255));
>>> +}
>>
>> Looking into this further, wouldn't it be more appropriate to limit the
>> cooling states to [0 .. 15], or in other words use data->duty_cycle
>> directly ? I don't know how the thermal subsystem reacts if it requests
>> to set the state to a certain value and the actual state is set to
>> something completely different. Also, it doesn't seem to make much sense
>> to bother the thermal subsystem with 256 states if the chip really only
>> supports 16 states.
> 
> So there's more: Yes, the chip has 16 PWM states (from Duty Cycle 0=30%
> to Duty Cycle 15=100% - each step has a linear 4,66...% increment).
> The chip also has a "shutdown state/mode". where the FAN(s) are all turned
> off (only the chip's I2C interface remains active for the wake-up signal).
> 
> This is why even the current pwm_show() looks so funny.
> <https://github.com/torvalds/linux/blob/master/drivers/hwmon/tc654.c#L365-L380>
> 
> |static ssize_t pwm_show(struct device *dev, struct device_attribute *da,
> |            char *buf)
> |{
> |    struct tc654_data *data = tc654_update_client(dev);
> |    int pwm;
> |
> |    if (IS_ERR(data))
> |        return PTR_ERR(data);
> |---- from here ----
> |    if (data->config & TC654_REG_CONFIG_SDM)
> |        pwm = 0;
> |    else
> |        pwm = tc654_pwm_map[data->duty_cycle];
> |---- to here ----
> |    return sprintf(buf, "%d\n", pwm);
> |}
> 
> If the chip is in that TC654_REG_CONFIG_SDM (SDM=Shutdown Mode), the PWM is 0%
> and the fan is off. If it's not in TC654_REG_CONFIG_SDM, then that duty_cycle=0
> gives you a PWM with 30%.
> 
> The same goes for the pwm_store. And yes, the current behavior through the
> hwmon sysfs interface is a bit extreme:
> 
> pwm_store with val=0 => SDM engaged - Fans turn off.
> pwm_store with val=1 => Fans are on 30%. (There's a steep cliff / steep climb)
> ...
> pwm_store with val=~78 > Fans are on 34%
> ...
> pwm_store with val=255 = Fans are on 100%
> 
> 
> So, I would like to keep that shutdown state in there. The Fan on the
> WNDR4700 is annoying and only needs to run from time to time.
> 
> For now, I fiddled around by adding +1 and -1 here and there. Another
> option would be to extend the tc654_pwm_map. But this has the
> consequence that it changes the existing behavior on the hwmon interface
> as well.
> 
> I've inlined a preliminary git diff patch, if you want to take a peek
> and maybe already have comments.
> 

Couple of comments inline.

> I want to test this on the hardware, before sending out a version...
> Which I only can do on weekends (so it will be a week, hope that's ok).
> 

No worries.

> Thanks,
> Christian
> ---
> diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
> index cf2a3acd5c91..f6b6375ffeaf 100644
> --- a/drivers/hwmon/tc654.c
> +++ b/drivers/hwmon/tc654.c
> @@ -15,6 +15,7 @@
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/slab.h>
> +#include <linux/thermal.h>
>   #include <linux/util_macros.h>
> 
>   enum tc654_regs {
> @@ -384,28 +385,19 @@ static ssize_t pwm_show(struct device *dev, struct device_attribute *da,
>       return sprintf(buf, "%d\n", pwm);
>   }
> 
> -static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
> -             const char *buf, size_t count)
> +static int _set_pwm(struct tc654_data *data, unsigned long val)
>   {
> -    struct tc654_data *data = dev_get_drvdata(dev);
>       struct i2c_client *client = data->client;
> -    unsigned long val;
>       int ret;
> 
> -    if (kstrtoul(buf, 10, &val))
> -        return -EINVAL;
> -    if (val > 255)
> -        return -EINVAL;
> -
>       mutex_lock(&data->update_lock);
> 
> -    if (val == 0)
> +    if (val == 0) {
>           data->config |= TC654_REG_CONFIG_SDM;
> -    else
> +    } else {
>           data->config &= ~TC654_REG_CONFIG_SDM;
> -
> -    data->duty_cycle = find_closest(val, tc654_pwm_map,
> -                    ARRAY_SIZE(tc654_pwm_map));
> +        data->duty_cycle = val - 1;
> +    }

This does change existing behavior a bit: The current code always sets
(and thus updates) TC654_REG_DUTY_CYCLE, and sets it to 0 even when setting
TC654_REG_CONFIG_SDM. With the above change, that is no longer the case,
and TC654_REG_DUTY_CYCLE is kept at whatever the previous value was.
Maybe that has no impact, but it might be safer to clear data->duty_cycle
when setting TC654_REG_CONFIG_SDM.

> 
>       ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
>       if (ret < 0)
> @@ -416,6 +408,24 @@ static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
> 
>   out:
>       mutex_unlock(&data->update_lock);
> +    return ret;
> +}
> +
> +static ssize_t pwm_store(struct device *dev, struct device_attribute *da,
> +             const char *buf, size_t count)
> +{
> +    struct tc654_data *data = dev_get_drvdata(dev);
> +    unsigned long val;
> +    int ret;
> +
> +    if (kstrtoul(buf, 10, &val))
> +        return -EINVAL;
> +    if (val > 255)
> +        return -EINVAL;
> +    if (val > 0)
> +        val = find_closest(val, tc654_pwm_map, ARRAY_SIZE(tc654_pwm_map)) + 1;
> +
> +    ret = _set_pwm(data, val);
>       return ret < 0 ? ret : count;
>   }
> 
> @@ -447,6 +457,56 @@ static struct attribute *tc654_attrs[] = {
> 
>   ATTRIBUTE_GROUPS(tc654);
> 
> +/* thermal cooling device functions

/*
  * Please use standard multi-line comments. This is not the networking subsystem.
  */

> + *
> + * Account for the "ShutDown Mode (SDM)" state by offseting

offsetting

> + * the 16 PWM duty cycle states by 1.
> + *
> + * State  0 =   0% PWM | Shutdown - Fan(s) are off
> + * State  1 =  30% PWM | duty_cycle =  0
> + * State  2 = ~35% PWM | duty_cycle =  1
> + * [...]
> + * State 15 = ~95% PWM | duty_cycle = 14
> + * State 16 = 100% PWM | duty_cycle = 15
> + */
> +#define TC654_MAX_COOLING_STATES    16

TC654_MAX_COOLING_STATES is misleading; it is really the maximum cooling state,
not the number of cooling states. Please drop the "S".

> +
> +static int tc654_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state)
> +{
> +    return TC654_MAX_COOLING_STATES;
> +}
> +
> +static int tc654_get_cur_state(struct thermal_cooling_device *cdev, unsigned long *state)
> +{
> +    struct tc654_data *data = tc654_update_client(cdev->devdata);
> +
> +    if (IS_ERR(data))
> +        return PTR_ERR(data);
> +
> +    if (data->config & TC654_REG_CONFIG_SDM)
> +        *state = 0;    /* FAN is off */
> +    else
> +        *state = data->duty_cycle + 1;    /* offset PWM States by 1 */
> +
> +    return 0;
> +}
> +
> +static int tc654_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +{
> +    struct tc654_data *data = tc654_update_client(cdev->devdata);
> +
> +    if (IS_ERR(data))
> +        return PTR_ERR(data);
> +
> +    return _set_pwm(data, clamp_val(state, 0, TC654_MAX_COOLING_STATES));
> +}
> +
> +static const struct thermal_cooling_device_ops tc654_fan_cool_ops = {
> +    .get_max_state = tc654_get_max_state,
> +    .get_cur_state = tc654_get_cur_state,
> +    .set_cur_state = tc654_set_cur_state,
> +};
> +
>   /*
>    * device probe and removal
>    */
> @@ -477,7 +537,18 @@ static int tc654_probe(struct i2c_client *client)
>       hwmon_dev =
>           devm_hwmon_device_register_with_groups(dev, client->name, data,
>                              tc654_groups);
> -    return PTR_ERR_OR_ZERO(hwmon_dev);
> +    if (IS_ERR(hwmon_dev))
> +        return PTR_ERR(hwmon_dev);
> +
> +    if (IS_ENABLED(CONFIG_THERMAL)) {
> +        struct thermal_cooling_device *cdev;
> +
> +        cdev = devm_thermal_of_cooling_device_register(dev, dev->of_node, client->name,
> +                                   hwmon_dev, &tc654_fan_cool_ops);
> +        return PTR_ERR_OR_ZERO(cdev);
> +    }
> +
> +    return 0;
>   }
> 
>   static const struct i2c_device_id tc654_id[] = {
> ---