[PATCH v3 0/4] expose lm90 to thermal fw

[PATCH v3 0/4] expose lm90 to thermal fw

[Wei Ni]

Expose lm90 to thermal framework via DT nodes.

This series is v3, previous version patches are:
[v2]: https://lkml.org/lkml/2014/3/4/194

Changes from v2:
add more description in documentation, per Stephen's comment.

Changes from v1:
1. remove the unnecessary log messages, per Guenter's request.
2. add thermal zones node for nct1008 on dalmore.

Wei Ni (1):
  thermal: add more description for thermal-zones

lightning314 (3):
  hwmon: (lm90) split set&show temp as common codes
  hwmon: lm90: expose to thermal fw via DT nodes
  ARM: tegra: dalmore: add thermal zones for nct1008

 .../devicetree/bindings/thermal/thermal.txt        |  10 +-
 arch/arm/boot/dts/tegra114-dalmore.dts             |  20 +-
 drivers/hwmon/lm90.c                               | 222 ++++++++++++++++-----
 3 files changed, 192 insertions(+), 60 deletions(-)

-- 
1.8.1.5

[PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

From: lightning314 <wni@nvidia.com>

Split set&show temp codes as common functions, so we can use it
directly when implement linux thermal framework.
And handle error return value for the lm90_select_remote_channel
and write_tempx, then set_temp8 and set_temp11 could return it
to user-space.

Signed-off-by: Wei Ni <wni@nvidia.com>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/hwmon/lm90.c | 164 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 109 insertions(+), 55 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index c9ff08d..cb33dcf 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
  * various registers have different meanings as a result of selecting a
  * non-default remote channel.
  */
-static inline void lm90_select_remote_channel(struct i2c_client *client,
-					      struct lm90_data *data,
-					      int channel)
+static inline int lm90_select_remote_channel(struct i2c_client *client,
+					     struct lm90_data *data,
+					     int channel)
 {
 	u8 config;
+	int err = 0;
 
 	if (data->kind == max6696) {
 		lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
 		config &= ~0x08;
 		if (channel)
 			config |= 0x08;
-		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
-					  config);
+		err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+						config);
 	}
+
+	return err;
 }
 
 /*
@@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
  * Sysfs stuff
  */
 
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
-			  char *buf)
+static int read_temp8(struct device *dev, int index)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
 	if (data->kind == adt7461 || data->kind == tmp451)
-		temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
+		temp = temp_from_u8_adt7461(data, data->temp8[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[attr->index]);
+		temp = temp_from_u8(data->temp8[index]);
 	else
-		temp = temp_from_s8(data->temp8[attr->index]);
+		temp = temp_from_s8(data->temp8[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		temp += 16000;
 
-	return sprintf(buf, "%d\n", temp);
+	return temp;
 }
 
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
+			  char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	return sprintf(buf, "%d\n", read_temp8(dev, attr->index));
+}
+
+static int write_temp8(struct device *dev, int index, long val)
 {
 	static const u8 reg[TEMP8_REG_NUM] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 		MAX6659_REG_W_REMOTE_EMERG,
 	};
 
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct lm90_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int nr = attr->index;
-	long val;
 	int err;
 
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
-
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		val -= 16000;
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461 || data->kind == tmp451)
-		data->temp8[nr] = temp_to_u8_adt7461(data, val);
+		data->temp8[index] = temp_to_u8_adt7461(data, val);
 	else if (data->kind == max6646)
-		data->temp8[nr] = temp_to_u8(val);
+		data->temp8[index] = temp_to_u8(val);
 	else
-		data->temp8[nr] = temp_to_s8(val);
-
-	lm90_select_remote_channel(client, data, nr >= 6);
-	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
-	lm90_select_remote_channel(client, data, 0);
+		data->temp8[index] = temp_to_s8(val);
 
+	if ((err = lm90_select_remote_channel(client, data, index >= 6)) ||
+	    (err = i2c_smbus_write_byte_data(client, reg[index],
+					     data->temp8[index])) ||
+	    (err = lm90_select_remote_channel(client, data, 0)))
+		dev_err(dev, "write_temp8 failed, err %d\n", err);
 	mutex_unlock(&data->update_lock);
+
+	return err;
+}
+
+static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	err = write_temp8(dev, index, val);
+	if (err < 0)
+		return err;
+
 	return count;
 }
 
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
-			   char *buf)
+static int read_temp11(struct device *dev, int index)
 {
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
 	if (data->kind == adt7461 || data->kind == tmp451)
-		temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
+		temp = temp_from_u16_adt7461(data, data->temp11[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u16(data->temp11[attr->index]);
+		temp = temp_from_u16(data->temp11[index]);
 	else
-		temp = temp_from_s16(data->temp11[attr->index]);
+		temp = temp_from_s16(data->temp11[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 &&  attr->index <= 2)
+	if (data->kind == lm99 && index <= 2)
 		temp += 16000;
 
-	return sprintf(buf, "%d\n", temp);
+	return temp;
 }
 
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-			  const char *buf, size_t count)
+static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+
+	return sprintf(buf, "%d\n", read_temp11(dev, attr->index));
+}
+
+static int write_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
 		u8 high;
@@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
 	};
 
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct lm90_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int nr = attr->nr;
-	int index = attr->index;
-	long val;
 	int err;
 
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
-
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind == lm99 && index <= 2)
 		val -= 16000;
@@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	else
 		data->temp11[index] = temp_to_s8(val) << 8;
 
-	lm90_select_remote_channel(client, data, reg[nr].channel);
-	i2c_smbus_write_byte_data(client, reg[nr].high,
-				  data->temp11[index] >> 8);
-	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
-		i2c_smbus_write_byte_data(client, reg[nr].low,
-					  data->temp11[index] & 0xff);
-	lm90_select_remote_channel(client, data, 0);
+	err = lm90_select_remote_channel(client, data, reg[nr].channel);
+	if (err)
+		goto error;
+
+	err = i2c_smbus_write_byte_data(client, reg[nr].high,
+					data->temp11[index] >> 8);
+	if (err)
+		goto error;
+
+	if (data->flags & LM90_HAVE_REM_LIMIT_EXT) {
+		err = i2c_smbus_write_byte_data(client, reg[nr].low,
+						data->temp11[index] & 0xff);
+		if (err)
+			goto error;
+	}
+
+	err = lm90_select_remote_channel(client, data, 0);
+
+error:
+	if (err)
+		dev_err(dev, "write_temp11 failed, err %d\n", err);
 
 	mutex_unlock(&data->update_lock);
+
+	return err;
+}
+
+static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int nr = attr->nr;
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	err = write_temp11(dev, nr, index, val);
+	if (err < 0)
+		return err;
+
 	return count;
 }
 
-- 
1.8.1.5

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Mikko Perttunen]

FWIW, please fix the authorship information for next version.

Cheers,
Mikko

On 25/08/14 09:29, Wei Ni wrote:
> From: lightning314 <wni@nvidia.com>
>
> Split set&show temp codes as common functions, so we can use it
> directly when implement linux thermal framework.
> And handle error return value for the lm90_select_remote_channel
> and write_tempx, then set_temp8 and set_temp11 could return it
> to user-space.
>
> Signed-off-by: Wei Ni <wni@nvidia.com>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>   drivers/hwmon/lm90.c | 164 ++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 109 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index c9ff08d..cb33dcf 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
>    * various registers have different meanings as a result of selecting a
>    * non-default remote channel.
>    */
> -static inline void lm90_select_remote_channel(struct i2c_client *client,
> -					      struct lm90_data *data,
> -					      int channel)
> +static inline int lm90_select_remote_channel(struct i2c_client *client,
> +					     struct lm90_data *data,
> +					     int channel)
>   {
>   	u8 config;
> +	int err = 0;
>
>   	if (data->kind == max6696) {
>   		lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
>   		config &= ~0x08;
>   		if (channel)
>   			config |= 0x08;
> -		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> -					  config);
> +		err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> +						config);
>   	}
> +
> +	return err;
>   }
>
>   /*
> @@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
>    * Sysfs stuff
>    */
>
> -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> -			  char *buf)
> +static int read_temp8(struct device *dev, int index)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct lm90_data *data = lm90_update_device(dev);
>   	int temp;
>
>   	if (data->kind == adt7461 || data->kind == tmp451)
> -		temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
> +		temp = temp_from_u8_adt7461(data, data->temp8[index]);
>   	else if (data->kind == max6646)
> -		temp = temp_from_u8(data->temp8[attr->index]);
> +		temp = temp_from_u8(data->temp8[index]);
>   	else
> -		temp = temp_from_s8(data->temp8[attr->index]);
> +		temp = temp_from_s8(data->temp8[index]);
>
>   	/* +16 degrees offset for temp2 for the LM99 */
> -	if (data->kind == lm99 && attr->index == 3)
> +	if (data->kind == lm99 && index == 3)
>   		temp += 16000;
>
> -	return sprintf(buf, "%d\n", temp);
> +	return temp;
>   }
>
> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> +			  char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> +	return sprintf(buf, "%d\n", read_temp8(dev, attr->index));
> +}
> +
> +static int write_temp8(struct device *dev, int index, long val)
>   {
>   	static const u8 reg[TEMP8_REG_NUM] = {
>   		LM90_REG_W_LOCAL_LOW,
> @@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>   		MAX6659_REG_W_REMOTE_EMERG,
>   	};
>
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct lm90_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	int nr = attr->index;
> -	long val;
>   	int err;
>
> -	err = kstrtol(buf, 10, &val);
> -	if (err < 0)
> -		return err;
> -
>   	/* +16 degrees offset for temp2 for the LM99 */
> -	if (data->kind == lm99 && attr->index == 3)
> +	if (data->kind == lm99 && index == 3)
>   		val -= 16000;
>
>   	mutex_lock(&data->update_lock);
>   	if (data->kind == adt7461 || data->kind == tmp451)
> -		data->temp8[nr] = temp_to_u8_adt7461(data, val);
> +		data->temp8[index] = temp_to_u8_adt7461(data, val);
>   	else if (data->kind == max6646)
> -		data->temp8[nr] = temp_to_u8(val);
> +		data->temp8[index] = temp_to_u8(val);
>   	else
> -		data->temp8[nr] = temp_to_s8(val);
> -
> -	lm90_select_remote_channel(client, data, nr >= 6);
> -	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> -	lm90_select_remote_channel(client, data, 0);
> +		data->temp8[index] = temp_to_s8(val);
>
> +	if ((err = lm90_select_remote_channel(client, data, index >= 6)) ||
> +	    (err = i2c_smbus_write_byte_data(client, reg[index],
> +					     data->temp8[index])) ||
> +	    (err = lm90_select_remote_channel(client, data, 0)))
> +		dev_err(dev, "write_temp8 failed, err %d\n", err);
>   	mutex_unlock(&data->update_lock);
> +
> +	return err;
> +}
> +
> +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> +			 const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	int index = attr->index;
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err < 0)
> +		return err;
> +
> +	err = write_temp8(dev, index, val);
> +	if (err < 0)
> +		return err;
> +
>   	return count;
>   }
>
> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> -			   char *buf)
> +static int read_temp11(struct device *dev, int index)
>   {
> -	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>   	struct lm90_data *data = lm90_update_device(dev);
>   	int temp;
>
>   	if (data->kind == adt7461 || data->kind == tmp451)
> -		temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
> +		temp = temp_from_u16_adt7461(data, data->temp11[index]);
>   	else if (data->kind == max6646)
> -		temp = temp_from_u16(data->temp11[attr->index]);
> +		temp = temp_from_u16(data->temp11[index]);
>   	else
> -		temp = temp_from_s16(data->temp11[attr->index]);
> +		temp = temp_from_s16(data->temp11[index]);
>
>   	/* +16 degrees offset for temp2 for the LM99 */
> -	if (data->kind == lm99 &&  attr->index <= 2)
> +	if (data->kind == lm99 && index <= 2)
>   		temp += 16000;
>
> -	return sprintf(buf, "%d\n", temp);
> +	return temp;
>   }
>
> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> -			  const char *buf, size_t count)
> +static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> +			   char *buf)
> +{
> +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> +
> +	return sprintf(buf, "%d\n", read_temp11(dev, attr->index));
> +}
> +
> +static int write_temp11(struct device *dev, int nr, int index, long val)
>   {
>   	struct {
>   		u8 high;
> @@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>   		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
>   	};
>
> -	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>   	struct lm90_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	int nr = attr->nr;
> -	int index = attr->index;
> -	long val;
>   	int err;
>
> -	err = kstrtol(buf, 10, &val);
> -	if (err < 0)
> -		return err;
> -
>   	/* +16 degrees offset for temp2 for the LM99 */
>   	if (data->kind == lm99 && index <= 2)
>   		val -= 16000;
> @@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>   	else
>   		data->temp11[index] = temp_to_s8(val) << 8;
>
> -	lm90_select_remote_channel(client, data, reg[nr].channel);
> -	i2c_smbus_write_byte_data(client, reg[nr].high,
> -				  data->temp11[index] >> 8);
> -	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
> -		i2c_smbus_write_byte_data(client, reg[nr].low,
> -					  data->temp11[index] & 0xff);
> -	lm90_select_remote_channel(client, data, 0);
> +	err = lm90_select_remote_channel(client, data, reg[nr].channel);
> +	if (err)
> +		goto error;
> +
> +	err = i2c_smbus_write_byte_data(client, reg[nr].high,
> +					data->temp11[index] >> 8);
> +	if (err)
> +		goto error;
> +
> +	if (data->flags & LM90_HAVE_REM_LIMIT_EXT) {
> +		err = i2c_smbus_write_byte_data(client, reg[nr].low,
> +						data->temp11[index] & 0xff);
> +		if (err)
> +			goto error;
> +	}
> +
> +	err = lm90_select_remote_channel(client, data, 0);
> +
> +error:
> +	if (err)
> +		dev_err(dev, "write_temp11 failed, err %d\n", err);
>
>   	mutex_unlock(&data->update_lock);
> +
> +	return err;
> +}
> +
> +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> +			  const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> +	int nr = attr->nr;
> +	int index = attr->index;
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err < 0)
> +		return err;
> +
> +	err = write_temp11(dev, nr, index, val);
> +	if (err < 0)
> +		return err;
> +
>   	return count;
>   }
>
>

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 08/25/2014 08:23 PM, Mikko Perttunen wrote:
> FWIW, please fix the authorship information for next version.

Sorry, I didn't get your point, did you mean I should remove the line
"From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I
will remove it in next version.

Thanks.
Wei.

