[PATCH] lm63: Added thermal zone support

[PATCH] lm63: Added thermal zone support

[Azat Gismatov]

There are target systems where temperature sensor in the lm63
is not the only temperature sensor in the system used to make
thermal decisions. For example temperature sensors aren't
connected to the lm63 (sensors are part of CPU and supported
by own linux driver). So automatic fan control by the lm63 chip
doesn't work for that systems.
Using of thermal zone is much more convenient for embedded
systems, than fan-control, or any other userspace software,
because It allow to get smaller software footprint. Also with
thermal zone we can store all cooling settings in devicetree,
which is easier to maintain for embedded systems, than some
userspace software.
Added support to use manual pwm mode by  property  in device
tree "#pwm-cells" (#pwm-cells = <2>). This property checkes
the presence of a pwm chip. If the property is set in the
device tree, we will use the manual pwm mode by default. From
the user mode it will not be possible to change automatical
mode in the sysfs attributes. So pwm_chip_add () is used. In
the device tree lable pwm_fan is used as a cooling device,
it has a "pwms" property in description which is a link to
the pwm device label. Driver pwm-fan registers thermal_zone
cooling device.
Example lm96163, pwm-fan device tree:
pwm: lm96163@4c {
	compatible = "national,lm96163";
	reg = <0x4c>;
	#pwm-cells = <2>;
	/* 0 - one sensor, 1 - many sensors */
	#thermal-sensor-cells = <1>;
};
fan: pwm_fan {
	compatible = "pwm-fan";
	/* arg 0 - pwm chip; arg 1 - pwm_id; arg 2 - period*/
	pwms = <&pwm 0 100>;
	#cooling-cells = <2>;
	cooling-levels = <0 25 128 255>;
};

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Azat Gismatov <gismatov.az@mail.ru>
---
 drivers/hwmon/lm63.c | 121 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 50f67265c71d..7d685cc4c0bc 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -34,7 +34,9 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/pwm.h>
 #include <linux/sysfs.h>
+#include <linux/thermal.h>
 #include <linux/types.h>
 
 /*
@@ -97,6 +99,8 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 #define LM63_MAX_CONVRATE_HZ		32
 #define LM96163_MAX_CONVRATE_HZ		26
 
+#define MAX_SENSORS	2
+
 /*
  * Conversions and various macros
  * For tachometer counts, the LM63 uses 16-bit values.
@@ -131,6 +135,12 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 
 enum chips { lm63, lm64, lm96163 };
 
+struct lm63_thermal_sensor {
+	struct lm63_data *data;
+	struct thermal_zone_device *tz;
+	unsigned int sensor_id;
+};
+
 /*
  * Client data (each client gets its own)
  */
@@ -173,8 +183,15 @@ struct lm63_data {
 	bool lut_temp_highres;
 	bool remote_unsigned; /* true if unsigned remote upper limits */
 	bool trutherm;
+	struct pwm_chip chip;
+	struct lm63_thermal_sensor thermal_sensor[MAX_SENSORS];
 };
 
+static inline struct lm63_data *to_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct lm63_data, chip);
+}
+
 static inline int temp8_from_reg(struct lm63_data *data, int nr)
 {
 	if (data->remote_unsigned)
@@ -421,6 +438,9 @@ static ssize_t pwm1_enable_store(struct device *dev,
 	unsigned long val;
 	int err;
 
+	if (of_get_property(dev->of_node, "#pwm-cells", NULL))
+		return -EPERM;
+
 	err = kstrtoul(buf, 10, &val);
 	if (err)
 		return err;
@@ -1018,6 +1038,8 @@ static void lm63_init_client(struct lm63_data *data)
 	u8 convrate;
 
 	data->config = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG1);
+	if (of_get_property(dev->of_node, "#pwm-cells", NULL))
+		i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, 0x20);
 	data->config_fan = i2c_smbus_read_byte_data(client,
 						    LM63_REG_CONFIG_FAN);
 
@@ -1087,6 +1109,96 @@ static void lm63_init_client(struct lm63_data *data)
 		(data->config_fan & 0x20) ? "manual" : "auto");
 }
 
