[PATCH v2 0/2] Intruduce thermal_zone_get_zone_by_node

[PATCH v2 0/2] Intruduce thermal_zone_get_zone_by_node

[Wei Ni]

The thermal fw start to support device tree, so add a new funciton
thermal_zone_get_zone_by_node()

This series is v2.

Changes from v1:
1. not remove the thermal_zone_get_zone_by_name, and add the
new one, so it will not affect other driver.
2. split one patch into two patches.

Wei Ni (2):
  thermal: introduce thermal_zone_get_zone_by_node helper function
  thermal: use device node to get thermal zone

 drivers/thermal/of-thermal.c   |    6 ++++--
 drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
 include/linux/thermal.h        |    3 +++
 3 files changed, 40 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/2] thermal: introduce thermal_zone_get_zone_by_node helper function

[Wei Ni]

The thermal framework start to support device tree, this
patch adds a helper function to get a reference of a
thermal zone, based on the zone's device node.

It will add a device_node *np member in the struct
thermal_zone_device, and initialize it when create a thermal
zone. This funciton perform a zone device node lookup and
return a reference to a thermal zone device that matches
the device node requested.
In case the zone is not found or if the required parameters
are invalid, it will return the corresponding error code (ERR_PTR).

Change-Id: I4d65f849e84425dddd387f70886a9c7c4c2002f2
Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/thermal/of-thermal.c   |    2 ++
 drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
 include/linux/thermal.h        |    3 +++
 3 files changed, 38 insertions(+)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 04b1be7..53f2d3a 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -804,6 +804,8 @@ int __init of_parse_thermal_zones(void)
 			of_thermal_free_zone(tz);
 			/* attempting to build remaining zones still */
 		}
+
+		zone->np = child;
 	}
 
 	return 0;
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 338a88b..eeddb94 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1672,6 +1672,39 @@ exit:
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
 
+/**
+* thermal_zone_get_zone_by_node() - search for a zone and returns its ref
+* @node: device node of the thermal zone
+*
+* When thermal zone is found with the passed device node, returns a reference
+* to it.
+*
+* Return: On success returns a reference to an unique thermal zone with
+* matching device node, an ERR_PTR otherwise (-EINVAL for invalid
+* paramenters, -ENODEV for not found).
+*/
+struct thermal_zone_device *
+thermal_zone_get_zone_by_node(struct device_node *node)
+{
+	struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
+	bool found = false;
+
+	if (!node)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&thermal_list_lock);
+	list_for_each_entry(pos, &thermal_tz_list, node)
+		if (node == pos->np) {
+			ref = pos;
+			found = true;
+			break;
+		}
+	mutex_unlock(&thermal_list_lock);
+
+	return ref;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
+
 #ifdef CONFIG_NET
 static const struct genl_multicast_group thermal_event_mcgrps[] = {
 	{ .name = THERMAL_GENL_MCAST_GROUP_NAME, },
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f7e11c7..288d272 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -162,6 +162,7 @@ struct thermal_zone_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
 	struct device device;
+	struct device_node *np;
 	struct thermal_attr *trip_temp_attrs;
 	struct thermal_attr *trip_type_attrs;
 	struct thermal_attr *trip_hyst_attrs;
@@ -286,6 +287,8 @@ thermal_of_cooling_device_register(struct device_node *np, char *, void *,
 				   const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
 struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
+struct thermal_zone_device *
+thermal_zone_get_zone_by_node(struct device_node *node);
 int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp);
 
 int get_tz_trend(struct thermal_zone_device *, int);
-- 
1.7.9.5

Re: [PATCH v2 1/2] thermal: introduce thermal_zone_get_zone_by_node helper function

[Eduardo Valentin]

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

On 09-01-2014 06:37, Wei Ni wrote:
> The thermal framework start to support device tree, this
> patch adds a helper function to get a reference of a
> thermal zone, based on the zone's device node.

I would prefer if you could provide a better justification why we need
this API. Think of the scope of this API: would it be used only by the
of-thermal code? only by the drivers/thermal code? or any driver?

So far you have provided only one user, and that user can already work
with existing APIs. As I mention, DT does not support name duplications.

Unless you enlighten  me with better uses of this API, I would prefer
not to have it.


> 
> It will add a device_node *np member in the struct
> thermal_zone_device, and initialize it when create a thermal
> zone. This funciton perform a zone device node lookup and
> return a reference to a thermal zone device that matches
> the device node requested.
> In case the zone is not found or if the required parameters
> are invalid, it will return the corresponding error code (ERR_PTR).
> 
> Change-Id: I4d65f849e84425dddd387f70886a9c7c4c2002f2

For your next patches, please done include gerrit change IDs. Linux
Kernel does not need to be linked to your inhouse development history
via gerrit IDs.

> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/thermal/of-thermal.c   |    2 ++
>  drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
>  include/linux/thermal.h        |    3 +++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 04b1be7..53f2d3a 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -804,6 +804,8 @@ int __init of_parse_thermal_zones(void)
>  			of_thermal_free_zone(tz);
>  			/* attempting to build remaining zones still */
>  		}
> +
> +		zone->np = child;
>  	}
>  
>  	return 0;
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 338a88b..eeddb94 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1672,6 +1672,39 @@ exit:
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
>  
> +/**
> +* thermal_zone_get_zone_by_node() - search for a zone and returns its ref
> +* @node: device node of the thermal zone
> +*
> +* When thermal zone is found with the passed device node, returns a reference
> +* to it.
> +*
> +* Return: On success returns a reference to an unique thermal zone with
> +* matching device node, an ERR_PTR otherwise (-EINVAL for invalid
> +* paramenters, -ENODEV for not found).
> +*/
> +struct thermal_zone_device *
> +thermal_zone_get_zone_by_node(struct device_node *node)
> +{
> +	struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
> +	bool found = false;
> +
> +	if (!node)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&thermal_list_lock);
> +	list_for_each_entry(pos, &thermal_tz_list, node)
> +		if (node == pos->np) {
> +			ref = pos;
> +			found = true;
> +			break;
> +		}
> +	mutex_unlock(&thermal_list_lock);
> +
> +	return ref;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
> +
>  #ifdef CONFIG_NET
>  static const struct genl_multicast_group thermal_event_mcgrps[] = {
>  	{ .name = THERMAL_GENL_MCAST_GROUP_NAME, },
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f7e11c7..288d272 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -162,6 +162,7 @@ struct thermal_zone_device {
>  	int id;
>  	char type[THERMAL_NAME_LENGTH];
>  	struct device device;
> +	struct device_node *np;
>  	struct thermal_attr *trip_temp_attrs;
>  	struct thermal_attr *trip_type_attrs;
>  	struct thermal_attr *trip_hyst_attrs;
> @@ -286,6 +287,8 @@ thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>  				   const struct thermal_cooling_device_ops *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
> +struct thermal_zone_device *
> +thermal_zone_get_zone_by_node(struct device_node *node);
>  int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp);
>  
>  int get_tz_trend(struct thermal_zone_device *, int);
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 1/2] thermal: introduce thermal_zone_get_zone_by_node helper function

[Wei Ni]