> 
> Cheers,
> Mikko
> 
> On 25/08/14 09:29, Wei Ni wrote:
>> From: lightning314 <wni@nvidia.com>
>>
>> Split set&show temp codes as common functions, so we can use it
>> directly when implement linux thermal framework.
>> And handle error return value for the lm90_select_remote_channel
>> and write_tempx, then set_temp8 and set_temp11 could return it
>> to user-space.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> Signed-off-by: Jean Delvare <khali@linux-fr.org>
>> ---
>>   drivers/hwmon/lm90.c | 164
>> ++++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 109 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index c9ff08d..cb33dcf 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client
>> *client, u8 regh, u8 regl, u16 *value)
>>    * various registers have different meanings as a result of selecting a
>>    * non-default remote channel.
>>    */
>> -static inline void lm90_select_remote_channel(struct i2c_client *client,
>> -                          struct lm90_data *data,
>> -                          int channel)
>> +static inline int lm90_select_remote_channel(struct i2c_client *client,
>> +                         struct lm90_data *data,
>> +                         int channel)
>>   {
>>       u8 config;
>> +    int err = 0;
>>
>>       if (data->kind == max6696) {
>>           lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
>>           config &= ~0x08;
>>           if (channel)
>>               config |= 0x08;
>> -        i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
>> -                      config);
>> +        err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
>> +                        config);
>>       }
>> +
>> +    return err;
>>   }
>>
>>   /*
>> @@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data
>> *data, long val)
>>    * Sysfs stuff
>>    */
>>
>> -static ssize_t show_temp8(struct device *dev, struct device_attribute
>> *devattr,
>> -              char *buf)
>> +static int read_temp8(struct device *dev, int index)
>>   {
>> -    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>       struct lm90_data *data = lm90_update_device(dev);
>>       int temp;
>>
>>       if (data->kind == adt7461 || data->kind == tmp451)
>> -        temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
>> +        temp = temp_from_u8_adt7461(data, data->temp8[index]);
>>       else if (data->kind == max6646)
>> -        temp = temp_from_u8(data->temp8[attr->index]);
>> +        temp = temp_from_u8(data->temp8[index]);
>>       else
>> -        temp = temp_from_s8(data->temp8[attr->index]);
>> +        temp = temp_from_s8(data->temp8[index]);
>>
>>       /* +16 degrees offset for temp2 for the LM99 */
>> -    if (data->kind == lm99 && attr->index == 3)
>> +    if (data->kind == lm99 && index == 3)
>>           temp += 16000;
>>
>> -    return sprintf(buf, "%d\n", temp);
>> +    return temp;
>>   }
>>
>> -static ssize_t set_temp8(struct device *dev, struct device_attribute
>> *devattr,
>> -             const char *buf, size_t count)
>> +static ssize_t show_temp8(struct device *dev, struct device_attribute
>> *devattr,
>> +              char *buf)
>> +{
>> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +
>> +    return sprintf(buf, "%d\n", read_temp8(dev, attr->index));
>> +}
>> +
>> +static int write_temp8(struct device *dev, int index, long val)
>>   {
>>       static const u8 reg[TEMP8_REG_NUM] = {
>>           LM90_REG_W_LOCAL_LOW,
>> @@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev,
>> struct device_attribute *devattr,
>>           MAX6659_REG_W_REMOTE_EMERG,
>>       };
>>
>> -    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>       struct lm90_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>> -    int nr = attr->index;
>> -    long val;
>>       int err;
>>
>> -    err = kstrtol(buf, 10, &val);
>> -    if (err < 0)
>> -        return err;
>> -
>>       /* +16 degrees offset for temp2 for the LM99 */
>> -    if (data->kind == lm99 && attr->index == 3)
>> +    if (data->kind == lm99 && index == 3)
>>           val -= 16000;
>>
>>       mutex_lock(&data->update_lock);
>>       if (data->kind == adt7461 || data->kind == tmp451)
>> -        data->temp8[nr] = temp_to_u8_adt7461(data, val);
>> +        data->temp8[index] = temp_to_u8_adt7461(data, val);
>>       else if (data->kind == max6646)
>> -        data->temp8[nr] = temp_to_u8(val);
>> +        data->temp8[index] = temp_to_u8(val);
>>       else
>> -        data->temp8[nr] = temp_to_s8(val);
>> -
>> -    lm90_select_remote_channel(client, data, nr >= 6);
>> -    i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
>> -    lm90_select_remote_channel(client, data, 0);
>> +        data->temp8[index] = temp_to_s8(val);
>>
>> +    if ((err = lm90_select_remote_channel(client, data, index >= 6)) ||
>> +        (err = i2c_smbus_write_byte_data(client, reg[index],
>> +                         data->temp8[index])) ||
>> +        (err = lm90_select_remote_channel(client, data, 0)))
>> +        dev_err(dev, "write_temp8 failed, err %d\n", err);
>>       mutex_unlock(&data->update_lock);
>> +
>> +    return err;
>> +}
>> +
>> +static ssize_t set_temp8(struct device *dev, struct device_attribute
>> *devattr,
>> +             const char *buf, size_t count)
>> +{
>> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +    int index = attr->index;
>> +    long val;
>> +    int err;
>> +
>> +    err = kstrtol(buf, 10, &val);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    err = write_temp8(dev, index, val);
>> +    if (err < 0)
>> +        return err;
>> +
>>       return count;
>>   }
>>
>> -static ssize_t show_temp11(struct device *dev, struct
>> device_attribute *devattr,
>> -               char *buf)
>> +static int read_temp11(struct device *dev, int index)
>>   {
>> -    struct sensor_device_attribute_2 *attr =
>> to_sensor_dev_attr_2(devattr);
>>       struct lm90_data *data = lm90_update_device(dev);
>>       int temp;
>>
>>       if (data->kind == adt7461 || data->kind == tmp451)
>> -        temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
>> +        temp = temp_from_u16_adt7461(data, data->temp11[index]);
>>       else if (data->kind == max6646)
>> -        temp = temp_from_u16(data->temp11[attr->index]);
>> +        temp = temp_from_u16(data->temp11[index]);
>>       else
>> -        temp = temp_from_s16(data->temp11[attr->index]);
>> +        temp = temp_from_s16(data->temp11[index]);
>>
>>       /* +16 degrees offset for temp2 for the LM99 */
>> -    if (data->kind == lm99 &&  attr->index <= 2)
>> +    if (data->kind == lm99 && index <= 2)
>>           temp += 16000;
>>
>> -    return sprintf(buf, "%d\n", temp);
>> +    return temp;
>>   }
>>
>> -static ssize_t set_temp11(struct device *dev, struct device_attribute
>> *devattr,
>> -              const char *buf, size_t count)
>> +static ssize_t show_temp11(struct device *dev, struct
>> device_attribute *devattr,
>> +               char *buf)
>> +{
>> +    struct sensor_device_attribute_2 *attr =
>> to_sensor_dev_attr_2(devattr);
>> +
>> +    return sprintf(buf, "%d\n", read_temp11(dev, attr->index));
>> +}
>> +
>> +static int write_temp11(struct device *dev, int nr, int index, long val)
>>   {
>>       struct {
>>           u8 high;
>> @@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev,
>> struct device_attribute *devattr,
>>           { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
>>       };
>>
>> -    struct sensor_device_attribute_2 *attr =
>> to_sensor_dev_attr_2(devattr);
>>       struct lm90_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>> -    int nr = attr->nr;
>> -    int index = attr->index;
>> -    long val;
>>       int err;
>>
>> -    err = kstrtol(buf, 10, &val);
>> -    if (err < 0)
>> -        return err;
>> -
>>       /* +16 degrees offset for temp2 for the LM99 */
>>       if (data->kind == lm99 && index <= 2)
>>           val -= 16000;
>> @@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev,
>> struct device_attribute *devattr,
>>       else
>>           data->temp11[index] = temp_to_s8(val) << 8;
>>
>> -    lm90_select_remote_channel(client, data, reg[nr].channel);
>> -    i2c_smbus_write_byte_data(client, reg[nr].high,
>> -                  data->temp11[index] >> 8);
>> -    if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
>> -        i2c_smbus_write_byte_data(client, reg[nr].low,
>> -                      data->temp11[index] & 0xff);
>> -    lm90_select_remote_channel(client, data, 0);
>> +    err = lm90_select_remote_channel(client, data, reg[nr].channel);
>> +    if (err)
>> +        goto error;
>> +
>> +    err = i2c_smbus_write_byte_data(client, reg[nr].high,
>> +                    data->temp11[index] >> 8);
>> +    if (err)
>> +        goto error;
>> +
>> +    if (data->flags & LM90_HAVE_REM_LIMIT_EXT) {
>> +        err = i2c_smbus_write_byte_data(client, reg[nr].low,
>> +                        data->temp11[index] & 0xff);
>> +        if (err)
>> +            goto error;
>> +    }
>> +
>> +    err = lm90_select_remote_channel(client, data, 0);
>> +
>> +error:
>> +    if (err)
>> +        dev_err(dev, "write_temp11 failed, err %d\n", err);
>>
>>       mutex_unlock(&data->update_lock);
>> +
>> +    return err;
>> +}
>> +
>> +static ssize_t set_temp11(struct device *dev, struct device_attribute
>> *devattr,
>> +              const char *buf, size_t count)
>> +{
>> +    struct sensor_device_attribute_2 *attr =
>> to_sensor_dev_attr_2(devattr);
>> +    int nr = attr->nr;
>> +    int index = attr->index;
>> +    long val;
>> +    int err;
>> +
>> +    err = kstrtol(buf, 10, &val);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    err = write_temp11(dev, nr, index, val);
>> +    if (err < 0)
>> +        return err;
>> +
>>       return count;
>>   }
>>
>>

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Eduardo Valentin]

On Tue, Aug 26, 2014 at 10:27:43AM +0800, Wei Ni wrote:
> On 08/25/2014 08:23 PM, Mikko Perttunen wrote:
> > FWIW, please fix the authorship information for next version.
> 
> Sorry, I didn't get your point, did you mean I should remove the line
> "From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I
> will remove it in next version.

No Wei, don't remove the author line. Well, based on email, it is your
patch, so, removing may satisfy. 

But the point is you should use meaninful names in tags like From and
Signed off by. "lightning314" sounds quite cryptic or even a nick name.
You must use real names there, such as "Wei Ni".

> 
> Thanks.
> Wei.
> 
> > 
> > Cheers,
> > Mikko
> > 
> > On 25/08/14 09:29, Wei Ni wrote:
> >> From: lightning314 <wni@nvidia.com>
> >>
> >> Split set&show temp codes as common functions, so we can use it
> >> directly when implement linux thermal framework.
> >> And handle error return value for the lm90_select_remote_channel
> >> and write_tempx, then set_temp8 and set_temp11 could return it
> >> to user-space.
> >>
> >> Signed-off-by: Wei Ni <wni@nvidia.com>
> >> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> >> ---
> >>   drivers/hwmon/lm90.c | 164
> >> ++++++++++++++++++++++++++++++++++-----------------
> >>   1 file changed, 109 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> >> index c9ff08d..cb33dcf 100644
> >> --- a/drivers/hwmon/lm90.c
> >> +++ b/drivers/hwmon/lm90.c
> >> @@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client
> >> *client, u8 regh, u8 regl, u16 *value)
> >>    * various registers have different meanings as a result of selecting a
> >>    * non-default remote channel.
> >>    */
> >> -static inline void lm90_select_remote_channel(struct i2c_client *client,
> >> -                          struct lm90_data *data,
> >> -                          int channel)
> >> +static inline int lm90_select_remote_channel(struct i2c_client *client,
> >> +                         struct lm90_data *data,
> >> +                         int channel)
> >>   {
> >>       u8 config;
> >> +    int err = 0;
> >>
> >>       if (data->kind == max6696) {
> >>           lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> >>           config &= ~0x08;
> >>           if (channel)
> >>               config |= 0x08;
> >> -        i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> >> -                      config);
> >> +        err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> >> +                        config);
> >>       }
> >> +
> >> +    return err;
> >>   }
> >>
> >>   /*
> >> @@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data
> >> *data, long val)
> >>    * Sysfs stuff
> >>    */
> >>
> >> -static ssize_t show_temp8(struct device *dev, struct device_attribute
> >> *devattr,
> >> -              char *buf)
> >> +static int read_temp8(struct device *dev, int index)
> >>   {
> >> -    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >>       struct lm90_data *data = lm90_update_device(dev);
> >>       int temp;
> >>
> >>       if (data->kind == adt7461 || data->kind == tmp451)
> >> -        temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
> >> +        temp = temp_from_u8_adt7461(data, data->temp8[index]);
> >>       else if (data->kind == max6646)
> >> -        temp = temp_from_u8(data->temp8[attr->index]);
> >> +        temp = temp_from_u8(data->temp8[index]);
> >>       else
> >> -        temp = temp_from_s8(data->temp8[attr->index]);
> >> +        temp = temp_from_s8(data->temp8[index]);
> >>
> >>       /* +16 degrees offset for temp2 for the LM99 */
> >> -    if (data->kind == lm99 && attr->index == 3)
> >> +    if (data->kind == lm99 && index == 3)
> >>           temp += 16000;
> >>
> >> -    return sprintf(buf, "%d\n", temp);
> >> +    return temp;
> >>   }
> >>
> >> -static ssize_t set_temp8(struct device *dev, struct device_attribute
> >> *devattr,
> >> -             const char *buf, size_t count)
> >> +static ssize_t show_temp8(struct device *dev, struct device_attribute
> >> *devattr,
> >> +              char *buf)
> >> +{
> >> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >> +
> >> +    return sprintf(buf, "%d\n", read_temp8(dev, attr->index));
> >> +}
> >> +
> >> +static int write_temp8(struct device *dev, int index, long val)
> >>   {
> >>       static const u8 reg[TEMP8_REG_NUM] = {
> >>           LM90_REG_W_LOCAL_LOW,
> >> @@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev,
> >> struct device_attribute *devattr,
> >>           MAX6659_REG_W_REMOTE_EMERG,
> >>       };
> >>
> >> -    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >>       struct lm90_data *data = dev_get_drvdata(dev);
> >>       struct i2c_client *client = data->client;
> >> -    int nr = attr->index;
> >> -    long val;
> >>       int err;
> >>
> >> -    err = kstrtol(buf, 10, &val);
> >> -    if (err < 0)
> >> -        return err;
> >> -
> >>       /* +16 degrees offset for temp2 for the LM99 */
> >> -    if (data->kind == lm99 && attr->index == 3)
> >> +    if (data->kind == lm99 && index == 3)
> >>           val -= 16000;
> >>
> >>       mutex_lock(&data->update_lock);
> >>       if (data->kind == adt7461 || data->kind == tmp451)
> >> -        data->temp8[nr] = temp_to_u8_adt7461(data, val);
> >> +        data->temp8[index] = temp_to_u8_adt7461(data, val);
> >>       else if (data->kind == max6646)
> >> -        data->temp8[nr] = temp_to_u8(val);
> >> +        data->temp8[index] = temp_to_u8(val);
> >>       else
> >> -        data->temp8[nr] = temp_to_s8(val);
> >> -
> >> -    lm90_select_remote_channel(client, data, nr >= 6);
> >> -    i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> >> -    lm90_select_remote_channel(client, data, 0);
> >> +        data->temp8[index] = temp_to_s8(val);
> >>
> >> +    if ((err = lm90_select_remote_channel(client, data, index >= 6)) ||
> >> +        (err = i2c_smbus_write_byte_data(client, reg[index],
> >> +                         data->temp8[index])) ||
> >> +        (err = lm90_select_remote_channel(client, data, 0)))
> >> +        dev_err(dev, "write_temp8 failed, err %d\n", err);
> >>       mutex_unlock(&data->update_lock);
> >> +
> >> +    return err;
> >> +}
> >> +
> >> +static ssize_t set_temp8(struct device *dev, struct device_attribute
> >> *devattr,
> >> +             const char *buf, size_t count)
> >> +{
> >> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >> +    int index = attr->index;
> >> +    long val;
> >> +    int err;
> >> +
> >> +    err = kstrtol(buf, 10, &val);
> >> +    if (err < 0)
> >> +        return err;
> >> +
> >> +    err = write_temp8(dev, index, val);
> >> +    if (err < 0)
> >> +        return err;
> >> +
> >>       return count;
> >>   }
> >>
> >> -static ssize_t show_temp11(struct device *dev, struct
> >> device_attribute *devattr,
> >> -               char *buf)
> >> +static int read_temp11(struct device *dev, int index)
> >>   {
> >> -    struct sensor_device_attribute_2 *attr =
> >> to_sensor_dev_attr_2(devattr);
> >>       struct lm90_data *data = lm90_update_device(dev);
> >>       int temp;
> >>
> >>       if (data->kind == adt7461 || data->kind == tmp451)
> >> -        temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
> >> +        temp = temp_from_u16_adt7461(data, data->temp11[index]);
> >>       else if (data->kind == max6646)
> >> -        temp = temp_from_u16(data->temp11[attr->index]);
> >> +        temp = temp_from_u16(data->temp11[index]);
> >>       else
> >> -        temp = temp_from_s16(data->temp11[attr->index]);
> >> +        temp = temp_from_s16(data->temp11[index]);
> >>
> >>       /* +16 degrees offset for temp2 for the LM99 */
> >> -    if (data->kind == lm99 &&  attr->index <= 2)
> >> +    if (data->kind == lm99 && index <= 2)
> >>           temp += 16000;
> >>
> >> -    return sprintf(buf, "%d\n", temp);
> >> +    return temp;
> >>   }
> >>
> >> -static ssize_t set_temp11(struct device *dev, struct device_attribute
> >> *devattr,
> >> -              const char *buf, size_t count)
> >> +static ssize_t show_temp11(struct device *dev, struct
> >> device_attribute *devattr,
> >> +               char *buf)
> >> +{
> >> +    struct sensor_device_attribute_2 *attr =
> >> to_sensor_dev_attr_2(devattr);
> >> +
> >> +    return sprintf(buf, "%d\n", read_temp11(dev, attr->index));
> >> +}
> >> +
> >> +static int write_temp11(struct device *dev, int nr, int index, long val)
> >>   {
> >>       struct {
> >>           u8 high;
> >> @@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev,
> >> struct device_attribute *devattr,
> >>           { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
> >>       };
> >>
> >> -    struct sensor_device_attribute_2 *attr =
> >> to_sensor_dev_attr_2(devattr);
> >>       struct lm90_data *data = dev_get_drvdata(dev);
> >>       struct i2c_client *client = data->client;
> >> -    int nr = attr->nr;
> >> -    int index = attr->index;
> >> -    long val;
> >>       int err;
> >>
> >> -    err = kstrtol(buf, 10, &val);
> >> -    if (err < 0)
> >> -        return err;
> >> -
> >>       /* +16 degrees offset for temp2 for the LM99 */
> >>       if (data->kind == lm99 && index <= 2)
> >>           val -= 16000;
> >> @@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev,
> >> struct device_attribute *devattr,
> >>       else
> >>           data->temp11[index] = temp_to_s8(val) << 8;
> >>
> >> -    lm90_select_remote_channel(client, data, reg[nr].channel);
> >> -    i2c_smbus_write_byte_data(client, reg[nr].high,
> >> -                  data->temp11[index] >> 8);
> >> -    if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
> >> -        i2c_smbus_write_byte_data(client, reg[nr].low,
> >> -                      data->temp11[index] & 0xff);
> >> -    lm90_select_remote_channel(client, data, 0);
> >> +    err = lm90_select_remote_channel(client, data, reg[nr].channel);
> >> +    if (err)
> >> +        goto error;
> >> +
> >> +    err = i2c_smbus_write_byte_data(client, reg[nr].high,
> >> +                    data->temp11[index] >> 8);
> >> +    if (err)
> >> +        goto error;
> >> +
> >> +    if (data->flags & LM90_HAVE_REM_LIMIT_EXT) {
> >> +        err = i2c_smbus_write_byte_data(client, reg[nr].low,
> >> +                        data->temp11[index] & 0xff);
> >> +        if (err)
> >> +            goto error;
> >> +    }
> >> +
> >> +    err = lm90_select_remote_channel(client, data, 0);
> >> +
> >> +error:
> >> +    if (err)
> >> +        dev_err(dev, "write_temp11 failed, err %d\n", err);
> >>
> >>       mutex_unlock(&data->update_lock);
> >> +
> >> +    return err;
> >> +}
> >> +
> >> +static ssize_t set_temp11(struct device *dev, struct device_attribute
> >> *devattr,
> >> +              const char *buf, size_t count)
> >> +{
> >> +    struct sensor_device_attribute_2 *attr =
> >> to_sensor_dev_attr_2(devattr);
> >> +    int nr = attr->nr;
> >> +    int index = attr->index;
> >> +    long val;
> >> +    int err;
> >> +
> >> +    err = kstrtol(buf, 10, &val);
> >> +    if (err < 0)
> >> +        return err;
> >> +
> >> +    err = write_temp11(dev, nr, index, val);
> >> +    if (err < 0)
> >> +        return err;
> >> +
> >>       return count;
> >>   }
> >>
> >>
> 

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Stephen Warren]

