Re: [PATCHv9 02/20] thermal: introduce device tree parser

[Eduardo Valentin <eduardo.valentin-l0cyMroinI0@public.gmane.org>]

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

On 06-01-2014 22:48, Wei Ni wrote:
> Hi, Eduardo
> Will you consider my comments :)

By now Wei, it is better if you start a new thread, by sending a patch
on top of it, as this thread has been already merged by Rui.

> 
> Thanks.
> Wei.
> 
> On 12/31/2013 06:17 PM, Wei Ni wrote:
>> On 11/13/2013 03:46 AM, Eduardo Valentin wrote:
>>> This patch introduces a device tree bindings for
>>> describing the hardware thermal behavior and limits.
>>> Also a parser to read and interpret the data and feed
>>> it in the thermal framework is presented.
>>>
>>> This patch introduces a thermal data parser for device
>>> tree. The parsed data is used to build thermal zones
>>> and thermal binding parameters. The output data
>>> can then be used to deploy thermal policies.
>>>
>>> This patch adds also documentation regarding this
>>> API and how to define tree nodes to use
>>> this infrastructure.
>>>
>>> Note that, in order to be able to have control
>>> on the sensor registration on the DT thermal zone,
>>> it was required to allow changing the thermal zone
>>> .get_temp callback. For this reason, this patch
>>> also removes the 'const' modifier from the .ops
>>> field of thermal zone devices.
>>>
>>> ...
>>> +
>>> +static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>>> +				enum thermal_trend *trend)
>>> +{
>>> +	struct __thermal_zone *data = tz->devdata;
>>> +	long dev_trend;
>>> +	int r;
>>> +
>>> +	if (!data->get_trend)
>>> +		return -EINVAL;
>>> +
>>> +	r = data->get_trend(data->sensor_data, &dev_trend);
>> I think the ->get_trend should be defined as:
>> .get_trend(*dev, int, *long)
>> so that the "trip" can be passed into it, otherwise the "trip" can't be
>> used.
>> And the dev_trend should be returned as THERMAL_TREND_XXX directly.
>> because the THERM_TREND_xx is more than three states.
>>
>> The code may be something like:
>> r = data->get_trend(data->sensor_data, trip, &dev_trend);
>> if (r)
>>     return r;
>> *trend = dev_trend;
>> return 0;
>>> +	if (r)
>>> +		return r;
>>> +
>>> +	/* TODO: These intervals might have some thresholds, but in core code */
>>> +	if (dev_trend > 0)
>>> +		*trend = THERMAL_TREND_RAISING;
>>> +	else if (dev_trend < 0)
>>> +		*trend = THERMAL_TREND_DROPPING;
>>> +	else
>>> +		*trend = THERMAL_TREND_STABLE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> .....
>>> +
>>> +/***   sensor API   ***/
>>> +
>>> +static struct thermal_zone_device *
>>> +thermal_zone_of_add_sensor(struct device_node *zone,
>>> +			   struct device_node *sensor, void *data,
>>> +			   int (*get_temp)(void *, long *),
>>> +			   int (*get_trend)(void *, long *))
>>> +{
>>> +	struct thermal_zone_device *tzd;
>>> +	struct __thermal_zone *tz;
>>> +
>>> +	tzd = thermal_zone_get_zone_by_name(zone->name);
>> I think we could get the thermal zone by node,
>> something like:
>> thermal_zone_get_zone_by_node(zone);
>> so that it can get unique zone.
>>
>> I think we can add a member "struct device_node *np" in the
>> thermal_zone_device,
>> and set it after you call thermal_zone_device_register successfully.
>> Then add following codes:
>> 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;
>> }
>>
>> Thanks.
>> Wei.
>>> +	if (IS_ERR(tzd))
>>> +		return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +	tz = tzd->devdata;
>>> +
>>> +	mutex_lock(&tzd->lock);
>>> +	tz->get_temp = get_temp;
>>> +	tz->get_trend = get_trend;
>>> +	tz->sensor_data = data;
>>> +
>>> +	tzd->ops->get_temp = of_thermal_get_temp;
>>> +	tzd->ops->get_trend = of_thermal_get_trend;
>>> +	mutex_unlock(&tzd->lock);
>>> +
>>> +	return tzd;
>>> +}
>>> +
>>>
>>
> 
> 
> 


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

Eduardo Valentin


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