[1/6] thermal: of: Add support for hardware-tracked trip points
diff mbox

Message ID 1403856699-2140-2-git-send-email-mperttunen@nvidia.com
State Changes Requested
Delegated to: Eduardo Valentin
Headers show

Commit Message

Mikko Perttunen June 27, 2014, 8:11 a.m. UTC
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.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/thermal/of-thermal.c | 97 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/thermal.h      |  3 +-
 2 files changed, 95 insertions(+), 5 deletions(-)

Comments

Stephen Warren June 30, 2014, 9:08 p.m. UTC | #1
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-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen July 1, 2014, 7:27 a.m. UTC | #2
Inline.

On 01/07/14 00:08, Stephen Warren wrote:
> 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?

The thermal core only supports a fixed number of trip points for each 
driver and the core informs the driver of any changes to those, so 
drivers using the core framework can already have hardware trip points, 
but just a fixed number of them.

The way of-thermal works, is it reads all the trip points from the 
device tree, registers a new thermal_zone_device with that number of 
trip points and then handles the trip points completely independently. 
Of course, if we're just polling, this is fine, since the thermal core
also knows about those trip points and will trigger cooling when polling 
the each zone. However, the driver doesn't, so it cannot setup any 
interrupts to call thermal_zone_device_update.

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

I followed the existing convention; "tz" is the name used most often by 
both the core and the of-thermal framework.

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

Same, though here both "data" and "tz" seem to be used..

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

I believe applying only to the low end is correct. Say that we have a 
trip point at 40C and hysteresis of 2C. When we exceed 40C cooling will 
start immediately, but it will only be stopped when we cool down to 38C. 
At that point there is again a 2C gap between the current temperature 
and the trip point. It would seem that this is the interpretation used 
by our downstream kernel and also some people on the Internet (however 
trustworthy they may be..)

If you don't feel this is right, please elaborate.

>
> 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?

You're right. I suppose this function is unneeded.

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

Very true.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 1, 2014, 6:15 p.m. UTC | #3
On 07/01/2014 01:27 AM, Mikko Perttunen wrote:
> Inline.
> 
> On 01/07/14 00:08, Stephen Warren wrote:
>> 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?
> 
> The thermal core only supports a fixed number of trip points for each
> driver and the core informs the driver of any changes to those, so
> drivers using the core framework can already have hardware trip points,
> but just a fixed number of them.
> 
> The way of-thermal works, is it reads all the trip points from the
> device tree, registers a new thermal_zone_device with that number of
> trip points and then handles the trip points completely independently.
> Of course, if we're just polling, this is fine, since the thermal core
> also knows about those trip points and will trigger cooling when polling
> the each zone. However, the driver doesn't, so it cannot setup any
> interrupts to call thermal_zone_device_update.

Is there any possibility of cleaning that up? It's obviously horribly
inconsistent if core driver functionality works completely differently
simply because the list of trip-points comes from DT rather than a
static table in the driver. of_thermal should be limited to DT parsing
and related device instantiation/lookup, not introducing a completely
different functionality model.

>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c

>>> +    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?
> 
> I believe applying only to the low end is correct. Say that we have a
> trip point at 40C and hysteresis of 2C. When we exceed 40C cooling will
> start immediately, but it will only be stopped when we cool down to 38C.
> At that point there is again a 2C gap between the current temperature
> and the trip point. It would seem that this is the interpretation used
> by our downstream kernel and also some people on the Internet (however
> trustworthy they may be..)
> 
> If you don't feel this is right, please elaborate.

Ah, the point I was missing is that each trip point is a single
temperature, not a temperature range. As such, the code in your patch is
correct.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen July 3, 2014, 2:15 p.m. UTC | #4
On 01/07/14 21:15, Stephen Warren wrote:
>> The thermal core only supports a fixed number of trip points for each
>> driver and the core informs the driver of any changes to those, so
>> drivers using the core framework can already have hardware trip points,
>> but just a fixed number of them.
>>
>> The way of-thermal works, is it reads all the trip points from the
>> device tree, registers a new thermal_zone_device with that number of
>> trip points and then handles the trip points completely independently.
>> Of course, if we're just polling, this is fine, since the thermal core
>> also knows about those trip points and will trigger cooling when polling
>> the each zone. However, the driver doesn't, so it cannot setup any
>> interrupts to call thermal_zone_device_update.
>
> Is there any possibility of cleaning that up? It's obviously horribly
> inconsistent if core driver functionality works completely differently
> simply because the list of trip-points comes from DT rather than a
> static table in the driver. of_thermal should be limited to DT parsing
> and related device instantiation/lookup, not introducing a completely
> different functionality model.