On 08/26/2014 06:17 AM, Eduardo Valentin wrote:
> On Tue, Aug 26, 2014 at 10:27:43AM +0800, Wei Ni wrote:
>> On 08/25/2014 08:23 PM, Mikko Perttunen wrote:
>>> FWIW, please fix the authorship information for next version.
>>
>> Sorry, I didn't get your point, did you mean I should remove the line
>> "From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I
>> will remove it in next version.
>
> No Wei, don't remove the author line. Well, based on email, it is your
> patch, so, removing may satisfy.

The patch authorship should be fixed to have the correct value; 
"lightning314" is not a valid real name. In turn, depending on your git 
email configuration, this might automatically remove that line from the 
message, since it might then exactly match the email's from address.

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 08/27/2014 12:37 AM, Stephen Warren wrote:
> On 08/26/2014 06:17 AM, Eduardo Valentin wrote:
>> On Tue, Aug 26, 2014 at 10:27:43AM +0800, Wei Ni wrote:
>>> On 08/25/2014 08:23 PM, Mikko Perttunen wrote:
>>>> FWIW, please fix the authorship information for next version.
>>>
>>> Sorry, I didn't get your point, did you mean I should remove the line
>>> "From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I
>>> will remove it in next version.
>>
>> No Wei, don't remove the author line. Well, based on email, it is your
>> patch, so, removing may satisfy.
> 
> The patch authorship should be fixed to have the correct value;
> "lightning314" is not a valid real name. In turn, depending on your git
> email configuration, this might automatically remove that line from the
> message, since it might then exactly match the email's from address.

Got it, thanks for the comments.
I get this patch from the patchwork, so it use my username
"lightning314". Anyway, I will fix it in next version.

Wei.

[PATCH v3 2/4] hwmon: lm90: expose to thermal fw via DT nodes

[Wei Ni]

From: lightning314 <wni@nvidia.com>

This patch adds to lm90 temperature sensor the possibility
to expose itself as thermal zone device, registered on the
thermal framework.

The thermal zone is built only if a device tree node
describing a thermal zone for this sensor is present
inside the lm90 DT node. Otherwise, the driver behavior
will be the same.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index cb33dcf..2a385c5 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -96,6 +96,8 @@
 #include <linux/sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/regulator/consumer.h>
+#include <linux/of.h>
+#include <linux/thermal.h>
 
 /*
  * Addresses to scan
@@ -119,6 +121,13 @@ static const unsigned short normal_i2c[] = {
 enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 	max6646, w83l771, max6696, sa56004, g781, tmp451 };
 
+enum sensor_id {
+	LOCAL = 0,
+	REMOTE,
+	REMOTE2,
+	SENSOR_ID_END,
+};
+
 /*
  * The LM90 registers
  */
@@ -368,6 +377,7 @@ struct lm90_data {
 	struct i2c_client *client;
 	struct device *hwmon_dev;
 	const struct attribute_group *groups[6];
+	struct thermal_zone_device *tz[SENSOR_ID_END];
 	struct mutex update_lock;
 	struct regulator *regulator;
 	char valid; /* zero until following fields are valid */
@@ -874,6 +884,24 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
 	return sprintf(buf, "%d\n", read_temp11(dev, attr->index));
 }
 
+static int lm90_read_local_temp(void *dev, long *temp)
+{
+	*temp = read_temp11(dev, LOCAL_TEMP);
+	return 0;
+}
+
+static int lm90_read_remote_temp(void *dev, long *temp)
+{
+	*temp = read_temp11(dev, REMOTE_TEMP);
+	return 0;
+}
+
+static int lm90_read_remote2_temp(void *dev, long *temp)
+{
+	*temp = read_temp11(dev, REMOTE2_TEMP);
+	return 0;
+}
+
 static int write_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
@@ -1653,6 +1681,33 @@ static int lm90_probe(struct i2c_client *client,
 		}
 	}
 
+	data->tz[LOCAL] = thermal_zone_of_sensor_register(&client->dev,
+							LOCAL,
+							&client->dev,
+							lm90_read_local_temp,
+							NULL);
+	if (IS_ERR(data->tz[LOCAL]))
+		data->tz[LOCAL] = NULL;
+
+	data->tz[REMOTE] = thermal_zone_of_sensor_register(&client->dev,
+							REMOTE,
+							&client->dev,
+							lm90_read_remote_temp,
+							NULL);
+	if (IS_ERR(data->tz[REMOTE]))
+		data->tz[REMOTE] = NULL;
+
+	if (data->flags & LM90_HAVE_TEMP3) {
+		data->tz[REMOTE2] = thermal_zone_of_sensor_register(
+							&client->dev,
+							REMOTE2,
+							&client->dev,
+							lm90_read_remote2_temp,
+							NULL);
+		if (IS_ERR(data->tz[REMOTE2]))
+			data->tz[REMOTE2] = NULL;
+	}
+
 	return 0;
 
 exit_unregister:
@@ -1668,8 +1723,11 @@ exit_restore:
 
 static int lm90_remove(struct i2c_client *client)
 {
+	int i;
 	struct lm90_data *data = i2c_get_clientdata(client);
 
+	for (i = 0; i < SENSOR_ID_END; i++)
+		thermal_zone_of_sensor_unregister(&client->dev, data->tz[i]);
 	hwmon_device_unregister(data->hwmon_dev);
 	device_remove_file(&client->dev, &dev_attr_pec);
 	lm90_restore_conf(client, data);
-- 
1.8.1.5

[PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

Add more description for the "polling-delay" property.
Set "trips" and "cooling maps" as optional property, because
if missing these two sub-nodes, the thermal zone device still
work properly.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
index f5db6b7..e3d3ed9 100644
--- a/Documentation/devicetree/bindings/thermal/thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/thermal.txt
@@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
 
 Required properties:
 - polling-delay:	The maximum number of milliseconds to wait between polls
-  Type: unsigned	when checking this thermal zone.
-  Size: one cell
+  Type: unsigned	when checking this thermal zone. If this value is 0, the
+  Size: one cell	driver will not run polling queue, but just cancel it.
 
 - polling-delay-passive: The maximum number of milliseconds to wait
   Type: unsigned	between polls when performing passive cooling.
@@ -148,14 +148,16 @@ Required properties:
   phandles + sensor
   specifier
 
+Optional property:
 - trips:		A sub-node which is a container of only trip point nodes
   Type: sub-node	required to describe the thermal zone.
 
 - cooling-maps:		A sub-node which is a container of only cooling device
   Type: sub-node	map nodes, used to describe the relation between trips
-			and cooling devices.
+			and cooling devices. If missing the "trips" property,
+			This sub-node will not be parsed, because no trips can
+			be bound to cooling devices.
 
-Optional property:
 - coefficients:		An array of integers (one signed cell) containing
   Type: array		coefficients to compose a linear relation between
   Elem size: one cell	the sensors listed in the thermal-sensors property.
-- 
1.8.1.5

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Eduardo Valentin]

Hello Wei Ni,

On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
> Add more description for the "polling-delay" property.
> Set "trips" and "cooling maps" as optional property, because
> if missing these two sub-nodes, the thermal zone device still
> work properly.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> index f5db6b7..e3d3ed9 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>  
>  Required properties:
>  - polling-delay:	The maximum number of milliseconds to wait between polls
> -  Type: unsigned	when checking this thermal zone.
> -  Size: one cell
> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
> +  Size: one cell	driver will not run polling queue, but just cancel it.
>  

The description above is specific to Linux kernel implementation
nomenclature. DT description needs to be OS agnostic.

>  - polling-delay-passive: The maximum number of milliseconds to wait
>    Type: unsigned	between polls when performing passive cooling.
> @@ -148,14 +148,16 @@ Required properties:
>    phandles + sensor
>    specifier
>  
> +Optional property:
>  - trips:		A sub-node which is a container of only trip point nodes
>    Type: sub-node	required to describe the thermal zone.
>  
>  - cooling-maps:		A sub-node which is a container of only cooling device
>    Type: sub-node	map nodes, used to describe the relation between trips
> -			and cooling devices.
> +			and cooling devices. If missing the "trips" property,
> +			This sub-node will not be parsed, because no trips can
> +			be bound to cooling devices.

Do you mean if the thermal zone misses the "trips" property? Actually,
the binding describes both, cooling-maps and trips, as required
properties. Thus, both needs to be in place to consider the thermal zone
as a proper described zone.

>  
> -Optional property:
>  - coefficients:		An array of integers (one signed cell) containing
>    Type: array		coefficients to compose a linear relation between
>    Elem size: one cell	the sensors listed in the thermal-sensors property.
> -- 
> 1.8.1.5
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> Hello Wei Ni,
> 
> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>> Add more description for the "polling-delay" property.
>> Set "trips" and "cooling maps" as optional property, because
>> if missing these two sub-nodes, the thermal zone device still
>> work properly.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>> index f5db6b7..e3d3ed9 100644
>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>  
>>  Required properties:
>>  - polling-delay:	The maximum number of milliseconds to wait between polls
>> -  Type: unsigned	when checking this thermal zone.
>> -  Size: one cell
>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
>> +  Size: one cell	driver will not run polling queue, but just cancel it.
>>  
> 
> The description above is specific to Linux kernel implementation
> nomenclature. DT description needs to be OS agnostic.
> 
>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>    Type: unsigned	between polls when performing passive cooling.
>> @@ -148,14 +148,16 @@ Required properties:
>>    phandles + sensor
>>    specifier
>>  
>> +Optional property:
>>  - trips:		A sub-node which is a container of only trip point nodes
>>    Type: sub-node	required to describe the thermal zone.
>>  
>>  - cooling-maps:		A sub-node which is a container of only cooling device
>>    Type: sub-node	map nodes, used to describe the relation between trips
>> -			and cooling devices.
>> +			and cooling devices. If missing the "trips" property,
>> +			This sub-node will not be parsed, because no trips can
>> +			be bound to cooling devices.
> 
> Do you mean if the thermal zone misses the "trips" property? Actually,
> the binding describes both, cooling-maps and trips, as required
> properties. Thus, both needs to be in place to consider the thermal zone
> as a proper described zone.

I moved the "trips" and "cooling-maps" to optional property, because if
missing these two properties, the thermal zone devices still can be
registered, and the driver can work properly, it has the basic function,
can read temperature from thermal sysfs, although it doesn't have trips
and bind with cooling devices.

Thanks.
Wei.

> 
>>  
>> -Optional property:
>>  - coefficients:		An array of integers (one signed cell) containing
>>    Type: array		coefficients to compose a linear relation between
>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
>> -- 
>> 1.8.1.5
>>

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Eduardo Valentin]

On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> > Hello Wei Ni,
> > 
> > On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
> >> Add more description for the "polling-delay" property.
> >> Set "trips" and "cooling maps" as optional property, because
> >> if missing these two sub-nodes, the thermal zone device still
> >> work properly.
> >>
> >> Signed-off-by: Wei Ni <wni@nvidia.com>
> >> ---
> >>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> index f5db6b7..e3d3ed9 100644
> >> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
> >>  
> >>  Required properties:
> >>  - polling-delay:	The maximum number of milliseconds to wait between polls
> >> -  Type: unsigned	when checking this thermal zone.
> >> -  Size: one cell
> >> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
> >> +  Size: one cell	driver will not run polling queue, but just cancel it.
> >>  
> > 
> > The description above is specific to Linux kernel implementation
> > nomenclature. DT description needs to be OS agnostic.
> > 
> >>  - polling-delay-passive: The maximum number of milliseconds to wait
> >>    Type: unsigned	between polls when performing passive cooling.
> >> @@ -148,14 +148,16 @@ Required properties:
> >>    phandles + sensor
> >>    specifier
> >>  
> >> +Optional property:
> >>  - trips:		A sub-node which is a container of only trip point nodes
> >>    Type: sub-node	required to describe the thermal zone.
> >>  
> >>  - cooling-maps:		A sub-node which is a container of only cooling device
> >>    Type: sub-node	map nodes, used to describe the relation between trips
> >> -			and cooling devices.
> >> +			and cooling devices. If missing the "trips" property,
> >> +			This sub-node will not be parsed, because no trips can
> >> +			be bound to cooling devices.
> > 
> > Do you mean if the thermal zone misses the "trips" property? Actually,
> > the binding describes both, cooling-maps and trips, as required
> > properties. Thus, both needs to be in place to consider the thermal zone
> > as a proper described zone.
> 
> I moved the "trips" and "cooling-maps" to optional property, because if
> missing these two properties, the thermal zone devices still can be
> registered, and the driver can work properly, it has the basic function,
> can read temperature from thermal sysfs, although it doesn't have trips
> and bind with cooling devices.


If a thermal zone is used only for monitoring, then I believe it lost
its purpose. As  Maybe a different framework shall be used, such as hwmon,
for instance?

The purpose of a thermal zone is to describe thermal behavior of a
hardware. As it is mentioned in the thermal.txt file.


> 
> Thanks.
> Wei.
> 
> > 
> >>  
> >> -Optional property:
> >>  - coefficients:		An array of integers (one signed cell) containing
> >>    Type: array		coefficients to compose a linear relation between
> >>    Elem size: one cell	the sensors listed in the thermal-sensors property.
> >> -- 
> >> 1.8.1.5
> >>
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>> Hello Wei Ni,
>>>
>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>> Add more description for the "polling-delay" property.
>>>> Set "trips" and "cooling maps" as optional property, because
>>>> if missing these two sub-nodes, the thermal zone device still
>>>> work properly.
>>>>
>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>> index f5db6b7..e3d3ed9 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>  
>>>>  Required properties:
>>>>  - polling-delay:	The maximum number of milliseconds to wait between polls
>>>> -  Type: unsigned	when checking this thermal zone.
>>>> -  Size: one cell
>>>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
>>>> +  Size: one cell	driver will not run polling queue, but just cancel it.
>>>>  
>>>
>>> The description above is specific to Linux kernel implementation
>>> nomenclature. DT description needs to be OS agnostic.
>>>
>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>    Type: unsigned	between polls when performing passive cooling.
>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>    phandles + sensor
>>>>    specifier
>>>>  
>>>> +Optional property:
>>>>  - trips:		A sub-node which is a container of only trip point nodes
>>>>    Type: sub-node	required to describe the thermal zone.
>>>>  
>>>>  - cooling-maps:		A sub-node which is a container of only cooling device
>>>>    Type: sub-node	map nodes, used to describe the relation between trips
>>>> -			and cooling devices.
>>>> +			and cooling devices. If missing the "trips" property,
>>>> +			This sub-node will not be parsed, because no trips can
>>>> +			be bound to cooling devices.
>>>
>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>> the binding describes both, cooling-maps and trips, as required
>>> properties. Thus, both needs to be in place to consider the thermal zone
>>> as a proper described zone.
>>
>> I moved the "trips" and "cooling-maps" to optional property, because if
>> missing these two properties, the thermal zone devices still can be
>> registered, and the driver can work properly, it has the basic function,
>> can read temperature from thermal sysfs, although it doesn't have trips
>> and bind with cooling devices.
> 
> 
> If a thermal zone is used only for monitoring, then I believe it lost
> its purpose. As  Maybe a different framework shall be used, such as hwmon,
> for instance?