On 01/14/2014 12:05 AM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On 09-01-2014 06:37, Wei Ni wrote:
>> The thermal framework start to support device tree, this
>> patch adds a helper function to get a reference of a
>> thermal zone, based on the zone's device node.
> 
> I would prefer if you could provide a better justification why we need
> this API. Think of the scope of this API: would it be used only by the
> of-thermal code? only by the drivers/thermal code? or any driver?

I have talked it in the previous patch "Re: [PATCH] thermal: use device
node to get thermal zone".
So let me copy to here again:

On the tegra board, it will use two or more sensors to estimate the skin
temperature by reading temps from these sensors and calculate them.
For example, we have two sensors: sensor1 and sensor2. We can register
them to thermal framework by using DT, something like:
thermal-zones {
                sensor1: lm90-local {
			...
                        thermal-sensors = <&lm90 0>;
                };

                sensor2: lm90-remote {
			...
                        thermal-sensors = <&lm90 1>;
                };
}

Then I will add a device node for my skin temperature driver, something
like:
skin_temp {
	...
	#thermal-sensor-cells = <0>;

	sub-devs {
		dev@0 {
			dev = <&sensor1>;
		};

		dev@1 {
			dev = <&sensor2>;
		};
	};
};
So I can parse the DT in the skin temperature driver to get the nodes of
the sensor1 and sensor2, and can use .*get_by_node to get thermal zone
device, then use .get_temp() and other callbacks to get temperature and
other information.

> 
> So far you have provided only one user, and that user can already work
> with existing APIs. As I mention, DT does not support name duplications.

If the system only have the DT, then there will not have name
duplications. But some drivers will call the
thermal_zone_device_register directly with any thermal zone type, and at
same time, other drivers may use of-thermal to register, and set the
same name in the DT. Then if use the *.get_by_name, it can't get the
uniqu one.

> 
> Unless you enlighten  me with better uses of this API, I would prefer
> not to have it.
> 
> 
>>
>> It will add a device_node *np member in the struct
>> thermal_zone_device, and initialize it when create a thermal
>> zone. This funciton perform a zone device node lookup and
>> return a reference to a thermal zone device that matches
>> the device node requested.
>> In case the zone is not found or if the required parameters
>> are invalid, it will return the corresponding error code (ERR_PTR).
>>
>> Change-Id: I4d65f849e84425dddd387f70886a9c7c4c2002f2
> 
> For your next patches, please done include gerrit change IDs. Linux
> Kernel does not need to be linked to your inhouse development history
> via gerrit IDs.

It's so sorry, I forgot to remove it.

> 
>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/thermal/of-thermal.c   |    2 ++
>>  drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
>>  include/linux/thermal.h        |    3 +++
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index 04b1be7..53f2d3a 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -804,6 +804,8 @@ int __init of_parse_thermal_zones(void)
>>  			of_thermal_free_zone(tz);
>>  			/* attempting to build remaining zones still */
>>  		}
>> +
>> +		zone->np = child;
>>  	}
>>  
>>  	return 0;
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 338a88b..eeddb94 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1672,6 +1672,39 @@ exit:
>>  }
>>  EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
>>  
>> +/**
>> +* thermal_zone_get_zone_by_node() - search for a zone and returns its ref
>> +* @node: device node of the thermal zone
>> +*
>> +* When thermal zone is found with the passed device node, returns a reference
>> +* to it.
>> +*
>> +* Return: On success returns a reference to an unique thermal zone with
>> +* matching device node, an ERR_PTR otherwise (-EINVAL for invalid
>> +* paramenters, -ENODEV for not found).
>> +*/
>> +struct thermal_zone_device *
>> +thermal_zone_get_zone_by_node(struct device_node *node)
>> +{
>> +	struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
>> +	bool found = false;
>> +
>> +	if (!node)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	mutex_lock(&thermal_list_lock);
>> +	list_for_each_entry(pos, &thermal_tz_list, node)
>> +		if (node == pos->np) {
>> +			ref = pos;
>> +			found = true;
>> +			break;
>> +		}
>> +	mutex_unlock(&thermal_list_lock);
>> +
>> +	return ref;
>> +}
>> +EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
>> +
>>  #ifdef CONFIG_NET
>>  static const struct genl_multicast_group thermal_event_mcgrps[] = {
>>  	{ .name = THERMAL_GENL_MCAST_GROUP_NAME, },
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index f7e11c7..288d272 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -162,6 +162,7 @@ struct thermal_zone_device {
>>  	int id;
>>  	char type[THERMAL_NAME_LENGTH];
>>  	struct device device;
>> +	struct device_node *np;
>>  	struct thermal_attr *trip_temp_attrs;
>>  	struct thermal_attr *trip_type_attrs;
>>  	struct thermal_attr *trip_hyst_attrs;
>> @@ -286,6 +287,8 @@ thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>>  				   const struct thermal_cooling_device_ops *);
>>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
>> +struct thermal_zone_device *
>> +thermal_zone_get_zone_by_node(struct device_node *node);
>>  int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp);
>>  
>>  int get_tz_trend(struct thermal_zone_device *, int);
>>
> 
> 

Re: [PATCH v2 1/2] thermal: introduce thermal_zone_get_zone_by_node helper function

[Eduardo Valentin]

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

Hello Wei,

On 14-01-2014 06:35, Wei Ni wrote:
> On 01/14/2014 12:05 AM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> On 09-01-2014 06:37, Wei Ni wrote:
>>> The thermal framework start to support device tree, this
>>> patch adds a helper function to get a reference of a
>>> thermal zone, based on the zone's device node.
>>
>> I would prefer if you could provide a better justification why we need
>> this API. Think of the scope of this API: would it be used only by the
>> of-thermal code? only by the drivers/thermal code? or any driver?
> 


> I have talked it in the previous patch "Re: [PATCH] thermal: use device
> node to get thermal zone".

Thanks for putting me in the same page


> So let me copy to here again:
> 
> On the tegra board, it will use two or more sensors to estimate the skin
> temperature by reading temps from these sensors and calculate them.

OK. Have you read the Example (c) in the thermal binding document
(Documentation/devicetree/bindings/thermal)?

> For example, we have two sensors: sensor1 and sensor2. We can register
> them to thermal framework by using DT, something like:
> thermal-zones {
>                 sensor1: lm90-local {
> 			...
>                         thermal-sensors = <&lm90 0>;
>                 };
> 
>                 sensor2: lm90-remote {
> 			...
>                         thermal-sensors = <&lm90 1>;
>                 };
> }

where is the descriptor for lm90?

This is actually not how it is supposed to be described.

> 
> Then I will add a device node for my skin temperature driver, something
> like:
> skin_temp {
> 	...
> 	#thermal-sensor-cells = <0>;
> 
> 	sub-devs {
> 		dev@0 {
> 			dev = <&sensor1>;
> 		};
> 
> 		dev@1 {
> 			dev = <&sensor2>;
> 		};
> 	};
> };

What is the above? A virtual device?

I believe the above is not really part of the thermal bindings.

> So I can parse the DT in the skin temperature driver to get the nodes of
> the sensor1 and sensor2, and can use .*get_by_node to get thermal zone
> device, then use .get_temp() and other callbacks to get temperature and
> other information.
> 

The above is a different binding and different API then of what has been
discussed last year for thermal bindings. I am copying here the existing
example:

#include <dt-bindings/thermal/thermal.h>