+static int lm63_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
+	struct lm63_data *data = to_pwm(chip);
+	struct i2c_client *client = data->client;
+	int ret = -EINVAL;
+	u8 pwm_mode;
+	u8 val;
+
+	mutex_lock(&data->update_lock);
+	pwm_mode = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
+	if (!(pwm_mode >> 5)) {
+		mutex_unlock(&data->update_lock);
+		return -EPERM;
+	}
+
+	if (state->period > 1) {
+		val = state->duty_cycle * 255 / (state->period - 1);
+		val = clamp_val(val, 0, 255);
+		val = data->pwm_highres ? val :
+				(val * data->pwm1_freq * 2 + 127) / 255;
+		ret = i2c_smbus_write_byte_data(client, LM63_REG_PWM_VALUE, val);
+	}
+
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static const struct pwm_ops lm63_pwm_ops = {
+	.apply = lm63_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static void lm63_pwm_remove(void *arg)
+{
+	struct lm63_data *data = arg;
+
+	pwmchip_remove(&data->chip);
+}
+
+static void lm63_init_pwm(struct lm63_data *data)
+{
+	struct i2c_client *client = data->client;
+	int ret;
+
+	/* Initialize chip */
+
+	data->chip.dev = &client->dev;
+	data->chip.ops = &lm63_pwm_ops;
+	data->chip.base = -1;
+	data->chip.npwm = 1;
+
+	ret = pwmchip_add(&data->chip);
+	if (ret < 0) {
+		dev_warn(&client->dev, "pwmchip_add() failed: %d\n", ret);
+		return;
+	}
+
+	devm_add_action_or_reset(&client->dev, lm63_pwm_remove, data);
+}
+
+static int lm63_get_temp(void *data, int *temp)
+{
+	struct lm63_thermal_sensor *thermal_sensor = data;
+
+	*temp = i2c_smbus_read_byte_data(thermal_sensor->data->client,
+			LM63_REG_LOCAL_TEMP + thermal_sensor->sensor_id) * 1000;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops lm63_tz_ops = {
+	.get_temp = lm63_get_temp,
+};
+
+static void lm63_init_thermal(struct lm63_data *data)
+{
+	struct i2c_client *client = data->client;
+	struct lm63_thermal_sensor *thermal_sensor;
+	unsigned int i;
+
+	thermal_sensor = data->thermal_sensor;
+	for (i = 0; i < MAX_SENSORS; i++, thermal_sensor++) {
+		thermal_sensor->data = data;
+		thermal_sensor->sensor_id = i;
+		thermal_sensor->tz = devm_thermal_zone_of_sensor_register(&client->dev,
+						 i, thermal_sensor, &lm63_tz_ops);
+	}
+}
+
 static const struct i2c_device_id lm63_id[];
 
 static int lm63_probe(struct i2c_client *client)
@@ -1126,7 +1238,14 @@ static int lm63_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);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	lm63_init_thermal(data);
+	if (IS_ENABLED(CONFIG_PWM))
+		lm63_init_pwm(data);
+
+	return 0;
 }
 
 /*
-- 
2.20.1

Re: [PATCH] lm63: Added thermal zone support

[Guenter Roeck]

On 4/5/21 5:55 AM, Azat Gismatov wrote:
> There are target systems where temperature sensor in the lm63
> is not the only temperature sensor in the system used to make
> thermal decisions. For example temperature sensors aren't
> connected to the lm63 (sensors are part of CPU and supported
> by own linux driver). 

The last sentence above can be removed. Or replace it all with
something like

"On some systems, temperature sensors not connected to the LM63
are used to make thermal decisions."

  So automatic fan control by the lm63 chip
> doesn't work for that systems.
> Using of thermal zone is much more convenient for embedded
> systems, than fan-control, or any other userspace software,
> because It allow to get smaller software footprint. Also with
> thermal zone we can store all cooling settings in devicetree,
> which is easier to maintain for embedded systems, than some
> userspace software.
> Added support to use manual pwm mode by  property  in device
> tree "#pwm-cells" (#pwm-cells = <2>). This property checkes

checks

> the presence of a pwm chip. If the property is set in the
> device tree, we will use the manual pwm mode by default. From
> the user mode it will not be possible to change automatical

change to automatic mode

> mode in the sysfs attributes. So pwm_chip_add () is used. In
> the device tree lable pwm_fan is used as a cooling device,

label

> it has a "pwms" property in description which is a link to
> the pwm device label. Driver pwm-fan registers thermal_zone
> cooling device.
> Example lm96163, pwm-fan device tree:
> pwm: lm96163@4c {
> 	compatible = "national,lm96163";
> 	reg = <0x4c>;
> 	#pwm-cells = <2>;
> 	/* 0 - one sensor, 1 - many sensors */
> 	#thermal-sensor-cells = <1>;
> };