I guess the smallest possible change would be to add a 
#hardware-trip-cells property to the thermal driver node (this would 
need to designate both the thermal zone and the trip point) and a 
hardware-trip-point phandle node to trip points. Then trip points could 
point to a hardware trip point that would get programmed. Since this is 
just adding properties, it would be backwards-compatible as well.
This is starting to sound like a good idea. Will have to give think 
about it some more.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Longnecker July 21, 2014, 11:53 p.m. UTC | #5
On 7/1/2014 11:15 AM, Stephen Warren wrote:
> On 07/01/2014 01:27 AM, Mikko Perttunen wrote:
>> >Inline.
>> >
>> >On 01/07/14 00:08, Stephen Warren wrote:
>>> >>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?

Stephen, the "core thermal code" defined two major object types:
  * a thermal zone
  * a cooling device

Each thermal zone corresponds to a physical region in space within a 
device. A sane person might expect there to be one physical temperature 
sensor associated with each zone. But, that's not always the case.

Because the thermal core did not expose the concept of a "sensor" 
separately from a "zone", many drivers which register thermal zones are 
messy -- they include a mix of device driver code (i.e. poking at 
register for a thermal sensor IP block) and device-agnostic code for 
implementing behaviors required of a thermal zone.

of-thermal does more than just parse device tree. It registers thermal 
zones using its own implementation of that device-agnostic code. Having 
that centralized implementation of the thermal zone code allows thermal 
sensor device drivers which talk to of-thermal to be simpler than 
drivers which try to talk directly to the thermal core.

The fact that you need to use DT to make use of the simpler sensor 
driver interface doesn't seem like a good reason to push back on Mikko's 
change.

-Matt

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Valentin July 30, 2014, 2:16 p.m. UTC | #6
Terve Mikko,

On Fri, Jun 27, 2014 at 11:11:34AM +0300, 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

One thing I don't follow on your proposal is the groundings you need to
'set_trips' whenever temperature changes. Given your intention is to add
support to interrupt driven devices, shouldn't we 'set_trips' just when
we cross the previously set trips range?

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

As already mentioned by swarren, the proposal must be wider. We shall
keep the same support in case the device is used in a system without
device tree. In other words, if you want to see extra functionality for
interrupt driven devices, you shall update the core part too, and draft
a common messaging path.

In general, interrupt driven devices are not mapped in the current
thermal framework. That is, the current code is timer interrupt driven.
Other interrupt updates from devices are propagated to
the framework using thermal_zone_device_update(). In other words, you would
reprogram your hardware trips from your interrupt handler/workqueue then
just let the framework know what is going on with temperature, via a simple
thermal_zone_device_update().

The way I see this going forward it would be a common interface to
configure the thermal zones to be monitored:
a. via polling only
b. via interrupt only
c. both a + b

obviously, the above shall be informative only for userland.

keep in mind also that changing interrupt configuration for high and low
temperature thresholds can be racy. 

This feature was kept in the TODO list of the of-thermal.c because the
we lack a proper support from the thermal framework (never came out of
the TODO list, I know, I apologize for this). And this missing feature
was spotted by the hwmon folks also, as they do have such support. So,
the major missing improvements on interrupt driven devices shall come in
three steps: (i) thermal framework, (ii) of-thermal (iii) thermal
framework and hwmon interface. 


Cheers,


> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/thermal/of-thermal.c | 97 ++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/thermal.h      |  3 +-
>  2 files changed, 95 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 04b1be7..bfccea5 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -89,6 +89,7 @@ struct __thermal_zone {
>  	/* trip data */
>  	int ntrips;
>  	struct __thermal_trip *trips;
> +	long prev_low_trip, prev_high_trip;
>  
>  	/* cooling binding data */
>  	int num_tbps;
> @@ -98,19 +99,66 @@ struct __thermal_zone {
>  	void *sensor_data;
>  	int (*get_temp)(void *, long *);
>  	int (*get_trend)(void *, long *);
> +	int (*set_trips)(void *, long, long);
>  };
>  
> +/***   Automatic trip handling   ***/
> +
> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp)
> +{
> +	struct __thermal_zone *data = tz->devdata;
> +	long low = LONG_MIN, high = LONG_MAX;
> +	int i;
> +
> +	/* Hardware trip points not supported */
> +	if (!data->set_trips)
> +		return 0;
> +
> +	/* No need to change trip points */
> +	if (temp > data->prev_low_trip && temp < data->prev_high_trip)
> +		return 0;
> +
> +	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;
> +	}
> +
> +	dev_dbg(&tz->device,
> +		"temperature %ld, updating trip points to %ld, %ld\n",
> +		temp, low, high);
> +
> +	data->prev_low_trip = low;
> +	data->prev_high_trip = high;
> +
> +	return data->set_trips(data->sensor_data, low, high);
> +}
> +
>  /***   DT thermal zone device callbacks   ***/
>  
>  static int of_thermal_get_temp(struct thermal_zone_device *tz,
>  			       unsigned long *temp)
>  {
>  	struct __thermal_zone *data = tz->devdata;
> +	int err;
>  
>  	if (!data->get_temp)
>  		return -EINVAL;
>  
> -	return data->get_temp(data->sensor_data, temp);
> +	err = data->get_temp(data->sensor_data, temp);
> +	if (err)
> +		return err;
> +
> +	err = of_thermal_set_trips(tz, *temp);

Here, if you update trips whenever you get_temp, you are possibly
reprogramming your trips on every poll. Remember, this function will be
called on every poll, in the current implementation.

> +	if (err)
> +		return err;
> +
> +	return 0;
>  }
>  
>  static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
> @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
>  	return 0;
>  }
>  
> +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);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>  static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int trip,
>  				    enum thermal_trip_type *type)
>  {
> @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>  				    unsigned long temp)
>  {
>  	struct __thermal_zone *data = tz->devdata;
> +	int err;
>  
>  	if (trip >= data->ntrips || trip < 0)
>  		return -EDOM;
> @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>  	/* thermal framework should take care of data->mask & (1 << trip) */
>  	data->trips[trip].temperature = temp;
>  
> +	err = of_thermal_update_trips(tz);
> +	if (err)
> +		return err;
> +
>  	return 0;
>  }
>  
> @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>  				    unsigned long hyst)
>  {
>  	struct __thermal_zone *data = tz->devdata;
> +	int err;
>  
>  	if (trip >= data->ntrips || trip < 0)
>  		return -EDOM;
> @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>  	/* thermal framework should take care of data->mask & (1 << trip) */
>  	data->trips[trip].hysteresis = hyst;
>  
> +	err = of_thermal_update_trips(tz);
> +	if (err)
> +		return err;
> +
>  	return 0;
>  }
>  
> @@ -325,10 +399,12 @@ 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 *))
> +			   int (*get_trend)(void *, long *),
> +			   int (*set_trips)(void *, long, long))