&i2c1 {
	...
	/*
	 * A simple IC with a single temperature sensor.
	 */
	adc: sensor@0x49 {
		...
		#thermal-sensor-cells = <0>;
	};
};

ocp {
	...
	/*
	 * A simple IC with a single bandgap temperature sensor.
	 */
	bandgap0: bandgap@0x0000ED00 {
		...
		#thermal-sensor-cells = <0>;
	};
};

thermal-zones {
	cpu-thermal: cpu-thermal {
		polling-delay-passive = <250>; /* milliseconds */
		polling-delay = <1000>; /* milliseconds */

		thermal-sensors = <&bandgap0>,	/* cpu */
				  <&adc>;	/* pcb north */

		/* hotspot = 100 * bandgap - 120 * adc + 484 */
		coefficients = 		<100	-120	484>;

		trips {
			...
		};

		cooling-maps {
			...
		};
	};
};



The idea is to have a producer-> consumer description, in which the
thermal zone is the consumer and the sensors are the producers (for
temperature at least).

What is the relation between the sensors you have?

Why the above structure cannot cover for your case? Here is what you
would do to describe your case:

/*
 * You did not specify which type of sensors, so I am assuming
 * they are I2C sensors
 */
&i2c1 {
                 lm90: sensor@48 {
 			...
                         #thermal-sensor-cells = <1>;
                 };
};

thermal-zones {
	skin-hotspot: skin-hotspot {
		polling-delay-passive = <2000>; /* put the right val */
		polling-delay = <1000>; /* put the right val */

		thermal-sensors = <&lm90 0>,	/* local */
				  <&lm90 1>;	/* remote */

		/* hotspot ?
		 * What is the relation that describes your sensors?
		 * coefficients =  <A_0	A_1>; you have to fill the coef.
		 */

		trips {
			...
		};

		cooling-maps {
			...
		};
	};
};

So, the idea is so that you define your sensor nodes and the respective
drivers, in the above example the i2c drivers, will probe them. When
creating thermal zones, you have to specify the list of sensors by
linking using phandles + sensor specifier, as documented in the thermal
binding text.

>>
>> So far you have provided only one user, and that user can already work
>> with existing APIs. As I mention, DT does not support name duplications.
> 
> If the system only have the DT, then there will not have name
> duplications. But some drivers will call the
> thermal_zone_device_register directly with any thermal zone type, and at
> same time, other drivers may use of-thermal to register, and set the
> same name in the DT. Then if use the *.get_by_name, it can't get the
> uniqu one.

The above example is odd to me. Why would a driver be registering zones
from DT and from in kernel definition? The usual path is to have either
one or the other. Even if you are migrating your system to DT based
booting, you may have a mix of devices that are probed via DT and others
that are probed via board files, but one single driver probing both, I
think this is unusual.

> 
>>
>> Unless you enlighten  me with better uses of this API, I would prefer
>> not to have it.
>>
>>
>>>
>>> It will add a device_node *np member in the struct
>>> thermal_zone_device, and initialize it when create a thermal
>>> zone. This funciton perform a zone device node lookup and
>>> return a reference to a thermal zone device that matches
>>> the device node requested.
>>> In case the zone is not found or if the required parameters
>>> are invalid, it will return the corresponding error code (ERR_PTR).
>>>
>>> Change-Id: I4d65f849e84425dddd387f70886a9c7c4c2002f2
>>
>> For your next patches, please done include gerrit change IDs. Linux
>> Kernel does not need to be linked to your inhouse development history
>> via gerrit IDs.
> 
> It's so sorry, I forgot to remove it.
> 
>>
>>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/thermal/of-thermal.c   |    2 ++
>>>  drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
>>>  include/linux/thermal.h        |    3 +++
>>>  3 files changed, 38 insertions(+)
>>>
>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>> index 04b1be7..53f2d3a 100644
>>> --- a/drivers/thermal/of-thermal.c
>>> +++ b/drivers/thermal/of-thermal.c
>>> @@ -804,6 +804,8 @@ int __init of_parse_thermal_zones(void)
>>>  			of_thermal_free_zone(tz);
>>>  			/* attempting to build remaining zones still */
>>>  		}
>>> +
>>> +		zone->np = child;
>>>  	}
>>>  
>>>  	return 0;
>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>> index 338a88b..eeddb94 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -1672,6 +1672,39 @@ exit:
>>>  }
>>>  EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
>>>  
>>> +/**
>>> +* thermal_zone_get_zone_by_node() - search for a zone and returns its ref
>>> +* @node: device node of the thermal zone
>>> +*
>>> +* When thermal zone is found with the passed device node, returns a reference
>>> +* to it.
>>> +*
>>> +* Return: On success returns a reference to an unique thermal zone with
>>> +* matching device node, an ERR_PTR otherwise (-EINVAL for invalid
>>> +* paramenters, -ENODEV for not found).
>>> +*/
>>> +struct thermal_zone_device *
>>> +thermal_zone_get_zone_by_node(struct device_node *node)
>>> +{
>>> +	struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
>>> +	bool found = false;
>>> +
>>> +	if (!node)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	mutex_lock(&thermal_list_lock);
>>> +	list_for_each_entry(pos, &thermal_tz_list, node)
>>> +		if (node == pos->np) {
>>> +			ref = pos;
>>> +			found = true;
>>> +			break;
>>> +		}
>>> +	mutex_unlock(&thermal_list_lock);
>>> +
>>> +	return ref;
>>> +}
>>> +EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
>>> +
>>>  #ifdef CONFIG_NET
>>>  static const struct genl_multicast_group thermal_event_mcgrps[] = {
>>>  	{ .name = THERMAL_GENL_MCAST_GROUP_NAME, },
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index f7e11c7..288d272 100644
>>> --- a/include/linux/thermal.h
>>> +++ b/include/linux/thermal.h
>>> @@ -162,6 +162,7 @@ struct thermal_zone_device {
>>>  	int id;
>>>  	char type[THERMAL_NAME_LENGTH];
>>>  	struct device device;
>>> +	struct device_node *np;
>>>  	struct thermal_attr *trip_temp_attrs;
>>>  	struct thermal_attr *trip_type_attrs;
>>>  	struct thermal_attr *trip_hyst_attrs;
>>> @@ -286,6 +287,8 @@ thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>>>  				   const struct thermal_cooling_device_ops *);
>>>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>>>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
>>> +struct thermal_zone_device *
>>> +thermal_zone_get_zone_by_node(struct device_node *node);
>>>  int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp);
>>>  
>>>  int get_tz_trend(struct thermal_zone_device *, int);
>>>
>>
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 1/2] thermal: introduce thermal_zone_get_zone_by_node helper function

[Wei Ni]

