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:

quoted

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;

quoted

+	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.

quoted

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html