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@xxxxxxxxxx>
>>>>> ---
>>>>> 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:
signature.asc
Description: OpenPGP digital signature
