[PATCH] thermal: document struct thermal_zone_device and thermal_governor

[PATCH] thermal: document struct thermal_zone_device and thermal_governor

[Javi Merino]

Document struct thermal_zone_device and struct thermal_governor fields
and their use by the thermal framework code.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>

---

Hi linux-pm,

I have some patches that add new fields to these structures but I
don't have a good place to describe those fields as these structs are
mostly undocumented so I thought I'd document them.

Changes since v1:
  * Clarified that some parameters are currently only used by the
    step-wise governor.
  * Clarified that forced_passive operates on ACPI processor cooling
    devices.

 include/linux/thermal.h | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f7e11c7ea7d9..6fca46c82c4d 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -158,6 +158,41 @@ struct thermal_attr {
 	char name[THERMAL_NAME_LENGTH];
 };
 
+/**
+ * struct thermal_zone_device - structure for a thermal zone
+ * @id:		unique id number for each thermal zone
+ * @type:	the thermal zone device type
+ * @device:	struct device for this thermal zone
+ * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
+ * @trip_type_attrs:	attributes for trip points for sysfs: trip type
+ * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
+ * @devdata:	private pointer for device private data
+ * @trips:	number of trip points the thermal zone supports
+ * @passive_delay:	number of milliseconds to wait between polls when
+ *			performing passive cooling.  Currenty only used by the
+ *			step-wise governor
+ * @polling_delay:	number of milliseconds to wait between polls when
+ *			checking whether trip points have been crossed (0 for
+ *			interrupt driven systems)
+ * @temperature:	current temperature.  This is only for core code,
+ *			drivers should use thermal_zone_get_temp() to get the
+ *			current temperature
+ * @last_temperature:	previous temperature read
+ * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
+ * @passive:		1 if you've crossed a passive trip point, 0 otherwise.
+ *			Currenty only used by the step-wise governor.
+ * @forced_passive:	If > 0, temperature at which to switch on all ACPI
+ *			processor cooling devices.  Currently only used by the
+ *			step-wise governor.
+ * @ops:	operations this thermal_zone_device supports
+ * @tzp:	thermal zone parameters
+ * @governor:	pointer to the governor for this thermal zone
+ * @thermal_instances:	list of struct thermal_instance of this thermal zone
+ * @idr:	struct idr to generate unique id for this zone's cooling devices
+ * @lock:	lock to protect thermal_instances list
+ * @node:	node in thermal_tz_list (in thermal_core.c)
+ * @poll_queue:	delayed work for polling
+ */
 struct thermal_zone_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
@@ -179,12 +214,18 @@ struct thermal_zone_device {
 	struct thermal_governor *governor;
 	struct list_head thermal_instances;
 	struct idr idr;
-	struct mutex lock; /* protect thermal_instances list */
+	struct mutex lock;
 	struct list_head node;
 	struct delayed_work poll_queue;
 };
 
