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

[PATCH v1] 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.

Note: Driver uses imply THERMAL. The driver previously worked fine with
just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the
devm_thermal_of_cooling_device_register() will be a
"return ERR_PTR(-ENODEV)" stub.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/hwmon/Kconfig |  1 +
 drivers/hwmon/tc654.c | 95 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8df25f1079ba..aa570bb05b38 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1135,6 +1135,7 @@ config SENSORS_MLXREG_FAN
 config SENSORS_TC654
 	tristate "Microchip TC654/TC655 and compatibles"
 	depends on I2C
+	imply THERMAL
 	help
 	  If you say yes here you get support for TC654 and TC655.
 	  The TC654 and TC655 are PWM mode fan speed controllers with
diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
index cf2a3acd5c91..bf7aae62477a 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,19 @@ 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 v1] hwmon: (tc654) Add thermal_cooling device support

[Guenter Roeck]

On Thu, Feb 03, 2022 at 10:28:53PM +0100, 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.
> 
> Note: Driver uses imply THERMAL. The driver previously worked fine with
> just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the
> devm_thermal_of_cooling_device_register() will be a
> "return ERR_PTR(-ENODEV)" stub.

What good does this do ? THERMAL is bool, so it is either there
or it isn't, and the IS_ENABLED() in the code handles that.

According to Kconfig language, "imply THERMAL" means that
THERMAL could be n, m, or y if SENSORS_TC654=y. THERMAL=m would,
if it were supported, be clearly wrong in that case.

Prior to commit 554b3529fe018 ("thermal/drivers/core: Remove the
module Kconfig's option"), THERMAL was tristate, and you would have
needed something like "depends on THERMAL || THERMAL=n". This
is, however, no longer needed.

Given this, I really don't understand what the imply is expected
to do. The above text does not explain that. Please either drop
the imply or provide a better explanation why it is needed.

> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  drivers/hwmon/Kconfig |  1 +
>  drivers/hwmon/tc654.c | 95 +++++++++++++++++++++++++++++++++++--------
>  2 files changed, 79 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 8df25f1079ba..aa570bb05b38 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1135,6 +1135,7 @@ config SENSORS_MLXREG_FAN
>  config SENSORS_TC654
>  	tristate "Microchip TC654/TC655 and compatibles"
>  	depends on I2C
> +	imply THERMAL
>  	help
>  	  If you say yes here you get support for TC654 and TC655.
>  	  The TC654 and TC655 are PWM mode fan speed controllers with
> diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
> index cf2a3acd5c91..bf7aae62477a 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)

Please align continuation lines with '('.

> +{
> +	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,19 @@ 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);

Please align continuation lines with '('. You can go up to 100 columns.

> +		return PTR_ERR_OR_ZERO(cdev);
> +	}
> +
> +	return 0;
>  }
>  
>  static const struct i2c_device_id tc654_id[] = {

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

[Christian Lamparter]

On 05/02/2022 19:23, Guenter Roeck wrote:
> On Thu, Feb 03, 2022 at 10:28:53PM +0100, 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.
>>
>> Note: Driver uses imply THERMAL. The driver previously worked fine with
>> just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the
>> devm_thermal_of_cooling_device_register() will be a
>> "return ERR_PTR(-ENODEV)" stub.
> 
> What good does this do ? THERMAL is bool, so it is either there
> or it isn't, and the IS_ENABLED() in the code handles that.
> 
> According to Kconfig language, "imply THERMAL" means that
> THERMAL could be n, m, or y if SENSORS_TC654=y. THERMAL=m would,
> if it were supported, be clearly wrong in that case.
> 
> Prior to commit 554b3529fe018 ("thermal/drivers/core: Remove the
> module Kconfig's option"), THERMAL was tristate, and you would have
> needed something like "depends on THERMAL || THERMAL=n". This
> is, however, no longer needed.
> 
> Given this, I really don't understand what the imply is expected
> to do. The above text does not explain that. Please either drop
> the imply or provide a better explanation why it is needed.

Oh, okay. Yes, you are right. A lot happend since 2016, I have to admit,
I was updating the driver code, but haven't kept track with the THERMAL
change.

So, I'm planning to address your comments in the following way:
	- Drop imply THERMAL (no replacement)
	- add __maybe_unused to the new functions+struct
	  In the !THERMAL case, compilers will spot the static declarations.
	  If you prefer, I could instead add a #ifdef CONFIG_THERMAL [...] #endif
	  around the new cooling functions and the tc654_fan_cool_ops struct.

	- Changed in tc654_probe() that IS_ENABLED(CONFIG_THERMAL) to
					IS_BUILTIN(CONFIG_THERMAL)

Thanks again for the pointer :).

Cheers,
Christian

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

[Guenter Roeck]

On 2/5/22 13:05, Christian Lamparter wrote:
> On 05/02/2022 19:23, Guenter Roeck wrote:
>> On Thu, Feb 03, 2022 at 10:28:53PM +0100, 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.
>>>
>>> Note: Driver uses imply THERMAL. The driver previously worked fine with
>>> just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the
>>> devm_thermal_of_cooling_device_register() will be a
>>> "return ERR_PTR(-ENODEV)" stub.
>>
>> What good does this do ? THERMAL is bool, so it is either there
>> or it isn't, and the IS_ENABLED() in the code handles that.
>>
>> According to Kconfig language, "imply THERMAL" means that
>> THERMAL could be n, m, or y if SENSORS_TC654=y. THERMAL=m would,
>> if it were supported, be clearly wrong in that case.
>>
>> Prior to commit 554b3529fe018 ("thermal/drivers/core: Remove the
>> module Kconfig's option"), THERMAL was tristate, and you would have
>> needed something like "depends on THERMAL || THERMAL=n". This
>> is, however, no longer needed.
>>
>> Given this, I really don't understand what the imply is expected
>> to do. The above text does not explain that. Please either drop
>> the imply or provide a better explanation why it is needed.
> 
> Oh, okay. Yes, you are right. A lot happend since 2016, I have to admit,
> I was updating the driver code, but haven't kept track with the THERMAL
> change.
> 
> So, I'm planning to address your comments in the following way:
>      - Drop imply THERMAL (no replacement)
>      - add __maybe_unused to the new functions+struct
>        In the !THERMAL case, compilers will spot the static declarations.
>        If you prefer, I could instead add a #ifdef CONFIG_THERMAL [...] #endif
>        around the new cooling functions and the tc654_fan_cool_ops struct.
> 
>      - Changed in tc654_probe() that IS_ENABLED(CONFIG_THERMAL) to
>                      IS_BUILTIN(CONFIG_THERMAL)
> 

Please no #ifdef, and __maybe_unused should not be necessary unless you use #ifdef.
I don't understand why IS_BUILTIN() instead of IS_ENABLED() would be necessary.

/*
  * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
  * otherwise. For boolean options, this is equivalent to
  * IS_ENABLED(CONFIG_FOO).
  */

Thanks,
Guenter