[PATCH v2] hwmon: max6650: add thermal cooling device capability

[PATCH v2] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

This allows max6650 devices to be referenced in dts as a cooling device.

The pwm value seems duplicated in cooling_dev_state but since pwm goes
through rounding logic into data->dac, it is modified and messes with
the thermal zone state algorithms. It's also better to serve a cache
value, thus avoiding periodic actual i2c traffic.

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
Changes in v2:
	Removed left-over debug printk.

 drivers/hwmon/max6650.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 61135a2d0cff..f1cbbaaa1206 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -40,6 +40,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/of_device.h>
+#include <linux/thermal.h>
 
 /*
  * Insmod parameters
@@ -113,6 +114,7 @@ module_param(clock, int, 0444);
 struct max6650_data {
 	struct i2c_client *client;
 	const struct attribute_group *groups[3];
+	struct thermal_cooling_device *cooling_dev;
 	struct mutex update_lock;
 	int nr_fans;
 	char valid; /* zero until following fields are valid */
@@ -125,6 +127,7 @@ struct max6650_data {
 	u8 count;
 	u8 dac;
 	u8 alarm;
+	unsigned long cooling_dev_state;
 };
 
 static const u8 tach_reg[] = {
@@ -694,6 +697,63 @@ static int max6650_init_client(struct max6650_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_THERMAL)
+/* thermal cooling device callbacks */
+static int max6650_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	*state = 255;
+
+	return 0;
+}
+
+static int max6650_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct max6650_data *data = cdev->devdata;
+
+	*state = data->cooling_dev_state;
+
+	return 0;
+}
+
+static int
+max6650_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+{
+	struct max6650_data *data = cdev->devdata;
+	struct i2c_client *client = data->client;
+	int err;
+
+	state = clamp_val(state, 0, 255);
+
+	mutex_lock(&data->update_lock);
+
+	if (data->config & MAX6650_CFG_V12)
+		data->dac = 180 - (180 * state)/255;
+	else
+		data->dac = 76 - (76 * state)/255;
+
+	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+
+	if (!err) {
+		max6650_set_operating_mode(data, state ?
+						   MAX6650_CFG_MODE_OPEN_LOOP :
+						   MAX6650_CFG_MODE_OFF);
+		data->cooling_dev_state = state;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return err < 0 ? err : 0;
+}
+
+static const struct thermal_cooling_device_ops max6650_cooling_ops = {
+	.get_max_state = max6650_get_max_state,
+	.get_cur_state = max6650_get_cur_state,
+	.set_cur_state = max6650_set_cur_state,
+};
+#endif
+
 static int max6650_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -709,6 +769,7 @@ static int max6650_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	data->client = client;
+	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 	data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data;
 
@@ -727,7 +788,34 @@ static int max6650_probe(struct i2c_client *client,
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
 							   client->name, data,
 							   data->groups);
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	err = PTR_ERR_OR_ZERO(hwmon_dev);
+	if (err)
+		return err;
+
+#if IS_ENABLED(CONFIG_THERMAL)
+	data->cooling_dev =
+		thermal_of_cooling_device_register(client->dev.of_node,
+						   id->name, data,
+						   &max6650_cooling_ops);
+	if (IS_ERR(data->cooling_dev)) {
+		err = PTR_ERR(data->cooling_dev);
+		dev_err(&client->dev,
+			"Failed to register as cooling device (%d)\n", err);
+		return err;
+	}
+
+	thermal_cdev_update(data->cooling_dev);
+#endif
+	return 0;
+}
+
+static int max6650_remove(struct i2c_client *client)
+{
+	struct max6650_data *data = i2c_get_clientdata(client);
+
+	thermal_cooling_device_unregister(data->cooling_dev);
+
+	return 0;
 }
 
 static const struct i2c_device_id max6650_id[] = {
@@ -743,6 +831,7 @@ static struct i2c_driver max6650_driver = {
 		.of_match_table = of_match_ptr(max6650_dt_match),
 	},
 	.probe		= max6650_probe,
+	.remove		= max6650_remove,
 	.id_table	= max6650_id,
 };
 
-- 
2.11.0

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