we need to clean the above arguments. they should become a .ops.

>  {
>  	struct thermal_zone_device *tzd;
>  	struct __thermal_zone *tz;
> +	int err;
>  
>  	tzd = thermal_zone_get_zone_by_name(zone->name);
>  	if (IS_ERR(tzd))
> @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  	mutex_lock(&tzd->lock);
>  	tz->get_temp = get_temp;
>  	tz->get_trend = get_trend;
> +	tz->set_trips = set_trips;
>  	tz->sensor_data = data;
>  
> +	err = of_thermal_update_trips(tzd);
> +	if (err) {
> +		mutex_unlock(&tzd->lock);
> +		return ERR_PTR(err);
> +	}
> +
>  	tzd->ops->get_temp = of_thermal_get_temp;
>  	tzd->ops->get_trend = of_thermal_get_trend;
>  	mutex_unlock(&tzd->lock);
> @@ -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))

ditto.

>  {
>  	struct device_node *np, *child, *sensor_np;
>  
> @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>  			return thermal_zone_of_add_sensor(child, sensor_np,
>  							  data,
>  							  get_temp,
> -							  get_trend);
> +							  get_trend,
> +							  set_trips);

ditto.

>  		}
>  	}
>  	of_node_put(np);
> @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>  
>  	tz->get_temp = NULL;
>  	tz->get_trend = NULL;
> +	tz->set_trips = NULL;
>  	tz->sensor_data = NULL;
>  	mutex_unlock(&tzd->lock);
>  }
> @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np)
>  	/* trips */
>  	child = of_get_child_by_name(np, "trips");
>  
> +	tz->prev_high_trip = LONG_MIN;
> +	tz->prev_low_trip = LONG_MAX;
> +
>  	/* No trips provided */
>  	if (!child)
>  		goto finish;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f7e11c7..2f8951c 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -248,7 +248,8 @@ struct thermal_genl_event {
>  struct thermal_zone_device *
>  thermal_zone_of_sensor_register(struct device *dev, int id,
>  				void *data, int (*get_temp)(void *, long *),
> -				int (*get_trend)(void *, long *));
> +				int (*get_trend)(void *, long *),
> +				int (*set_trips)(void *, long, long));
>  void thermal_zone_of_sensor_unregister(struct device *dev,
>  				       struct thermal_zone_device *tz);
>  #else
> -- 
> 1.8.1.5
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen Aug. 1, 2014, 11:42 a.m. UTC | #7
Moi Eduardo :)

On 30/07/14 17:16, Eduardo Valentin wrote:
> Terve Mikko,
>
> On Fri, Jun 27, 2014 at 11:11:34AM +0300, 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
>
> One thing I don't follow on your proposal is the groundings you need to
> 'set_trips' whenever temperature changes. Given your intention is to add
> support to interrupt driven devices, shouldn't we 'set_trips' just when
> we cross the previously set trips range?

I think the reasoning for this was that I didn't want to make changes to 
thermal_core and thermal_zone_device_update only calls get_temp on the 
zone, so I had to add this code to get_temp. set_trips would anyway only 
be called if we had crossed a trip point.

>
>> 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.
>
> As already mentioned by swarren, the proposal must be wider. We shall
> keep the same support in case the device is used in a system without
> device tree. In other words, if you want to see extra functionality for
> interrupt driven devices, you shall update the core part too, and draft
> a common messaging path.