This would need to be documented.

> fan: pwm_fan {
> 	compatible = "pwm-fan";
> 	/* arg 0 - pwm chip; arg 1 - pwm_id; arg 2 - period*/
> 	pwms = <&pwm 0 100>;
> 	#cooling-cells = <2>;
> 	cooling-levels = <0 25 128 255>;
> };
> 

This seems like an odd and unusual way to register a cooling device,
and it doesn't really explain why the lm63 driver doesn't register
itself as cooling device directly. This is true even more since it
requires that both CONFIG_PWM _and_ SENSORS_PWM_FAN have to be enabled,
which directly conflicts with the notion of limiting code size.
I don't see a technical reason for the current implementation.

Either case, new properties and their use needs to be documented in
Documentation/devicetree/bindings/hwmon/lm63.yaml.

> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Azat Gismatov <gismatov.az@mail.ru>

Is that a new submission or a resubmit or a new version of a previous
patch ? I can't tell. Please explain. If it is a new version of a
previous patch, please version your patches and provide change logs.

> ---
>  drivers/hwmon/lm63.c | 121 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 120 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 50f67265c71d..7d685cc4c0bc 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -34,7 +34,9 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> +#include <linux/pwm.h>
>  #include <linux/sysfs.h>
> +#include <linux/thermal.h>
>  #include <linux/types.h>
>  
>  /*
> @@ -97,6 +99,8 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>  #define LM63_MAX_CONVRATE_HZ		32
>  #define LM96163_MAX_CONVRATE_HZ		26
>  
> +#define MAX_SENSORS	2
> +
>  /*
>   * Conversions and various macros
>   * For tachometer counts, the LM63 uses 16-bit values.
> @@ -131,6 +135,12 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>  
>  enum chips { lm63, lm64, lm96163 };
>  
> +struct lm63_thermal_sensor {
> +	struct lm63_data *data;

This should not be needed. Something like

#define to_lm63_data(s, i)     container_of((s), struct lm63_data, thermal_sensor[i])

and

struct lm63_data *lm63_data = to_lm63_data(thermal_sensor,
                                           thermal_sensor->sensor_id);

should do.

> +	struct thermal_zone_device *tz;
> +	unsigned int sensor_id;
> +};
> +
>  /*
>   * Client data (each client gets its own)
>   */
> @@ -173,8 +183,15 @@ struct lm63_data {
>  	bool lut_temp_highres;
>  	bool remote_unsigned; /* true if unsigned remote upper limits */
>  	bool trutherm;
> +	struct pwm_chip chip;
> +	struct lm63_thermal_sensor thermal_sensor[MAX_SENSORS];
>  };
>  
> +static inline struct lm63_data *to_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct lm63_data, chip);
> +}
> +
>  static inline int temp8_from_reg(struct lm63_data *data, int nr)
>  {
>  	if (data->remote_unsigned)
> @@ -421,6 +438,9 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	unsigned long val;
>  	int err;
>  
> +	if (of_get_property(dev->of_node, "#pwm-cells", NULL))
> +		return -EPERM;
> +
>  	err = kstrtoul(buf, 10, &val);
>  	if (err)
>  		return err;
> @@ -1018,6 +1038,8 @@ static void lm63_init_client(struct lm63_data *data)
>  	u8 convrate;
>  
>  	data->config = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG1);
> +	if (of_get_property(dev->of_node, "#pwm-cells", NULL))
> +		i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, 0x20);
>  	data->config_fan = i2c_smbus_read_byte_data(client,
>  						    LM63_REG_CONFIG_FAN);
>  
> @@ -1087,6 +1109,96 @@ static void lm63_init_client(struct lm63_data *data)
>  		(data->config_fan & 0x20) ? "manual" : "auto");
>  }
>  
> +static int lm63_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)
> +{
> +	struct lm63_data *data = to_pwm(chip);
> +	struct i2c_client *client = data->client;
> +	int ret = -EINVAL;
> +	u8 pwm_mode;
> +	u8 val;
> +
> +	mutex_lock(&data->update_lock);
> +	pwm_mode = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
> +	if (!(pwm_mode >> 5)) {
> +		mutex_unlock(&data->update_lock);
> +		return -EPERM;
> +	}
> +
> +	if (state->period > 1) {
> +		val = state->duty_cycle * 255 / (state->period - 1);
> +		val = clamp_val(val, 0, 255);
> +		val = data->pwm_highres ? val :
> +				(val * data->pwm1_freq * 2 + 127) / 255;
> +		ret = i2c_smbus_write_byte_data(client, LM63_REG_PWM_VALUE, val);
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static const struct pwm_ops lm63_pwm_ops = {
> +	.apply = lm63_pwm_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static void lm63_pwm_remove(void *arg)
> +{
> +	struct lm63_data *data = arg;
> +
> +	pwmchip_remove(&data->chip);
> +}
> +
> +static void lm63_init_pwm(struct lm63_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	/* Initialize chip */
> +
> +	data->chip.dev = &client->dev;
> +	data->chip.ops = &lm63_pwm_ops;
> +	data->chip.base = -1;
> +	data->chip.npwm = 1;
> +
> +	ret = pwmchip_add(&data->chip);
> +	if (ret < 0) {
> +		dev_warn(&client->dev, "pwmchip_add() failed: %d\n", ret);
> +		return;
> +	}
> +
> +	devm_add_action_or_reset(&client->dev, lm63_pwm_remove, data);
> +}
> +
> +static int lm63_get_temp(void *data, int *temp)
> +{
> +	struct lm63_thermal_sensor *thermal_sensor = data;
> +
> +	*temp = i2c_smbus_read_byte_data(thermal_sensor->data->client,
> +			LM63_REG_LOCAL_TEMP + thermal_sensor->sensor_id) * 1000;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops lm63_tz_ops = {
> +	.get_temp = lm63_get_temp,
> +};
> +
> +static void lm63_init_thermal(struct lm63_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	struct lm63_thermal_sensor *thermal_sensor;
> +	unsigned int i;
> +
> +	thermal_sensor = data->thermal_sensor;
> +	for (i = 0; i < MAX_SENSORS; i++, thermal_sensor++) {
> +		thermal_sensor->data = data;
> +		thermal_sensor->sensor_id = i;
> +		thermal_sensor->tz = devm_thermal_zone_of_sensor_register(&client->dev,
> +						 i, thermal_sensor, &lm63_tz_ops);
> +	}
> +}
> +
>  static const struct i2c_device_id lm63_id[];
>  
>  static int lm63_probe(struct i2c_client *client)
> @@ -1126,7 +1238,14 @@ static int lm63_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);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	lm63_init_thermal(data);
> +	if (IS_ENABLED(CONFIG_PWM))
> +		lm63_init_pwm(data);
> +
> +	return 0;
>  }
>  
>  /*
> 

Re: [PATCH] lm63: Added thermal zone support

[Guenter Roeck]

On 3/22/21 2:39 AM, Azat Gismatov wrote:
> There are target systems where pwm controller is the lm63, but 
> temperature sensors aren't connected to the lm63 (sensors are
> part of CPU and supported by own linux driver). Automatic fan
> control by the lm63 chip doesn't work for that systems.
> Thermal zone is much more convenient for embedded systems, than
> fan-control, or any other userspace software, because It allow
> to get smaller software footprint. Also with thermal zone we
> can store all cooling settings in devicetree, which is easier
> to maintain for embedded systems, than some userspace software.
> Added support property "pwm-mode" (pwm-mode = "manual") for
> devicetree to use a manual pwm mode in the lm63 driver.
> 
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Azat Gismatov <gismatov.az@mail.ru>
> ---
>  drivers/hwmon/lm63.c | 133 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 132 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 50f67265c71d..764bc528294f 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -34,7 +34,9 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> +#include <linux/pwm.h>
>  #include <linux/sysfs.h>
> +#include <linux/thermal.h>
>  #include <linux/types.h>
>  
>  /*
> @@ -97,6 +99,8 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>  #define LM63_MAX_CONVRATE_HZ		32
>  #define LM96163_MAX_CONVRATE_HZ		26
>  
> +#define MAX_SENSORS	2
> +
>  /*
>   * Conversions and various macros
>   * For tachometer counts, the LM63 uses 16-bit values.
> @@ -173,8 +177,15 @@ struct lm63_data {
>  	bool lut_temp_highres;
>  	bool remote_unsigned; /* true if unsigned remote upper limits */
>  	bool trutherm;
> +	struct pwm_chip chip;
> +	struct lm63_thermal_sensor thermal_sensor[MAX_SENSORS];
>  };
>  
> +static inline struct lm63_data *to_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct lm63_data, chip);
> +}
> +
>  static inline int temp8_from_reg(struct lm63_data *data, int nr)
>  {
>  	if (data->remote_unsigned)
> @@ -1015,9 +1026,15 @@ static void lm63_init_client(struct lm63_data *data)
>  {
>  	struct i2c_client *client = data->client;
>  	struct device *dev = &client->dev;
> +	const char *pwm_mode;
>  	u8 convrate;
>  
>  	data->config = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG1);
> +	pwm_mode = of_get_property(dev->of_node, "pwm-mode", NULL);
> +	if (pwm_mode) {
> +		if (!strcmp(pwm_mode, "manual"))
> +			i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, 0x20);
> +	}

