[RFC PATCH 0/9] Support for tegra30 thermal.

[RFC PATCH 0/9] Support for tegra30 thermal.

[Wei Ni]

This patch series is to add tegra30 thermal driver.
It use new linux thermal framework, which is developing by Durgadoss.

Note:
1. my series are based on the patches:
http://thread.gmane.org/gmane.linux.power-management.general/30692 ,
which come from Durgadoss.
And the tree is Rui's -next tree.

Next steps:
1. enhance the tegra30 thermal driver, something like handling the irq, setting
the sensor limit value.
2. seprate codes to create a new files which is for cooling device, something
like tegra3_cooling.c. it will handle dvfs.

Wei Ni (9):
  ARM: dt: t30 cardhu: add dt entry for lm90
  hwmon: (lm90) split set&show temp as common codes
  hwmon: (lm90) add support to handle irq
  hwmon: (lm90) use macros for the indexes of temp8 and temp11
  Thermal: Support using dt node to get sensor
  hwmon: (lm90) Register to the thermal framework
  thermal: tegra30: add tegra30 thermal driver
  ARM: dt: t30 cardhu: add dt entry for thermal driver
  ARM: tegra: defconfig: enable thermal framework

 .../devicetree/bindings/thermal/tegra3-thermal.txt |   41 ++
 arch/arm/boot/dts/tegra30-cardhu.dtsi              |   27 +
 arch/arm/configs/tegra_defconfig                   |    2 +
 drivers/hwmon/lm90.c                               |  514 +++++++++++++++-----
 drivers/thermal/Kconfig                            |    9 +
 drivers/thermal/Makefile                           |    1 +
 drivers/thermal/tegra3_thermal.c                   |  289 +++++++++++
 drivers/thermal/thermal_sys.c                      |   30 ++
 include/linux/thermal.h                            |    9 +
 9 files changed, 812 insertions(+), 110 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/tegra3-thermal.txt
 create mode 100644 drivers/thermal/tegra3_thermal.c

-- 
1.7.9.5

[RFC PATCH 1/9] ARM: dt: t30 cardhu: add dt entry for lm90

[Wei Ni]

Enable thermal sensor lm90 in the dts file.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30-cardhu.dtsi |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index bdb2a66..15ad1ad 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -273,6 +273,13 @@
 				};
 			};
 		};