Yeah, this is sensible. A simpler solution would be to just tell 
of-thermal drivers about the trip points and let the driver do whatever 
it wants. That would mirror the way normal thermal_core drivers are 
done. What is your opinion on that?

>
> In general, interrupt driven devices are not mapped in the current
> thermal framework. That is, the current code is timer interrupt driven.
> Other interrupt updates from devices are propagated to
> the framework using thermal_zone_device_update(). In other words, you would
> reprogram your hardware trips from your interrupt handler/workqueue then
> just let the framework know what is going on with temperature, via a simple
> thermal_zone_device_update().
>
> The way I see this going forward it would be a common interface to
> configure the thermal zones to be monitored:
> a. via polling only
> b. via interrupt only
> c. both a + b
>
> obviously, the above shall be informative only for userland.
>
> keep in mind also that changing interrupt configuration for high and low
> temperature thresholds can be racy.
>
> This feature was kept in the TODO list of the of-thermal.c because the
> we lack a proper support from the thermal framework (never came out of
> the TODO list, I know, I apologize for this). And this missing feature
> was spotted by the hwmon folks also, as they do have such support. So,
> the major missing improvements on interrupt driven devices shall come in
> three steps: (i) thermal framework, (ii) of-thermal (iii) thermal
> framework and hwmon interface.

For now, I think I'll submit a driver with just polling support so that 
we can get some support in.

>
>
> Cheers,
>
>

Thanks, Mikko

>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>   drivers/thermal/of-thermal.c | 97 ++++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/thermal.h      |  3 +-
>>   2 files changed, 95 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index 04b1be7..bfccea5 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -89,6 +89,7 @@ struct __thermal_zone {
>>        /* trip data */
>>        int ntrips;
>>        struct __thermal_trip *trips;
>> +     long prev_low_trip, prev_high_trip;
>>
>>        /* cooling binding data */
>>        int num_tbps;
>> @@ -98,19 +99,66 @@ struct __thermal_zone {
>>        void *sensor_data;
>>        int (*get_temp)(void *, long *);
>>        int (*get_trend)(void *, long *);
>> +     int (*set_trips)(void *, long, long);
>>   };
>>
>> +/***   Automatic trip handling   ***/
>> +
>> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp)
>> +{
>> +     struct __thermal_zone *data = tz->devdata;
>> +     long low = LONG_MIN, high = LONG_MAX;
>> +     int i;
>> +
>> +     /* Hardware trip points not supported */
>> +     if (!data->set_trips)
>> +             return 0;
>> +
>> +     /* No need to change trip points */
>> +     if (temp > data->prev_low_trip && temp < data->prev_high_trip)
>> +             return 0;
>> +
>> +     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;
>> +     }
>> +
>> +     dev_dbg(&tz->device,
>> +             "temperature %ld, updating trip points to %ld, %ld\n",
>> +             temp, low, high);
>> +
>> +     data->prev_low_trip = low;
>> +     data->prev_high_trip = high;
>> +
>> +     return data->set_trips(data->sensor_data, low, high);
>> +}
>> +
>>   /***   DT thermal zone device callbacks   ***/
>>
>>   static int of_thermal_get_temp(struct thermal_zone_device *tz,
>>                               unsigned long *temp)
>>   {
>>        struct __thermal_zone *data = tz->devdata;
>> +     int err;
>>
>>        if (!data->get_temp)
>>                return -EINVAL;
>>
>> -     return data->get_temp(data->sensor_data, temp);
>> +     err = data->get_temp(data->sensor_data, temp);
>> +     if (err)
>> +             return err;
>> +
>> +     err = of_thermal_set_trips(tz, *temp);
>
> Here, if you update trips whenever you get_temp, you are possibly
> reprogramming your trips on every poll. Remember, this function will be
> called on every poll, in the current implementation.
>
>> +     if (err)
>> +             return err;
>> +
>> +     return 0;
>>   }
>>
>>   static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>> @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
>>        return 0;
>>   }
>>
>> +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);
>> +     if (err)
>> +             return err;
>> +
>> +     return 0;
>> +}
>> +
>>   static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int trip,
>>                                    enum thermal_trip_type *type)
>>   {
>> @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>>                                    unsigned long temp)
>>   {
>>        struct __thermal_zone *data = tz->devdata;
>> +     int err;
>>
>>        if (trip >= data->ntrips || trip < 0)
>>                return -EDOM;
>> @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>>        /* thermal framework should take care of data->mask & (1 << trip) */
>>        data->trips[trip].temperature = temp;
>>
>> +     err = of_thermal_update_trips(tz);
>> +     if (err)
>> +             return err;
>> +
>>        return 0;
>>   }
>>
>> @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>>                                    unsigned long hyst)
>>   {
>>        struct __thermal_zone *data = tz->devdata;
>> +     int err;
>>
>>        if (trip >= data->ntrips || trip < 0)
>>                return -EDOM;
>> @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>>        /* thermal framework should take care of data->mask & (1 << trip) */
>>        data->trips[trip].hysteresis = hyst;
>>
>> +     err = of_thermal_update_trips(tz);
>> +     if (err)
>> +             return err;
>> +
>>        return 0;
>>   }
>>
>> @@ -325,10 +399,12 @@ 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 *))
>> +                        int (*get_trend)(void *, long *),
>> +                        int (*set_trips)(void *, long, long))
>
> we need to clean the above arguments. they should become a .ops.
>
>>   {
>>        struct thermal_zone_device *tzd;
>>        struct __thermal_zone *tz;
>> +     int err;
>>
>>        tzd = thermal_zone_get_zone_by_name(zone->name);
>>        if (IS_ERR(tzd))
>> @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>        mutex_lock(&tzd->lock);
>>        tz->get_temp = get_temp;
>>        tz->get_trend = get_trend;
>> +     tz->set_trips = set_trips;
>>        tz->sensor_data = data;
>>
>> +     err = of_thermal_update_trips(tzd);
>> +     if (err) {
>> +             mutex_unlock(&tzd->lock);
>> +             return ERR_PTR(err);
>> +     }
>> +
>>        tzd->ops->get_temp = of_thermal_get_temp;
>>        tzd->ops->get_trend = of_thermal_get_trend;
>>        mutex_unlock(&tzd->lock);
>> @@ -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))
>
> ditto.
>
>>   {
>>        struct device_node *np, *child, *sensor_np;
>>
>> @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>>                        return thermal_zone_of_add_sensor(child, sensor_np,
>>                                                          data,
>>                                                          get_temp,
>> -                                                       get_trend);
>> +                                                       get_trend,
>> +                                                       set_trips);
>
> ditto.
>
>>                }
>>        }
>>        of_node_put(np);
>> @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>>
>>        tz->get_temp = NULL;
>>        tz->get_trend = NULL;
>> +     tz->set_trips = NULL;
>>        tz->sensor_data = NULL;
>>        mutex_unlock(&tzd->lock);
>>   }
>> @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np)
>>        /* trips */
>>        child = of_get_child_by_name(np, "trips");
>>
>> +     tz->prev_high_trip = LONG_MIN;
>> +     tz->prev_low_trip = LONG_MAX;
>> +
>>        /* No trips provided */
>>        if (!child)
>>                goto finish;
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index f7e11c7..2f8951c 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -248,7 +248,8 @@ struct thermal_genl_event {
>>   struct thermal_zone_device *
>>   thermal_zone_of_sensor_register(struct device *dev, int id,
>>                                void *data, int (*get_temp)(void *, long *),
>> -                             int (*get_trend)(void *, long *));
>> +                             int (*get_trend)(void *, long *),
>> +                             int (*set_trips)(void *, long, long));
>>   void thermal_zone_of_sensor_unregister(struct device *dev,
>>                                       struct thermal_zone_device *tz);
>>   #else
>> --
>> 1.8.1.5
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Valentin Aug. 1, 2014, 1:15 p.m. UTC | #8
Moro,

