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

[PATCH v3 0/4] Lm90 Enhancements

[Wei Ni]

This patch set enhance the lm90 driver,
it make the driver more readable and easier to use thermal framework.

This series is v2, previous version patches:
[RFC]: http://thread.gmane.org/gmane.linux.power-management.general/31056
[v1]: http://thread.gmane.org/gmane.linux.ports.tegra/11710/
[v2]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg465555.html

Changes from v2:
1. update the defines for status bit, and go into a separate patch.
2. introduce the new lm90_is_tripped() for lm90_irq_thread and lm90_alert.

Changes from v1:
1. change the string "irq" to "IRQ"
2. add macro defines for the alarm status
3. consider the shared IRQ.

Changes from RFC:
1. change _show_temp() to read_temp(), _set_temp() to write_temp().
2. simply return value for the read_temp(), not use pointer.
3. use devm_request_threaded_irq() to request irq and set flag IRQF_ONESHOT.

Wei Ni (4):
  hwmon: (lm90) split set&show temp as common codes
  hwmon: (lm90) use macro defines for the status bit
  hwmon: (lm90) add support to handle IRQ
  hwmon: (lm90) use enums for the indexes of temp8 and temp11

 drivers/hwmon/lm90.c |  387 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 256 insertions(+), 131 deletions(-)

-- 
1.7.9.5

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

[Wei Ni]

Split set&show temp codes as common functions, so we can use it directly when
implement linux thermal framework.

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 8eeb141..5f30f90 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -702,29 +702,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)
-		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 void write_temp8(struct device *dev, int index, long val)
 {
 	static const u8 reg[8] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -737,60 +742,73 @@ 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 i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(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->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);
+		data->temp8[index] = 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, index >= 6);
+	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+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;
+
+	write_temp8(dev, index, val);
+
 	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)
-		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 void write_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
 		u8 high;
@@ -804,17 +822,8 @@ 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 i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(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)
@@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+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;
+
+	write_temp11(dev, nr, index, val);
+
 	return count;
 }
 
-- 
1.7.9.5

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

[Jean Delvare]

Hi Wei, Guenter,

Guenter, thanks for reviewing the previous version of this patch.

Wei, thanks for incorporating review feedback and posting updated
patches so quickly, this is very appreciated, even though I'm too busy
these days to be that fast on my end, sorry about that.

On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote:
> Split set&show temp codes as common functions, so we can use it directly when
> implement linux thermal framework.

Can I see a recent version of the code which will need this change? It
makes no sense to apply this first part which makes the code more
complex with no benefit, without the second part which needs it, so
they should be applied together or not at all.

One thing I am a little worried about (but maybe I'm wrong) is that I
seem to understand you want to register every LM90-like chip as both a
hwmon device and two thermal devices. I seem to recall that every
thermal device is also exposed automatically as a virtual hwmon
device, is that correct? If so we will be presenting the same values
twice to libsensors, which would be confusing.

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

The code changes look good, however I have one suggestion for
improvement (plus a minor cleanup request):

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 8eeb141..5f30f90 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> (...)
> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> (...)
> +static void write_temp8(struct device *dev, int index, long val)
>  {
>  	static const u8 reg[8] = {
>  		LM90_REG_W_LOCAL_LOW,
> @@ -737,60 +742,73 @@ 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 i2c_client *client = to_i2c_client(dev);
>  	struct lm90_data *data = i2c_get_clientdata(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->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);
> +		data->temp8[index] = 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, index >= 6);
> +	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);

This write could fail. So far the lm90 driver has failed to handle
register write errors at all, and I will take the blame for that. But
if we want to integrate properly with the thermal subsystem, I suspect
we will have to properly report errors. So it might be the right time
to catch and return write errors here. Then set_temp8() below could
return it to user-space (either in this patch or in a separate patch,
as you prefer.)

And then as a next step, lm90_select_remote_channel should return
errors as they happen as well, so that they can be transmitted to the
caller.

>  	lm90_select_remote_channel(client, data, 0);
>  
>  	mutex_unlock(&data->update_lock);
> +}
> +
> +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;
> +
> +	write_temp8(dev, index, val);
> +
>  	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)
> -		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)

There's a doubled space on this line. It isn't added by your patch, it
was already there before, but please fix it while you're here.

>  		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 void write_temp11(struct device *dev, int nr, int index, long val)

Here too I would suggest returning errors from the I2C layer, and
handling them in set_temp11() now or later.

>  {
>  	struct {
>  		u8 high;
> @@ -804,17 +822,8 @@ 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 i2c_client *client = to_i2c_client(dev);
>  	struct lm90_data *data = i2c_get_clientdata(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)
> @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  	lm90_select_remote_channel(client, data, 0);
>  
>  	mutex_unlock(&data->update_lock);
> +}
> +
> +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;
> +
> +	write_temp11(dev, nr, index, val);
> +
>  	return count;
>  }
>  

-- 
Jean Delvare

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

[Guenter Roeck]

On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
> Hi Wei, Guenter,
> 
> Guenter, thanks for reviewing the previous version of this patch.
> 
> Wei, thanks for incorporating review feedback and posting updated
> patches so quickly, this is very appreciated, even though I'm too busy
> these days to be that fast on my end, sorry about that.
> 
> On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote:
> > Split set&show temp codes as common functions, so we can use it directly when
> > implement linux thermal framework.
> 
> Can I see a recent version of the code which will need this change? It
> makes no sense to apply this first part which makes the code more
> complex with no benefit, without the second part which needs it, so
> they should be applied together or not at all.
> 
> One thing I am a little worried about (but maybe I'm wrong) is that I
> seem to understand you want to register every LM90-like chip as both a
> hwmon device and two thermal devices. I seem to recall that every
> thermal device is also exposed automatically as a virtual hwmon
> device, is that correct? If so we will be presenting the same values
> twice to libsensors, which would be confusing.
> 
Not sure if that is a good idea, but if I recall correctly, the thermal folks
plan to remove that path.

Guenter

> > Signed-off-by: Wei Ni <wni@nvidia.com>
> > ---
> >  drivers/hwmon/lm90.c |  112 +++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 69 insertions(+), 43 deletions(-)
> 
> The code changes look good, however I have one suggestion for
> improvement (plus a minor cleanup request):
> 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 8eeb141..5f30f90 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > (...)
> > -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > -			 const char *buf, size_t count)
> > (...)
> > +static void write_temp8(struct device *dev, int index, long val)
> >  {
> >  	static const u8 reg[8] = {
> >  		LM90_REG_W_LOCAL_LOW,
> > @@ -737,60 +742,73 @@ 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 i2c_client *client = to_i2c_client(dev);
> >  	struct lm90_data *data = i2c_get_clientdata(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->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);
> > +		data->temp8[index] = 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, index >= 6);
> > +	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
> 
> This write could fail. So far the lm90 driver has failed to handle
> register write errors at all, and I will take the blame for that. But
> if we want to integrate properly with the thermal subsystem, I suspect
> we will have to properly report errors. So it might be the right time
> to catch and return write errors here. Then set_temp8() below could
> return it to user-space (either in this patch or in a separate patch,
> as you prefer.)
> 
> And then as a next step, lm90_select_remote_channel should return
> errors as they happen as well, so that they can be transmitted to the
> caller.
> 
> >  	lm90_select_remote_channel(client, data, 0);
> >  
> >  	mutex_unlock(&data->update_lock);
> > +}
> > +
> > +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;
> > +
> > +	write_temp8(dev, index, val);
> > +
> >  	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)
> > -		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)
> 
> There's a doubled space on this line. It isn't added by your patch, it
> was already there before, but please fix it while you're here.
> 
> >  		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 void write_temp11(struct device *dev, int nr, int index, long val)
> 
> Here too I would suggest returning errors from the I2C layer, and
> handling them in set_temp11() now or later.
> 
> >  {
> >  	struct {
> >  		u8 high;
> > @@ -804,17 +822,8 @@ 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 i2c_client *client = to_i2c_client(dev);
> >  	struct lm90_data *data = i2c_get_clientdata(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)
> > @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> >  	lm90_select_remote_channel(client, data, 0);
> >  
> >  	mutex_unlock(&data->update_lock);
> > +}
> > +
> > +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;
> > +
> > +	write_temp11(dev, nr, index, val);
> > +
> >  	return count;
> >  }
> >  
> 
> -- 
> Jean Delvare
> 

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

[Jean Delvare]

Hi Guenter,

On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote:
> On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
> > One thing I am a little worried about (but maybe I'm wrong) is that I
> > seem to understand you want to register every LM90-like chip as both a
> > hwmon device and two thermal devices. I seem to recall that every
> > thermal device is also exposed automatically as a virtual hwmon
> > device, is that correct? If so we will be presenting the same values
> > twice to libsensors, which would be confusing.
> 
> Not sure if that is a good idea, but if I recall correctly, the thermal folks
> plan to remove that path.

If that means that for example the ACPI thermal zone is no longer
displayed by "sensors", then I strongly object - unless it is
explicitly registered as a separate hwmon device from now on, of course.

My idea was to make the bridge optional - you decide when you register
a thermal device if it should be exposed as hwmon or not.

I don't have a strong opinion on the implementation, as long as each
input is listed by "sensors" once and only once.

-- 
Jean Delvare

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

[Guenter Roeck]

On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> Hi Guenter,
> 
> On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote:
> > On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
> > > One thing I am a little worried about (but maybe I'm wrong) is that I
> > > seem to understand you want to register every LM90-like chip as both a
> > > hwmon device and two thermal devices. I seem to recall that every
> > > thermal device is also exposed automatically as a virtual hwmon
> > > device, is that correct? If so we will be presenting the same values
> > > twice to libsensors, which would be confusing.
> > 
> > Not sure if that is a good idea, but if I recall correctly, the thermal folks
> > plan to remove that path.
> 
> If that means that for example the ACPI thermal zone is no longer
> displayed by "sensors", then I strongly object - unless it is
> explicitly registered as a separate hwmon device from now on, of course.
>
If I recall correctly that was the idea. Of course, in practice that will mean
that devices will _not_ get exposed as hwmon devices, as implementers won't
bother doing both.

> My idea was to make the bridge optional - you decide when you register
> a thermal device if it should be exposed as hwmon or not.
> 
Yes, that would be a much better solution.

> I don't have a strong opinion on the implementation, as long as each
> input is listed by "sensors" once and only once.
> 
Agreed.

Guenter

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

[Wei Ni]

On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>> Hi Guenter,
>>
>> On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote:
>>> On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
>>>> One thing I am a little worried about (but maybe I'm wrong) is that I
>>>> seem to understand you want to register every LM90-like chip as both a
>>>> hwmon device and two thermal devices. I seem to recall that every
>>>> thermal device is also exposed automatically as a virtual hwmon
>>>> device, is that correct? If so we will be presenting the same values
>>>> twice to libsensors, which would be confusing.
>>>
>>> Not sure if that is a good idea, but if I recall correctly, the thermal folks
>>> plan to remove that path.

Hi, Rui
As Jean said, if we want to register the lm90 as thermal device, it will
have two hwmon devices in the sysfs, one is registered by the lm90
driver, another one is registered by the thermal_zone_device_register(),
this would be confusing.

Do you have any ideas for it?

Thanks.
Wei.

>>
>> If that means that for example the ACPI thermal zone is no longer
>> displayed by "sensors", then I strongly object - unless it is
>> explicitly registered as a separate hwmon device from now on, of course.
>>
> If I recall correctly that was the idea. Of course, in practice that will mean
> that devices will _not_ get exposed as hwmon devices, as implementers won't
> bother doing both.
> 
>> My idea was to make the bridge optional - you decide when you register
>> a thermal device if it should be exposed as hwmon or not.
>>
> Yes, that would be a much better solution.

I think we can decide it in the DT, we can add a dt property in the lm90
device node, such as:
sys-interface = SYS_HWMON;
or
sys-interface = SYS_THERMAL;
So we register it as the hwmon or thermal device

> 
>> I don't have a strong opinion on the implementation, as long as each
>> input is listed by "sensors" once and only once.
>>
> Agreed.
> 
> Guenter
> 

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

[Jean Delvare]

On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> >> If that means that for example the ACPI thermal zone is no longer
> >> displayed by "sensors", then I strongly object - unless it is
> >> explicitly registered as a separate hwmon device from now on, of course.
> >
> > If I recall correctly that was the idea. Of course, in practice that will mean
> > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > bother doing both.
> > 
> >> My idea was to make the bridge optional - you decide when you register
> >> a thermal device if it should be exposed as hwmon or not.
> >
> > Yes, that would be a much better solution.
> 
> I think we can decide it in the DT, we can add a dt property in the lm90
> device node, such as:
> sys-interface = SYS_HWMON;
> or
> sys-interface = SYS_THERMAL;
> So we register it as the hwmon or thermal device

This is an option, but please keep in mind that DT is not the only way
to instantiate LM90-like devices, and we should not expose duplicate
inputs by default. It is fine with me if the default is to create only a
HWMON device (as the lm90 driver was doing so far), and only
DT-instantiated devices have the choice.

Another option, as discussed before, would be that the thermal devices
registered by lm90 are specifically tagged as "do not expose as hwmon".
This would avoid the duplicate hwmon inputs in all cases.

Again - no strong opinion on the implementation, as long as it does the
right thing.

Oh, and we'll have to be careful with the Kconfig dependencies. I do
not want the lm90 driver to depend on the thermal framework.

-- 
Jean Delvare

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

[Wei Ni]

On 07/15/2013 03:24 PM, Jean Delvare wrote:
> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
>>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>>>> If that means that for example the ACPI thermal zone is no longer
>>>> displayed by "sensors", then I strongly object - unless it is
>>>> explicitly registered as a separate hwmon device from now on, of course.
>>>
>>> If I recall correctly that was the idea. Of course, in practice that will mean
>>> that devices will _not_ get exposed as hwmon devices, as implementers won't
>>> bother doing both.
>>>
>>>> My idea was to make the bridge optional - you decide when you register
>>>> a thermal device if it should be exposed as hwmon or not.
>>>
>>> Yes, that would be a much better solution.
>>
>> I think we can decide it in the DT, we can add a dt property in the lm90
>> device node, such as:
>> sys-interface = SYS_HWMON;
>> or
>> sys-interface = SYS_THERMAL;
>> So we register it as the hwmon or thermal device
> 
> This is an option, but please keep in mind that DT is not the only way
> to instantiate LM90-like devices, and we should not expose duplicate
> inputs by default. It is fine with me if the default is to create only a
> HWMON device (as the lm90 driver was doing so far), and only
> DT-instantiated devices have the choice.

Yes, we should not expose duplicate inputs, we may have tree permutation:
1. only hwmon device:
for this items, we just need to call hwmon_device_register().
2. only thermal device + virtual hwmon device:
for this items, we just need to call thermal_zone_device_register().

We can set #1 as the default, and if use DT, we provide option to choice
#1 or #2.

3. hwmon device + thermal device:
for this items, we doesn't need the virtual hwmon which registered by
the thermal fw, how to handle this one? Add flag when register as
thermal device to indicate if want virtual hwmon device or not,
something like your below another option.

> 
> Another option, as discussed before, would be that the thermal devices
> registered by lm90 are specifically tagged as "do not expose as hwmon".
> This would avoid the duplicate hwmon inputs in all cases.

It seems in current thermal fw, it can't be tagged as "do not expose as
hwmon", we need to add flag when register thermal device.

Rui, what do you think for it?

> 
> Again - no strong opinion on the implementation, as long as it does the
> right thing.

I'm working on the Tegra platform, we uses nct1008 as the temperature
sensor, and we want to register it as thermal device, so that we can run
the throttle function. So I prepared these patches to enhance this driver.

> 
> Oh, and we'll have to be careful with the Kconfig dependencies. I do
> not want the lm90 driver to depend on the thermal framework.

Yes, absolutely agree, lm90 should be independent.
Indeed, the thermal folks is trying to restructure the thermal fw, and
in this new fw, it's more easy to add the generic sensors to the thermal
fw. If you interest it, please refer
http://thread.gmane.org/gmane.linux.power-management.general/30692.

Thanks.
Wei.

> 

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

[Jean Delvare]

Hi Wei,

On Mon, 15 Jul 2013 17:14:07 +0800, Wei Ni wrote:
> On 07/15/2013 03:24 PM, Jean Delvare wrote:
> > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> >> I think we can decide it in the DT, we can add a dt property in the lm90
> >> device node, such as:
> >> sys-interface = SYS_HWMON;
> >> or
> >> sys-interface = SYS_THERMAL;
> >> So we register it as the hwmon or thermal device
> > 
> > This is an option, but please keep in mind that DT is not the only way
> > to instantiate LM90-like devices, and we should not expose duplicate
> > inputs by default. It is fine with me if the default is to create only a
> > HWMON device (as the lm90 driver was doing so far), and only
> > DT-instantiated devices have the choice.
> 
> Yes, we should not expose duplicate inputs, we may have tree permutation:
> 1. only hwmon device:
> for this items, we just need to call hwmon_device_register().
> 2. only thermal device + virtual hwmon device:
> for this items, we just need to call thermal_zone_device_register().
> 
> We can set #1 as the default, and if use DT, we provide option to choice
> #1 or #2.

#2 makes little sense IMHO, for a driver which properly implements
hwmon support. The point of the virtual hwmon device created for
thermal zones was to not put an extra burden on thermal driver authors
by asking them to additionally implement the hwmon interface. But the
hwmon interface it richer than the thermal interface in some respects
so native hwmon implementations are preferred when available. Thus I
think your option #3 below is what we want in addition to #1, and we
don't need #2.

> 3. hwmon device + thermal device:
> for this items, we doesn't need the virtual hwmon which registered by
> the thermal fw, how to handle this one? Add flag when register as
> thermal device to indicate if want virtual hwmon device or not,
> something like your below another option.

-- 
Jean Delvare

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

[Thierry Reding]

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

On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> > On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> > >> If that means that for example the ACPI thermal zone is no longer
> > >> displayed by "sensors", then I strongly object - unless it is
> > >> explicitly registered as a separate hwmon device from now on, of course.
> > >
> > > If I recall correctly that was the idea. Of course, in practice that will mean
> > > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > > bother doing both.
> > > 
> > >> My idea was to make the bridge optional - you decide when you register
> > >> a thermal device if it should be exposed as hwmon or not.
> > >
> > > Yes, that would be a much better solution.
> > 
> > I think we can decide it in the DT, we can add a dt property in the lm90
> > device node, such as:
> > sys-interface = SYS_HWMON;
> > or
> > sys-interface = SYS_THERMAL;
> > So we register it as the hwmon or thermal device
> 
> This is an option, but please keep in mind that DT is not the only way
> to instantiate LM90-like devices, and we should not expose duplicate
> inputs by default. It is fine with me if the default is to create only a
> HWMON device (as the lm90 driver was doing so far), and only
> DT-instantiated devices have the choice.

I don't think this information belongs in the device tree. Whether the
device is exposed as hwmon or thermal device (or both) is entirely a
software issue whereas DT is a means to describe the hardware.

It seems to me that the earlier proposal of communicating to the bridge
whether or not a device should be exposed as hwmon device is the right
thing to do here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Guenter Roeck]

On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> > > On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> > > >> If that means that for example the ACPI thermal zone is no longer
> > > >> displayed by "sensors", then I strongly object - unless it is
> > > >> explicitly registered as a separate hwmon device from now on, of course.
> > > >
> > > > If I recall correctly that was the idea. Of course, in practice that will mean
> > > > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > > > bother doing both.
> > > > 
> > > >> My idea was to make the bridge optional - you decide when you register
> > > >> a thermal device if it should be exposed as hwmon or not.
> > > >
> > > > Yes, that would be a much better solution.
> > > 
> > > I think we can decide it in the DT, we can add a dt property in the lm90
> > > device node, such as:
> > > sys-interface = SYS_HWMON;
> > > or
> > > sys-interface = SYS_THERMAL;
> > > So we register it as the hwmon or thermal device
> > 
> > This is an option, but please keep in mind that DT is not the only way
> > to instantiate LM90-like devices, and we should not expose duplicate
> > inputs by default. It is fine with me if the default is to create only a
> > HWMON device (as the lm90 driver was doing so far), and only
> > DT-instantiated devices have the choice.
> 
> I don't think this information belongs in the device tree. Whether the
> device is exposed as hwmon or thermal device (or both) is entirely a
> software issue whereas DT is a means to describe the hardware.
> 
Correct; this is exactly the type of information which does _not_ belong int
devicetree.

> It seems to me that the earlier proposal of communicating to the bridge
> whether or not a device should be exposed as hwmon device is the right
> thing to do here.
> 
Agreed..

Guenter

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

[Wei Ni]

On 07/17/2013 01:14 PM, Guenter Roeck wrote:
> On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
>> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
>>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>>>> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
>>>>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>>>>>> If that means that for example the ACPI thermal zone is no longer
>>>>>> displayed by "sensors", then I strongly object - unless it is
>>>>>> explicitly registered as a separate hwmon device from now on, of course.
>>>>>
>>>>> If I recall correctly that was the idea. Of course, in practice that will mean
>>>>> that devices will _not_ get exposed as hwmon devices, as implementers won't
>>>>> bother doing both.
>>>>>
>>>>>> My idea was to make the bridge optional - you decide when you register
>>>>>> a thermal device if it should be exposed as hwmon or not.
>>>>>
>>>>> Yes, that would be a much better solution.
>>>>
>>>> I think we can decide it in the DT, we can add a dt property in the lm90
>>>> device node, such as:
>>>> sys-interface = SYS_HWMON;
>>>> or
>>>> sys-interface = SYS_THERMAL;
>>>> So we register it as the hwmon or thermal device
>>>
>>> This is an option, but please keep in mind that DT is not the only way
>>> to instantiate LM90-like devices, and we should not expose duplicate
>>> inputs by default. It is fine with me if the default is to create only a
>>> HWMON device (as the lm90 driver was doing so far), and only
>>> DT-instantiated devices have the choice.
>>
>> I don't think this information belongs in the device tree. Whether the
>> device is exposed as hwmon or thermal device (or both) is entirely a
>> software issue whereas DT is a means to describe the hardware.
>>
> Correct; this is exactly the type of information which does _not_ belong int
> devicetree.
> 
>> It seems to me that the earlier proposal of communicating to the bridge
>> whether or not a device should be exposed as hwmon device is the right
>> thing to do here.
>>
> Agreed..

Sorry, what's the "bridge" mean, does it mean we need to add a flag in
the thermal_zone_device_register() to indicate if we want to register
virtual hwmon device or not?

Thanks.
Wei.

> 
> Guenter
> 

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

[Jean Delvare]

On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote:
> On 07/17/2013 01:14 PM, Guenter Roeck wrote:
> > On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
> >> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> >>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> >>>> I think we can decide it in the DT, we can add a dt property in the lm90
> >>>> device node, such as:
> >>>> sys-interface = SYS_HWMON;
> >>>> or
> >>>> sys-interface = SYS_THERMAL;
> >>>> So we register it as the hwmon or thermal device
> >>>
> >>> This is an option, but please keep in mind that DT is not the only way
> >>> to instantiate LM90-like devices, and we should not expose duplicate
> >>> inputs by default. It is fine with me if the default is to create only a
> >>> HWMON device (as the lm90 driver was doing so far), and only
> >>> DT-instantiated devices have the choice.
> >>
> >> I don't think this information belongs in the device tree. Whether the
> >> device is exposed as hwmon or thermal device (or both) is entirely a
> >> software issue whereas DT is a means to describe the hardware.
> >>
> > Correct; this is exactly the type of information which does _not_ belong int
> > devicetree.
> > 
> >> It seems to me that the earlier proposal of communicating to the bridge
> >> whether or not a device should be exposed as hwmon device is the right
> >> thing to do here.
> >
> > Agreed..
> 
> Sorry, what's the "bridge" mean,

The code which creates a virtual hwmon input when a new thermal zone is
registered (this code is in thermal_core.c.)

> does it mean we need to add a flag in
> the thermal_zone_device_register() to indicate if we want to register
> virtual hwmon device or not?

I think so, yes.

Alternatively the flag could be added to struct
thermal_zone_device_ops, so that you don't have to update all the
callers. But I admit it's a hack as the flag doesn't really belong
there, so I suppose we don't really want to do that.

I have been thinking of an automatic approach, based on comparing the
type string passed to thermal_zone_device_register() with already
registered hwmon devices, but I couldn't come up with something good
and robust enough, so let's forget about it.

-- 
Jean Delvare

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

[Wei Ni]

On 07/17/2013 05:11 PM, Jean Delvare wrote:
> On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote:
>> On 07/17/2013 01:14 PM, Guenter Roeck wrote:
>>> On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
>>>> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
>>>>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>>>>>> I think we can decide it in the DT, we can add a dt property in the lm90
>>>>>> device node, such as:
>>>>>> sys-interface = SYS_HWMON;
>>>>>> or
>>>>>> sys-interface = SYS_THERMAL;
>>>>>> So we register it as the hwmon or thermal device
>>>>>
>>>>> This is an option, but please keep in mind that DT is not the only way
>>>>> to instantiate LM90-like devices, and we should not expose duplicate
>>>>> inputs by default. It is fine with me if the default is to create only a
>>>>> HWMON device (as the lm90 driver was doing so far), and only
>>>>> DT-instantiated devices have the choice.
>>>>
>>>> I don't think this information belongs in the device tree. Whether the
>>>> device is exposed as hwmon or thermal device (or both) is entirely a
>>>> software issue whereas DT is a means to describe the hardware.
>>>>
>>> Correct; this is exactly the type of information which does _not_ belong int
>>> devicetree.
>>>
>>>> It seems to me that the earlier proposal of communicating to the bridge
>>>> whether or not a device should be exposed as hwmon device is the right
>>>> thing to do here.
>>>
>>> Agreed..
>>
>> Sorry, what's the "bridge" mean,
> 
> The code which creates a virtual hwmon input when a new thermal zone is
> registered (this code is in thermal_core.c.)
> 
>> does it mean we need to add a flag in
>> the thermal_zone_device_register() to indicate if we want to register
>> virtual hwmon device or not?
> 
> I think so, yes.
> 
> Alternatively the flag could be added to struct
> thermal_zone_device_ops, so that you don't have to update all the
> callers. But I admit it's a hack as the flag doesn't really belong
> there, so I suppose we don't really want to do that.

I think it's better to add it to struct thermal_zone_params, the
thermal_zone_device_ops is for the callback functions.
And I ask it with thermal fw engineers in
http://thread.gmane.org/gmane.linux.power-management.general/35874.
May be you can look it.

> 
> I have been thinking of an automatic approach, based on comparing the
> type string passed to thermal_zone_device_register() with already
> registered hwmon devices, but I couldn't come up with something good
> and robust enough, so let's forget about it.
> 

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

[Wei Ni]

On 07/12/2013 09:26 PM, Jean Delvare wrote:
> Hi Wei, Guenter,
> 
> Guenter, thanks for reviewing the previous version of this patch.
> 
> Wei, thanks for incorporating review feedback and posting updated
> patches so quickly, this is very appreciated, even though I'm too busy
> these days to be that fast on my end, sorry about that.
> 
> On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote:
>> Split set&show temp codes as common functions, so we can use it directly when
>> implement linux thermal framework.
> 
> Can I see a recent version of the code which will need this change? It
> makes no sense to apply this first part which makes the code more
> complex with no benefit, without the second part which needs it, so
> they should be applied together or not at all.

In my RFC patches, there had many codes about thermal fw, which need
this patch, so I put them together.
And now I split the RFC patches, this series is preparing to use the
thermal fw.
As you said, I want to register lm90 as the thermal zone device, it need
to hook some callback, such as .get_temp. if apply this patch, I can
write the .get_temp simply, something like:

+static int lm90_read_temp2_temp(struct thermal_zone_device *thz,
unsigned long *temp)
+{
+        struct lm90_data *data = thz->devdata;
+        struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
+        struct device *dev = &client->dev;+
+
+        *temp = (long)read_temp11(dev, TEMP11_REMOTE_TEMP);
+
+        return 0;
+}
+
+static struct thermal_zone_device_ops remote_ops = {
+        .get_temp = lm90_read_temp2_temp,
+};

If without this patch, I have to rewrite the lm90_read_temp2_temp(),
which almost same as the show_temp11(), I think it's not good. When use
this patch and following 3/3 patch, the code will be more readable and
clear.

Anyway, if you want, I can send this patch as a separate one. :)

> 
> One thing I am a little worried about (but maybe I'm wrong) is that I
> seem to understand you want to register every LM90-like chip as both a
> hwmon device and two thermal devices. I seem to recall that every
> thermal device is also exposed automatically as a virtual hwmon
> device, is that correct? If so we will be presenting the same values
> twice to libsensors, which would be confusing.
> 
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/hwmon/lm90.c |  112 +++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 69 insertions(+), 43 deletions(-)
> 
> The code changes look good, however I have one suggestion for
> improvement (plus a minor cleanup request):
> 
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 8eeb141..5f30f90 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> (...)
>> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> -			 const char *buf, size_t count)
>> (...)
>> +static void write_temp8(struct device *dev, int index, long val)
>>  {
>>  	static const u8 reg[8] = {
>>  		LM90_REG_W_LOCAL_LOW,
>> @@ -737,60 +742,73 @@ 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 i2c_client *client = to_i2c_client(dev);
>>  	struct lm90_data *data = i2c_get_clientdata(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->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);
>> +		data->temp8[index] = 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, index >= 6);
>> +	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
> 
> This write could fail. So far the lm90 driver has failed to handle
> register write errors at all, and I will take the blame for that. But
> if we want to integrate properly with the thermal subsystem, I suspect
> we will have to properly report errors. So it might be the right time
> to catch and return write errors here. Then set_temp8() below could
> return it to user-space (either in this patch or in a separate patch,
> as you prefer.)

Ok, I will add error handler in my next version.

> 
> And then as a next step, lm90_select_remote_channel should return
> errors as they happen as well, so that they can be transmitted to the
> caller.
> 
>>  	lm90_select_remote_channel(client, data, 0);
>>  
>>  	mutex_unlock(&data->update_lock);
>> +}
>> +
>> +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;
>> +
>> +	write_temp8(dev, index, val);
>> +
>>  	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)
>> -		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)
> 
> There's a doubled space on this line. It isn't added by your patch, it
> was already there before, but please fix it while you're here.

Oh, you are so carefully, I will fix it :)

> 
>>  		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 void write_temp11(struct device *dev, int nr, int index, long val)
> 
> Here too I would suggest returning errors from the I2C layer, and
> handling them in set_temp11() now or later.
> 
>>  {
>>  	struct {
>>  		u8 high;
>> @@ -804,17 +822,8 @@ 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 i2c_client *client = to_i2c_client(dev);
>>  	struct lm90_data *data = i2c_get_clientdata(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)
>> @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>>  	lm90_select_remote_channel(client, data, 0);
>>  
>>  	mutex_unlock(&data->update_lock);
>> +}
>> +
>> +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;
>> +
>> +	write_temp11(dev, nr, index, val);
>> +
>>  	return count;
>>  }
>>  
> 

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

[Jean Delvare]

Hi Wei,

On Mon, 15 Jul 2013 14:05:08 +0800, Wei Ni wrote:
> On 07/12/2013 09:26 PM, Jean Delvare wrote:
> > Can I see a recent version of the code which will need this change? It
> > makes no sense to apply this first part which makes the code more
> > complex with no benefit, without the second part which needs it, so
> > they should be applied together or not at all.
> 
> In my RFC patches, there had many codes about thermal fw, which need
> this patch, so I put them together.
> And now I split the RFC patches, this series is preparing to use the
> thermal fw.
> As you said, I want to register lm90 as the thermal zone device, it need
> to hook some callback, such as .get_temp. if apply this patch, I can
> write the .get_temp simply, something like:
> 
> +static int lm90_read_temp2_temp(struct thermal_zone_device *thz,
> unsigned long *temp)
> +{
> +        struct lm90_data *data = thz->devdata;
> +        struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> +        struct device *dev = &client->dev;+
> +
> +        *temp = (long)read_temp11(dev, TEMP11_REMOTE_TEMP);
> +
> +        return 0;
> +}
> +
> +static struct thermal_zone_device_ops remote_ops = {
> +        .get_temp = lm90_read_temp2_temp,
> +};
> 
> If without this patch, I have to rewrite the lm90_read_temp2_temp(),
> which almost same as the show_temp11(), I think it's not good. When use
> this patch and following 3/3 patch, the code will be more readable and
> clear.

I understand the idea.

> Anyway, if you want, I can send this patch as a separate one. :)

Yes please, I think it would help me do a better code review and
testing as well.

-- 
Jean Delvare