+
+		nct1008: lm90@4c {
+			compatible = "lm90";
+			reg = <0x4c>;
+			interrupt-parent = <&gpio>;
+			interrupts = <226 0x08>; /* gpio PCC2 */
+		};
 	};
 
 	spi@7000da00 {
-- 
1.7.9.5

Re: [RFC PATCH 1/9] ARM: dt: t30 cardhu: add dt entry for lm90

[Alex Courbot]

On 02/18/2013 08:30 PM, Wei Ni wrote:
> Enable thermal sensor lm90 in the dts file.

Acked-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Btw, shouldn't this patch come last, after all the changes you did to 
lm90? If you keep the current order, you will need to ensure that the 
device performs as expected on Cardhu for each of the remaining patches 
of the series. It is probably safer to just move this one at the end to 
avoid trouble.

Alex.

Re: [RFC PATCH 1/9] ARM: dt: t30 cardhu: add dt entry for lm90

[Wei Ni]

On 02/19/2013 11:28 AM, Alex Courbot wrote:
> On 02/18/2013 08:30 PM, Wei Ni wrote:
>> Enable thermal sensor lm90 in the dts file.
> 
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> Btw, shouldn't this patch come last, after all the changes you did to 
> lm90? If you keep the current order, you will need to ensure that the 
> device performs as expected on Cardhu for each of the remaining patches 
> of the series. It is probably safer to just move this one at the end to 
> avoid trouble.

I will try split this patch set into 3 separate sets in my next version,
like Matthew's suggestion, so the patches will be clear.

Wei.

> 
> Alex.
> 

Re: [RFC PATCH 1/9] ARM: dt: t30 cardhu: add dt entry for lm90

[Stephen Warren]

On 02/18/2013 08:28 PM, Alex Courbot wrote:
> On 02/18/2013 08:30 PM, Wei Ni wrote:
>> Enable thermal sensor lm90 in the dts file.
> 
> Acked-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Btw, shouldn't this patch come last, after all the changes you did to
> lm90? If you keep the current order, you will need to ensure that the
> device performs as expected on Cardhu for each of the remaining patches
> of the series. It is probably safer to just move this one at the end to
> avoid trouble.

Hmm. This patch on its own seems simple enough, and so I should really
just apply it as soon as 3.9-rc1 is out.

Note that we (and probably others) have many device trees for boards
besides Cardhu that instantiate the LM90 driver. The driver has to
continue to work perfectly for all the patches in the series. Not
enabling the driver on Cardhu isn't enough to allow patches in the
middle of the series to break the already-working driver.

[RFC PATCH 3/9] hwmon: (lm90) add support to handle irq

[Wei Ni]

Add support to handle irq. When the temperature touch the limit value, the
driver can handle the interrupt.

Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/hwmon/lm90.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index caf01b0..80311ef 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -89,6 +89,8 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
 
 /*
  * Addresses to scan
@@ -302,6 +304,7 @@ static const struct lm90_params lm90_params[] = {
 struct lm90_data {
 	struct device *hwmon_dev;
 	struct mutex update_lock;
+	struct work_struct irq_work;
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
 	int kind;
@@ -1418,6 +1421,29 @@ static void lm90_init_client(struct i2c_client *client)
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
 }
 
+static void lm90_alert(struct i2c_client *client, unsigned int flag);
+
+static void lm90_irq_work(struct work_struct *work)
+{
+	struct lm90_data *data = container_of(work, struct lm90_data,
+					      irq_work);
+	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
+
+	lm90_alert(client, 0);
+
+	enable_irq(client->irq);
+}
+
+static irqreturn_t lm90_irq(int irq, void *dev_id)
+{
+	struct lm90_data *data = dev_id;
+
+	disable_irq_nosync(irq);
+	schedule_work(&data->irq_work);
+
+	return IRQ_HANDLED;
+}
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1494,6 +1520,17 @@ static int lm90_probe(struct i2c_client *client,
 		goto exit_remove_files;
 	}
 
+	if (client->irq >= 0) {
+		INIT_WORK(&data->irq_work, lm90_irq_work);
+		dev_dbg(dev, "lm90 irq: %d\n", client->irq);
+		err = request_irq(client->irq, lm90_irq, IRQF_TRIGGER_LOW,
+				       "lm90", data);
+		if (err < 0) {
+			dev_err(dev, "cannot request interrupt\n");
+			goto exit_remove_files;
+		}
+	}
+
 	return 0;
 
 exit_remove_files:
@@ -1507,6 +1544,7 @@ static int lm90_remove(struct i2c_client *client)
 {
 	struct lm90_data *data = i2c_get_clientdata(client);
 
+	free_irq(client->irq, data);
 	hwmon_device_unregister(data->hwmon_dev);
 	lm90_remove_files(client, data);
 	lm90_restore_conf(client, data);
-- 
1.7.9.5

Re: [lm-sensors] [RFC PATCH 3/9] hwmon: (lm90) add support to handle irq

[Guenter Roeck]

On Mon, Feb 18, 2013 at 07:30:25PM +0800, Wei Ni wrote:
> Add support to handle irq. When the temperature touch the limit value, the
> driver can handle the interrupt.
> 
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/hwmon/lm90.c |   38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index caf01b0..80311ef 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,8 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_irq.h>
>  
>  /*
>   * Addresses to scan
> @@ -302,6 +304,7 @@ static const struct lm90_params lm90_params[] = {
>  struct lm90_data {
>  	struct device *hwmon_dev;
>  	struct mutex update_lock;
> +	struct work_struct irq_work;
>  	char valid; /* zero until following fields are valid */
>  	unsigned long last_updated; /* in jiffies */
>  	int kind;
> @@ -1418,6 +1421,29 @@ static void lm90_init_client(struct i2c_client *client)
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>  }
>  
> +static void lm90_alert(struct i2c_client *client, unsigned int flag);
> +
> +static void lm90_irq_work(struct work_struct *work)
> +{
> +	struct lm90_data *data = container_of(work, struct lm90_data,
> +					      irq_work);
> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> +
> +	lm90_alert(client, 0);
> +
> +	enable_irq(client->irq);
> +}
> +
> +static irqreturn_t lm90_irq(int irq, void *dev_id)
> +{
> +	struct lm90_data *data = dev_id;
> +
> +	disable_irq_nosync(irq);
> +	schedule_work(&data->irq_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1494,6 +1520,17 @@ static int lm90_probe(struct i2c_client *client,
>  		goto exit_remove_files;
>  	}
>  
> +	if (client->irq >= 0) {
> +		INIT_WORK(&data->irq_work, lm90_irq_work);
> +		dev_dbg(dev, "lm90 irq: %d\n", client->irq);
> +		err = request_irq(client->irq, lm90_irq, IRQF_TRIGGER_LOW,
> +				       "lm90", data);

request_threaded_irq or even better devm_request_threaded_irq would be better here.

> +		if (err < 0) {
> +			dev_err(dev, "cannot request interrupt\n");
> +			goto exit_remove_files;
> +		}
> +	}
> +
>  	return 0;
>  
>  exit_remove_files:
> @@ -1507,6 +1544,7 @@ static int lm90_remove(struct i2c_client *client)
>  {
>  	struct lm90_data *data = i2c_get_clientdata(client);
>  
> +	free_irq(client->irq, data);
>  	hwmon_device_unregister(data->hwmon_dev);
>  	lm90_remove_files(client, data);
>  	lm90_restore_conf(client, data);
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> 

Re: [lm-sensors] [RFC PATCH 3/9] hwmon: (lm90) add support to handle irq

[Wei Ni]

On 02/19/2013 11:34 AM, Guenter Roeck wrote:
> On Mon, Feb 18, 2013 at 07:30:25PM +0800, Wei Ni wrote:
>> Add support to handle irq. When the temperature touch the limit value, the
>> driver can handle the interrupt.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/hwmon/lm90.c |   38 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index caf01b0..80311ef 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -89,6 +89,8 @@
>>  #include <linux/err.h>
>>  #include <linux/mutex.h>
>>  #include <linux/sysfs.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_irq.h>
>>  
>>  /*
>>   * Addresses to scan
>> @@ -302,6 +304,7 @@ static const struct lm90_params lm90_params[] = {
>>  struct lm90_data {
>>  	struct device *hwmon_dev;
>>  	struct mutex update_lock;
>> +	struct work_struct irq_work;
>>  	char valid; /* zero until following fields are valid */
>>  	unsigned long last_updated; /* in jiffies */
>>  	int kind;
>> @@ -1418,6 +1421,29 @@ static void lm90_init_client(struct i2c_client *client)
>>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>  }
>>  
>> +static void lm90_alert(struct i2c_client *client, unsigned int flag);
>> +
>> +static void lm90_irq_work(struct work_struct *work)
>> +{
>> +	struct lm90_data *data = container_of(work, struct lm90_data,
>> +					      irq_work);
>> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
>> +
>> +	lm90_alert(client, 0);
>> +
>> +	enable_irq(client->irq);
>> +}
>> +
>> +static irqreturn_t lm90_irq(int irq, void *dev_id)
>> +{
>> +	struct lm90_data *data = dev_id;
>> +
>> +	disable_irq_nosync(irq);
>> +	schedule_work(&data->irq_work);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int lm90_probe(struct i2c_client *client,
>>  		      const struct i2c_device_id *id)
>>  {
>> @@ -1494,6 +1520,17 @@ static int lm90_probe(struct i2c_client *client,
>>  		goto exit_remove_files;
>>  	}
>>  
>> +	if (client->irq >= 0) {
>> +		INIT_WORK(&data->irq_work, lm90_irq_work);
>> +		dev_dbg(dev, "lm90 irq: %d\n", client->irq);
>> +		err = request_irq(client->irq, lm90_irq, IRQF_TRIGGER_LOW,
>> +				       "lm90", data);
> 
> request_threaded_irq or even better devm_request_threaded_irq would be better here.

yes, it's better to use request_threaded_irq here.
we can set lm90_irq() as interrupt handle, and set lm90_alert() as
thread function.
I will try it on my next version.

> 
>> +		if (err < 0) {
>> +			dev_err(dev, "cannot request interrupt\n");
>> +			goto exit_remove_files;
>> +		}
>> +	}
>> +
>>  	return 0;
>>  
>>  exit_remove_files:
>> @@ -1507,6 +1544,7 @@ static int lm90_remove(struct i2c_client *client)
>>  {
>>  	struct lm90_data *data = i2c_get_clientdata(client);
>>  
>> +	free_irq(client->irq, data);
>>  	hwmon_device_unregister(data->hwmon_dev);
>>  	lm90_remove_files(client, data);
>>  	lm90_restore_conf(client, data);
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> lm-sensors mailing list
>> lm-sensors@lm-sensors.org
>> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>>
> --
> 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: [RFC PATCH 3/9] hwmon: (lm90) add support to handle irq

[Stephen Warren]

On 02/18/2013 04:30 AM, Wei Ni wrote:
> Add support to handle irq. When the temperature touch the limit value, the
> driver can handle the interrupt.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c

> +static void lm90_irq_work(struct work_struct *work)
...
> +	enable_irq(client->irq);
> +}
> +
> +static irqreturn_t lm90_irq(int irq, void *dev_id)
> +{
> +	struct lm90_data *data = dev_id;
> +
> +	disable_irq_nosync(irq);

I think IRQF_ONESHOT would enable you to remove the manual
enable/disable_irq calls.

Re: [RFC PATCH 3/9] hwmon: (lm90) add support to handle irq

[Alex Courbot]

On 02/20/2013 08:00 AM, Stephen Warren wrote:
> On 02/18/2013 04:30 AM, Wei Ni wrote:
>> Add support to handle irq. When the temperature touch the limit value, the
>> driver can handle the interrupt.
>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>
>> +static void lm90_irq_work(struct work_struct *work)
> ...
>> +	enable_irq(client->irq);
>> +}
>> +
>> +static irqreturn_t lm90_irq(int irq, void *dev_id)
>> +{
>> +	struct lm90_data *data = dev_id;
>> +
>> +	disable_irq_nosync(irq);
>
> I think IRQF_ONESHOT would enable you to remove the manual
> enable/disable_irq calls.

Oh, right. Using a threaded IRQ with IRQF_ONESHOT would make that happen 
transparently. Wasn't aware of that, thanks.

Alex.

Re: [RFC PATCH 3/9] hwmon: (lm90) add support to handle irq

[Wei Ni]

On 02/20/2013 11:27 AM, Alex Courbot wrote:
> On 02/20/2013 08:00 AM, Stephen Warren wrote:
>> On 02/18/2013 04:30 AM, Wei Ni wrote:
>>> Add support to handle irq. When the temperature touch the limit value, the
>>> driver can handle the interrupt.
>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>
>>> +static void lm90_irq_work(struct work_struct *work)
>> ...
>>> +	enable_irq(client->irq);
>>> +}
>>> +
>>> +static irqreturn_t lm90_irq(int irq, void *dev_id)
>>> +{
>>> +	struct lm90_data *data = dev_id;
>>> +
>>> +	disable_irq_nosync(irq);
>>
>> I think IRQF_ONESHOT would enable you to remove the manual
>> enable/disable_irq calls.
> 
> Oh, right. Using a threaded IRQ with IRQF_ONESHOT would make that happen 
> transparently. Wasn't aware of that, thanks.

Oh, yes, I will use it in my next version.

Thanks.
Wei.

> 
> Alex.
> 
> 

[RFC PATCH 5/9] Thermal: Support using dt node to get sensor

[Wei Ni]

Add functions to support using dt node with args to get sensor.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/thermal/thermal_sys.c |   30 ++++++++++++++++++++++++++++++
 include/linux/thermal.h       |    9 +++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index e284b67..b5bedab 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -2183,6 +2183,28 @@ struct thermal_sensor *get_sensor_by_name(const char *name)
 }
 EXPORT_SYMBOL(get_sensor_by_name);
 
+struct thermal_sensor *get_sensor_by_node(struct node_args *np_args)
+{
+	struct thermal_sensor *pos;
+	struct thermal_sensor *ts = NULL;
+	struct node_args *args;
+
+	mutex_lock(&sensor_list_lock);
+	for_each_thermal_sensor(pos) {
+		args = &pos->np_args;
+		if (args->np) {
+			if ((args->np == np_args->np) &&
+			   (args->index == np_args->index)) {
+				ts = pos;
+				break;
+			}
+		}
+	}
+	mutex_unlock(&sensor_list_lock);
+	return ts;
+}
+EXPORT_SYMBOL(get_sensor_by_node);
+
 static int create_map_attrs(struct thermal_zone *tz, int indx)
 {
 	int ret, i;
@@ -2368,6 +2390,7 @@ EXPORT_SYMBOL(add_sensor_trip_info);
  * @devdata:	private device data
  */
 struct thermal_sensor *thermal_sensor_register(const char *name, int count,
+			struct node_args *np_args,
 			struct thermal_sensor_ops *ops, void *devdata)
 {
 	struct thermal_sensor *ts;
@@ -2412,6 +2435,13 @@ struct thermal_sensor *thermal_sensor_register(const char *name, int count,
 			goto exit_temp;
 	}
 
+	if (np_args) {
+		ts->np_args.np = np_args->np;
+		ts->np_args.index = np_args->index;
+	} else {
+		ts->np_args.np = NULL;
+	}
+
 	/* Add this sensor to the global list of sensors */
 	mutex_lock(&sensor_list_lock);
 	list_add_tail(&ts->node, &thermal_sensor_list);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 4389599..b560ffa 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -113,6 +113,12 @@ enum {
 };
 #define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
 
+/* support to parse node with args to get sensor or cooling device */
+struct node_args {
+	struct device_node *np;
+	int index;
+};
+
 struct thermal_zone_device_ops {
 	int (*bind) (struct thermal_zone_device *,
 		     struct thermal_cooling_device *);
@@ -223,6 +229,7 @@ struct thermal_sensor {
 	void *devdata;
 	struct idr idr;
 	struct device device;
+	struct node_args np_args;
 	struct list_head node;
 	struct thermal_sensor_ops *ops;
 	struct thermal_attr *thresh_attrs;
@@ -350,6 +357,7 @@ int thermal_register_governor(struct thermal_governor *);
 void thermal_unregister_governor(struct thermal_governor *);
 
 struct thermal_sensor *thermal_sensor_register(const char *, int,
+				struct node_args *,
 				struct thermal_sensor_ops *, void *);
 void thermal_sensor_unregister(struct thermal_sensor *);
 
@@ -357,6 +365,7 @@ struct thermal_zone *create_thermal_zone(const char *, void *);
 void remove_thermal_zone(struct thermal_zone *);
 int add_sensor_to_zone(struct thermal_zone *, struct thermal_sensor *);
 struct thermal_sensor *get_sensor_by_name(const char *);
+struct thermal_sensor *get_sensor_by_node(struct node_args *);
 
 int add_cdev_to_zone(struct thermal_zone *, struct thermal_cooling_device *);
 struct thermal_cooling_device *get_cdev_by_name(const char *);
-- 
1.7.9.5

Re: [RFC PATCH 5/9] Thermal: Support using dt node to get sensor

[Stephen Warren]

On 02/18/2013 04:30 AM, Wei Ni wrote:
> Add functions to support using dt node with args to get sensor.

You need to write a device tree binding document to explain this.

> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c

> +struct thermal_sensor *get_sensor_by_node(struct node_args *np_args)
> +{
> +	struct thermal_sensor *pos;

"pos" isn't a great variable name. Why not use "sensor", or just the
"ts" variable you have right below?

> +	struct thermal_sensor *ts = NULL;
> +	struct node_args *args;
> +
> +	mutex_lock(&sensor_list_lock);
> +	for_each_thermal_sensor(pos) {
> +		args = &pos->np_args;
> +		if (args->np) {
> +			if ((args->np == np_args->np) &&
> +			   (args->index == np_args->index)) {
> +				ts = pos;
> +				break;

Replace those 2 lines with "goto out;".

> +			}
> +		}
> +	}

here, add:

	ts = NULL;
out:

That way, you can use "ts" as the loop iteration variable.

This whole patch rather assumes that all DT nodes can identify their
exposed thermal sensors using an index in a single DT cell. That's not
very flexible. All other DT bindings work like this:

Provider of a service indicates how many DT cells are in the object
(GPIO, IRQ, thermal sensors) specifier:

sensor1: lm90@1c {
	...
	#thermal-sensor-cells = <1>;
};

Each consumer of a service imports it by referencing it:

thermal-zone {
	...
	sensors = <&sensor1 0>;
};

The driver for LM90 provides an "of_xlate" function which receives a
struct of_phandle_args and outputs/returns whatever Linux-internal
identification/representation of the object is required. For example, see:

> include/linux/pwm.h:161:	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,

This allows each providing object's DT binding to define its own value
of #thermal-sensor-cells, as suited for its own requirements, and allows
each driver to implement the mapping from DT to internal ID in whatever
way is necessary.

Now, many bindings/drivers might just end up using a common simple
implementation. That's why functions such as of_pwm_simple_xlate() or
of_gpio_simple_xlate() exist.

Re: [RFC PATCH 5/9] Thermal: Support using dt node to get sensor

[Wei Ni]

On 02/20/2013 07:12 AM, Stephen Warren wrote:
> On 02/18/2013 04:30 AM, Wei Ni wrote:
>> Add functions to support using dt node with args to get sensor.
> 
> You need to write a device tree binding document to explain this.
> 
>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> 
>> +struct thermal_sensor *get_sensor_by_node(struct node_args *np_args)
>> +{
>> +	struct thermal_sensor *pos;
> 
> "pos" isn't a great variable name. Why not use "sensor", or just the
> "ts" variable you have right below?

Oh, yes, it can just use "ts" simply. I didn't consider it, thanks.

> 
>> +	struct thermal_sensor *ts = NULL;
>> +	struct node_args *args;
>> +
>> +	mutex_lock(&sensor_list_lock);
>> +	for_each_thermal_sensor(pos) {
>> +		args = &pos->np_args;
>> +		if (args->np) {
>> +			if ((args->np == np_args->np) &&
>> +			   (args->index == np_args->index)) {
>> +				ts = pos;
>> +				break;
> 
> Replace those 2 lines with "goto out;".
> 
>> +			}
>> +		}
>> +	}
> 
> here, add:
> 
> 	ts = NULL;
> out:
> 
> That way, you can use "ts" as the loop iteration variable.
> 
> This whole patch rather assumes that all DT nodes can identify their
> exposed thermal sensors using an index in a single DT cell. That's not
> very flexible. All other DT bindings work like this:
> 
> Provider of a service indicates how many DT cells are in the object
> (GPIO, IRQ, thermal sensors) specifier:
> 
> sensor1: lm90@1c {
> 	...
> 	#thermal-sensor-cells = <1>;
> };
> 
> Each consumer of a service imports it by referencing it:
> 
> thermal-zone {
> 	...
> 	sensors = <&sensor1 0>;
> };
> 
> The driver for LM90 provides an "of_xlate" function which receives a
> struct of_phandle_args and outputs/returns whatever Linux-internal
> identification/representation of the object is required. For example, see:
> 
>> include/linux/pwm.h:161:	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
> 
> This allows each providing object's DT binding to define its own value
> of #thermal-sensor-cells, as suited for its own requirements, and allows
> each driver to implement the mapping from DT to internal ID in whatever
> way is necessary.
> 
> Now, many bindings/drivers might just end up using a common simple
> implementation. That's why functions such as of_pwm_simple_xlate() or
> of_gpio_simple_xlate() exist.

It looks like the "of_xlate" can handle the sensor identification.
I'm not familiar with this function, I will look into it and try to use.
Thanks for your comments.

Wei.

> 

[RFC PATCH 6/9] hwmon: (lm90) Register to the thermal framework

[Wei Ni]

Register the remote sensor to the thermal framework.
It can support to show the temperature and read/write threshold.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30-cardhu.dtsi |    1 +
 drivers/hwmon/lm90.c                  |  182 ++++++++++++++++++++++++++++++++-
 2 files changed, 182 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index 15ad1ad..3f6ab89 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -279,6 +279,7 @@
 			reg = <0x4c>;
 			interrupt-parent = <&gpio>;
 			interrupts = <226 0x08>; /* gpio PCC2 */
+			#sensor-cells = <1>;
 		};
 	};
 
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index de5a476..0abdedc 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -91,6 +91,7 @@
 #include <linux/sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/of_irq.h>
+#include <linux/thermal.h>
 
 /*
  * Addresses to scan
@@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
 
 /*
+ * Thermal framework
+ */
+enum lm90_thresholds {
+	LM90_LOW_THRESHOLDS = 0,	/* threshold 0: low limits */
+	LM90_HIGH_THRESHOLDS,		/* threshold 1: high limits */
+	LM90_NUM_THRESHOLDS
+};
+
+/*
  * Driver data (common to all clients)
  */
 
@@ -377,6 +387,9 @@ struct lm90_data {
 	s16 temp11[TEMP11_REG_NUM];
 	u8 temp_hyst;
 	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
+
+	struct thermal_sensor *ts_remote;
+	struct thermal_sensor *ts_local;
 };
 
 /*
@@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp)
+{
+	struct i2c_client *client = ts->devdata;
+	struct device *dev = &client->dev;
+
+	_show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp);
+
+	return 0;
+}
+
+static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index,
+					long *val)
+{
+	struct i2c_client *client = ts->devdata;
+	struct device *dev = &client->dev;
+	int index;
+
+	switch (th_index) {
+	case LM90_LOW_THRESHOLDS:
+		/* remote low limit */
+		index = TEMP11_REMOTE_LOW;
+		break;
+	case LM90_HIGH_THRESHOLDS:
+		/* remote high limit */
+		index = TEMP11_REMOTE_HIGH;
+		break;
+	default:
+		dev_err(dev, "read remote threshold failed.\n");
+		return -EINVAL;
+	}
+
+	_show_temp11(dev, index, (int *)val);
+
+	return 0;
+}
+
+static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index,
+					long val)
+{
+	struct i2c_client *client = ts->devdata;
+	struct device *dev = &client->dev;
+	int nr, index;
+
+	switch (th_index) {
+	case LM90_LOW_THRESHOLDS:
+		/* remote low limit */
+		nr = NR_CHAN_0_REMOTE_LOW;
+		index = TEMP11_REMOTE_LOW;
+		break;
+	case LM90_HIGH_THRESHOLDS:
+		/* remote high limit */
+		nr = NR_CHAN_0_REMOTE_HIGH;
+		index = TEMP11_REMOTE_HIGH;
+		break;
+	default:
+		dev_err(dev, "write remote threshold failed.\n");
+		return -EINVAL;
+	}
+
+	_set_temp11(dev, nr, index, val);
+
+	return 0;
+}
+
+static struct thermal_sensor_ops remote_ops = {
+	.get_temp = lm90_read_remote_temp,
+	.get_threshold = lm90_read_remote_threshold,
+	.set_threshold = lm90_write_remote_threshold,
+};
+
+static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp)
+{
+	struct i2c_client *client = ts->devdata;
+	struct device *dev = &client->dev;
+
+	_show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp);
+
+	return 0;
+}
+
+static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index,
+					long *val)
+{
+	struct i2c_client *client = ts->devdata;
+	struct device *dev = &client->dev;
+	int index;
+
+	switch (th_index) {
+	case LM90_LOW_THRESHOLDS:
+		/* local low limit */
+		index = TEMP8_LOCAL_LOW;
+		break;
+	case LM90_HIGH_THRESHOLDS:
+		/* local high limit */
+		index = TEMP8_LOCAL_HIGH;
+		break;
+	default:
+		dev_err(dev, "read local threshold failed.\n");
+		return -EINVAL;
+	}
+
+	_show_temp8(dev, index, (int *)val);
+
+	return 0;
+}
+
+static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index,
+					long val)
+{
+	struct i2c_client *client = ts->devdata;
+	struct device *dev = &client->dev;
+	int index;
+
+	switch (th_index) {
+	case LM90_LOW_THRESHOLDS:
+		/* local low limit */
+		index = TEMP8_LOCAL_LOW;
+		break;
+	case LM90_HIGH_THRESHOLDS:
+		/* local high limit */
+		index = TEMP8_LOCAL_HIGH;
+		break;
+	default:
+		dev_err(dev, "write local threshold failed.\n");
+		return -EINVAL;
+	}
+
+	_set_temp8(dev, index, val);
+
+	return 0;
+}
+
+static struct thermal_sensor_ops local_ops = {
+	.get_temp = lm90_read_local_temp,
+	.get_threshold = lm90_read_local_threshold,
+	.set_threshold = lm90_write_local_threshold,
+};
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
 	struct i2c_adapter *adapter = to_i2c_adapter(dev->parent);
 	struct lm90_data *data;
+	struct node_args np_args;
 	int err;
 
 	data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
@@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client,
 				       "lm90", data);
 		if (err < 0) {
 			dev_err(dev, "cannot request interrupt\n");
-			goto exit_remove_files;
+			goto exit_unregister_hwmon;
 		}
 	}
 