On Fri, Aug 1, 2014 at 7:42 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
> Moi Eduardo :)
>
>
> On 30/07/14 17:16, Eduardo Valentin wrote:
>>
>> Terve Mikko,
>>
>> On Fri, Jun 27, 2014 at 11:11:34AM +0300, 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
>>
>>
>> One thing I don't follow on your proposal is the groundings you need to
>> 'set_trips' whenever temperature changes. Given your intention is to add
>> support to interrupt driven devices, shouldn't we 'set_trips' just when
>> we cross the previously set trips range?
>
>
> I think the reasoning for this was that I didn't want to make changes to
> thermal_core and thermal_zone_device_update only calls get_temp on the zone,
> so I had to add this code to get_temp. set_trips would anyway only be called
> if we had crossed a trip point.
>
>

I see.

>>
>>> 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.
>>
>>
>> As already mentioned by swarren, the proposal must be wider. We shall
>> keep the same support in case the device is used in a system without
>> device tree. In other words, if you want to see extra functionality for
>> interrupt driven devices, you shall update the core part too, and draft
>> a common messaging path.
>
>
> Yeah, this is sensible. A simpler solution would be to just tell of-thermal
> drivers about the trip points and let the driver do whatever it wants. That
> would mirror the way normal thermal_core drivers are done. What is your
> opinion on that?
>
>

In fact, with the current implementation in the thermal framework,
there is nothing else left than coding the feature in the driver
itself. The framework would know only about the existance of the
trips. This approach would need to be cleaned whenever we write the
fix in the core though.

For this reason, I would prefer to clean the core first, then write
the driver as a proof that the changes in core are properly covering
the existing drivers and interrupt driven drivers.

>>
>> In general, interrupt driven devices are not mapped in the current
>> thermal framework. That is, the current code is timer interrupt driven.
>> Other interrupt updates from devices are propagated to
>> the framework using thermal_zone_device_update(). In other words, you
>> would
>> reprogram your hardware trips from your interrupt handler/workqueue then
>> just let the framework know what is going on with temperature, via a
>> simple
>> thermal_zone_device_update().
>>
>> The way I see this going forward it would be a common interface to
>> configure the thermal zones to be monitored:
>> a. via polling only
>> b. via interrupt only
>> c. both a + b
>>
>> obviously, the above shall be informative only for userland.
>>
>> keep in mind also that changing interrupt configuration for high and low
>> temperature thresholds can be racy.
>>
>> This feature was kept in the TODO list of the of-thermal.c because the
>> we lack a proper support from the thermal framework (never came out of
>> the TODO list, I know, I apologize for this). And this missing feature
>> was spotted by the hwmon folks also, as they do have such support. So,
>> the major missing improvements on interrupt driven devices shall come in
>> three steps: (i) thermal framework, (ii) of-thermal (iii) thermal
>> framework and hwmon interface.
>
>
> For now, I think I'll submit a driver with just polling support so that we
> can get some support in.


I see. I will be looking into the changes in core. Whenever I have a
working RFC I will share with the list.

Terveydeksi!,