Yes, if we only use it for monitoring, we can use hwmon. But we have
more functions base on these two thermal zone devices. We have a
skin-temperature driver, which used nct1008's remote and local
temperatures to estimator the skin temperature. As you know the thermal
framework is more powerful, the remote/local sensors can be register as
thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
to read their temperatures and then estimator skin's temp. We also will
set trips and cooling devices for this skin-temp.

Wei.

> 
> The purpose of a thermal zone is to describe thermal behavior of a
> hardware. As it is mentioned in the thermal.txt file.
> 
> 
>>
>> Thanks.
>> Wei.
>>
>>>
>>>>  
>>>> -Optional property:
>>>>  - coefficients:		An array of integers (one signed cell) containing
>>>>    Type: array		coefficients to compose a linear relation between
>>>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
>>>> -- 
>>>> 1.8.1.5
>>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Eduardo Valentin]

Hello Wei,

On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
> > On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
> >> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> >>> Hello Wei Ni,
> >>>
> >>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
> >>>> Add more description for the "polling-delay" property.
> >>>> Set "trips" and "cooling maps" as optional property, because
> >>>> if missing these two sub-nodes, the thermal zone device still
> >>>> work properly.
> >>>>
> >>>> Signed-off-by: Wei Ni <wni@nvidia.com>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
> >>>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>> index f5db6b7..e3d3ed9 100644
> >>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
> >>>>  
> >>>>  Required properties:
> >>>>  - polling-delay:	The maximum number of milliseconds to wait between polls
> >>>> -  Type: unsigned	when checking this thermal zone.
> >>>> -  Size: one cell
> >>>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
> >>>> +  Size: one cell	driver will not run polling queue, but just cancel it.
> >>>>  
> >>>
> >>> The description above is specific to Linux kernel implementation
> >>> nomenclature. DT description needs to be OS agnostic.
> >>>
> >>>>  - polling-delay-passive: The maximum number of milliseconds to wait
> >>>>    Type: unsigned	between polls when performing passive cooling.
> >>>> @@ -148,14 +148,16 @@ Required properties:
> >>>>    phandles + sensor
> >>>>    specifier
> >>>>  
> >>>> +Optional property:
> >>>>  - trips:		A sub-node which is a container of only trip point nodes
> >>>>    Type: sub-node	required to describe the thermal zone.
> >>>>  
> >>>>  - cooling-maps:		A sub-node which is a container of only cooling device
> >>>>    Type: sub-node	map nodes, used to describe the relation between trips
> >>>> -			and cooling devices.
> >>>> +			and cooling devices. If missing the "trips" property,
> >>>> +			This sub-node will not be parsed, because no trips can
> >>>> +			be bound to cooling devices.
> >>>
> >>> Do you mean if the thermal zone misses the "trips" property? Actually,
> >>> the binding describes both, cooling-maps and trips, as required
> >>> properties. Thus, both needs to be in place to consider the thermal zone
> >>> as a proper described zone.
> >>
> >> I moved the "trips" and "cooling-maps" to optional property, because if
> >> missing these two properties, the thermal zone devices still can be
> >> registered, and the driver can work properly, it has the basic function,
> >> can read temperature from thermal sysfs, although it doesn't have trips
> >> and bind with cooling devices.
> > 
> > 
> > If a thermal zone is used only for monitoring, then I believe it lost
> > its purpose. As  Maybe a different framework shall be used, such as hwmon,
> > for instance?
> 
> Yes, if we only use it for monitoring, we can use hwmon. But we have
> more functions base on these two thermal zone devices. We have a
> skin-temperature driver, which used nct1008's remote and local
> temperatures to estimator the skin temperature. As you know the thermal
> framework is more powerful, the remote/local sensors can be register as
> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
> to read their temperatures and then estimator skin's temp. We also will
> set trips and cooling devices for this skin-temp.

In this case, don't you think it would make sense to create a thermal
zone for the skin temperature and add the required sensors (including
the nct1008) in it?

> 
> Wei.
> 
> > 
> > The purpose of a thermal zone is to describe thermal behavior of a
> > hardware. As it is mentioned in the thermal.txt file.
> > 
> > 
> >>
> >> Thanks.
> >> Wei.
> >>
> >>>
> >>>>  
> >>>> -Optional property:
> >>>>  - coefficients:		An array of integers (one signed cell) containing
> >>>>    Type: array		coefficients to compose a linear relation between
> >>>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
> >>>> -- 
> >>>> 1.8.1.5
> >>>>
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
> Hello Wei,
> 
> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>>>> Hello Wei Ni,
>>>>>
>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>>>> Add more description for the "polling-delay" property.
>>>>>> Set "trips" and "cooling maps" as optional property, because
>>>>>> if missing these two sub-nodes, the thermal zone device still
>>>>>> work properly.
>>>>>>
>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>> index f5db6b7..e3d3ed9 100644
>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>>>  
>>>>>>  Required properties:
>>>>>>  - polling-delay:	The maximum number of milliseconds to wait between polls
>>>>>> -  Type: unsigned	when checking this thermal zone.
>>>>>> -  Size: one cell
>>>>>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
>>>>>> +  Size: one cell	driver will not run polling queue, but just cancel it.
>>>>>>  
>>>>>
>>>>> The description above is specific to Linux kernel implementation
>>>>> nomenclature. DT description needs to be OS agnostic.
>>>>>
>>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>>>    Type: unsigned	between polls when performing passive cooling.
>>>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>>>    phandles + sensor
>>>>>>    specifier
>>>>>>  
>>>>>> +Optional property:
>>>>>>  - trips:		A sub-node which is a container of only trip point nodes
>>>>>>    Type: sub-node	required to describe the thermal zone.
>>>>>>  
>>>>>>  - cooling-maps:		A sub-node which is a container of only cooling device
>>>>>>    Type: sub-node	map nodes, used to describe the relation between trips
>>>>>> -			and cooling devices.
>>>>>> +			and cooling devices. If missing the "trips" property,
>>>>>> +			This sub-node will not be parsed, because no trips can
>>>>>> +			be bound to cooling devices.
>>>>>
>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>>>> the binding describes both, cooling-maps and trips, as required
>>>>> properties. Thus, both needs to be in place to consider the thermal zone
>>>>> as a proper described zone.
>>>>
>>>> I moved the "trips" and "cooling-maps" to optional property, because if
>>>> missing these two properties, the thermal zone devices still can be
>>>> registered, and the driver can work properly, it has the basic function,
>>>> can read temperature from thermal sysfs, although it doesn't have trips
>>>> and bind with cooling devices.
>>>
>>>
>>> If a thermal zone is used only for monitoring, then I believe it lost
>>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
>>> for instance?
>>
>> Yes, if we only use it for monitoring, we can use hwmon. But we have
>> more functions base on these two thermal zone devices. We have a
>> skin-temperature driver, which used nct1008's remote and local
>> temperatures to estimator the skin temperature. As you know the thermal
>> framework is more powerful, the remote/local sensors can be register as
>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
>> to read their temperatures and then estimator skin's temp. We also will
>> set trips and cooling devices for this skin-temp.
> 
> In this case, don't you think it would make sense to create a thermal
> zone for the skin temperature and add the required sensors (including
> the nct1008) in it?

Hi, Eduardo
Yes, we will create a thermal zone for the skin-temp driver and add the
required sensors in it, but in here we want to register these required
sensors as thermal zone devices, then the thermal framework can export
functions to read these sensors temperature. If use hwmon framework, it
can't export nct1008's internal sensors, such as remote/local sensors,
and no exported functions to read these temperatures. If we doesn't use
the thermal/of-thermal framework, we have to develop a new framework to
parse those sensor nodes, and I think this new one may only have slight
difference with current thermal framework.
If we set those two properties as optional property, then we can use it
in this case simply.

The skin-temp nodes will something like this:

skin_temp: therm-est {
	compatible = "nvidia,tegra124-therm-est";

	#thermal-sensor-cells = <0>;

	sub-devs {
		dev@0 {
/* we need to register nct1008_remote as thermal zone devices, so that
it's easy to handle it by using thermal framework's exported functions. */
			dev = <&nct1008_remote>;
		};
		dev@1 {
			dev = <&nct1008_local>;
		};
	};
};

thermal-zones {
	skin-therm {
		polling-delay-passive = <15000>
		polling-delay = <0>;

		thermal-sensors = <&skin_temp>;

		trips {
		};
		cooling-maps {
		};
	};
};

Wei.
> 
>>
>> Wei.
>>
>>>
>>> The purpose of a thermal zone is to describe thermal behavior of a
>>> hardware. As it is mentioned in the thermal.txt file.
>>>
>>>
>>>>
>>>> Thanks.
>>>> Wei.
>>>>
>>>>>
>>>>>>  
>>>>>> -Optional property:
>>>>>>  - coefficients:		An array of integers (one signed cell) containing
>>>>>>    Type: array		coefficients to compose a linear relation between
>>>>>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
>>>>>> -- 
>>>>>> 1.8.1.5
>>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Eduardo Valentin]

Hello Wei,

On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
> > Hello Wei,
> > 
> > On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
> >> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
> >>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
> >>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> >>>>> Hello Wei Ni,
> >>>>>
> >>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
> >>>>>> Add more description for the "polling-delay" property.
> >>>>>> Set "trips" and "cooling maps" as optional property, because
> >>>>>> if missing these two sub-nodes, the thermal zone device still
> >>>>>> work properly.
> >>>>>>
> >>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
> >>>>>> ---
> >>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
> >>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>> index f5db6b7..e3d3ed9 100644
> >>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
> >>>>>>  
> >>>>>>  Required properties:
> >>>>>>  - polling-delay:	The maximum number of milliseconds to wait between polls
> >>>>>> -  Type: unsigned	when checking this thermal zone.
> >>>>>> -  Size: one cell
> >>>>>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
> >>>>>> +  Size: one cell	driver will not run polling queue, but just cancel it.
> >>>>>>  
> >>>>>
> >>>>> The description above is specific to Linux kernel implementation
> >>>>> nomenclature. DT description needs to be OS agnostic.
> >>>>>
> >>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
> >>>>>>    Type: unsigned	between polls when performing passive cooling.
> >>>>>> @@ -148,14 +148,16 @@ Required properties:
> >>>>>>    phandles + sensor
> >>>>>>    specifier
> >>>>>>  
> >>>>>> +Optional property:
> >>>>>>  - trips:		A sub-node which is a container of only trip point nodes
> >>>>>>    Type: sub-node	required to describe the thermal zone.
> >>>>>>  
> >>>>>>  - cooling-maps:		A sub-node which is a container of only cooling device
> >>>>>>    Type: sub-node	map nodes, used to describe the relation between trips
> >>>>>> -			and cooling devices.
> >>>>>> +			and cooling devices. If missing the "trips" property,
> >>>>>> +			This sub-node will not be parsed, because no trips can
> >>>>>> +			be bound to cooling devices.
> >>>>>
> >>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
> >>>>> the binding describes both, cooling-maps and trips, as required
> >>>>> properties. Thus, both needs to be in place to consider the thermal zone
> >>>>> as a proper described zone.
> >>>>
> >>>> I moved the "trips" and "cooling-maps" to optional property, because if
> >>>> missing these two properties, the thermal zone devices still can be
> >>>> registered, and the driver can work properly, it has the basic function,
> >>>> can read temperature from thermal sysfs, although it doesn't have trips
> >>>> and bind with cooling devices.
> >>>
> >>>
> >>> If a thermal zone is used only for monitoring, then I believe it lost
> >>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
> >>> for instance?
> >>
> >> Yes, if we only use it for monitoring, we can use hwmon. But we have
> >> more functions base on these two thermal zone devices. We have a
> >> skin-temperature driver, which used nct1008's remote and local
> >> temperatures to estimator the skin temperature. As you know the thermal
> >> framework is more powerful, the remote/local sensors can be register as
> >> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
> >> to read their temperatures and then estimator skin's temp. We also will
> >> set trips and cooling devices for this skin-temp.
> > 
> > In this case, don't you think it would make sense to create a thermal
> > zone for the skin temperature and add the required sensors (including
> > the nct1008) in it?
> 
> Hi, Eduardo
> Yes, we will create a thermal zone for the skin-temp driver and add the
> required sensors in it, but in here we want to register these required
> sensors as thermal zone devices, then the thermal framework can export
> functions to read these sensors temperature. If use hwmon framework, it
> can't export nct1008's internal sensors, such as remote/local sensors,
> and no exported functions to read these temperatures. If we doesn't use
> the thermal/of-thermal framework, we have to develop a new framework to
> parse those sensor nodes, and I think this new one may only have slight
> difference with current thermal framework.
> If we set those two properties as optional property, then we can use it
> in this case simply.
> 
> The skin-temp nodes will something like this:
> 
> skin_temp: therm-est {
> 	compatible = "nvidia,tegra124-therm-est";
> 
> 	#thermal-sensor-cells = <0>;
> 
> 	sub-devs {
> 		dev@0 {
> /* we need to register nct1008_remote as thermal zone devices, so that
> it's easy to handle it by using thermal framework's exported functions. */
> 			dev = <&nct1008_remote>;
> 		};
> 		dev@1 {
> 			dev = <&nct1008_local>;
> 		};
> 	};
> };
> 
> thermal-zones {
> 	skin-therm {
> 		polling-delay-passive = <15000>
> 		polling-delay = <0>;
> 
> 		thermal-sensors = <&skin_temp>;

The binding allows you to add several sensors in one thermal zone.
Please have a look on the example (c) of the thermal.txt binding
description. It might be that what you want to do is actually:
 		thermal-sensors = <&ntc1008_local>,
				<&ntc1008_remote>;


> 
> 		trips {
> 		};
> 		cooling-maps {
> 		};
> 	};
> };
> 
> Wei.
> > 
> >>
> >> Wei.
> >>
> >>>
> >>> The purpose of a thermal zone is to describe thermal behavior of a
> >>> hardware. As it is mentioned in the thermal.txt file.
> >>>
> >>>
> >>>>
> >>>> Thanks.
> >>>> Wei.
> >>>>
> >>>>>
> >>>>>>  
> >>>>>> -Optional property:
> >>>>>>  - coefficients:		An array of integers (one signed cell) containing
> >>>>>>    Type: array		coefficients to compose a linear relation between
> >>>>>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
> >>>>>> -- 
> >>>>>> 1.8.1.5
> >>>>>>
> >>>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>
> >>
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/28/2014 09:21 PM, Eduardo Valentin wrote:
> Hello Wei,
> 
> On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
>> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
>>> Hello Wei,
>>>
>>> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
>>>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
>>>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
>>>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>>>>>> Hello Wei Ni,
>>>>>>>
>>>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>>>>>> Add more description for the "polling-delay" property.
>>>>>>>> Set "trips" and "cooling maps" as optional property, because
>>>>>>>> if missing these two sub-nodes, the thermal zone device still
>>>>>>>> work properly.
>>>>>>>>
>>>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>> index f5db6b7..e3d3ed9 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>>>>>  
>>>>>>>>  Required properties:
>>>>>>>>  - polling-delay:	The maximum number of milliseconds to wait between polls
>>>>>>>> -  Type: unsigned	when checking this thermal zone.
>>>>>>>> -  Size: one cell
>>>>>>>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
>>>>>>>> +  Size: one cell	driver will not run polling queue, but just cancel it.
>>>>>>>>  
>>>>>>>
>>>>>>> The description above is specific to Linux kernel implementation
>>>>>>> nomenclature. DT description needs to be OS agnostic.
>>>>>>>
>>>>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>>>>>    Type: unsigned	between polls when performing passive cooling.
>>>>>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>>>>>    phandles + sensor
>>>>>>>>    specifier
>>>>>>>>  
>>>>>>>> +Optional property:
>>>>>>>>  - trips:		A sub-node which is a container of only trip point nodes
>>>>>>>>    Type: sub-node	required to describe the thermal zone.
>>>>>>>>  
>>>>>>>>  - cooling-maps:		A sub-node which is a container of only cooling device
>>>>>>>>    Type: sub-node	map nodes, used to describe the relation between trips
>>>>>>>> -			and cooling devices.
>>>>>>>> +			and cooling devices. If missing the "trips" property,
>>>>>>>> +			This sub-node will not be parsed, because no trips can
>>>>>>>> +			be bound to cooling devices.
>>>>>>>
>>>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>>>>>> the binding describes both, cooling-maps and trips, as required
>>>>>>> properties. Thus, both needs to be in place to consider the thermal zone
>>>>>>> as a proper described zone.
>>>>>>
>>>>>> I moved the "trips" and "cooling-maps" to optional property, because if
>>>>>> missing these two properties, the thermal zone devices still can be
>>>>>> registered, and the driver can work properly, it has the basic function,
>>>>>> can read temperature from thermal sysfs, although it doesn't have trips
>>>>>> and bind with cooling devices.
>>>>>
>>>>>
>>>>> If a thermal zone is used only for monitoring, then I believe it lost
>>>>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
>>>>> for instance?
>>>>
>>>> Yes, if we only use it for monitoring, we can use hwmon. But we have
>>>> more functions base on these two thermal zone devices. We have a
>>>> skin-temperature driver, which used nct1008's remote and local
>>>> temperatures to estimator the skin temperature. As you know the thermal
>>>> framework is more powerful, the remote/local sensors can be register as
>>>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
>>>> to read their temperatures and then estimator skin's temp. We also will
>>>> set trips and cooling devices for this skin-temp.
>>>
>>> In this case, don't you think it would make sense to create a thermal
>>> zone for the skin temperature and add the required sensors (including
>>> the nct1008) in it?
>>
>> Hi, Eduardo
>> Yes, we will create a thermal zone for the skin-temp driver and add the
>> required sensors in it, but in here we want to register these required
>> sensors as thermal zone devices, then the thermal framework can export
>> functions to read these sensors temperature. If use hwmon framework, it
>> can't export nct1008's internal sensors, such as remote/local sensors,
>> and no exported functions to read these temperatures. If we doesn't use
>> the thermal/of-thermal framework, we have to develop a new framework to
>> parse those sensor nodes, and I think this new one may only have slight
>> difference with current thermal framework.
>> If we set those two properties as optional property, then we can use it
>> in this case simply.
>>
>> The skin-temp nodes will something like this:
>>
>> skin_temp: therm-est {
>> 	compatible = "nvidia,tegra124-therm-est";
>>
>> 	#thermal-sensor-cells = <0>;
>>
>> 	sub-devs {
>> 		dev@0 {
>> /* we need to register nct1008_remote as thermal zone devices, so that
>> it's easy to handle it by using thermal framework's exported functions. */
>> 			dev = <&nct1008_remote>;
>> 		};
>> 		dev@1 {
>> 			dev = <&nct1008_local>;
>> 		};
>> 	};
>> };
>>
>> thermal-zones {
>> 	skin-therm {
>> 		polling-delay-passive = <15000>
>> 		polling-delay = <0>;
>>
>> 		thermal-sensors = <&skin_temp>;
> 
> The binding allows you to add several sensors in one thermal zone.
> Please have a look on the example (c) of the thermal.txt binding
> description. It might be that what you want to do is actually:
>  		thermal-sensors = <&ntc1008_local>,
> 				<&ntc1008_remote>;


Yes, we have considered it, but in current kernel, it only supports 1
sensor per zone. and according to the example (c), it's supposed to only
support simple algorithm, such as:
/* hotspot = 100 * bandgap - 120 * adc + 484 */
 coefficients =          <100    -120    484>;

but in our skin-temp driver, we will recored 20 history temperatures for
every sensors, and set different coefficients for every history
temperatures, our HW team have private mathematical mode to calculate
these coeffs for different boards. And I think other users may use
different algorithm to calculate thermal zone temperature based on
several sensors.

So I think it's better to register sensors to thermal zones, not set
trips and binding cooling devices, then user's thermal driver can use
them freely.

Thanks.
Wei.

> 
> 
>>
>> 		trips {
>> 		};
>> 		cooling-maps {
>> 		};
>> 	};
>> };
>>
>> Wei.
>>>
>>>>
>>>> Wei.
>>>>
>>>>>
>>>>> The purpose of a thermal zone is to describe thermal behavior of a
>>>>> hardware. As it is mentioned in the thermal.txt file.
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks.
>>>>>> Wei.
>>>>>>
>>>>>>>
>>>>>>>>  
>>>>>>>> -Optional property:
>>>>>>>>  - coefficients:		An array of integers (one signed cell) containing
>>>>>>>>    Type: array		coefficients to compose a linear relation between
>>>>>>>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
>>>>>>>> -- 
>>>>>>>> 1.8.1.5
>>>>>>>>
>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[edubezval]