On 01/14/2014 11:30 PM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> Hello Wei,
> 
> On 14-01-2014 06:35, Wei Ni wrote:
>> On 01/14/2014 12:05 AM, Eduardo Valentin wrote:
>>>> Old Signed by an unknown key
>>>
>>> On 09-01-2014 06:37, Wei Ni wrote:
>>>> The thermal framework start to support device tree, this
>>>> patch adds a helper function to get a reference of a
>>>> thermal zone, based on the zone's device node.
>>>
>>> I would prefer if you could provide a better justification why we need
>>> this API. Think of the scope of this API: would it be used only by the
>>> of-thermal code? only by the drivers/thermal code? or any driver?
>>
> 
> 
>> I have talked it in the previous patch "Re: [PATCH] thermal: use device
>> node to get thermal zone".
> 
> Thanks for putting me in the same page
> 
> 
>> So let me copy to here again:
>>
>> On the tegra board, it will use two or more sensors to estimate the skin
>> temperature by reading temps from these sensors and calculate them.
> 
> OK. Have you read the Example (c) in the thermal binding document
> (Documentation/devicetree/bindings/thermal)?
> 
>> For example, we have two sensors: sensor1 and sensor2. We can register
>> them to thermal framework by using DT, something like:
>> thermal-zones {
>>                 sensor1: lm90-local {
>>                       ...
>>                         thermal-sensors = <&lm90 0>;
>>                 };
>>
>>                 sensor2: lm90-remote {
>>                       ...
>>                         thermal-sensors = <&lm90 1>;
>>                 };
>> }
> 
> where is the descriptor for lm90?

There has descriptior for lm90, I didn't post it.

> 
> This is actually not how it is supposed to be described.
> 
>>
>> Then I will add a device node for my skin temperature driver, something
>> like:
>> skin_temp {
>>       ...
>>       #thermal-sensor-cells = <0>;
>>
>>       sub-devs {
>>               dev@0 {
>>                       dev = <&sensor1>;
>>               };
>>
>>               dev@1 {
>>                       dev = <&sensor2>;
>>               };
>>       };
>> };
> 
> What is the above? A virtual device?

Yes, it is the node for the skin temperature estimator driver.

> 
> I believe the above is not really part of the thermal bindings.
> 
>> So I can parse the DT in the skin temperature driver to get the nodes of
>> the sensor1 and sensor2, and can use .*get_by_node to get thermal zone
>> device, then use .get_temp() and other callbacks to get temperature and
>> other information.
>>
> 
> The above is a different binding and different API then of what has been
> discussed last year for thermal bindings. I am copying here the existing
> example:
> 
> #include <dt-bindings/thermal/thermal.h>
> 
> &i2c1 {
>         ...
>         /*
>          * A simple IC with a single temperature sensor.
>          */
>         adc: sensor@0x49 {
>                 ...
>                 #thermal-sensor-cells = <0>;
>         };
> };
> 
> ocp {
>         ...
>         /*
>          * A simple IC with a single bandgap temperature sensor.
>          */
>         bandgap0: bandgap@0x0000ED00 {
>                 ...
>                 #thermal-sensor-cells = <0>;
>         };
> };
> 
> thermal-zones {
>         cpu-thermal: cpu-thermal {
>                 polling-delay-passive = <250>; /* milliseconds */
>                 polling-delay = <1000>; /* milliseconds */
> 
>                 thermal-sensors = <&bandgap0>,  /* cpu */
>                                   <&adc>;       /* pcb north */
> 
>                 /* hotspot = 100 * bandgap - 120 * adc + 484 */
>                 coefficients =          <100    -120    484>;
> 
>                 trips {
>                         ...
>                 };
> 
>                 cooling-maps {
>                         ...
>                 };
>         };
> };
> 
> 
> 
> The idea is to have a producer-> consumer description, in which the
> thermal zone is the consumer and the sensors are the producers (for
> temperature at least).
> 
> What is the relation between the sensors you have?
> 
> Why the above structure cannot cover for your case? Here is what you
> would do to describe your case:

Yes, I noticed that you have this sample in the documents, but it seems
the of-thermal only support one sensor in the thermal-sensors list, and
didn't handle the coefficients yet.
And in my skin temperature estimator driver, it will have more complex
logic to estimate the temperature, it will consider the history
temperatures, and have coefficients for every history temp, it seems
can't use your coefficients easily.