This would require dt maintainer approval.

I would, however argue that it is not needed: If the chip registers as thermal
cooling device and has associated devicetree nodes, it should be locked to
manual mode, and manipulating chip configuration using sysfs attributes
should be disabled.

>  	data->config_fan = i2c_smbus_read_byte_data(client,
>  						    LM63_REG_CONFIG_FAN);
>  
> @@ -1087,6 +1104,108 @@ static void lm63_init_client(struct lm63_data *data)
>  		(data->config_fan & 0x20) ? "manual" : "auto");
>  }
>  
> +static int lm63_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)
> +{
> +	struct lm63_data *data = to_pwm(chip);
> +	struct i2c_client *client = data->client;
> +	int ret = -EINVAL;
> +	u8 pwm_mode;
> +	u8 val;
> +
> +	mutex_lock(&data->update_lock);
> +	pwm_mode = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
> +	if (!(pwm_mode >> 5)) {
> +		mutex_unlock(&data->update_lock);
> +		return -EPERM;
> +	}
> +
> +	if (state->period > 1) {
> +		val = state->duty_cycle * 255 / (state->period - 1);
> +		val = clamp_val(val, 0, 255);
> +		val = data->pwm_highres ? val :
> +				(val * data->pwm1_freq * 2 + 127) / 255;
> +		ret = i2c_smbus_write_byte_data(client, LM63_REG_PWM_VALUE, val);
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static const struct pwm_ops lm63_pwm_ops = {
> +	.apply = lm63_pwm_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static void lm63_pwm_remove(void *arg)
> +{
> +	struct i2c_client *client = arg;
> +	struct lm63_data *data = i2c_get_clientdata(client);
> +
> +	pwmchip_remove(&data->chip);
> +}
> +
> +static int lm63_init_pwm(struct lm63_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	ret = devm_add_action(&client->dev, lm63_pwm_remove, client);
> +	if (ret)
> +		return ret;
> +

This must be done after calling pwmchip_add().

> +	/* Initialize chip */
> +
> +	data->chip.dev = &client->dev;
> +	data->chip.ops = &lm63_pwm_ops;
> +	data->chip.base = -1;
> +	data->chip.npwm = 1;
> +
> +	ret = pwmchip_add(&data->chip);
> +	if (ret < 0) {
> +		dev_warn(&client->dev, "pwmchip_add() failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lm63_get_temp(void *data, int *temp)
> +{
> +	struct lm63_thermal_sensor *thermal_sensor = data;
> +
> +	mutex_lock(&thermal_sensor->data->update_lock);
> +	*temp = i2c_smbus_read_byte_data(thermal_sensor->data->client,
> +			LM63_REG_LOCAL_TEMP + thermal_sensor->sensor_id) * 1000;
> +	mutex_unlock(&thermal_sensor->data->update_lock);

I don't immediately see the need for this lock.

> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops lm63_tz_ops = {
> +	.get_temp = lm63_get_temp,
> +};
> +
> +static void lm63_init_thermal(struct lm63_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	struct lm63_thermal_sensor *thermal_sensor;
> +	unsigned int i;
> +
> +	thermal_sensor = data->thermal_sensor;
> +	for (i = 0; i < MAX_SENSORS; i++, thermal_sensor++) {
> +		thermal_sensor->data = data;
> +		thermal_sensor->sensor_id = i;
> +		thermal_sensor->tz = devm_thermal_zone_of_sensor_register(&client->dev,
> +						 i, thermal_sensor, &lm63_tz_ops);
> +

As far as I can see this will happen if there is no thermal zone
configuration in the system, meaning almost all the time. The warning
message below is therefore unacceptable. A return value of -ENODEV
must be silently ignored.

Also, this change does not match the commit description. The argument made there
was that the LM63 is the pwm controller, but that the chip temperature sensor
is not used to make thermal decisions. If the chip temperature sensor is not
used to make thermal decisions, it does not have to be registered as thermal
zone.

The argument made in the description should probably be that the temperature sensor
in the lm63 is not the only temperature sensor in the system used to make thermal
decisions. That is, however, not currently the case. Please make sure that the
description matches the code.

> +		if (IS_ERR(thermal_sensor->tz)) {
> +			dev_warn(&client->dev, "unable to register thermal sensor %ld\n",
> +				 PTR_ERR(thermal_sensor->tz));
> +		}
> +	}
> +}
> +
>  static const struct i2c_device_id lm63_id[];
>  
>  static int lm63_probe(struct i2c_client *client)
> @@ -1095,6 +1214,7 @@ static int lm63_probe(struct i2c_client *client)
>  	struct device *hwmon_dev;
>  	struct lm63_data *data;
>  	int groups = 0;
> +	int ret;
>  
>  	data = devm_kzalloc(dev, sizeof(struct lm63_data), GFP_KERNEL);
>  	if (!data)
> @@ -1126,7 +1246,18 @@ static int lm63_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);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	i2c_set_clientdata(client, data);
> +	lm63_init_thermal(data);
> +	if (IS_ENABLED(CONFIG_PWM)) {
> +		ret = lm63_init_pwm(data);
> +		if (ret)
> +			lm63_pwm_remove(data);

This is not needed if devm_add_action_or_reset() is called in lm63_init_pwm()
after successful pwm chip registration.

Also, other hwmon drivers drivers register as thermal cooling device. You'll
have to explain why this is better (and how it ties into the thermal subsystem),
and why the dependency on the pwm subsystem is needed for this cooling device
to work.

Thanks,
Guenter

[PATCH] lm63: Added thermal zone support

[Azat Gismatov]

There are target systems where pwm controller is the lm63, but 
temperature sensors aren't connected to the lm63 (sensors are
part of CPU and supported by own linux driver). Automatic fan
control by the lm63 chip doesn't work for that systems.
Thermal zone is much more convenient for embedded systems, than
fan-control, or any other userspace software, because It allow
to get smaller software footprint. Also with thermal zone we
can store all cooling settings in devicetree, which is easier
to maintain for embedded systems, than some userspace software.
Added support property "pwm-mode" (pwm-mode = "manual") for
devicetree to use a manual pwm mode in the lm63 driver.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Azat Gismatov <gismatov.az@mail.ru>
---
 drivers/hwmon/lm63.c | 133 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 132 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 50f67265c71d..764bc528294f 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -34,7 +34,9 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/pwm.h>
 #include <linux/sysfs.h>
+#include <linux/thermal.h>
 #include <linux/types.h>
 
 /*
@@ -97,6 +99,8 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 #define LM63_MAX_CONVRATE_HZ		32
 #define LM96163_MAX_CONVRATE_HZ		26
 
+#define MAX_SENSORS	2
+
 /*
  * Conversions and various macros
  * For tachometer counts, the LM63 uses 16-bit values.
@@ -173,8 +177,15 @@ struct lm63_data {
 	bool lut_temp_highres;
 	bool remote_unsigned; /* true if unsigned remote upper limits */
 	bool trutherm;
+	struct pwm_chip chip;
+	struct lm63_thermal_sensor thermal_sensor[MAX_SENSORS];
 };
 
+static inline struct lm63_data *to_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct lm63_data, chip);
+}
+
 static inline int temp8_from_reg(struct lm63_data *data, int nr)
 {
 	if (data->remote_unsigned)
@@ -1015,9 +1026,15 @@ static void lm63_init_client(struct lm63_data *data)
 {
 	struct i2c_client *client = data->client;
 	struct device *dev = &client->dev;
+	const char *pwm_mode;
 	u8 convrate;
 
 	data->config = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG1);
+	pwm_mode = of_get_property(dev->of_node, "pwm-mode", NULL);
+	if (pwm_mode) {
+		if (!strcmp(pwm_mode, "manual"))
+			i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, 0x20);
+	}
 	data->config_fan = i2c_smbus_read_byte_data(client,
 						    LM63_REG_CONFIG_FAN);
 
@@ -1087,6 +1104,108 @@ static void lm63_init_client(struct lm63_data *data)
 		(data->config_fan & 0x20) ? "manual" : "auto");
 }
 
+static int lm63_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
+	struct lm63_data *data = to_pwm(chip);
+	struct i2c_client *client = data->client;
+	int ret = -EINVAL;
+	u8 pwm_mode;
+	u8 val;
+
+	mutex_lock(&data->update_lock);
+	pwm_mode = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
+	if (!(pwm_mode >> 5)) {
+		mutex_unlock(&data->update_lock);
+		return -EPERM;
+	}
+
+	if (state->period > 1) {
+		val = state->duty_cycle * 255 / (state->period - 1);
+		val = clamp_val(val, 0, 255);
+		val = data->pwm_highres ? val :
+				(val * data->pwm1_freq * 2 + 127) / 255;
+		ret = i2c_smbus_write_byte_data(client, LM63_REG_PWM_VALUE, val);
+	}
+
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static const struct pwm_ops lm63_pwm_ops = {
+	.apply = lm63_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static void lm63_pwm_remove(void *arg)
+{
+	struct i2c_client *client = arg;
+	struct lm63_data *data = i2c_get_clientdata(client);
+
+	pwmchip_remove(&data->chip);
+}
+
+static int lm63_init_pwm(struct lm63_data *data)
+{
+	struct i2c_client *client = data->client;
+	int ret;
+
+	ret = devm_add_action(&client->dev, lm63_pwm_remove, client);
+	if (ret)
+		return ret;
+
+	/* Initialize chip */
+
+	data->chip.dev = &client->dev;
+	data->chip.ops = &lm63_pwm_ops;
+	data->chip.base = -1;
+	data->chip.npwm = 1;
+
+	ret = pwmchip_add(&data->chip);
+	if (ret < 0) {
+		dev_warn(&client->dev, "pwmchip_add() failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int lm63_get_temp(void *data, int *temp)
+{
+	struct lm63_thermal_sensor *thermal_sensor = data;
+
+	mutex_lock(&thermal_sensor->data->update_lock);
+	*temp = i2c_smbus_read_byte_data(thermal_sensor->data->client,
+			LM63_REG_LOCAL_TEMP + thermal_sensor->sensor_id) * 1000;
+	mutex_unlock(&thermal_sensor->data->update_lock);
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops lm63_tz_ops = {
+	.get_temp = lm63_get_temp,
+};
+
+static void lm63_init_thermal(struct lm63_data *data)
+{
+	struct i2c_client *client = data->client;
+	struct lm63_thermal_sensor *thermal_sensor;
+	unsigned int i;
+
+	thermal_sensor = data->thermal_sensor;
+	for (i = 0; i < MAX_SENSORS; i++, thermal_sensor++) {
+		thermal_sensor->data = data;
+		thermal_sensor->sensor_id = i;
+		thermal_sensor->tz = devm_thermal_zone_of_sensor_register(&client->dev,
+						 i, thermal_sensor, &lm63_tz_ops);
+
+		if (IS_ERR(thermal_sensor->tz)) {
+			dev_warn(&client->dev, "unable to register thermal sensor %ld\n",
+				 PTR_ERR(thermal_sensor->tz));
+		}
+	}
+}
+
 static const struct i2c_device_id lm63_id[];
 
 static int lm63_probe(struct i2c_client *client)
@@ -1095,6 +1214,7 @@ static int lm63_probe(struct i2c_client *client)
 	struct device *hwmon_dev;
 	struct lm63_data *data;
 	int groups = 0;
+	int ret;
 
 	data = devm_kzalloc(dev, sizeof(struct lm63_data), GFP_KERNEL);
 	if (!data)
@@ -1126,7 +1246,18 @@ static int lm63_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);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	i2c_set_clientdata(client, data);
+	lm63_init_thermal(data);
+	if (IS_ENABLED(CONFIG_PWM)) {
+		ret = lm63_init_pwm(data);
+		if (ret)
+			lm63_pwm_remove(data);
+	}
+
+	return 0;
 }
 
 /*
-- 
2.20.1