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

Javi Merino
2016-05-25T16:44:21Z

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@pengutronix.de>
> >>
> >>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@kapsi.fi>
> >>
> >>Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >>Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> >>Cc: Zhang Rui <rui.zhang@intel.com>
> >>Cc: Eduardo Valentin <edubezval@gmail.com>
> >>Cc: linux-pm@vger.kernel.org
> >>
> >>---
> >>
> >>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