On Thu, Apr 18, 2019 at 12:48:13PM -0400, Jean-Francois Dagenais wrote:
> This allows max6650 devices to be referenced in dts as a cooling device.
> 
> The pwm value seems duplicated in cooling_dev_state but since pwm goes
> through rounding logic into data->dac, it is modified and messes with
> the thermal zone state algorithms. It's also better to serve a cache
> value, thus avoiding periodic actual i2c traffic.
> 
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> ---
> Changes in v2:
> 	Removed left-over debug printk.
> 
>  drivers/hwmon/max6650.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> index 61135a2d0cff..f1cbbaaa1206 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -40,6 +40,7 @@
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/err.h>
>  #include <linux/of_device.h>
> +#include <linux/thermal.h>
>  
>  /*
>   * Insmod parameters
> @@ -113,6 +114,7 @@ module_param(clock, int, 0444);
>  struct max6650_data {
>  	struct i2c_client *client;
>  	const struct attribute_group *groups[3];
> +	struct thermal_cooling_device *cooling_dev;
>  	struct mutex update_lock;
>  	int nr_fans;
>  	char valid; /* zero until following fields are valid */
> @@ -125,6 +127,7 @@ struct max6650_data {
>  	u8 count;
>  	u8 dac;
>  	u8 alarm;
> +	unsigned long cooling_dev_state;
>  };
>  
>  static const u8 tach_reg[] = {
> @@ -694,6 +697,63 @@ static int max6650_init_client(struct max6650_data *data,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_THERMAL)
> +/* thermal cooling device callbacks */
> +static int max6650_get_max_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	*state = 255;
> +
> +	return 0;
> +}
> +
> +static int max6650_get_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	struct max6650_data *data = cdev->devdata;
> +
> +	*state = data->cooling_dev_state;
> +
> +	return 0;
> +}
> +
> +static int
> +max6650_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +{
> +	struct max6650_data *data = cdev->devdata;
> +	struct i2c_client *client = data->client;
> +	int err;
> +
> +	state = clamp_val(state, 0, 255);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (data->config & MAX6650_CFG_V12)
> +		data->dac = 180 - (180 * state)/255;
> +	else
> +		data->dac = 76 - (76 * state)/255;
> +
> +	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
> +
> +	if (!err) {
> +		max6650_set_operating_mode(data, state ?
> +						   MAX6650_CFG_MODE_OPEN_LOOP :
> +						   MAX6650_CFG_MODE_OFF);
> +		data->cooling_dev_state = state;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return err < 0 ? err : 0;
> +}
> +
> +static const struct thermal_cooling_device_ops max6650_cooling_ops = {
> +	.get_max_state = max6650_get_max_state,
> +	.get_cur_state = max6650_get_cur_state,
> +	.set_cur_state = max6650_set_cur_state,
> +};
> +#endif
> +
>  static int max6650_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -709,6 +769,7 @@ static int max6650_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	data->client = client;
> +	i2c_set_clientdata(client, data);
>  	mutex_init(&data->update_lock);
>  	data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data;
>  
> @@ -727,7 +788,34 @@ static int max6650_probe(struct i2c_client *client,
>  	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
>  							   client->name, data,
>  							   data->groups);
> -	return PTR_ERR_OR_ZERO(hwmon_dev);
> +	err = PTR_ERR_OR_ZERO(hwmon_dev);
> +	if (err)
> +		return err;
> +
> +#if IS_ENABLED(CONFIG_THERMAL)

This will result in missing symbols if THERMAL is built as module
and this driver is built into the kernel. You'll have to adjust
Kconfig dependencies accordingly. See other drivers for examples.

> +	data->cooling_dev =
> +		thermal_of_cooling_device_register(client->dev.of_node,
> +						   id->name, data,
> +						   &max6650_cooling_ops);
> +	if (IS_ERR(data->cooling_dev)) {
> +		err = PTR_ERR(data->cooling_dev);
> +		dev_err(&client->dev,
> +			"Failed to register as cooling device (%d)\n", err);
> +		return err;

Why would it be fatal for the driver if this fails ? It wasn't
fatal before.

> +	}
> +
> +	thermal_cdev_update(data->cooling_dev);
> +#endif
> +	return 0;
> +}
> +
> +static int max6650_remove(struct i2c_client *client)
> +{
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +
> +	thermal_cooling_device_unregister(data->cooling_dev);
> +
> +	return 0;
>  }
>  
>  static const struct i2c_device_id max6650_id[] = {
> @@ -743,6 +831,7 @@ static struct i2c_driver max6650_driver = {
>  		.of_match_table = of_match_ptr(max6650_dt_match),
>  	},
>  	.probe		= max6650_probe,
> +	.remove		= max6650_remove,
>  	.id_table	= max6650_id,
>  };
>  
> -- 
> 2.11.0
> 

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

> On Apr 18, 2019, at 13:38, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> +#if IS_ENABLED(CONFIG_THERMAL)
> 
> This will result in missing symbols if THERMAL is built as module
> and this driver is built into the kernel. You'll have to adjust
> Kconfig dependencies accordingly. See other drivers for examples.

Right! Was not a problem for me, but I do remember seing the "funny"
ifdefs around.

