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

[Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

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
>