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

[Wei Ni <wni@nvidia.com>]

On 01/07/2014 07:17 PM, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> 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.

Ok, I will send it.

Thanks.
Wei.
> 
>>
>> 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;
>>>> +}
>>>> +
>>>>
>>>
>>
>>
>>
> 
>