+	np_args.np = dev->of_node;
+	np_args.index = 0;
+	data->ts_remote = thermal_sensor_register("lm90_remote",
+						LM90_NUM_THRESHOLDS,
+						&np_args,
+						&remote_ops, client);
+	if (IS_ERR(data->ts_remote)) {
+		dev_err(dev, "cannot register sensor to thermal framework\n");
+		err = -EINVAL;
+		goto exit_unregister_hwmon;
+	}
+
+	np_args.index = 1;
+	data->ts_local = thermal_sensor_register("lm90_local",
+						LM90_NUM_THRESHOLDS,
+						&np_args,
+						&local_ops, client);
+
+	if (IS_ERR(data->ts_local)) {
+		dev_err(dev, "cannot register sensor to thermal framework\n");
+		err = -EINVAL;
+		goto exit_unregister_hwmon;
+	}
+
 	return 0;
 
+exit_unregister_hwmon:
+	hwmon_device_unregister(data->hwmon_dev);
 exit_remove_files:
 	lm90_remove_files(client, data);
 exit_restore:
@@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client)
 	struct lm90_data *data = i2c_get_clientdata(client);
 
 	free_irq(client->irq, data);
+	thermal_sensor_unregister(data->ts_remote);
+	thermal_sensor_unregister(data->ts_local);
 	hwmon_device_unregister(data->hwmon_dev);
 	lm90_remove_files(client, data);
 	lm90_restore_conf(client, data);
-- 
1.7.9.5

Re: [lm-sensors] [RFC PATCH 6/9] hwmon: (lm90) Register to the thermal framework

[Guenter Roeck]

On Mon, Feb 18, 2013 at 07:30:28PM +0800, Wei Ni wrote:
> Register the remote sensor to the thermal framework.
> It can support to show the temperature and read/write threshold.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra30-cardhu.dtsi |    1 +
>  drivers/hwmon/lm90.c                  |  182 ++++++++++++++++++++++++++++++++-
>  2 files changed, 182 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> index 15ad1ad..3f6ab89 100644
> --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
> +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> @@ -279,6 +279,7 @@
>  			reg = <0x4c>;
>  			interrupt-parent = <&gpio>;
>  			interrupts = <226 0x08>; /* gpio PCC2 */
> +			#sensor-cells = <1>;
>  		};
>  	};
>  
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index de5a476..0abdedc 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -91,6 +91,7 @@
>  #include <linux/sysfs.h>
>  #include <linux/interrupt.h>
>  #include <linux/of_irq.h>
> +#include <linux/thermal.h>
>  
>  /*
>   * Addresses to scan
> @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>  
>  /*
> + * Thermal framework
> + */
> +enum lm90_thresholds {
> +	LM90_LOW_THRESHOLDS = 0,	/* threshold 0: low limits */
> +	LM90_HIGH_THRESHOLDS,		/* threshold 1: high limits */
> +	LM90_NUM_THRESHOLDS
> +};
> +
> +/*
>   * Driver data (common to all clients)
>   */
>  
> @@ -377,6 +387,9 @@ struct lm90_data {
>  	s16 temp11[TEMP11_REG_NUM];
>  	u8 temp_hyst;
>  	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
> +
> +	struct thermal_sensor *ts_remote;
> +	struct thermal_sensor *ts_local;
>  };
>  
>  /*
> @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +
> +	_show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp);
> +
> +	return 0;
> +}
> +
> +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index,
> +					long *val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* remote low limit */
> +		index = TEMP11_REMOTE_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* remote high limit */
> +		index = TEMP11_REMOTE_HIGH;
> +		break;
> +	default:
> +		dev_err(dev, "read remote threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_show_temp11(dev, index, (int *)val);

Typecasting a pointer like this doesn't work. Try this on a big-endian system with
sizeof(int) != sizeof(long). Note that you would not have the problem if you
would pass the value instead of the pointer.

> +
> +	return 0;
> +}
> +
> +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index,
> +					long val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int nr, index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* remote low limit */
> +		nr = NR_CHAN_0_REMOTE_LOW;
> +		index = TEMP11_REMOTE_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* remote high limit */
> +		nr = NR_CHAN_0_REMOTE_HIGH;
> +		index = TEMP11_REMOTE_HIGH;
> +		break;
> +	default:
> +		dev_err(dev, "write remote threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_set_temp11(dev, nr, index, val);
> +
> +	return 0;
> +}
> +
> +static struct thermal_sensor_ops remote_ops = {
> +	.get_temp = lm90_read_remote_temp,
> +	.get_threshold = lm90_read_remote_threshold,
> +	.set_threshold = lm90_write_remote_threshold,
> +};
> +
> +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +
> +	_show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp);
> +
Same here and everywhere else.

> +	return 0;
> +}
> +
> +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index,
> +					long *val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* local low limit */
> +		index = TEMP8_LOCAL_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* local high limit */
> +		index = TEMP8_LOCAL_HIGH;
> +		break;
> +	default:
> +		dev_err(dev, "read local threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_show_temp8(dev, index, (int *)val);
> +
> +	return 0;
> +}
> +
> +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index,
> +					long val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* local low limit */
> +		index = TEMP8_LOCAL_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* local high limit */
> +		index = TEMP8_LOCAL_HIGH;
> +		break;
> +	default:
> +		dev_err(dev, "write local threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_set_temp8(dev, index, val);
> +
> +	return 0;
> +}
> +
> +static struct thermal_sensor_ops local_ops = {
> +	.get_temp = lm90_read_local_temp,
> +	.get_threshold = lm90_read_local_threshold,
> +	.set_threshold = lm90_write_local_threshold,
> +};
> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
>  	struct i2c_adapter *adapter = to_i2c_adapter(dev->parent);
>  	struct lm90_data *data;
> +	struct node_args np_args;
>  	int err;
>  
>  	data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client,
>  				       "lm90", data);
>  		if (err < 0) {
>  			dev_err(dev, "cannot request interrupt\n");
> -			goto exit_remove_files;
> +			goto exit_unregister_hwmon;
>  		}
>  	}
>  
> +	np_args.np = dev->of_node;
> +	np_args.index = 0;
> +	data->ts_remote = thermal_sensor_register("lm90_remote",
> +						LM90_NUM_THRESHOLDS,
> +						&np_args,
> +						&remote_ops, client);
> +	if (IS_ERR(data->ts_remote)) {
> +		dev_err(dev, "cannot register sensor to thermal framework\n");
> +		err = -EINVAL;
> +		goto exit_unregister_hwmon;
> +	}
> +
> +	np_args.index = 1;
> +	data->ts_local = thermal_sensor_register("lm90_local",
> +						LM90_NUM_THRESHOLDS,
> +						&np_args,
> +						&local_ops, client);
> +
> +	if (IS_ERR(data->ts_local)) {
> +		dev_err(dev, "cannot register sensor to thermal framework\n");
> +		err = -EINVAL;
> +		goto exit_unregister_hwmon;
> +	}
> +

How about the second remote sensor ? Granted, not all chips supported by this
driver have it, but if we are adding this we might as well add it for all
sensors on all chips.

>  	return 0;
>  
> +exit_unregister_hwmon:
> +	hwmon_device_unregister(data->hwmon_dev);
>  exit_remove_files:
>  	lm90_remove_files(client, data);
>  exit_restore:
> @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client)
>  	struct lm90_data *data = i2c_get_clientdata(client);
>  
>  	free_irq(client->irq, data);
> +	thermal_sensor_unregister(data->ts_remote);
> +	thermal_sensor_unregister(data->ts_local);
>  	hwmon_device_unregister(data->hwmon_dev);
>  	lm90_remove_files(client, data);
>  	lm90_restore_conf(client, data);
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> 

Re: [lm-sensors] [RFC PATCH 6/9] hwmon: (lm90) Register to the thermal framework

[Wei Ni]

On 02/19/2013 11:42 AM, Guenter Roeck wrote:
> On Mon, Feb 18, 2013 at 07:30:28PM +0800, Wei Ni wrote:
>> Register the remote sensor to the thermal framework.
>> It can support to show the temperature and read/write threshold.
>>
>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/tegra30-cardhu.dtsi |    1 +
>>  drivers/hwmon/lm90.c                  |  182 ++++++++++++++++++++++++++++++++-
>>  2 files changed, 182 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
>> index 15ad1ad..3f6ab89 100644
>> --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
>> +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
>> @@ -279,6 +279,7 @@
>>  			reg = <0x4c>;
>>  			interrupt-parent = <&gpio>;
>>  			interrupts = <226 0x08>; /* gpio PCC2 */
>> +			#sensor-cells = <1>;
>>  		};
>>  	};
>>  
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index de5a476..0abdedc 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -91,6 +91,7 @@
>>  #include <linux/sysfs.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/of_irq.h>
>> +#include <linux/thermal.h>
>>  
>>  /*
>>   * Addresses to scan
>> @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>>  #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>>  
>>  /*
>> + * Thermal framework
>> + */
>> +enum lm90_thresholds {
>> +	LM90_LOW_THRESHOLDS = 0,	/* threshold 0: low limits */
>> +	LM90_HIGH_THRESHOLDS,		/* threshold 1: high limits */
>> +	LM90_NUM_THRESHOLDS
>> +};
>> +
>> +/*
>>   * Driver data (common to all clients)
>>   */
>>  
>> @@ -377,6 +387,9 @@ struct lm90_data {
>>  	s16 temp11[TEMP11_REG_NUM];
>>  	u8 temp_hyst;
>>  	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
>> +
>> +	struct thermal_sensor *ts_remote;
>> +	struct thermal_sensor *ts_local;
>>  };
>>  
>>  /*
>> @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +
>> +	_show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp);
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index,
>> +					long *val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* remote low limit */
>> +		index = TEMP11_REMOTE_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* remote high limit */
>> +		index = TEMP11_REMOTE_HIGH;
>> +		break;
>> +	default:
>> +		dev_err(dev, "read remote threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_show_temp11(dev, index, (int *)val);
> 
> Typecasting a pointer like this doesn't work. Try this on a big-endian system with
> sizeof(int) != sizeof(long). Note that you would not have the problem if you
> would pass the value instead of the pointer.

Yes, you are right, it will cause problems.
I will return the value for _show_temp11() and _show_temp8().

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index,
>> +					long val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int nr, index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* remote low limit */
>> +		nr = NR_CHAN_0_REMOTE_LOW;
>> +		index = TEMP11_REMOTE_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* remote high limit */
>> +		nr = NR_CHAN_0_REMOTE_HIGH;
>> +		index = TEMP11_REMOTE_HIGH;
>> +		break;
>> +	default:
>> +		dev_err(dev, "write remote threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_set_temp11(dev, nr, index, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_sensor_ops remote_ops = {
>> +	.get_temp = lm90_read_remote_temp,
>> +	.get_threshold = lm90_read_remote_threshold,
>> +	.set_threshold = lm90_write_remote_threshold,
>> +};
>> +
>> +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +
>> +	_show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp);
>> +
> Same here and everywhere else.
> 
>> +	return 0;
>> +}
>> +
>> +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index,
>> +					long *val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* local low limit */
>> +		index = TEMP8_LOCAL_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* local high limit */
>> +		index = TEMP8_LOCAL_HIGH;
>> +		break;
>> +	default:
>> +		dev_err(dev, "read local threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_show_temp8(dev, index, (int *)val);
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index,
>> +					long val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* local low limit */
>> +		index = TEMP8_LOCAL_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* local high limit */
>> +		index = TEMP8_LOCAL_HIGH;
>> +		break;
>> +	default:
>> +		dev_err(dev, "write local threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_set_temp8(dev, index, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_sensor_ops local_ops = {
>> +	.get_temp = lm90_read_local_temp,
>> +	.get_threshold = lm90_read_local_threshold,
>> +	.set_threshold = lm90_write_local_threshold,
>> +};
>> +
>>  static int lm90_probe(struct i2c_client *client,
>>  		      const struct i2c_device_id *id)
>>  {
>>  	struct device *dev = &client->dev;
>>  	struct i2c_adapter *adapter = to_i2c_adapter(dev->parent);
>>  	struct lm90_data *data;
>> +	struct node_args np_args;
>>  	int err;
>>  
>>  	data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
>> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client,
>>  				       "lm90", data);
>>  		if (err < 0) {
>>  			dev_err(dev, "cannot request interrupt\n");
>> -			goto exit_remove_files;
>> +			goto exit_unregister_hwmon;
>>  		}
>>  	}
>>  
>> +	np_args.np = dev->of_node;
>> +	np_args.index = 0;
>> +	data->ts_remote = thermal_sensor_register("lm90_remote",
>> +						LM90_NUM_THRESHOLDS,
>> +						&np_args,
>> +						&remote_ops, client);
>> +	if (IS_ERR(data->ts_remote)) {
>> +		dev_err(dev, "cannot register sensor to thermal framework\n");
>> +		err = -EINVAL;
>> +		goto exit_unregister_hwmon;
>> +	}
>> +
>> +	np_args.index = 1;
>> +	data->ts_local = thermal_sensor_register("lm90_local",
>> +						LM90_NUM_THRESHOLDS,
>> +						&np_args,
>> +						&local_ops, client);
>> +
>> +	if (IS_ERR(data->ts_local)) {
>> +		dev_err(dev, "cannot register sensor to thermal framework\n");
>> +		err = -EINVAL;
>> +		goto exit_unregister_hwmon;
>> +	}
>> +
> 
> How about the second remote sensor ? Granted, not all chips supported by this
> driver have it, but if we are adding this we might as well add it for all
> sensors on all chips.

Ok, I will improve it.

> 
>>  	return 0;
>>  
>> +exit_unregister_hwmon:
>> +	hwmon_device_unregister(data->hwmon_dev);
>>  exit_remove_files:
>>  	lm90_remove_files(client, data);
>>  exit_restore:
>> @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client)
>>  	struct lm90_data *data = i2c_get_clientdata(client);
>>  
>>  	free_irq(client->irq, data);
>> +	thermal_sensor_unregister(data->ts_remote);
>> +	thermal_sensor_unregister(data->ts_local);
>>  	hwmon_device_unregister(data->hwmon_dev);
>>  	lm90_remove_files(client, data);
>>  	lm90_restore_conf(client, data);
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> lm-sensors mailing list
>> lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
>> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>>

Re: [RFC PATCH 6/9] hwmon: (lm90) Register to the thermal framework