Hello Wei,

On Thu, Aug 28, 2014 at 11:03 PM, Wei Ni <wni@nvidia.com> wrote:
> On 08/28/2014 09:21 PM, Eduardo Valentin wrote:
>> Hello Wei,
>>
>> On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
>>> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
>>>> Hello Wei,
>>>>
>>>> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
>>>>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
>>>>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
>>>>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>>>>>>> Hello Wei Ni,
>>>>>>>>
>>>>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>>>>>>> Add more description for the "polling-delay" property.
>>>>>>>>> Set "trips" and "cooling maps" as optional property, because
>>>>>>>>> if missing these two sub-nodes, the thermal zone device still
>>>>>>>>> work properly.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>>>>> ---
>>>>>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>> index f5db6b7..e3d3ed9 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>>>>>>
>>>>>>>>>  Required properties:
>>>>>>>>>  - polling-delay:      The maximum number of milliseconds to wait between polls
>>>>>>>>> -  Type: unsigned      when checking this thermal zone.
>>>>>>>>> -  Size: one cell
>>>>>>>>> +  Type: unsigned      when checking this thermal zone. If this value is 0, the
>>>>>>>>> +  Size: one cell      driver will not run polling queue, but just cancel it.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The description above is specific to Linux kernel implementation
>>>>>>>> nomenclature. DT description needs to be OS agnostic.
>>>>>>>>
>>>>>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>>>>>>    Type: unsigned      between polls when performing passive cooling.
>>>>>>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>>>>>>    phandles + sensor
>>>>>>>>>    specifier
>>>>>>>>>
>>>>>>>>> +Optional property:
>>>>>>>>>  - trips:              A sub-node which is a container of only trip point nodes
>>>>>>>>>    Type: sub-node      required to describe the thermal zone.
>>>>>>>>>
>>>>>>>>>  - cooling-maps:               A sub-node which is a container of only cooling device
>>>>>>>>>    Type: sub-node      map nodes, used to describe the relation between trips
>>>>>>>>> -                      and cooling devices.
>>>>>>>>> +                      and cooling devices. If missing the "trips" property,
>>>>>>>>> +                      This sub-node will not be parsed, because no trips can
>>>>>>>>> +                      be bound to cooling devices.
>>>>>>>>
>>>>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>>>>>>> the binding describes both, cooling-maps and trips, as required
>>>>>>>> properties. Thus, both needs to be in place to consider the thermal zone
>>>>>>>> as a proper described zone.
>>>>>>>
>>>>>>> I moved the "trips" and "cooling-maps" to optional property, because if
>>>>>>> missing these two properties, the thermal zone devices still can be
>>>>>>> registered, and the driver can work properly, it has the basic function,
>>>>>>> can read temperature from thermal sysfs, although it doesn't have trips
>>>>>>> and bind with cooling devices.
>>>>>>
>>>>>>
>>>>>> If a thermal zone is used only for monitoring, then I believe it lost
>>>>>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
>>>>>> for instance?
>>>>>
>>>>> Yes, if we only use it for monitoring, we can use hwmon. But we have
>>>>> more functions base on these two thermal zone devices. We have a
>>>>> skin-temperature driver, which used nct1008's remote and local
>>>>> temperatures to estimator the skin temperature. As you know the thermal
>>>>> framework is more powerful, the remote/local sensors can be register as
>>>>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
>>>>> to read their temperatures and then estimator skin's temp. We also will
>>>>> set trips and cooling devices for this skin-temp.
>>>>
>>>> In this case, don't you think it would make sense to create a thermal
>>>> zone for the skin temperature and add the required sensors (including
>>>> the nct1008) in it?
>>>
>>> Hi, Eduardo
>>> Yes, we will create a thermal zone for the skin-temp driver and add the
>>> required sensors in it, but in here we want to register these required
>>> sensors as thermal zone devices, then the thermal framework can export
>>> functions to read these sensors temperature. If use hwmon framework, it
>>> can't export nct1008's internal sensors, such as remote/local sensors,
>>> and no exported functions to read these temperatures. If we doesn't use
>>> the thermal/of-thermal framework, we have to develop a new framework to
>>> parse those sensor nodes, and I think this new one may only have slight
>>> difference with current thermal framework.
>>> If we set those two properties as optional property, then we can use it
>>> in this case simply.
>>>
>>> The skin-temp nodes will something like this:
>>>
>>> skin_temp: therm-est {
>>>      compatible = "nvidia,tegra124-therm-est";
>>>
>>>      #thermal-sensor-cells = <0>;
>>>
>>>      sub-devs {
>>>              dev@0 {
>>> /* we need to register nct1008_remote as thermal zone devices, so that
>>> it's easy to handle it by using thermal framework's exported functions. */
>>>                      dev = <&nct1008_remote>;
>>>              };
>>>              dev@1 {
>>>                      dev = <&nct1008_local>;
>>>              };
>>>      };
>>> };
>>>
>>> thermal-zones {
>>>      skin-therm {
>>>              polling-delay-passive = <15000>
>>>              polling-delay = <0>;
>>>
>>>              thermal-sensors = <&skin_temp>;
>>
>> The binding allows you to add several sensors in one thermal zone.
>> Please have a look on the example (c) of the thermal.txt binding
>> description. It might be that what you want to do is actually:
>>               thermal-sensors = <&ntc1008_local>,
>>                               <&ntc1008_remote>;
>
>
> Yes, we have considered it, but in current kernel, it only supports 1
> sensor per zone. and according to the example (c), it's supposed to only
> support simple algorithm, such as:
> /* hotspot = 100 * bandgap - 120 * adc + 484 */
>  coefficients =          <100    -120    484>;
>
> but in our skin-temp driver, we will recored 20 history temperatures for
> every sensors, and set different coefficients for every history
> temperatures, our HW team have private mathematical mode to calculate
> these coeffs for different boards. And I think other users may use
> different algorithm to calculate thermal zone temperature based on
> several sensors.
>
> So I think it's better to register sensors to thermal zones, not set
> trips and binding cooling devices, then user's thermal driver can use
> them freely.
>

In your system, the used governor is userspace, then?

> Thanks.
> Wei.
>
>>
>>
>>>
>>>              trips {
>>>              };
>>>              cooling-maps {
>>>              };
>>>      };
>>> };
>>>
>>> Wei.
>>>>
>>>>>
>>>>> Wei.
>>>>>
>>>>>>
>>>>>> The purpose of a thermal zone is to describe thermal behavior of a
>>>>>> hardware. As it is mentioned in the thermal.txt file.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks.
>>>>>>> Wei.
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> -Optional property:
>>>>>>>>>  - coefficients:               An array of integers (one signed cell) containing
>>>>>>>>>    Type: array         coefficients to compose a linear relation between
>>>>>>>>>    Elem size: one cell the sensors listed in the thermal-sensors property.
>>>>>>>>> --
>>>>>>>>> 1.8.1.5
>>>>>>>>>
>>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>>
>>>
>



-- 
Eduardo Bezerra Valentin

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/29/2014 07:32 PM, edubezval@gmail.com wrote:
> Hello Wei,
> 
> On Thu, Aug 28, 2014 at 11:03 PM, Wei Ni <wni@nvidia.com> wrote:
>> On 08/28/2014 09:21 PM, Eduardo Valentin wrote:
>>> Hello Wei,
>>>
>>> On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
>>>> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
>>>>> Hello Wei,
>>>>>
>>>>> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
>>>>>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
>>>>>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
>>>>>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>>>>>>>> Hello Wei Ni,
>>>>>>>>>
>>>>>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>>>>>>>> Add more description for the "polling-delay" property.
>>>>>>>>>> Set "trips" and "cooling maps" as optional property, because
>>>>>>>>>> if missing these two sub-nodes, the thermal zone device still
>>>>>>>>>> work properly.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>>>>>> ---
>>>>>>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>> index f5db6b7..e3d3ed9 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>>>>>>>
>>>>>>>>>>  Required properties:
>>>>>>>>>>  - polling-delay:      The maximum number of milliseconds to wait between polls
>>>>>>>>>> -  Type: unsigned      when checking this thermal zone.
>>>>>>>>>> -  Size: one cell
>>>>>>>>>> +  Type: unsigned      when checking this thermal zone. If this value is 0, the
>>>>>>>>>> +  Size: one cell      driver will not run polling queue, but just cancel it.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The description above is specific to Linux kernel implementation
>>>>>>>>> nomenclature. DT description needs to be OS agnostic.
>>>>>>>>>
>>>>>>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>>>>>>>    Type: unsigned      between polls when performing passive cooling.
>>>>>>>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>>>>>>>    phandles + sensor
>>>>>>>>>>    specifier
>>>>>>>>>>
>>>>>>>>>> +Optional property:
>>>>>>>>>>  - trips:              A sub-node which is a container of only trip point nodes
>>>>>>>>>>    Type: sub-node      required to describe the thermal zone.
>>>>>>>>>>
>>>>>>>>>>  - cooling-maps:               A sub-node which is a container of only cooling device
>>>>>>>>>>    Type: sub-node      map nodes, used to describe the relation between trips
>>>>>>>>>> -                      and cooling devices.
>>>>>>>>>> +                      and cooling devices. If missing the "trips" property,
>>>>>>>>>> +                      This sub-node will not be parsed, because no trips can
>>>>>>>>>> +                      be bound to cooling devices.
>>>>>>>>>
>>>>>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>>>>>>>> the binding describes both, cooling-maps and trips, as required
>>>>>>>>> properties. Thus, both needs to be in place to consider the thermal zone
>>>>>>>>> as a proper described zone.
>>>>>>>>
>>>>>>>> I moved the "trips" and "cooling-maps" to optional property, because if
>>>>>>>> missing these two properties, the thermal zone devices still can be
>>>>>>>> registered, and the driver can work properly, it has the basic function,
>>>>>>>> can read temperature from thermal sysfs, although it doesn't have trips
>>>>>>>> and bind with cooling devices.
>>>>>>>
>>>>>>>
>>>>>>> If a thermal zone is used only for monitoring, then I believe it lost
>>>>>>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
>>>>>>> for instance?
>>>>>>
>>>>>> Yes, if we only use it for monitoring, we can use hwmon. But we have
>>>>>> more functions base on these two thermal zone devices. We have a
>>>>>> skin-temperature driver, which used nct1008's remote and local
>>>>>> temperatures to estimator the skin temperature. As you know the thermal
>>>>>> framework is more powerful, the remote/local sensors can be register as
>>>>>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
>>>>>> to read their temperatures and then estimator skin's temp. We also will
>>>>>> set trips and cooling devices for this skin-temp.
>>>>>
>>>>> In this case, don't you think it would make sense to create a thermal
>>>>> zone for the skin temperature and add the required sensors (including
>>>>> the nct1008) in it?
>>>>
>>>> Hi, Eduardo
>>>> Yes, we will create a thermal zone for the skin-temp driver and add the
>>>> required sensors in it, but in here we want to register these required
>>>> sensors as thermal zone devices, then the thermal framework can export
>>>> functions to read these sensors temperature. If use hwmon framework, it
>>>> can't export nct1008's internal sensors, such as remote/local sensors,
>>>> and no exported functions to read these temperatures. If we doesn't use
>>>> the thermal/of-thermal framework, we have to develop a new framework to
>>>> parse those sensor nodes, and I think this new one may only have slight
>>>> difference with current thermal framework.
>>>> If we set those two properties as optional property, then we can use it
>>>> in this case simply.
>>>>
>>>> The skin-temp nodes will something like this:
>>>>
>>>> skin_temp: therm-est {
>>>>      compatible = "nvidia,tegra124-therm-est";
>>>>
>>>>      #thermal-sensor-cells = <0>;
>>>>
>>>>      sub-devs {
>>>>              dev@0 {
>>>> /* we need to register nct1008_remote as thermal zone devices, so that
>>>> it's easy to handle it by using thermal framework's exported functions. */
>>>>                      dev = <&nct1008_remote>;
>>>>              };
>>>>              dev@1 {
>>>>                      dev = <&nct1008_local>;
>>>>              };
>>>>      };
>>>> };
>>>>
>>>> thermal-zones {
>>>>      skin-therm {
>>>>              polling-delay-passive = <15000>
>>>>              polling-delay = <0>;
>>>>
>>>>              thermal-sensors = <&skin_temp>;
>>>
>>> The binding allows you to add several sensors in one thermal zone.
>>> Please have a look on the example (c) of the thermal.txt binding
>>> description. It might be that what you want to do is actually:
>>>               thermal-sensors = <&ntc1008_local>,
>>>                               <&ntc1008_remote>;
>>
>>
>> Yes, we have considered it, but in current kernel, it only supports 1
>> sensor per zone. and according to the example (c), it's supposed to only
>> support simple algorithm, such as:
>> /* hotspot = 100 * bandgap - 120 * adc + 484 */
>>  coefficients =          <100    -120    484>;
>>
>> but in our skin-temp driver, we will recored 20 history temperatures for
>> every sensors, and set different coefficients for every history
>> temperatures, our HW team have private mathematical mode to calculate
>> these coeffs for different boards. And I think other users may use
>> different algorithm to calculate thermal zone temperature based on
>> several sensors.
>>
>> So I think it's better to register sensors to thermal zones, not set
>> trips and binding cooling devices, then user's thermal driver can use
>> them freely.
>>
> 
> In your system, the used governor is userspace, then?

No, we developed a new governor named as "pid governor", it manages
thermals based on output values of PID controller.

Indeed, our thermal management includes skin-temp, soc-thermal,
cpu-throttle driver and pid governor. This pach set is prepared for the
skin-temp driver.
As you know the tegra soc-therm is in upstreaming, and we will post
other drivers step by step.

Thanks.
Wei.

> 
>> Thanks.
>> Wei.
>>
>>>
>>>
>>>>
>>>>              trips {
>>>>              };
>>>>              cooling-maps {
>>>>              };
>>>>      };
>>>> };
>>>>
>>>> Wei.
>>>>>
>>>>>>
>>>>>> Wei.
>>>>>>
>>>>>>>
>>>>>>> The purpose of a thermal zone is to describe thermal behavior of a
>>>>>>> hardware. As it is mentioned in the thermal.txt file.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Wei.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -Optional property:
>>>>>>>>>>  - coefficients:               An array of integers (one signed cell) containing
>>>>>>>>>>    Type: array         coefficients to compose a linear relation between
>>>>>>>>>>    Elem size: one cell the sensors listed in the thermal-sensors property.
>>>>>>>>>> --
>>>>>>>>>> 1.8.1.5
>>>>>>>>>>
>>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>>>>>
>>>>
>>
> 
> 
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