> 
>> +	data->cooling_dev =
>> +		thermal_of_cooling_device_register(client->dev.of_node,
>> +						   id->name, data,
>> +						   &max6650_cooling_ops);
>> +	if (IS_ERR(data->cooling_dev)) {
>> +		err = PTR_ERR(data->cooling_dev);
>> +		dev_err(&client->dev,
>> +			"Failed to register as cooling device (%d)\n", err);
>> +		return err;
> 
> Why would it be fatal for the driver if this fails ? It wasn't
> fatal before.

Mmmh, you are right. This assumes that all users of max6650 would now require to
be referred to in some thermal zone. Again, this was not a problem for my test
environment.

Wow, two very egocentric issues. Will fix and send V3, thanks for the review!

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

On Thu, Apr 18, 2019 at 02:02:48PM -0400, Jean-Francois Dagenais wrote:
> 
> > On Apr 18, 2019, at 13:38, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> +#if IS_ENABLED(CONFIG_THERMAL)
> > 
> > This will result in missing symbols if THERMAL is built as module
> > and this driver is built into the kernel. You'll have to adjust
> > Kconfig dependencies accordingly. See other drivers for examples.
> 
> Right! Was not a problem for me, but I do remember seing the "funny"
> ifdefs around.
> 

I know, it is annoying. There was an effort to make THERMAL boolean
instead of tristate, but it looks like that has stalled or was
rejected.

> > 
> >> +	data->cooling_dev =
> >> +		thermal_of_cooling_device_register(client->dev.of_node,
> >> +						   id->name, data,
> >> +						   &max6650_cooling_ops);
> >> +	if (IS_ERR(data->cooling_dev)) {
> >> +		err = PTR_ERR(data->cooling_dev);
> >> +		dev_err(&client->dev,
> >> +			"Failed to register as cooling device (%d)\n", err);
> >> +		return err;
> > 
> > Why would it be fatal for the driver if this fails ? It wasn't
> > fatal before.
> 
> Mmmh, you are right. This assumes that all users of max6650 would now require to
> be referred to in some thermal zone. Again, this was not a problem for my test
> environment.
> 
> Wow, two very egocentric issues. Will fix and send V3, thanks for the review!

Not really egocentric. Just keep in mind that there are existing use cases.

Thanks,
Guenter

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

> On Apr 18, 2019, at 14:06, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> On Thu, Apr 18, 2019 at 02:02:48PM -0400, Jean-Francois Dagenais wrote:
>> 
>>> On Apr 18, 2019, at 13:38, Guenter Roeck <linux@roeck-us.net> wrote:
>>> 
>>>> +#if IS_ENABLED(CONFIG_THERMAL)
>>> 
>>> This will result in missing symbols if THERMAL is built as module
>>> and this driver is built into the kernel. You'll have to adjust
>>> Kconfig dependencies accordingly. See other drivers for examples.
>> 
>> Right! Was not a problem for me, but I do remember seing the "funny"
>> ifdefs around.
>> 
> 
> I know, it is annoying. There was an effort to make THERMAL boolean
> instead of tristate, but it looks like that has stalled or was
> rejected.

Ok, so I had used pwm-fan.c as a reference and simply forgotten to copy over the
Kconfig bit which makes sure if the thermal is a module, then so is the chip
driver. I will put in v3.

> 
>>> 
>>>> +	data->cooling_dev =
>>>> +		thermal_of_cooling_device_register(client->dev.of_node,
>>>> +						   id->name, data,
>>>> +						   &max6650_cooling_ops);
>>>> +	if (IS_ERR(data->cooling_dev)) {
>>>> +		err = PTR_ERR(data->cooling_dev);
>>>> +		dev_err(&client->dev,
>>>> +			"Failed to register as cooling device (%d)\n", err);
>>>> +		return err;
>>> 
>>> Why would it be fatal for the driver if this fails ? It wasn't
>>> fatal before.
>> 
>> Mmmh, you are right. This assumes that all users of max6650 would now require to
>> be referred to in some thermal zone. Again, this was not a problem for my test
>> environment.

And about this bit... I had confused the thermal cooling register with the
thermal_of_sensor register. So if my static analysis of the code in
__thermal_cooling_device_register is correct, the function cannot really fail
unless something is terribly wrong in the kernel... like a malloc failure or
something.

If I am wrong, then pwm-fan.c should also be visited since my max6650_probe code
was copy-pasted from it.


On a related note, I am worried about everyone having to progressively add
thermal-zone support to each fan chip in drivers/hwmon. Would it not be possible
for hwmon to detect, through `enum hwmon_sensor_types` set to `hwmon_fan` in
`struct hwmon_channel_info` to be registered as cooling device in thermal ? Of
course for max665x to work, it would have to be modified to use
`devm_hwmon_device_register_with_info` instead of
`devm_hwmon_device_register_with_groups`

Another possiblity would be to create a new driver in drivers/thermal for
max665x as a thermal cooling device. Or even a
drivers/thermal/thermal-generic-hwmon-fan.c or something, where DTS would
instanciate such a cooling device which references a hwmon device's specific
pwm channel.

Phew! So many possibilities!

[PATCH v3] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

This allows max6650 devices to be referenced in dts as a cooling device.

The pwm value seems duplicated in cooling_dev_state but since pwm goes
through rounding logic into data->dac, it is modified and messes with
the thermal zone state algorithms. It's also better to serve a cache
value, thus avoiding periodic actual i2c traffic.

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
Changes in v2:
        Removed left-over debug printk.
Changes in v3:
        Add missing dependency in Kconfig

 drivers/hwmon/Kconfig   |  1 +
 drivers/hwmon/max6650.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d0f1dfe2bcbb..46d69fcdd48b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -898,6 +898,7 @@ config SENSORS_MAX6642
 config SENSORS_MAX6650
 	tristate "Maxim MAX6650 sensor chip"
 	depends on I2C
+	depends on THERMAL || THERMAL=n
 	help
 	  If you say yes here you get support for the MAX6650 / MAX6651
 	  sensor chips.
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 61135a2d0cff..f1cbbaaa1206 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -40,6 +40,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/of_device.h>
+#include <linux/thermal.h>
 
 /*
  * Insmod parameters
@@ -113,6 +114,7 @@ module_param(clock, int, 0444);
 struct max6650_data {
 	struct i2c_client *client;
 	const struct attribute_group *groups[3];
+	struct thermal_cooling_device *cooling_dev;
 	struct mutex update_lock;
 	int nr_fans;
 	char valid; /* zero until following fields are valid */
@@ -125,6 +127,7 @@ struct max6650_data {
 	u8 count;
 	u8 dac;
 	u8 alarm;
+	unsigned long cooling_dev_state;
 };
 
 static const u8 tach_reg[] = {
@@ -694,6 +697,63 @@ static int max6650_init_client(struct max6650_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_THERMAL)
+/* thermal cooling device callbacks */
+static int max6650_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	*state = 255;
+
+	return 0;
+}
+
+static int max6650_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct max6650_data *data = cdev->devdata;
+
+	*state = data->cooling_dev_state;
+
+	return 0;
+}
+
+static int
+max6650_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+{
+	struct max6650_data *data = cdev->devdata;
+	struct i2c_client *client = data->client;
+	int err;
+
+	state = clamp_val(state, 0, 255);
+
+	mutex_lock(&data->update_lock);
+
+	if (data->config & MAX6650_CFG_V12)
+		data->dac = 180 - (180 * state)/255;
+	else
+		data->dac = 76 - (76 * state)/255;
+
+	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+
+	if (!err) {
+		max6650_set_operating_mode(data, state ?
+						   MAX6650_CFG_MODE_OPEN_LOOP :
+						   MAX6650_CFG_MODE_OFF);
+		data->cooling_dev_state = state;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return err < 0 ? err : 0;
+}
+
+static const struct thermal_cooling_device_ops max6650_cooling_ops = {
+	.get_max_state = max6650_get_max_state,
+	.get_cur_state = max6650_get_cur_state,
+	.set_cur_state = max6650_set_cur_state,
+};
+#endif
+
 static int max6650_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -709,6 +769,7 @@ static int max6650_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	data->client = client;
+	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 	data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data;
 
@@ -727,7 +788,34 @@ static int max6650_probe(struct i2c_client *client,
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
 							   client->name, data,
 							   data->groups);
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	err = PTR_ERR_OR_ZERO(hwmon_dev);
+	if (err)
+		return err;
+
+#if IS_ENABLED(CONFIG_THERMAL)
+	data->cooling_dev =
+		thermal_of_cooling_device_register(client->dev.of_node,
+						   id->name, data,
+						   &max6650_cooling_ops);
+	if (IS_ERR(data->cooling_dev)) {
+		err = PTR_ERR(data->cooling_dev);
+		dev_err(&client->dev,
+			"Failed to register as cooling device (%d)\n", err);
+		return err;
+	}
+
+	thermal_cdev_update(data->cooling_dev);
+#endif
+	return 0;
+}
+
+static int max6650_remove(struct i2c_client *client)
+{
+	struct max6650_data *data = i2c_get_clientdata(client);
+
+	thermal_cooling_device_unregister(data->cooling_dev);
+
+	return 0;
 }
 
 static const struct i2c_device_id max6650_id[] = {
@@ -743,6 +831,7 @@ static struct i2c_driver max6650_driver = {
 		.of_match_table = of_match_ptr(max6650_dt_match),
 	},
 	.probe		= max6650_probe,
+	.remove		= max6650_remove,
 	.id_table	= max6650_id,
 };
 
-- 
2.11.0

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

On Thu, Apr 18, 2019 at 03:54:32PM -0400, Jean-Francois Dagenais wrote:
> 
> 
> > On Apr 18, 2019, at 14:06, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > On Thu, Apr 18, 2019 at 02:02:48PM -0400, Jean-Francois Dagenais wrote:
> >> 
> >>> On Apr 18, 2019, at 13:38, Guenter Roeck <linux@roeck-us.net> wrote:
> >>> 
> >>>> +#if IS_ENABLED(CONFIG_THERMAL)
> >>> 
> >>> This will result in missing symbols if THERMAL is built as module
> >>> and this driver is built into the kernel. You'll have to adjust
> >>> Kconfig dependencies accordingly. See other drivers for examples.
> >> 
> >> Right! Was not a problem for me, but I do remember seing the "funny"
> >> ifdefs around.
> >> 
> > 
> > I know, it is annoying. There was an effort to make THERMAL boolean
> > instead of tristate, but it looks like that has stalled or was
> > rejected.
> 
> Ok, so I had used pwm-fan.c as a reference and simply forgotten to copy over the
> Kconfig bit which makes sure if the thermal is a module, then so is the chip
> driver. I will put in v3.
> 
> > 
> >>> 
> >>>> +	data->cooling_dev =
> >>>> +		thermal_of_cooling_device_register(client->dev.of_node,
> >>>> +						   id->name, data,
> >>>> +						   &max6650_cooling_ops);
> >>>> +	if (IS_ERR(data->cooling_dev)) {
> >>>> +		err = PTR_ERR(data->cooling_dev);
> >>>> +		dev_err(&client->dev,
> >>>> +			"Failed to register as cooling device (%d)\n", err);
> >>>> +		return err;
> >>> 
> >>> Why would it be fatal for the driver if this fails ? It wasn't
> >>> fatal before.
> >> 
> >> Mmmh, you are right. This assumes that all users of max6650 would now require to
> >> be referred to in some thermal zone. Again, this was not a problem for my test
> >> environment.
> 
> And about this bit... I had confused the thermal cooling register with the
> thermal_of_sensor register. So if my static analysis of the code in
> __thermal_cooling_device_register is correct, the function cannot really fail
> unless something is terribly wrong in the kernel... like a malloc failure or
> something.
> 

That explains why the other drivers don't generate an error message.
You might want to reconsider the dev_err() above; it appears it adds zero
value.

> If I am wrong, then pwm-fan.c should also be visited since my max6650_probe code
> was copy-pasted from it.
> 

You are correct, that should not fail either. But two wrongs don't make it right.

> 
> On a related note, I am worried about everyone having to progressively add
> thermal-zone support to each fan chip in drivers/hwmon. Would it not be possible
> for hwmon to detect, through `enum hwmon_sensor_types` set to `hwmon_fan` in
> `struct hwmon_channel_info` to be registered as cooling device in thermal ? Of
> course for max665x to work, it would have to be modified to use
> `devm_hwmon_device_register_with_info` instead of
> `devm_hwmon_device_register_with_groups`
> 

Yes, that would be desirable. Patches welcome. And, yes, the driver would have
to be reworked to use the new API.

> Another possiblity would be to create a new driver in drivers/thermal for
> max665x as a thermal cooling device. Or even a

Only if we also drop the existing driver from hwmon. Fine with me if others
agree and let you do it (one less hwmon driver to maintain).

Note that I just submitted a patch series to introduce
devm_thermal_of_cooling_device_register(). We'll see if the thermal maintainers
accept it.

Guenter

> drivers/thermal/thermal-generic-hwmon-fan.c or something, where DTS would
> instanciate such a cooling device which references a hwmon device's specific
> pwm channel.
> 

Re: [PATCH v3] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

On Thu, Apr 18, 2019 at 04:04:52PM -0400, Jean-Francois Dagenais wrote:
> This allows max6650 devices to be referenced in dts as a cooling device.
> 
> The pwm value seems duplicated in cooling_dev_state but since pwm goes
> through rounding logic into data->dac, it is modified and messes with
> the thermal zone state algorithms. It's also better to serve a cache
> value, thus avoiding periodic actual i2c traffic.
> 
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> ---
> Changes in v2:
>         Removed left-over debug printk.
> Changes in v3:
>         Add missing dependency in Kconfig
> 
>  drivers/hwmon/Kconfig   |  1 +
>  drivers/hwmon/max6650.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d0f1dfe2bcbb..46d69fcdd48b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -898,6 +898,7 @@ config SENSORS_MAX6642
>  config SENSORS_MAX6650
>  	tristate "Maxim MAX6650 sensor chip"
>  	depends on I2C
> +	depends on THERMAL || THERMAL=n
>  	help
>  	  If you say yes here you get support for the MAX6650 / MAX6651
>  	  sensor chips.
> diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> index 61135a2d0cff..f1cbbaaa1206 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -40,6 +40,7 @@
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/err.h>
>  #include <linux/of_device.h>
> +#include <linux/thermal.h>
>  
>  /*
>   * Insmod parameters
> @@ -113,6 +114,7 @@ module_param(clock, int, 0444);
>  struct max6650_data {
>  	struct i2c_client *client;
>  	const struct attribute_group *groups[3];
> +	struct thermal_cooling_device *cooling_dev;
>  	struct mutex update_lock;
>  	int nr_fans;
>  	char valid; /* zero until following fields are valid */
> @@ -125,6 +127,7 @@ struct max6650_data {
>  	u8 count;
>  	u8 dac;
>  	u8 alarm;
> +	unsigned long cooling_dev_state;
>  };
>  
>  static const u8 tach_reg[] = {
> @@ -694,6 +697,63 @@ static int max6650_init_client(struct max6650_data *data,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_THERMAL)
> +/* thermal cooling device callbacks */
> +static int max6650_get_max_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	*state = 255;
> +
> +	return 0;
> +}
> +
> +static int max6650_get_cur_state(struct thermal_cooling_device *cdev,
> +				 unsigned long *state)
> +{
> +	struct max6650_data *data = cdev->devdata;
> +
> +	*state = data->cooling_dev_state;
> +
> +	return 0;
> +}
> +
> +static int
> +max6650_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +{
> +	struct max6650_data *data = cdev->devdata;
> +	struct i2c_client *client = data->client;
> +	int err;
> +
> +	state = clamp_val(state, 0, 255);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (data->config & MAX6650_CFG_V12)
> +		data->dac = 180 - (180 * state)/255;
> +	else
> +		data->dac = 76 - (76 * state)/255;
> +
> +	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
> +
> +	if (!err) {
> +		max6650_set_operating_mode(data, state ?
> +						   MAX6650_CFG_MODE_OPEN_LOOP :
> +						   MAX6650_CFG_MODE_OFF);
> +		data->cooling_dev_state = state;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return err < 0 ? err : 0;
> +}
> +
> +static const struct thermal_cooling_device_ops max6650_cooling_ops = {
> +	.get_max_state = max6650_get_max_state,
> +	.get_cur_state = max6650_get_cur_state,
> +	.set_cur_state = max6650_set_cur_state,
> +};
> +#endif
> +
>  static int max6650_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -709,6 +769,7 @@ static int max6650_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	data->client = client;
> +	i2c_set_clientdata(client, data);
>  	mutex_init(&data->update_lock);
>  	data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data;
>  
> @@ -727,7 +788,34 @@ static int max6650_probe(struct i2c_client *client,
>  	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
>  							   client->name, data,
>  							   data->groups);
> -	return PTR_ERR_OR_ZERO(hwmon_dev);
> +	err = PTR_ERR_OR_ZERO(hwmon_dev);
> +	if (err)
> +		return err;
> +
> +#if IS_ENABLED(CONFIG_THERMAL)
> +	data->cooling_dev =
> +		thermal_of_cooling_device_register(client->dev.of_node,
> +						   id->name, data,
> +						   &max6650_cooling_ops);
> +	if (IS_ERR(data->cooling_dev)) {
> +		err = PTR_ERR(data->cooling_dev);
> +		dev_err(&client->dev,
> +			"Failed to register as cooling device (%d)\n", err);

As mentioned in my other mail, this message adds zero value. Please drop.

> +		return err;

Sorry, I won't accept this.

Guenter

> +	}
> +
> +	thermal_cdev_update(data->cooling_dev);
> +#endif
> +	return 0;
> +}
> +
> +static int max6650_remove(struct i2c_client *client)
> +{
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +
> +	thermal_cooling_device_unregister(data->cooling_dev);
> +
> +	return 0;
>  }
>  
>  static const struct i2c_device_id max6650_id[] = {
> @@ -743,6 +831,7 @@ static struct i2c_driver max6650_driver = {
>  		.of_match_table = of_match_ptr(max6650_dt_match),
>  	},
>  	.probe		= max6650_probe,
> +	.remove		= max6650_remove,
>  	.id_table	= max6650_id,
>  };
>  
> -- 
> 2.11.0
> 

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

> On Apr 18, 2019, at 16:12, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> That explains why the other drivers don't generate an error message.
> You might want to reconsider the dev_err() above; it appears it adds zero
> value.
> 
>> If I am wrong, then pwm-fan.c should also be visited since my max6650_probe code
>> was copy-pasted from it.
>> 
> 
> You are correct, that should not fail either. But two wrongs don't make it right.

So you think any fan chip driver probe function in hwmon should succeed even if
(devm_)thermal_of_cooling_device_register fails?. Mmmh... perhaps I am missing
something, but I tend to disagree. That means that for example my system boots
normally without any errors, yet the thermal-zone I defined, which is there to
prevent my system from burning out, would remain incomplete (without required
cooling device) and yet no errors came out of the kernel.

I just tested it and that's exactly what's happening! Yikes.

> 
>> 
>> On a related note, I am worried about everyone having to progressively add
>> thermal-zone support to each fan chip in drivers/hwmon. Would it not be possible
>> for hwmon to detect, through `enum hwmon_sensor_types` set to `hwmon_fan` in
>> `struct hwmon_channel_info` to be registered as cooling device in thermal ? Of
>> course for max665x to work, it would have to be modified to use
>> `devm_hwmon_device_register_with_info` instead of
>> `devm_hwmon_device_register_with_groups`
>> 
> 
> Yes, that would be desirable. Patches welcome. And, yes, the driver would have
> to be reworked to use the new API.
> 
>> Another possiblity would be to create a new driver in drivers/thermal for
>> max665x as a thermal cooling device. Or even a
> 
> Only if we also drop the existing driver from hwmon. Fine with me if others
> agree and let you do it (one less hwmon driver to maintain).
> 
> Note that I just submitted a patch series to introduce
> devm_thermal_of_cooling_device_register(). We'll see if the thermal maintainers
> accept it.

Saw that. For a minute I really thought you pulled out a series which did exactly
what I was talking about (i.e. auto-registering fans in thermal cooling)!!

Since my problem is fixed in the short term, and am very stretched for time with
our project, I will not have time to do this now. Perhaps one day... ;)

Re: [PATCH v3] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

> On Apr 18, 2019, at 16:13, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> +	if (IS_ERR(data->cooling_dev)) {
>> +		err = PTR_ERR(data->cooling_dev);
>> +		dev_err(&client->dev,
>> +			"Failed to register as cooling device (%d)\n", err);
> 
> As mentioned in my other mail, this message adds zero value. Please drop.

How does one distinguish the different failures which can occur within a probe
function then. I know it is only useful for system integrators while debugging
DTS for example. But I find my self always having to insert these kind of messages
at each "return" statements of a failing probe implementation. I just think it's
nice for developers which can use the info to quickly troubleshoot the problem.

I won't fight this though. You are maintainer of this subsys so...

> 
>> +		return err;
> 
> Sorry, I won't accept this.

You mean, you won't accept v3, but a v4 without the dev_err?

Perhaps also, we should wait a v4 which would use your new devm_register... ?

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

On Thu, Apr 18, 2019 at 04:39:35PM -0400, Jean-Francois Dagenais wrote:
> 
> 
> > On Apr 18, 2019, at 16:12, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > That explains why the other drivers don't generate an error message.
> > You might want to reconsider the dev_err() above; it appears it adds zero
> > value.
> > 
> >> If I am wrong, then pwm-fan.c should also be visited since my max6650_probe code
> >> was copy-pasted from it.
> >> 
> > 
> > You are correct, that should not fail either. But two wrongs don't make it right.
> 
> So you think any fan chip driver probe function in hwmon should succeed even if
> (devm_)thermal_of_cooling_device_register fails?. Mmmh... perhaps I am missing
> something, but I tend to disagree. That means that for example my system boots
> normally without any errors, yet the thermal-zone I defined, which is there to
> prevent my system from burning out, would remain incomplete (without required
> cooling device) and yet no errors came out of the kernel.
> 
> I just tested it and that's exactly what's happening! Yikes.
> 
You would still get a warning message. Better than failing to load
the driver completely. On top of that, this used to work before.
Plus, you said it only happens in severe situations, ie if the kernel
is out of memory, and in thact case there would be a traceback.
Sorry, I don't get your point.

Guenter

Re: [PATCH v3] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

On Thu, Apr 18, 2019 at 04:45:00PM -0400, Jean-Francois Dagenais wrote:
> 
> > On Apr 18, 2019, at 16:13, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> +	if (IS_ERR(data->cooling_dev)) {
> >> +		err = PTR_ERR(data->cooling_dev);
> >> +		dev_err(&client->dev,
> >> +			"Failed to register as cooling device (%d)\n", err);
> > 
> > As mentioned in my other mail, this message adds zero value. Please drop.
> 
> How does one distinguish the different failures which can occur within a probe
> function then. I know it is only useful for system integrators while debugging
> DTS for example. But I find my self always having to insert these kind of messages
> at each "return" statements of a failing probe implementation. I just think it's
> nice for developers which can use the info to quickly troubleshoot the problem.
> 
> I won't fight this though. You are maintainer of this subsys so...
> 

Yes, I know there are peple who like a lot of messages. I don't - it happend
to me too often that the actual important message(s) get lost in the noise.
Anyway, see below.

> > 
> >> +		return err;
> > 
> > Sorry, I won't accept this.
> 
> You mean, you won't accept v3, but a v4 without the dev_err?
> 
I won't accept v3 due to the error return, which may be considered a
regression for those who don't care about using the driver with the
thermal subsystem.

If you insist on an extra message, I'll accept a dev_warn.

> Perhaps also, we should wait a v4 which would use your new devm_register... ?

Your call, but there is no guarantee that this series will ever be
accepted. The thermal maintainers may object to the new devm function.

Guenter

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

> On Apr 18, 2019, at 16:50, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> normally without any errors, yet the thermal-zone I defined, which is there to
>> prevent my system from burning out, would remain incomplete (without required
>> cooling device) and yet no errors came out of the kernel.
>> 
>> I just tested it and that's exactly what's happening! Yikes.
>> 
> You would still get a warning message. Better than failing to load
> the driver completely. On top of that, this used to work before.
> Plus, you said it only happens in severe situations, ie if the kernel
> is out of memory, and in thact case there would be a traceback.
> Sorry, I don't get your point.

Ok, I see. We're having the same discussion than on the v3 submission then.

If I change the dev_err to a dev_warn, but still return 0 on
thermal_of_cooling_device_register failure, you would apply the patch. It sounds
like a compromise.

Completely failing the probe call or partially succeed it results in the same
behaviour for thermal cooling use cases. But the dev_warn definitely becomes
important though.

I will v4 with that if you agree and leave the devm_ thermal register for
another day since I agree the series might be delayed too long (or
indefinitely).

Cheers!

Re: [PATCH v2] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

On Thu, Apr 18, 2019 at 05:33:13PM -0400, Jean-Francois Dagenais wrote:
> 
> > On Apr 18, 2019, at 16:50, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> normally without any errors, yet the thermal-zone I defined, which is there to
> >> prevent my system from burning out, would remain incomplete (without required
> >> cooling device) and yet no errors came out of the kernel.
> >> 
> >> I just tested it and that's exactly what's happening! Yikes.
> >> 
> > You would still get a warning message. Better than failing to load
> > the driver completely. On top of that, this used to work before.
> > Plus, you said it only happens in severe situations, ie if the kernel
> > is out of memory, and in thact case there would be a traceback.
> > Sorry, I don't get your point.
> 
> Ok, I see. We're having the same discussion than on the v3 submission then.
> 
> If I change the dev_err to a dev_warn, but still return 0 on
> thermal_of_cooling_device_register failure, you would apply the patch. It sounds
> like a compromise.
> 
> Completely failing the probe call or partially succeed it results in the same
> behaviour for thermal cooling use cases. But the dev_warn definitely becomes
> important though.
> 
> I will v4 with that if you agree and leave the devm_ thermal register for
> another day since I agree the series might be delayed too long (or
> indefinitely).
> 
Ok.

Guenter

[PATCH v4] hwmon: max6650: add thermal cooling device capability

[Jean-Francois Dagenais]

This allows max6650 devices to be referenced in dts as a cooling device.

The pwm value seems duplicated in cooling_dev_state but since pwm goes
through rounding logic into data->dac, it is modified and messes with
the thermal zone state algorithms. It's also better to serve a cache
value, thus avoiding periodic actual i2c traffic.

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
Changes in v2:
        - Removed left-over debug printk.
Changes in v3:
        - Add missing dependency in Kconfig
Changes in v4:
	- Remove useless comment "thermal cooling device callbacks"
	- fix max6650_set_cur_state signature declaration style
	- Only warn when thermal_of_cooling_device_register fails

 drivers/hwmon/Kconfig   |  1 +
 drivers/hwmon/max6650.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d0f1dfe2bcbb..46d69fcdd48b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -898,6 +898,7 @@ config SENSORS_MAX6642
 config SENSORS_MAX6650
 	tristate "Maxim MAX6650 sensor chip"
 	depends on I2C
+	depends on THERMAL || THERMAL=n
 	help
 	  If you say yes here you get support for the MAX6650 / MAX6651
 	  sensor chips.
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 61135a2d0cff..137395ea5e95 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -40,6 +40,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/of_device.h>
+#include <linux/thermal.h>
 
 /*
  * Insmod parameters
@@ -113,6 +114,7 @@ module_param(clock, int, 0444);
 struct max6650_data {
 	struct i2c_client *client;
 	const struct attribute_group *groups[3];
+	struct thermal_cooling_device *cooling_dev;
 	struct mutex update_lock;
 	int nr_fans;
 	char valid; /* zero until following fields are valid */
@@ -125,6 +127,7 @@ struct max6650_data {
 	u8 count;
 	u8 dac;
 	u8 alarm;
+	unsigned long cooling_dev_state;
 };
 
 static const u8 tach_reg[] = {
@@ -694,6 +697,63 @@ static int max6650_init_client(struct max6650_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_THERMAL)
+
+static int max6650_get_max_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	*state = 255;
+
+	return 0;
+}
+
+static int max6650_get_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long *state)
+{
+	struct max6650_data *data = cdev->devdata;
+
+	*state = data->cooling_dev_state;
+
+	return 0;
+}
+
+static int max6650_set_cur_state(struct thermal_cooling_device *cdev,
+				 unsigned long state)
+{
+	struct max6650_data *data = cdev->devdata;
+	struct i2c_client *client = data->client;
+	int err;
+
+	state = clamp_val(state, 0, 255);
+
+	mutex_lock(&data->update_lock);
+
+	if (data->config & MAX6650_CFG_V12)
+		data->dac = 180 - (180 * state)/255;
+	else
+		data->dac = 76 - (76 * state)/255;
+
+	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+
+	if (!err) {
+		max6650_set_operating_mode(data, state ?
+						   MAX6650_CFG_MODE_OPEN_LOOP :
+						   MAX6650_CFG_MODE_OFF);
+		data->cooling_dev_state = state;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return err < 0 ? err : 0;
+}
+
+static const struct thermal_cooling_device_ops max6650_cooling_ops = {
+	.get_max_state = max6650_get_max_state,
+	.get_cur_state = max6650_get_cur_state,
+	.set_cur_state = max6650_set_cur_state,
+};
+#endif
+
 static int max6650_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -709,6 +769,7 @@ static int max6650_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	data->client = client;
+	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 	data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data;
 
@@ -727,7 +788,33 @@ static int max6650_probe(struct i2c_client *client,
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
 							   client->name, data,
 							   data->groups);
-	return PTR_ERR_OR_ZERO(hwmon_dev);
+	err = PTR_ERR_OR_ZERO(hwmon_dev);
+	if (err)
+		return err;
+
+#if IS_ENABLED(CONFIG_THERMAL)
+	data->cooling_dev =
+		thermal_of_cooling_device_register(client->dev.of_node,
+						   id->name, data,
+						   &max6650_cooling_ops);
+	if (IS_ERR(data->cooling_dev))
+		dev_warn(&client->dev,
+			 "thermal cooling device register failed: %d\n",
+			 PTR_ERR(data->cooling_dev));
+	else
+		thermal_cdev_update(data->cooling_dev);
+#endif
+	return 0;
+}
+
+static int max6650_remove(struct i2c_client *client)
+{
+	struct max6650_data *data = i2c_get_clientdata(client);
+
+	if (!IS_ERR(data->cooling_dev))
+		thermal_cooling_device_unregister(data->cooling_dev);
+
+	return 0;
 }
 
 static const struct i2c_device_id max6650_id[] = {
@@ -743,6 +830,7 @@ static struct i2c_driver max6650_driver = {
 		.of_match_table = of_match_ptr(max6650_dt_match),
 	},
 	.probe		= max6650_probe,
+	.remove		= max6650_remove,
 	.id_table	= max6650_id,
 };
 
-- 
2.11.0