>
>>
>>
>> Cheers,
>>
>>
>
> Thanks, Mikko
>
>
>>>
>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>> ---
>>>   drivers/thermal/of-thermal.c | 97
>>> ++++++++++++++++++++++++++++++++++++++++++--
>>>   include/linux/thermal.h      |  3 +-
>>>   2 files changed, 95 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>> index 04b1be7..bfccea5 100644
>>> --- a/drivers/thermal/of-thermal.c
>>> +++ b/drivers/thermal/of-thermal.c
>>> @@ -89,6 +89,7 @@ struct __thermal_zone {
>>>        /* trip data */
>>>        int ntrips;
>>>        struct __thermal_trip *trips;
>>> +     long prev_low_trip, prev_high_trip;
>>>
>>>        /* cooling binding data */
>>>        int num_tbps;
>>> @@ -98,19 +99,66 @@ struct __thermal_zone {
>>>        void *sensor_data;
>>>        int (*get_temp)(void *, long *);
>>>        int (*get_trend)(void *, long *);
>>> +     int (*set_trips)(void *, long, long);
>>>   };
>>>
>>> +/***   Automatic trip handling   ***/
>>> +
>>> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long
>>> temp)
>>> +{
>>> +     struct __thermal_zone *data = tz->devdata;
>>> +     long low = LONG_MIN, high = LONG_MAX;
>>> +     int i;
>>> +
>>> +     /* Hardware trip points not supported */
>>> +     if (!data->set_trips)
>>> +             return 0;
>>> +
>>> +     /* No need to change trip points */
>>> +     if (temp > data->prev_low_trip && temp < data->prev_high_trip)
>>> +             return 0;
>>> +
>>> +     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;
>>> +     }
>>> +
>>> +     dev_dbg(&tz->device,
>>> +             "temperature %ld, updating trip points to %ld, %ld\n",
>>> +             temp, low, high);
>>> +
>>> +     data->prev_low_trip = low;
>>> +     data->prev_high_trip = high;
>>> +
>>> +     return data->set_trips(data->sensor_data, low, high);
>>> +}
>>> +
>>>   /***   DT thermal zone device callbacks   ***/
>>>
>>>   static int of_thermal_get_temp(struct thermal_zone_device *tz,
>>>                               unsigned long *temp)
>>>   {
>>>        struct __thermal_zone *data = tz->devdata;
>>> +     int err;
>>>
>>>        if (!data->get_temp)
>>>                return -EINVAL;
>>>
>>> -     return data->get_temp(data->sensor_data, temp);
>>> +     err = data->get_temp(data->sensor_data, temp);
>>> +     if (err)
>>> +             return err;
>>> +
>>> +     err = of_thermal_set_trips(tz, *temp);
>>
>>
>> Here, if you update trips whenever you get_temp, you are possibly
>> reprogramming your trips on every poll. Remember, this function will be
>> called on every poll, in the current implementation.
>>
>>> +     if (err)
>>> +             return err;
>>> +
>>> +     return 0;
>>>   }
>>>
>>>   static int of_thermal_get_trend(struct thermal_zone_device *tz, int
>>> trip,
>>> @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct
>>> thermal_zone_device *tz,
>>>        return 0;
>>>   }
>>>
>>> +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);
>>> +     if (err)
>>> +             return err;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>   static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int
>>> trip,
>>>                                    enum thermal_trip_type *type)
>>>   {
>>> @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct
>>> thermal_zone_device *tz, int trip,
>>>                                    unsigned long temp)
>>>   {
>>>        struct __thermal_zone *data = tz->devdata;
>>> +     int err;
>>>
>>>        if (trip >= data->ntrips || trip < 0)
>>>                return -EDOM;
>>> @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct
>>> thermal_zone_device *tz, int trip,
>>>        /* thermal framework should take care of data->mask & (1 << trip)
>>> */
>>>        data->trips[trip].temperature = temp;
>>>
>>> +     err = of_thermal_update_trips(tz);
>>> +     if (err)
>>> +             return err;
>>> +
>>>        return 0;
>>>   }
>>>
>>> @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct
>>> thermal_zone_device *tz, int trip,
>>>                                    unsigned long hyst)
>>>   {
>>>        struct __thermal_zone *data = tz->devdata;
>>> +     int err;
>>>
>>>        if (trip >= data->ntrips || trip < 0)
>>>                return -EDOM;
>>> @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct
>>> thermal_zone_device *tz, int trip,
>>>        /* thermal framework should take care of data->mask & (1 << trip)
>>> */
>>>        data->trips[trip].hysteresis = hyst;
>>>
>>> +     err = of_thermal_update_trips(tz);
>>> +     if (err)
>>> +             return err;
>>> +
>>>        return 0;
>>>   }
>>>
>>> @@ -325,10 +399,12 @@ 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 *))
>>> +                        int (*get_trend)(void *, long *),
>>> +                        int (*set_trips)(void *, long, long))
>>
>>
>> we need to clean the above arguments. they should become a .ops.
>>
>>>   {
>>>        struct thermal_zone_device *tzd;
>>>        struct __thermal_zone *tz;
>>> +     int err;
>>>
>>>        tzd = thermal_zone_get_zone_by_name(zone->name);
>>>        if (IS_ERR(tzd))
>>> @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>>        mutex_lock(&tzd->lock);
>>>        tz->get_temp = get_temp;
>>>        tz->get_trend = get_trend;
>>> +     tz->set_trips = set_trips;
>>>        tz->sensor_data = data;
>>>
>>> +     err = of_thermal_update_trips(tzd);
>>> +     if (err) {
>>> +             mutex_unlock(&tzd->lock);
>>> +             return ERR_PTR(err);
>>> +     }
>>> +
>>>        tzd->ops->get_temp = of_thermal_get_temp;
>>>        tzd->ops->get_trend = of_thermal_get_trend;
>>>        mutex_unlock(&tzd->lock);
>>> @@ -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))
>>
>>
>> ditto.
>>
>>>   {
>>>        struct device_node *np, *child, *sensor_np;
>>>
>>> @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev,
>>> int sensor_id,
>>>                        return thermal_zone_of_add_sensor(child,
>>> sensor_np,
>>>                                                          data,
>>>                                                          get_temp,
>>> -                                                       get_trend);
>>> +                                                       get_trend,
>>> +                                                       set_trips);
>>
>>
>> ditto.
>>
>>>                }
>>>        }
>>>        of_node_put(np);
>>> @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device
>>> *dev,
>>>
>>>        tz->get_temp = NULL;
>>>        tz->get_trend = NULL;
>>> +     tz->set_trips = NULL;
>>>        tz->sensor_data = NULL;
>>>        mutex_unlock(&tzd->lock);
>>>   }
>>> @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np)
>>>        /* trips */
>>>        child = of_get_child_by_name(np, "trips");
>>>
>>> +     tz->prev_high_trip = LONG_MIN;
>>> +     tz->prev_low_trip = LONG_MAX;
>>> +
>>>        /* No trips provided */
>>>        if (!child)
>>>                goto finish;
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index f7e11c7..2f8951c 100644
>>> --- a/include/linux/thermal.h
>>> +++ b/include/linux/thermal.h
>>> @@ -248,7 +248,8 @@ struct thermal_genl_event {
>>>   struct thermal_zone_device *
>>>   thermal_zone_of_sensor_register(struct device *dev, int id,
>>>                                void *data, int (*get_temp)(void *, long
>>> *),
>>> -                             int (*get_trend)(void *, long *));
>>> +                             int (*get_trend)(void *, long *),
>>> +                             int (*set_trips)(void *, long, long));
>>>   void thermal_zone_of_sensor_unregister(struct device *dev,
>>>                                       struct thermal_zone_device *tz);
>>>   #else
>>> --
>>> 1.8.1.5
>>>
>>
>

