Re: [PATCH v2 1/5] thermal: Add support for hardware-tracked trip points

[Javi Merino]

Hi Caesar,

On Wed, May 25, 2016 at 11:27:24AM +0800, Caesar Wang wrote:
> On 2016å05æ24æ 20:57, Javi Merino wrote:
> >On Tue, May 03, 2016 at 05:33:29PM +0800, Caesar Wang wrote:
> >>From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> >>
> >>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 .set_trips
> >>callback is then called with the temperatures. If there is no trip
> >>point above or below the current temperature, the passed trip
> >>temperature will be -INT_MAX or INT_MAX 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 .set_trips is not implemented, the framework behaves as before.
> >>
> >>This patch is based on an earlier version from Mikko Perttunen
> >><mikko.perttunen@xxxxxxxx>
> >>
> >>Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> >>Signed-off-by: Caesar Wang <wxt@xxxxxxxxxxxxxx>
> >>Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> >>Cc: Eduardo Valentin <edubezval@xxxxxxxxx>
> >>Cc: linux-pm@xxxxxxxxxxxxxxx
> >>
> >>---
> >>
> >>Changes in v2:
> >>- update the sysfs-api.txt for set_trips
> >>
> >>  Documentation/thermal/sysfs-api.txt |  7 +++++
> >>  drivers/thermal/thermal_core.c      | 52 +++++++++++++++++++++++++++++++++++++
> >>  include/linux/thermal.h             |  3 +++
> >>  3 files changed, 62 insertions(+)
> <cut..>
> >>+	/*
> >>+	 * Set a temperature window. When this window is left the driver
> >>+	 * must inform the thermal core via thermal_zone_device_update.
> >>+	 */
> >>+	ret = tz->ops->set_trips(tz, low, high);
> >>+	if (ret)
> >>+		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
> >This function can be called at the same time from multiple places so
> >it should be reentrant.  I think you should call mutex_lock(tz->lock)
> >before "if (tz->prev_low_trip == low && ..." and unlock it here.
> 
> Sound reasonable, fixes it in next version.
> 
> >>+}
> >>+
> >>  static void update_temperature(struct thermal_zone_device *tz)
> >>  {
> >>  	int temp, ret;
> >>@@ -569,6 +614,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
> >>  	update_temperature(tz);
> >>+	thermal_zone_set_trips(tz);
> >>+
> >>  	for (count = 0; count < tz->trips; count++)
> >>  		handle_thermal_trip(tz, count);
> >>  }
> >>@@ -754,6 +801,9 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
> >>  	 */
> >>  	ret = tz->ops->set_trip_hyst(tz, trip, temperature);
> >>+	if (!ret)
> >>+		thermal_zone_set_trips(tz);
> >>+
> >You should add a similar call to thermal_zone_set_trips() in trip_point_temp_store()
> 
> No, this patch has been done.
> 
> if you see the linux next kernel.
> 72f3ada UPSTREAM: thermal: trip_point_temp_store() calls
> thermal_zone_device_update()
> 
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -733,8 +733,12 @@ trip_point_temp_store(struct device *dev,
> struct device_attribute *attr,
> return -EINVAL;
> 
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
> + if (ret)
> + return ret;
> 
> - return ret ? ret : count;
> + thermal_zone_device_update(tz);
> +
> + return count;
> }
> 
> So the "thermal_zone_set_trips(tz)" have been set in
> thermal_zone_device_update.

Ah, right, I missed that.  Thanks!
Javi