Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH v4] hwmon: max6650: add thermal cooling device capability
@ 2019-04-19 13:15 Guenter Roeck
  2019-04-19 13:40 ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2019-04-19 13:15 UTC (permalink / raw)
  To: Jean-Francois Dagenais; +Cc: linux-hwmon, jdelvare

On Thu, Apr 18, 2019 at 08:57:22PM -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>

Looking into this more closely, I notice that it has a substantial downside:
It only registers one channel with the thermal subsystem. That is ok for
max6650, but it is a problem for max6651. There, it only registers a single
channel. Worse, extending it later would be difficult since the single-channel
support implies that the thermal data in the dts file would (probably) have to
look completely different for multi-channel support.

How does your proposed dts file look like ? Can we possibly adjust it to work
with multiple channels (ie with all channels of max6651) ?

Thanks,
Guenter

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] hwmon: max6650: add thermal cooling device capability
  2019-04-19 13:15 [PATCH v4] hwmon: max6650: add thermal cooling device capability Guenter Roeck
@ 2019-04-19 13:40 ` Guenter Roeck
  2019-04-20  2:44   ` Jean-Francois Dagenais
  0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2019-04-19 13:40 UTC (permalink / raw)
  To: Jean-Francois Dagenais; +Cc: linux-hwmon, jdelvare

On Fri, Apr 19, 2019 at 06:15:20AM -0700, Guenter Roeck wrote:
> On Thu, Apr 18, 2019 at 08:57:22PM -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>
> 
> Looking into this more closely, I notice that it has a substantial downside:
> It only registers one channel with the thermal subsystem. That is ok for
> max6650, but it is a problem for max6651. There, it only registers a single
> channel. Worse, extending it later would be difficult since the single-channel
> support implies that the thermal data in the dts file would (probably) have to
> look completely different for multi-channel support.
> 
> How does your proposed dts file look like ? Can we possibly adjust it to work
> with multiple channels (ie with all channels of max6651) ?
> 
Never mind, I am talking nonsense. The max6651 has 4 tachometer input channels,
but only one pwm channel. Sorry for the noise. I'll apply your patch.

Can you send me a register dump for the chip (from i2cdump) ? 

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] hwmon: max6650: add thermal cooling device capability
  2019-04-19 13:40 ` Guenter Roeck
@ 2019-04-20  2:44   ` Jean-Francois Dagenais
  0 siblings, 0 replies; 4+ messages in thread
From: Jean-Francois Dagenais @ 2019-04-20  2:44 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-hwmon, jdelvare


> On Apr 19, 2019, at 09:40, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> Can you send me a register dump for the chip (from i2cdump) ? 

Here you go...

~# i2cdump -f -r 0x0-0x16 -y 0 0x1b b
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 00 12 12 ff ff ff ff 00 00 00 00 01 01 01 01    ..??........????
10: 01 01 01 01 1f 1f 02                               ???????         

Our board has the max6650 and doesn't have the tach line connected.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v4] hwmon: max6650: add thermal cooling device capability
  2019-04-18 22:02 [PATCH v2] " Guenter Roeck
@ 2019-04-19  0:57 ` " Jean-Francois Dagenais
  0 siblings, 0 replies; 4+ messages in thread
From: Jean-Francois Dagenais @ 2019-04-19  0:57 UTC (permalink / raw)
  To: linux-hwmon; +Cc: jdelvare, linux, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-19 13:15 [PATCH v4] hwmon: max6650: add thermal cooling device capability Guenter Roeck
2019-04-19 13:40 ` Guenter Roeck
2019-04-20  2:44   ` Jean-Francois Dagenais
  -- strict thread matches above, loose matches on Subject: below --
2019-04-18 22:02 [PATCH v2] " Guenter Roeck
2019-04-19  0:57 ` [PATCH v4] " Jean-Francois Dagenais

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox