[PATCH v2 0/3] expose lm90 to thermal fw

[PATCH v2 0/3] expose lm90 to thermal fw

[Wei Ni]

Expose lm90 to thermal framework via DT nodes.

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

Wei Ni (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

 arch/arm/boot/dts/tegra114-dalmore.dts |   20 ++-
 drivers/hwmon/lm90.c                   |  222 ++++++++++++++++++++++++--------
 2 files changed, 186 insertions(+), 56 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] 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.
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.

Change-Id: Id7178f680718fd54b9ba953118d91052b59b692e
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.7.9.5

[PATCH v2 2/3] hwmon: lm90: expose to thermal fw via DT nodes

[Wei Ni]

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.

Change-Id: Ie263e8973ec45d057fddadd3042c7d3fefdf4acc
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.7.9.5

[PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

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

Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
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 8de5437..453f5ea 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -780,12 +780,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>;
 		};
 	};
 
@@ -1276,4 +1278,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.7.9.5

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Stephen Warren]

On 03/04/2014 04:31 AM, Wei Ni wrote:
> Add dt node to describe the thermal zone for the nct1008.
> 
> Change-Id: I0256836e52d73ec282574e4fd91df52958936e35

???????

> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts

> +	thermal-zones {
> +		nct1008-local {
> +			polling-delay-passive = <2000>; /* milliseconds */

Nothing can possible make use of this node, since there's no compatible
value or anything else like that to bind a driver to it.

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Guenter Roeck]

On Tue, Mar 04, 2014 at 03:06:48PM -0700, Stephen Warren wrote:
> On 03/04/2014 04:31 AM, Wei Ni wrote:
> > Add dt node to describe the thermal zone for the nct1008.
> > 
> > Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
> 
> ???????
> 
I think this is for one of those open-source sites out there
which count open source contributions. Not Ohloh, but similar.
You have to specify that Change-Id to have it associate your
name with the patch.

Guenter

> > diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> 
> > +	thermal-zones {
> > +		nct1008-local {
> > +			polling-delay-passive = <2000>; /* milliseconds */
> 
> Nothing can possible make use of this node, since there's no compatible
> value or anything else like that to bind a driver to it.
> 

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

On 03/05/2014 06:23 AM, Guenter Roeck wrote:
> On Tue, Mar 04, 2014 at 03:06:48PM -0700, Stephen Warren wrote:
>> On 03/04/2014 04:31 AM, Wei Ni wrote:
>>> Add dt node to describe the thermal zone for the nct1008.
>>>
>>> Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
>>
>> ???????
>>
> I think this is for one of those open-source sites out there
> which count open source contributions. Not Ohloh, but similar.
> You have to specify that Change-Id to have it associate your
> name with the patch.

It's so sorry, I'm working for chromiumos, this change-id is generated
by gerrit, I should remove it.

> 
> Guenter
> 
>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>
>>> +	thermal-zones {
>>> +		nct1008-local {
>>> +			polling-delay-passive = <2000>; /* milliseconds */
>>
>> Nothing can possible make use of this node, since there's no compatible
>> value or anything else like that to bind a driver to it.

We have special Tskin driver that estimator skin temperature based on
nct1008 local and remote sensors.

Thanks.
Wei.

>>

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Stephen Warren]

On 03/04/2014 07:30 PM, Wei Ni wrote:
> On 03/05/2014 06:23 AM, Guenter Roeck wrote:
>> On Tue, Mar 04, 2014 at 03:06:48PM -0700, Stephen Warren wrote:
>>> On 03/04/2014 04:31 AM, Wei Ni wrote:
>>>> Add dt node to describe the thermal zone for the nct1008.
>>>>
>>>> Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
>>>
>>> ???????
>>>
>> I think this is for one of those open-source sites out there
>> which count open source contributions. Not Ohloh, but similar.
>> You have to specify that Change-Id to have it associate your
>> name with the patch.
> 
> It's so sorry, I'm working for chromiumos, this change-id is generated
> by gerrit, I should remove it.
> 
>>
>> Guenter
>>
>>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>>
>>>> +	thermal-zones {
>>>> +		nct1008-local {
>>>> +			polling-delay-passive = <2000>; /* milliseconds */
>>>
>>> Nothing can possible make use of this node, since there's no compatible
>>> value or anything else like that to bind a driver to it.
> 
> We have special Tskin driver that estimator skin temperature based on
> nct1008 local and remote sensors.

Where? Anything that gets added to the upstream kernel DT files must be
supported by a driver and binding in the upstream kernel.

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

On 03/05/2014 10:58 AM, Stephen Warren wrote:
> On 03/04/2014 07:30 PM, Wei Ni wrote:
>> On 03/05/2014 06:23 AM, Guenter Roeck wrote:
>>> On Tue, Mar 04, 2014 at 03:06:48PM -0700, Stephen Warren wrote:
>>>> On 03/04/2014 04:31 AM, Wei Ni wrote:
>>>>> Add dt node to describe the thermal zone for the nct1008.
>>>>>
>>>>> Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
>>>>
>>>> ???????
>>>>
>>> I think this is for one of those open-source sites out there
>>> which count open source contributions. Not Ohloh, but similar.
>>> You have to specify that Change-Id to have it associate your
>>> name with the patch.
>>
>> It's so sorry, I'm working for chromiumos, this change-id is generated
>> by gerrit, I should remove it.
>>
>>>
>>> Guenter
>>>
>>>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>>>
>>>>> +	thermal-zones {
>>>>> +		nct1008-local {
>>>>> +			polling-delay-passive = <2000>; /* milliseconds */
>>>>
>>>> Nothing can possible make use of this node, since there's no compatible
>>>> value or anything else like that to bind a driver to it.
>>
>> We have special Tskin driver that estimator skin temperature based on
>> nct1008 local and remote sensors.
> 
> Where? Anything that gets added to the upstream kernel DT files must be
> supported by a driver and binding in the upstream kernel.

I didn't submit the Tskin driver yet, but the of-thermal framework can
parse this node, and create thermal zone devices.

Thanks.
Wei.

> 

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Stephen Warren]

On 03/04/2014 08:07 PM, Wei Ni wrote:
> On 03/05/2014 10:58 AM, Stephen Warren wrote:
>> On 03/04/2014 07:30 PM, Wei Ni wrote:
>>> On 03/05/2014 06:23 AM, Guenter Roeck wrote:
>>>> On Tue, Mar 04, 2014 at 03:06:48PM -0700, Stephen Warren wrote:
>>>>> On 03/04/2014 04:31 AM, Wei Ni wrote:
>>>>>> Add dt node to describe the thermal zone for the nct1008.
>>>>>>
>>>>>> Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
>>>>>
>>>>> ???????
>>>>>
>>>> I think this is for one of those open-source sites out there
>>>> which count open source contributions. Not Ohloh, but similar.
>>>> You have to specify that Change-Id to have it associate your
>>>> name with the patch.
>>>
>>> It's so sorry, I'm working for chromiumos, this change-id is generated
>>> by gerrit, I should remove it.
>>>
>>>>
>>>> Guenter
>>>>
>>>>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>>>>
>>>>>> +	thermal-zones {
>>>>>> +		nct1008-local {
>>>>>> +			polling-delay-passive = <2000>; /* milliseconds */
>>>>>
>>>>> Nothing can possible make use of this node, since there's no compatible
>>>>> value or anything else like that to bind a driver to it.
>>>
>>> We have special Tskin driver that estimator skin temperature based on
>>> nct1008 local and remote sensors.
>>
>> Where? Anything that gets added to the upstream kernel DT files must be
>> supported by a driver and binding in the upstream kernel.
> 
> I didn't submit the Tskin driver yet, but the of-thermal framework can
> parse this node, and create thermal zone devices.

How is that possible? There's no compatible value for the of-thermal
framework to know that it should process this node. It's not correct for
the of-thermal framework to do anything like look for a node with a
particular name... has the DT binding for this been reviewed by any DT
maintainer?

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

On 03/05/2014 11:14 AM, Stephen Warren wrote:
> On 03/04/2014 08:07 PM, Wei Ni wrote:
>> On 03/05/2014 10:58 AM, Stephen Warren wrote:
>>> On 03/04/2014 07:30 PM, Wei Ni wrote:
>>>> On 03/05/2014 06:23 AM, Guenter Roeck wrote:
>>>>> On Tue, Mar 04, 2014 at 03:06:48PM -0700, Stephen Warren wrote:
>>>>>> On 03/04/2014 04:31 AM, Wei Ni wrote:
>>>>>>> Add dt node to describe the thermal zone for the nct1008.
>>>>>>>
>>>>>>> Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
>>>>>>
>>>>>> ???????
>>>>>>
>>>>> I think this is for one of those open-source sites out there
>>>>> which count open source contributions. Not Ohloh, but similar.
>>>>> You have to specify that Change-Id to have it associate your
>>>>> name with the patch.
>>>>
>>>> It's so sorry, I'm working for chromiumos, this change-id is generated
>>>> by gerrit, I should remove it.
>>>>
>>>>>
>>>>> Guenter
>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>>>>>
>>>>>>> +	thermal-zones {
>>>>>>> +		nct1008-local {
>>>>>>> +			polling-delay-passive = <2000>; /* milliseconds */
>>>>>>
>>>>>> Nothing can possible make use of this node, since there's no compatible
>>>>>> value or anything else like that to bind a driver to it.
>>>>
>>>> We have special Tskin driver that estimator skin temperature based on
>>>> nct1008 local and remote sensors.
>>>
>>> Where? Anything that gets added to the upstream kernel DT files must be
>>> supported by a driver and binding in the upstream kernel.
>>
>> I didn't submit the Tskin driver yet, but the of-thermal framework can
>> parse this node, and create thermal zone devices.
> 
> How is that possible? There's no compatible value for the of-thermal
> framework to know that it should process this node. It's not correct for
> the of-thermal framework to do anything like look for a node with a
> particular name... has the DT binding for this been reviewed by any DT
> maintainer?

Eduardo developed the of-thermal framework, and have merged in
linux-next, it can parse the node thermal-zones, and properties, such as
polling-delay-passive. Please check the omap.dtsi, it also has this node.

> 

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

On 03/05/2014 12:26 PM, Wei Ni wrote:
> On 03/05/2014 11:14 AM, Stephen Warren wrote:
>> On 03/04/2014 08:07 PM, Wei Ni wrote:
>>> On 03/05/2014 10:58 AM, Stephen Warren wrote:
>>>> On 03/04/2014 07:30 PM, Wei Ni wrote:
>>>>> On 03/05/2014 06:23 AM, Guenter Roeck wrote:
>>>>>> On Tue, Mar 04, 2014 at 03:06:48PM -0700, Stephen Warren wrote:
>>>>>>> On 03/04/2014 04:31 AM, Wei Ni wrote:
>>>>>>>> Add dt node to describe the thermal zone for the nct1008.
>>>>>>>>
>>>>>>>> Change-Id: I0256836e52d73ec282574e4fd91df52958936e35
>>>>>>>
>>>>>>> ???????
>>>>>>>
>>>>>> I think this is for one of those open-source sites out there
>>>>>> which count open source contributions. Not Ohloh, but similar.
>>>>>> You have to specify that Change-Id to have it associate your
>>>>>> name with the patch.
>>>>>
>>>>> It's so sorry, I'm working for chromiumos, this change-id is generated
>>>>> by gerrit, I should remove it.
>>>>>
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>>>>>>
>>>>>>>> +	thermal-zones {
>>>>>>>> +		nct1008-local {
>>>>>>>> +			polling-delay-passive = <2000>; /* milliseconds */
>>>>>>>
>>>>>>> Nothing can possible make use of this node, since there's no compatible
>>>>>>> value or anything else like that to bind a driver to it.
>>>>>
>>>>> We have special Tskin driver that estimator skin temperature based on
>>>>> nct1008 local and remote sensors.
>>>>
>>>> Where? Anything that gets added to the upstream kernel DT files must be
>>>> supported by a driver and binding in the upstream kernel.
>>>
>>> I didn't submit the Tskin driver yet, but the of-thermal framework can
>>> parse this node, and create thermal zone devices.
>>
>> How is that possible? There's no compatible value for the of-thermal
>> framework to know that it should process this node. It's not correct for
>> the of-thermal framework to do anything like look for a node with a
>> particular name... has the DT binding for this been reviewed by any DT
>> maintainer?
> 
> Eduardo developed the of-thermal framework, and have merged in
> linux-next, it can parse the node thermal-zones, and properties, such as
> polling-delay-passive. Please check the omap.dtsi, it also has this node.

Please check Documentation/devicetree/bindings/thermal/thermal.txt

> 
>>
> 
> --
> 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 v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Stephen Warren]

On 03/04/2014 04:31 AM, Wei Ni wrote:
> Add dt node to describe the thermal zone for the nct1008.

> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts

> +	thermal-zones {
> +		nct1008-local {
> +			polling-delay-passive = <2000>; /* milliseconds */
> +			polling-delay = <0>; /* milliseconds */

A delay of 0 implies that software continually polls this zone, thus
consuming an entire CPU. Is this really what you want?

> +
> +			thermal-sensors = <&nct1008 0>;
> +		};

This zone (and the other) is missing the required (per the binding
documentation) trips and cooling-maps sub-nodes.

I've also sent a query in response to the thermal-zones binding so see
whether or not it can be fixed so that software doesn't bind against the
node name, but rather against a compatible property like almost
everything else. Let's see what the response is there before reposting
this patch.

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Wei Ni]

On 03/07/2014 01:59 AM, Stephen Warren wrote:
> On 03/04/2014 04:31 AM, Wei Ni wrote:
>> Add dt node to describe the thermal zone for the nct1008.
> 
>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> 
>> +	thermal-zones {
>> +		nct1008-local {
>> +			polling-delay-passive = <2000>; /* milliseconds */
>> +			polling-delay = <0>; /* milliseconds */
> 
> A delay of 0 implies that software continually polls this zone, thus
> consuming an entire CPU. Is this really what you want?

No, if this delay is 0, the driver will not run polling queue, but just
cancel it.

> 
>> +
>> +			thermal-sensors = <&nct1008 0>;
>> +		};
> 
> This zone (and the other) is missing the required (per the binding
> documentation) trips and cooling-maps sub-nodes.

Eduardo,
Are the trips and cooling-maps required properties ?
I think if miss these sub-nodes, the thermal zone device also can work,
and in the of-thermal framework, it doesn't handle them as required
property either.

> 
> I've also sent a query in response to the thermal-zones binding so see
> whether or not it can be fixed so that software doesn't bind against the
> node name, but rather against a compatible property like almost
> everything else. Let's see what the response is there before reposting
> this patch.

Sure.

Thanks.
Wei.

> 

Re: [PATCH v2 3/3] ARM: tegra: dalmore: add thermal zones for nct1008

[Stephen Warren]

On 03/06/2014 08:45 PM, Wei Ni wrote:
> On 03/07/2014 01:59 AM, Stephen Warren wrote:
>> On 03/04/2014 04:31 AM, Wei Ni wrote:
>>> Add dt node to describe the thermal zone for the nct1008.
>>
>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>
>>> +	thermal-zones {
>>> +		nct1008-local {
>>> +			polling-delay-passive = <2000>; /* milliseconds */
>>> +			polling-delay = <0>; /* milliseconds */
>>
>> A delay of 0 implies that software continually polls this zone, thus
>> consuming an entire CPU. Is this really what you want?
> 
> No, if this delay is 0, the driver will not run polling queue, but just
> cancel it.

The binding doesn't say this. The DT needs to be written to the binding
as specified in the documentation, not to the behaviour of one
particular OS's driver.

Now, if the DT binding gets updated to explicitly document this, the DT
content above would be fine.