Hi, Eduardo

On 09/01/2014 06:26 PM, Wei Ni wrote:
> On 08/29/2014 07:32 PM, edubezval@gmail.com wrote:
>> Hello Wei,
>>
>> On Thu, Aug 28, 2014 at 11:03 PM, Wei Ni <wni@nvidia.com> wrote:
>>> On 08/28/2014 09:21 PM, Eduardo Valentin wrote:
>>>> Hello Wei,
>>>>
>>>> On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
>>>>> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
>>>>>> Hello Wei,
>>>>>>
>>>>>> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
>>>>>>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
>>>>>>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
>>>>>>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>>>>>>>>> Hello Wei Ni,
>>>>>>>>>>
>>>>>>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>>>>>>>>> Add more description for the "polling-delay" property.
>>>>>>>>>>> Set "trips" and "cooling maps" as optional property, because
>>>>>>>>>>> if missing these two sub-nodes, the thermal zone device still
>>>>>>>>>>> work properly.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>>>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>>> index f5db6b7..e3d3ed9 100644
>>>>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>>>>>>>>
>>>>>>>>>>>  Required properties:
>>>>>>>>>>>  - polling-delay:      The maximum number of milliseconds to wait between polls
>>>>>>>>>>> -  Type: unsigned      when checking this thermal zone.
>>>>>>>>>>> -  Size: one cell
>>>>>>>>>>> +  Type: unsigned      when checking this thermal zone. If this value is 0, the
>>>>>>>>>>> +  Size: one cell      driver will not run polling queue, but just cancel it.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The description above is specific to Linux kernel implementation
>>>>>>>>>> nomenclature. DT description needs to be OS agnostic.
>>>>>>>>>>
>>>>>>>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>>>>>>>>    Type: unsigned      between polls when performing passive cooling.
>>>>>>>>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>>>>>>>>    phandles + sensor
>>>>>>>>>>>    specifier
>>>>>>>>>>>
>>>>>>>>>>> +Optional property:
>>>>>>>>>>>  - trips:              A sub-node which is a container of only trip point nodes
>>>>>>>>>>>    Type: sub-node      required to describe the thermal zone.
>>>>>>>>>>>
>>>>>>>>>>>  - cooling-maps:               A sub-node which is a container of only cooling device
>>>>>>>>>>>    Type: sub-node      map nodes, used to describe the relation between trips
>>>>>>>>>>> -                      and cooling devices.
>>>>>>>>>>> +                      and cooling devices. If missing the "trips" property,
>>>>>>>>>>> +                      This sub-node will not be parsed, because no trips can
>>>>>>>>>>> +                      be bound to cooling devices.
>>>>>>>>>>
>>>>>>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>>>>>>>>> the binding describes both, cooling-maps and trips, as required
>>>>>>>>>> properties. Thus, both needs to be in place to consider the thermal zone
>>>>>>>>>> as a proper described zone.
>>>>>>>>>
>>>>>>>>> I moved the "trips" and "cooling-maps" to optional property, because if
>>>>>>>>> missing these two properties, the thermal zone devices still can be
>>>>>>>>> registered, and the driver can work properly, it has the basic function,
>>>>>>>>> can read temperature from thermal sysfs, although it doesn't have trips
>>>>>>>>> and bind with cooling devices.
>>>>>>>>
>>>>>>>>
>>>>>>>> If a thermal zone is used only for monitoring, then I believe it lost
>>>>>>>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
>>>>>>>> for instance?
>>>>>>>
>>>>>>> Yes, if we only use it for monitoring, we can use hwmon. But we have
>>>>>>> more functions base on these two thermal zone devices. We have a
>>>>>>> skin-temperature driver, which used nct1008's remote and local
>>>>>>> temperatures to estimator the skin temperature. As you know the thermal
>>>>>>> framework is more powerful, the remote/local sensors can be register as
>>>>>>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
>>>>>>> to read their temperatures and then estimator skin's temp. We also will
>>>>>>> set trips and cooling devices for this skin-temp.
>>>>>>
>>>>>> In this case, don't you think it would make sense to create a thermal
>>>>>> zone for the skin temperature and add the required sensors (including
>>>>>> the nct1008) in it?
>>>>>
>>>>> Hi, Eduardo
>>>>> Yes, we will create a thermal zone for the skin-temp driver and add the
>>>>> required sensors in it, but in here we want to register these required
>>>>> sensors as thermal zone devices, then the thermal framework can export
>>>>> functions to read these sensors temperature. If use hwmon framework, it
>>>>> can't export nct1008's internal sensors, such as remote/local sensors,
>>>>> and no exported functions to read these temperatures. If we doesn't use
>>>>> the thermal/of-thermal framework, we have to develop a new framework to
>>>>> parse those sensor nodes, and I think this new one may only have slight
>>>>> difference with current thermal framework.
>>>>> If we set those two properties as optional property, then we can use it
>>>>> in this case simply.
>>>>>
>>>>> The skin-temp nodes will something like this:
>>>>>
>>>>> skin_temp: therm-est {
>>>>>      compatible = "nvidia,tegra124-therm-est";
>>>>>
>>>>>      #thermal-sensor-cells = <0>;
>>>>>
>>>>>      sub-devs {
>>>>>              dev@0 {
>>>>> /* we need to register nct1008_remote as thermal zone devices, so that
>>>>> it's easy to handle it by using thermal framework's exported functions. */
>>>>>                      dev = <&nct1008_remote>;
>>>>>              };
>>>>>              dev@1 {
>>>>>                      dev = <&nct1008_local>;
>>>>>              };
>>>>>      };
>>>>> };
>>>>>
>>>>> thermal-zones {
>>>>>      skin-therm {
>>>>>              polling-delay-passive = <15000>
>>>>>              polling-delay = <0>;
>>>>>
>>>>>              thermal-sensors = <&skin_temp>;
>>>>
>>>> The binding allows you to add several sensors in one thermal zone.
>>>> Please have a look on the example (c) of the thermal.txt binding
>>>> description. It might be that what you want to do is actually:
>>>>               thermal-sensors = <&ntc1008_local>,
>>>>                               <&ntc1008_remote>;
>>>
>>>
>>> Yes, we have considered it, but in current kernel, it only supports 1
>>> sensor per zone. and according to the example (c), it's supposed to only
>>> support simple algorithm, such as:
>>> /* hotspot = 100 * bandgap - 120 * adc + 484 */
>>>  coefficients =          <100    -120    484>;
>>>
>>> but in our skin-temp driver, we will recored 20 history temperatures for
>>> every sensors, and set different coefficients for every history
>>> temperatures, our HW team have private mathematical mode to calculate
>>> these coeffs for different boards. And I think other users may use
>>> different algorithm to calculate thermal zone temperature based on
>>> several sensors.
>>>
>>> So I think it's better to register sensors to thermal zones, not set
>>> trips and binding cooling devices, then user's thermal driver can use
>>> them freely.
>>>
>>
>> In your system, the used governor is userspace, then?
> 
> No, we developed a new governor named as "pid governor", it manages
> thermals based on output values of PID controller.
> 
> Indeed, our thermal management includes skin-temp, soc-thermal,
> cpu-throttle driver and pid governor. This pach set is prepared for the
> skin-temp driver.
> As you know the tegra soc-therm is in upstreaming, and we will post
> other drivers step by step.

Will you take this patch, if so i will post skin-temp driver later.
Looking forward your comments :)

Thanks.
Wei.

> 
> Thanks.
> Wei.
> 
>>
>>> Thanks.
>>> Wei.
>>>
>>>>
>>>>
>>>>>
>>>>>              trips {
>>>>>              };
>>>>>              cooling-maps {
>>>>>              };
>>>>>      };
>>>>> };
>>>>>
>>>>> Wei.
>>>>>>
>>>>>>>
>>>>>>> Wei.
>>>>>>>
>>>>>>>>
>>>>>>>> The purpose of a thermal zone is to describe thermal behavior of a
>>>>>>>> hardware. As it is mentioned in the thermal.txt file.
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>> Wei.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> -Optional property:
>>>>>>>>>>>  - coefficients:               An array of integers (one signed cell) containing
>>>>>>>>>>>    Type: array         coefficients to compose a linear relation between
>>>>>>>>>>>    Elem size: one cell the sensors listed in the thermal-sensors property.
>>>>>>>>>>> --
>>>>>>>>>>> 1.8.1.5
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Eduardo Valentin]

Hello Wei,

