On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
>
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A sensor driver
> callback `set_trips' is then called with the temperatures.
> If there is no trip point above or below the current temperature,
> the passed trip temperature will be LONG_MAX or LONG_MIN respectively.
> In this callback, the driver should program the hardware such that
> it is notified when either of these trip points are triggered.
> When a trip point is triggered, the driver should call
> `thermal_zone_device_update' for the respective thermal zone. This
> will cause the trip points to be updated again.
>
> If the `set_trips' callback is not implemented (is NULL), the framework
> behaves as before.
Is there no "core thermal" code? I would have expected this new feature
to be implemented in "core" code rather than of/dt "support" code.
Perhaps there would also be some additions to the of/dt code, but I'd
still expect the bulk of the feature to be complete independant of
of/dt. Systems still using board files or ACPI or ... would surely
benefit from this too?
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp)
s/tz/tzd/ or s/tz/tzdev/ ? Since "tz" says "thermal zone" to me, but
it's actually a "thermal zone device".
> + struct __thermal_zone *data = tz->devdata;
s/data/tz/ ? "data" is a rather generic term, and "tz" seems like a good
abbreviation for a __thermal_zone.
> + for (i = 0; i < data->ntrips; ++i) {
> + struct __thermal_trip *trip = data->trips + i;
> + long trip_low = trip->temperature - trip->hysteresis;
> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip->temperature > temp && trip->temperature < high)
> + high = trip->temperature;
> + }
That seems to always apply hysteresis to the low end of a trip object.
Don't you need to apply the hysteresis to either the low or high end of
the range, depending on whether the temperature is currently below/above
the range, and hence which direction the edge will be crossed?
Similar comments elsewhere. Perhaps the patch is consistent with some
existing confusing naming style though?
> +static int of_thermal_update_trips(struct thermal_zone_device *tz)
> +{
> + long temp;
> + int err;
> +
> + err = of_thermal_get_temp(tz, &temp);
> + if (err)
> + return err;
> +
> + err = of_thermal_set_trips(tz, temp);
Doesn't this patch modify of_thermal_get_temp() to call
of_thermal_set_trips() itself?
> @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone,
> struct thermal_zone_device *
> thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
> void *data, int (*get_temp)(void *, long *),
> - int (*get_trend)(void *, long *))
> + int (*get_trend)(void *, long *),
> + int (*set_trips)(void *, long, long))
Passing in a struct containing fields for all the ops seem better than
forever extending this function with more and more function pointers.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
