[hwmon:hwmon-next 119/119] drivers/hwmon/max6650.c:798:10: warning: passing argument 2 of 'thermal_of_cooling_device_register' discards 'const' qualifier from pointer target type

[hwmon:hwmon-next 119/119] drivers/hwmon/max6650.c:798:10: warning: passing argument 2 of 'thermal_of_cooling_device_register' discards 'const' qualifier from pointer target type

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
head:   11d9bd51ecfe00a422920854d9585417f153cf99
commit: 11d9bd51ecfe00a422920854d9585417f153cf99 [119/119] hwmon: (max6650) add thermal cooling device capability
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        git checkout 11d9bd51ecfe00a422920854d9585417f153cf99
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


All warnings (new ones prefixed by >>):

   drivers/hwmon/max6650.c: In function 'max6650_probe':
>> drivers/hwmon/max6650.c:798:10: warning: passing argument 2 of 'thermal_of_cooling_device_register' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
             id->name, data,
             ^~
   In file included from drivers/hwmon/max6650.c:43:0:
   include/linux/thermal.h:448:1: note: expected 'char *' but argument is of type 'const char *'
    thermal_of_cooling_device_register(struct device_node *np, char *, void *,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/i2c.h:30:0,
                    from drivers/hwmon/max6650.c:38:
>> drivers/hwmon/max6650.c:802:5: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
        "thermal cooling device register failed: %d\n",
        ^
   include/linux/device.h:1400:22: note: in definition of macro 'dev_fmt'
    #define dev_fmt(fmt) fmt
                         ^~~
>> drivers/hwmon/max6650.c:801:3: note: in expansion of macro 'dev_warn'
      dev_warn(&client->dev,
      ^~~~~~~~

vim +798 drivers/hwmon/max6650.c

   756	
   757	static int max6650_probe(struct i2c_client *client,
   758				 const struct i2c_device_id *id)
   759	{
   760		struct device *dev = &client->dev;
   761		const struct of_device_id *of_id =
   762			of_match_device(of_match_ptr(max6650_dt_match), dev);
   763		struct max6650_data *data;
   764		struct device *hwmon_dev;
   765		int err;
   766	
   767		data = devm_kzalloc(dev, sizeof(struct max6650_data), GFP_KERNEL);
   768		if (!data)
   769			return -ENOMEM;
   770	
   771		data->client = client;
   772		i2c_set_clientdata(client, data);
   773		mutex_init(&data->update_lock);
   774		data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data;
   775	
   776		/*
   777		 * Initialize the max6650 chip
   778		 */
   779		err = max6650_init_client(data, client);
   780		if (err)
   781			return err;
   782	
   783		data->groups[0] = &max6650_group;
   784		/* 3 additional fan inputs for the MAX6651 */
   785		if (data->nr_fans == 4)
   786			data->groups[1] = &max6651_group;
   787	
   788		hwmon_dev = devm_hwmon_device_register_with_groups(dev,
   789								   client->name, data,
   790								   data->groups);
   791		err = PTR_ERR_OR_ZERO(hwmon_dev);
   792		if (err)
   793			return err;
   794	
   795	#if IS_ENABLED(CONFIG_THERMAL)
   796		data->cooling_dev =
   797			thermal_of_cooling_device_register(client->dev.of_node,
 > 798							   id->name, data,
   799							   &max6650_cooling_ops);
   800		if (IS_ERR(data->cooling_dev))
 > 801			dev_warn(&client->dev,
 > 802				 "thermal cooling device register failed: %d\n",
   803				 PTR_ERR(data->cooling_dev));
   804		else
   805			thermal_cdev_update(data->cooling_dev);
   806	#endif
   807		return 0;
   808	}
   809	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41720 bytes --]

Re: [hwmon:hwmon-next 119/119] drivers/hwmon/max6650.c:798:10: warning: passing argument 2 of 'thermal_of_cooling_device_register' discards 'const' qualifier from pointer target type

[Jean-Francois Dagenais]

> On Apr 19, 2019, at 11:36, kbuild test robot <lkp@intel.com> wrote:
> 
>>> drivers/hwmon/max6650.c:798:10: warning: passing argument 2 of 'thermal_of_cooling_device_register' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>             id->name, data,
>             ^~
>   In file included from drivers/hwmon/max6650.c:43:0:
>   include/linux/thermal.h:448:1: note: expected 'char *' but argument is of type 'const char *'
>    thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Hmmm... I forgot I was indeed using a patched version of thermal_of_cooling_device_register (submitted already: https://marc.info/?l=linux-pm&m=155560541430929&w=2)

I will v5 using client->name instead, which is non-const char[].

>   In file included from include/linux/i2c.h:30:0,
>                    from drivers/hwmon/max6650.c:38:
>>> drivers/hwmon/max6650.c:802:5: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
>        "thermal cooling device register failed: %d\n",
>        ^
>   include/linux/device.h:1400:22: note: in definition of macro 'dev_fmt'
>    #define dev_fmt(fmt) fmt
>                         ^~~
>>> drivers/hwmon/max6650.c:801:3: note: in expansion of macro 'dev_warn'
>      dev_warn(&client->dev,

Been' compiling through yocto, which puts compiler outputs in log files. Warnings don't fail compilation so bitbake doesn't print them. Excuses, excuses... shame.

V5!

[PATCH v5] 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
Changes in v5:
	- Use switch to non-const client->name to prevent compiler warning
	- fix incorrect length of printk format long int

 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..0607d54f8090 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,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,
+						   client->name, data,
+						   &max6650_cooling_ops);
+	if (IS_ERR(data->cooling_dev))
+		dev_warn(&client->dev,
+			 "thermal cooling device register failed: %ld\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 +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 v5] hwmon: max6650: add thermal cooling device capability

[Guenter Roeck]

Hi,

On Fri, Apr 19, 2019 at 11:02:33PM -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>

I finally had some time to play with the driver and patch, and I do have
a concern after all. Please see below.

> ---
> 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
> Changes in v5:
> 	- Use switch to non-const client->name to prevent compiler warning
> 	- fix incorrect length of printk format long int
> 
>  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..0607d54f8090 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,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,
> +						   client->name, data,
> +						   &max6650_cooling_ops);
> +	if (IS_ERR(data->cooling_dev))
> +		dev_warn(&client->dev,
> +			 "thermal cooling device register failed: %ld\n",
> +			 PTR_ERR(data->cooling_dev));
> +	else
> +		thermal_cdev_update(data->cooling_dev);

On a system which has CONFIG_THERMAL enabled but does not really utilize
it, the call to thermal_cdev_update() will result in the fan being stopped
immediately since max6650_set_cur_state() is called with state=0.

I see that this function called from pwm_fan.c. It is not called from
gpio-fan.c or npcm750-pwm-fan.c. Other than from pwm-fan.c, it is only
called from thermal governors, and not from any other thermal drivers
registering a thermal cooling device. This leads me to believe that
the calls from here and from pwm_fan.c may be wrong.

Are you sure that this call needed ? If so, can you explain ?
What happens in your system if you remove this call ?

Thanks,
Guenter

> +#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 +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,
>  };
>  

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

[Jean-Francois Dagenais]

> On Apr 22, 2019, at 09:40, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> On a system which has CONFIG_THERMAL enabled but does not really utilize
> it, the call to thermal_cdev_update() will result in the fan being stopped
> immediately since max6650_set_cur_state() is called with state=0.
> 
> I see that this function called from pwm_fan.c. It is not called from
> gpio-fan.c or npcm750-pwm-fan.c. Other than from pwm-fan.c, it is only
> called from thermal governors, and not from any other thermal drivers
> registering a thermal cooling device. This leads me to believe that
> the calls from here and from pwm_fan.c may be wrong.
> 
> Are you sure that this call needed ? If so, can you explain ?
> What happens in your system if you remove this call ?

Yes, you are correct. It is a carry over from pwm-fan.c and so probably wrong
there too. I have tested without the thermal_cdev_update call in probe() and
basically, if the cooling device is not part of a TZ, then set_cur_state is
never called. If part of a TZ, it is called by the governor (step_wise in my
case) as required and all works as expected.

Cheers!

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

[Guenter Roeck]

On 4/23/19 5:45 AM, Jean-Francois Dagenais wrote:
> 
>> On Apr 22, 2019, at 09:40, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On a system which has CONFIG_THERMAL enabled but does not really utilize
>> it, the call to thermal_cdev_update() will result in the fan being stopped
>> immediately since max6650_set_cur_state() is called with state=0.
>>
>> I see that this function called from pwm_fan.c. It is not called from
>> gpio-fan.c or npcm750-pwm-fan.c. Other than from pwm-fan.c, it is only
>> called from thermal governors, and not from any other thermal drivers
>> registering a thermal cooling device. This leads me to believe that
>> the calls from here and from pwm_fan.c may be wrong.
>>
>> Are you sure that this call needed ? If so, can you explain ?
>> What happens in your system if you remove this call ?
> 
> Yes, you are correct. It is a carry over from pwm-fan.c and so probably wrong
> there too. I have tested without the thermal_cdev_update call in probe() and
> basically, if the cooling device is not part of a TZ, then set_cur_state is
> never called. If part of a TZ, it is called by the governor (step_wise in my
> case) as required and all works as expected.
> 

Excellent - thanks for testing and getting back.

Guenter