On Fri, Sep 05, 2014 at 05:41:12PM +0800, Wei Ni wrote:
> Hi, Eduardo
> 
> On 09/01/2014 06:26 PM, Wei Ni wrote:
> > On 08/29/2014 07:32 PM, edubezval@gmail.com wrote:
> >> Hello Wei,
> >>
> >> On Thu, Aug 28, 2014 at 11:03 PM, Wei Ni <wni@nvidia.com> wrote:
> >>> On 08/28/2014 09:21 PM, Eduardo Valentin wrote:
> >>>> Hello Wei,
> >>>>
> >>>> On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
> >>>>> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
> >>>>>> Hello Wei,
> >>>>>>
> >>>>>> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
> >>>>>>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
> >>>>>>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
> >>>>>>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> >>>>>>>>>> Hello Wei Ni,
> >>>>>>>>>>
> >>>>>>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
> >>>>>>>>>>> Add more description for the "polling-delay" property.
> >>>>>>>>>>> Set "trips" and "cooling maps" as optional property, because
> >>>>>>>>>>> if missing these two sub-nodes, the thermal zone device still
> >>>>>>>>>>> work properly.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
> >>>>>>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>>>>>>> index f5db6b7..e3d3ed9 100644
> >>>>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >>>>>>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
> >>>>>>>>>>>
> >>>>>>>>>>>  Required properties:
> >>>>>>>>>>>  - polling-delay:      The maximum number of milliseconds to wait between polls
> >>>>>>>>>>> -  Type: unsigned      when checking this thermal zone.
> >>>>>>>>>>> -  Size: one cell
> >>>>>>>>>>> +  Type: unsigned      when checking this thermal zone. If this value is 0, the
> >>>>>>>>>>> +  Size: one cell      driver will not run polling queue, but just cancel it.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> The description above is specific to Linux kernel implementation
> >>>>>>>>>> nomenclature. DT description needs to be OS agnostic.
> >>>>>>>>>>
> >>>>>>>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
> >>>>>>>>>>>    Type: unsigned      between polls when performing passive cooling.
> >>>>>>>>>>> @@ -148,14 +148,16 @@ Required properties:
> >>>>>>>>>>>    phandles + sensor
> >>>>>>>>>>>    specifier
> >>>>>>>>>>>
> >>>>>>>>>>> +Optional property:
> >>>>>>>>>>>  - trips:              A sub-node which is a container of only trip point nodes
> >>>>>>>>>>>    Type: sub-node      required to describe the thermal zone.
> >>>>>>>>>>>
> >>>>>>>>>>>  - cooling-maps:               A sub-node which is a container of only cooling device
> >>>>>>>>>>>    Type: sub-node      map nodes, used to describe the relation between trips
> >>>>>>>>>>> -                      and cooling devices.
> >>>>>>>>>>> +                      and cooling devices. If missing the "trips" property,
> >>>>>>>>>>> +                      This sub-node will not be parsed, because no trips can
> >>>>>>>>>>> +                      be bound to cooling devices.
> >>>>>>>>>>
> >>>>>>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
> >>>>>>>>>> the binding describes both, cooling-maps and trips, as required
> >>>>>>>>>> properties. Thus, both needs to be in place to consider the thermal zone
> >>>>>>>>>> as a proper described zone.
> >>>>>>>>>
> >>>>>>>>> I moved the "trips" and "cooling-maps" to optional property, because if
> >>>>>>>>> missing these two properties, the thermal zone devices still can be
> >>>>>>>>> registered, and the driver can work properly, it has the basic function,
> >>>>>>>>> can read temperature from thermal sysfs, although it doesn't have trips
> >>>>>>>>> and bind with cooling devices.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> If a thermal zone is used only for monitoring, then I believe it lost
> >>>>>>>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
> >>>>>>>> for instance?
> >>>>>>>
> >>>>>>> Yes, if we only use it for monitoring, we can use hwmon. But we have
> >>>>>>> more functions base on these two thermal zone devices. We have a
> >>>>>>> skin-temperature driver, which used nct1008's remote and local
> >>>>>>> temperatures to estimator the skin temperature. As you know the thermal
> >>>>>>> framework is more powerful, the remote/local sensors can be register as
> >>>>>>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
> >>>>>>> to read their temperatures and then estimator skin's temp. We also will
> >>>>>>> set trips and cooling devices for this skin-temp.
> >>>>>>
> >>>>>> In this case, don't you think it would make sense to create a thermal
> >>>>>> zone for the skin temperature and add the required sensors (including
> >>>>>> the nct1008) in it?
> >>>>>
> >>>>> Hi, Eduardo
> >>>>> Yes, we will create a thermal zone for the skin-temp driver and add the
> >>>>> required sensors in it, but in here we want to register these required
> >>>>> sensors as thermal zone devices, then the thermal framework can export
> >>>>> functions to read these sensors temperature. If use hwmon framework, it
> >>>>> can't export nct1008's internal sensors, such as remote/local sensors,
> >>>>> and no exported functions to read these temperatures. If we doesn't use
> >>>>> the thermal/of-thermal framework, we have to develop a new framework to
> >>>>> parse those sensor nodes, and I think this new one may only have slight
> >>>>> difference with current thermal framework.
> >>>>> If we set those two properties as optional property, then we can use it
> >>>>> in this case simply.
> >>>>>
> >>>>> The skin-temp nodes will something like this:
> >>>>>
> >>>>> skin_temp: therm-est {
> >>>>>      compatible = "nvidia,tegra124-therm-est";
> >>>>>
> >>>>>      #thermal-sensor-cells = <0>;
> >>>>>
> >>>>>      sub-devs {
> >>>>>              dev@0 {
> >>>>> /* we need to register nct1008_remote as thermal zone devices, so that
> >>>>> it's easy to handle it by using thermal framework's exported functions. */
> >>>>>                      dev = <&nct1008_remote>;
> >>>>>              };
> >>>>>              dev@1 {
> >>>>>                      dev = <&nct1008_local>;
> >>>>>              };
> >>>>>      };
> >>>>> };
> >>>>>
> >>>>> thermal-zones {
> >>>>>      skin-therm {
> >>>>>              polling-delay-passive = <15000>
> >>>>>              polling-delay = <0>;
> >>>>>
> >>>>>              thermal-sensors = <&skin_temp>;
> >>>>
> >>>> The binding allows you to add several sensors in one thermal zone.
> >>>> Please have a look on the example (c) of the thermal.txt binding
> >>>> description. It might be that what you want to do is actually:
> >>>>               thermal-sensors = <&ntc1008_local>,
> >>>>                               <&ntc1008_remote>;
> >>>
> >>>
> >>> Yes, we have considered it, but in current kernel, it only supports 1
> >>> sensor per zone. and according to the example (c), it's supposed to only
> >>> support simple algorithm, such as:
> >>> /* hotspot = 100 * bandgap - 120 * adc + 484 */
> >>>  coefficients =          <100    -120    484>;
> >>>
> >>> but in our skin-temp driver, we will recored 20 history temperatures for
> >>> every sensors, and set different coefficients for every history
> >>> temperatures, our HW team have private mathematical mode to calculate
> >>> these coeffs for different boards. And I think other users may use
> >>> different algorithm to calculate thermal zone temperature based on
> >>> several sensors.
> >>>
> >>> So I think it's better to register sensors to thermal zones, not set
> >>> trips and binding cooling devices, then user's thermal driver can use
> >>> them freely.
> >>>
> >>
> >> In your system, the used governor is userspace, then?
> > 
> > No, we developed a new governor named as "pid governor", it manages
> > thermals based on output values of PID controller.

Have you tried the patch set from Javi? the power allocator schem 
includes a PID controller governor. Might be worth having a common
solution. I think it is easier if I merge his changes into a branch and
you have try on it. His patch series have already gone through several
reviews. But you are free to comment too.

> > 
> > Indeed, our thermal management includes skin-temp, soc-thermal,
> > cpu-throttle driver and pid governor. This pach set is prepared for the
> > skin-temp driver.
> > As you know the tegra soc-therm is in upstreaming, and we will post
> > other drivers step by step.
> 
> Will you take this patch, if so i will post skin-temp driver later.
> Looking forward your comments :)
> 

I am afraid moving the maps and trips properties to optional abuses the
purpose of the thermal DT. The primary goal is to describe hardware, and
in this specific case, the hardware proper thermal operating conditions.

DT has to be as agnostic to OS implementation as possible.

For the above reason, I do not think this patch is a good idea.


Cheers,

> Thanks.
> Wei.
> 
> > 
> > Thanks.
> > Wei.
> > 
> >>
> >>> Thanks.
> >>> Wei.
> >>>
> >>>>
> >>>>
> >>>>>
> >>>>>              trips {
> >>>>>              };
> >>>>>              cooling-maps {
> >>>>>              };
> >>>>>      };
> >>>>> };
> >>>>>
> >>>>> Wei.
> >>>>>>
> >>>>>>>
> >>>>>>> Wei.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> The purpose of a thermal zone is to describe thermal behavior of a
> >>>>>>>> hardware. As it is mentioned in the thermal.txt file.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>> Wei.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> -Optional property:
> >>>>>>>>>>>  - coefficients:               An array of integers (one signed cell) containing
> >>>>>>>>>>>    Type: array         coefficients to compose a linear relation between
> >>>>>>>>>>>    Elem size: one cell the sensors listed in the thermal-sensors property.
> >>>>>>>>>>> --
> >>>>>>>>>>> 1.8.1.5
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>> --
> >>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> >>>>>>>> the body of a message to majordomo@vger.kernel.org
> >>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>
> >>
> >>
> >>
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/31/2014 03:43 AM, Eduardo Valentin wrote:
> 
> Hello Wei,
> 
> On Fri, Sep 05, 2014 at 05:41:12PM +0800, Wei Ni wrote:
>> Hi, Eduardo
>>
>> On 09/01/2014 06:26 PM, Wei Ni wrote:
>>> On 08/29/2014 07:32 PM, edubezval@gmail.com wrote:
>>>> Hello Wei,
>>>>
>>>> On Thu, Aug 28, 2014 at 11:03 PM, Wei Ni <wni@nvidia.com> wrote:
>>>>> On 08/28/2014 09:21 PM, Eduardo Valentin wrote:
>>>>>> Hello Wei,
>>>>>>
>>>>>> On Thu, Aug 28, 2014 at 09:50:01AM +0800, Wei Ni wrote:
>>>>>>> On 08/27/2014 09:32 PM, Eduardo Valentin wrote:
>>>>>>>> Hello Wei,
>>>>>>>>
>>>>>>>> On Wed, Aug 27, 2014 at 10:54:07AM +0800, Wei Ni wrote:
>>>>>>>>> On 08/26/2014 08:12 PM, Eduardo Valentin wrote:
>>>>>>>>>> On Tue, Aug 26, 2014 at 10:17:29AM +0800, Wei Ni wrote:
>>>>>>>>>>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>>>>>>>>>>> Hello Wei Ni,
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>>>>>>>>>>> Add more description for the "polling-delay" property.
>>>>>>>>>>>>> Set "trips" and "cooling maps" as optional property, because
>>>>>>>>>>>>> if missing these two sub-nodes, the thermal zone device still
>>>>>>>>>>>>> work properly.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>>>>>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>>>>> index f5db6b7..e3d3ed9 100644
>>>>>>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>>>>>>>>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>>>>>>>>>>
>>>>>>>>>>>>>  Required properties:
>>>>>>>>>>>>>  - polling-delay:      The maximum number of milliseconds to wait between polls
>>>>>>>>>>>>> -  Type: unsigned      when checking this thermal zone.
>>>>>>>>>>>>> -  Size: one cell
>>>>>>>>>>>>> +  Type: unsigned      when checking this thermal zone. If this value is 0, the
>>>>>>>>>>>>> +  Size: one cell      driver will not run polling queue, but just cancel it.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> The description above is specific to Linux kernel implementation
>>>>>>>>>>>> nomenclature. DT description needs to be OS agnostic.
>>>>>>>>>>>>
>>>>>>>>>>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>>>>>>>>>>    Type: unsigned      between polls when performing passive cooling.
>>>>>>>>>>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>>>>>>>>>>    phandles + sensor
>>>>>>>>>>>>>    specifier
>>>>>>>>>>>>>
>>>>>>>>>>>>> +Optional property:
>>>>>>>>>>>>>  - trips:              A sub-node which is a container of only trip point nodes
>>>>>>>>>>>>>    Type: sub-node      required to describe the thermal zone.
>>>>>>>>>>>>>
>>>>>>>>>>>>>  - cooling-maps:               A sub-node which is a container of only cooling device
>>>>>>>>>>>>>    Type: sub-node      map nodes, used to describe the relation between trips
>>>>>>>>>>>>> -                      and cooling devices.
>>>>>>>>>>>>> +                      and cooling devices. If missing the "trips" property,
>>>>>>>>>>>>> +                      This sub-node will not be parsed, because no trips can
>>>>>>>>>>>>> +                      be bound to cooling devices.
>>>>>>>>>>>>
>>>>>>>>>>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>>>>>>>>>>> the binding describes both, cooling-maps and trips, as required
>>>>>>>>>>>> properties. Thus, both needs to be in place to consider the thermal zone
>>>>>>>>>>>> as a proper described zone.
>>>>>>>>>>>
>>>>>>>>>>> I moved the "trips" and "cooling-maps" to optional property, because if
>>>>>>>>>>> missing these two properties, the thermal zone devices still can be
>>>>>>>>>>> registered, and the driver can work properly, it has the basic function,
>>>>>>>>>>> can read temperature from thermal sysfs, although it doesn't have trips
>>>>>>>>>>> and bind with cooling devices.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If a thermal zone is used only for monitoring, then I believe it lost
>>>>>>>>>> its purpose. As  Maybe a different framework shall be used, such as hwmon,
>>>>>>>>>> for instance?
>>>>>>>>>
>>>>>>>>> Yes, if we only use it for monitoring, we can use hwmon. But we have
>>>>>>>>> more functions base on these two thermal zone devices. We have a
>>>>>>>>> skin-temperature driver, which used nct1008's remote and local
>>>>>>>>> temperatures to estimator the skin temperature. As you know the thermal
>>>>>>>>> framework is more powerful, the remote/local sensors can be register as
>>>>>>>>> thermal zone, then the skin-temp driver can use thermal_zone_get_temp()
>>>>>>>>> to read their temperatures and then estimator skin's temp. We also will
>>>>>>>>> set trips and cooling devices for this skin-temp.
>>>>>>>>
>>>>>>>> In this case, don't you think it would make sense to create a thermal
>>>>>>>> zone for the skin temperature and add the required sensors (including
>>>>>>>> the nct1008) in it?
>>>>>>>
>>>>>>> Hi, Eduardo
>>>>>>> Yes, we will create a thermal zone for the skin-temp driver and add the
>>>>>>> required sensors in it, but in here we want to register these required
>>>>>>> sensors as thermal zone devices, then the thermal framework can export
>>>>>>> functions to read these sensors temperature. If use hwmon framework, it
>>>>>>> can't export nct1008's internal sensors, such as remote/local sensors,
>>>>>>> and no exported functions to read these temperatures. If we doesn't use
>>>>>>> the thermal/of-thermal framework, we have to develop a new framework to
>>>>>>> parse those sensor nodes, and I think this new one may only have slight
>>>>>>> difference with current thermal framework.
>>>>>>> If we set those two properties as optional property, then we can use it
>>>>>>> in this case simply.
>>>>>>>
>>>>>>> The skin-temp nodes will something like this:
>>>>>>>
>>>>>>> skin_temp: therm-est {
>>>>>>>      compatible = "nvidia,tegra124-therm-est";
>>>>>>>
>>>>>>>      #thermal-sensor-cells = <0>;
>>>>>>>
>>>>>>>      sub-devs {
>>>>>>>              dev@0 {
>>>>>>> /* we need to register nct1008_remote as thermal zone devices, so that
>>>>>>> it's easy to handle it by using thermal framework's exported functions. */
>>>>>>>                      dev = <&nct1008_remote>;
>>>>>>>              };
>>>>>>>              dev@1 {
>>>>>>>                      dev = <&nct1008_local>;
>>>>>>>              };
>>>>>>>      };
>>>>>>> };
>>>>>>>
>>>>>>> thermal-zones {
>>>>>>>      skin-therm {
>>>>>>>              polling-delay-passive = <15000>
>>>>>>>              polling-delay = <0>;
>>>>>>>
>>>>>>>              thermal-sensors = <&skin_temp>;
>>>>>>
>>>>>> The binding allows you to add several sensors in one thermal zone.
>>>>>> Please have a look on the example (c) of the thermal.txt binding
>>>>>> description. It might be that what you want to do is actually:
>>>>>>               thermal-sensors = <&ntc1008_local>,
>>>>>>                               <&ntc1008_remote>;
>>>>>
>>>>>
>>>>> Yes, we have considered it, but in current kernel, it only supports 1
>>>>> sensor per zone. and according to the example (c), it's supposed to only
>>>>> support simple algorithm, such as:
>>>>> /* hotspot = 100 * bandgap - 120 * adc + 484 */
>>>>>  coefficients =          <100    -120    484>;
>>>>>
>>>>> but in our skin-temp driver, we will recored 20 history temperatures for
>>>>> every sensors, and set different coefficients for every history
>>>>> temperatures, our HW team have private mathematical mode to calculate
>>>>> these coeffs for different boards. And I think other users may use
>>>>> different algorithm to calculate thermal zone temperature based on
>>>>> several sensors.
>>>>>
>>>>> So I think it's better to register sensors to thermal zones, not set
>>>>> trips and binding cooling devices, then user's thermal driver can use
>>>>> them freely.
>>>>>
>>>>
>>>> In your system, the used governor is userspace, then?
>>>
>>> No, we developed a new governor named as "pid governor", it manages
>>> thermals based on output values of PID controller.
> 
> Have you tried the patch set from Javi? the power allocator schem 
> includes a PID controller governor. Might be worth having a common
> solution. I think it is easier if I merge his changes into a branch and
> you have try on it. His patch series have already gone through several
> reviews. But you are free to comment too.

Oh, it's great, could you show me link of the patch set, so that I can
try it.

> 
>>>
>>> Indeed, our thermal management includes skin-temp, soc-thermal,
>>> cpu-throttle driver and pid governor. This pach set is prepared for the
>>> skin-temp driver.
>>> As you know the tegra soc-therm is in upstreaming, and we will post
>>> other drivers step by step.
>>
>> Will you take this patch, if so i will post skin-temp driver later.
>> Looking forward your comments :)
>>
> 
> I am afraid moving the maps and trips properties to optional abuses the
> purpose of the thermal DT. The primary goal is to describe hardware, and
> in this specific case, the hardware proper thermal operating conditions.
> 
> DT has to be as agnostic to OS implementation as possible.
> 
> For the above reason, I do not think this patch is a good idea.

Yes, I understand what you are worrying about, but it seems it's
difficult to handle this specific case in current framework or even
develop a new framework. How about to still set trips and maps as
required properties, but just add description something like "If missing
it, the thermal zone only has basic function such as reading temperature"

Thanks.
Wei.

> 
> 
> Cheers,
> 
>> Thanks.
>> Wei.
>>
>>>
>>> Thanks.
>>> Wei.
>>>
>>>>
>>>>> Thanks.
>>>>> Wei.
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>              trips {
>>>>>>>              };
>>>>>>>              cooling-maps {
>>>>>>>              };
>>>>>>>      };
>>>>>>> };
>>>>>>>
>>>>>>> Wei.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Wei.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The purpose of a thermal zone is to describe thermal behavior of a
>>>>>>>>>> hardware. As it is mentioned in the thermal.txt file.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>> Wei.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> -Optional property:
>>>>>>>>>>>>>  - coefficients:               An array of integers (one signed cell) containing
>>>>>>>>>>>>>    Type: array         coefficients to compose a linear relation between
>>>>>>>>>>>>>    Elem size: one cell the sensors listed in the thermal-sensors property.
>>>>>>>>>>>>> --
>>>>>>>>>>>>> 1.8.1.5
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Eduardo Valentin]

Hello Wei,

On Wed, Sep 10, 2014 at 04:14:02PM +0800, Wei Ni wrote:

<big cut>

> > 
> > Have you tried the patch set from Javi? the power allocator schem 
> > includes a PID controller governor. Might be worth having a common
> > solution. I think it is easier if I merge his changes into a branch and
> > you have try on it. His patch series have already gone through several
> > reviews. But you are free to comment too.
> 
> Oh, it's great, could you show me link of the patch set, so that I can
> try it.

Sure. Version 5 was posted several weeks ago in here:
https://lkml.org/lkml/2014/7/10/366

Also, I have merged it into a branch in my personal tree here:
https://git.kernel.org/cgit/linux/kernel/git/evalenti/linux.git/log/?h=thermal_work/thermal_core/power_allocator


> 
> > 
> >>>
> >>> Indeed, our thermal management includes skin-temp, soc-thermal,
> >>> cpu-throttle driver and pid governor. This pach set is prepared for the
> >>> skin-temp driver.
> >>> As you know the tegra soc-therm is in upstreaming, and we will post
> >>> other drivers step by step.
> >>
> >> Will you take this patch, if so i will post skin-temp driver later.
> >> Looking forward your comments :)
> >>
> > 
> > I am afraid moving the maps and trips properties to optional abuses the
> > purpose of the thermal DT. The primary goal is to describe hardware, and
> > in this specific case, the hardware proper thermal operating conditions.
> > 
> > DT has to be as agnostic to OS implementation as possible.
> > 
> > For the above reason, I do not think this patch is a good idea.
> 
> Yes, I understand what you are worrying about, but it seems it's
> difficult to handle this specific case in current framework or even
> develop a new framework. How about to still set trips and maps as
> required properties, but just add description something like "If missing
> it, the thermal zone only has basic function such as reading temperature"

I believe the right way of compsing a skin temperature zone is already
documented in the thermal.txt DT descriptor. You must add several
sensors into a zone. And not add several thermal zones that are used
only for monitoring.

The DT part description needs to focus on the hardware description. If
you see something is missing in the Linux kernel implementation, let me
know, and we can improve it.

Cheers,

> 
> Thanks.
> Wei.
> 
> > 
> > 
> > Cheers,
> > 
> >> Thanks.
> >> Wei.
> >>
> >>>
> >>> Thanks.
> >>> Wei.
> >>>
> >>>>
> >>>>> Thanks.
> >>>>> Wei.
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>              trips {
> >>>>>>>              };
> >>>>>>>              cooling-maps {
> >>>>>>>              };
> >>>>>>>      };
> >>>>>>> };
> >>>>>>>
> >>>>>>> Wei.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Wei.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> The purpose of a thermal zone is to describe thermal behavior of a
> >>>>>>>>>> hardware. As it is mentioned in the thermal.txt file.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks.
> >>>>>>>>>>> Wei.
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> -Optional property:
> >>>>>>>>>>>>>  - coefficients:               An array of integers (one signed cell) containing
> >>>>>>>>>>>>>    Type: array         coefficients to compose a linear relation between
> >>>>>>>>>>>>>    Elem size: one cell the sensors listed in the thermal-sensors property.
> >>>>>>>>>>>>> --
> >>>>>>>>>>>>> 1.8.1.5
> >>>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> >>>>>>>>>> the body of a message to majordomo@vger.kernel.org
> >>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>
> >>
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> Hello Wei Ni,
> 
> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>> Add more description for the "polling-delay" property.
>> Set "trips" and "cooling maps" as optional property, because
>> if missing these two sub-nodes, the thermal zone device still
>> work properly.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>> index f5db6b7..e3d3ed9 100644
>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>  
>>  Required properties:
>>  - polling-delay:	The maximum number of milliseconds to wait between polls
>> -  Type: unsigned	when checking this thermal zone.
>> -  Size: one cell
>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
>> +  Size: one cell	driver will not run polling queue, but just cancel it.
>>  
> 
> The description above is specific to Linux kernel implementation
> nomenclature. DT description needs to be OS agnostic.