[Alex Courbot]

On 02/18/2013 08:30 PM, Wei Ni wrote:
> Register the remote sensor to the thermal framework.
> It can support to show the temperature and read/write threshold.
>
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>   arch/arm/boot/dts/tegra30-cardhu.dtsi |    1 +
>   drivers/hwmon/lm90.c                  |  182 ++++++++++++++++++++++++++++++++-
>   2 files changed, 182 insertions(+), 1 deletion(-)

Making changes to a driver *and* a board file in the same patch? I think 
this should be separated, and the board file change preferably squashed 
with the first patch of this series, and moved right after this one.

>
> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> index 15ad1ad..3f6ab89 100644
> --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
> +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> @@ -279,6 +279,7 @@
>   			reg = <0x4c>;
>   			interrupt-parent = <&gpio>;
>   			interrupts = <226 0x08>; /* gpio PCC2 */
> +			#sensor-cells = <1>;
>   		};
>   	};
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index de5a476..0abdedc 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -91,6 +91,7 @@
>   #include <linux/sysfs.h>
>   #include <linux/interrupt.h>
>   #include <linux/of_irq.h>
> +#include <linux/thermal.h>
>
>   /*
>    * Addresses to scan
> @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>   #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>
>   /*
> + * Thermal framework
> + */
> +enum lm90_thresholds {
> +	LM90_LOW_THRESHOLDS = 0,	/* threshold 0: low limits */
> +	LM90_HIGH_THRESHOLDS,		/* threshold 1: high limits */
> +	LM90_NUM_THRESHOLDS
> +};
> +
> +/*
>    * Driver data (common to all clients)
>    */
>
> @@ -377,6 +387,9 @@ struct lm90_data {
>   	s16 temp11[TEMP11_REG_NUM];
>   	u8 temp_hyst;
>   	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
> +
> +	struct thermal_sensor *ts_remote;
> +	struct thermal_sensor *ts_local;
>   };
>
>   /*
> @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id)
>   	return IRQ_HANDLED;
>   }
>
> +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +
> +	_show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp);

As Guenter pointed, this might break. Since you introduced _show_temp11 
in a previous patch, you should revise it to take a long * as third 
argument (or even better, return a long). Or if you cannot do that for 
some reason, use a temporary int and affect temp properly (*temp = 
temp_int).

> +
> +	return 0;
> +}
> +
> +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index,
> +					long *val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* remote low limit */
> +		index = TEMP11_REMOTE_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* remote high limit */
> +		index = TEMP11_REMOTE_HIGH;
> +		break;
> +	default:
> +		dev_err(dev, "read remote threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_show_temp11(dev, index, (int *)val);
> +
> +	return 0;
> +}
> +
> +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index,
> +					long val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int nr, index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* remote low limit */
> +		nr = NR_CHAN_0_REMOTE_LOW;
> +		index = TEMP11_REMOTE_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* remote high limit */
> +		nr = NR_CHAN_0_REMOTE_HIGH;
> +		index = TEMP11_REMOTE_HIGH;
> +		break;
> +	default:
> +		dev_err(dev, "write remote threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_set_temp11(dev, nr, index, val);
> +
> +	return 0;
> +}
> +
> +static struct thermal_sensor_ops remote_ops = {
> +	.get_temp = lm90_read_remote_temp,
> +	.get_threshold = lm90_read_remote_threshold,
> +	.set_threshold = lm90_write_remote_threshold,
> +};
> +
> +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +
> +	_show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp);
> +
> +	return 0;
> +}
> +
> +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index,
> +					long *val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* local low limit */
> +		index = TEMP8_LOCAL_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* local high limit */
> +		index = TEMP8_LOCAL_HIGH;
> +		break;

I think the comments are unneeded here, the macro name should be 
explicit enough.

> +	default:
> +		dev_err(dev, "read local threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_show_temp8(dev, index, (int *)val);
> +
> +	return 0;
> +}
> +
> +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index,
> +					long val)
> +{
> +	struct i2c_client *client = ts->devdata;
> +	struct device *dev = &client->dev;
> +	int index;
> +
> +	switch (th_index) {
> +	case LM90_LOW_THRESHOLDS:
> +		/* local low limit */
> +		index = TEMP8_LOCAL_LOW;
> +		break;
> +	case LM90_HIGH_THRESHOLDS:
> +		/* local high limit */
> +		index = TEMP8_LOCAL_HIGH;
> +		break;
> +	default:
> +		dev_err(dev, "write local threshold failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	_set_temp8(dev, index, val);
> +
> +	return 0;
> +}
> +
> +static struct thermal_sensor_ops local_ops = {
> +	.get_temp = lm90_read_local_temp,
> +	.get_threshold = lm90_read_local_threshold,
> +	.set_threshold = lm90_write_local_threshold,
> +};
> +
>   static int lm90_probe(struct i2c_client *client,
>   		      const struct i2c_device_id *id)
>   {
>   	struct device *dev = &client->dev;
>   	struct i2c_adapter *adapter = to_i2c_adapter(dev->parent);
>   	struct lm90_data *data;
> +	struct node_args np_args;
>   	int err;
>
>   	data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client,
>   				       "lm90", data);
>   		if (err < 0) {
>   			dev_err(dev, "cannot request interrupt\n");
> -			goto exit_remove_files;
> +			goto exit_unregister_hwmon;
>   		}
>   	}
>
> +	np_args.np = dev->of_node;
> +	np_args.index = 0;
> +	data->ts_remote = thermal_sensor_register("lm90_remote",
> +						LM90_NUM_THRESHOLDS,
> +						&np_args,
> +						&remote_ops, client);
> +	if (IS_ERR(data->ts_remote)) {
> +		dev_err(dev, "cannot register sensor to thermal framework\n");
> +		err = -EINVAL;

When don't you return the error code provided by 
thermal_sensor_register, e.g. err = PTR_ERR(data->ts_remote) ?

> +		goto exit_unregister_hwmon;
> +	}
> +
> +	np_args.index = 1;
> +	data->ts_local = thermal_sensor_register("lm90_local",
> +						LM90_NUM_THRESHOLDS,
> +						&np_args,
> +						&local_ops, client);
> +
> +	if (IS_ERR(data->ts_local)) {
> +		dev_err(dev, "cannot register sensor to thermal framework\n");
> +		err = -EINVAL;

Same thing here.

> +		goto exit_unregister_hwmon;
> +	}
> +
>   	return 0;
>
> +exit_unregister_hwmon:
> +	hwmon_device_unregister(data->hwmon_dev);
>   exit_remove_files:
>   	lm90_remove_files(client, data);
>   exit_restore:
> @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client)
>   	struct lm90_data *data = i2c_get_clientdata(client);
>
>   	free_irq(client->irq, data);
> +	thermal_sensor_unregister(data->ts_remote);
> +	thermal_sensor_unregister(data->ts_local);

Ideally you would unregister your sensors in the reverse order they have 
been registered, but I'm being picky here.

Alex.

Re: [RFC PATCH 6/9] hwmon: (lm90) Register to the thermal framework

[Wei Ni]

On 02/19/2013 01:22 PM, Alex Courbot wrote:
> On 02/18/2013 08:30 PM, Wei Ni wrote:
>> Register the remote sensor to the thermal framework.
>> It can support to show the temperature and read/write threshold.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>   arch/arm/boot/dts/tegra30-cardhu.dtsi |    1 +
>>   drivers/hwmon/lm90.c                  |  182 ++++++++++++++++++++++++++++++++-
>>   2 files changed, 182 insertions(+), 1 deletion(-)
> 
> Making changes to a driver *and* a board file in the same patch? I think 
> this should be separated, and the board file change preferably squashed 
> with the first patch of this series, and moved right after this one.
> 
>>
>> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
>> index 15ad1ad..3f6ab89 100644
>> --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
>> +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
>> @@ -279,6 +279,7 @@
>>   			reg = <0x4c>;
>>   			interrupt-parent = <&gpio>;
>>   			interrupts = <226 0x08>; /* gpio PCC2 */
>> +			#sensor-cells = <1>;
>>   		};
>>   	};
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index de5a476..0abdedc 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -91,6 +91,7 @@
>>   #include <linux/sysfs.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/of_irq.h>
>> +#include <linux/thermal.h>
>>
>>   /*
>>    * Addresses to scan
>> @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>>   #define LM90_HAVE_BROKEN_ALERT	(1 << 7) /* Broken alert		*/
>>
>>   /*
>> + * Thermal framework
>> + */
>> +enum lm90_thresholds {
>> +	LM90_LOW_THRESHOLDS = 0,	/* threshold 0: low limits */
>> +	LM90_HIGH_THRESHOLDS,		/* threshold 1: high limits */
>> +	LM90_NUM_THRESHOLDS
>> +};
>> +
>> +/*
>>    * Driver data (common to all clients)
>>    */
>>
>> @@ -377,6 +387,9 @@ struct lm90_data {
>>   	s16 temp11[TEMP11_REG_NUM];
>>   	u8 temp_hyst;
>>   	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
>> +
>> +	struct thermal_sensor *ts_remote;
>> +	struct thermal_sensor *ts_local;
>>   };
>>
>>   /*
>> @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id)
>>   	return IRQ_HANDLED;
>>   }
>>
>> +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +
>> +	_show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp);
> 
> As Guenter pointed, this might break. Since you introduced _show_temp11 
> in a previous patch, you should revise it to take a long * as third 
> argument (or even better, return a long). Or if you cannot do that for 
> some reason, use a temporary int and affect temp properly (*temp = 
> temp_int).

yes, the pointer will cause problems here.
I will follow Guenter suggestion to return the value simply for
_show_temp11 and _show_temp8

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index,
>> +					long *val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* remote low limit */
>> +		index = TEMP11_REMOTE_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* remote high limit */
>> +		index = TEMP11_REMOTE_HIGH;
>> +		break;
>> +	default:
>> +		dev_err(dev, "read remote threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_show_temp11(dev, index, (int *)val);
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index,
>> +					long val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int nr, index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* remote low limit */
>> +		nr = NR_CHAN_0_REMOTE_LOW;
>> +		index = TEMP11_REMOTE_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* remote high limit */
>> +		nr = NR_CHAN_0_REMOTE_HIGH;
>> +		index = TEMP11_REMOTE_HIGH;
>> +		break;
>> +	default:
>> +		dev_err(dev, "write remote threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_set_temp11(dev, nr, index, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_sensor_ops remote_ops = {
>> +	.get_temp = lm90_read_remote_temp,
>> +	.get_threshold = lm90_read_remote_threshold,
>> +	.set_threshold = lm90_write_remote_threshold,
>> +};
>> +
>> +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +
>> +	_show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp);
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index,
>> +					long *val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* local low limit */
>> +		index = TEMP8_LOCAL_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* local high limit */
>> +		index = TEMP8_LOCAL_HIGH;
>> +		break;
> 
> I think the comments are unneeded here, the macro name should be 
> explicit enough.

OK.

> 
>> +	default:
>> +		dev_err(dev, "read local threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_show_temp8(dev, index, (int *)val);
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index,
>> +					long val)
>> +{
>> +	struct i2c_client *client = ts->devdata;
>> +	struct device *dev = &client->dev;
>> +	int index;
>> +
>> +	switch (th_index) {
>> +	case LM90_LOW_THRESHOLDS:
>> +		/* local low limit */
>> +		index = TEMP8_LOCAL_LOW;
>> +		break;
>> +	case LM90_HIGH_THRESHOLDS:
>> +		/* local high limit */
>> +		index = TEMP8_LOCAL_HIGH;
>> +		break;
>> +	default:
>> +		dev_err(dev, "write local threshold failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	_set_temp8(dev, index, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_sensor_ops local_ops = {
>> +	.get_temp = lm90_read_local_temp,
>> +	.get_threshold = lm90_read_local_threshold,
>> +	.set_threshold = lm90_write_local_threshold,
>> +};
>> +
>>   static int lm90_probe(struct i2c_client *client,
>>   		      const struct i2c_device_id *id)
>>   {
>>   	struct device *dev = &client->dev;
>>   	struct i2c_adapter *adapter = to_i2c_adapter(dev->parent);
>>   	struct lm90_data *data;
>> +	struct node_args np_args;
>>   	int err;
>>
>>   	data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL);
>> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client,
>>   				       "lm90", data);
>>   		if (err < 0) {
>>   			dev_err(dev, "cannot request interrupt\n");
>> -			goto exit_remove_files;
>> +			goto exit_unregister_hwmon;
>>   		}
>>   	}
>>
>> +	np_args.np = dev->of_node;
>> +	np_args.index = 0;
>> +	data->ts_remote = thermal_sensor_register("lm90_remote",
>> +						LM90_NUM_THRESHOLDS,
>> +						&np_args,
>> +						&remote_ops, client);
>> +	if (IS_ERR(data->ts_remote)) {
>> +		dev_err(dev, "cannot register sensor to thermal framework\n");
>> +		err = -EINVAL;
> 
> When don't you return the error code provided by 
> thermal_sensor_register, e.g. err = PTR_ERR(data->ts_remote) ?

I didn't consider it, I will change it.

> 
>> +		goto exit_unregister_hwmon;
>> +	}
>> +
>> +	np_args.index = 1;
>> +	data->ts_local = thermal_sensor_register("lm90_local",
>> +						LM90_NUM_THRESHOLDS,
>> +						&np_args,
>> +						&local_ops, client);
>> +
>> +	if (IS_ERR(data->ts_local)) {
>> +		dev_err(dev, "cannot register sensor to thermal framework\n");
>> +		err = -EINVAL;
> 
> Same thing here.
> 
>> +		goto exit_unregister_hwmon;
>> +	}
>> +
>>   	return 0;
>>
>> +exit_unregister_hwmon:
>> +	hwmon_device_unregister(data->hwmon_dev);
>>   exit_remove_files:
>>   	lm90_remove_files(client, data);
>>   exit_restore:
>> @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client)
>>   	struct lm90_data *data = i2c_get_clientdata(client);
>>
>>   	free_irq(client->irq, data);
>> +	thermal_sensor_unregister(data->ts_remote);
>> +	thermal_sensor_unregister(data->ts_local);
> 
> Ideally you would unregister your sensors in the reverse order they have 
> been registered, but I'm being picky here.

Yes, it's better in reverse order.
I really appreciate you reviewing my patches so carefully :)

Wei.

> 
> Alex.
> 

Re: [RFC PATCH 6/9] hwmon: (lm90) Register to the thermal framework

[Stephen Warren]

On 02/18/2013 04:30 AM, Wei Ni wrote:
> Register the remote sensor to the thermal framework.
> It can support to show the temperature and read/write threshold.