-/* Structure that holds thermal governor information */
+/**
+ * struct thermal_governor - structure that holds thermal governor information
+ * @name:	name of the governor
+ * @throttle:	callback called for every trip point even if temperature is
+ *		below the trip point temperature
+ * @governor_list:	node in thermal_governor_list (in thermal_core.c)
+ */
 struct thermal_governor {
 	char name[THERMAL_NAME_LENGTH];
 	int (*throttle)(struct thermal_zone_device *tz, int trip);
-- 
1.9.1

Re: [PATCH] thermal: document struct thermal_zone_device and thermal_governor

[Javi Merino]

On Fri, May 23, 2014 at 02:44:51PM +0100, Eduardo Valentin wrote:
> Hello Javi,

Hi Eduardo,

Sorry for the wrong From: in my previous email, I hope that this one
goes out with the correct one.

> On Fri, May 23, 2014 at 10:35:52AM +0100, Javi Merino wrote:
> > On Thu, May 22, 2014 at 04:27:30PM +0100, Eduardo Valentin wrote:
> > > On Fri, May 16, 2014 at 12:16:08PM +0100, Javi Merino wrote:
> > > 
> > > > Document struct struct thermal_zone_device and struct thermal_governor
> > > > fields and their use by the thermal framework code.
> > > > 
> > > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > > Cc: Eduardo Valentin <eduardo.valentin@ti.com>
> > > > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > > > 
> > > > ---
> > > > 
> > > > Hi linux-pm,
> > > > 
> > > > I have some patches that add new fields to these structures but I
> > > > don't have a good place to describe those fields as these structs are
> > > > mostly undocumented so I thought I'd document them.
> > > > 
> > > > I'm unsure about some of the descriptions, specially for passive and
> > > > forced_passive so please review them.
> > > > 
> > > >  include/linux/thermal.h |   44 ++++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > > index f7e11c7ea7d9..af928c667dba 100644
> > > > --- a/include/linux/thermal.h
> > > > +++ b/include/linux/thermal.h
> > > > @@ -158,6 +158,40 @@ struct thermal_attr {
> > > >  	char name[THERMAL_NAME_LENGTH];
> > > >  };
> > > >  
> > > > +/**
> > > > + * struct thermal_zone_device - structure for a thermal zone
> > > > + * @id:		unique id number for each thermal zone
> > > > + * @type:	the thermal zone device type
> > > > + * @device:	struct device for this thermal zone
> > > > + * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
> > > > + * @trip_type_attrs:	attributes for trip points for sysfs: trip type
> > > > + * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
> > > > + * @devdata:	private pointer for device private data
> > > > + * @trips:	number of trip points the thermal zone supports
> > > > + * @passive_delay:	number of milliseconds to wait between polls when
> > > > + *			performing passive cooling.  Only used by the step-wise
> > > 
> > > I don't think this parameter is specific to step-wise.
> > 
> > It's only used by step-wise, all the other governors always poll at
> > polling_delay.
> 
> Agreed, but the fact we have only one governor using it, it does not
> imply it is a specific concept for that governor. The concept can and
> should be reused by other governors. Idea is simple, you have different
> temporal / thermal evolution when idle, when monitoring passive cooling actions. And this fact won't change, no matter the policy you apply.

If this is so generic, shouldn't it be done by the thermal core code
instead of left to the governors?  Something like:

if (temperature > temp_of_first_passive_trip_point)
	tz->passive = 1;

> > While we are at it, can we change the name so that it's more generic and
> > can be reused by other governors?  Something like idle_poll and
> > active_poll?  I'm open to suggestions for a better name.
> > 
> 
> active_poll means when cooling devices are active? which kind of cooling
> devices?

Ok, I guess it's just confusing in my head.  I'll change the power
allocator governor to set tz->passive to true when we want to poll at
a faster rate.

> > > > + *			governor
> > > > + * @polling_delay:	number of milliseconds to wait between polls when
> > > > + *			checking whether trip points have been crossed (0 for
> > > > + *			interrupt driven systems)
> > > > + * @temperature:	current temperature.  This is only for core code,
> > > > + *			drivers should use thermal_zone_get_temp() to get the
> > > > + *			current temperature
> > > > + * @last_temperature:	previous temperature read
> > > > + * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
> > > > + * @passive:	step-wise specific parameter.  1 if you've crossed a passive
> > > > + *		trip point, 0 otherwise
> > > 
> > > ditto.
> > 
> > ditto, no other governor changes passive and it can't be changed from
> > sysfs.
> > 
> > > > + * @forced_passive:	step-wise specific parameter.  If > 0, temperature at
> > > > + *			which to switch on all cpufreq cooling devices.
> > > 
> > > ditto.
> > 
> > Again, this is step-wise specific.  You can change it from sysfs, but
> > the other governors (userspace and fair-share) will happily ignore
> > you.
> > 
> > > Also, governors are not aware of specific cooling devices. 
> > 
> > I didn't say that governors are aware of specific cooling devices.  I
> > said that if forced_passive is greater than 0, then only cpufreq
> > cooling devices are switched on.  It's wrong, it's not cpufreq cooling
> > devices, it's ACPI Processor cooling devices.
> > 
> 
> I mean, we don't have only cpufreq cooling devices. 

True, but forced_passive only acts on ACPI Processor cooling devices,
that's why I was trying to document it.  In
drivers/thermal/thermal_core.c:passive_store():

		list_for_each_entry(cdev, &thermal_cdev_list, node) {
			if (!strncmp("Processor", cdev->type,
				     sizeof("Processor")))
				thermal_zone_bind_cooling_device(tz,
						THERMAL_TRIPS_NONE, cdev,
						THERMAL_NO_LIMIT,
						THERMAL_NO_LIMIT);
		}


By the way, shouldn't that sizeof() be strlen()?

Cheers,
Javi

Re: [PATCH] thermal: document struct thermal_zone_device and thermal_governor

[Eduardo Valentin]

Hello Javi,

On Fri, May 23, 2014 at 10:35:52AM +0100, Javi Merino wrote:
> On Thu, May 22, 2014 at 04:27:30PM +0100, Eduardo Valentin wrote:
> > Hello Javi,
> > 
> > On Fri, May 16, 2014 at 12:16:08PM +0100, Javi Merino wrote:
> > 
> > > Document struct struct thermal_zone_device and struct thermal_governor
> > > fields and their use by the thermal framework code.
> > > 
> > > Cc: Zhang Rui <rui.zhang@intel.com>
> > > Cc: Eduardo Valentin <eduardo.valentin@ti.com>
> > > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > > 
> > > ---
> > > 
> > > Hi linux-pm,
> > > 
> > > I have some patches that add new fields to these structures but I
> > > don't have a good place to describe those fields as these structs are
> > > mostly undocumented so I thought I'd document them.
> > > 
> > > I'm unsure about some of the descriptions, specially for passive and
> > > forced_passive so please review them.
> > > 
> > >  include/linux/thermal.h |   44 ++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index f7e11c7ea7d9..af928c667dba 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -158,6 +158,40 @@ struct thermal_attr {
> > >  	char name[THERMAL_NAME_LENGTH];
> > >  };
> > >  
> > > +/**
> > > + * struct thermal_zone_device - structure for a thermal zone
> > > + * @id:		unique id number for each thermal zone
> > > + * @type:	the thermal zone device type
> > > + * @device:	struct device for this thermal zone
> > > + * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
> > > + * @trip_type_attrs:	attributes for trip points for sysfs: trip type
> > > + * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
> > > + * @devdata:	private pointer for device private data
> > > + * @trips:	number of trip points the thermal zone supports
> > > + * @passive_delay:	number of milliseconds to wait between polls when
> > > + *			performing passive cooling.  Only used by the step-wise
> > 
> > I don't think this parameter is specific to step-wise.
> 
> It's only used by step-wise, all the other governors always poll at
> polling_delay.

Agreed, but the fact we have only one governor using it, it does not
imply it is a specific concept for that governor. The concept can and
should be reused by other governors. Idea is simple, you have different
temporal / thermal evolution when idle, when monitoring passive cooling actions. And this fact won't change, no matter the policy you apply.

> 
> While we are at it, can we change the name so that it's more generic and
> can be reused by other governors?  Something like idle_poll and
> active_poll?  I'm open to suggestions for a better name.
> 

active_poll means when cooling devices are active? which kind of cooling
devices?

> > > + *			governor
> > > + * @polling_delay:	number of milliseconds to wait between polls when
> > > + *			checking whether trip points have been crossed (0 for
> > > + *			interrupt driven systems)
> > > + * @temperature:	current temperature.  This is only for core code,
> > > + *			drivers should use thermal_zone_get_temp() to get the
> > > + *			current temperature
> > > + * @last_temperature:	previous temperature read
> > > + * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
> > > + * @passive:	step-wise specific parameter.  1 if you've crossed a passive
> > > + *		trip point, 0 otherwise
> > 
> > ditto.
> 
> ditto, no other governor changes passive and it can't be changed from
> sysfs.
> 
> > > + * @forced_passive:	step-wise specific parameter.  If > 0, temperature at
> > > + *			which to switch on all cpufreq cooling devices.
> > 
> > ditto.
> 
> Again, this is step-wise specific.  You can change it from sysfs, but
> the other governors (userspace and fair-share) will happily ignore
> you.
> 
> > Also, governors are not aware of specific cooling devices. 
> 
> I didn't say that governors are aware of specific cooling devices.  I
> said that if forced_passive is greater than 0, then only cpufreq
> cooling devices are switched on.  It's wrong, it's not cpufreq cooling
> devices, it's ACPI Processor cooling devices.
> 

I mean, we don't have only cpufreq cooling devices. 

> Cheers,
> Javi

Re: [PATCH] thermal: document struct thermal_zone_device and thermal_governor

[Eduardo Valentin]

Hello Javi,

On Fri, May 16, 2014 at 12:16:08PM +0100, Javi Merino wrote:

> Document struct struct thermal_zone_device and struct thermal_governor
> fields and their use by the thermal framework code.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <eduardo.valentin@ti.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> 
> ---
> 
> Hi linux-pm,
> 
> I have some patches that add new fields to these structures but I
> don't have a good place to describe those fields as these structs are
> mostly undocumented so I thought I'd document them.
> 
> I'm unsure about some of the descriptions, specially for passive and
> forced_passive so please review them.
> 
>  include/linux/thermal.h |   44 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f7e11c7ea7d9..af928c667dba 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -158,6 +158,40 @@ struct thermal_attr {
>  	char name[THERMAL_NAME_LENGTH];
>  };
>  
> +/**
> + * struct thermal_zone_device - structure for a thermal zone
> + * @id:		unique id number for each thermal zone
> + * @type:	the thermal zone device type
> + * @device:	struct device for this thermal zone
> + * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
> + * @trip_type_attrs:	attributes for trip points for sysfs: trip type
> + * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
> + * @devdata:	private pointer for device private data
> + * @trips:	number of trip points the thermal zone supports
> + * @passive_delay:	number of milliseconds to wait between polls when
> + *			performing passive cooling.  Only used by the step-wise

I don't think this parameter is specific to step-wise.

> + *			governor
> + * @polling_delay:	number of milliseconds to wait between polls when
> + *			checking whether trip points have been crossed (0 for
> + *			interrupt driven systems)
> + * @temperature:	current temperature.  This is only for core code,
> + *			drivers should use thermal_zone_get_temp() to get the
> + *			current temperature
> + * @last_temperature:	previous temperature read
> + * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
> + * @passive:	step-wise specific parameter.  1 if you've crossed a passive
> + *		trip point, 0 otherwise

ditto.

> + * @forced_passive:	step-wise specific parameter.  If > 0, temperature at
> + *			which to switch on all cpufreq cooling devices.

ditto.

Also, governors are not aware of specific cooling devices. 


> + * @ops:	operations this thermal_zone_device supports
> + * @tzp:	thermal zone parameters
> + * @governor:	pointer to the governor for this thermal zone
> + * @thermal_instances:	list of struct thermal_instance of this thermal zone
> + * @idr:	struct idr to generate unique id for this zone's cooling devices
> + * @lock:	lock to protect thermal_instances list
> + * @node:	node in thermal_tz_list (in thermal_core.c)
> + * @poll_queue:	delayed work for polling
> + */
>  struct thermal_zone_device {
>  	int id;
>  	char type[THERMAL_NAME_LENGTH];
> @@ -179,12 +213,18 @@ struct thermal_zone_device {
>  	struct thermal_governor *governor;
>  	struct list_head thermal_instances;
>  	struct idr idr;
> -	struct mutex lock; /* protect thermal_instances list */
> +	struct mutex lock;
>  	struct list_head node;
>  	struct delayed_work poll_queue;
>  };
>  
> -/* Structure that holds thermal governor information */
> +/**
> + * struct thermal_governor - structure that holds thermal governor information
> + * @name:	name of the governor
> + * @throttle:	callback called for every trip point even if temperature is
> + *		below the trip point temperature
> + * @governor_list:	node in thermal_governor_list (in thermal_core.c)
> + */
>  struct thermal_governor {
>  	char name[THERMAL_NAME_LENGTH];
>  	int (*throttle)(struct thermal_zone_device *tz, int trip);
> -- 
> 1.7.9.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

[PATCH] thermal: document struct thermal_zone_device and thermal_governor

[Javi Merino]

Document struct struct thermal_zone_device and struct thermal_governor
fields and their use by the thermal framework code.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <eduardo.valentin@ti.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>

---

Hi linux-pm,

I have some patches that add new fields to these structures but I
don't have a good place to describe those fields as these structs are
mostly undocumented so I thought I'd document them.

I'm unsure about some of the descriptions, specially for passive and
forced_passive so please review them.

 include/linux/thermal.h |   44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f7e11c7ea7d9..af928c667dba 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -158,6 +158,40 @@ struct thermal_attr {
 	char name[THERMAL_NAME_LENGTH];
 };
 
+/**
+ * struct thermal_zone_device - structure for a thermal zone
+ * @id:		unique id number for each thermal zone
+ * @type:	the thermal zone device type
+ * @device:	struct device for this thermal zone
+ * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
+ * @trip_type_attrs:	attributes for trip points for sysfs: trip type
+ * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
+ * @devdata:	private pointer for device private data
+ * @trips:	number of trip points the thermal zone supports
+ * @passive_delay:	number of milliseconds to wait between polls when
+ *			performing passive cooling.  Only used by the step-wise
+ *			governor
+ * @polling_delay:	number of milliseconds to wait between polls when
+ *			checking whether trip points have been crossed (0 for
+ *			interrupt driven systems)
+ * @temperature:	current temperature.  This is only for core code,
+ *			drivers should use thermal_zone_get_temp() to get the
+ *			current temperature
+ * @last_temperature:	previous temperature read
+ * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
+ * @passive:	step-wise specific parameter.  1 if you've crossed a passive
+ *		trip point, 0 otherwise
+ * @forced_passive:	step-wise specific parameter.  If > 0, temperature at
+ *			which to switch on all cpufreq cooling devices.
+ * @ops:	operations this thermal_zone_device supports
+ * @tzp:	thermal zone parameters
+ * @governor:	pointer to the governor for this thermal zone
+ * @thermal_instances:	list of struct thermal_instance of this thermal zone
+ * @idr:	struct idr to generate unique id for this zone's cooling devices
+ * @lock:	lock to protect thermal_instances list
+ * @node:	node in thermal_tz_list (in thermal_core.c)
+ * @poll_queue:	delayed work for polling
+ */
 struct thermal_zone_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
@@ -179,12 +213,18 @@ struct thermal_zone_device {
 	struct thermal_governor *governor;
 	struct list_head thermal_instances;
 	struct idr idr;
-	struct mutex lock; /* protect thermal_instances list */
+	struct mutex lock;
 	struct list_head node;
 	struct delayed_work poll_queue;
 };
 
-/* Structure that holds thermal governor information */
+/**
+ * struct thermal_governor - structure that holds thermal governor information
+ * @name:	name of the governor
+ * @throttle:	callback called for every trip point even if temperature is
+ *		below the trip point temperature
+ * @governor_list:	node in thermal_governor_list (in thermal_core.c)
+ */
 struct thermal_governor {
 	char name[THERMAL_NAME_LENGTH];
 	int (*throttle)(struct thermal_zone_device *tz, int trip);
-- 
1.7.9.5