? 2016?04?28? 05:48, Eduardo Valentin ??:
> A couple of comments as follows,
>
> On Mon, Apr 25, 2016 at 11:02:44AM +0800, Caesar Wang wrote:
>> From: Sascha Hauer <s.hauer at 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 at kapsi.fi>
>>
>> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
>> Signed-off-by: Caesar Wang <wxt at rock-chips.com>
>> Cc: Zhang Rui <rui.zhang at intel.com>
>> Cc: Eduardo Valentin <edubezval at gmail.com>
>> Cc: linux-pm at vger.kernel.org
>> ---
>>
>> drivers/thermal/thermal_core.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>> include/linux/thermal.h | 3 +++
> Given that this is adding a new feature in the framework, I would prefer
> if you could also describe the .set_trips() in the sysfs-api.txt
> documentation file.
Okay, done.
>
> A short description of the expectation of what the framework is going to
> do is also welcome. For example, are drivers supposed to setup the
> polling together with the threshold based approach?
>
>> 2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index f1db496..cfef8cc 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -520,6 +520,47 @@ exit:
>> }
>> EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>>
>> +static void thermal_zone_set_trips(struct thermal_zone_device *tz)
>> +{
>> + int low = -INT_MAX;
>> + int high = INT_MAX;
>> + int trip_temp, hysteresis;
>> + int temp = tz->temperature;
>> + int i, ret;
>> +
>> + if (!tz->ops->set_trips)
>> + return;
>> +
>> + for (i = 0; i < tz->trips; i++) {
>> + int trip_low;
>> +
>> + tz->ops->get_trip_temp(tz, i, &trip_temp);
>> + tz->ops->get_trip_hyst(tz, i, &hysteresis);
>> +
>> + trip_low = trip_temp - hysteresis;
>> +
>> + if (trip_low < temp && trip_low > low)
>> + low = trip_low;
>> +
>> + if (trip_temp > temp && trip_temp < high)
>> + high = trip_temp;
>> + }
> Did I understand correctly that you will be flooded by IRQs when you
> have:
> 1. One single trip point.
> 2. Your temp is above trip_temp
>
> With the above, you would program as threshold:
> high == trip_temp
> low == trip_temp - hyst
>
> And the IRQ would fire immediattely, causing a device update, causing a
> reprogramming, causing another irq, and this would continue, until the
> temperature goes below trip_temp, right?
Right, As the example tested by the rockchip platform. (70/75/ degree is
the trip point)
..
[ 663.984327] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 66111,
retval: 0
[ 664.048326] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 68333,
retval: 0
[ 664.055600] rockchip-thermal ff260000.tsadc:
rockchip_thermal_set_trips: sensor 1: low: 68000, high 70000
[ 664.992322] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 68888,
retval: 0
[ 664.999579] rockchip-thermal ff260000.tsadc:
rockchip_thermal_set_trips: sensor 0: low: 68000, high 70000
[ 665.066322] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 68333,
retval: 0
...
[ 4250.474102] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 73333,
retval: 0
[ 4250.481432] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 74444,
retval: 0
[ 4250.488792] rockchip-thermal ff260000.tsadc:
rockchip_thermal_set_trips: sensor 1: low: 73000, high 75000
[ 4250.581360] rockchip-thermal ff260000.tsadc: sensor 0 - temp: 72777,
retval: 0
[ 4250.588767] rockchip-thermal ff260000.tsadc:
rockchip_thermal_set_trips: sensor 0: low: 68000, high 75000
[ 4250.598735] rockchip-thermal ff260000.tsadc: sensor 1 - temp: 75000,
retval: 0
[ 4250.606065] rockchip-thermal ff260000.tsadc:
rockchip_thermal_set_trips: sensor 1: low: 73000, high 95000
...
>> +
>> + /* No need to change trip points */
>> + if (tz->prev_low_trip == low && tz->prev_high_trip == high)
>> + return;
>> +
>> + tz->prev_low_trip = low;
>> + tz->prev_high_trip = high;
>> +
>> + dev_dbg(&tz->device, "new temperature boundaries: %d < x < %d\n",
>> + low, high);
>> +
>> + ret = tz->ops->set_trips(tz, low, high);
>> + if (ret)
>> + dev_err(&tz->device, "Failed to set trips: %d\n", ret);
>> +}
>> +
>> static void update_temperature(struct thermal_zone_device *tz)
>> {
>> int temp, ret;
>> @@ -569,6 +610,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 +797,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 would probably want to do the same on trip_point_temp_store().
Sure.
>> return ret ? ret : count;
>> }
>>
>> @@ -1843,6 +1889,8 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>> tz->trips = trips;
>> tz->passive_delay = passive_delay;
>> tz->polling_delay = polling_delay;
>> + tz->prev_low_trip = INT_MAX;
>> + tz->prev_high_trip = -INT_MAX;
>> /* A new thermal zone needs to be updated anyway. */
>> atomic_set(&tz->need_update, 1);
>>
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index e45abe7..e258359 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -98,6 +98,7 @@ struct thermal_zone_device_ops {
>> int (*unbind) (struct thermal_zone_device *,
>> struct thermal_cooling_device *);
>> int (*get_temp) (struct thermal_zone_device *, int *);
>> + int (*set_trips) (struct thermal_zone_device *, int, int);
>> int (*get_mode) (struct thermal_zone_device *,
>> enum thermal_device_mode *);
>> int (*set_mode) (struct thermal_zone_device *,
>> @@ -199,6 +200,8 @@ struct thermal_zone_device {
>> int last_temperature;
>> int emul_temperature;
>> int passive;
>> + int prev_low_trip;
>> + int prev_high_trip;
>> unsigned int forced_passive;
>> atomic_t need_update;
>> struct thermal_zone_device_ops *ops;
>> --
>> 1.9.1
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
--
Thanks,
Caesar