> 
> /*
>  * You did not specify which type of sensors, so I am assuming
>  * they are I2C sensors
>  */
> &i2c1 {
>                  lm90: sensor@48 {
>                         ...
>                          #thermal-sensor-cells = <1>;
>                  };
> };
> 
> thermal-zones {
>         skin-hotspot: skin-hotspot {
>                 polling-delay-passive = <2000>; /* put the right val */
>                 polling-delay = <1000>; /* put the right val */
> 
>                 thermal-sensors = <&lm90 0>,    /* local */
>                                   <&lm90 1>;    /* remote */
> 
>                 /* hotspot ?
>                  * What is the relation that describes your sensors?
>                  * coefficients =  <A_0 A_1>; you have to fill the coef.
>                  */
> 
>                 trips {
>                         ...
>                 };
> 
>                 cooling-maps {
>                         ...
>                 };
>         };
> };
> 
> So, the idea is so that you define your sensor nodes and the respective
> drivers, in the above example the i2c drivers, will probe them. When
> creating thermal zones, you have to specify the list of sensors by
> linking using phandles + sensor specifier, as documented in the thermal
> binding text.
> 
>>>
>>> So far you have provided only one user, and that user can already work
>>> with existing APIs. As I mention, DT does not support name duplications.
>>
>> If the system only have the DT, then there will not have name
>> duplications. But some drivers will call the
>> thermal_zone_device_register directly with any thermal zone type, and at
>> same time, other drivers may use of-thermal to register, and set the
>> same name in the DT. Then if use the *.get_by_name, it can't get the
>> uniqu one.
> 
> The above example is odd to me. Why would a driver be registering zones
> from DT and from in kernel definition? The usual path is to have either
> one or the other. Even if you are migrating your system to DT based
> booting, you may have a mix of devices that are probed via DT and others
> that are probed via board files, but one single driver probing both, I
> think this is unusual.
> 
>>
>>>
>>> Unless you enlighten  me with better uses of this API, I would prefer
>>> not to have it.
>>>
>>>
>>>>
>>>> It will add a device_node *np member in the struct
>>>> thermal_zone_device, and initialize it when create a thermal
>>>> zone. This funciton perform a zone device node lookup and
>>>> return a reference to a thermal zone device that matches
>>>> the device node requested.
>>>> In case the zone is not found or if the required parameters
>>>> are invalid, it will return the corresponding error code (ERR_PTR).
>>>>
>>>> Change-Id: I4d65f849e84425dddd387f70886a9c7c4c2002f2
>>>
>>> For your next patches, please done include gerrit change IDs. Linux
>>> Kernel does not need to be linked to your inhouse development history
>>> via gerrit IDs.
>>
>> It's so sorry, I forgot to remove it.
>>
>>>
>>>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  drivers/thermal/of-thermal.c   |    2 ++
>>>>  drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
>>>>  include/linux/thermal.h        |    3 +++
>>>>  3 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>> index 04b1be7..53f2d3a 100644
>>>> --- a/drivers/thermal/of-thermal.c
>>>> +++ b/drivers/thermal/of-thermal.c
>>>> @@ -804,6 +804,8 @@ int __init of_parse_thermal_zones(void)
>>>>                     of_thermal_free_zone(tz);
>>>>                     /* attempting to build remaining zones still */
>>>>             }
>>>> +
>>>> +           zone->np = child;
>>>>     }
>>>>
>>>>     return 0;
>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>> index 338a88b..eeddb94 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -1672,6 +1672,39 @@ exit:
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
>>>>
>>>> +/**
>>>> +* thermal_zone_get_zone_by_node() - search for a zone and returns its ref
>>>> +* @node: device node of the thermal zone
>>>> +*
>>>> +* When thermal zone is found with the passed device node, returns a reference
>>>> +* to it.
>>>> +*
>>>> +* Return: On success returns a reference to an unique thermal zone with
>>>> +* matching device node, an ERR_PTR otherwise (-EINVAL for invalid
>>>> +* paramenters, -ENODEV for not found).
>>>> +*/
>>>> +struct thermal_zone_device *
>>>> +thermal_zone_get_zone_by_node(struct device_node *node)
>>>> +{
>>>> +   struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
>>>> +   bool found = false;
>>>> +
>>>> +   if (!node)
>>>> +           return ERR_PTR(-EINVAL);
>>>> +
>>>> +   mutex_lock(&thermal_list_lock);
>>>> +   list_for_each_entry(pos, &thermal_tz_list, node)
>>>> +           if (node == pos->np) {
>>>> +                   ref = pos;
>>>> +                   found = true;
>>>> +                   break;
>>>> +           }
>>>> +   mutex_unlock(&thermal_list_lock);
>>>> +
>>>> +   return ref;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
>>>> +
>>>>  #ifdef CONFIG_NET
>>>>  static const struct genl_multicast_group thermal_event_mcgrps[] = {
>>>>     { .name = THERMAL_GENL_MCAST_GROUP_NAME, },
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index f7e11c7..288d272 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -162,6 +162,7 @@ struct thermal_zone_device {
>>>>     int id;
>>>>     char type[THERMAL_NAME_LENGTH];
>>>>     struct device device;
>>>> +   struct device_node *np;
>>>>     struct thermal_attr *trip_temp_attrs;
>>>>     struct thermal_attr *trip_type_attrs;
>>>>     struct thermal_attr *trip_hyst_attrs;
>>>> @@ -286,6 +287,8 @@ thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>>>>                                const struct thermal_cooling_device_ops *);
>>>>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>>>>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
>>>> +struct thermal_zone_device *
>>>> +thermal_zone_get_zone_by_node(struct device_node *node);
>>>>  int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp);
>>>>
>>>>  int get_tz_trend(struct thermal_zone_device *, int);
>>>>
>>>
>>>
>>
>>
>>
> 
> 
> --
> You have got to be excited about what you are doing. (L. Lamport)
> 
> Eduardo Valentin
> 
> 
> * Unknown Key
> * 0x75D0BCFD
> 

Re: [PATCH v2 1/2] thermal: introduce thermal_zone_get_zone_by_node helper function

[Eduardo Valentin]

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

On 15-01-2014 07:52, Wei Ni wrote:
> On 01/14/2014 11:30 PM, Eduardo Valentin wrote:
>> * PGP Signed by an unknown key
>>
>> Hello Wei,
>>
>> On 14-01-2014 06:35, Wei Ni wrote:
>>> On 01/14/2014 12:05 AM, Eduardo Valentin wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On 09-01-2014 06:37, Wei Ni wrote:
>>>>> The thermal framework start to support device tree, this
>>>>> patch adds a helper function to get a reference of a
>>>>> thermal zone, based on the zone's device node.
>>>>
>>>> I would prefer if you could provide a better justification why we need
>>>> this API. Think of the scope of this API: would it be used only by the
>>>> of-thermal code? only by the drivers/thermal code? or any driver?
>>>
>>
>>
>>> I have talked it in the previous patch "Re: [PATCH] thermal: use device
>>> node to get thermal zone".
>>
>> Thanks for putting me in the same page
>>
>>
>>> So let me copy to here again:
>>>
>>> On the tegra board, it will use two or more sensors to estimate the skin
>>> temperature by reading temps from these sensors and calculate them.
>>
>> OK. Have you read the Example (c) in the thermal binding document
>> (Documentation/devicetree/bindings/thermal)?
>>
>>> For example, we have two sensors: sensor1 and sensor2. We can register
>>> them to thermal framework by using DT, something like:
>>> thermal-zones {
>>>                 sensor1: lm90-local {
>>>                       ...
>>>                         thermal-sensors = <&lm90 0>;
>>>                 };
>>>
>>>                 sensor2: lm90-remote {
>>>                       ...
>>>                         thermal-sensors = <&lm90 1>;
>>>                 };
>>> }
>>
>> where is the descriptor for lm90?
> 
> There has descriptior for lm90, I didn't post it.
> 
>>
>> This is actually not how it is supposed to be described.
>>
>>>
>>> Then I will add a device node for my skin temperature driver, something
>>> like:
>>> skin_temp {
>>>       ...
>>>       #thermal-sensor-cells = <0>;
>>>
>>>       sub-devs {
>>>               dev@0 {
>>>                       dev = <&sensor1>;
>>>               };
>>>
>>>               dev@1 {
>>>                       dev = <&sensor2>;
>>>               };
>>>       };
>>> };
>>
>> What is the above? A virtual device?
> 
> Yes, it is the node for the skin temperature estimator driver.
> 
>>
>> I believe the above is not really part of the thermal bindings.
>>
>>> So I can parse the DT in the skin temperature driver to get the nodes of
>>> the sensor1 and sensor2, and can use .*get_by_node to get thermal zone
>>> device, then use .get_temp() and other callbacks to get temperature and
>>> other information.
>>>
>>
>> The above is a different binding and different API then of what has been
>> discussed last year for thermal bindings. I am copying here the existing
>> example:
>>
>> #include <dt-bindings/thermal/thermal.h>
>>
>> &i2c1 {
>>         ...
>>         /*
>>          * A simple IC with a single temperature sensor.
>>          */
>>         adc: sensor@0x49 {
>>                 ...
>>                 #thermal-sensor-cells = <0>;
>>         };
>> };
>>
>> ocp {
>>         ...
>>         /*
>>          * A simple IC with a single bandgap temperature sensor.
>>          */
>>         bandgap0: bandgap@0x0000ED00 {
>>                 ...
>>                 #thermal-sensor-cells = <0>;
>>         };
>> };
>>
>> thermal-zones {
>>         cpu-thermal: cpu-thermal {
>>                 polling-delay-passive = <250>; /* milliseconds */
>>                 polling-delay = <1000>; /* milliseconds */
>>
>>                 thermal-sensors = <&bandgap0>,  /* cpu */
>>                                   <&adc>;       /* pcb north */
>>
>>                 /* hotspot = 100 * bandgap - 120 * adc + 484 */
>>                 coefficients =          <100    -120    484>;
>>
>>                 trips {
>>                         ...
>>                 };
>>
>>                 cooling-maps {
>>                         ...
>>                 };
>>         };
>> };
>>
>>
>>
>> The idea is to have a producer-> consumer description, in which the
>> thermal zone is the consumer and the sensors are the producers (for
>> temperature at least).
>>
>> What is the relation between the sensors you have?
>>
>> Why the above structure cannot cover for your case? Here is what you
>> would do to describe your case:
> 
> Yes, I noticed that you have this sample in the documents, but it seems
> the of-thermal only support one sensor in the thermal-sensors list, and
> didn't handle the coefficients yet.
> And in my skin temperature estimator driver, it will have more complex
> logic to estimate the temperature, it will consider the history
> temperatures, and have coefficients for every history temp, it seems
> can't use your coefficients easily.

Is it possible to disclose what is the real equation you are using to
populate your virtual sensor?  In this way we can either discuss how to
implement it using the existing API or propose the correct improvements.

I am considering posting a series of statistical treatment for the
thermal framework.

> 
>>
>> /*
>>  * You did not specify which type of sensors, so I am assuming
>>  * they are I2C sensors
>>  */
>> &i2c1 {
>>                  lm90: sensor@48 {
>>                         ...
>>                          #thermal-sensor-cells = <1>;
>>                  };
>> };
>>
>> thermal-zones {
>>         skin-hotspot: skin-hotspot {
>>                 polling-delay-passive = <2000>; /* put the right val */
>>                 polling-delay = <1000>; /* put the right val */
>>
>>                 thermal-sensors = <&lm90 0>,    /* local */
>>                                   <&lm90 1>;    /* remote */
>>
>>                 /* hotspot ?
>>                  * What is the relation that describes your sensors?
>>                  * coefficients =  <A_0 A_1>; you have to fill the coef.
>>                  */
>>
>>                 trips {
>>                         ...
>>                 };
>>
>>                 cooling-maps {
>>                         ...
>>                 };
>>         };
>> };
>>
>> So, the idea is so that you define your sensor nodes and the respective
>> drivers, in the above example the i2c drivers, will probe them. When
>> creating thermal zones, you have to specify the list of sensors by
>> linking using phandles + sensor specifier, as documented in the thermal
>> binding text.
>>
>>>>
>>>> So far you have provided only one user, and that user can already work
>>>> with existing APIs. As I mention, DT does not support name duplications.
>>>
>>> If the system only have the DT, then there will not have name
>>> duplications. But some drivers will call the
>>> thermal_zone_device_register directly with any thermal zone type, and at
>>> same time, other drivers may use of-thermal to register, and set the
>>> same name in the DT. Then if use the *.get_by_name, it can't get the
>>> uniqu one.
>>
>> The above example is odd to me. Why would a driver be registering zones
>> from DT and from in kernel definition? The usual path is to have either
>> one or the other. Even if you are migrating your system to DT based
>> booting, you may have a mix of devices that are probed via DT and others
>> that are probed via board files, but one single driver probing both, I
>> think this is unusual.
>>
>>>
>>>>
>>>> Unless you enlighten  me with better uses of this API, I would prefer
>>>> not to have it.
>>>>
>>>>
>>>>>
>>>>> It will add a device_node *np member in the struct
>>>>> thermal_zone_device, and initialize it when create a thermal
>>>>> zone. This funciton perform a zone device node lookup and
>>>>> return a reference to a thermal zone device that matches
>>>>> the device node requested.
>>>>> In case the zone is not found or if the required parameters
>>>>> are invalid, it will return the corresponding error code (ERR_PTR).
>>>>>
>>>>> Change-Id: I4d65f849e84425dddd387f70886a9c7c4c2002f2
>>>>
>>>> For your next patches, please done include gerrit change IDs. Linux
>>>> Kernel does not need to be linked to your inhouse development history
>>>> via gerrit IDs.
>>>
>>> It's so sorry, I forgot to remove it.
>>>
>>>>
>>>>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>  drivers/thermal/of-thermal.c   |    2 ++
>>>>>  drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
>>>>>  include/linux/thermal.h        |    3 +++
>>>>>  3 files changed, 38 insertions(+)
>>>>>
>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>>> index 04b1be7..53f2d3a 100644
>>>>> --- a/drivers/thermal/of-thermal.c
>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>> @@ -804,6 +804,8 @@ int __init of_parse_thermal_zones(void)
>>>>>                     of_thermal_free_zone(tz);
>>>>>                     /* attempting to build remaining zones still */
>>>>>             }
>>>>> +
>>>>> +           zone->np = child;
>>>>>     }
>>>>>
>>>>>     return 0;
>>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>>> index 338a88b..eeddb94 100644
>>>>> --- a/drivers/thermal/thermal_core.c
>>>>> +++ b/drivers/thermal/thermal_core.c
>>>>> @@ -1672,6 +1672,39 @@ exit:
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
>>>>>
>>>>> +/**
>>>>> +* thermal_zone_get_zone_by_node() - search for a zone and returns its ref
>>>>> +* @node: device node of the thermal zone
>>>>> +*
>>>>> +* When thermal zone is found with the passed device node, returns a reference
>>>>> +* to it.
>>>>> +*
>>>>> +* Return: On success returns a reference to an unique thermal zone with
>>>>> +* matching device node, an ERR_PTR otherwise (-EINVAL for invalid
>>>>> +* paramenters, -ENODEV for not found).
>>>>> +*/
>>>>> +struct thermal_zone_device *
>>>>> +thermal_zone_get_zone_by_node(struct device_node *node)
>>>>> +{
>>>>> +   struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
>>>>> +   bool found = false;
>>>>> +
>>>>> +   if (!node)
>>>>> +           return ERR_PTR(-EINVAL);
>>>>> +
>>>>> +   mutex_lock(&thermal_list_lock);
>>>>> +   list_for_each_entry(pos, &thermal_tz_list, node)
>>>>> +           if (node == pos->np) {
>>>>> +                   ref = pos;
>>>>> +                   found = true;
>>>>> +                   break;
>>>>> +           }
>>>>> +   mutex_unlock(&thermal_list_lock);
>>>>> +
>>>>> +   return ref;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
>>>>> +
>>>>>  #ifdef CONFIG_NET
>>>>>  static const struct genl_multicast_group thermal_event_mcgrps[] = {
>>>>>     { .name = THERMAL_GENL_MCAST_GROUP_NAME, },
>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>>> index f7e11c7..288d272 100644
>>>>> --- a/include/linux/thermal.h
>>>>> +++ b/include/linux/thermal.h
>>>>> @@ -162,6 +162,7 @@ struct thermal_zone_device {
>>>>>     int id;
>>>>>     char type[THERMAL_NAME_LENGTH];
>>>>>     struct device device;
>>>>> +   struct device_node *np;
>>>>>     struct thermal_attr *trip_temp_attrs;
>>>>>     struct thermal_attr *trip_type_attrs;
>>>>>     struct thermal_attr *trip_hyst_attrs;
>>>>> @@ -286,6 +287,8 @@ thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>>>>>                                const struct thermal_cooling_device_ops *);
>>>>>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>>>>>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
>>>>> +struct thermal_zone_device *
>>>>> +thermal_zone_get_zone_by_node(struct device_node *node);
>>>>>  int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp);
>>>>>
>>>>>  int get_tz_trend(struct thermal_zone_device *, int);
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>> --
>> You have got to be excited about what you are doing. (L. Lamport)
>>
>> Eduardo Valentin
>>
>>
>> * Unknown Key
>> * 0x75D0BCFD
>>
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]