Normally, the user may think a delay of 0 means that software
continually polls this zone, but in here it mean no polling. May be it's
better to add this description.

> 
>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>    Type: unsigned	between polls when performing passive cooling.
>> @@ -148,14 +148,16 @@ Required properties:
>>    phandles + sensor
>>    specifier
>>  
>> +Optional property:
>>  - trips:		A sub-node which is a container of only trip point nodes
>>    Type: sub-node	required to describe the thermal zone.
>>  
>>  - cooling-maps:		A sub-node which is a container of only cooling device
>>    Type: sub-node	map nodes, used to describe the relation between trips
>> -			and cooling devices.
>> +			and cooling devices. If missing the "trips" property,
>> +			This sub-node will not be parsed, because no trips can
>> +			be bound to cooling devices.
> 
> Do you mean if the thermal zone misses the "trips" property? Actually,
> the binding describes both, cooling-maps and trips, as required
> properties. Thus, both needs to be in place to consider the thermal zone
> as a proper described zone.
> 
>>  
>> -Optional property:
>>  - coefficients:		An array of integers (one signed cell) containing
>>    Type: array		coefficients to compose a linear relation between
>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
>> -- 
>> 1.8.1.5
>>

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Eduardo Valentin]

Hi,

On Tue, Aug 26, 2014 at 11:03:21AM +0800, Wei Ni wrote:
> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
> > Hello Wei Ni,
> > 
> > On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
> >> Add more description for the "polling-delay" property.
> >> Set "trips" and "cooling maps" as optional property, because
> >> if missing these two sub-nodes, the thermal zone device still
> >> work properly.
> >>
> >> Signed-off-by: Wei Ni <wni@nvidia.com>
> >> ---
> >>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> index f5db6b7..e3d3ed9 100644
> >> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> >> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> >> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
> >>  
> >>  Required properties:
> >>  - polling-delay:	The maximum number of milliseconds to wait between polls
> >> -  Type: unsigned	when checking this thermal zone.
> >> -  Size: one cell
> >> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
> >> +  Size: one cell	driver will not run polling queue, but just cancel it.
> >>  
> > 
> > The description above is specific to Linux kernel implementation
> > nomenclature. DT description needs to be OS agnostic.
> 
> Normally, the user may think a delay of 0 means that software
> continually polls this zone, but in here it mean no polling. May be it's
> better to add this description.
> 

Sure, I am fine adding it. Just please do not use Linux specific
nomenclature.

> > 
> >>  - polling-delay-passive: The maximum number of milliseconds to wait
> >>    Type: unsigned	between polls when performing passive cooling.
> >> @@ -148,14 +148,16 @@ Required properties:
> >>    phandles + sensor
> >>    specifier
> >>  
> >> +Optional property:
> >>  - trips:		A sub-node which is a container of only trip point nodes
> >>    Type: sub-node	required to describe the thermal zone.
> >>  
> >>  - cooling-maps:		A sub-node which is a container of only cooling device
> >>    Type: sub-node	map nodes, used to describe the relation between trips
> >> -			and cooling devices.
> >> +			and cooling devices. If missing the "trips" property,
> >> +			This sub-node will not be parsed, because no trips can
> >> +			be bound to cooling devices.
> > 
> > Do you mean if the thermal zone misses the "trips" property? Actually,
> > the binding describes both, cooling-maps and trips, as required
> > properties. Thus, both needs to be in place to consider the thermal zone
> > as a proper described zone.
> > 
> >>  
> >> -Optional property:
> >>  - coefficients:		An array of integers (one signed cell) containing
> >>    Type: array		coefficients to compose a linear relation between
> >>    Elem size: one cell	the sensors listed in the thermal-sensors property.
> >> -- 
> >> 1.8.1.5
> >>
> 

Re: [PATCH v3 3/4] thermal: add more description for thermal-zones

[Wei Ni]

On 08/26/2014 08:08 PM, Eduardo Valentin wrote:
> Hi,
> 
> On Tue, Aug 26, 2014 at 11:03:21AM +0800, Wei Ni wrote:
>> On 08/25/2014 07:07 PM, Eduardo Valentin wrote:
>>> Hello Wei Ni,
>>>
>>> On Mon, Aug 25, 2014 at 02:29:47PM +0800, Wei Ni wrote:
>>>> Add more description for the "polling-delay" property.
>>>> Set "trips" and "cooling maps" as optional property, because
>>>> if missing these two sub-nodes, the thermal zone device still
>>>> work properly.
>>>>
>>>> Signed-off-by: Wei Ni <wni@nvidia.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/thermal/thermal.txt | 10 ++++++----
>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>> index f5db6b7..e3d3ed9 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>>>> @@ -136,8 +136,8 @@ containing trip nodes and one sub-node containing all the zone cooling maps.
>>>>  
>>>>  Required properties:
>>>>  - polling-delay:	The maximum number of milliseconds to wait between polls
>>>> -  Type: unsigned	when checking this thermal zone.
>>>> -  Size: one cell
>>>> +  Type: unsigned	when checking this thermal zone. If this value is 0, the
>>>> +  Size: one cell	driver will not run polling queue, but just cancel it.
>>>>  
>>>
>>> The description above is specific to Linux kernel implementation
>>> nomenclature. DT description needs to be OS agnostic.
>>
>> Normally, the user may think a delay of 0 means that software
>> continually polls this zone, but in here it mean no polling. May be it's
>> better to add this description.
>>
> 
> Sure, I am fine adding it. Just please do not use Linux specific
> nomenclature.

Yes, you are right, I will just add "Cancel the polling if the delay is 0".

Wei.

> 
>>>
>>>>  - polling-delay-passive: The maximum number of milliseconds to wait
>>>>    Type: unsigned	between polls when performing passive cooling.
>>>> @@ -148,14 +148,16 @@ Required properties:
>>>>    phandles + sensor
>>>>    specifier
>>>>  
>>>> +Optional property:
>>>>  - trips:		A sub-node which is a container of only trip point nodes
>>>>    Type: sub-node	required to describe the thermal zone.
>>>>  
>>>>  - cooling-maps:		A sub-node which is a container of only cooling device
>>>>    Type: sub-node	map nodes, used to describe the relation between trips
>>>> -			and cooling devices.
>>>> +			and cooling devices. If missing the "trips" property,
>>>> +			This sub-node will not be parsed, because no trips can
>>>> +			be bound to cooling devices.
>>>
>>> Do you mean if the thermal zone misses the "trips" property? Actually,
>>> the binding describes both, cooling-maps and trips, as required
>>> properties. Thus, both needs to be in place to consider the thermal zone
>>> as a proper described zone.
>>>
>>>>  
>>>> -Optional property:
>>>>  - coefficients:		An array of integers (one signed cell) containing
>>>>    Type: array		coefficients to compose a linear relation between
>>>>    Elem size: one cell	the sensors listed in the thermal-sensors property.
>>>> -- 
>>>> 1.8.1.5
>>>>
>>

[PATCH v3 4/4] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

From: lightning314 <wni@nvidia.com>

Add dt node to describe the thermal zone for the nct1008.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 arch/arm/boot/dts/tegra114-dalmore.dts | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
index 5c21d21..94a1b5d 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -779,12 +779,14 @@
 				<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
 		};
 
-		temperature-sensor@4c {
+		nct1008: temperature-sensor@4c {
 			compatible = "onnn,nct1008";
 			reg = <0x4c>;
 			vcc-supply = <&palmas_ldo6_reg>;
 			interrupt-parent = <&gpio>;
 			interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
+
+			#thermal-sensor-cells = <1>;
 		};
 	};
 
@@ -1283,4 +1285,20 @@
 			 <&tegra_car TEGRA114_CLK_EXTERN1>;
 		clock-names = "pll_a", "pll_a_out0", "mclk";
 	};
+
+	thermal-zones {
+		nct1008-local {
+			polling-delay-passive = <2000>; /* milliseconds */
+			polling-delay = <0>; /* milliseconds */
+
+			thermal-sensors = <&nct1008 0>;
+		};
+
+		nct1008-remote {
+			polling-delay-passive = <1000>; /* milliseconds */
+			polling-delay = <0>; /* milliseconds */
+
+			thermal-sensors = <&nct1008 1>;
+		};
+	};
 };
-- 
1.8.1.5

Re: [PATCH v3 4/4] ARM: tegra: dalmore: add thermal zones for nct1008

[Eduardo Valentin]

On Mon, Aug 25, 2014 at 02:29:48PM +0800, Wei Ni wrote:
> From: lightning314 <wni@nvidia.com>
> 
> Add dt node to describe the thermal zone for the nct1008.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra114-dalmore.dts | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> index 5c21d21..94a1b5d 100644
> --- a/arch/arm/boot/dts/tegra114-dalmore.dts
> +++ b/arch/arm/boot/dts/tegra114-dalmore.dts
> @@ -779,12 +779,14 @@
>  				<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
>  		};
>  
> -		temperature-sensor@4c {
> +		nct1008: temperature-sensor@4c {
>  			compatible = "onnn,nct1008";
>  			reg = <0x4c>;
>  			vcc-supply = <&palmas_ldo6_reg>;
>  			interrupt-parent = <&gpio>;
>  			interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
> +
> +			#thermal-sensor-cells = <1>;
>  		};
>  	};
>  
> @@ -1283,4 +1285,20 @@
>  			 <&tegra_car TEGRA114_CLK_EXTERN1>;
>  		clock-names = "pll_a", "pll_a_out0", "mclk";
>  	};
> +
> +	thermal-zones {
> +		nct1008-local {
> +			polling-delay-passive = <2000>; /* milliseconds */
> +			polling-delay = <0>; /* milliseconds */
> +
> +			thermal-sensors = <&nct1008 0>;
> +		};
> +
> +		nct1008-remote {
> +			polling-delay-passive = <1000>; /* milliseconds */
> +			polling-delay = <0>; /* milliseconds */
> +
> +			thermal-sensors = <&nct1008 1>;
> +		};
> +	};

The above zones misses the required properties, as per the thermal.txt
binding description. Could you please have a look on those that are
required and improve the zones above?

>  };
> -- 
> 1.8.1.5
> 

Re: [PATCH v3 4/4] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

On 08/25/2014 07:08 PM, Eduardo Valentin wrote:
> On Mon, Aug 25, 2014 at 02:29:48PM +0800, Wei Ni wrote:
>> From: lightning314 <wni@nvidia.com>
>>
>> Add dt node to describe the thermal zone for the nct1008.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  arch/arm/boot/dts/tegra114-dalmore.dts | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>> index 5c21d21..94a1b5d 100644
>> --- a/arch/arm/boot/dts/tegra114-dalmore.dts
>> +++ b/arch/arm/boot/dts/tegra114-dalmore.dts
>> @@ -779,12 +779,14 @@
>>  				<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
>>  		};
>>  
>> -		temperature-sensor@4c {
>> +		nct1008: temperature-sensor@4c {
>>  			compatible = "onnn,nct1008";
>>  			reg = <0x4c>;
>>  			vcc-supply = <&palmas_ldo6_reg>;
>>  			interrupt-parent = <&gpio>;
>>  			interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
>> +
>> +			#thermal-sensor-cells = <1>;
>>  		};
>>  	};
>>  
>> @@ -1283,4 +1285,20 @@
>>  			 <&tegra_car TEGRA114_CLK_EXTERN1>;
>>  		clock-names = "pll_a", "pll_a_out0", "mclk";
>>  	};
>> +
>> +	thermal-zones {
>> +		nct1008-local {
>> +			polling-delay-passive = <2000>; /* milliseconds */
>> +			polling-delay = <0>; /* milliseconds */
>> +
>> +			thermal-sensors = <&nct1008 0>;
>> +		};
>> +
>> +		nct1008-remote {
>> +			polling-delay-passive = <1000>; /* milliseconds */
>> +			polling-delay = <0>; /* milliseconds */
>> +
>> +			thermal-sensors = <&nct1008 1>;
>> +		};
>> +	};
> 
> The above zones misses the required properties, as per the thermal.txt
> binding description. Could you please have a look on those that are
> required and improve the zones above?

I changed the thermal.txt in the patch 3/4, to move these two properties
to optional property.
On the Dalmore, we just need to register these two sensors as thermal
zone devices.
Indeed, we have a skin-temperature driver, which used these two thermal
zone deives' temperature to estimator the skin temperature, so we
doesn't need to set trips and bind with any cooling devices on them.

Thanks.
Wei.

> 
>>  };
>> -- 
>> 1.8.1.5
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 4/4] ARM: tegra: dalmore: add thermal zones for nct1008

[Eduardo Valentin]

On Mon, Aug 25, 2014 at 02:29:48PM +0800, Wei Ni wrote:
> From: lightning314 <wni@nvidia.com>
> 
> Add dt node to describe the thermal zone for the nct1008.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra114-dalmore.dts | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> index 5c21d21..94a1b5d 100644
> --- a/arch/arm/boot/dts/tegra114-dalmore.dts
> +++ b/arch/arm/boot/dts/tegra114-dalmore.dts
> @@ -779,12 +779,14 @@
>  				<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
>  		};
>  
> -		temperature-sensor@4c {
> +		nct1008: temperature-sensor@4c {
>  			compatible = "onnn,nct1008";
>  			reg = <0x4c>;
>  			vcc-supply = <&palmas_ldo6_reg>;
>  			interrupt-parent = <&gpio>;
>  			interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
> +
> +			#thermal-sensor-cells = <1>;
>  		};
>  	};
>  
> @@ -1283,4 +1285,20 @@
>  			 <&tegra_car TEGRA114_CLK_EXTERN1>;
>  		clock-names = "pll_a", "pll_a_out0", "mclk";
>  	};
> +
> +	thermal-zones {
> +		nct1008-local {

at this level, can the thermal zone name be a meaningful string? What
part of the hardware does the local and remote monitors the the dalmore
platform?

> +			polling-delay-passive = <2000>; /* milliseconds */
> +			polling-delay = <0>; /* milliseconds */
> +
> +			thermal-sensors = <&nct1008 0>;
> +		};
> +
> +		nct1008-remote {
> +			polling-delay-passive = <1000>; /* milliseconds */
> +			polling-delay = <0>; /* milliseconds */
> +
> +			thermal-sensors = <&nct1008 1>;
> +		};
> +	};
>  };
> -- 
> 1.8.1.5
> 

Re: [PATCH v3 4/4] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

On 08/25/2014 07:10 PM, Eduardo Valentin wrote:
> On Mon, Aug 25, 2014 at 02:29:48PM +0800, Wei Ni wrote:
>> From: lightning314 <wni@nvidia.com>
>>
>> Add dt node to describe the thermal zone for the nct1008.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  arch/arm/boot/dts/tegra114-dalmore.dts | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>> index 5c21d21..94a1b5d 100644
>> --- a/arch/arm/boot/dts/tegra114-dalmore.dts
>> +++ b/arch/arm/boot/dts/tegra114-dalmore.dts
>> @@ -779,12 +779,14 @@
>>  				<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
>>  		};
>>  
>> -		temperature-sensor@4c {
>> +		nct1008: temperature-sensor@4c {
>>  			compatible = "onnn,nct1008";
>>  			reg = <0x4c>;
>>  			vcc-supply = <&palmas_ldo6_reg>;
>>  			interrupt-parent = <&gpio>;
>>  			interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
>> +
>> +			#thermal-sensor-cells = <1>;
>>  		};
>>  	};
>>  
>> @@ -1283,4 +1285,20 @@
>>  			 <&tegra_car TEGRA114_CLK_EXTERN1>;
>>  		clock-names = "pll_a", "pll_a_out0", "mclk";
>>  	};
>> +
>> +	thermal-zones {
>> +		nct1008-local {
> 
> at this level, can the thermal zone name be a meaningful string? What
> part of the hardware does the local and remote monitors the the dalmore
> platform?

Oh, yes, you are right.
I think it's better to use "board_thermal_sensor: nct1008-local" and
"diode_thermal_sensor: nct1008-remote".

Thanks.
Wei.

> 
>> +			polling-delay-passive = <2000>; /* milliseconds */
>> +			polling-delay = <0>; /* milliseconds */
>> +
>> +			thermal-sensors = <&nct1008 0>;
>> +		};
>> +
>> +		nct1008-remote {
>> +			polling-delay-passive = <1000>; /* milliseconds */
>> +			polling-delay = <0>; /* milliseconds */
>> +
>> +			thermal-sensors = <&nct1008 1>;
>> +		};
>> +	};
>>  };
>> -- 
>> 1.8.1.5
>>