>  arch/arm/boot/dts/tegra30-cardhu.dtsi |    1 +
>  drivers/hwmon/lm90.c                  |  182 ++++++++++++++++++++++++++++++++-

There shouldn't be any need to edit both those files in the same patch.
This patch is making multiple separate logical changes, which should be
separate patches.

> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi

> +			#sensor-cells = <1>;

Again, in order to allow that, you need some documentation in
Documentation/devicetree/bindings/. Related to that, you need to Cc the
devicetree maintainers on at least the patches that add that documentation.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c

> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client,
...
> +	np_args.np = dev->of_node;
> +	np_args.index = 0;

As I mentioned before, that'd be better replaced by an explicit of_xlate
function.

Re: [RFC PATCH 6/9] hwmon: (lm90) Register to the thermal framework

[Wei Ni]

On 02/20/2013 07:18 AM, Stephen Warren wrote:
> On 02/18/2013 04:30 AM, Wei Ni wrote:
>> Register the remote sensor to the thermal framework.
>> It can support to show the temperature and read/write threshold.
> 
>>  arch/arm/boot/dts/tegra30-cardhu.dtsi |    1 +
>>  drivers/hwmon/lm90.c                  |  182 ++++++++++++++++++++++++++++++++-
> 
> There shouldn't be any need to edit both those files in the same patch.
> This patch is making multiple separate logical changes, which should be
> separate patches.

I will follow Matthew's suggestion to split this serial patch into 3
separate sets.

> 
>> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> 
>> +			#sensor-cells = <1>;
> 
> Again, in order to allow that, you need some documentation in
> Documentation/devicetree/bindings/. Related to that, you need to Cc the
> devicetree maintainers on at least the patches that add that documentation.

Ok, I will do it.

> 
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> 
>> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client,
> ...
>> +	np_args.np = dev->of_node;
>> +	np_args.index = 0;
> 
> As I mentioned before, that'd be better replaced by an explicit of_xlate
> function.

Yes, I will try to use this func.

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

[RFC PATCH 8/9] ARM: dt: t30 cardhu: add dt entry for thermal driver

[Wei Ni]

Enable thermal driver in the dts file.
Set sensor as lm90 remote sensor, and set throttle data.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/thermal/tegra3-thermal.txt |   41 ++++++++++++++++++++
 arch/arm/boot/dts/tegra30-cardhu.dtsi              |   19 +++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/tegra3-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt
new file mode 100644
index 0000000..dc3f922
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt
@@ -0,0 +1,41 @@
+* Nvidia Tegra30 Thermal
+
+** Thermal node properties:
+
+- compatible : "nvidia,tegra30-thermal";
+- sensors: the sensor device node which we want to use in the thermal zone,
+  the arguments is the index of the sensor in sensor device node;
+- passive-delay: passive delay;
+- num-passive-trips : number of passive trip points, this is required, set
+  it 0 if none, if greater than 0, the following properties must be defined;
+- passive-trips : temperature of passive trip points;
+- num-active-trips: number of active trip points, this is required, set
+  it 0 if none, if greater than 0, the following properties must be defined;
+- active-trips: temperature of active trip points;
+- throt-tab-size: size of the throttle table, it's the max cooling state.
+- throt-tab: throttle table. the cooling state will be defined according to
+  this table.
+
+Usually these properties are separated in board related dts files.
+
+Example:
+thermal {
+	compatible = "nvidia,tegra30-thermal";
+	sensors = <&nct1008 0>;
+	passive-delay = <2000>;
+	num-passive-trips = <3>;
+	passive-trips = <70 80 90>;
+	num-active-trips = <4>;
+	active-trips = < 60 70 80 90>;
+	throt-tab-size = <10>;
+	throt-tab = <0 1000
+		640000 1000
+		640000 1000
+		640000 1000
+		640000 1000
+		640000 1000
+		760000 1050
+		760000 1050
+		1000000 1050
+		1000000 1050>;
+};
diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index 3f6ab89..9748b9b 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -492,4 +492,23 @@
 		nvidia,spkr-en-gpios = <&wm8903 2 0>;
 		nvidia,hp-det-gpios = <&gpio 178 0>; /* gpio PW2 */
 	};
+
+	thermal {
+		compatible = "nvidia,tegra30-thermal";
+		sensors = <&nct1008 0>;
+		passive-delay = <2000>;
+		num-passive-trips = <1>;
+		passive-trips = <80>;
+		throt-tab-size = <10>;
+		throt-tab = <0 1000
+			640000 1000
+			640000 1000
+			640000 1000
+			640000 1000
+			640000 1000
+			760000 1050
+			760000 1050
+			1000000 1050
+			1000000 1050>;
+	};
 };
-- 
1.7.9.5

Re: [RFC PATCH 8/9] ARM: dt: t30 cardhu: add dt entry for thermal driver

[Alex Courbot]

On 02/18/2013 08:30 PM, Wei Ni wrote:
> diff --git a/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt
> new file mode 100644

This should go with the previous patch (which introduces the driver) 
instead of this one. Bindings should be documented with their driver.

Alex.

Re: [RFC PATCH 8/9] ARM: dt: t30 cardhu: add dt entry for thermal driver

[Wei Ni]

On 02/19/2013 11:42 AM, Alex Courbot wrote:
> On 02/18/2013 08:30 PM, Wei Ni wrote:
>> diff --git a/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt
>> new file mode 100644
> 
> This should go with the previous patch (which introduces the driver) 
> instead of this one. Bindings should be documented with their driver.

Ok, I will change it.

Wei.

> 
> Alex.
> 

Re: [RFC PATCH 8/9] ARM: dt: t30 cardhu: add dt entry for thermal driver

[Stephen Warren]

On 02/18/2013 04:30 AM, Wei Ni wrote:
> Enable thermal driver in the dts file.
> Set sensor as lm90 remote sensor, and set throttle data.

> diff --git a/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt

The DT documentation should be either a separate patch before the one
which implements it in the driver, or part of the driver patch. It
shouldn't be part of the patch which instantiates the binding in the
board DT file.

> +* Nvidia Tegra30 Thermal

NVIDIA is all capitals.

It seems like a word is missing: Thermal Management? Control/Zone?

> +** Thermal node properties:
> +
> +- compatible : "nvidia,tegra30-thermal";
> +- sensors: the sensor device node which we want to use in the thermal zone,
> +  the arguments is the index of the sensor in sensor device node;

The arguments should be defined by the DT binding documentation for that
referenced device, not by the referencing device.

DT properties that are custom defined for this binding should include
the vendor prefix in their name. So, that'd be "nvidia,sensors". This
may apply less if a "sensors" becomes a more generic standard, but it
surely applies to all the other properties below.

> +- passive-delay: passive delay;

What does this mean?

> +- num-passive-trips : number of passive trip points, this is required, set
> +  it 0 if none, if greater than 0, the following properties must be defined;
> +- passive-trips : temperature of passive trip points;

Presumably "num-passive-trips" can be calculated simply by counting the
number of entries in "passive-trips"?

Please describe what passive and active trips are; someone who just
reads this binding document (perhaps in conjunction with some HW
documentation and/or other binding documentation) should be able to fill
in this binding without other knowledge.

> +- num-active-trips: number of active trip points, this is required, set
> +  it 0 if none, if greater than 0, the following properties must be defined;
> +- active-trips: temperature of active trip points;

Both comments above apply here too.

> +- throt-tab-size: size of the throttle table, it's the max cooling state.
> +- throt-tab: throttle table. the cooling state will be defined according to
> +  this table.

Both comments above apply here too.

Also, what units are used for all these properties?

Judging by the example below, this property is a list of tuples. The
meaning of each field in the tuple needs to be explained.

What happens when the CPU/SoC needs to be throttled? Must some clock or
voltage be lowered/limited? If so, you need properties that indicate
which clock/voltage/... needs to be acted upon.

> +Usually these properties are separated in board related dts files.

That's not really relevant.

Re: [RFC PATCH 8/9] ARM: dt: t30 cardhu: add dt entry for thermal driver

[Wei Ni]

On 02/20/2013 07:28 AM, Stephen Warren wrote:
> On 02/18/2013 04:30 AM, Wei Ni wrote:
>> Enable thermal driver in the dts file.
>> Set sensor as lm90 remote sensor, and set throttle data.
> 
>> diff --git a/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt
> 
> The DT documentation should be either a separate patch before the one
> which implements it in the driver, or part of the driver patch. It
> shouldn't be part of the patch which instantiates the binding in the
> board DT file.

Ok, I will do it.

> 
>> +* Nvidia Tegra30 Thermal
> 
> NVIDIA is all capitals.

Got it.

> 
> It seems like a word is missing: Thermal Management? Control/Zone?

It's better to use "NVIDIA Tegra30 Thermal Zone".

> 
>> +** Thermal node properties:
>> +
>> +- compatible : "nvidia,tegra30-thermal";
>> +- sensors: the sensor device node which we want to use in the thermal zone,
>> +  the arguments is the index of the sensor in sensor device node;
> 
> The arguments should be defined by the DT binding documentation for that
> referenced device, not by the referencing device.
> 
> DT properties that are custom defined for this binding should include
> the vendor prefix in their name. So, that'd be "nvidia,sensors". This
> may apply less if a "sensors" becomes a more generic standard, but it
> surely applies to all the other properties below.

So these properties should be introduced in the DT binding documentation
of the thermal framework.

> 
>> +- passive-delay: passive delay;
> 
> What does this mean?

This is used for the thermal framework. As you said, this should be
introduced with the framework.

> 
>> +- num-passive-trips : number of passive trip points, this is required, set
>> +  it 0 if none, if greater than 0, the following properties must be defined;
>> +- passive-trips : temperature of passive trip points;
> 
> Presumably "num-passive-trips" can be calculated simply by counting the
> number of entries in "passive-trips"?

Oh, yes, I can remove it.

> 
> Please describe what passive and active trips are; someone who just
> reads this binding document (perhaps in conjunction with some HW
> documentation and/or other binding documentation) should be able to fill
> in this binding without other knowledge.

Durgadoss's patches introduced these value in the framework
documentation, but he didn't touch the DT. So I will add it.

> 
>> +- num-active-trips: number of active trip points, this is required, set
>> +  it 0 if none, if greater than 0, the following properties must be defined;
>> +- active-trips: temperature of active trip points;
> 
> Both comments above apply here too.
> 
>> +- throt-tab-size: size of the throttle table, it's the max cooling state.
>> +- throt-tab: throttle table. the cooling state will be defined according to
>> +  this table.
> 
> Both comments above apply here too.
> 
> Also, what units are used for all these properties?

they use "int".
And according to our downstream dvfs driver, the throttle table use
"unsigned long".

> 
> Judging by the example below, this property is a list of tuples. The
> meaning of each field in the tuple needs to be explained.
> 
> What happens when the CPU/SoC needs to be throttled? Must some clock or
> voltage be lowered/limited? If so, you need properties that indicate
> which clock/voltage/... needs to be acted upon.

This table which will be used by our DVFS driver, although it didn't be
upstreamed yet. The table set clock frequency limited value with
different cooling state. When the temperature touch the different
limited value, we will set difficult cooling state, find the limited
freq from this table and pass to the dvfs driver.
I think may be this table should be set for cooling device node. This
table is only for our tegra dvfs, so I think we can parse this table in
the tegra3_cooling.c, which will be the new driver for cooling device.

> 
>> +Usually these properties are separated in board related dts files.
> 
> That's not really relevant.

Ok.

> 

Re: [RFC PATCH 8/9] ARM: dt: t30 cardhu: add dt entry for thermal driver

[Stephen Warren]

On 02/20/2013 04:53 AM, Wei Ni wrote:
> On 02/20/2013 07:28 AM, Stephen Warren wrote:
>> On 02/18/2013 04:30 AM, Wei Ni wrote:
>>> Enable thermal driver in the dts file.
>>> Set sensor as lm90 remote sensor, and set throttle data.
>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt

>> Also, what units are used for all these properties?
> 
> they use "int".
> And according to our downstream dvfs driver, the throttle table use
> "unsigned long".

int and unsigned long are types, not units. Units means degrees C vs.
degrees F vs. degrees K, or Hz vs. KHz vs. MHz, etc.

>> Judging by the example below, this property is a list of tuples. The
>> meaning of each field in the tuple needs to be explained.
>>
>> What happens when the CPU/SoC needs to be throttled? Must some clock or
>> voltage be lowered/limited? If so, you need properties that indicate
>> which clock/voltage/... needs to be acted upon.
> 
> This table which will be used by our DVFS driver, although it didn't be
> upstreamed yet. The table set clock frequency limited value with
> different cooling state. When the temperature touch the different
> limited value, we will set difficult cooling state, find the limited
> freq from this table and pass to the dvfs driver.
> I think may be this table should be set for cooling device node. This
> table is only for our tegra dvfs, so I think we can parse this table in
> the tegra3_cooling.c, which will be the new driver for cooling device.

Note that I expect the answers to these questions (pretty much all the
questions I asked in response to your patch) to be included in the DT
binding documentation, not just in email.

[RFC PATCH 9/9] ARM: tegra: defconfig: enable thermal framework

[Wei Ni]

Enable tegra30 thermal framework.
New options enabled:
* TEGRA30_THERMAL: for tegra30 thermal framework.
* THERMAL: dependency for TEGRA30_THERMAL.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/configs/tegra_defconfig |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index a7827fd..4aa3356 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -128,6 +128,8 @@ CONFIG_GPIO_TPS65910=y
 CONFIG_POWER_SUPPLY=y
 CONFIG_BATTERY_SBS=y
 CONFIG_SENSORS_LM90=y
+CONFIG_THERMAL=y
+CONFIG_TEGRA30_THERMAL=y
 CONFIG_MFD_TPS6586X=y
 CONFIG_MFD_TPS65910=y
 CONFIG_MFD_MAX8907=y
-- 
1.7.9.5

[RFC PATCH 2/9] 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 |  117 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 72 insertions(+), 45 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 863412a..caf01b0 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -702,29 +702,35 @@ 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 void _show_temp8(struct device *dev, int index, int *temp)
 {
-	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)
-		temp += 16000;
+	if (data->kind == lm99 && index == 3)
+		*temp += 16000;
+
+}
+
+static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
+			  char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int temp;
+
+	_show_temp8(dev, attr->index, &temp);
 
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static void _set_temp8(struct device *dev, int index, long val)
 {
 	static const u8 reg[8] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -737,60 +743,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;
+
+	_set_temp8(dev, index, val);
+
 	return count;
 }
 
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
-			   char *buf)
+static void _show_temp11(struct device *dev, int index, int *temp)
 {
-	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)
-		temp += 16000;
+	if (data->kind == lm99 &&  index <= 2)
+		*temp += 16000;
+}
+
+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);
+	int temp;
+
+	_show_temp11(dev, attr->index, &temp);
 
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-			  const char *buf, size_t count)
+static void _set_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
 		u8 high;