Re: [PATCH v2 1/2] thermal: introduce thermal_zone_get_zone_by_node helper function

[Wei Ni]

On 01/15/2014 10:09 PM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On 15-01-2014 07:52, Wei Ni wrote:
>> On 01/14/2014 11:30 PM, Eduardo Valentin wrote:
>>>> Old Signed by an unknown key
>>>
>>> Hello Wei,
>>>
>>> On 14-01-2014 06:35, Wei Ni wrote:
>>>> On 01/14/2014 12:05 AM, Eduardo Valentin wrote:
>>>>>> Old Signed by an unknown key
>>>>>
>>>>> On 09-01-2014 06:37, Wei Ni wrote:
>>>>>> The thermal framework start to support device tree, this
>>>>>> patch adds a helper function to get a reference of a
>>>>>> thermal zone, based on the zone's device node.
>>>>>
>>>>> I would prefer if you could provide a better justification why we need
>>>>> this API. Think of the scope of this API: would it be used only by the
>>>>> of-thermal code? only by the drivers/thermal code? or any driver?
>>>>
>>>
>>>
>>>> I have talked it in the previous patch "Re: [PATCH] thermal: use device
>>>> node to get thermal zone".
>>>
>>> Thanks for putting me in the same page
>>>
>>>
>>>> So let me copy to here again:
>>>>
>>>> On the tegra board, it will use two or more sensors to estimate the skin
>>>> temperature by reading temps from these sensors and calculate them.
>>>
>>> OK. Have you read the Example (c) in the thermal binding document
>>> (Documentation/devicetree/bindings/thermal)?
>>>
>>>> For example, we have two sensors: sensor1 and sensor2. We can register
>>>> them to thermal framework by using DT, something like:
>>>> thermal-zones {
>>>>                 sensor1: lm90-local {
>>>>                       ...
>>>>                         thermal-sensors = <&lm90 0>;
>>>>                 };
>>>>
>>>>                 sensor2: lm90-remote {
>>>>                       ...
>>>>                         thermal-sensors = <&lm90 1>;
>>>>                 };
>>>> }
>>>
>>> where is the descriptor for lm90?
>>
>> There has descriptior for lm90, I didn't post it.
>>
>>>
>>> This is actually not how it is supposed to be described.
>>>
>>>>
>>>> Then I will add a device node for my skin temperature driver, something
>>>> like:
>>>> skin_temp {
>>>>       ...
>>>>       #thermal-sensor-cells = <0>;
>>>>
>>>>       sub-devs {
>>>>               dev@0 {
>>>>                       dev = <&sensor1>;
>>>>               };
>>>>
>>>>               dev@1 {
>>>>                       dev = <&sensor2>;
>>>>               };
>>>>       };
>>>> };
>>>
>>> What is the above? A virtual device?
>>
>> Yes, it is the node for the skin temperature estimator driver.
>>
>>>
>>> I believe the above is not really part of the thermal bindings.
>>>
>>>> So I can parse the DT in the skin temperature driver to get the nodes of
>>>> the sensor1 and sensor2, and can use .*get_by_node to get thermal zone
>>>> device, then use .get_temp() and other callbacks to get temperature and
>>>> other information.
>>>>
>>>
>>> The above is a different binding and different API then of what has been
>>> discussed last year for thermal bindings. I am copying here the existing
>>> example:
>>>
>>> #include <dt-bindings/thermal/thermal.h>
>>>
>>> &i2c1 {
>>>         ...
>>>         /*
>>>          * A simple IC with a single temperature sensor.
>>>          */
>>>         adc: sensor@0x49 {
>>>                 ...
>>>                 #thermal-sensor-cells = <0>;
>>>         };
>>> };
>>>
>>> ocp {
>>>         ...
>>>         /*
>>>          * A simple IC with a single bandgap temperature sensor.
>>>          */
>>>         bandgap0: bandgap@0x0000ED00 {
>>>                 ...
>>>                 #thermal-sensor-cells = <0>;
>>>         };
>>> };
>>>
>>> thermal-zones {
>>>         cpu-thermal: cpu-thermal {
>>>                 polling-delay-passive = <250>; /* milliseconds */
>>>                 polling-delay = <1000>; /* milliseconds */
>>>
>>>                 thermal-sensors = <&bandgap0>,  /* cpu */
>>>                                   <&adc>;       /* pcb north */
>>>
>>>                 /* hotspot = 100 * bandgap - 120 * adc + 484 */
>>>                 coefficients =          <100    -120    484>;
>>>
>>>                 trips {
>>>                         ...
>>>                 };
>>>
>>>                 cooling-maps {
>>>                         ...
>>>                 };
>>>         };
>>> };
>>>
>>>
>>>
>>> The idea is to have a producer-> consumer description, in which the
>>> thermal zone is the consumer and the sensors are the producers (for
>>> temperature at least).
>>>
>>> What is the relation between the sensors you have?
>>>
>>> Why the above structure cannot cover for your case? Here is what you
>>> would do to describe your case:
>>
>> Yes, I noticed that you have this sample in the documents, but it seems
>> the of-thermal only support one sensor in the thermal-sensors list, and
>> didn't handle the coefficients yet.
>> And in my skin temperature estimator driver, it will have more complex
>> logic to estimate the temperature, it will consider the history
>> temperatures, and have coefficients for every history temp, it seems
>> can't use your coefficients easily.
> 
> Is it possible to disclose what is the real equation you are using to
> populate your virtual sensor?  In this way we can either discuss how to
> implement it using the existing API or propose the correct improvements.

