On 01/08/2014 08:31 PM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
>
> On 08-01-2014 07:10, R, Durgadoss wrote:
>> Hi Wei,
>>
>>> -----Original Message-----
>>> From: [email protected] [mailto:linux-pm-
>>> [email protected]] On Behalf Of Wei Ni
>>> Sent: Wednesday, January 08, 2014 2:37 PM
>>> To: [email protected]; Zhang, Rui; [email protected]
>>> Cc: [email protected]; [email protected]; linux-
>>> [email protected]; [email protected]; Wei Ni
>>> Subject: [PATCH] thermal: use device node to get thermal zone
>>>
>>> If use name to get the thermal zone, sometimes it can't
>>> get the unique thermal zone, because the thermal fw support
>>> same name for different thermal zone.
>>> So we can change to use device node to get thermal zone.
>
> Please check the documentation for .*by_name function. There is an error
> code for the case of multiple instances. In fact, it is designed to be
> used by users who know that in the system there is going to exist only
> one zone with that name.
>
> I don't think it is correct to simply remove the existing API and
> enforce DT usage. What if the system does not use DT?
>
>>
>> Eduardo introduced this API because we wanted to get the
>> tzd pointer by using the name. So, for your case, can you
>> introduce a new API *get_by_node instead of changing this
>> API ?
>
> Agreed, if you need a search by DT node, then you need to provide the
> use cases and propose a new one.
Yes, you are right, I should not remove it simply, I will add a new API
.*by_node.
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. If use the *.get_by_name, it may not get the uniqu
one, because I don't know if there has the same name thermal zone,
because some other driver may not use DT to register thermal zone, it
can define any name by itself.
>
>>
>> Thanks,
>> Durga
>>
>>>
>>> Signed-off-by: Wei Ni <[email protected]>
>>> ---
>>> drivers/thermal/of-thermal.c | 6 ++++--
>>> drivers/thermal/thermal_core.c | 37 ++++++++++++++++---------------------
>>> include/linux/thermal.h | 4 +++-
>>> 3 files changed, 23 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>> index 04b1be7..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);
>
> In DT, nodes have unique names. Are you seen a problem with the above code?
this file is used for DT parse, I think it's better to use .*by_node
instead.
>
>>> if (IS_ERR(tzd))
>>> return ERR_PTR(-EPROBE_DEFER);
>>>
>>> @@ -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;
>>> @@ -837,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;
>>>
>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>> index 338a88b..89e0637 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -1635,42 +1635,37 @@ void thermal_zone_device_unregister(struct
>>> thermal_zone_device *tz)
>>> EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
>>>
>>> /**
>>> - * thermal_zone_get_zone_by_name() - search for a zone and returns its ref
>>> - * @name: thermal zone name to fetch the temperature
>>> + * thermal_zone_get_zone_by_node() - search for a zone and returns its ref
>>> + * @node: device node of the thermal zone
>>> *
>>> - * When only one zone is found with the passed name, returns a reference
>>> to it.
>>> + * 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 name equals to @name, an ERR_PTR otherwise (-EINVAL for invalid
>>> - * paramenters, -ENODEV for not found and -EEXIST for multiple matches).
>>> + * matching device node, an ERR_PTR otherwise (-EINVAL for invalid
>>> + * paramenters, -ENODEV for not found).
>>> */
>>> -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)
>>> {
>
> You are removing an API, are you checking that all existing users of
> this API are changed accordingly? I don't think so.
Sorry, it's my mistake, I will not remove it in my next pactch.
>
>>> - struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-EINVAL);
>>> - unsigned int found = 0;
>>> + struct thermal_zone_device *pos = NULL, *ref = ERR_PTR(-ENODEV);
>>> + bool found = false;
>>>
>>> - if (!name)
>>> - goto exit;
>>> + if (!node)
>>> + return ERR_PTR(-EINVAL);
>>>
>>> mutex_lock(&thermal_list_lock);
>>> list_for_each_entry(pos, &thermal_tz_list, node)
>>> - if (!strnicmp(name, pos->type, THERMAL_NAME_LENGTH)) {
>>> - found++;
>>> + if (node == pos->np) {
>>> ref = pos;
>>> + found = true;
>>> + break;
>>> }
>>> mutex_unlock(&thermal_list_lock);
>>>
>>> - /* nothing has been found, thus an error code for it */
>>> - if (found == 0)
>>> - ref = ERR_PTR(-ENODEV);
>>> - else if (found > 1)
>>> - /* Success only when an unique zone is found */
>>> - ref = ERR_PTR(-EEXIST);
>>> -
>>> -exit:
>>> return ref;
>>> }
>>> -EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
>>> +EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_node);
>>>
>>> #ifdef CONFIG_NET
>>> static const struct genl_multicast_group thermal_event_mcgrps[] = {
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index f7e11c7..a94de8c 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;
>
> Please describe better if you want to add this node in here. Better to
> add it in a separated patch.
>
>>> struct thermal_attr *trip_temp_attrs;
>>> struct thermal_attr *trip_type_attrs;
>>> struct thermal_attr *trip_hyst_attrs;
>>> @@ -285,7 +286,8 @@ struct thermal_cooling_device *
>>> 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
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