@@ -804,17 +823,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 +849,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;
+
+	_set_temp11(dev, nr, index, val);
+
 	return count;
 }
 
-- 
1.7.9.5

RE: [RFC PATCH 2/9] hwmon: (lm90) split set&show temp as common codes

[Matthew Longnecker]

Wei,

This set of patches seems to include 3 logical subsets
1) lm90 changes
2) dt support in drivers/thermal
3) tegra30 support in drivers/thermal

Would it make sense to split this into 3 separate sets of changes?

-Matt Longnecker

nvpublic

Re: [RFC PATCH 2/9] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 02/19/2013 06:29 AM, Matthew Longnecker wrote:
> Wei,
> 
> This set of patches seems to include 3 logical subsets
> 1) lm90 changes
> 2) dt support in drivers/thermal
> 3) tegra30 support in drivers/thermal
> 
> Would it make sense to split this into 3 separate sets of changes?

Ok, I will do it in my next version.

> 
> -Matt Longnecker
> 
> nvpublic
> 

Re: [lm-sensors] [RFC PATCH 2/9] hwmon: (lm90) split set&show temp as common codes

[Guenter Roeck]

On Mon, Feb 18, 2013 at 07:30:24PM +0800, Wei Ni wrote:
> 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 |  117 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 72 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 863412a..caf01b0 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -702,29 +702,35 @@ 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 void _show_temp8(struct device *dev, int index, int *temp)

Why do the new functions not simply return the value ? 
Void return and returning the value in a pointer doesn't make much sense to me.

Guenter

>  {
> -	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)
> -		temp += 16000;
> +	if (data->kind == lm99 && index == 3)
> +		*temp += 16000;
> +
> +}
> +
> +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> +			  char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	int temp;
> +
> +	_show_temp8(dev, attr->index, &temp);
>  
>  	return sprintf(buf, "%d\n", temp);
>  }
>  
> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> +static void _set_temp8(struct device *dev, int index, long val)
>  {
>  	static const u8 reg[8] = {
>  		LM90_REG_W_LOCAL_LOW,
> @@ -737,60 +743,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;
> +
> +	_set_temp8(dev, index, val);
> +
>  	return count;
>  }
>  
> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> -			   char *buf)
> +static void _show_temp11(struct device *dev, int index, int *temp)
>  {
> -	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)
> -		temp += 16000;
> +	if (data->kind == lm99 &&  index <= 2)
> +		*temp += 16000;
> +}
> +
> +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);
> +	int temp;
> +
> +	_show_temp11(dev, attr->index, &temp);
>  
>  	return sprintf(buf, "%d\n", temp);
>  }
>  
> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> -			  const char *buf, size_t count)
> +static void _set_temp11(struct device *dev, int nr, int index, long val)
>  {
>  	struct {
>  		u8 high;
> @@ -804,17 +823,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 +849,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;
> +
> +	_set_temp11(dev, nr, index, val);
> +
>  	return count;
>  }
>  
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> 

Re: [lm-sensors] [RFC PATCH 2/9] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 02/19/2013 11:31 AM, Guenter Roeck wrote:
> On Mon, Feb 18, 2013 at 07:30:24PM +0800, Wei Ni wrote:
>> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/hwmon/lm90.c |  117 +++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 72 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 863412a..caf01b0 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -702,29 +702,35 @@ 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 void _show_temp8(struct device *dev, int index, int *temp)
> 
> Why do the new functions not simply return the value ? 
> Void return and returning the value in a pointer doesn't make much sense to me.

Yes, you are right, it's better to return value simply.
I will change it in my next version.

Wei.

> 
> Guenter
> 
>>  {
>> -	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)
>> -		temp += 16000;
>> +	if (data->kind == lm99 && index == 3)
>> +		*temp += 16000;
>> +
>> +}
>> +
>> +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>> +			  char *buf)
>> +{
>> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +	int temp;
>> +
>> +	_show_temp8(dev, attr->index, &temp);
>>  
>>  	return sprintf(buf, "%d\n", temp);
>>  }
>>  
>> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> -			 const char *buf, size_t count)
>> +static void _set_temp8(struct device *dev, int index, long val)
>>  {
>>  	static const u8 reg[8] = {
>>  		LM90_REG_W_LOCAL_LOW,
>> @@ -737,60 +743,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;
>> +
>> +	_set_temp8(dev, index, val);
>> +
>>  	return count;
>>  }
>>  
>> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>> -			   char *buf)
>> +static void _show_temp11(struct device *dev, int index, int *temp)
>>  {
>> -	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)
>> -		temp += 16000;
>> +	if (data->kind == lm99 &&  index <= 2)
>> +		*temp += 16000;
>> +}
>> +
>> +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);
>> +	int temp;
>> +
>> +	_show_temp11(dev, attr->index, &temp);
>>  
>>  	return sprintf(buf, "%d\n", temp);
>>  }
>>  
>> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>> -			  const char *buf, size_t count)
>> +static void _set_temp11(struct device *dev, int nr, int index, long val)
>>  {
>>  	struct {
>>  		u8 high;
>> @@ -804,17 +823,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 +849,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;
>> +
>> +	_set_temp11(dev, nr, index, val);
>> +
>>  	return count;
>>  }
>>  
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> lm-sensors mailing list
>> lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
>> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>>

Re: [RFC PATCH 2/9] hwmon: (lm90) split set&show temp as common codes

[Stephen Warren]

On 02/18/2013 04:30 AM, Wei Ni wrote:
> Split set&show temp codes as common functions, so we can use it directly when
> implement linux thermal framework.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c

> -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> -			  char *buf)
> +static void _show_temp8(struct device *dev, int index, int *temp)

This function now seems to be "get" not "show". If you rename it to e.g.
"get_temp8", then you won't need to use an _ in the function name. If
that name causes conflicts with later patches, perhaps "read_temp8".

> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> +static void _set_temp8(struct device *dev, int index, long val)

Similarly, perhaps "write_temp8"?

Similar comments for the rest of the patch.

Re: [RFC PATCH 2/9] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 02/20/2013 06:56 AM, Stephen Warren wrote:
> On 02/18/2013 04:30 AM, Wei Ni wrote:
>> Split set&show temp codes as common functions, so we can use it directly when
>> implement linux thermal framework.
> 
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> 
>> -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>> -			  char *buf)
>> +static void _show_temp8(struct device *dev, int index, int *temp)
> 
> This function now seems to be "get" not "show". If you rename it to e.g.
> "get_temp8", then you won't need to use an _ in the function name. If
> that name causes conflicts with later patches, perhaps "read_temp8".

Yes, it looks "read_" is better.
I will change it.

> 
>> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> -			 const char *buf, size_t count)
>> +static void _set_temp8(struct device *dev, int index, long val)
> 
> Similarly, perhaps "write_temp8"?
> 
> Similar comments for the rest of the patch.
> 

[RFC PATCH 4/9] hwmon: (lm90) use macros for the indexes of temp8 and temp11

[Wei Ni]

Using macros for the indexes and nrs of temp8 and temp11.
This make the code much clearer.

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 80311ef..de5a476 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -298,6 +298,59 @@ static const struct lm90_params lm90_params[] = {
 };
 
 /*
+ * TEMP8 register index
+ */
+enum lm90_temp8_reg_index {
+	TEMP8_LOCAL_LOW = 0,	/* 0: local low limit */
+	TEMP8_LOCAL_HIGH,	/* 1: local high limit */
+	TEMP8_LOCAL_CRIT,	/* 2: local critical limit */
+	TEMP8_REMOTE_CRIT,	/* 3: remote critical limit */
+	TEMP8_LOCAL_EMERG,	/* 4: local emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE_EMERG,	/* 5: remote emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE2_CRIT,	/* 6: remote 2 critical limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REMOTE2_EMERG,	/* 7: remote 2 emergency limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REG_NUM
+};
+
+/*
+ * TEMP11 register index
+ */
+enum lm90_temp11_reg_index {
+	TEMP11_REMOTE_TEMP = 0,	/* 0: remote input */
+	TEMP11_REMOTE_LOW,	/* 1: remote low limit */
+	TEMP11_REMOTE_HIGH,	/* 2: remote high limit */
+	TEMP11_REMOTE_OFFSET,	/* 3: remote offset
+				 * (except max6646, max6657/58/59,
+				 *  and max6695/96)
+				 */
+	TEMP11_LOCAL_TEMP,	/* 4: local input */
+	TEMP11_REMOTE2_TEMP,	/* 5: remote 2 input (max6695/96 only) */
+	TEMP11_REMOTE2_LOW,	/* 6: remote 2 low limit (max6695/96 only) */
+	TEMP11_REMOTE2_HIGH,	/* 7: remote 2 high limit (max6695/96 only) */
+	TEMP11_REG_NUM
+};
+
+/*
+ * TEMP11 register NR
+ */
+enum lm90_temp11_reg_nr {
+	NR_CHAN_0_REMOTE_LOW = 0,	/* 0: channel 0, remote low limit */
+	NR_CHAN_0_REMOTE_HIGH,		/* 1: channel 0, remote high limit */
+	NR_CHAN_0_REMOTE_OFFSET,	/* 2: channel 0, remote offset */
+	NR_CHAN_1_REMOTE_LOW,		/* 3: channel 1, remote low limit */
+	NR_CHAN_1_REMOTE_HIGH,		/* 4: channel 1, remote high limit */
+	NR_NUM				/* number of the NRs for temp11 */
+};
+
+/*
  * Client data (each client gets its own)
  */
 
@@ -320,25 +373,8 @@ struct lm90_data {
 	u8 reg_local_ext;	/* local extension register offset */
 
 	/* registers values */
-	s8 temp8[8];	/* 0: local low limit
-			 * 1: local high limit
-			 * 2: local critical limit
-			 * 3: remote critical limit
-			 * 4: local emergency limit (max6659 and max6695/96)
-			 * 5: remote emergency limit (max6659 and max6695/96)
-			 * 6: remote 2 critical limit (max6695/96 only)
-			 * 7: remote 2 emergency limit (max6695/96 only)
-			 */
-	s16 temp11[8];	/* 0: remote input
-			 * 1: remote low limit
-			 * 2: remote high limit
-			 * 3: remote offset (except max6646, max6657/58/59,
-			 *		     and max6695/96)
-			 * 4: local input
-			 * 5: remote 2 input (max6695/96 only)
-			 * 6: remote 2 low limit (max6695/96 only)
-			 * 7: remote 2 high limit (max6695/96 only)
-			 */
+	s8 temp8[TEMP8_REG_NUM];
+	s16 temp11[TEMP11_REG_NUM];
 	u8 temp_hyst;
 	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
 };
@@ -480,37 +516,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		u8 alarms;
 
 		dev_dbg(&client->dev, "Updating lm90 data.\n");
-		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
-		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
+				&data->temp8[TEMP8_LOCAL_LOW]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
+				&data->temp8[TEMP8_LOCAL_HIGH]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
+				&data->temp8[TEMP8_LOCAL_CRIT]);
+		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+				&data->temp8[TEMP8_REMOTE_CRIT]);
 		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
 
 		if (data->reg_local_ext) {
 			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
 				    data->reg_local_ext,
-				    &data->temp11[4]);
+				    &data->temp11[TEMP11_LOCAL_TEMP]);
 		} else {
 			if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
 					  &h) == 0)
-				data->temp11[4] = h << 8;
+				data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
 		}
 		lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-			    LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
+			LM90_REG_R_REMOTE_TEMPL,
+			&data->temp11[TEMP11_REMOTE_TEMP]);
 
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
-			data->temp11[1] = h << 8;
+			data->temp11[TEMP11_REMOTE_LOW] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
 					  &l) == 0)
-				data->temp11[1] |= l;
+				data->temp11[TEMP11_REMOTE_LOW] |= l;
 		}
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
-			data->temp11[2] = h << 8;
+			data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
 					  &l) == 0)
-				data->temp11[2] |= l;
+				data->temp11[TEMP11_REMOTE_HIGH] |= l;
 		}
 
 		if (data->flags & LM90_HAVE_OFFSET) {
@@ -518,13 +559,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 					  &h) == 0
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
 					  &l) == 0)
-				data->temp11[3] = (h << 8) | l;
+				data->temp11[TEMP11_REMOTE_OFFSET] =
+								(h << 8) | l;
 		}
 		if (data->flags & LM90_HAVE_EMERGENCY) {
 			lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
-				      &data->temp8[4]);
+				      &data->temp8[TEMP8_LOCAL_EMERG]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[5]);
+				      &data->temp8[TEMP8_REMOTE_EMERG]);
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
 		data->alarms = alarms;	/* save as 16 bit value */
@@ -532,15 +574,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		if (data->kind == max6696) {
 			lm90_select_remote_channel(client, data, 1);
 			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
-				      &data->temp8[6]);
+				      &data->temp8[TEMP8_REMOTE2_CRIT]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[7]);
+				      &data->temp8[TEMP8_REMOTE2_EMERG]);
 			lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-				    LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
+					LM90_REG_R_REMOTE_TEMPL,
+					&data->temp11[TEMP11_REMOTE2_TEMP]);
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
-				data->temp11[6] = h << 8;
+				data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
-				data->temp11[7] = h << 8;
+				data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
 			lm90_select_remote_channel(client, data, 0);
 
 			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
@@ -735,7 +778,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
 
 static void _set_temp8(struct device *dev, int index, long val)
 {
-	static const u8 reg[8] = {
+	static const u8 reg[TEMP8_REG_NUM] = {
 		LM90_REG_W_LOCAL_LOW,
 		LM90_REG_W_LOCAL_HIGH,
 		LM90_REG_W_LOCAL_CRIT,
@@ -818,7 +861,7 @@ static void _set_temp11(struct device *dev, int nr, int index, long val)
 		u8 high;
 		u8 low;
 		int channel;
-	} reg[5] = {
+	} reg[NR_NUM] = {
 		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
 		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
@@ -909,11 +952,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461)
-		temp = temp_from_u8_adt7461(data, data->temp8[2]);
+		temp = temp_from_u8_adt7461(data,
+					data->temp8[TEMP8_LOCAL_CRIT]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[2]);
+		temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
 	else
-		temp = temp_from_s8(data->temp8[2]);
+		temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
 
 	data->temp_hyst = hyst_to_reg(temp - val);
 	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
@@ -967,25 +1011,28 @@ static ssize_t set_update_interval(struct device *dev,
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
-static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 0);
+	set_temp8, TEMP8_LOCAL_LOW);
 static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 0, 1);