To approximate skin we estimate skin temperature based on the two
temperature sensors (eg. lm90's local/remote sensors). It will read
temperature from them every about 1s, and record the history temps per
sensor. It meant that it's just the weighted sum of the past N readings
from a few physical temperature sensors. From platform to platform we
may need to change the weights, in here the weights are something like
the coefficients. The it also have a offset for the sum value. so the
equation is something like:
local_temp = coeffs[0]*temp[0] + ... + coeffs[n]*temp[n]
remote_temp = coeffs[0]*temp[0] + ... + coeffs[n]*temp[n]
skin-temp = proportional * (local_temp + remote_temp) + offset;

> 
> I am considering posting a series of statistical treatment for the
> thermal framework.

So, looking forward your patches :)
If could please consider above issue.
And I may be pending this series, wand wait your new series.

Thanks.
Wei.

> 
>>
>>>
>>> /*
>>>  * You did not specify which type of sensors, so I am assuming
>>>  * they are I2C sensors
>>>  */
>>> &i2c1 {
>>>                  lm90: sensor@48 {
>>>                         ...
>>>                          #thermal-sensor-cells = <1>;
>>>                  };
>>> };
>>>
>>> thermal-zones {
>>>         skin-hotspot: skin-hotspot {
>>>                 polling-delay-passive = <2000>; /* put the right val */
>>>                 polling-delay = <1000>; /* put the right val */
>>>
>>>                 thermal-sensors = <&lm90 0>,    /* local */
>>>                                   <&lm90 1>;    /* remote */
>>>
>>>                 /* hotspot ?
>>>                  * What is the relation that describes your sensors?
>>>                  * coefficients =  <A_0 A_1>; you have to fill the coef.
>>>                  */
>>>
>>>                 trips {
>>>                         ...
>>>                 };
>>>
>>>                 cooling-maps {
>>>                         ...
>>>                 };
>>>         };
>>> };
>>>
>>> So, the idea is so that you define your sensor nodes and the respective
>>> drivers, in the above example the i2c drivers, will probe them. When
>>> creating thermal zones, you have to specify the list of sensors by
>>> linking using phandles + sensor specifier, as documented in the thermal
>>> binding text.
>>>
>>>>>
>>>>> So far you have provided only one user, and that user can already work
>>>>> with existing APIs. As I mention, DT does not support name duplications.
>>>>
>>>> If the system only have the DT, then there will not have name
>>>> duplications. But some drivers will call the
>>>> thermal_zone_device_register directly with any thermal zone type, and at
>>>> same time, other drivers may use of-thermal to register, and set the
>>>> same name in the DT. Then if use the *.get_by_name, it can't get the
>>>> uniqu one.
>>>
>>> The above example is odd to me. Why would a driver be registering zones
>>> from DT and from in kernel definition? The usual path is to have either
>>> one or the other. Even if you are migrating your system to DT based
>>> booting, you may have a mix of devices that are probed via DT and others
>>> that are probed via board files, but one single driver probing both, I
>>> think this is unusual.
>>>
>>>>
>>>>>
>>>>> Unless you enlighten  me with better uses of this API, I would prefer
>>>>> not to have it.
>>>>>
>>>>>
>>>>>>
>>>>>> It will add a device_node *np member in the struct
>>>>>> thermal_zone_device, and initialize it when create a thermal
>>>>>> zone. This funciton perform a zone device node lookup and
>>>>>> return a reference to a thermal zone device that matches
>>>>>> the device node requested.
>>>>>> In case the zone is not found or if the required parameters
>>>>>> are invalid, it will return the corresponding error code (ERR_PTR).
>>>>>>
>>>>>> Change-Id: I4d65f849e84425dddd387f70886a9c7c4c2002f2
>>>>>
>>>>> For your next patches, please done include gerrit change IDs. Linux
>>>>> Kernel does not need to be linked to your inhouse development history
>>>>> via gerrit IDs.
>>>>
>>>> It's so sorry, I forgot to remove it.
>>>>
>>>>>
>>>>>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>> ---
>>>>>>  drivers/thermal/of-thermal.c   |    2 ++
>>>>>>  drivers/thermal/thermal_core.c |   33 +++++++++++++++++++++++++++++++++
>>>>>>  include/linux/thermal.h        |    3 +++
>>>>>>  3 files changed, 38 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>>>> index 04b1be7..53f2d3a 100644
>>>>>> --- a/drivers/thermal/of-thermal.c
>>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>>> @@ -804,6 +804,8 @@ int __init of_parse_thermal_zones(void)
>>>>>>                     of_thermal_free_zone(tz);
>>>>>>                     /* attempting to build remaining zones still */
>>>>>>             }
>>>>>> +
>>>>>> +           zone->np = child;
>>>>>>     }
>>>>>>
>>>>>>     return 0;
>>>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>>>> index 338a88b..eeddb94 100644
>>>>>> --- a/drivers/thermal/thermal_core.c
>>>>>> +++ b/drivers/thermal/thermal_core.c
>>>>>> @@ -1672,6 +1672,39 @@ exit:
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
>>>>>>
>>>>>> +/**
>>>>>> +* thermal_zone_get_zone_by_node() - search for a zone and returns its ref
>>>>>> +* @node: device node of the thermal zone
>>>>>> +*
>>>>>> +* When thermal zone is found with the passed device node, returns a reference
>>>>>> +* to it.
>>>>>> +*
>>>>>> +* Return: On success returns a reference to an unique thermal zone with
>>>>>> +* matching device node, an ERR_PTR otherwise (-EINVAL for invalid
>>>>>> +* paramenters, -ENODEV for not found).
>>>>>> +*/
>>>>>> +struct thermal_zone_device *
>>>>>> +thermal_zone_get_zone_by_node(struct device_node *node)
>>>>>> +{
>>>>>> +   struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
>>>>>> +   bool found = false;
>>>>>> +
>>>>>> +   if (!node)
>>>>>> +           return ERR_PTR(-EINVAL);
>>>>>> +
>>>>>> +   mutex_lock(&thermal_list_lock);
>>>>>> +   list_for_each_entry(pos, &thermal_tz_list, node)
>>>>>> +           if (node == pos->np) {
>>>>>> +                   ref = pos;
>>>>>> +                   found = true;
>>>>>> +                   break;
>>>>>> +           }
>>>>>> +   mutex_unlock(&thermal_list_lock);
>>>>>> +
>>>>>> +   return ref;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
>>>>>> +
>>>>>>  #ifdef CONFIG_NET
>>>>>>  static const struct genl_multicast_group thermal_event_mcgrps[] = {
>>>>>>     { .name = THERMAL_GENL_MCAST_GROUP_NAME, },
>>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>>>> index f7e11c7..288d272 100644
>>>>>> --- a/include/linux/thermal.h
>>>>>> +++ b/include/linux/thermal.h
>>>>>> @@ -162,6 +162,7 @@ struct thermal_zone_device {
>>>>>>     int id;
>>>>>>     char type[THERMAL_NAME_LENGTH];
>>>>>>     struct device device;
>>>>>> +   struct device_node *np;
>>>>>>     struct thermal_attr *trip_temp_attrs;
>>>>>>     struct thermal_attr *trip_type_attrs;
>>>>>>     struct thermal_attr *trip_hyst_attrs;
>>>>>> @@ -286,6 +287,8 @@ thermal_of_cooling_device_register(struct device_node *np, char *, void *,
>>>>>>                                const struct thermal_cooling_device_ops *);
>>>>>>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>>>>>>  struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
>>>>>> +struct thermal_zone_device *
>>>>>> +thermal_zone_get_zone_by_node(struct device_node *node);
>>>>>>  int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp);
>>>>>>
>>>>>>  int get_tz_trend(struct thermal_zone_device *, int);
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> You have got to be excited about what you are doing. (L. Lamport)
>>>
>>> Eduardo Valentin
>>>
>>>
>>> * Unknown Key
>>> * 0x75D0BCFD
>>>
>>
>>
>>
> 
> 
> --
> You have got to be excited about what you are doing. (L. Lamport)
> 
> Eduardo Valentin
> 
> 
> * Unknown Key
> * 0x75D0BCFD
> 

[PATCH v2 2/2] thermal: use device node to get thermal zone

[Wei Ni]

The thermal fw support same name for different thermal zones.
If use name to get the thermal zone, sometimes it can't get
the unique thermal zone. And the of-thermal driver is used
to parse DT for thermal fw, so it's better to use device node
to lookup and get thermal zone.

Change-Id: I6c690902a9535915b2f49f9921779fbcd5ecd88c
Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/thermal/of-thermal.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 53f2d3a..97c12cf 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -330,7 +330,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 	struct thermal_zone_device *tzd;
 	struct __thermal_zone *tz;
 
-	tzd = thermal_zone_get_zone_by_name(zone->name);
+	tzd = thermal_zone_get_zone_by_node(zone);
 	if (IS_ERR(tzd))
 		return ERR_PTR(-EPROBE_DEFER);
 
@@ -839,7 +839,7 @@ void of_thermal_destroy_zones(void)
 	for_each_child_of_node(np, child) {
 		struct thermal_zone_device *zone;
 
-		zone = thermal_zone_get_zone_by_name(child->name);
+		zone = thermal_zone_get_zone_by_node(child);
 		if (IS_ERR(zone))
 			continue;
 
-- 
1.7.9.5