Patch
diff mbox

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 04b1be7..bfccea5 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -89,6 +89,7 @@  struct __thermal_zone {
 	/* trip data */
 	int ntrips;
 	struct __thermal_trip *trips;
+	long prev_low_trip, prev_high_trip;
 
 	/* cooling binding data */
 	int num_tbps;
@@ -98,19 +99,66 @@  struct __thermal_zone {
 	void *sensor_data;
 	int (*get_temp)(void *, long *);
 	int (*get_trend)(void *, long *);
+	int (*set_trips)(void *, long, long);
 };
 
+/***   Automatic trip handling   ***/
+
+static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp)
+{
+	struct __thermal_zone *data = tz->devdata;
+	long low = LONG_MIN, high = LONG_MAX;
+	int i;
+
+	/* Hardware trip points not supported */
+	if (!data->set_trips)
+		return 0;
+
+	/* No need to change trip points */
+	if (temp > data->prev_low_trip && temp < data->prev_high_trip)
+		return 0;
+
+	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;
+	}
+
+	dev_dbg(&tz->device,
+		"temperature %ld, updating trip points to %ld, %ld\n",
+		temp, low, high);
+
+	data->prev_low_trip = low;
+	data->prev_high_trip = high;
+
+	return data->set_trips(data->sensor_data, low, high);
+}
+
 /***   DT thermal zone device callbacks   ***/
 
 static int of_thermal_get_temp(struct thermal_zone_device *tz,
 			       unsigned long *temp)
 {
 	struct __thermal_zone *data = tz->devdata;
+	int err;
 
 	if (!data->get_temp)
 		return -EINVAL;
 
-	return data->get_temp(data->sensor_data, temp);
+	err = data->get_temp(data->sensor_data, temp);
+	if (err)
+		return err;
+
+	err = of_thermal_set_trips(tz, *temp);
+	if (err)
+		return err;
+
+	return 0;
 }
 
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
@@ -222,6 +270,22 @@  static int of_thermal_set_mode(struct thermal_zone_device *tz,
 	return 0;
 }
 
+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);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int trip,
 				    enum thermal_trip_type *type)
 {
@@ -252,6 +316,7 @@  static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
 				    unsigned long temp)
 {
 	struct __thermal_zone *data = tz->devdata;
+	int err;
 
 	if (trip >= data->ntrips || trip < 0)
 		return -EDOM;
@@ -259,6 +324,10 @@  static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
 	/* thermal framework should take care of data->mask & (1 << trip) */
 	data->trips[trip].temperature = temp;
 
+	err = of_thermal_update_trips(tz);
+	if (err)
+		return err;
+
 	return 0;
 }
 
@@ -279,6 +348,7 @@  static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
 				    unsigned long hyst)
 {
 	struct __thermal_zone *data = tz->devdata;
+	int err;
 
 	if (trip >= data->ntrips || trip < 0)
 		return -EDOM;
@@ -286,6 +356,10 @@  static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
 	/* thermal framework should take care of data->mask & (1 << trip) */
 	data->trips[trip].hysteresis = hyst;
 
+	err = of_thermal_update_trips(tz);
+	if (err)
+		return err;
+
 	return 0;
 }
 
@@ -325,10 +399,12 @@  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 *))
+			   int (*get_trend)(void *, long *),
+			   int (*set_trips)(void *, long, long))
 {
 	struct thermal_zone_device *tzd;
 	struct __thermal_zone *tz;
+	int err;
 
 	tzd = thermal_zone_get_zone_by_name(zone->name);
 	if (IS_ERR(tzd))
@@ -339,8 +415,15 @@  thermal_zone_of_add_sensor(struct device_node *zone,
 	mutex_lock(&tzd->lock);
 	tz->get_temp = get_temp;
 	tz->get_trend = get_trend;
+	tz->set_trips = set_trips;
 	tz->sensor_data = data;
 
+	err = of_thermal_update_trips(tzd);
+	if (err) {
+		mutex_unlock(&tzd->lock);
+		return ERR_PTR(err);
+	}
+
 	tzd->ops->get_temp = of_thermal_get_temp;
 	tzd->ops->get_trend = of_thermal_get_trend;
 	mutex_unlock(&tzd->lock);
@@ -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))
 {
 	struct device_node *np, *child, *sensor_np;
 
@@ -422,7 +506,8 @@  thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
 			return thermal_zone_of_add_sensor(child, sensor_np,
 							  data,
 							  get_temp,
-							  get_trend);
+							  get_trend,
+							  set_trips);
 		}
 	}
 	of_node_put(np);
@@ -466,6 +551,7 @@  void thermal_zone_of_sensor_unregister(struct device *dev,
 
 	tz->get_temp = NULL;
 	tz->get_trend = NULL;
+	tz->set_trips = NULL;
 	tz->sensor_data = NULL;
 	mutex_unlock(&tzd->lock);
 }
@@ -671,6 +757,9 @@  thermal_of_build_thermal_zone(struct device_node *np)
 	/* trips */
 	child = of_get_child_by_name(np, "trips");
 
+	tz->prev_high_trip = LONG_MIN;
+	tz->prev_low_trip = LONG_MAX;
+
 	/* No trips provided */
 	if (!child)
 		goto finish;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f7e11c7..2f8951c 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -248,7 +248,8 @@  struct thermal_genl_event {
 struct thermal_zone_device *
 thermal_zone_of_sensor_register(struct device *dev, int id,
 				void *data, int (*get_temp)(void *, long *),
-				int (*get_trend)(void *, long *));
+				int (*get_trend)(void *, long *),
+				int (*set_trips)(void *, long, long));
 void thermal_zone_of_sensor_unregister(struct device *dev,
 				       struct thermal_zone_device *tz);
 #else