+	set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 1);
+	set_temp8, TEMP8_LOCAL_HIGH);
 static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 1, 2);
+	set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 2);
+	set_temp8, TEMP8_LOCAL_CRIT);
 static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 3);
+	set_temp8, TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
-	set_temphyst, 2);
-static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
+	set_temphyst, TEMP8_LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 2, 3);
+	set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
 
 /* Individual alarm files */
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
@@ -1033,13 +1080,13 @@ static const struct attribute_group lm90_group = {
  * Additional attributes for devices with emergency sensors
  */
 static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 4);
+	set_temp8, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 5);
+	set_temp8, TEMP8_REMOTE_EMERG);
 static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 4);
+			  NULL, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 5);
+			  NULL, TEMP8_REMOTE_EMERG);
 
 static struct attribute *lm90_emergency_attributes[] = {
 	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
@@ -1069,18 +1116,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
 /*
  * Additional attributes for devices with 3 temperature sensors
  */
-static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
+static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
 static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 3, 6);
+	set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
 static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 4, 7);
+	set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
 static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 6);
-static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6);
+	set_temp8, TEMP8_REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE2_CRIT);
 static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 7);
+	set_temp8, TEMP8_REMOTE2_EMERG);
 static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 7);
+			  NULL, TEMP8_REMOTE2_EMERG);
 
 static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
 static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
-- 
1.7.9.5

Re: [RFC PATCH 4/9] hwmon: (lm90) use macros for the indexes of temp8 and temp11

[Alex Courbot]

On 02/18/2013 08:30 PM, Wei Ni wrote:
> Using macros for the indexes and nrs of temp8 and temp11.
> This make the code much clearer.
>
> Signed-off-by: Wei Ni <wni@nvidia.com>

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Great patch. This makes the code much more readable. I also compiled 
lm90.o before and after this patch, and diff'ed the stripped binaries to 
make sure there was no error in the macro indexes. Both binaries were 
indentical, therefore:

Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Alex.

Re: [RFC PATCH 4/9] hwmon: (lm90) use macros for the indexes of temp8 and temp11

[Wei Ni]

On 02/19/2013 01:39 PM, Alex Courbot wrote:
> On 02/18/2013 08:30 PM, Wei Ni wrote:
>> Using macros for the indexes and nrs of temp8 and temp11.
>> This make the code much clearer.
>>
>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Acked-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Great patch. This makes the code much more readable. I also compiled 
> lm90.o before and after this patch, and diff'ed the stripped binaries to 
> make sure there was no error in the macro indexes. Both binaries were 
> indentical, therefore:
> 
> Tested-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Great, thanks for your test, I will do it in my next version.

Wei.

> 
> Alex.
> 
> 

Re: [RFC PATCH 4/9] hwmon: (lm90) use macros for the indexes of temp8 and temp11

[Stephen Warren]

On 02/18/2013 04:30 AM, Wei Ni wrote:
> Using macros for the indexes and nrs of temp8 and temp11.
> This make the code much clearer.

Sorry to nit-pick, but those aren't macros, but rather they're enums.

The patch looks good though.

Re: [RFC PATCH 4/9] hwmon: (lm90) use macros for the indexes of temp8 and temp11

[Wei Ni]

On 02/20/2013 07:02 AM, Stephen Warren wrote:
> On 02/18/2013 04:30 AM, Wei Ni wrote:
>> Using macros for the indexes and nrs of temp8 and temp11.
>> This make the code much clearer.
> 
> Sorry to nit-pick, but those aren't macros, but rather they're enums.

Yes, you are right, it's better to write "enums".
I will change it.

Wei.

> 
> The patch looks good though.
> 

[RFC PATCH 7/9] thermal: tegra30: add tegra30 thermal driver

[Wei Ni]

dd Tegra30 thermal driver support. It create thermal zone with thermal
sensors and cooling device to participate in the linux thermal management.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/thermal/Kconfig          |    9 ++
 drivers/thermal/Makefile         |    1 +
 drivers/thermal/tegra3_thermal.c |  289 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 299 insertions(+)
 create mode 100644 drivers/thermal/tegra3_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index eadef5b..2403681 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -141,6 +141,15 @@ config INTEL_POWERCLAMP
 	  enforce idle time which results in more package C-state residency. The
 	  user interface is exposed via generic thermal framework.
 
+config TEGRA30_THERMAL
+	tristate "Tegra30 thermal driver"
+	depends on ARCH_TEGRA
+	help
+	  Select this to enable the Tegra30 thermal driver. Adds Tegra30 thermal
+	  implementation according to the thermal management framework. Create
+	  thermal zone with thermal sensors and cooling device to participate
+	  in the linux thermal management.
+
 config THERMAL_TEST
 	tristate "test driver"
 	help
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index ee0f687..de0b411 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_EXYNOS_THERMAL)	+= exynos_thermal.o
 obj-$(CONFIG_DB8500_THERMAL)	+= db8500_thermal.o
 obj-$(CONFIG_DB8500_CPUFREQ_COOLING)	+= db8500_cpufreq_cooling.o
 obj-$(CONFIG_INTEL_POWERCLAMP)	+= intel_powerclamp.o
+obj-$(CONFIG_TEGRA30_THERMAL)	+= tegra3_thermal.o
 
 # dummy driver for testing
 obj-$(CONFIG_THERMAL_TEST)	+= thermal_test.o
