Hi, Eduardo
Will you consider my comments :)
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;
>> +}
>> +
>>
>
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
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;
>>>> +}
>>>> +
>>>>
>>>
>>
>>
>>
>
>