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

[Wei Ni <wni@nvidia.com>]

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;
> +}
> +
>