diff mbox

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

Message ID 1389263879-10483-2-git-send-email-wni@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Zhang Rui
Headers show

Commit Message

Wei Ni Jan. 9, 2014, 10:37 a.m. UTC
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(+)

Comments

Eduardo Valentin Jan. 13, 2014, 4:05 p.m. UTC | #1
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@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);
>
Wei Ni Jan. 14, 2014, 10:35 a.m. UTC | #2
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@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);
>>
> 
> 

--
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
Eduardo Valentin Jan. 14, 2014, 3:30 p.m. UTC | #3
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@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);
>>>
>>
>>
> 
> 
>
Wei Ni Jan. 15, 2014, 11:52 a.m. UTC | #4
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@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);
>>>>
>>>
>>>
>>
>>
>>
> 
> 
> --
> You have got to be excited about what you are doing. (L. Lamport)
> 
> Eduardo Valentin
> 
> 
> * Unknown Key
> * 0x75D0BCFD
> 

--
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
Eduardo Valentin Jan. 15, 2014, 2:09 p.m. UTC | #5
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@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);
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>> --
>> You have got to be excited about what you are doing. (L. Lamport)
>>
>> Eduardo Valentin
>>
>>
>> * Unknown Key
>> * 0x75D0BCFD
>>
> 
> 
>
Wei Ni Jan. 17, 2014, 7:38 a.m. UTC | #6
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@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);
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> 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
> 

--
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
diff mbox

Patch

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);