diff --git a/drivers/thermal/tegra3_thermal.c b/drivers/thermal/tegra3_thermal.c
new file mode 100644
index 0000000..384168f
--- /dev/null
+++ b/drivers/thermal/tegra3_thermal.c
@@ -0,0 +1,289 @@
+/*
+ * Tegra thermal driver.
+ *
+ * Copyright (C) 2010-2013 NVIDIA Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define MAX_THROT_TABLE_SIZE    (64)
+
+struct throttle_table {
+	unsigned int cpu_freq;
+	int core_cap_level;
+};
+
+struct balanced_throttle {
+	struct thermal_cooling_device *cdev;
+	struct list_head node;
+	int is_throttling;
+	int throttle_count;
+	int throttle_index;
+	int throt_tab_size;
+	struct throttle_table throt_tab[MAX_THROT_TABLE_SIZE + 1];
+};
+
+struct tegra_thermal_data {
+	struct thermal_zone *tz;
+	struct node_args np_args;
+	int passive_delay;
+	struct balanced_throttle tj_throttle;
+	struct thermal_trip_point trip_ext;
+};
+
+static DEFINE_MUTEX(cpu_throttle_lock);
+
+static int tegra_throttle_get_max_state(struct thermal_cooling_device *cdev,
+				unsigned long *max_state)
+{
+	struct balanced_throttle *bthrot = cdev->devdata;
+
+	*max_state = bthrot->throt_tab_size;
+
+	return 0;
+}
+
+static int
+tegra_throttle_get_cur_state(struct thermal_cooling_device *cdev,
+				unsigned long *cur_state)
+{
+	struct balanced_throttle *bthrot = cdev->devdata;
+
+	mutex_lock(&cpu_throttle_lock);
+	*cur_state = bthrot->is_throttling ?
+			(bthrot->throt_tab_size - bthrot->throttle_index) :
+			0;
+	mutex_unlock(&cpu_throttle_lock);
+
+	return 0;
+}
+
+static int
+tegra_throttle_set_cur_state(struct thermal_cooling_device *cdev,
+				unsigned long cur_state)
+{
+	struct balanced_throttle *bthrot = cdev->devdata;
+	int index;
+
+	mutex_lock(&cpu_throttle_lock);
+
+	/* TODO: we will handle the dvfs here */
+
+	bthrot->throttle_index = bthrot->throt_tab_size - cur_state;
+	index = bthrot->throttle_index;
+
+	mutex_unlock(&cpu_throttle_lock);
+
+	return 0;
+}
+
+static struct thermal_cooling_device_ops tegra_throttle_cooling_ops = {
+	.get_max_state = tegra_throttle_get_max_state,
+	.get_cur_state = tegra_throttle_get_cur_state,
+	.set_cur_state = tegra_throttle_set_cur_state,
+};
+
+struct thermal_cooling_device *balanced_throttle_register(
+		struct balanced_throttle *bthrot, char *type)
+{
+	bthrot->cdev = thermal_cooling_device_register(type, bthrot,
+						&tegra_throttle_cooling_ops);
+
+	if (IS_ERR(bthrot->cdev)) {
+		bthrot->cdev = NULL;
+		return ERR_PTR(-ENODEV);
+	}
+
+	return bthrot->cdev;
+}
+
+static struct tegra_thermal_data * __devinit thermal_tegra_dt_parse_pdata(
+						struct platform_device *pdev)
+{
+	struct tegra_thermal_data *tdata;
+	struct device_node *np = pdev->dev.of_node;
+	struct of_phandle_args args;
+	u32 val;
+	int ret;
+
+	if (!np)
+		return NULL;
+
+	tdata = devm_kzalloc(&pdev->dev, sizeof(*tdata), GFP_KERNEL);
+	if (!tdata) {
+		dev_err(&pdev->dev, "Can't allocate platform data\n");
+		return NULL;
+	}
+	memset(tdata, 0, sizeof(*tdata));
+
+	ret = of_parse_phandle_with_args(np, "sensors", "#sensor-cells", 0,
+					&args);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't get sensor.\n");
+		return NULL;
+	}
+	tdata->np_args.np = args.np;
+	tdata->np_args.index = args.args[0];
+
+	ret = of_property_read_u32(np, "passive-delay", &val);
+	if (!ret)
+		tdata->passive_delay = val;
+
+	ret = of_property_read_u32(np, "num-passive-trips", &val);
+	if (!ret)
+		tdata->trip_ext.num_passive_trips = val;
+
+	if (tdata->trip_ext.num_passive_trips) {
+		tdata->trip_ext.passive_trips = devm_kzalloc(&pdev->dev,
+						sizeof(int) * val, GFP_KERNEL);
+
+		of_property_read_u32_array(np, "passive-trips",
+				(u32 *)(tdata->trip_ext.passive_trips),
+				tdata->trip_ext.num_passive_trips);
+	}
+
+	ret = of_property_read_u32(np, "num-active-trips", &val);
+	if (!ret)
+		tdata->trip_ext.num_active_trips = val;
+
+	if (tdata->trip_ext.num_active_trips) {
+		tdata->trip_ext.active_trips = devm_kzalloc(&pdev->dev,
+						sizeof(int) * val, GFP_KERNEL);
+
+		of_property_read_u32_array(np, "active-trips",
+				(u32 *)(tdata->trip_ext.active_trips),
+				tdata->trip_ext.num_active_trips);
+	}
+
+	ret = of_property_read_u32(np, "throt-tab-size", &val);
+	if (!ret)
+		tdata->tj_throttle.throt_tab_size = val;
+
+	of_property_read_u32_array(np, "throt-tab",
+				(u32 *)(&tdata->tj_throttle.throt_tab),
+				tdata->tj_throttle.throt_tab_size * 2);
+
+	return tdata;
+}
+
+static int tegra30_thermal_probe(struct platform_device *pdev)
+{
+	struct tegra_thermal_data *pdata = pdev->dev.platform_data;
+	struct thermal_zone *tz;
+	struct thermal_sensor *ts;
+	static struct thermal_cooling_device *cdev;
+	int ret;
+
+	pdata = thermal_tegra_dt_parse_pdata(pdev);
+	if (!pdata) {
+		dev_err(&pdev->dev, "Get platform data failed.\n");
+		return -EINVAL;
+	}
+
+	/* Create a thermal zone */
+	tz = create_thermal_zone("tz_tegra", NULL);
+	if (!tz) {
+		dev_err(&pdev->dev, "Create thermal_zone failed.\n");
+		return -EINVAL;
+	}
+
+	pdata->tz = tz;
+
+	/* Register cooling device */
+	cdev = balanced_throttle_register(&pdata->tj_throttle, "cdev_throttle");
+	if (!cdev) {
+		dev_err(&pdev->dev, "Register cooling device failed.\n");
+		goto exit_remove_thermal_zone;
+	}
+
+	/* Get sensor */
+	ts = get_sensor_by_node(&pdata->np_args);
+	if (!ts) {
+		dev_err(&pdev->dev, "get_sensor_by_node failed.\n");
+		goto exit_unregister_cooling;
+	}
+
+	ret = add_sensor_to_zone(pdata->tz, ts);
+	if (ret) {
+		dev_err(&pdev->dev, "add_sensor_to_zone failed.\n");
+		goto exit_unregister_cooling;
+	}
+
+	ret = add_cdev_to_zone(pdata->tz, cdev);
+	if (ret) {
+		dev_err(&pdev->dev, "add_cdev_to_zone failed.\n");
+		goto exit_unregister_cooling;
+	}
+
+	ret = add_sensor_trip_info(pdata->tz, ts, &pdata->trip_ext);
+	if (ret) {
+		dev_err(&pdev->dev, "add_sensor_trip_info failed.\n");
+		goto exit_unregister_cooling;
+	}
+
+	return 0;
+
+exit_unregister_cooling:
+	thermal_cooling_device_unregister(cdev);
+exit_remove_thermal_zone:
+	remove_thermal_zone(pdata->tz);
+	return -EINVAL;
+
+}
+
+static int tegra30_thermal_remove(struct platform_device *pdev)
+{
+	struct tegra_thermal_data *pdata = pdev->dev.platform_data;
+	int i;
+
+	for (i = 0; i < MAX_CDEVS_PER_ZONE; i++) {
+		if (pdata->tz->cdevs[i])
+			thermal_cooling_device_unregister(pdata->tz->cdevs[i]);
+		else
+			break;
+	}
+
+	remove_thermal_zone(pdata->tz);
+
+	return 0;
+}
+
+static const struct of_device_id tegra30_thermal_id_table[] = {
+	{ .compatible = "nvidia,tegra30-thermal" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tegra30_thermal_id_table);
+
+static struct platform_driver tegra3_thermal_driver = {
+	.probe = tegra30_thermal_probe,
+	.remove = tegra30_thermal_remove,
+	.driver = {
+		.name = "tegra_thermal",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(tegra30_thermal_id_table),
+	},
+};
+module_platform_driver(tegra3_thermal_driver);
+
+MODULE_AUTHOR("Wei Ni <wni@nvidia.com>");
+MODULE_DESCRIPTION("Tegra30 thermal throttle driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [RFC PATCH 7/9] thermal: tegra30: add tegra30 thermal driver

[Stephen Warren]

On 02/18/2013 04:30 AM, Wei Ni wrote:
> dd Tegra30 thermal driver support. It create thermal zone with thermal
> sensors and cooling device to participate in the linux thermal management.

> diff --git a/drivers/thermal/tegra3_thermal.c b/drivers/thermal/tegra3_thermal.c

> +struct tegra_thermal_data {

That's not really "thermal data", but that "thermal zone" or "thermal
device" itself.

> +static int
> +tegra_throttle_set_cur_state(struct thermal_cooling_device *cdev,
> +				unsigned long cur_state)
> +{
> +	struct balanced_throttle *bthrot = cdev->devdata;
> +	int index;
> +
> +	mutex_lock(&cpu_throttle_lock);
> +
> +	/* TODO: we will handle the dvfs here */

That seems like rather a large TODO. We don't have any DVFS support for
Tegra yet. Is it even worth moving forward with this driver without that?

> +struct thermal_cooling_device *balanced_throttle_register(
> +		struct balanced_throttle *bthrot, char *type)

What does "balanced" mean here; isn't balanced a governor, whereas this
driver is simply providing the implementation that an arbitrary governor
would use to do its will?

This function also appears to be rather generically named. A consistent
function/symbol-name prefix of e.g. "tegra30_thermal" would be useful
throughout the file.

> +static struct tegra_thermal_data * __devinit thermal_tegra_dt_parse_pdata(
> +						struct platform_device *pdev)

I'd like some slight re-structing here.

Tegra only supports device tree now; no board files (upstream at least,
which is all that's relevant here). Hence, none of the Tegra drivers
support platform data at all after the last round of cleanup patches I
sent on Friday (most of which haven't been checked in yet). Hence, this
function is not parsing platform data out of DT, it's plain parsing
device tree.

So, can you move the devm_kzalloc of the tdata into probe() right at the
start, rename this function tegra30_thermal_parse_dt(), and call it
right after allocating "tdata" (which also is more like "tdev" or
"tzone" perhaps).

Downstream if you still need to support platform data, you can simply
copy the platform data into "tdev" rather than calling
tegra30_thermal_parse_dt(); a simple patch to carry.

> +	tdata = devm_kzalloc(&pdev->dev, sizeof(*tdata), GFP_KERNEL);
> +	if (!tdata) {
> +		dev_err(&pdev->dev, "Can't allocate platform data\n");
> +		return NULL;
> +	}
> +	memset(tdata, 0, sizeof(*tdata));

That last line is kinda the whole point of k*z*alloc...

> +
> +	ret = of_parse_phandle_with_args(np, "sensors", "#sensor-cells", 0,
> +					&args);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't get sensor.\n");
> +		return NULL;
> +	}
> +	tdata->np_args.np = args.np;
> +	tdata->np_args.index = args.args[0];

That lookup should be implemented as part of the thermal core. It
shouldn't be duplicated in every single driver.

> +	ret = of_property_read_u32(np, "passive-delay", &val);
> +	if (!ret)
> +		tdata->passive_delay = val;

The DT binding documentation doesn't say this property is optional. If
you intend it to be, the documentation should say so, and specify what
the default is if the property is missing.

> +	ret = of_property_read_u32(np, "num-passive-trips", &val);
> +	if (!ret)
> +		tdata->trip_ext.num_passive_trips = val;
> +
> +	if (tdata->trip_ext.num_passive_trips) {
> +		tdata->trip_ext.passive_trips = devm_kzalloc(&pdev->dev,
> +						sizeof(int) * val, GFP_KERNEL);

DT cells are specifically U32 not int. You probably want the driver to
use U32 instead of int to make 100% sure the range matches.

> +
> +		of_property_read_u32_array(np, "passive-trips",
> +				(u32 *)(tdata->trip_ext.passive_trips),
> +				tdata->trip_ext.num_passive_trips);

... and then you wouldn't need this cast!

> +	}

> +	of_property_read_u32_array(np, "throt-tab",
> +				(u32 *)(&tdata->tj_throttle.throt_tab),
> +				tdata->tj_throttle.throt_tab_size * 2);

What about error-handling that API call, and all the others?

> +static int tegra30_thermal_probe(struct platform_device *pdev)
> +{
> +	struct tegra_thermal_data *pdata = pdev->dev.platform_data;

Lets not support platform data at all upstream.

> +	/* Register cooling device */
> +	cdev = balanced_throttle_register(&pdata->tj_throttle, "cdev_throttle");

Is this picking specific policy ("balanced")? Should it be? What is
"cdev_throttle"?

Re: [RFC PATCH 7/9] thermal: tegra30: add tegra30 thermal driver

[Wei Ni]

On 02/20/2013 07:48 AM, Stephen Warren wrote:
> On 02/18/2013 04:30 AM, Wei Ni wrote:
>> dd Tegra30 thermal driver support. It create thermal zone with thermal
>> sensors and cooling device to participate in the linux thermal management.
> 
>> diff --git a/drivers/thermal/tegra3_thermal.c b/drivers/thermal/tegra3_thermal.c
> 
>> +struct tegra_thermal_data {
> 
> That's not really "thermal data", but that "thermal zone" or "thermal
> device" itself.
> 
>> +static int
>> +tegra_throttle_set_cur_state(struct thermal_cooling_device *cdev,
>> +				unsigned long cur_state)
>> +{
>> +	struct balanced_throttle *bthrot = cdev->devdata;
>> +	int index;
>> +
>> +	mutex_lock(&cpu_throttle_lock);
>> +
>> +	/* TODO: we will handle the dvfs here */
> 
> That seems like rather a large TODO. We don't have any DVFS support for
> Tegra yet. Is it even worth moving forward with this driver without that?

I think it's better to split this file into two driver, one is for the
thermal sensor, it can read/write temperature. And the other one is for
the cooling device, something like tegra3_cooling .c, it will call dvfs
interface to handle throttling when dvfs is ready.
And by now, we can focus on this tegra3_thermal.c.

> 
>> +struct thermal_cooling_device *balanced_throttle_register(
>> +		struct balanced_throttle *bthrot, char *type)
> 
> What does "balanced" mean here; isn't balanced a governor, whereas this
> driver is simply providing the implementation that an arbitrary governor
> would use to do its will?
> 
> This function also appears to be rather generically named. A consistent
> function/symbol-name prefix of e.g. "tegra30_thermal" would be useful
> throughout the file.

I port these codes from our downstream kernel, I will move them to the
new tegra3_cooling.c, and since the dvfs is not ready, I will remain
simplest codes, and remove the "balanced".
> 
>> +static struct tegra_thermal_data * __devinit thermal_tegra_dt_parse_pdata(
>> +						struct platform_device *pdev)
> 
> I'd like some slight re-structing here.
> 
> Tegra only supports device tree now; no board files (upstream at least,
> which is all that's relevant here). Hence, none of the Tegra drivers
> support platform data at all after the last round of cleanup patches I
> sent on Friday (most of which haven't been checked in yet). Hence, this
> function is not parsing platform data out of DT, it's plain parsing
> device tree.
> 
> So, can you move the devm_kzalloc of the tdata into probe() right at the
> start, rename this function tegra30_thermal_parse_dt(), and call it
> right after allocating "tdata" (which also is more like "tdev" or
> "tzone" perhaps).

Got it, I will change it.

> 
> Downstream if you still need to support platform data, you can simply
> copy the platform data into "tdev" rather than calling
> tegra30_thermal_parse_dt(); a simple patch to carry.
> 
>> +	tdata = devm_kzalloc(&pdev->dev, sizeof(*tdata), GFP_KERNEL);
>> +	if (!tdata) {
>> +		dev_err(&pdev->dev, "Can't allocate platform data\n");
>> +		return NULL;
>> +	}
>> +	memset(tdata, 0, sizeof(*tdata));
> 
> That last line is kinda the whole point of k*z*alloc...

Yes, I forgot to remove it.

> 
>> +
>> +	ret = of_parse_phandle_with_args(np, "sensors", "#sensor-cells", 0,
>> +					&args);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Can't get sensor.\n");
>> +		return NULL;
>> +	}
>> +	tdata->np_args.np = args.np;
>> +	tdata->np_args.index = args.args[0];
> 
> That lookup should be implemented as part of the thermal core. It
> shouldn't be duplicated in every single driver.
> 
>> +	ret = of_property_read_u32(np, "passive-delay", &val);
>> +	if (!ret)
>> +		tdata->passive_delay = val;
> 
> The DT binding documentation doesn't say this property is optional. If
> you intend it to be, the documentation should say so, and specify what
> the default is if the property is missing.

Ok, I will change it.

> 
>> +	ret = of_property_read_u32(np, "num-passive-trips", &val);
>> +	if (!ret)
>> +		tdata->trip_ext.num_passive_trips = val;
>> +
>> +	if (tdata->trip_ext.num_passive_trips) {
>> +		tdata->trip_ext.passive_trips = devm_kzalloc(&pdev->dev,
>> +						sizeof(int) * val, GFP_KERNEL);
> 
> DT cells are specifically U32 not int. You probably want the driver to
> use U32 instead of int to make 100% sure the range matches.

Ok.

> 
>> +
>> +		of_property_read_u32_array(np, "passive-trips",
>> +				(u32 *)(tdata->trip_ext.passive_trips),
>> +				tdata->trip_ext.num_passive_trips);
> 
> ... and then you wouldn't need this cast!
> 
>> +	}
> 
>> +	of_property_read_u32_array(np, "throt-tab",
>> +				(u32 *)(&tdata->tj_throttle.throt_tab),
>> +				tdata->tj_throttle.throt_tab_size * 2);
> 
> What about error-handling that API call, and all the others?

I will add it.

> 
>> +static int tegra30_thermal_probe(struct platform_device *pdev)
>> +{
>> +	struct tegra_thermal_data *pdata = pdev->dev.platform_data;
> 
> Lets not support platform data at all upstream.

Got it, thanks.

> 
>> +	/* Register cooling device */
>> +	cdev = balanced_throttle_register(&pdata->tj_throttle, "cdev_throttle");
> 
> Is this picking specific policy ("balanced")? Should it be? What is
> "cdev_throttle"?

These codes are ported from our downstream kernel, it may handle
multiple cooling device, so it named as "balanced", but in here we
didn't use it, so it's better to remove them, and remain simplest codes.
I will change it as I said above.

> --
> 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: [RFC PATCH 7/9] thermal: tegra30: add tegra30 thermal driver

[Russell King - ARM Linux]

On Mon, Feb 18, 2013 at 07:30:29PM +0800, Wei Ni wrote:
> +static struct tegra_thermal_data * __devinit thermal_tegra_dt_parse_pdata(

__dev* no longer exists.

> +	tdata = devm_kzalloc(&pdev->dev, sizeof(*tdata), GFP_KERNEL);
> +	if (!tdata) {
> +		dev_err(&pdev->dev, "Can't allocate platform data\n");
> +		return NULL;
> +	}
> +	memset(tdata, 0, sizeof(*tdata));

Useless memset.  k*z*alloc already zeros the memory before returning.

> +static int tegra30_thermal_probe(struct platform_device *pdev)
> +{
> +	struct tegra_thermal_data *pdata = pdev->dev.platform_data;

You read pdata here....

> +	struct thermal_zone *tz;
> +	struct thermal_sensor *ts;
> +	static struct thermal_cooling_device *cdev;
> +	int ret;
> +
> +	pdata = thermal_tegra_dt_parse_pdata(pdev);

and immediately overwrite it here.

> +	if (!pdata) {
> +		dev_err(&pdev->dev, "Get platform data failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Create a thermal zone */
> +	tz = create_thermal_zone("tz_tegra", NULL);
> +	if (!tz) {
> +		dev_err(&pdev->dev, "Create thermal_zone failed.\n");
> +		return -EINVAL;
> +	}
> +
> +	pdata->tz = tz;

This isn't how you deal with driver data.  Set driver data against a
platform device using platform_set_drvdata(pdev, tz).

> +static int tegra30_thermal_remove(struct platform_device *pdev)
> +{
> +	struct tegra_thermal_data *pdata = pdev->dev.platform_data;

and use platform_get_drvdata() here - and don't use pdata->tz.
	struct struct thermal_zone *tz = platform_get_drvdata(pdev);

Re: [RFC PATCH 7/9] thermal: tegra30: add tegra30 thermal driver

[Wei Ni]

On 02/20/2013 07:56 AM, Russell King - ARM Linux wrote:
> On Mon, Feb 18, 2013 at 07:30:29PM +0800, Wei Ni wrote:
>> +static struct tegra_thermal_data * __devinit thermal_tegra_dt_parse_pdata(
> 
> __dev* no longer exists.

Ok, I will change it.

> 
>> +	tdata = devm_kzalloc(&pdev->dev, sizeof(*tdata), GFP_KERNEL);
>> +	if (!tdata) {
>> +		dev_err(&pdev->dev, "Can't allocate platform data\n");
>> +		return NULL;
>> +	}
>> +	memset(tdata, 0, sizeof(*tdata));
> 
> Useless memset.  k*z*alloc already zeros the memory before returning.

Yes, I forgot to remove this line.

> 
>> +static int tegra30_thermal_probe(struct platform_device *pdev)
>> +{
>> +	struct tegra_thermal_data *pdata = pdev->dev.platform_data;
> 
> You read pdata here....
> 
>> +	struct thermal_zone *tz;
>> +	struct thermal_sensor *ts;
>> +	static struct thermal_cooling_device *cdev;
>> +	int ret;
>> +
>> +	pdata = thermal_tegra_dt_parse_pdata(pdev);
> 
> and immediately overwrite it here.
> 
>> +	if (!pdata) {
>> +		dev_err(&pdev->dev, "Get platform data failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Create a thermal zone */
>> +	tz = create_thermal_zone("tz_tegra", NULL);
>> +	if (!tz) {
>> +		dev_err(&pdev->dev, "Create thermal_zone failed.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pdata->tz = tz;
> 
> This isn't how you deal with driver data.  Set driver data against a
> platform device using platform_set_drvdata(pdev, tz).

Yes, I didn't consider it carefully.
As Stephen said, our tegra will only support DT, so I will remove the
platform date.

Thanks.
Wei.

> 
>> +static int tegra30_thermal_remove(struct platform_device *pdev)
>> +{
>> +	struct tegra_thermal_data *pdata = pdev->dev.platform_data;
> 
> and use platform_get_drvdata() here - and don't use pdata->tz.
> 	struct struct thermal_zone *tz = platform_get_drvdata(pdev);
>