[PATCH 00/13] Thermal Framework Enhancements

[PATCH 00/13] Thermal Framework Enhancements

[Durgadoss R]

This patch series attempts to enhance the thermal framework
by adding some simple governors/throttling algorithms.

These patches are based on Rui's tree here: (branch - thermal)
git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git

Patch 1: Refactor thermal.h so that it is easy to read/maintain
Patch 2: Move data structures that need to be visible to thermal.h
Patch 3: Adds some APIs to thermal_sys.c
Patch 4: Adds required platform layer information to thermal.h
Patch 5: Adds code to thermal_sys, that can retrieve platform data
         (if provided) for a thermal zone
Patch 6: Creates a policy sysfs attribute (RO, for now)
Patch 7: Modifies the bind/unbind logic based on the platform data
	 With this modification, the bind/unbind can work with or
	 without platform data
Patch 8: Introduces a fair_share governor. This throttles the
	 cooling_devices according to their weights. The weights
	 in turn describe the effectiveness of a particular
	 cooling device in cooling a thermal zone.
Patch 9: Introduce step_wise governor
	 This throttles/de-throttles the cooling devices one
	 step at a time. This is exactly similar to the code
	 we have in thermal_zone_device_update function. The
	 intention is to move all 'throttling logic' related
	 code outside thermal_sys.c and keep them separate.
Patch 10: Removes all throttling code outside thermal_sys.c
Patch 11: Adds a notification API so that thermal sensor drivers
	  can notify the framework of any thermal events.
Patch 12: Adds documentation to Documentation/thermal/sysfs-api.txt
Patch 13: Shows some sample code on how to provide platform level
	  thermal data. This patch is not for merge.

Durgadoss R (13):
  Thermal: Refactor thermal.h file
  Thermal: Move thermal_instance to thermal.h
  Thermal: Add get trend, get instance API's to thermal_sys
  Thermal: Add platform level information to thermal.h
  Thermal: Obtain platform data for thermal zone
  Thermal: Add a policy sysfs attribute
  Thermal: Update binding logic based on platform data
  Thermal: Introduce fair_share thermal governor
  Thermal: Introduce a step_wise thermal governor
  Thermal: Remove throttling logic out of thermal_sys.c
  Thermal: Add a notification API
  Thermal: Add documentation for platform layer data
  Thermal: Platform layer changes to provide thermal data

 Documentation/thermal/sysfs-api.txt |   30 ++
 arch/x86/platform/mrst/mrst.c       |   42 +++
 drivers/thermal/Kconfig             |   12 +
 drivers/thermal/Makefile            |    4 +-
 drivers/thermal/fair_share.c        |  128 +++++++
 drivers/thermal/step_wise.c         |  204 +++++++++++
 drivers/thermal/thermal_sys.c       |  673 +++++++++++++++++++++--------------
 include/linux/thermal.h             |  167 +++++++--
 8 files changed, 963 insertions(+), 297 deletions(-)
 create mode 100644 drivers/thermal/fair_share.c
 create mode 100644 drivers/thermal/step_wise.c

-- 
1.7.9.5

[PATCH 01/13] Thermal: Refactor thermal.h file

[Durgadoss R]

This file rearranges the code in thermal.h file,
in the following order, so that it is easy to
read/maintain.
1. All #defines
2. All enums
3. All fops structures
4. All device structures
5. All function declarations

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 include/linux/thermal.h |   78 ++++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 35 deletions(-)

diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 91b3481..8611e3e 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -29,6 +29,23 @@
 #include <linux/device.h>
 #include <linux/workqueue.h>
 
+#define THERMAL_TRIPS_NONE	-1
+#define THERMAL_MAX_TRIPS	12
+#define THERMAL_NAME_LENGTH	20
+
+/* No upper/lower limit requirement */
+#define THERMAL_NO_LIMIT	-1UL
+
+/* Unit conversion macros */
+#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
+				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
+#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
+
+/* Adding event notification support elements */
+#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
+#define THERMAL_GENL_VERSION                    0x01
+#define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
+
 struct thermal_zone_device;
 struct thermal_cooling_device;
 
@@ -50,6 +67,30 @@ enum thermal_trend {
 	THERMAL_TREND_DROPPING, /* temperature is dropping */
 };
 
+/* Events supported by Thermal Netlink */
+enum events {
+	THERMAL_AUX0,
+	THERMAL_AUX1,
+	THERMAL_CRITICAL,
+	THERMAL_DEV_FAULT,
+};
+
+/* attributes of thermal_genl_family */
+enum {
+	THERMAL_GENL_ATTR_UNSPEC,
+	THERMAL_GENL_ATTR_EVENT,
+	__THERMAL_GENL_ATTR_MAX,
+};
+#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
+
+/* commands supported by the thermal_genl_family */
+enum {
+	THERMAL_GENL_CMD_UNSPEC,
+	THERMAL_GENL_CMD_EVENT,
+	__THERMAL_GENL_CMD_MAX,
+};
+#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
+
 struct thermal_zone_device_ops {
 	int (*bind) (struct thermal_zone_device *,
 		     struct thermal_cooling_device *);
@@ -83,11 +124,6 @@ struct thermal_cooling_device_ops {
 	int (*set_cur_state) (struct thermal_cooling_device *, unsigned long);
 };
 
-#define THERMAL_NO_LIMIT -1UL /* no upper/lower limit requirement */
-
-#define THERMAL_TRIPS_NONE -1
-#define THERMAL_MAX_TRIPS 12
-#define THERMAL_NAME_LENGTH 20
 struct thermal_cooling_device {
 	int id;
 	char type[THERMAL_NAME_LENGTH];
@@ -100,10 +136,6 @@ struct thermal_cooling_device {
 	struct list_head node;
 };
 
-#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
-				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
-#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
-
 struct thermal_attr {
 	struct device_attribute attr;
 	char name[THERMAL_NAME_LENGTH];
@@ -131,38 +163,13 @@ struct thermal_zone_device {
 	struct list_head node;
 	struct delayed_work poll_queue;
 };
-/* Adding event notification support elements */
-#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
-#define THERMAL_GENL_VERSION                    0x01
-#define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
-
-enum events {
-	THERMAL_AUX0,
-	THERMAL_AUX1,
-	THERMAL_CRITICAL,
-	THERMAL_DEV_FAULT,
-};
 
 struct thermal_genl_event {
 	u32 orig;
 	enum events event;
 };
-/* attributes of thermal_genl_family */
-enum {
-	THERMAL_GENL_ATTR_UNSPEC,
-	THERMAL_GENL_ATTR_EVENT,
-	__THERMAL_GENL_ATTR_MAX,
-};
-#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
-
-/* commands supported by the thermal_genl_family */
-enum {
-	THERMAL_GENL_CMD_UNSPEC,
-	THERMAL_GENL_CMD_EVENT,
-	__THERMAL_GENL_CMD_MAX,
-};
-#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
 
+/* Function declarations */
 struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
 		void *, const struct thermal_zone_device_ops *, int, int);
 void thermal_zone_device_unregister(struct thermal_zone_device *);
@@ -173,6 +180,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
 				       struct thermal_cooling_device *);
 void thermal_zone_device_update(struct thermal_zone_device *);
+
 struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
 		const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
-- 
1.7.9.5

Re: [PATCH 01/13] Thermal: Refactor thermal.h file

[Eduardo Valentin]

Hello Durga,

On Thu, Aug 09, 2012 at 06:15:53PM +0530, Durgadoss R wrote:
> This file rearranges the code in thermal.h file,

I suppose you meant 'this patch'?

> in the following order, so that it is easy to
> read/maintain.
> 1. All #defines
> 2. All enums
> 3. All fops structures
> 4. All device structures
> 5. All function declarations

Cool!

> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  include/linux/thermal.h |   78 ++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 91b3481..8611e3e 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -29,6 +29,23 @@
>  #include <linux/device.h>
>  #include <linux/workqueue.h>
>  
> +#define THERMAL_TRIPS_NONE	-1
> +#define THERMAL_MAX_TRIPS	12
> +#define THERMAL_NAME_LENGTH	20
> +
> +/* No upper/lower limit requirement */
> +#define THERMAL_NO_LIMIT	-1UL
> +
> +/* Unit conversion macros */
> +#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> +				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
> +#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
> +
> +/* Adding event notification support elements */
> +#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> +#define THERMAL_GENL_VERSION                    0x01
> +#define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
> +
>  struct thermal_zone_device;
>  struct thermal_cooling_device;
>  
> @@ -50,6 +67,30 @@ enum thermal_trend {
>  	THERMAL_TREND_DROPPING, /* temperature is dropping */
>  };
>  
> +/* Events supported by Thermal Netlink */
> +enum events {
> +	THERMAL_AUX0,
> +	THERMAL_AUX1,
> +	THERMAL_CRITICAL,
> +	THERMAL_DEV_FAULT,
> +};
> +
> +/* attributes of thermal_genl_family */
> +enum {
> +	THERMAL_GENL_ATTR_UNSPEC,
> +	THERMAL_GENL_ATTR_EVENT,
> +	__THERMAL_GENL_ATTR_MAX,
> +};
> +#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
> +
> +/* commands supported by the thermal_genl_family */
> +enum {
> +	THERMAL_GENL_CMD_UNSPEC,
> +	THERMAL_GENL_CMD_EVENT,
> +	__THERMAL_GENL_CMD_MAX,
> +};
> +#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> +
>  struct thermal_zone_device_ops {
>  	int (*bind) (struct thermal_zone_device *,
>  		     struct thermal_cooling_device *);
> @@ -83,11 +124,6 @@ struct thermal_cooling_device_ops {
>  	int (*set_cur_state) (struct thermal_cooling_device *, unsigned long);
>  };
>  
> -#define THERMAL_NO_LIMIT -1UL /* no upper/lower limit requirement */
> -
> -#define THERMAL_TRIPS_NONE -1
> -#define THERMAL_MAX_TRIPS 12
> -#define THERMAL_NAME_LENGTH 20
>  struct thermal_cooling_device {
>  	int id;
>  	char type[THERMAL_NAME_LENGTH];
> @@ -100,10 +136,6 @@ struct thermal_cooling_device {
>  	struct list_head node;
>  };
>  
> -#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> -				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
> -#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
> -
>  struct thermal_attr {
>  	struct device_attribute attr;
>  	char name[THERMAL_NAME_LENGTH];
> @@ -131,38 +163,13 @@ struct thermal_zone_device {
>  	struct list_head node;
>  	struct delayed_work poll_queue;
>  };
> -/* Adding event notification support elements */
> -#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> -#define THERMAL_GENL_VERSION                    0x01
> -#define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
> -
> -enum events {
> -	THERMAL_AUX0,
> -	THERMAL_AUX1,
> -	THERMAL_CRITICAL,
> -	THERMAL_DEV_FAULT,
> -};
>  
>  struct thermal_genl_event {
>  	u32 orig;
>  	enum events event;
>  };
> -/* attributes of thermal_genl_family */
> -enum {
> -	THERMAL_GENL_ATTR_UNSPEC,
> -	THERMAL_GENL_ATTR_EVENT,
> -	__THERMAL_GENL_ATTR_MAX,
> -};
> -#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
> -
> -/* commands supported by the thermal_genl_family */
> -enum {
> -	THERMAL_GENL_CMD_UNSPEC,
> -	THERMAL_GENL_CMD_EVENT,
> -	__THERMAL_GENL_CMD_MAX,
> -};
> -#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
>  
> +/* Function declarations */

for the functions exposed in this header,
should we also provide function nops when CONFIG_THERMAL is not set?

>  struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
>  		void *, const struct thermal_zone_device_ops *, int, int);
>  void thermal_zone_device_unregister(struct thermal_zone_device *);
> @@ -173,6 +180,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
>  int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
>  				       struct thermal_cooling_device *);
>  void thermal_zone_device_update(struct thermal_zone_device *);
> +
>  struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
>  		const struct thermal_cooling_device_ops *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
> -- 
> 1.7.9.5
> 

RE: [PATCH 01/13] Thermal: Refactor thermal.h file

[R, Durgadoss]

Hi Eduardo,

> -----Original Message-----
> From: Eduardo Valentin [mailto:eduardo.valentin@ti.com]
> Sent: Monday, August 20, 2012 9:28 PM
> To: R, Durgadoss
> Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> amit.kachhap@linaro.org; wni@nvidia.com
> Subject: Re: [PATCH 01/13] Thermal: Refactor thermal.h file
> 
> Hello Durga,
> 
> On Thu, Aug 09, 2012 at 06:15:53PM +0530, Durgadoss R wrote:
> > This file rearranges the code in thermal.h file,
> 
> I suppose you meant 'this patch'?

Oh yes. v2 on the way, will fix it there.

> 
> > in the following order, so that it is easy to
> > read/maintain.
> > 1. All #defines
> > 2. All enums
> > 3. All fops structures
> > 4. All device structures
> > 5. All function declarations
> 
> Cool!
> 
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  include/linux/thermal.h |   78 ++++++++++++++++++++++++++-------------
> --------
> >  1 file changed, 43 insertions(+), 35 deletions(-)
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 91b3481..8611e3e 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -29,6 +29,23 @@
> >  #include <linux/device.h>
> >  #include <linux/workqueue.h>
> >
> > +#define THERMAL_TRIPS_NONE	-1
> > +#define THERMAL_MAX_TRIPS	12
> > +#define THERMAL_NAME_LENGTH	20
> > +
> > +/* No upper/lower limit requirement */
> > +#define THERMAL_NO_LIMIT	-1UL
> > +
> > +/* Unit conversion macros */
> > +#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > +				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
> > +#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
> > +
> > +/* Adding event notification support elements */
> > +#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> > +#define THERMAL_GENL_VERSION                    0x01
> > +#define THERMAL_GENL_MCAST_GROUP_NAME
> "thermal_mc_group"
> > +
> >  struct thermal_zone_device;
> >  struct thermal_cooling_device;
> >
> > @@ -50,6 +67,30 @@ enum thermal_trend {
> >  	THERMAL_TREND_DROPPING, /* temperature is dropping */
> >  };
> >
> > +/* Events supported by Thermal Netlink */
> > +enum events {
> > +	THERMAL_AUX0,
> > +	THERMAL_AUX1,
> > +	THERMAL_CRITICAL,
> > +	THERMAL_DEV_FAULT,
> > +};
> > +
> > +/* attributes of thermal_genl_family */
> > +enum {
> > +	THERMAL_GENL_ATTR_UNSPEC,
> > +	THERMAL_GENL_ATTR_EVENT,
> > +	__THERMAL_GENL_ATTR_MAX,
> > +};
> > +#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX -
> 1)
> > +
> > +/* commands supported by the thermal_genl_family */
> > +enum {
> > +	THERMAL_GENL_CMD_UNSPEC,
> > +	THERMAL_GENL_CMD_EVENT,
> > +	__THERMAL_GENL_CMD_MAX,
> > +};
> > +#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> > +
> >  struct thermal_zone_device_ops {
> >  	int (*bind) (struct thermal_zone_device *,
> >  		     struct thermal_cooling_device *);
> > @@ -83,11 +124,6 @@ struct thermal_cooling_device_ops {
> >  	int (*set_cur_state) (struct thermal_cooling_device *, unsigned
> long);
> >  };
> >
> > -#define THERMAL_NO_LIMIT -1UL /* no upper/lower limit requirement
> */
> > -
> > -#define THERMAL_TRIPS_NONE -1
> > -#define THERMAL_MAX_TRIPS 12
> > -#define THERMAL_NAME_LENGTH 20
> >  struct thermal_cooling_device {
> >  	int id;
> >  	char type[THERMAL_NAME_LENGTH];
> > @@ -100,10 +136,6 @@ struct thermal_cooling_device {
> >  	struct list_head node;
> >  };
> >
> > -#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > -				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
> > -#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
> > -
> >  struct thermal_attr {
> >  	struct device_attribute attr;
> >  	char name[THERMAL_NAME_LENGTH];
> > @@ -131,38 +163,13 @@ struct thermal_zone_device {
> >  	struct list_head node;
> >  	struct delayed_work poll_queue;
> >  };
> > -/* Adding event notification support elements */
> > -#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> > -#define THERMAL_GENL_VERSION                    0x01
> > -#define THERMAL_GENL_MCAST_GROUP_NAME
> "thermal_mc_group"
> > -
> > -enum events {
> > -	THERMAL_AUX0,
> > -	THERMAL_AUX1,
> > -	THERMAL_CRITICAL,
> > -	THERMAL_DEV_FAULT,
> > -};
> >
> >  struct thermal_genl_event {
> >  	u32 orig;
> >  	enum events event;
> >  };
> > -/* attributes of thermal_genl_family */
> > -enum {
> > -	THERMAL_GENL_ATTR_UNSPEC,
> > -	THERMAL_GENL_ATTR_EVENT,
> > -	__THERMAL_GENL_ATTR_MAX,
> > -};
> > -#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
> > -
> > -/* commands supported by the thermal_genl_family */
> > -enum {
> > -	THERMAL_GENL_CMD_UNSPEC,
> > -	THERMAL_GENL_CMD_EVENT,
> > -	__THERMAL_GENL_CMD_MAX,
> > -};
> > -#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> >
> > +/* Function declarations */
> 
> for the functions exposed in this header,
> should we also provide function nops when CONFIG_THERMAL is not set?
> 

Theoretically agree with you.
We will not enable thermal_sys.c if CONFIG_THERMAL is not set;
and hence no driver should be using these API's in this case.
So, I think it is better to throw errors rather than the driver assuming
Thermal management will be done.
Would prefer to leave it this way..

Thanks,
Durga

Re: [PATCH 01/13] Thermal: Refactor thermal.h file

[Eduardo Valentin]

Hello,

On Mon, Aug 20, 2012 at 04:42:12PM +0000, R, Durgadoss wrote:
> Hi Eduardo,
> 
> > -----Original Message-----
> > From: Eduardo Valentin [mailto:eduardo.valentin@ti.com]
> > Sent: Monday, August 20, 2012 9:28 PM
> > To: R, Durgadoss
> > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > amit.kachhap@linaro.org; wni@nvidia.com
> > Subject: Re: [PATCH 01/13] Thermal: Refactor thermal.h file
> > 
> > Hello Durga,
> > 
> > On Thu, Aug 09, 2012 at 06:15:53PM +0530, Durgadoss R wrote:
> > > This file rearranges the code in thermal.h file,
> > 
> > I suppose you meant 'this patch'?
> 
> Oh yes. v2 on the way, will fix it there.

Oh.. Ok. I will go through V1 anyway, just in case.

> 
> > 
> > > in the following order, so that it is easy to
> > > read/maintain.
> > > 1. All #defines
> > > 2. All enums
> > > 3. All fops structures
> > > 4. All device structures
> > > 5. All function declarations
> > 
> > Cool!
> > 
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > ---
> > >  include/linux/thermal.h |   78 ++++++++++++++++++++++++++-------------
> > --------
> > >  1 file changed, 43 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index 91b3481..8611e3e 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -29,6 +29,23 @@
> > >  #include <linux/device.h>
> > >  #include <linux/workqueue.h>
> > >
> > > +#define THERMAL_TRIPS_NONE	-1
> > > +#define THERMAL_MAX_TRIPS	12
> > > +#define THERMAL_NAME_LENGTH	20
> > > +
> > > +/* No upper/lower limit requirement */
> > > +#define THERMAL_NO_LIMIT	-1UL
> > > +
> > > +/* Unit conversion macros */
> > > +#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > > +				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
> > > +#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
> > > +
> > > +/* Adding event notification support elements */
> > > +#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> > > +#define THERMAL_GENL_VERSION                    0x01
> > > +#define THERMAL_GENL_MCAST_GROUP_NAME
> > "thermal_mc_group"
> > > +
> > >  struct thermal_zone_device;
> > >  struct thermal_cooling_device;
> > >
> > > @@ -50,6 +67,30 @@ enum thermal_trend {
> > >  	THERMAL_TREND_DROPPING, /* temperature is dropping */
> > >  };
> > >
> > > +/* Events supported by Thermal Netlink */
> > > +enum events {
> > > +	THERMAL_AUX0,
> > > +	THERMAL_AUX1,
> > > +	THERMAL_CRITICAL,
> > > +	THERMAL_DEV_FAULT,
> > > +};
> > > +
> > > +/* attributes of thermal_genl_family */
> > > +enum {
> > > +	THERMAL_GENL_ATTR_UNSPEC,
> > > +	THERMAL_GENL_ATTR_EVENT,
> > > +	__THERMAL_GENL_ATTR_MAX,
> > > +};
> > > +#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX -
> > 1)
> > > +
> > > +/* commands supported by the thermal_genl_family */
> > > +enum {
> > > +	THERMAL_GENL_CMD_UNSPEC,
> > > +	THERMAL_GENL_CMD_EVENT,
> > > +	__THERMAL_GENL_CMD_MAX,
> > > +};
> > > +#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> > > +
> > >  struct thermal_zone_device_ops {
> > >  	int (*bind) (struct thermal_zone_device *,
> > >  		     struct thermal_cooling_device *);
> > > @@ -83,11 +124,6 @@ struct thermal_cooling_device_ops {
> > >  	int (*set_cur_state) (struct thermal_cooling_device *, unsigned
> > long);
> > >  };
> > >
> > > -#define THERMAL_NO_LIMIT -1UL /* no upper/lower limit requirement
> > */
> > > -
> > > -#define THERMAL_TRIPS_NONE -1
> > > -#define THERMAL_MAX_TRIPS 12
> > > -#define THERMAL_NAME_LENGTH 20
> > >  struct thermal_cooling_device {
> > >  	int id;
> > >  	char type[THERMAL_NAME_LENGTH];
> > > @@ -100,10 +136,6 @@ struct thermal_cooling_device {
> > >  	struct list_head node;
> > >  };
> > >
> > > -#define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
> > > -				((long)t-2732+5)/10 : ((long)t-2732-5)/10)
> > > -#define CELSIUS_TO_KELVIN(t)	((t)*10+2732)
> > > -
> > >  struct thermal_attr {
> > >  	struct device_attribute attr;
> > >  	char name[THERMAL_NAME_LENGTH];
> > > @@ -131,38 +163,13 @@ struct thermal_zone_device {
> > >  	struct list_head node;
> > >  	struct delayed_work poll_queue;
> > >  };
> > > -/* Adding event notification support elements */
> > > -#define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> > > -#define THERMAL_GENL_VERSION                    0x01
> > > -#define THERMAL_GENL_MCAST_GROUP_NAME
> > "thermal_mc_group"
> > > -
> > > -enum events {
> > > -	THERMAL_AUX0,
> > > -	THERMAL_AUX1,
> > > -	THERMAL_CRITICAL,
> > > -	THERMAL_DEV_FAULT,
> > > -};
> > >
> > >  struct thermal_genl_event {
> > >  	u32 orig;
> > >  	enum events event;
> > >  };
> > > -/* attributes of thermal_genl_family */
> > > -enum {
> > > -	THERMAL_GENL_ATTR_UNSPEC,
> > > -	THERMAL_GENL_ATTR_EVENT,
> > > -	__THERMAL_GENL_ATTR_MAX,
> > > -};
> > > -#define THERMAL_GENL_ATTR_MAX (__THERMAL_GENL_ATTR_MAX - 1)
> > > -
> > > -/* commands supported by the thermal_genl_family */
> > > -enum {
> > > -	THERMAL_GENL_CMD_UNSPEC,
> > > -	THERMAL_GENL_CMD_EVENT,
> > > -	__THERMAL_GENL_CMD_MAX,
> > > -};
> > > -#define THERMAL_GENL_CMD_MAX (__THERMAL_GENL_CMD_MAX - 1)
> > >
> > > +/* Function declarations */
> > 
> > for the functions exposed in this header,
> > should we also provide function nops when CONFIG_THERMAL is not set?
> > 
> 
> Theoretically agree with you.
> We will not enable thermal_sys.c if CONFIG_THERMAL is not set;
> and hence no driver should be using these API's in this case.
> So, I think it is better to throw errors rather than the driver assuming
> Thermal management will be done.
> Would prefer to leave it this way..
> 
> Thanks,
> Durga

[PATCH 02/13] Thermal: Move thermal_instance to thermal.h

[Durgadoss R]

This patch moves the thermal_instance structure
to thermal.h so that thermal management files
(other than thermal_sys.c) can also access it.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |   21 ---------------------
 include/linux/thermal.h       |   23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 5be8728..a0e20f9 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -41,27 +41,6 @@ MODULE_AUTHOR("Zhang Rui");
 MODULE_DESCRIPTION("Generic thermal management sysfs support");
 MODULE_LICENSE("GPL");
 
-#define THERMAL_NO_TARGET -1UL
-/*
- * This structure is used to describe the behavior of
- * a certain cooling device on a certain trip point
- * in a certain thermal zone
- */
-struct thermal_instance {
-	int id;
-	char name[THERMAL_NAME_LENGTH];
-	struct thermal_zone_device *tz;
-	struct thermal_cooling_device *cdev;
-	int trip;
-	unsigned long upper;	/* Highest cooling state for this trip point */
-	unsigned long lower;	/* Lowest cooling state for this trip point */
-	unsigned long target;	/* expected cooling state */
-	char attr_name[THERMAL_NAME_LENGTH];
-	struct device_attribute attr;
-	struct list_head tz_node; /* node in tz->thermal_instances */
-	struct list_head cdev_node; /* node in cdev->thermal_instances */
-};
-
 static DEFINE_IDR(thermal_tz_idr);
 static DEFINE_IDR(thermal_cdev_idr);
 static DEFINE_MUTEX(thermal_idr_lock);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 8611e3e..f25df23 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -33,6 +33,9 @@
 #define THERMAL_MAX_TRIPS	12
 #define THERMAL_NAME_LENGTH	20
 
+/* Initial state of a cooling device during binding */
+#define THERMAL_NO_TARGET	-1UL
+
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT	-1UL
 
@@ -164,6 +167,26 @@ struct thermal_zone_device {
 	struct delayed_work poll_queue;
 };
 
+/*
+ * This structure is used to describe the behavior of
+ * a certain cooling device on a certain trip point
+ * in a certain thermal zone
+ */
+struct thermal_instance {
+	int id;
+	char name[THERMAL_NAME_LENGTH];
+	struct thermal_zone_device *tz;
+	struct thermal_cooling_device *cdev;
+	int trip;
+	unsigned long upper;	/* Highest cooling state for this trip point */
+	unsigned long lower;	/* Lowest cooling state for this trip point */
+	unsigned long target;	/* expected cooling state */
+	char attr_name[THERMAL_NAME_LENGTH];
+	struct device_attribute attr;
+	struct list_head tz_node; /* node in tz->thermal_instances */
+	struct list_head cdev_node; /* node in cdev->thermal_instances */
+};
+
 struct thermal_genl_event {
 	u32 orig;
 	enum events event;
-- 
1.7.9.5

Re: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h

[Zhang Rui]

On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> This patch moves the thermal_instance structure
> to thermal.h so that thermal management files
> (other than thermal_sys.c) can also access it.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |   21 ---------------------
>  include/linux/thermal.h       |   23 +++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 5be8728..a0e20f9 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -41,27 +41,6 @@ MODULE_AUTHOR("Zhang Rui");
>  MODULE_DESCRIPTION("Generic thermal management sysfs support");
>  MODULE_LICENSE("GPL");
>  
> -#define THERMAL_NO_TARGET -1UL
> -/*
> - * This structure is used to describe the behavior of
> - * a certain cooling device on a certain trip point
> - * in a certain thermal zone
> - */
> -struct thermal_instance {
> -	int id;
> -	char name[THERMAL_NAME_LENGTH];
> -	struct thermal_zone_device *tz;
> -	struct thermal_cooling_device *cdev;
> -	int trip;
> -	unsigned long upper;	/* Highest cooling state for this trip point */
> -	unsigned long lower;	/* Lowest cooling state for this trip point */
> -	unsigned long target;	/* expected cooling state */
> -	char attr_name[THERMAL_NAME_LENGTH];
> -	struct device_attribute attr;
> -	struct list_head tz_node; /* node in tz->thermal_instances */
> -	struct list_head cdev_node; /* node in cdev->thermal_instances */
> -};
> -
>  static DEFINE_IDR(thermal_tz_idr);
>  static DEFINE_IDR(thermal_cdev_idr);
>  static DEFINE_MUTEX(thermal_idr_lock);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 8611e3e..f25df23 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -33,6 +33,9 @@
>  #define THERMAL_MAX_TRIPS	12
>  #define THERMAL_NAME_LENGTH	20
>  
> +/* Initial state of a cooling device during binding */
> +#define THERMAL_NO_TARGET	-1UL
> +
>  /* No upper/lower limit requirement */
>  #define THERMAL_NO_LIMIT	-1UL
>  
> @@ -164,6 +167,26 @@ struct thermal_zone_device {
>  	struct delayed_work poll_queue;
>  };
>  
> +/*
> + * This structure is used to describe the behavior of
> + * a certain cooling device on a certain trip point
> + * in a certain thermal zone
> + */
> +struct thermal_instance {
> +	int id;
> +	char name[THERMAL_NAME_LENGTH];
> +	struct thermal_zone_device *tz;
> +	struct thermal_cooling_device *cdev;
> +	int trip;
> +	unsigned long upper;	/* Highest cooling state for this trip point */
> +	unsigned long lower;	/* Lowest cooling state for this trip point */
> +	unsigned long target;	/* expected cooling state */
> +	char attr_name[THERMAL_NAME_LENGTH];
> +	struct device_attribute attr;
> +	struct list_head tz_node; /* node in tz->thermal_instances */
> +	struct list_head cdev_node; /* node in cdev->thermal_instances */
> +};
> +

as this structure is used internally only, I'm thinking if we can rename
drivers/thermal/thermal_sys.c to drivers/thermal/thermal_core.c,
and introduce drivers/thermal/thermal_core.h for these internal stuff.
what do you think?

thanks,
rui
>  struct thermal_genl_event {
>  	u32 orig;
>  	enum events event;


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h

[R, Durgadoss]

Hi Rui,


> -----Original Message-----
> From: Zhang, Rui
> Sent: Thursday, August 16, 2012 11:44 AM
> To: R, Durgadoss
> Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> wni@nvidia.com
> Subject: Re: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h
> 
> On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> > This patch moves the thermal_instance structure
> > to thermal.h so that thermal management files
> > (other than thermal_sys.c) can also access it.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  drivers/thermal/thermal_sys.c |   21 ---------------------
> >  include/linux/thermal.h       |   23 +++++++++++++++++++++++
> >  2 files changed, 23 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 5be8728..a0e20f9 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -41,27 +41,6 @@ MODULE_AUTHOR("Zhang Rui");
> >  MODULE_DESCRIPTION("Generic thermal management sysfs support");
> >  MODULE_LICENSE("GPL");
> >
> > -#define THERMAL_NO_TARGET -1UL
> > -/*
> > - * This structure is used to describe the behavior of
> > - * a certain cooling device on a certain trip point
> > - * in a certain thermal zone
> > - */
> > -struct thermal_instance {
> > -	int id;
> > -	char name[THERMAL_NAME_LENGTH];
> > -	struct thermal_zone_device *tz;
> > -	struct thermal_cooling_device *cdev;
> > -	int trip;
> > -	unsigned long upper;	/* Highest cooling state for this trip point */
> > -	unsigned long lower;	/* Lowest cooling state for this trip point */
> > -	unsigned long target;	/* expected cooling state */
> > -	char attr_name[THERMAL_NAME_LENGTH];
> > -	struct device_attribute attr;
> > -	struct list_head tz_node; /* node in tz->thermal_instances */
> > -	struct list_head cdev_node; /* node in cdev->thermal_instances */
> > -};
> > -
> >  static DEFINE_IDR(thermal_tz_idr);
> >  static DEFINE_IDR(thermal_cdev_idr);
> >  static DEFINE_MUTEX(thermal_idr_lock);
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 8611e3e..f25df23 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -33,6 +33,9 @@
> >  #define THERMAL_MAX_TRIPS	12
> >  #define THERMAL_NAME_LENGTH	20
> >
> > +/* Initial state of a cooling device during binding */
> > +#define THERMAL_NO_TARGET	-1UL
> > +
> >  /* No upper/lower limit requirement */
> >  #define THERMAL_NO_LIMIT	-1UL
> >
> > @@ -164,6 +167,26 @@ struct thermal_zone_device {
> >  	struct delayed_work poll_queue;
> >  };
> >
> > +/*
> > + * This structure is used to describe the behavior of
> > + * a certain cooling device on a certain trip point
> > + * in a certain thermal zone
> > + */
> > +struct thermal_instance {
> > +	int id;
> > +	char name[THERMAL_NAME_LENGTH];
> > +	struct thermal_zone_device *tz;
> > +	struct thermal_cooling_device *cdev;
> > +	int trip;
> > +	unsigned long upper;	/* Highest cooling state for this trip point */
> > +	unsigned long lower;	/* Lowest cooling state for this trip point */
> > +	unsigned long target;	/* expected cooling state */
> > +	char attr_name[THERMAL_NAME_LENGTH];
> > +	struct device_attribute attr;
> > +	struct list_head tz_node; /* node in tz->thermal_instances */
> > +	struct list_head cdev_node; /* node in cdev->thermal_instances */
> > +};
> > +
> 
> as this structure is used internally only, I'm thinking if we can rename
> drivers/thermal/thermal_sys.c to drivers/thermal/thermal_core.c,
> and introduce drivers/thermal/thermal_core.h for these internal stuff.
> what do you think?

Yes agree with you.
Also, we can keep the sysfs things in thermal_sys.c 
and rest of the things in thermal_core.c, and have a thermal_core.h also.
(This is how the power supply subsystem does it)

I will include this clean up, as part of v2, if you are Ok with this.

Other things;
 I was thinking is 'removal of netlink things' from
thermal_sys.c

Removing the hwmon related code (the thermal subsystem has grown
quite a bit and provides more thermal functionalities than Hwmon)
So, why do we need CONFIG_HWMON inside thermal subsystem ?
If all of us agree, I am Ok to remove this also.

Thanks,
Durga

RE: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h

[Zhang Rui]

On 四, 2012-08-16 at 00:19 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> 
> > -----Original Message-----
> > From: Zhang, Rui
> > Sent: Thursday, August 16, 2012 11:44 AM
> > To: R, Durgadoss
> > Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> > pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> > wni@nvidia.com
> > Subject: Re: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h
> > 
> > On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> > > This patch moves the thermal_instance structure
> > > to thermal.h so that thermal management files
> > > (other than thermal_sys.c) can also access it.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > ---
> > >  drivers/thermal/thermal_sys.c |   21 ---------------------
> > >  include/linux/thermal.h       |   23 +++++++++++++++++++++++
> > >  2 files changed, 23 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > > index 5be8728..a0e20f9 100644
> > > --- a/drivers/thermal/thermal_sys.c
> > > +++ b/drivers/thermal/thermal_sys.c
> > > @@ -41,27 +41,6 @@ MODULE_AUTHOR("Zhang Rui");
> > >  MODULE_DESCRIPTION("Generic thermal management sysfs support");
> > >  MODULE_LICENSE("GPL");
> > >
> > > -#define THERMAL_NO_TARGET -1UL
> > > -/*
> > > - * This structure is used to describe the behavior of
> > > - * a certain cooling device on a certain trip point
> > > - * in a certain thermal zone
> > > - */
> > > -struct thermal_instance {
> > > -	int id;
> > > -	char name[THERMAL_NAME_LENGTH];
> > > -	struct thermal_zone_device *tz;
> > > -	struct thermal_cooling_device *cdev;
> > > -	int trip;
> > > -	unsigned long upper;	/* Highest cooling state for this trip point */
> > > -	unsigned long lower;	/* Lowest cooling state for this trip point */
> > > -	unsigned long target;	/* expected cooling state */
> > > -	char attr_name[THERMAL_NAME_LENGTH];
> > > -	struct device_attribute attr;
> > > -	struct list_head tz_node; /* node in tz->thermal_instances */
> > > -	struct list_head cdev_node; /* node in cdev->thermal_instances */
> > > -};
> > > -
> > >  static DEFINE_IDR(thermal_tz_idr);
> > >  static DEFINE_IDR(thermal_cdev_idr);
> > >  static DEFINE_MUTEX(thermal_idr_lock);
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index 8611e3e..f25df23 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -33,6 +33,9 @@
> > >  #define THERMAL_MAX_TRIPS	12
> > >  #define THERMAL_NAME_LENGTH	20
> > >
> > > +/* Initial state of a cooling device during binding */
> > > +#define THERMAL_NO_TARGET	-1UL
> > > +
> > >  /* No upper/lower limit requirement */
> > >  #define THERMAL_NO_LIMIT	-1UL
> > >
> > > @@ -164,6 +167,26 @@ struct thermal_zone_device {
> > >  	struct delayed_work poll_queue;
> > >  };
> > >
> > > +/*
> > > + * This structure is used to describe the behavior of
> > > + * a certain cooling device on a certain trip point
> > > + * in a certain thermal zone
> > > + */
> > > +struct thermal_instance {
> > > +	int id;
> > > +	char name[THERMAL_NAME_LENGTH];
> > > +	struct thermal_zone_device *tz;
> > > +	struct thermal_cooling_device *cdev;
> > > +	int trip;
> > > +	unsigned long upper;	/* Highest cooling state for this trip point */
> > > +	unsigned long lower;	/* Lowest cooling state for this trip point */
> > > +	unsigned long target;	/* expected cooling state */
> > > +	char attr_name[THERMAL_NAME_LENGTH];
> > > +	struct device_attribute attr;
> > > +	struct list_head tz_node; /* node in tz->thermal_instances */
> > > +	struct list_head cdev_node; /* node in cdev->thermal_instances */
> > > +};
> > > +
> > 
> > as this structure is used internally only, I'm thinking if we can rename
> > drivers/thermal/thermal_sys.c to drivers/thermal/thermal_core.c,
> > and introduce drivers/thermal/thermal_core.h for these internal stuff.
> > what do you think?
> 
> Yes agree with you.
> Also, we can keep the sysfs things in thermal_sys.c 
> and rest of the things in thermal_core.c, and have a thermal_core.h also.
> (This is how the power supply subsystem does it)
> 
> I will include this clean up, as part of v2, if you are Ok with this.
> 
yes, please go ahead.

> Other things;
>  I was thinking is 'removal of netlink things' from
> thermal_sys.c

and where to move it to?
> 
> Removing the hwmon related code (the thermal subsystem has grown
> quite a bit and provides more thermal functionalities than Hwmon)
> So, why do we need CONFIG_HWMON inside thermal subsystem ?
> If all of us agree, I am Ok to remove this also.
> 
we need Jean's opinion on this.
But anyway, we can do this at anytime, if really needed.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h

[R, Durgadoss]

Hi Rui,

[cut.]
> > > > +/*
> > > > + * This structure is used to describe the behavior of
> > > > + * a certain cooling device on a certain trip point
> > > > + * in a certain thermal zone
> > > > + */
> > > > +struct thermal_instance {
> > > > +	int id;
> > > > +	char name[THERMAL_NAME_LENGTH];
> > > > +	struct thermal_zone_device *tz;
> > > > +	struct thermal_cooling_device *cdev;
> > > > +	int trip;
> > > > +	unsigned long upper;	/* Highest cooling state for this trip point */
> > > > +	unsigned long lower;	/* Lowest cooling state for this trip point */
> > > > +	unsigned long target;	/* expected cooling state */
> > > > +	char attr_name[THERMAL_NAME_LENGTH];
> > > > +	struct device_attribute attr;
> > > > +	struct list_head tz_node; /* node in tz->thermal_instances */
> > > > +	struct list_head cdev_node; /* node in cdev->thermal_instances */
> > > > +};
> > > > +
> > >
> > > as this structure is used internally only, I'm thinking if we can rename
> > > drivers/thermal/thermal_sys.c to drivers/thermal/thermal_core.c,
> > > and introduce drivers/thermal/thermal_core.h for these internal stuff.
> > > what do you think?
> >
> > Yes agree with you.
> > Also, we can keep the sysfs things in thermal_sys.c
> > and rest of the things in thermal_core.c, and have a thermal_core.h also.
> > (This is how the power supply subsystem does it)
> >
> > I will include this clean up, as part of v2, if you are Ok with this.
> >
> yes, please go ahead.

Ok. I will include this change.

> 
> > Other things;
> >  I was thinking is 'removal of netlink things' from
> > thermal_sys.c
> 
> and where to move it to?

I was thinking of completely removing this, as raw netlink
usage is phasing out, and kobj_uevent () is being used
increasingly.

But we will keep it as a separate change, and not club with this one.

> >
> > Removing the hwmon related code (the thermal subsystem has grown
> > quite a bit and provides more thermal functionalities than Hwmon)
> > So, why do we need CONFIG_HWMON inside thermal subsystem ?
> > If all of us agree, I am Ok to remove this also.
> >
> we need Jean's opinion on this.
> But anyway, we can do this at anytime, if really needed.

Yes, will wait for Jean's thoughts..

Thanks,
Durga

> 
> thanks,
> rui

RE: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h

[Zhang Rui]

On 四, 2012-08-16 at 00:31 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> [cut.]
> > > > > +/*
> > > > > + * This structure is used to describe the behavior of
> > > > > + * a certain cooling device on a certain trip point
> > > > > + * in a certain thermal zone
> > > > > + */
> > > > > +struct thermal_instance {
> > > > > +	int id;
> > > > > +	char name[THERMAL_NAME_LENGTH];
> > > > > +	struct thermal_zone_device *tz;
> > > > > +	struct thermal_cooling_device *cdev;
> > > > > +	int trip;
> > > > > +	unsigned long upper;	/* Highest cooling state for this trip point */
> > > > > +	unsigned long lower;	/* Lowest cooling state for this trip point */
> > > > > +	unsigned long target;	/* expected cooling state */
> > > > > +	char attr_name[THERMAL_NAME_LENGTH];
> > > > > +	struct device_attribute attr;
> > > > > +	struct list_head tz_node; /* node in tz->thermal_instances */
> > > > > +	struct list_head cdev_node; /* node in cdev->thermal_instances */
> > > > > +};
> > > > > +
> > > >
> > > > as this structure is used internally only, I'm thinking if we can rename
> > > > drivers/thermal/thermal_sys.c to drivers/thermal/thermal_core.c,
> > > > and introduce drivers/thermal/thermal_core.h for these internal stuff.
> > > > what do you think?
> > >
> > > Yes agree with you.
> > > Also, we can keep the sysfs things in thermal_sys.c
> > > and rest of the things in thermal_core.c, and have a thermal_core.h also.
> > > (This is how the power supply subsystem does it)
> > >
> > > I will include this clean up, as part of v2, if you are Ok with this.
> > >
> > yes, please go ahead.
> 
> Ok. I will include this change.
> 
> > 
> > > Other things;
> > >  I was thinking is 'removal of netlink things' from
> > > thermal_sys.c
> > 
> > and where to move it to?
> 
> I was thinking of completely removing this, as raw netlink
> usage is phasing out, and kobj_uevent () is being used
> increasingly.
> 
> But we will keep it as a separate change, and not club with this one.
> 
We need to hold this for a while as I'm not sure if someone is using
this or not.
IMO, you can introduce a Config option to enable/disable the netlink
events for now, and marking it as deprecated.

thanks,
rui
> > >
> > > Removing the hwmon related code (the thermal subsystem has grown
> > > quite a bit and provides more thermal functionalities than Hwmon)
> > > So, why do we need CONFIG_HWMON inside thermal subsystem ?
> > > If all of us agree, I am Ok to remove this also.
> > >
> > we need Jean's opinion on this.
> > But anyway, we can do this at anytime, if really needed.
> 
> Yes, will wait for Jean's thoughts..
> 
> Thanks,
> Durga
> 
> > 
> > thanks,
> > rui


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/13] Thermal: Move thermal_instance to thermal.h

[Eduardo Valentin]

Hello,

On Thu, Aug 16, 2012 at 03:12:32PM +0800, Zhang Rui wrote:
> On 四, 2012-08-16 at 00:31 -0600, R, Durgadoss wrote:
> > Hi Rui,
> > 
> > [cut.]
> > > > > > +/*
> > > > > > + * This structure is used to describe the behavior of
> > > > > > + * a certain cooling device on a certain trip point
> > > > > > + * in a certain thermal zone
> > > > > > + */
> > > > > > +struct thermal_instance {
> > > > > > +	int id;
> > > > > > +	char name[THERMAL_NAME_LENGTH];
> > > > > > +	struct thermal_zone_device *tz;
> > > > > > +	struct thermal_cooling_device *cdev;
> > > > > > +	int trip;
> > > > > > +	unsigned long upper;	/* Highest cooling state for this trip point */
> > > > > > +	unsigned long lower;	/* Lowest cooling state for this trip point */
> > > > > > +	unsigned long target;	/* expected cooling state */
> > > > > > +	char attr_name[THERMAL_NAME_LENGTH];
> > > > > > +	struct device_attribute attr;
> > > > > > +	struct list_head tz_node; /* node in tz->thermal_instances */
> > > > > > +	struct list_head cdev_node; /* node in cdev->thermal_instances */
> > > > > > +};
> > > > > > +
> > > > >
> > > > > as this structure is used internally only, I'm thinking if we can rename
> > > > > drivers/thermal/thermal_sys.c to drivers/thermal/thermal_core.c,
> > > > > and introduce drivers/thermal/thermal_core.h for these internal stuff.
> > > > > what do you think?
> > > >
> > > > Yes agree with you.
> > > > Also, we can keep the sysfs things in thermal_sys.c
> > > > and rest of the things in thermal_core.c, and have a thermal_core.h also.
> > > > (This is how the power supply subsystem does it)
> > > >
> > > > I will include this clean up, as part of v2, if you are Ok with this.
> > > >
> > > yes, please go ahead.

I also second this step. This split makes a lot of sense.

> > 
> > Ok. I will include this change.
> > 
> > > 
> > > > Other things;
> > > >  I was thinking is 'removal of netlink things' from
> > > > thermal_sys.c
> > > 
> > > and where to move it to?
> > 
> > I was thinking of completely removing this, as raw netlink
> > usage is phasing out, and kobj_uevent () is being used
> > increasingly.
> > 
> > But we will keep it as a separate change, and not club with this one.
> > 
> We need to hold this for a while as I'm not sure if someone is using
> this or not.
> IMO, you can introduce a Config option to enable/disable the netlink
> events for now, and marking it as deprecated.


At least from my side, I don't have any legacy application using the netlink :-)

How about you guys, what are the known applications using this link?

In any case, for me, at least we need to have a standard notification system,
for monitoring, debugging and also in case applications need to react on thermal
events.

> 
> thanks,
> rui
> > > >
> > > > Removing the hwmon related code (the thermal subsystem has grown
> > > > quite a bit and provides more thermal functionalities than Hwmon)
> > > > So, why do we need CONFIG_HWMON inside thermal subsystem ?
> > > > If all of us agree, I am Ok to remove this also.
> > > >
> > > we need Jean's opinion on this.
> > > But anyway, we can do this at anytime, if really needed.
> > 
> > Yes, will wait for Jean's thoughts..

What where the original design decisions to have these two linked together?

> > 
> > Thanks,
> > Durga
> > 
> > > 
> > > thanks,
> > > rui
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/13] Thermal: Add get trend, get instance API's to thermal_sys

[Durgadoss R]

This patch adds the following API's to thermal_sys.c, that
can be used by other Thermal drivers.
 * get_tz_trend: obtain the trend of the given thermal zone
 * get_cdev_by_name: obtain cdev pointer given its name
 * get_thermal_instance: obtain the instance corresponding
   to the given tz, cdev and the trip point.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |   59 +++++++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h       |    5 ++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index a0e20f9..998c16e 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -80,6 +80,65 @@ static void release_idr(struct idr *idr, struct mutex *lock, int id)
 		mutex_unlock(lock);
 }
 
+int get_tz_trend(struct thermal_zone_device *tz, int trip)
+{
+	enum thermal_trend trend;
+
+	if (!tz->ops->get_trend || tz->ops->get_trend(tz, trip, &trend)) {
+		if (tz->temperature > tz->last_temperature)
+			trend = THERMAL_TREND_RAISING;
+		else if (tz->temperature < tz->last_temperature)
+			trend = THERMAL_TREND_DROPPING;
+		else
+			trend = THERMAL_TREND_STABLE;
+	}
+
+	return trend;
+}
+EXPORT_SYMBOL(get_tz_trend);
+
+struct thermal_cooling_device *get_cdev_by_name(const char *name)
+{
+	struct thermal_cooling_device *pos;
+	struct thermal_cooling_device *target_cdev = NULL;
+
+	mutex_lock(&thermal_list_lock);
+
+	list_for_each_entry(pos, &thermal_cdev_list, node) {
+		if (!strcmp(pos->type, name)) {
+			target_cdev = pos;
+			break;
+		}
+	}
+	mutex_unlock(&thermal_list_lock);
+
+	return target_cdev;
+}
+EXPORT_SYMBOL(get_cdev_by_name);
+
+struct thermal_instance *get_thermal_instance(struct thermal_zone_device *tz,
+			struct thermal_cooling_device *cdev, int trip)
+{
+	struct thermal_instance *pos = NULL;
+	struct thermal_instance *target_instance = NULL;
+
+	mutex_lock(&tz->lock);
+	mutex_lock(&cdev->lock);
+
+	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
+		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
+			target_instance = pos;
+			break;
+		}
+	}
+
+	mutex_unlock(&cdev->lock);
+	mutex_unlock(&tz->lock);
+
+	return target_instance;
+}
+EXPORT_SYMBOL(get_thermal_instance);
+
 /* sys I/F for thermal zone */
 
 #define to_thermal_zone(_dev) \
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f25df23..757a007 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -208,6 +208,11 @@ struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
 		const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
 
+int get_tz_trend(struct thermal_zone_device *, int);
+struct thermal_cooling_device *get_cdev_by_name(const char *);
+struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
+		struct thermal_cooling_device *, int);
+
 #ifdef CONFIG_NET
 extern int thermal_generate_netlink_event(u32 orig, enum events event);
 #else
-- 
1.7.9.5

Re: [PATCH 03/13] Thermal: Add get trend, get instance API's to thermal_sys

[Eduardo Valentin]

Hello,

On Thu, Aug 09, 2012 at 06:15:55PM +0530, Durgadoss R wrote:
> This patch adds the following API's to thermal_sys.c, that
> can be used by other Thermal drivers.
>  * get_tz_trend: obtain the trend of the given thermal zone
>  * get_cdev_by_name: obtain cdev pointer given its name
>  * get_thermal_instance: obtain the instance corresponding
>    to the given tz, cdev and the trip point.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |   59 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h       |    5 ++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index a0e20f9..998c16e 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -80,6 +80,65 @@ static void release_idr(struct idr *idr, struct mutex *lock, int id)
>  		mutex_unlock(lock);
>  }
>  
> +int get_tz_trend(struct thermal_zone_device *tz, int trip)
> +{
> +	enum thermal_trend trend;
> +
> +	if (!tz->ops->get_trend || tz->ops->get_trend(tz, trip, &trend)) {
> +		if (tz->temperature > tz->last_temperature)
> +			trend = THERMAL_TREND_RAISING;
> +		else if (tz->temperature < tz->last_temperature)
> +			trend = THERMAL_TREND_DROPPING;
> +		else
> +			trend = THERMAL_TREND_STABLE;
> +	}
> +
> +	return trend;
> +}
> +EXPORT_SYMBOL(get_tz_trend);

You may want to reuse the above function inside thermal_zone_trip_update().

> +
> +struct thermal_cooling_device *get_cdev_by_name(const char *name)
> +{
> +	struct thermal_cooling_device *pos;
> +	struct thermal_cooling_device *target_cdev = NULL;
> +
> +	mutex_lock(&thermal_list_lock);
> +
> +	list_for_each_entry(pos, &thermal_cdev_list, node) {
> +		if (!strcmp(pos->type, name)) {

strncmp?

> +			target_cdev = pos;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&thermal_list_lock);
> +
> +	return target_cdev;
> +}
> +EXPORT_SYMBOL(get_cdev_by_name);
> +
> +struct thermal_instance *get_thermal_instance(struct thermal_zone_device *tz,
> +			struct thermal_cooling_device *cdev, int trip)
> +{
> +	struct thermal_instance *pos = NULL;
> +	struct thermal_instance *target_instance = NULL;
> +
> +	mutex_lock(&tz->lock);
> +	mutex_lock(&cdev->lock);
> +
> +	list_for_each_entry(pos, &tz->thermal_instances, tz_node) {
> +		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
> +			target_instance = pos;
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&cdev->lock);
> +	mutex_unlock(&tz->lock);
> +
> +	return target_instance;
> +}
> +EXPORT_SYMBOL(get_thermal_instance);
> +
>  /* sys I/F for thermal zone */
>  
>  #define to_thermal_zone(_dev) \
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f25df23..757a007 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -208,6 +208,11 @@ struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
>  		const struct thermal_cooling_device_ops *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>  
> +int get_tz_trend(struct thermal_zone_device *, int);
> +struct thermal_cooling_device *get_cdev_by_name(const char *);
> +struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
> +		struct thermal_cooling_device *, int);
> +
>  #ifdef CONFIG_NET
>  extern int thermal_generate_netlink_event(u32 orig, enum events event);
>  #else
> -- 
> 1.7.9.5
> 

[PATCH 04/13] Thermal: Add platform level information to thermal.h

[Durgadoss R]

This patch creates a structure to hold a thermal zone's
platform level info, and also defines an extern function to
retrieve zone parameters from thermal_sys.c.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |    3 +++
 include/linux/thermal.h       |   43 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 998c16e..f043cd6 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -49,6 +49,9 @@ static LIST_HEAD(thermal_tz_list);
 static LIST_HEAD(thermal_cdev_list);
 static DEFINE_MUTEX(thermal_list_lock);
 
+int (*get_platform_thermal_params)(struct thermal_zone_device *);
+EXPORT_SYMBOL(get_platform_thermal_params);
+
 static int get_idr(struct idr *idr, struct mutex *lock, int *id)
 {
 	int err;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 757a007..f9ce1e2 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -33,6 +33,8 @@
 #define THERMAL_MAX_TRIPS	12
 #define THERMAL_NAME_LENGTH	20
 
+#define MAX_COOLING_DEVS	THERMAL_MAX_TRIPS
+
 /* Initial state of a cooling device during binding */
 #define THERMAL_NO_TARGET	-1UL
 
@@ -49,6 +51,11 @@
 #define THERMAL_GENL_VERSION                    0x01
 #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
 
+/* Thermal policies */
+#define THERMAL_USER_SPACE	0
+#define THERMAL_FAIR_SHARE	1
+#define	THERMAL_STEP_WISE	2
+
 struct thermal_zone_device;
 struct thermal_cooling_device;
 
@@ -165,6 +172,7 @@ struct thermal_zone_device {
 	struct mutex lock; /* protect thermal_instances list */
 	struct list_head node;
 	struct delayed_work poll_queue;
+	struct thermal_zone_params *tzp;
 };
 
 /*
@@ -187,6 +195,31 @@ struct thermal_instance {
 	struct list_head cdev_node; /* node in cdev->thermal_instances */
 };
 
+/* Platform level parameters associated with a thermal zone */
+struct thermal_zone_params {
+	char *thermal_zone_name;
+	int throttle_policy;
+
+	/* Number of cooling devices associated with this thermal zone */
+	int num_cdevs;
+	char *cdevs_name[MAX_COOLING_DEVS];
+
+	/*
+	 * This is a measure of 'how effectively these devices can
+	 * cool 'this' thermal zone. The shall be determined by platform
+	 * characterization. This is on a 'percentage' scale.
+	 * See Documentation/thermal/sysfs-api.txt for more information.
+	 */
+	int weights[MAX_COOLING_DEVS];
+
+	/*
+	 * This is a bit mask that gives the binding relation between this
+	 * thermal zone and cdev, for a particular trip point.
+	 * See Documentation/thermal/sysfs-api.txt for more information.
+	 */
+	int trip_mask[MAX_COOLING_DEVS];
+};
+
 struct thermal_genl_event {
 	u32 orig;
 	enum events event;
@@ -213,6 +246,16 @@ struct thermal_cooling_device *get_cdev_by_name(const char *);
 struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
 		struct thermal_cooling_device *, int);
 
+/*
+ * The platform layer shall define a 'function' that provides the
+ * parameters for all thermal zones in the platform. This pointer
+ * should point to that 'function'.
+ *
+ * In thermal_zone_device_register() we update the parameters
+ * for the particular thermal zone.
+ */
+extern int (*get_platform_thermal_params)(struct thermal_zone_device *);
+
 #ifdef CONFIG_NET
 extern int thermal_generate_netlink_event(u32 orig, enum events event);
 #else
-- 
1.7.9.5

Re: [PATCH 04/13] Thermal: Add platform level information to thermal.h

[Zhang Rui]

On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> This patch creates a structure to hold a thermal zone's
> platform level info, and also defines an extern function to
> retrieve zone parameters from thermal_sys.c.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |    3 +++
>  include/linux/thermal.h       |   43 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 998c16e..f043cd6 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -49,6 +49,9 @@ static LIST_HEAD(thermal_tz_list);
>  static LIST_HEAD(thermal_cdev_list);
>  static DEFINE_MUTEX(thermal_list_lock);
>  
> +int (*get_platform_thermal_params)(struct thermal_zone_device *);
> +EXPORT_SYMBOL(get_platform_thermal_params);
> +
>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>  {
>  	int err;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 757a007..f9ce1e2 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -33,6 +33,8 @@
>  #define THERMAL_MAX_TRIPS	12
>  #define THERMAL_NAME_LENGTH	20
>  
> +#define MAX_COOLING_DEVS	THERMAL_MAX_TRIPS
> +
why MAX_COOLING_DEVS equals THERMAL_MAX_TRIPS?
if this is an arbitrary number, please use the number instead, or else
this would be confusing.

>  /* Initial state of a cooling device during binding */
>  #define THERMAL_NO_TARGET	-1UL
>  
> @@ -49,6 +51,11 @@
>  #define THERMAL_GENL_VERSION                    0x01
>  #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
>  
> +/* Thermal policies */
> +#define THERMAL_USER_SPACE	0
> +#define THERMAL_FAIR_SHARE	1
> +#define	THERMAL_STEP_WISE	2

coding style.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 04/13] Thermal: Add platform level information to thermal.h

[R, Durgadoss]

Hi Rui,

Thanks for the review:)

> > @@ -33,6 +33,8 @@
> >  #define THERMAL_MAX_TRIPS	12
> >  #define THERMAL_NAME_LENGTH	20
> >
> > +#define MAX_COOLING_DEVS	THERMAL_MAX_TRIPS
> > +
> why MAX_COOLING_DEVS equals THERMAL_MAX_TRIPS?
> if this is an arbitrary number, please use the number instead, or else
> this would be confusing.

I wanted to use an arbitrary number, but did not have any great reason.
So, thought use as many cdevs as that of the trip points.

Okay, will use a number.

> 
> >  /* Initial state of a cooling device during binding */
> >  #define THERMAL_NO_TARGET	-1UL
> >
> > @@ -49,6 +51,11 @@
> >  #define THERMAL_GENL_VERSION                    0x01
> >  #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
> >
> > +/* Thermal policies */
> > +#define THERMAL_USER_SPACE	0
> > +#define THERMAL_FAIR_SHARE	1
> > +#define	THERMAL_STEP_WISE	2
> 
> coding style.

Will fix..

> 
> thanks,
> rui

Re: [PATCH 04/13] Thermal: Add platform level information to thermal.h

[Zhang Rui]

On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> This patch creates a structure to hold a thermal zone's
> platform level info, and also defines an extern function to
> retrieve zone parameters from thermal_sys.c.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |    3 +++
>  include/linux/thermal.h       |   43 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 998c16e..f043cd6 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -49,6 +49,9 @@ static LIST_HEAD(thermal_tz_list);
>  static LIST_HEAD(thermal_cdev_list);
>  static DEFINE_MUTEX(thermal_list_lock);
>  
> +int (*get_platform_thermal_params)(struct thermal_zone_device *);
> +EXPORT_SYMBOL(get_platform_thermal_params);
> +
>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>  {
>  	int err;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 757a007..f9ce1e2 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -33,6 +33,8 @@
>  #define THERMAL_MAX_TRIPS	12
>  #define THERMAL_NAME_LENGTH	20
>  
> +#define MAX_COOLING_DEVS	THERMAL_MAX_TRIPS
> +
>  /* Initial state of a cooling device during binding */
>  #define THERMAL_NO_TARGET	-1UL
>  
> @@ -49,6 +51,11 @@
>  #define THERMAL_GENL_VERSION                    0x01
>  #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
>  
> +/* Thermal policies */
> +#define THERMAL_USER_SPACE	0
> +#define THERMAL_FAIR_SHARE	1
> +#define	THERMAL_STEP_WISE	2
> +
I'd prefer using enum here, for thermal policies.

>  struct thermal_zone_device;
>  struct thermal_cooling_device;
>  
> @@ -165,6 +172,7 @@ struct thermal_zone_device {
>  	struct mutex lock; /* protect thermal_instances list */
>  	struct list_head node;
>  	struct delayed_work poll_queue;
> +	struct thermal_zone_params *tzp;
>  };
>  
>  /*
> @@ -187,6 +195,31 @@ struct thermal_instance {
>  	struct list_head cdev_node; /* node in cdev->thermal_instances */
>  };
>  
> +/* Platform level parameters associated with a thermal zone */
> +struct thermal_zone_params {
> +	char *thermal_zone_name;
> +	int throttle_policy;
> +
> +	/* Number of cooling devices associated with this thermal zone */
> +	int num_cdevs;
> +	char *cdevs_name[MAX_COOLING_DEVS];
> +
> +	/*
> +	 * This is a measure of 'how effectively these devices can
> +	 * cool 'this' thermal zone. The shall be determined by platform
> +	 * characterization. This is on a 'percentage' scale.
> +	 * See Documentation/thermal/sysfs-api.txt for more information.
> +	 */
> +	int weights[MAX_COOLING_DEVS];
> +
> +	/*
> +	 * This is a bit mask that gives the binding relation between this
> +	 * thermal zone and cdev, for a particular trip point.
> +	 * See Documentation/thermal/sysfs-api.txt for more information.
> +	 */
> +	int trip_mask[MAX_COOLING_DEVS];
> +};
> +
>  struct thermal_genl_event {
>  	u32 orig;
>  	enum events event;
> @@ -213,6 +246,16 @@ struct thermal_cooling_device *get_cdev_by_name(const char *);
>  struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>  		struct thermal_cooling_device *, int);
>  
> +/*
> + * The platform layer shall define a 'function' that provides the
> + * parameters for all thermal zones in the platform. This pointer
> + * should point to that 'function'.
> + *
> + * In thermal_zone_device_register() we update the parameters
> + * for the particular thermal zone.
> + */
> +extern int (*get_platform_thermal_params)(struct thermal_zone_device *);
> +
>  #ifdef CONFIG_NET
>  extern int thermal_generate_netlink_event(u32 orig, enum events event);
>  #else


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/13] Thermal: Add platform level information to thermal.h

[Eduardo Valentin]

On Thu, Aug 09, 2012 at 06:15:56PM +0530, Durgadoss R wrote:
> This patch creates a structure to hold a thermal zone's
> platform level info, and also defines an extern function to
> retrieve zone parameters from thermal_sys.c.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |    3 +++
>  include/linux/thermal.h       |   43 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 998c16e..f043cd6 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -49,6 +49,9 @@ static LIST_HEAD(thermal_tz_list);
>  static LIST_HEAD(thermal_cdev_list);
>  static DEFINE_MUTEX(thermal_list_lock);
>  
> +int (*get_platform_thermal_params)(struct thermal_zone_device *);
> +EXPORT_SYMBOL(get_platform_thermal_params);
> +
>  static int get_idr(struct idr *idr, struct mutex *lock, int *id)
>  {
>  	int err;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 757a007..f9ce1e2 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -33,6 +33,8 @@
>  #define THERMAL_MAX_TRIPS	12
>  #define THERMAL_NAME_LENGTH	20
>  
> +#define MAX_COOLING_DEVS	THERMAL_MAX_TRIPS
> +
>  /* Initial state of a cooling device during binding */
>  #define THERMAL_NO_TARGET	-1UL
>  
> @@ -49,6 +51,11 @@
>  #define THERMAL_GENL_VERSION                    0x01
>  #define THERMAL_GENL_MCAST_GROUP_NAME           "thermal_mc_group"
>  
> +/* Thermal policies */
> +#define THERMAL_USER_SPACE	0
> +#define THERMAL_FAIR_SHARE	1
> +#define	THERMAL_STEP_WISE	2
> +
>  struct thermal_zone_device;
>  struct thermal_cooling_device;
>  
> @@ -165,6 +172,7 @@ struct thermal_zone_device {
>  	struct mutex lock; /* protect thermal_instances list */
>  	struct list_head node;
>  	struct delayed_work poll_queue;
> +	struct thermal_zone_params *tzp;
>  };
>  
>  /*
> @@ -187,6 +195,31 @@ struct thermal_instance {
>  	struct list_head cdev_node; /* node in cdev->thermal_instances */
>  };
>  
> +/* Platform level parameters associated with a thermal zone */
> +struct thermal_zone_params {
> +	char *thermal_zone_name;

I suppose this will be plat data definition right? does it make sense to make the above a const?

> +	int throttle_policy;
> +
> +	/* Number of cooling devices associated with this thermal zone */
> +	int num_cdevs;
> +	char *cdevs_name[MAX_COOLING_DEVS];

dito

> +
> +	/*
> +	 * This is a measure of 'how effectively these devices can
> +	 * cool 'this' thermal zone. The shall be determined by platform
> +	 * characterization. This is on a 'percentage' scale.
> +	 * See Documentation/thermal/sysfs-api.txt for more information.
> +	 */
> +	int weights[MAX_COOLING_DEVS];
> +
> +	/*
> +	 * This is a bit mask that gives the binding relation between this
> +	 * thermal zone and cdev, for a particular trip point.
> +	 * See Documentation/thermal/sysfs-api.txt for more information.
> +	 */
> +	int trip_mask[MAX_COOLING_DEVS];

just a matter of taste, but how about defining a struct with cdev_name, weight, trip_max, and then declaring an array in thermal_zone_params?

> +};
> +
>  struct thermal_genl_event {
>  	u32 orig;
>  	enum events event;
> @@ -213,6 +246,16 @@ struct thermal_cooling_device *get_cdev_by_name(const char *);
>  struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>  		struct thermal_cooling_device *, int);
>  
> +/*
> + * The platform layer shall define a 'function' that provides the
> + * parameters for all thermal zones in the platform. This pointer
> + * should point to that 'function'.
> + *
> + * In thermal_zone_device_register() we update the parameters
> + * for the particular thermal zone.
> + */
> +extern int (*get_platform_thermal_params)(struct thermal_zone_device *);
> +
>  #ifdef CONFIG_NET
>  extern int thermal_generate_netlink_event(u32 orig, enum events event);
>  #else
> -- 
> 1.7.9.5
> 

[PATCH 05/13] Thermal: Obtain platform data for thermal zone

[Durgadoss R]

This patch retrieves the platform level data for
a zone during its registration. It is not an error
to not have any platform data.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index f043cd6..243a3f0 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -142,6 +142,22 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *tz,
 }
 EXPORT_SYMBOL(get_thermal_instance);
 
+static void retrieve_zone_params(struct thermal_zone_device *tz)
+{
+	int ret;
+
+	/* Check whether the platform data pointer is defined */
+	if (!get_platform_thermal_params)
+		return;
+
+	ret = get_platform_thermal_params(tz);
+	if (ret) {
+		dev_err(&tz->device,
+		"parameters for zone %s not defined:%d\n", tz->type, ret);
+		tz->tzp = NULL;
+	}
+}
+
 /* sys I/F for thermal zone */
 
 #define to_thermal_zone(_dev) \
@@ -1460,6 +1476,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	if (result)
 		goto unregister;
 
+	/* Retrieve platform level parameters for this zone */
+	retrieve_zone_params(tz);
+
 	mutex_lock(&thermal_list_lock);
 	list_add_tail(&tz->node, &thermal_tz_list);
 	if (ops->bind)
-- 
1.7.9.5

Re: [PATCH 05/13] Thermal: Obtain platform data for thermal zone

[Eduardo Valentin]

Hello,

On Thu, Aug 09, 2012 at 06:15:57PM +0530, Durgadoss R wrote:
> This patch retrieves the platform level data for
> a zone during its registration. It is not an error
> to not have any platform data.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index f043cd6..243a3f0 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -142,6 +142,22 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *tz,
>  }
>  EXPORT_SYMBOL(get_thermal_instance);
>  
> +static void retrieve_zone_params(struct thermal_zone_device *tz)
> +{
> +	int ret;
> +
> +	/* Check whether the platform data pointer is defined */
> +	if (!get_platform_thermal_params)
> +		return;
> +
> +	ret = get_platform_thermal_params(tz);
> +	if (ret) {
> +		dev_err(&tz->device,
> +		"parameters for zone %s not defined:%d\n", tz->type, ret);

If it is not an error to be without pdata, we should not be farty at the console log.
This may generate several prints during boot, for instance.


> +		tz->tzp = NULL;
> +	}
> +}
> +
>  /* sys I/F for thermal zone */
>  
>  #define to_thermal_zone(_dev) \
> @@ -1460,6 +1476,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	if (result)
>  		goto unregister;
>  
> +	/* Retrieve platform level parameters for this zone */
> +	retrieve_zone_params(tz);
> +

Should it be any sanity check on the pdata passed?

>  	mutex_lock(&thermal_list_lock);
>  	list_add_tail(&tz->node, &thermal_tz_list);
>  	if (ops->bind)
> -- 
> 1.7.9.5
> 

[PATCH 06/13] Thermal: Add a policy sysfs attribute

[Durgadoss R]

This patch adds a policy sysfs attribute to a thermal zone.
This attribute will give us the throttling policy used
for the zone. This is a RO attribute.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |   40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h       |    1 +
 2 files changed, 41 insertions(+)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 243a3f0..195e529 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -411,6 +411,27 @@ passive_show(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", tz->forced_passive);
 }
 
+static ssize_t show_throttle_policy(struct device *dev,
+			struct device_attribute *devattr, char *buf)
+{
+	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	struct thermal_zone_params *tzp = tz->tzp;
+
+	if (!tzp)
+		return sprintf(buf, "step_wise(default)\n");
+
+	switch (tzp->throttle_policy) {
+	case THERMAL_FAIR_SHARE:
+		return sprintf(buf, "fair_share\n");
+	case THERMAL_STEP_WISE:
+		return sprintf(buf, "step_wise\n");
+	case THERMAL_USER_SPACE:
+		return sprintf(buf, "user_space\n");
+	default:
+		return sprintf(buf, "unknown\n");
+	}
+}
+
 static DEVICE_ATTR(type, 0444, type_show, NULL);
 static DEVICE_ATTR(temp, 0444, temp_show, NULL);
 static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
@@ -1269,6 +1290,15 @@ leave:
 }
 EXPORT_SYMBOL(thermal_zone_device_update);
 
+static int create_policy_attr(struct thermal_zone_device *tz)
+{
+	sysfs_attr_init(&tz->policy_attr.attr);
+	tz->policy_attr.attr.name = "throttle_policy";
+	tz->policy_attr.attr.mode = S_IRUGO | S_IWUSR;
+	tz->policy_attr.show = show_throttle_policy;
+	return device_create_file(&tz->device, &tz->policy_attr);
+}
+
 /**
  * create_trip_attrs - create attributes for trip points
  * @tz:		the thermal zone device
@@ -1479,6 +1509,14 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	/* Retrieve platform level parameters for this zone */
 	retrieve_zone_params(tz);
 
+	result = create_policy_attr(tz);
+	if (result) {
+		dev_err(&tz->device,
+			"create_policy_attr for zone %s failed:%d\n",
+			tz->type, result);
+		goto unregister;
+	}
+
 	mutex_lock(&thermal_list_lock);
 	list_add_tail(&tz->node, &thermal_tz_list);
 	if (ops->bind)
@@ -1539,6 +1577,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 		device_remove_file(&tz->device, &dev_attr_mode);
 	remove_trip_attrs(tz);
 
+	device_remove_file(&tz->device, &tz->policy_attr);
+
 	thermal_remove_hwmon_sysfs(tz);
 	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
 	idr_destroy(&tz->idr);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f9ce1e2..1d49f05 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -173,6 +173,7 @@ struct thermal_zone_device {
 	struct list_head node;
 	struct delayed_work poll_queue;
 	struct thermal_zone_params *tzp;
+	struct device_attribute policy_attr;
 };
 
 /*
-- 
1.7.9.5

Re: [PATCH 06/13] Thermal: Add a policy sysfs attribute

[Zhang Rui]

On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> This patch adds a policy sysfs attribute to a thermal zone.
> This attribute will give us the throttling policy used
> for the zone. This is a RO attribute.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h       |    1 +
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 243a3f0..195e529 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -411,6 +411,27 @@ passive_show(struct device *dev, struct device_attribute *attr,
>  	return sprintf(buf, "%d\n", tz->forced_passive);
>  }
>  
> +static ssize_t show_throttle_policy(struct device *dev,
> +			struct device_attribute *devattr, char *buf)
> +{
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +	struct thermal_zone_params *tzp = tz->tzp;
> +
> +	if (!tzp)
> +		return sprintf(buf, "step_wise(default)\n");
> +
> +	switch (tzp->throttle_policy) {
> +	case THERMAL_FAIR_SHARE:
> +		return sprintf(buf, "fair_share\n");
> +	case THERMAL_STEP_WISE:
> +		return sprintf(buf, "step_wise\n");
> +	case THERMAL_USER_SPACE:
> +		return sprintf(buf, "user_space\n");
> +	default:
> +		return sprintf(buf, "unknown\n");
> +	}
> +}
> +
>  static DEVICE_ATTR(type, 0444, type_show, NULL);
>  static DEVICE_ATTR(temp, 0444, temp_show, NULL);
>  static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
> @@ -1269,6 +1290,15 @@ leave:
>  }
>  EXPORT_SYMBOL(thermal_zone_device_update);
>  
> +static int create_policy_attr(struct thermal_zone_device *tz)
> +{
> +	sysfs_attr_init(&tz->policy_attr.attr);
> +	tz->policy_attr.attr.name = "throttle_policy";
> +	tz->policy_attr.attr.mode = S_IRUGO | S_IWUSR;
> +	tz->policy_attr.show = show_throttle_policy;
> +	return device_create_file(&tz->device, &tz->policy_attr);
> +}
> +
>  /**
>   * create_trip_attrs - create attributes for trip points
>   * @tz:		the thermal zone device
> @@ -1479,6 +1509,14 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	/* Retrieve platform level parameters for this zone */
>  	retrieve_zone_params(tz);
>  
> +	result = create_policy_attr(tz);
> +	if (result) {
> +		dev_err(&tz->device,
> +			"create_policy_attr for zone %s failed:%d\n",
> +			tz->type, result);
> +		goto unregister;
> +	}
> +
>  	mutex_lock(&thermal_list_lock);
>  	list_add_tail(&tz->node, &thermal_tz_list);
>  	if (ops->bind)
> @@ -1539,6 +1577,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  		device_remove_file(&tz->device, &dev_attr_mode);
>  	remove_trip_attrs(tz);
>  
> +	device_remove_file(&tz->device, &tz->policy_attr);
> +
>  	thermal_remove_hwmon_sysfs(tz);
>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>  	idr_destroy(&tz->idr);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f9ce1e2..1d49f05 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -173,6 +173,7 @@ struct thermal_zone_device {
>  	struct list_head node;
>  	struct delayed_work poll_queue;
>  	struct thermal_zone_params *tzp;
> +	struct device_attribute policy_attr;

do we need a device_attribute for each thermal zone?
I think we can share one static DEVICE_ATTR here.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 06/13] Thermal: Add a policy sysfs attribute

[R, Durgadoss]

Hi Rui,

> +
> >  	thermal_remove_hwmon_sysfs(tz);
> >  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> >  	idr_destroy(&tz->idr);
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index f9ce1e2..1d49f05 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -173,6 +173,7 @@ struct thermal_zone_device {
> >  	struct list_head node;
> >  	struct delayed_work poll_queue;
> >  	struct thermal_zone_params *tzp;
> > +	struct device_attribute policy_attr;
> 
> do we need a device_attribute for each thermal zone?
> I think we can share one static DEVICE_ATTR here.

We need a policy sysfs node per zone. That’s why I put it
this way, and thought this looks cleaner.

But, as per the implementation point, static DEVICE_ATTR 
would also work. Either ways is fine for me.

Thanks,
Durga

RE: [PATCH 06/13] Thermal: Add a policy sysfs attribute

[Zhang Rui]

On 一, 2012-08-13 at 00:34 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> > +
> > >  	thermal_remove_hwmon_sysfs(tz);
> > >  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> > >  	idr_destroy(&tz->idr);
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index f9ce1e2..1d49f05 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -173,6 +173,7 @@ struct thermal_zone_device {
> > >  	struct list_head node;
> > >  	struct delayed_work poll_queue;
> > >  	struct thermal_zone_params *tzp;
> > > +	struct device_attribute policy_attr;
> > 
> > do we need a device_attribute for each thermal zone?
> > I think we can share one static DEVICE_ATTR here.
> 
> We need a policy sysfs node per zone. That’s why I put it
> this way, and thought this looks cleaner.
> 
> But, as per the implementation point, static DEVICE_ATTR 
> would also work. Either ways is fine for me.
> 
I'd prefer the static DEVICE_ATTR, which is the way we do for other
attributes like type, mode, temp, etc.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] Thermal: Add a policy sysfs attribute

[Eduardo Valentin]

Hello,

On Thu, Aug 09, 2012 at 06:15:58PM +0530, Durgadoss R wrote:
> This patch adds a policy sysfs attribute to a thermal zone.
> This attribute will give us the throttling policy used
> for the zone. This is a RO attribute.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h       |    1 +
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 243a3f0..195e529 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -411,6 +411,27 @@ passive_show(struct device *dev, struct device_attribute *attr,
>  	return sprintf(buf, "%d\n", tz->forced_passive);
>  }
>  
> +static ssize_t show_throttle_policy(struct device *dev,
> +			struct device_attribute *devattr, char *buf)
> +{
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +	struct thermal_zone_params *tzp = tz->tzp;
> +
> +	if (!tzp)
> +		return sprintf(buf, "step_wise(default)\n");
> +
> +	switch (tzp->throttle_policy) {
> +	case THERMAL_FAIR_SHARE:
> +		return sprintf(buf, "fair_share\n");
> +	case THERMAL_STEP_WISE:
> +		return sprintf(buf, "step_wise\n");
> +	case THERMAL_USER_SPACE:
> +		return sprintf(buf, "user_space\n");
> +	default:
> +		return sprintf(buf, "unknown\n");
> +	}

Assume you store the policy type under platform data tzp, does it mean that
zones which does not have the tzp won't be capable to change the policy?

> +}
> +
>  static DEVICE_ATTR(type, 0444, type_show, NULL);
>  static DEVICE_ATTR(temp, 0444, temp_show, NULL);
>  static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
> @@ -1269,6 +1290,15 @@ leave:
>  }
>  EXPORT_SYMBOL(thermal_zone_device_update);
>  
> +static int create_policy_attr(struct thermal_zone_device *tz)
> +{
> +	sysfs_attr_init(&tz->policy_attr.attr);
> +	tz->policy_attr.attr.name = "throttle_policy";
> +	tz->policy_attr.attr.mode = S_IRUGO | S_IWUSR;
> +	tz->policy_attr.show = show_throttle_policy;
> +	return device_create_file(&tz->device, &tz->policy_attr);
> +}
> +
>  /**
>   * create_trip_attrs - create attributes for trip points
>   * @tz:		the thermal zone device
> @@ -1479,6 +1509,14 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	/* Retrieve platform level parameters for this zone */
>  	retrieve_zone_params(tz);
>  
> +	result = create_policy_attr(tz);
> +	if (result) {
> +		dev_err(&tz->device,
> +			"create_policy_attr for zone %s failed:%d\n",
> +			tz->type, result);
> +		goto unregister;
> +	}
> +
>  	mutex_lock(&thermal_list_lock);
>  	list_add_tail(&tz->node, &thermal_tz_list);
>  	if (ops->bind)
> @@ -1539,6 +1577,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  		device_remove_file(&tz->device, &dev_attr_mode);
>  	remove_trip_attrs(tz);
>  
> +	device_remove_file(&tz->device, &tz->policy_attr);
> +
>  	thermal_remove_hwmon_sysfs(tz);
>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>  	idr_destroy(&tz->idr);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index f9ce1e2..1d49f05 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -173,6 +173,7 @@ struct thermal_zone_device {
>  	struct list_head node;
>  	struct delayed_work poll_queue;
>  	struct thermal_zone_params *tzp;
> +	struct device_attribute policy_attr;
>  };
>  
>  /*
> -- 
> 1.7.9.5
> 

[PATCH 07/13] Thermal: Update binding logic based on platform data

[Durgadoss R]

This patch updates the binding logic in thermal_sys.c
It uses the platform layer data to bind a thermal zone
to a cdev for a particular trip point.

 * If we do not have platform data and do not have
   .bind defined, do not bind.
 * If we do not have platform data but .bind is
   defined, then use tz->ops->bind.
 * If we have platform data, use it to create binding.

The same logic sequence is followed for unbind also.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |  170 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 144 insertions(+), 26 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 195e529..748b12f 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -158,6 +158,107 @@ static void retrieve_zone_params(struct thermal_zone_device *tz)
 	}
 }
 
+static void print_bind_err_msg(struct thermal_zone_device *tz,
+			struct thermal_cooling_device *cdev, int ret)
+{
+	dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
+				tz->type, cdev->type, ret);
+}
+
+static void __bind(struct thermal_zone_device *tz, int mask,
+			struct thermal_cooling_device *cdev)
+{
+	int i, ret;
+
+	for (i = 0; i < tz->trips; i++) {
+		if (mask & (1 << i)) {
+			ret = thermal_zone_bind_cooling_device(tz, i, cdev,
+					THERMAL_NO_LIMIT, THERMAL_NO_LIMIT);
+			if (ret)
+				print_bind_err_msg(tz, cdev, ret);
+		}
+	}
+}
+
+static void __unbind(struct thermal_zone_device *tz, int mask,
+			struct thermal_cooling_device *cdev)
+{
+	int i;
+
+	for (i = 0; i < tz->trips; i++)
+		if (mask & (1 << i))
+			thermal_zone_unbind_cooling_device(tz, i, cdev);
+}
+
+static void update_bind_info(struct thermal_cooling_device *cdev)
+{
+	int i, ret;
+	struct thermal_zone_params *tzp;
+	struct thermal_zone_device *pos = NULL;
+
+	mutex_lock(&thermal_list_lock);
+
+	list_for_each_entry(pos, &thermal_tz_list, node) {
+		if (!pos->tzp && !pos->ops->bind)
+			continue;
+
+		if (!pos->tzp && pos->ops->bind) {
+			ret = pos->ops->bind(pos, cdev);
+			if (ret)
+				print_bind_err_msg(pos, cdev, ret);
+		}
+
+		tzp = pos->tzp;
+		for (i = 0; i < tzp->num_cdevs; i++) {
+			if (!strcmp(tzp->cdevs_name[i], cdev->type)) {
+				__bind(pos, tzp->trip_mask[i], cdev);
+				break;
+			}
+		}
+	}
+	mutex_unlock(&thermal_list_lock);
+}
+
+static void do_binding(struct thermal_zone_device *tz)
+{
+	int i, ret;
+	char *name;
+	struct thermal_cooling_device *pos = NULL;
+	struct thermal_zone_params *tzp = tz->tzp;
+
+	if (!tzp && !tz->ops->bind)
+		return;
+
+	/* If there is no platform data, try to use ops->bind */
+	if (!tzp && tz->ops->bind) {
+		mutex_lock(&thermal_list_lock);
+
+		list_for_each_entry(pos, &thermal_cdev_list, node) {
+			ret = tz->ops->bind(tz, pos);
+			if (ret)
+				print_bind_err_msg(tz, pos, ret);
+		}
+
+		mutex_unlock(&thermal_list_lock);
+		return;
+	}
+
+	/* If platform data is available, use it to create binding */
+	for (i = 0; i < tzp->num_cdevs; i++) {
+		name = tzp->cdevs_name[i];
+		pos = get_cdev_by_name(name);
+
+		if (!pos) {
+			dev_err(&tz->device, "cannot find cdev %s\n", name);
+			continue;
+		}
+
+		mutex_lock(&thermal_list_lock);
+		__bind(tz, tzp->trip_mask[i], pos);
+		mutex_unlock(&thermal_list_lock);
+	}
+}
+
 /* sys I/F for thermal zone */
 
 #define to_thermal_zone(_dev) \
@@ -975,7 +1076,6 @@ thermal_cooling_device_register(char *type, void *devdata,
 				const struct thermal_cooling_device_ops *ops)
 {
 	struct thermal_cooling_device *cdev;
-	struct thermal_zone_device *pos;
 	int result;
 
 	if (strlen(type) >= THERMAL_NAME_LENGTH)
@@ -1025,20 +1125,15 @@ thermal_cooling_device_register(char *type, void *devdata,
 	if (result)
 		goto unregister;
 
+	/* Add 'this' new cdev to the global cdev list */
 	mutex_lock(&thermal_list_lock);
 	list_add(&cdev->node, &thermal_cdev_list);
-	list_for_each_entry(pos, &thermal_tz_list, node) {
-		if (!pos->ops->bind)
-			continue;
-		result = pos->ops->bind(pos, cdev);
-		if (result)
-			break;
-
-	}
 	mutex_unlock(&thermal_list_lock);
 
-	if (!result)
-		return cdev;
+	/* Update binding information for 'this' new cdev */
+	update_bind_info(cdev);
+
+	return cdev;
 
 unregister:
 	release_idr(&thermal_cdev_idr, &thermal_idr_lock, cdev->id);
@@ -1054,10 +1149,10 @@ EXPORT_SYMBOL(thermal_cooling_device_register);
  * thermal_cooling_device_unregister() must be called when the device is no
  * longer needed.
  */
-void thermal_cooling_device_unregister(struct
-				       thermal_cooling_device
-				       *cdev)
+void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 {
+	int i;
+	struct thermal_zone_params *tzp;
 	struct thermal_zone_device *tz;
 	struct thermal_cooling_device *pos = NULL;
 
@@ -1074,12 +1169,23 @@ void thermal_cooling_device_unregister(struct
 		return;
 	}
 	list_del(&cdev->node);
+
+	/* Unbind all thermal zones associated with 'this' cdev */
 	list_for_each_entry(tz, &thermal_tz_list, node) {
-		if (!tz->ops->unbind)
+		tzp = tz->tzp;
+		if (!tzp && !tz->ops->unbind)
 			continue;
-		tz->ops->unbind(tz, cdev);
+
+		if (!tzp && tz->ops->unbind)
+			tz->ops->unbind(tz, cdev);
+
+		for (i = 0; i < tzp->num_cdevs; i++)
+			if (!strcmp(cdev->type, tzp->cdevs_name[i]))
+				__unbind(tz, tzp->trip_mask[i], cdev);
 	}
+
 	mutex_unlock(&thermal_list_lock);
+
 	if (cdev->type[0])
 		device_remove_file(&cdev->device, &dev_attr_cdev_type);
 	device_remove_file(&cdev->device, &dev_attr_max_state);
@@ -1424,7 +1530,6 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	int passive_delay, int polling_delay)
 {
 	struct thermal_zone_device *tz;
-	struct thermal_cooling_device *pos;
 	enum thermal_trip_type trip_type;
 	int result;
 	int count;
@@ -1519,14 +1624,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 
 	mutex_lock(&thermal_list_lock);
 	list_add_tail(&tz->node, &thermal_tz_list);
-	if (ops->bind)
-		list_for_each_entry(pos, &thermal_cdev_list, node) {
-		result = ops->bind(tz, pos);
-		if (result)
-			break;
-		}
 	mutex_unlock(&thermal_list_lock);
 
+	/* Bind cooling devices for this zone */
+	do_binding(tz);
+
+
 	INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
 
 	thermal_zone_device_update(tz);
@@ -1547,12 +1650,16 @@ EXPORT_SYMBOL(thermal_zone_device_register);
  */
 void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 {
+	int i;
+	struct thermal_zone_params *tzp;
 	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *pos = NULL;
 
 	if (!tz)
 		return;
 
+	tzp = tz->tzp;
+
 	mutex_lock(&thermal_list_lock);
 	list_for_each_entry(pos, &thermal_tz_list, node)
 	    if (pos == tz)
@@ -1563,9 +1670,20 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 		return;
 	}
 	list_del(&tz->node);
-	if (tz->ops->unbind)
-		list_for_each_entry(cdev, &thermal_cdev_list, node)
-		    tz->ops->unbind(tz, cdev);
+
+	/* Unbind all cdevs associated with 'this' thermal zone */
+	list_for_each_entry(cdev, &thermal_cdev_list, node) {
+		if (!tzp && !tz->ops->unbind)
+			break;
+
+		if (!tzp && tz->ops->unbind)
+			tz->ops->unbind(tz, cdev);
+
+		for (i = 0; i < tzp->num_cdevs; i++)
+			if (!strcmp(cdev->type, tzp->cdevs_name[i]))
+				__unbind(tz, tzp->trip_mask[i], cdev);
+	}
+
 	mutex_unlock(&thermal_list_lock);
 
 	thermal_zone_device_set_polling(tz, 0);
-- 
1.7.9.5

Re: [PATCH 07/13] Thermal: Update binding logic based on platform data

[Zhang Rui]

On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> This patch updates the binding logic in thermal_sys.c
> It uses the platform layer data to bind a thermal zone
> to a cdev for a particular trip point.
> 
>  * If we do not have platform data and do not have
>    .bind defined, do not bind.
>  * If we do not have platform data but .bind is
>    defined, then use tz->ops->bind.
>  * If we have platform data, use it to create binding.
> 
> The same logic sequence is followed for unbind also.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |  170 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 144 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 195e529..748b12f 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -158,6 +158,107 @@ static void retrieve_zone_params(struct thermal_zone_device *tz)
>  	}
>  }
>  
> +static void print_bind_err_msg(struct thermal_zone_device *tz,
> +			struct thermal_cooling_device *cdev, int ret)
> +{
> +	dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
> +				tz->type, cdev->type, ret);
> +}
> +
> +static void __bind(struct thermal_zone_device *tz, int mask,
> +			struct thermal_cooling_device *cdev)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < tz->trips; i++) {
> +		if (mask & (1 << i)) {
> +			ret = thermal_zone_bind_cooling_device(tz, i, cdev,
> +					THERMAL_NO_LIMIT, THERMAL_NO_LIMIT);
> +			if (ret)
> +				print_bind_err_msg(tz, cdev, ret);
> +		}
> +	}
> +}
> +
> +static void __unbind(struct thermal_zone_device *tz, int mask,
> +			struct thermal_cooling_device *cdev)
> +{
> +	int i;
> +
> +	for (i = 0; i < tz->trips; i++)
> +		if (mask & (1 << i))
> +			thermal_zone_unbind_cooling_device(tz, i, cdev);
> +}
> +
> +static void update_bind_info(struct thermal_cooling_device *cdev)
> +{
> +	int i, ret;
> +	struct thermal_zone_params *tzp;
> +	struct thermal_zone_device *pos = NULL;
> +
> +	mutex_lock(&thermal_list_lock);
> +
> +	list_for_each_entry(pos, &thermal_tz_list, node) {
> +		if (!pos->tzp && !pos->ops->bind)
> +			continue;
> +
> +		if (!pos->tzp && pos->ops->bind) {
> +			ret = pos->ops->bind(pos, cdev);
> +			if (ret)
> +				print_bind_err_msg(pos, cdev, ret);
> +		}
> +
> +		tzp = pos->tzp;
> +		for (i = 0; i < tzp->num_cdevs; i++) {
> +			if (!strcmp(tzp->cdevs_name[i], cdev->type)) {
> +				__bind(pos, tzp->trip_mask[i], cdev);
> +				break;
> +			}
> +		}
> +	}
> +	mutex_unlock(&thermal_list_lock);
> +}

I still do not understand why we need this kind of bind.
Say, the platform thermal driver knows the platform data, i.e. it knows
which cooling devices should be bound to which trip points.
why we can not move this kind of logic to the .bind() callback, offered
by the platform thermal driver?
say, in .bind() callback,
the platform thermal driver has the pointer of the platform data, right?
the .cdev parameter can be used to find the cooling device name,
and we can make the comparison there. instead of introducing new binding
functions in the generic thermal layer.

> +
> +static void do_binding(struct thermal_zone_device *tz)
> +{
> +	int i, ret;
> +	char *name;
> +	struct thermal_cooling_device *pos = NULL;
> +	struct thermal_zone_params *tzp = tz->tzp;
> +
> +	if (!tzp && !tz->ops->bind)
> +		return;
> +
> +	/* If there is no platform data, try to use ops->bind */
> +	if (!tzp && tz->ops->bind) {
> +		mutex_lock(&thermal_list_lock);
> +
> +		list_for_each_entry(pos, &thermal_cdev_list, node) {
> +			ret = tz->ops->bind(tz, pos);
> +			if (ret)
> +				print_bind_err_msg(tz, pos, ret);
> +		}
> +
> +		mutex_unlock(&thermal_list_lock);
> +		return;
> +	}
> +
> +	/* If platform data is available, use it to create binding */
> +	for (i = 0; i < tzp->num_cdevs; i++) {
> +		name = tzp->cdevs_name[i];
> +		pos = get_cdev_by_name(name);
> +
> +		if (!pos) {
> +			dev_err(&tz->device, "cannot find cdev %s\n", name);
> +			continue;
> +		}
> +
> +		mutex_lock(&thermal_list_lock);
> +		__bind(tz, tzp->trip_mask[i], pos);
> +		mutex_unlock(&thermal_list_lock);
> +	}
> +}
> +
>  /* sys I/F for thermal zone */
>  
>  #define to_thermal_zone(_dev) \
> @@ -975,7 +1076,6 @@ thermal_cooling_device_register(char *type, void *devdata,
>  				const struct thermal_cooling_device_ops *ops)
>  {
>  	struct thermal_cooling_device *cdev;
> -	struct thermal_zone_device *pos;
>  	int result;
>  
>  	if (strlen(type) >= THERMAL_NAME_LENGTH)
> @@ -1025,20 +1125,15 @@ thermal_cooling_device_register(char *type, void *devdata,
>  	if (result)
>  		goto unregister;
>  
> +	/* Add 'this' new cdev to the global cdev list */
>  	mutex_lock(&thermal_list_lock);
>  	list_add(&cdev->node, &thermal_cdev_list);
> -	list_for_each_entry(pos, &thermal_tz_list, node) {
> -		if (!pos->ops->bind)
> -			continue;
> -		result = pos->ops->bind(pos, cdev);
> -		if (result)
> -			break;
> -
> -	}
>  	mutex_unlock(&thermal_list_lock);
>  
> -	if (!result)
> -		return cdev;
> +	/* Update binding information for 'this' new cdev */
> +	update_bind_info(cdev);
> +
> +	return cdev;
>  
>  unregister:
>  	release_idr(&thermal_cdev_idr, &thermal_idr_lock, cdev->id);
> @@ -1054,10 +1149,10 @@ EXPORT_SYMBOL(thermal_cooling_device_register);
>   * thermal_cooling_device_unregister() must be called when the device is no
>   * longer needed.
>   */
> -void thermal_cooling_device_unregister(struct
> -				       thermal_cooling_device
> -				       *cdev)
> +void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>  {
> +	int i;
> +	struct thermal_zone_params *tzp;
>  	struct thermal_zone_device *tz;
>  	struct thermal_cooling_device *pos = NULL;
>  
> @@ -1074,12 +1169,23 @@ void thermal_cooling_device_unregister(struct
>  		return;
>  	}
>  	list_del(&cdev->node);
> +
> +	/* Unbind all thermal zones associated with 'this' cdev */
>  	list_for_each_entry(tz, &thermal_tz_list, node) {
> -		if (!tz->ops->unbind)
> +		tzp = tz->tzp;
> +		if (!tzp && !tz->ops->unbind)
>  			continue;
> -		tz->ops->unbind(tz, cdev);
> +
> +		if (!tzp && tz->ops->unbind)
> +			tz->ops->unbind(tz, cdev);
> +
> +		for (i = 0; i < tzp->num_cdevs; i++)
> +			if (!strcmp(cdev->type, tzp->cdevs_name[i]))
> +				__unbind(tz, tzp->trip_mask[i], cdev);
>  	}
> +
>  	mutex_unlock(&thermal_list_lock);
> +
>  	if (cdev->type[0])
>  		device_remove_file(&cdev->device, &dev_attr_cdev_type);
>  	device_remove_file(&cdev->device, &dev_attr_max_state);
> @@ -1424,7 +1530,6 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	int passive_delay, int polling_delay)
>  {
>  	struct thermal_zone_device *tz;
> -	struct thermal_cooling_device *pos;
>  	enum thermal_trip_type trip_type;
>  	int result;
>  	int count;
> @@ -1519,14 +1624,12 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  
>  	mutex_lock(&thermal_list_lock);
>  	list_add_tail(&tz->node, &thermal_tz_list);
> -	if (ops->bind)
> -		list_for_each_entry(pos, &thermal_cdev_list, node) {
> -		result = ops->bind(tz, pos);
> -		if (result)
> -			break;
> -		}
>  	mutex_unlock(&thermal_list_lock);
>  
> +	/* Bind cooling devices for this zone */
> +	do_binding(tz);
> +
> +
>  	INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
>  
>  	thermal_zone_device_update(tz);
> @@ -1547,12 +1650,16 @@ EXPORT_SYMBOL(thermal_zone_device_register);
>   */
>  void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  {
> +	int i;
> +	struct thermal_zone_params *tzp;
>  	struct thermal_cooling_device *cdev;
>  	struct thermal_zone_device *pos = NULL;
>  
>  	if (!tz)
>  		return;
>  
> +	tzp = tz->tzp;
> +
>  	mutex_lock(&thermal_list_lock);
>  	list_for_each_entry(pos, &thermal_tz_list, node)
>  	    if (pos == tz)
> @@ -1563,9 +1670,20 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>  		return;
>  	}
>  	list_del(&tz->node);
> -	if (tz->ops->unbind)
> -		list_for_each_entry(cdev, &thermal_cdev_list, node)
> -		    tz->ops->unbind(tz, cdev);
> +
> +	/* Unbind all cdevs associated with 'this' thermal zone */
> +	list_for_each_entry(cdev, &thermal_cdev_list, node) {
> +		if (!tzp && !tz->ops->unbind)
> +			break;
> +
> +		if (!tzp && tz->ops->unbind)
> +			tz->ops->unbind(tz, cdev);
> +
> +		for (i = 0; i < tzp->num_cdevs; i++)
> +			if (!strcmp(cdev->type, tzp->cdevs_name[i]))
> +				__unbind(tz, tzp->trip_mask[i], cdev);
> +	}
> +
>  	mutex_unlock(&thermal_list_lock);
>  
>  	thermal_zone_device_set_polling(tz, 0);


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 07/13] Thermal: Update binding logic based on platform data

[R, Durgadoss]

Hi Rui,

> -----Original Message-----
> From: Zhang, Rui
> Sent: Monday, August 13, 2012 12:11 PM
> To: R, Durgadoss
> Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> wni@nvidia.com
> Subject: Re: [PATCH 07/13] Thermal: Update binding logic based on platform data
> 
> On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> > This patch updates the binding logic in thermal_sys.c
> > It uses the platform layer data to bind a thermal zone
> > to a cdev for a particular trip point.
> >
> >  * If we do not have platform data and do not have
> >    .bind defined, do not bind.
> >  * If we do not have platform data but .bind is
> >    defined, then use tz->ops->bind.
> >  * If we have platform data, use it to create binding.
> >
> > The same logic sequence is followed for unbind also.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  drivers/thermal/thermal_sys.c |  170
> ++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 144 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 195e529..748b12f 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -158,6 +158,107 @@ static void retrieve_zone_params(struct
> thermal_zone_device *tz)
> >  	}
> >  }
> >
> > +static void print_bind_err_msg(struct thermal_zone_device *tz,
> > +			struct thermal_cooling_device *cdev, int ret)
> > +{
> > +	dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
> > +				tz->type, cdev->type, ret);
> > +}
> > +
> > +static void __bind(struct thermal_zone_device *tz, int mask,
> > +			struct thermal_cooling_device *cdev)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < tz->trips; i++) {
> > +		if (mask & (1 << i)) {
> > +			ret = thermal_zone_bind_cooling_device(tz, i, cdev,
> > +					THERMAL_NO_LIMIT,
> THERMAL_NO_LIMIT);
> > +			if (ret)
> > +				print_bind_err_msg(tz, cdev, ret);
> > +		}
> > +	}
> > +}
> > +
> > +static void __unbind(struct thermal_zone_device *tz, int mask,
> > +			struct thermal_cooling_device *cdev)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < tz->trips; i++)
> > +		if (mask & (1 << i))
> > +			thermal_zone_unbind_cooling_device(tz, i, cdev);
> > +}
> > +
> > +static void update_bind_info(struct thermal_cooling_device *cdev)
> > +{
> > +	int i, ret;
> > +	struct thermal_zone_params *tzp;
> > +	struct thermal_zone_device *pos = NULL;
> > +
> > +	mutex_lock(&thermal_list_lock);
> > +
> > +	list_for_each_entry(pos, &thermal_tz_list, node) {
> > +		if (!pos->tzp && !pos->ops->bind)
> > +			continue;
> > +
> > +		if (!pos->tzp && pos->ops->bind) {
> > +			ret = pos->ops->bind(pos, cdev);
> > +			if (ret)
> > +				print_bind_err_msg(pos, cdev, ret);
> > +		}
> > +
> > +		tzp = pos->tzp;
> > +		for (i = 0; i < tzp->num_cdevs; i++) {
> > +			if (!strcmp(tzp->cdevs_name[i], cdev->type)) {
> > +				__bind(pos, tzp->trip_mask[i], cdev);
> > +				break;
> > +			}
> > +		}
> > +	}
> > +	mutex_unlock(&thermal_list_lock);
> > +}
> 
> I still do not understand why we need this kind of bind.
> Say, the platform thermal driver knows the platform data, i.e. it knows
> which cooling devices should be bound to which trip points.
> why we can not move this kind of logic to the .bind() callback, offered
> by the platform thermal driver?
> say, in .bind() callback,
> the platform thermal driver has the pointer of the platform data, right?
> the .cdev parameter can be used to find the cooling device name,
> and we can make the comparison there. instead of introducing new binding
> functions in the generic thermal layer.

For once, I got little confused between the generic platform thermal sensor
drivers (the chip drivers) and the platform level driver (not specific for chip,
but for a platform). So, yes we can put this in the platform level driver.

On the other hand, I believe we will have more and more platform thermal
drivers, as new devices arrive. And in each of the drivers, we are going to
do the 'looping, finding cdev and then binding' part. I was wondering wouldn't
it be better to move the common code to the framework, so that the same
code does not get duplicated, over several places.

So, please give a second thought on this and let me know your opinion.

Thanks,
Durga

RE: [PATCH 07/13] Thermal: Update binding logic based on platform data

[Zhang Rui]

On 一, 2012-08-13 at 09:41 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> > -----Original Message-----
> > From: Zhang, Rui
> > Sent: Monday, August 13, 2012 12:11 PM
> > To: R, Durgadoss
> > Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> > pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> > wni@nvidia.com
> > Subject: Re: [PATCH 07/13] Thermal: Update binding logic based on platform data
> > 
> > On 四, 2012-08-09 at 18:15 +0530, Durgadoss R wrote:
> > > This patch updates the binding logic in thermal_sys.c
> > > It uses the platform layer data to bind a thermal zone
> > > to a cdev for a particular trip point.
> > >
> > >  * If we do not have platform data and do not have
> > >    .bind defined, do not bind.
> > >  * If we do not have platform data but .bind is
> > >    defined, then use tz->ops->bind.
> > >  * If we have platform data, use it to create binding.
> > >
> > > The same logic sequence is followed for unbind also.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > ---
> > >  drivers/thermal/thermal_sys.c |  170
> > ++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 144 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > > index 195e529..748b12f 100644
> > > --- a/drivers/thermal/thermal_sys.c
> > > +++ b/drivers/thermal/thermal_sys.c
> > > @@ -158,6 +158,107 @@ static void retrieve_zone_params(struct
> > thermal_zone_device *tz)
> > >  	}
> > >  }
> > >
> > > +static void print_bind_err_msg(struct thermal_zone_device *tz,
> > > +			struct thermal_cooling_device *cdev, int ret)
> > > +{
> > > +	dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
> > > +				tz->type, cdev->type, ret);
> > > +}
> > > +
> > > +static void __bind(struct thermal_zone_device *tz, int mask,
> > > +			struct thermal_cooling_device *cdev)
> > > +{
> > > +	int i, ret;
> > > +
> > > +	for (i = 0; i < tz->trips; i++) {
> > > +		if (mask & (1 << i)) {
> > > +			ret = thermal_zone_bind_cooling_device(tz, i, cdev,
> > > +					THERMAL_NO_LIMIT,
> > THERMAL_NO_LIMIT);
> > > +			if (ret)
> > > +				print_bind_err_msg(tz, cdev, ret);
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static void __unbind(struct thermal_zone_device *tz, int mask,
> > > +			struct thermal_cooling_device *cdev)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < tz->trips; i++)
> > > +		if (mask & (1 << i))
> > > +			thermal_zone_unbind_cooling_device(tz, i, cdev);
> > > +}
> > > +
> > > +static void update_bind_info(struct thermal_cooling_device *cdev)
> > > +{
> > > +	int i, ret;
> > > +	struct thermal_zone_params *tzp;
> > > +	struct thermal_zone_device *pos = NULL;
> > > +
> > > +	mutex_lock(&thermal_list_lock);
> > > +
> > > +	list_for_each_entry(pos, &thermal_tz_list, node) {
> > > +		if (!pos->tzp && !pos->ops->bind)
> > > +			continue;
> > > +
> > > +		if (!pos->tzp && pos->ops->bind) {
> > > +			ret = pos->ops->bind(pos, cdev);
> > > +			if (ret)
> > > +				print_bind_err_msg(pos, cdev, ret);
> > > +		}
> > > +
> > > +		tzp = pos->tzp;
> > > +		for (i = 0; i < tzp->num_cdevs; i++) {
> > > +			if (!strcmp(tzp->cdevs_name[i], cdev->type)) {
> > > +				__bind(pos, tzp->trip_mask[i], cdev);
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > > +	mutex_unlock(&thermal_list_lock);
> > > +}
> > 
> > I still do not understand why we need this kind of bind.
> > Say, the platform thermal driver knows the platform data, i.e. it knows
> > which cooling devices should be bound to which trip points.
> > why we can not move this kind of logic to the .bind() callback, offered
> > by the platform thermal driver?
> > say, in .bind() callback,
> > the platform thermal driver has the pointer of the platform data, right?
> > the .cdev parameter can be used to find the cooling device name,
> > and we can make the comparison there. instead of introducing new binding
> > functions in the generic thermal layer.
> 
> For once, I got little confused between the generic platform thermal sensor
> drivers (the chip drivers) and the platform level driver (not specific for chip,
> but for a platform). So, yes we can put this in the platform level driver.
> 
Hmm,
I'm not clear about the difference between these two drivers.
what is supposed to be done in the platform thermal sensor drivers and
what is supposed to be done in the platform level driver?

At least for now, all the thermal drivers are both thermal sensor driver
and platform level driver, right?

thanks,
rui
> On the other hand, I believe we will have more and more platform thermal
> drivers, as new devices arrive. And in each of the drivers, we are going to
> do the 'looping, finding cdev and then binding' part. I was wondering wouldn't
> it be better to move the common code to the framework, so that the same
> code does not get duplicated, over several places.
> 
> 
> 
> So, please give a second thought on this and let me know your opinion.

> Thanks,
> Durga


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 07/13] Thermal: Update binding logic based on platform data

[R, Durgadoss]

Hi Rui,

> > > > +static void update_bind_info(struct thermal_cooling_device *cdev)
> > > > +{
> > > > +	int i, ret;
> > > > +	struct thermal_zone_params *tzp;
> > > > +	struct thermal_zone_device *pos = NULL;
> > > > +
> > > > +	mutex_lock(&thermal_list_lock);
> > > > +
> > > > +	list_for_each_entry(pos, &thermal_tz_list, node) {
> > > > +		if (!pos->tzp && !pos->ops->bind)
> > > > +			continue;
> > > > +
> > > > +		if (!pos->tzp && pos->ops->bind) {
> > > > +			ret = pos->ops->bind(pos, cdev);
> > > > +			if (ret)
> > > > +				print_bind_err_msg(pos, cdev, ret);
> > > > +		}
> > > > +
> > > > +		tzp = pos->tzp;
> > > > +		for (i = 0; i < tzp->num_cdevs; i++) {
> > > > +			if (!strcmp(tzp->cdevs_name[i], cdev->type)) {
> > > > +				__bind(pos, tzp->trip_mask[i], cdev);
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +	mutex_unlock(&thermal_list_lock);
> > > > +}
> > >
> > > I still do not understand why we need this kind of bind.
> > > Say, the platform thermal driver knows the platform data, i.e. it knows
> > > which cooling devices should be bound to which trip points.
> > > why we can not move this kind of logic to the .bind() callback, offered
> > > by the platform thermal driver?
> > > say, in .bind() callback,
> > > the platform thermal driver has the pointer of the platform data, right?
> > > the .cdev parameter can be used to find the cooling device name,
> > > and we can make the comparison there. instead of introducing new binding
> > > functions in the generic thermal layer.
> >
> > For once, I got little confused between the generic platform thermal sensor
> > drivers (the chip drivers) and the platform level driver (not specific for chip,
> > but for a platform). So, yes we can put this in the platform level driver.
> >
> Hmm,
> I'm not clear about the difference between these two drivers.
> what is supposed to be done in the platform thermal sensor drivers and
> what is supposed to be done in the platform level driver?

A sensor driver can be a generic chip driver like emc1403 (this is the one
that I have worked on..) or coretemp (the CPU DTS driver for x86). They sit
in different sub systems (these two in hwmon). We might not be allowed to
add any thermal framework specific code in these drivers. The same driver
works on all platforms.

A platform level thermal driver knows information about the thermal sensors,
and their zones on the platform; and is specific to the platform.
For x86, this will be in drivers/x86/platform/ whereas might be in some other
place for other architectures. An example is intel_mid_thermal.c which sits
in drivers/x86/platform. We can add our thermal framework specific code
to this driver.

> 
> At least for now, all the thermal drivers are both thermal sensor driver
> and platform level driver, right?
 
Not all the times, although there are some instances where both are same.
We use coretemp.c and intel_mid_thermal.c (which are different), for the
x86 mid platforms.

I hope this explanation helps you..

Thanks,
Durga

RE: [PATCH 07/13] Thermal: Update binding logic based on platform data

[Zhang Rui]

On 三, 2012-08-15 at 03:17 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> > > > > +static void update_bind_info(struct thermal_cooling_device *cdev)
> > > > > +{
> > > > > +	int i, ret;
> > > > > +	struct thermal_zone_params *tzp;
> > > > > +	struct thermal_zone_device *pos = NULL;
> > > > > +
> > > > > +	mutex_lock(&thermal_list_lock);
> > > > > +
> > > > > +	list_for_each_entry(pos, &thermal_tz_list, node) {
> > > > > +		if (!pos->tzp && !pos->ops->bind)
> > > > > +			continue;
> > > > > +
> > > > > +		if (!pos->tzp && pos->ops->bind) {
> > > > > +			ret = pos->ops->bind(pos, cdev);
> > > > > +			if (ret)
> > > > > +				print_bind_err_msg(pos, cdev, ret);
> > > > > +		}
> > > > > +
> > > > > +		tzp = pos->tzp;
> > > > > +		for (i = 0; i < tzp->num_cdevs; i++) {
> > > > > +			if (!strcmp(tzp->cdevs_name[i], cdev->type)) {
> > > > > +				__bind(pos, tzp->trip_mask[i], cdev);
> > > > > +				break;
> > > > > +			}
> > > > > +		}
> > > > > +	}
> > > > > +	mutex_unlock(&thermal_list_lock);
> > > > > +}
> > > >
> > > > I still do not understand why we need this kind of bind.
> > > > Say, the platform thermal driver knows the platform data, i.e. it knows
> > > > which cooling devices should be bound to which trip points.
> > > > why we can not move this kind of logic to the .bind() callback, offered
> > > > by the platform thermal driver?
> > > > say, in .bind() callback,
> > > > the platform thermal driver has the pointer of the platform data, right?
> > > > the .cdev parameter can be used to find the cooling device name,
> > > > and we can make the comparison there. instead of introducing new binding
> > > > functions in the generic thermal layer.
> > >
> > > For once, I got little confused between the generic platform thermal sensor
> > > drivers (the chip drivers) and the platform level driver (not specific for chip,
> > > but for a platform). So, yes we can put this in the platform level driver.
> > >
> > Hmm,
> > I'm not clear about the difference between these two drivers.
> > what is supposed to be done in the platform thermal sensor drivers and
> > what is supposed to be done in the platform level driver?
> 
> A sensor driver can be a generic chip driver like emc1403 (this is the one
> that I have worked on..) or coretemp (the CPU DTS driver for x86). They sit
> in different sub systems (these two in hwmon). We might not be allowed to
> add any thermal framework specific code in these drivers. The same driver
> works on all platforms.

does the sensor know anything about the "policy"?
Say, does it have any trip points? does it know which device can be
throttled to cool itself?
I think the answer is "no", right?

> 
> A platform level thermal driver knows information about the thermal sensors,
> and their zones on the platform; and is specific to the platform.
> For x86, this will be in drivers/x86/platform/ whereas might be in some other
> place for other architectures. An example is intel_mid_thermal.c which sits
> in drivers/x86/platform. We can add our thermal framework specific code
> to this driver.
> 
but I think intel_mide_thermal driver is also a platform thermal sensor
driver at the same time.

> > 
> > At least for now, all the thermal drivers are both thermal sensor driver
> > and platform level driver, right?
>  
> Not all the times, although there are some instances where both are same.
> We use coretemp.c and intel_mid_thermal.c (which are different), for the
> x86 mid platforms.
> 
so you want to use coretemp.c as a temperature sensor, and then bind
your own cooling devices to it in your platform level thermal driver?

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 07/13] Thermal: Update binding logic based on platform data

[R, Durgadoss]

Hi Rui,


> -----Original Message-----
> From: Zhang, Rui
> Sent: Thursday, August 16, 2012 9:00 AM
> To: R, Durgadoss
> Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> wni@nvidia.com
> Subject: RE: [PATCH 07/13] Thermal: Update binding logic based on platform
> data
> 
> On 三, 2012-08-15 at 03:17 -0600, R, Durgadoss wrote:
> > Hi Rui,
> >
> > > > > > +static void update_bind_info(struct thermal_cooling_device
> *cdev)
> > > > > > +{
> > > > > > +	int i, ret;
> > > > > > +	struct thermal_zone_params *tzp;
> > > > > > +	struct thermal_zone_device *pos = NULL;
> > > > > > +
> > > > > > +	mutex_lock(&thermal_list_lock);
> > > > > > +
> > > > > > +	list_for_each_entry(pos, &thermal_tz_list, node) {
> > > > > > +		if (!pos->tzp && !pos->ops->bind)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		if (!pos->tzp && pos->ops->bind) {
> > > > > > +			ret = pos->ops->bind(pos, cdev);
> > > > > > +			if (ret)
> > > > > > +				print_bind_err_msg(pos, cdev, ret);
> > > > > > +		}
> > > > > > +
> > > > > > +		tzp = pos->tzp;
> > > > > > +		for (i = 0; i < tzp->num_cdevs; i++) {
> > > > > > +			if (!strcmp(tzp->cdevs_name[i], cdev->type))
> {
> > > > > > +				__bind(pos, tzp->trip_mask[i], cdev);
> > > > > > +				break;
> > > > > > +			}
> > > > > > +		}
> > > > > > +	}
> > > > > > +	mutex_unlock(&thermal_list_lock);
> > > > > > +}
> > > > >
> > > > > I still do not understand why we need this kind of bind.
> > > > > Say, the platform thermal driver knows the platform data, i.e. it
> knows
> > > > > which cooling devices should be bound to which trip points.
> > > > > why we can not move this kind of logic to the .bind() callback, offered
> > > > > by the platform thermal driver?
> > > > > say, in .bind() callback,
> > > > > the platform thermal driver has the pointer of the platform data,
> right?
> > > > > the .cdev parameter can be used to find the cooling device name,
> > > > > and we can make the comparison there. instead of introducing new
> binding
> > > > > functions in the generic thermal layer.
> > > >
> > > > For once, I got little confused between the generic platform thermal
> sensor
> > > > drivers (the chip drivers) and the platform level driver (not specific for
> chip,
> > > > but for a platform). So, yes we can put this in the platform level driver.
> > > >
> > > Hmm,
> > > I'm not clear about the difference between these two drivers.
> > > what is supposed to be done in the platform thermal sensor drivers and
> > > what is supposed to be done in the platform level driver?
> >
> > A sensor driver can be a generic chip driver like emc1403 (this is the one
> > that I have worked on..) or coretemp (the CPU DTS driver for x86). They sit
> > in different sub systems (these two in hwmon). We might not be allowed
> to
> > add any thermal framework specific code in these drivers. The same driver
> > works on all platforms.
> 
> does the sensor know anything about the "policy"?
> Say, does it have any trip points? does it know which device can be
> throttled to cool itself?
> I think the answer is "no", right?

Yes. You are right :-)
The answer is 'No'.

> >
> > A platform level thermal driver knows information about the thermal
> sensors,
> > and their zones on the platform; and is specific to the platform.
> > For x86, this will be in drivers/x86/platform/ whereas might be in some
> other
> > place for other architectures. An example is intel_mid_thermal.c which sits
> > in drivers/x86/platform. We can add our thermal framework specific code
> > to this driver.
> >
> but I think intel_mide_thermal driver is also a platform thermal sensor
> driver at the same time.

Yes today it is both..

> > >
> > > At least for now, all the thermal drivers are both thermal sensor driver
> > > and platform level driver, right?
> >
> > Not all the times, although there are some instances where both are same.
> > We use coretemp.c and intel_mid_thermal.c (which are different), for the
> > x86 mid platforms.
> >
> so you want to use coretemp.c as a temperature sensor, and then bind
> your own cooling devices to it in your platform level thermal driver?

Yes, coretemp is one fine example.
At least I would like to get the same thing done for emc1403.c
(and few hwmon drivers)..

Thanks,
Durga

Re: [PATCH 07/13] Thermal: Update binding logic based on platform data

[Eduardo Valentin]

Hello,

On Thu, Aug 16, 2012 at 03:31:55AM +0000, R, Durgadoss wrote:
> Hi Rui,
> 
> 
> > -----Original Message-----
> > From: Zhang, Rui
> > Sent: Thursday, August 16, 2012 9:00 AM
> > To: R, Durgadoss
> > Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> > pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> > wni@nvidia.com
> > Subject: RE: [PATCH 07/13] Thermal: Update binding logic based on platform
> > data
> > 
> > On 三, 2012-08-15 at 03:17 -0600, R, Durgadoss wrote:
> > > Hi Rui,
> > >
> > > > > > > +static void update_bind_info(struct thermal_cooling_device
> > *cdev)
> > > > > > > +{
> > > > > > > +	int i, ret;
> > > > > > > +	struct thermal_zone_params *tzp;
> > > > > > > +	struct thermal_zone_device *pos = NULL;
> > > > > > > +
> > > > > > > +	mutex_lock(&thermal_list_lock);
> > > > > > > +
> > > > > > > +	list_for_each_entry(pos, &thermal_tz_list, node) {
> > > > > > > +		if (!pos->tzp && !pos->ops->bind)
> > > > > > > +			continue;
> > > > > > > +
> > > > > > > +		if (!pos->tzp && pos->ops->bind) {
> > > > > > > +			ret = pos->ops->bind(pos, cdev);
> > > > > > > +			if (ret)
> > > > > > > +				print_bind_err_msg(pos, cdev, ret);
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		tzp = pos->tzp;
> > > > > > > +		for (i = 0; i < tzp->num_cdevs; i++) {
> > > > > > > +			if (!strcmp(tzp->cdevs_name[i], cdev->type))
> > {
> > > > > > > +				__bind(pos, tzp->trip_mask[i], cdev);
> > > > > > > +				break;
> > > > > > > +			}
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +	mutex_unlock(&thermal_list_lock);
> > > > > > > +}
> > > > > >
> > > > > > I still do not understand why we need this kind of bind.
> > > > > > Say, the platform thermal driver knows the platform data, i.e. it
> > knows
> > > > > > which cooling devices should be bound to which trip points.
> > > > > > why we can not move this kind of logic to the .bind() callback, offered
> > > > > > by the platform thermal driver?
> > > > > > say, in .bind() callback,
> > > > > > the platform thermal driver has the pointer of the platform data,
> > right?
> > > > > > the .cdev parameter can be used to find the cooling device name,
> > > > > > and we can make the comparison there. instead of introducing new
> > binding
> > > > > > functions in the generic thermal layer.
> > > > >
> > > > > For once, I got little confused between the generic platform thermal
> > sensor
> > > > > drivers (the chip drivers) and the platform level driver (not specific for
> > chip,
> > > > > but for a platform). So, yes we can put this in the platform level driver.
> > > > >
> > > > Hmm,
> > > > I'm not clear about the difference between these two drivers.
> > > > what is supposed to be done in the platform thermal sensor drivers and
> > > > what is supposed to be done in the platform level driver?
> > >
> > > A sensor driver can be a generic chip driver like emc1403 (this is the one
> > > that I have worked on..) or coretemp (the CPU DTS driver for x86). They sit
> > > in different sub systems (these two in hwmon). We might not be allowed
> > to
> > > add any thermal framework specific code in these drivers. The same driver
> > > works on all platforms.
> > 
> > does the sensor know anything about the "policy"?
> > Say, does it have any trip points? does it know which device can be
> > throttled to cool itself?
> > I think the answer is "no", right?
> 
> Yes. You are right :-)
> The answer is 'No'.
> 
> > >
> > > A platform level thermal driver knows information about the thermal
> > sensors,
> > > and their zones on the platform; and is specific to the platform.
> > > For x86, this will be in drivers/x86/platform/ whereas might be in some
> > other
> > > place for other architectures. An example is intel_mid_thermal.c which sits
> > > in drivers/x86/platform. We can add our thermal framework specific code
> > > to this driver.
> > >
> > but I think intel_mide_thermal driver is also a platform thermal sensor
> > driver at the same time.
> 
> Yes today it is both..

So, what is the conclusion from above? I think we need to have a call here as
it will drive how driver code is going to look like. So far, the way I am
designing the OMAP thermal support is that the temp sensor drivers would
know about how to cool the zones, at SoC level. But the cooling of a platform/end-product
level would require another driver, which would require knowledge of availability
of sensors and cooling devices, in order to define the board policy.

> 
> > > >
> > > > At least for now, all the thermal drivers are both thermal sensor driver
> > > > and platform level driver, right?
> > >
> > > Not all the times, although there are some instances where both are same.
> > > We use coretemp.c and intel_mid_thermal.c (which are different), for the
> > > x86 mid platforms.
> > >
> > so you want to use coretemp.c as a temperature sensor, and then bind
> > your own cooling devices to it in your platform level thermal driver?
> 
> Yes, coretemp is one fine example.
> At least I would like to get the same thing done for emc1403.c
> (and few hwmon drivers)..
> 
> Thanks,
> Durga
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/13] Thermal: Introduce fair_share thermal governor

[Durgadoss R]

This patch introduces a simple 'weight' based
governor named fair_share governor. Whenever the
thermal framework gets notified of the trip point
violation, this governor (if configured), throttles
the cooling devices associated with a thermal zone.

This mapping between a thermal zone and a cooling device
and the effectiveness of cooling are provided in the
platform layer.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/Kconfig      |    6 ++
 drivers/thermal/Makefile     |    3 +-
 drivers/thermal/fair_share.c |  128 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h      |    9 +++
 4 files changed, 145 insertions(+), 1 deletion(-)
 create mode 100644 drivers/thermal/fair_share.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 3ab2bd5..f5110c0 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -27,3 +27,9 @@ config SPEAR_THERMAL
 	help
 	  Enable this to plug the SPEAr thermal sensor driver into the Linux
 	  thermal framework
+
+config FAIR_SHARE
+	bool "Fair-share thermal governor"
+	depends on THERMAL
+	help
+	  Enable this to manage platform thermals using fair-share governor.
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index a9fff0b..4ffe1a8 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -3,4 +3,5 @@
 #
 
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
-obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
\ No newline at end of file
+obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
+obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
new file mode 100644
index 0000000..b8c8d45
--- /dev/null
+++ b/drivers/thermal/fair_share.c
@@ -0,0 +1,128 @@
+/*
+ *  fair_share.c - A simple weight based Thermal governor
+ *
+ *  Copyright (C) 2012 Intel Corp
+ *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/thermal.h>
+
+/**
+ * get_trip_level: - obtains the current trip level for a zone
+ * @tz:		thermal zone device
+ */
+static int get_trip_level(struct thermal_zone_device *tz)
+{
+	int count = 0;
+	unsigned long trip_temp;
+
+	if (tz->trips == 0 || !tz->ops->get_trip_temp)
+		return 0;
+
+	for (count = 0; count < tz->trips; count++) {
+		tz->ops->get_trip_temp(tz, count, &trip_temp);
+		if (tz->temperature < trip_temp)
+			break;
+	}
+	return count;
+}
+
+static long get_target_state(struct thermal_zone_device *tz,
+		struct thermal_cooling_device *cdev, int weight, int level)
+{
+	unsigned long max_state;
+
+	cdev->ops->get_max_state(cdev, &max_state);
+
+	return (long)(weight * level * max_state) / (100 * tz->trips);
+}
+
+static void thermal_cdev_update(struct thermal_cooling_device *cdev)
+{
+	struct thermal_instance *instance;
+	unsigned long target = 0;
+
+	mutex_lock(&cdev->lock);
+
+	/* Make sure cdev enters the deepest cooling state */
+	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
+		if (instance->target > target)
+			target = instance->target;
+	}
+
+	mutex_unlock(&cdev->lock);
+
+	cdev->ops->set_cur_state(cdev, target);
+}
+
+/**
+ * fair_share_throttle - throttles devices asscciated with the given zone
+ * @tz - thermal_zone_device
+ *
+ * Throttling Logic: This uses three parameters to calculate the new
+ * throttle state of the cooling devices associated with the given zone.
+ *
+ * Parameters used for Throttling:
+ * P1. max_state: Maximum throttle state exposed by the cooling device.
+ * P2. weight[i]/100:
+ *	How 'effective' the 'i'th device is, in cooling the given zone.
+ * P3. cur_trip_level/max_no_of_trips:
+ *	This describes the extent to which the devices should be throttled.
+ *	We do not want to throttle too much when we trip a lower temperature,
+ *	whereas the throttling is at full swing if we trip critical levels.
+ *	(Heavily assumes the trip points are in ascending order)
+ * new_state of cooling device = P3 * P2 * P1
+ */
+void fair_share_throttle(struct thermal_zone_device *tz, int trip)
+{
+	struct thermal_zone_params *tzp;
+	struct thermal_cooling_device *cdev;
+	struct thermal_instance *instance;
+	int i;
+	int cur_trip_level = get_trip_level(tz);
+
+	if (!tz->tzp)
+		return;
+
+	tzp = tz->tzp;
+
+	for (i = 0; i < tzp->num_cdevs; i++) {
+		cdev = get_cdev_by_name(tzp->cdevs_name[i]);
+		if (!cdev)
+			continue;
+
+		instance = get_thermal_instance(tz, cdev, trip);
+		if (!instance)
+			continue;
+
+		instance->target = get_target_state(tz, cdev,
+					tzp->weights[i], cur_trip_level);
+
+		thermal_cdev_update(cdev);
+	}
+}
+EXPORT_SYMBOL(fair_share_throttle);
+
+MODULE_AUTHOR("Durgadoss R");
+MODULE_DESCRIPTION("A simple weight based thermal throttling governor");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 1d49f05..4e2dc2c 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -257,6 +257,15 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
  */
 extern int (*get_platform_thermal_params)(struct thermal_zone_device *);
 
+#ifdef CONFIG_FAIR_SHARE
+extern void fair_share_throttle(struct thermal_zone_device *tz, int trip);
+#else
+static inline void fair_share_throttle(struct thermal_zone_device *tz, int trip)
+{
+	return;
+}
+#endif
+
 #ifdef CONFIG_NET
 extern int thermal_generate_netlink_event(u32 orig, enum events event);
 #else
-- 
1.7.9.5

Re: [PATCH 08/13] Thermal: Introduce fair_share thermal governor

[Eduardo Valentin]

Hello,

On Thu, Aug 09, 2012 at 06:16:00PM +0530, Durgadoss R wrote:
> This patch introduces a simple 'weight' based
> governor named fair_share governor. Whenever the
> thermal framework gets notified of the trip point
> violation, this governor (if configured), throttles
> the cooling devices associated with a thermal zone.
> 
> This mapping between a thermal zone and a cooling device
> and the effectiveness of cooling are provided in the
> platform layer.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/Kconfig      |    6 ++
>  drivers/thermal/Makefile     |    3 +-
>  drivers/thermal/fair_share.c |  128 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h      |    9 +++
>  4 files changed, 145 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/thermal/fair_share.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 3ab2bd5..f5110c0 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -27,3 +27,9 @@ config SPEAR_THERMAL
>  	help
>  	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>  	  thermal framework
> +
> +config FAIR_SHARE
> +	bool "Fair-share thermal governor"
> +	depends on THERMAL
> +	help
> +	  Enable this to manage platform thermals using fair-share governor.
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index a9fff0b..4ffe1a8 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -3,4 +3,5 @@
>  #
>  
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
> -obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
> \ No newline at end of file
> +obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
> +obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> new file mode 100644
> index 0000000..b8c8d45
> --- /dev/null
> +++ b/drivers/thermal/fair_share.c
> @@ -0,0 +1,128 @@
> +/*
> + *  fair_share.c - A simple weight based Thermal governor
> + *
> + *  Copyright (C) 2012 Intel Corp
> + *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
> + *
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +
> +/**
> + * get_trip_level: - obtains the current trip level for a zone
> + * @tz:		thermal zone device
> + */
> +static int get_trip_level(struct thermal_zone_device *tz)
> +{
> +	int count = 0;
> +	unsigned long trip_temp;
> +
> +	if (tz->trips == 0 || !tz->ops->get_trip_temp)
> +		return 0;
> +
> +	for (count = 0; count < tz->trips; count++) {
> +		tz->ops->get_trip_temp(tz, count, &trip_temp);
> +		if (tz->temperature < trip_temp)
> +			break;
> +	}
> +	return count;
> +}
> +
> +static long get_target_state(struct thermal_zone_device *tz,
> +		struct thermal_cooling_device *cdev, int weight, int level)
> +{
> +	unsigned long max_state;
> +
> +	cdev->ops->get_max_state(cdev, &max_state);
> +
> +	return (long)(weight * level * max_state) / (100 * tz->trips);
> +}
> +
> +static void thermal_cdev_update(struct thermal_cooling_device *cdev)
> +{
> +	struct thermal_instance *instance;
> +	unsigned long target = 0;
> +
> +	mutex_lock(&cdev->lock);
> +
> +	/* Make sure cdev enters the deepest cooling state */
> +	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> +		if (instance->target > target)
> +			target = instance->target;
> +	}
> +
> +	mutex_unlock(&cdev->lock);
> +
> +	cdev->ops->set_cur_state(cdev, target);
> +}

I believe Rui has already provided an arbitrator, can we reuse it here as well?

> +
> +/**
> + * fair_share_throttle - throttles devices asscciated with the given zone
> + * @tz - thermal_zone_device
> + *
> + * Throttling Logic: This uses three parameters to calculate the new
> + * throttle state of the cooling devices associated with the given zone.
> + *
> + * Parameters used for Throttling:
> + * P1. max_state: Maximum throttle state exposed by the cooling device.
> + * P2. weight[i]/100:
> + *	How 'effective' the 'i'th device is, in cooling the given zone.
> + * P3. cur_trip_level/max_no_of_trips:
> + *	This describes the extent to which the devices should be throttled.
> + *	We do not want to throttle too much when we trip a lower temperature,
> + *	whereas the throttling is at full swing if we trip critical levels.
> + *	(Heavily assumes the trip points are in ascending order)
> + * new_state of cooling device = P3 * P2 * P1
> + */
> +void fair_share_throttle(struct thermal_zone_device *tz, int trip)
> +{
> +	struct thermal_zone_params *tzp;
> +	struct thermal_cooling_device *cdev;
> +	struct thermal_instance *instance;
> +	int i;
> +	int cur_trip_level = get_trip_level(tz);
> +
> +	if (!tz->tzp)
> +		return;
> +
> +	tzp = tz->tzp;
> +
> +	for (i = 0; i < tzp->num_cdevs; i++) {
> +		cdev = get_cdev_by_name(tzp->cdevs_name[i]);
> +		if (!cdev)
> +			continue;
> +
> +		instance = get_thermal_instance(tz, cdev, trip);
> +		if (!instance)
> +			continue;
> +
> +		instance->target = get_target_state(tz, cdev,
> +					tzp->weights[i], cur_trip_level);
> +
> +		thermal_cdev_update(cdev);
> +	}
> +}
> +EXPORT_SYMBOL(fair_share_throttle);
> +
> +MODULE_AUTHOR("Durgadoss R");
> +MODULE_DESCRIPTION("A simple weight based thermal throttling governor");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 1d49f05..4e2dc2c 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -257,6 +257,15 @@ struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>   */
>  extern int (*get_platform_thermal_params)(struct thermal_zone_device *);
>  
> +#ifdef CONFIG_FAIR_SHARE
> +extern void fair_share_throttle(struct thermal_zone_device *tz, int trip);
> +#else
> +static inline void fair_share_throttle(struct thermal_zone_device *tz, int trip)
> +{
> +	return;
> +}
> +#endif
> +
>  #ifdef CONFIG_NET
>  extern int thermal_generate_netlink_event(u32 orig, enum events event);
>  #else
> -- 
> 1.7.9.5
> 

RE: [PATCH 08/13] Thermal: Introduce fair_share thermal governor

[R, Durgadoss]

Hi Eduardo,

> > +static long get_target_state(struct thermal_zone_device *tz,
> > +		struct thermal_cooling_device *cdev, int weight, int level)
> > +{
> > +	unsigned long max_state;
> > +
> > +	cdev->ops->get_max_state(cdev, &max_state);
> > +
> > +	return (long)(weight * level * max_state) / (100 * tz->trips);
> > +}
> > +
> > +static void thermal_cdev_update(struct thermal_cooling_device *cdev)
> > +{
> > +	struct thermal_instance *instance;
> > +	unsigned long target = 0;
> > +
> > +	mutex_lock(&cdev->lock);
> > +
> > +	/* Make sure cdev enters the deepest cooling state */
> > +	list_for_each_entry(instance, &cdev->thermal_instances,
> cdev_node) {
> > +		if (instance->target > target)
> > +			target = instance->target;
> > +	}
> > +
> > +	mutex_unlock(&cdev->lock);
> > +
> > +	cdev->ops->set_cur_state(cdev, target);
> > +}
> 
> I believe Rui has already provided an arbitrator, can we reuse it here as well?

I thought about this. I was of the opinion a governor can choose to do arbitration
in its own specific way. For example, The fair_share does not check 'cdev->updated'
value whenever it tries to update the state of a cooling device.

Thanks,
Durga

Re: [PATCH 08/13] Thermal: Introduce fair_share thermal governor

[Eduardo Valentin]

hello,

On Tue, Aug 21, 2012 at 05:59:00AM +0000, R, Durgadoss wrote:
> Hi Eduardo,
> 
> > > +static long get_target_state(struct thermal_zone_device *tz,
> > > +		struct thermal_cooling_device *cdev, int weight, int level)
> > > +{
> > > +	unsigned long max_state;
> > > +
> > > +	cdev->ops->get_max_state(cdev, &max_state);
> > > +
> > > +	return (long)(weight * level * max_state) / (100 * tz->trips);
> > > +}
> > > +
> > > +static void thermal_cdev_update(struct thermal_cooling_device *cdev)
> > > +{
> > > +	struct thermal_instance *instance;
> > > +	unsigned long target = 0;
> > > +
> > > +	mutex_lock(&cdev->lock);
> > > +
> > > +	/* Make sure cdev enters the deepest cooling state */
> > > +	list_for_each_entry(instance, &cdev->thermal_instances,
> > cdev_node) {
> > > +		if (instance->target > target)
> > > +			target = instance->target;
> > > +	}
> > > +
> > > +	mutex_unlock(&cdev->lock);
> > > +
> > > +	cdev->ops->set_cur_state(cdev, target);
> > > +}
> > 
> > I believe Rui has already provided an arbitrator, can we reuse it here as well?
> 
> I thought about this. I was of the opinion a governor can choose to do arbitration
> in its own specific way. For example, The fair_share does not check 'cdev->updated'
> value whenever it tries to update the state of a cooling device.

I see. the arbitration starts to get more and more complex. not to talk about constraints
coming from performance domain and not thermal.

I'd still try to find a central point for doing the arbitration, this way we grow the code
in a scalable way...

> 
> Thanks,
> Durga

[PATCH 09/13] Thermal: Introduce a step_wise thermal governor

[Durgadoss R]

This patch adds a simple step_wise governor to the
generic thermal layer. This algorithm throttles the
cooling devices in a linear fashion. If the 'trend'
is heating, it throttles by one step. And if the
thermal trend is cooling it de-throttles by one step.

This actually moves the throttling logic from thermal_sys.c
and puts inside step_wise.c, without any change.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/Kconfig     |    6 ++
 drivers/thermal/Makefile    |    1 +
 drivers/thermal/step_wise.c |  204 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h     |    9 ++
 4 files changed, 220 insertions(+)
 create mode 100644 drivers/thermal/step_wise.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index f5110c0..0401cdf 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -33,3 +33,9 @@ config FAIR_SHARE
 	depends on THERMAL
 	help
 	  Enable this to manage platform thermals using fair-share governor.
+
+config STEP_WISE
+	bool "Step_wise thermal governor"
+	depends on THERMAL
+	help
+	  Enable this to manage platform thermals using a simple linear
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 4ffe1a8..c2c0ce0 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -5,3 +5,4 @@
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
 obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
 obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
+obj-$(CONFIG_STEP_WISE)			+= step_wise.o
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
new file mode 100644
index 0000000..0456b38
--- /dev/null
+++ b/drivers/thermal/step_wise.c
@@ -0,0 +1,204 @@
+/*
+ *  step_wise.c - A step-by-step Thermal throttling governor
+ *
+ *  Copyright (C) 2012 Intel Corp
+ *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/thermal.h>
+
+/*
+ * If the temperature is higher than a trip point,
+ *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
+ *       state for this trip point
+ *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
+ *       state for this trip point
+ */
+static unsigned long get_target_state(struct thermal_instance *instance,
+					enum thermal_trend trend)
+{
+	struct thermal_cooling_device *cdev = instance->cdev;
+	unsigned long cur_state;
+
+	cdev->ops->get_cur_state(cdev, &cur_state);
+
+	if (trend == THERMAL_TREND_RAISING) {
+		cur_state = cur_state < instance->upper ?
+			    (cur_state + 1) : instance->upper;
+	} else if (trend == THERMAL_TREND_DROPPING) {
+		cur_state = cur_state > instance->lower ?
+			    (cur_state - 1) : instance->lower;
+	}
+
+	return cur_state;
+}
+
+static void update_passive_instance(struct thermal_zone_device *tz,
+				enum thermal_trip_type type, int value)
+{
+	/*
+	 * If value is +1, activate a passive instance.
+	 * If value is -1, deactivate a passive instance.
+	 */
+	if (type == THERMAL_TRIP_PASSIVE || type == THERMAL_TRIPS_NONE)
+		tz->passive += value;
+}
+
+static void update_instance_for_throttle(struct thermal_zone_device *tz,
+				int trip, enum thermal_trip_type trip_type,
+				enum thermal_trend trend)
+{
+	struct thermal_instance *instance;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		if (instance->trip != trip)
+			continue;
+
+		instance->target = get_target_state(instance, trend);
+
+		/* Activate a passive thermal instance */
+		if (instance->target == THERMAL_NO_TARGET)
+			update_passive_instance(tz, trip_type, 1);
+
+		instance->cdev->updated = false; /* cdev needs update */
+	}
+}
+
+static void update_instance_for_dethrottle(struct thermal_zone_device *tz,
+				int trip, enum thermal_trip_type trip_type)
+{
+	struct thermal_instance *instance;
+	struct thermal_cooling_device *cdev;
+	unsigned long cur_state;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		if (instance->trip != trip ||
+			instance->target == THERMAL_NO_TARGET)
+			continue;
+
+		cdev = instance->cdev;
+		cdev->ops->get_cur_state(cdev, &cur_state);
+
+		instance->target = cur_state > instance->lower ?
+			    (cur_state - 1) : THERMAL_NO_TARGET;
+
+		/* Deactivate a passive thermal instance */
+		if (instance->target == THERMAL_NO_TARGET)
+			update_passive_instance(tz, trip_type, -1);
+
+		cdev->updated = false; /* cdev needs update */
+	}
+}
+
+static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
+{
+	long trip_temp;
+	enum thermal_trip_type trip_type;
+	enum thermal_trend trend;
+
+	if (trip == THERMAL_TRIPS_NONE) {
+		trip_temp = tz->forced_passive;
+		trip_type = THERMAL_TRIPS_NONE;
+	} else {
+		tz->ops->get_trip_temp(tz, trip, &trip_temp);
+		tz->ops->get_trip_type(tz, trip, &trip_type);
+	}
+
+	trend = get_tz_trend(tz, trip);
+
+	mutex_lock(&tz->lock);
+
+	if (tz->temperature >= trip_temp)
+		update_instance_for_throttle(tz, trip, trip_type, trend);
+	else
+		update_instance_for_dethrottle(tz, trip, trip_type);
+
+	mutex_unlock(&tz->lock);
+}
+
+/**
+ * thermal_cdev_update - Updates the throttle state for the given cdev
+ * @cdev - cooling device, for which the throttle state is to be set
+ *
+ * This function loops through all the instances for the given cdev,
+ * to find out the maximum throttle state; and sets the cur_state of
+ * the cdev to that value.
+ */
+static void thermal_cdev_update(struct thermal_cooling_device *cdev)
+{
+	struct thermal_instance *instance;
+	unsigned long target = 0;
+
+	/* cdev is already updated */
+	if (cdev->updated)
+		return;
+
+	mutex_lock(&cdev->lock);
+
+	/* Make sure cdev enters the deepest cooling state */
+	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
+		if (instance->target == THERMAL_NO_TARGET)
+			continue;
+		if (instance->target > target)
+			target = instance->target;
+	}
+
+	mutex_unlock(&cdev->lock);
+
+	cdev->ops->set_cur_state(cdev, target);
+	cdev->updated = true;
+}
+
+/**
+ * step_wise_throttle - throttles devices asscciated with the given zone
+ * @tz - thermal_zone_device
+ * @trip - the trip point
+ * @trip_type - type of the trip point
+ *
+ * Throttling Logic: This uses the trend of the thermal zone to throttle.
+ * If the thermal zone is 'heating up' this throttles all the cooling
+ * devices associated with the zone and its particular trip point, by one
+ * step. If the zone is 'cooling down' it brings back the performance of
+ * the devices by one step.
+ */
+void step_wise_throttle(struct thermal_zone_device *tz, int trip)
+{
+	struct thermal_instance *instance;
+
+	thermal_zone_trip_update(tz, trip);
+
+	if (tz->forced_passive)
+		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
+
+	mutex_lock(&tz->lock);
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
+		thermal_cdev_update(instance->cdev);
+
+	mutex_unlock(&tz->lock);
+}
+EXPORT_SYMBOL(step_wise_throttle);
+
+MODULE_AUTHOR("Durgadoss R");
+MODULE_DESCRIPTION("A step-by-step thermal throttling governor");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 4e2dc2c..60d2743 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -266,6 +266,15 @@ static inline void fair_share_throttle(struct thermal_zone_device *tz, int trip)
 }
 #endif
 
+#ifdef CONFIG_STEP_WISE
+extern void step_wise_throttle(struct thermal_zone_device *tz, int trip);
+#else
+static inline void step_wise_throttle(struct thermal_zone_device *tz, int trip)
+{
+	return;
+}
+#endif
+
 #ifdef CONFIG_NET
 extern int thermal_generate_netlink_event(u32 orig, enum events event);
 #else
-- 
1.7.9.5

Re: [PATCH 09/13] Thermal: Introduce a step_wise thermal governor

[Eduardo Valentin]

Hello,

On Thu, Aug 09, 2012 at 06:16:01PM +0530, Durgadoss R wrote:
> This patch adds a simple step_wise governor to the
> generic thermal layer. This algorithm throttles the
> cooling devices in a linear fashion. If the 'trend'
> is heating, it throttles by one step. And if the
> thermal trend is cooling it de-throttles by one step.
> 
> This actually moves the throttling logic from thermal_sys.c
> and puts inside step_wise.c, without any change.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/Kconfig     |    6 ++
>  drivers/thermal/Makefile    |    1 +
>  drivers/thermal/step_wise.c |  204 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h     |    9 ++
>  4 files changed, 220 insertions(+)
>  create mode 100644 drivers/thermal/step_wise.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index f5110c0..0401cdf 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -33,3 +33,9 @@ config FAIR_SHARE
>  	depends on THERMAL
>  	help
>  	  Enable this to manage platform thermals using fair-share governor.
> +
> +config STEP_WISE
> +	bool "Step_wise thermal governor"
> +	depends on THERMAL
> +	help
> +	  Enable this to manage platform thermals using a simple linear
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 4ffe1a8..c2c0ce0 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -5,3 +5,4 @@
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
>  obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
>  obj-$(CONFIG_FAIR_SHARE)		+= fair_share.o
> +obj-$(CONFIG_STEP_WISE)			+= step_wise.o
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> new file mode 100644
> index 0000000..0456b38
> --- /dev/null
> +++ b/drivers/thermal/step_wise.c
> @@ -0,0 +1,204 @@
> +/*
> + *  step_wise.c - A step-by-step Thermal throttling governor
> + *
> + *  Copyright (C) 2012 Intel Corp
> + *  Copyright (C) 2012 Durgadoss R <durgadoss.r@intel.com>
> + *
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +
> +/*
> + * If the temperature is higher than a trip point,
> + *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> + *       state for this trip point
> + *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> + *       state for this trip point
> + */
> +static unsigned long get_target_state(struct thermal_instance *instance,
> +					enum thermal_trend trend)
> +{
> +	struct thermal_cooling_device *cdev = instance->cdev;
> +	unsigned long cur_state;
> +
> +	cdev->ops->get_cur_state(cdev, &cur_state);
> +
> +	if (trend == THERMAL_TREND_RAISING) {
> +		cur_state = cur_state < instance->upper ?
> +			    (cur_state + 1) : instance->upper;
> +	} else if (trend == THERMAL_TREND_DROPPING) {
> +		cur_state = cur_state > instance->lower ?
> +			    (cur_state - 1) : instance->lower;
> +	}
> +
> +	return cur_state;
> +}
> +
> +static void update_passive_instance(struct thermal_zone_device *tz,
> +				enum thermal_trip_type type, int value)
> +{
> +	/*
> +	 * If value is +1, activate a passive instance.
> +	 * If value is -1, deactivate a passive instance.
> +	 */
> +	if (type == THERMAL_TRIP_PASSIVE || type == THERMAL_TRIPS_NONE)
> +		tz->passive += value;
> +}
> +
> +static void update_instance_for_throttle(struct thermal_zone_device *tz,
> +				int trip, enum thermal_trip_type trip_type,
> +				enum thermal_trend trend)
> +{
> +	struct thermal_instance *instance;
> +
> +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +		if (instance->trip != trip)
> +			continue;
> +
> +		instance->target = get_target_state(instance, trend);
> +
> +		/* Activate a passive thermal instance */
> +		if (instance->target == THERMAL_NO_TARGET)
> +			update_passive_instance(tz, trip_type, 1);
> +
> +		instance->cdev->updated = false; /* cdev needs update */
> +	}
> +}
> +
> +static void update_instance_for_dethrottle(struct thermal_zone_device *tz,
> +				int trip, enum thermal_trip_type trip_type)
> +{
> +	struct thermal_instance *instance;
> +	struct thermal_cooling_device *cdev;
> +	unsigned long cur_state;
> +
> +	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +		if (instance->trip != trip ||
> +			instance->target == THERMAL_NO_TARGET)
> +			continue;
> +
> +		cdev = instance->cdev;
> +		cdev->ops->get_cur_state(cdev, &cur_state);
> +
> +		instance->target = cur_state > instance->lower ?
> +			    (cur_state - 1) : THERMAL_NO_TARGET;
> +
> +		/* Deactivate a passive thermal instance */
> +		if (instance->target == THERMAL_NO_TARGET)
> +			update_passive_instance(tz, trip_type, -1);
> +
> +		cdev->updated = false; /* cdev needs update */
> +	}
> +}
> +
> +static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
> +{
> +	long trip_temp;
> +	enum thermal_trip_type trip_type;
> +	enum thermal_trend trend;
> +
> +	if (trip == THERMAL_TRIPS_NONE) {
> +		trip_temp = tz->forced_passive;
> +		trip_type = THERMAL_TRIPS_NONE;
> +	} else {
> +		tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +		tz->ops->get_trip_type(tz, trip, &trip_type);
> +	}
> +
> +	trend = get_tz_trend(tz, trip);
> +
> +	mutex_lock(&tz->lock);
> +
> +	if (tz->temperature >= trip_temp)
> +		update_instance_for_throttle(tz, trip, trip_type, trend);
> +	else
> +		update_instance_for_dethrottle(tz, trip, trip_type);
> +
> +	mutex_unlock(&tz->lock);
> +}
> +
> +/**
> + * thermal_cdev_update - Updates the throttle state for the given cdev
> + * @cdev - cooling device, for which the throttle state is to be set
> + *
> + * This function loops through all the instances for the given cdev,
> + * to find out the maximum throttle state; and sets the cur_state of
> + * the cdev to that value.
> + */
> +static void thermal_cdev_update(struct thermal_cooling_device *cdev)
> +{
> +	struct thermal_instance *instance;
> +	unsigned long target = 0;
> +
> +	/* cdev is already updated */
> +	if (cdev->updated)
> +		return;
> +
> +	mutex_lock(&cdev->lock);
> +
> +	/* Make sure cdev enters the deepest cooling state */
> +	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> +		if (instance->target == THERMAL_NO_TARGET)
> +			continue;
> +		if (instance->target > target)
> +			target = instance->target;
> +	}
> +
> +	mutex_unlock(&cdev->lock);
> +
> +	cdev->ops->set_cur_state(cdev, target);
> +	cdev->updated = true;
> +}

We start to spread arbitrators. I believe we should revisit this and reuse what is currently
available. Or, in case not usable, refactor is so that it can be reused.

> +
> +/**
> + * step_wise_throttle - throttles devices asscciated with the given zone
> + * @tz - thermal_zone_device
> + * @trip - the trip point
> + * @trip_type - type of the trip point
> + *
> + * Throttling Logic: This uses the trend of the thermal zone to throttle.
> + * If the thermal zone is 'heating up' this throttles all the cooling
> + * devices associated with the zone and its particular trip point, by one
> + * step. If the zone is 'cooling down' it brings back the performance of
> + * the devices by one step.
> + */
> +void step_wise_throttle(struct thermal_zone_device *tz, int trip)
> +{
> +	struct thermal_instance *instance;
> +
> +	thermal_zone_trip_update(tz, trip);
> +
> +	if (tz->forced_passive)
> +		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
> +
> +	mutex_lock(&tz->lock);
> +
> +	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> +		thermal_cdev_update(instance->cdev);
> +
> +	mutex_unlock(&tz->lock);
> +}
> +EXPORT_SYMBOL(step_wise_throttle);
> +
> +MODULE_AUTHOR("Durgadoss R");
> +MODULE_DESCRIPTION("A step-by-step thermal throttling governor");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 4e2dc2c..60d2743 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -266,6 +266,15 @@ static inline void fair_share_throttle(struct thermal_zone_device *tz, int trip)
>  }
>  #endif
>  
> +#ifdef CONFIG_STEP_WISE
> +extern void step_wise_throttle(struct thermal_zone_device *tz, int trip);
> +#else
> +static inline void step_wise_throttle(struct thermal_zone_device *tz, int trip)
> +{
> +	return;
> +}
> +#endif
> +
>  #ifdef CONFIG_NET
>  extern int thermal_generate_netlink_event(u32 orig, enum events event);
>  #else
> -- 
> 1.7.9.5
> 

[PATCH 10/13] Thermal: Remove throttling logic out of thermal_sys.c

[Durgadoss R]

This patch removes the throttling logic out of
thermal_sys.c; also refactors the code into smaller
functions so that are easy to read/maintain.
 * Seperates the handling of critical and non-critical trips
 * Re-arranges the set_polling and device_check methods, so
   that all related functions are arranged in one place.
 * Removes the 'do_update' and 'trip_update' method, as part
   of moving the throttling logic out of thermal_sys.c

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |  359 ++++++++++++++++-------------------------
 include/linux/thermal.h       |    2 +-
 2 files changed, 137 insertions(+), 224 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 748b12f..193d071 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -259,6 +259,142 @@ static void do_binding(struct thermal_zone_device *tz)
 	}
 }
 
+static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
+					    int delay)
+{
+	cancel_delayed_work(&(tz->poll_queue));
+
+	if (!delay)
+		return;
+
+	if (delay > 1000)
+		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
+				      round_jiffies(msecs_to_jiffies(delay)));
+	else
+		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
+				      msecs_to_jiffies(delay));
+}
+
+static void monitor_thermal_zone(struct thermal_zone_device *tz)
+{
+	mutex_lock(&tz->lock);
+
+	if (tz->passive)
+		thermal_zone_device_set_polling(tz, tz->passive_delay);
+	else if (tz->polling_delay)
+		thermal_zone_device_set_polling(tz, tz->polling_delay);
+	else
+		thermal_zone_device_set_polling(tz, 0);
+
+	mutex_unlock(&tz->lock);
+}
+
+static void notify_user_space(struct thermal_zone_device *tz, int trip)
+{
+	mutex_lock(&tz->lock);
+
+	kobject_uevent(&tz->device.kobj, KOBJ_CHANGE);
+
+	mutex_unlock(&tz->lock);
+}
+
+static void handle_non_critical_trips(struct thermal_zone_device *tz,
+			int trip, enum thermal_trip_type trip_type)
+{
+	int throttle_policy = THERMAL_STEP_WISE;
+
+	if (tz->tzp)
+		throttle_policy = tz->tzp->throttle_policy;
+
+	switch (throttle_policy) {
+	case THERMAL_FAIR_SHARE:
+		fair_share_throttle(tz, trip);
+		break;
+	case THERMAL_STEP_WISE:
+		step_wise_throttle(tz, trip);
+		break;
+	case THERMAL_USER_SPACE:
+		notify_user_space(tz, trip);
+		break;
+	}
+}
+
+static void handle_critical_trips(struct thermal_zone_device *tz,
+				int trip, enum thermal_trip_type trip_type)
+{
+	long trip_temp;
+
+	tz->ops->get_trip_temp(tz, trip, &trip_temp);
+
+	/* If we have not crossed the trip_temp, we do not care. */
+	if (tz->temperature < trip_temp)
+		return;
+
+	if (tz->ops->notify)
+		tz->ops->notify(tz, trip, trip_type);
+
+	if (trip_type == THERMAL_TRIP_CRITICAL) {
+		pr_emerg("Critical temperature reached(%d C),shutting down\n",
+			 tz->temperature / 1000);
+		orderly_poweroff(true);
+	}
+}
+
+static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
+{
+	enum thermal_trip_type type;
+
+	tz->ops->get_trip_type(tz, trip, &type);
+
+	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
+		handle_critical_trips(tz, trip, type);
+	else
+		handle_non_critical_trips(tz, trip, type);
+	/*
+	 * Alright, we handled this trip successfully.
+	 * So, start monitoring again.
+	 */
+	monitor_thermal_zone(tz);
+}
+
+static void update_temperature(struct thermal_zone_device *tz)
+{
+	long temp;
+	int ret;
+
+	mutex_lock(&tz->lock);
+
+	ret = tz->ops->get_temp(tz, &temp);
+	if (ret) {
+		pr_warn("failed to read out thermal zone %d\n", tz->id);
+		return;
+	}
+
+	tz->last_temperature = tz->temperature;
+	tz->temperature = temp;
+
+	mutex_unlock(&tz->lock);
+}
+
+void thermal_zone_device_update(struct thermal_zone_device *tz)
+{
+	int count;
+
+	update_temperature(tz);
+
+	for (count = 0; count < tz->trips; count++)
+		handle_thermal_trip(tz, count);
+}
+EXPORT_SYMBOL(thermal_zone_device_update);
+
+static void thermal_zone_device_check(struct work_struct *work)
+{
+	struct thermal_zone_device *tz = container_of(work, struct
+						      thermal_zone_device,
+						      poll_queue.work);
+	thermal_zone_device_update(tz);
+}
+
 /* sys I/F for thermal zone */
 
 #define to_thermal_zone(_dev) \
@@ -878,30 +1014,6 @@ thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
 }
 #endif
 
-static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
-					    int delay)
-{
-	cancel_delayed_work(&(tz->poll_queue));
-
-	if (!delay)
-		return;
-
-	if (delay > 1000)
-		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
-				      round_jiffies(msecs_to_jiffies(delay)));
-	else
-		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
-				      msecs_to_jiffies(delay));
-}
-
-static void thermal_zone_device_check(struct work_struct *work)
-{
-	struct thermal_zone_device *tz = container_of(work, struct
-						      thermal_zone_device,
-						      poll_queue.work);
-	thermal_zone_device_update(tz);
-}
-
 /**
  * thermal_zone_bind_cooling_device - bind a cooling device to a thermal zone
  * @tz:		thermal zone device
@@ -1197,205 +1309,6 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 }
 EXPORT_SYMBOL(thermal_cooling_device_unregister);
 
-static void thermal_cdev_do_update(struct thermal_cooling_device *cdev)
-{
-	struct thermal_instance *instance;
-	unsigned long target = 0;
-
-	/* cooling device is updated*/
-	if (cdev->updated)
-		return;
-
-	mutex_lock(&cdev->lock);
-	/* Make sure cdev enters the deepest cooling state */
-	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
-		if (instance->target == THERMAL_NO_TARGET)
-			continue;
-		if (instance->target > target)
-			target = instance->target;
-	}
-	mutex_unlock(&cdev->lock);
-	cdev->ops->set_cur_state(cdev, target);
-	cdev->updated = true;
-}
-
-static void thermal_zone_do_update(struct thermal_zone_device *tz)
-{
-	struct thermal_instance *instance;
-
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		thermal_cdev_do_update(instance->cdev);
-}
-
-/*
- * Cooling algorithm for both active and passive cooling
- *
- * 1. if the temperature is higher than a trip point,
- *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
- *       state for this trip point
- *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
- *       state for this trip point
- *
- * 2. if the temperature is lower than a trip point, use lower
- *    cooling state for this trip point
- *
- * Note that this behaves the same as the previous passive cooling
- * algorithm.
- */
-
-static void thermal_zone_trip_update(struct thermal_zone_device *tz,
-				     int trip, long temp)
-{
-	struct thermal_instance *instance;
-	struct thermal_cooling_device *cdev = NULL;
-	unsigned long cur_state, max_state;
-	long trip_temp;
-	enum thermal_trip_type trip_type;
-	enum thermal_trend trend;
-
-	if (trip == THERMAL_TRIPS_NONE) {
-		trip_temp = tz->forced_passive;
-		trip_type = THERMAL_TRIPS_NONE;
-	} else {
-		tz->ops->get_trip_temp(tz, trip, &trip_temp);
-		tz->ops->get_trip_type(tz, trip, &trip_type);
-	}
-
-	if (!tz->ops->get_trend || tz->ops->get_trend(tz, trip, &trend)) {
-		/*
-		 * compare the current temperature and previous temperature
-		 * to get the thermal trend, if no special requirement
-		 */
-		if (tz->temperature > tz->last_temperature)
-			trend = THERMAL_TREND_RAISING;
-		else if (tz->temperature < tz->last_temperature)
-			trend = THERMAL_TREND_DROPPING;
-		else
-			trend = THERMAL_TREND_STABLE;
-	}
-
-	if (temp >= trip_temp) {
-		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-			if (instance->trip != trip)
-				continue;
-
-			cdev = instance->cdev;
-
-			cdev->ops->get_cur_state(cdev, &cur_state);
-			cdev->ops->get_max_state(cdev, &max_state);
-
-			if (trend == THERMAL_TREND_RAISING) {
-				cur_state = cur_state < instance->upper ?
-					    (cur_state + 1) : instance->upper;
-			} else if (trend == THERMAL_TREND_DROPPING) {
-				cur_state = cur_state > instance->lower ?
-				    (cur_state - 1) : instance->lower;
-			}
-
-			/* activate a passive thermal instance */
-			if ((trip_type == THERMAL_TRIP_PASSIVE ||
-			     trip_type == THERMAL_TRIPS_NONE) &&
-			     instance->target == THERMAL_NO_TARGET)
-				tz->passive++;
-
-			instance->target = cur_state;
-			cdev->updated = false; /* cooling device needs update */
-		}
-	} else {	/* below trip */
-		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-			if (instance->trip != trip)
-				continue;
-
-			/* Do not use the inactive thermal instance */
-			if (instance->target == THERMAL_NO_TARGET)
-				continue;
-			cdev = instance->cdev;
-			cdev->ops->get_cur_state(cdev, &cur_state);
-
-			cur_state = cur_state > instance->lower ?
-				    (cur_state - 1) : THERMAL_NO_TARGET;
-
-			/* deactivate a passive thermal instance */
-			if ((trip_type == THERMAL_TRIP_PASSIVE ||
-			     trip_type == THERMAL_TRIPS_NONE) &&
-			     cur_state == THERMAL_NO_TARGET)
-				tz->passive--;
-			instance->target = cur_state;
-			cdev->updated = false; /* cooling device needs update */
-		}
-	}
-
-	return;
-}
-/**
- * thermal_zone_device_update - force an update of a thermal zone's state
- * @ttz:	the thermal zone to update
- */
-
-void thermal_zone_device_update(struct thermal_zone_device *tz)
-{
-	int count, ret = 0;
-	long temp, trip_temp;
-	enum thermal_trip_type trip_type;
-
-	mutex_lock(&tz->lock);
-
-	if (tz->ops->get_temp(tz, &temp)) {
-		/* get_temp failed - retry it later */
-		pr_warn("failed to read out thermal zone %d\n", tz->id);
-		goto leave;
-	}
-
-	tz->last_temperature = tz->temperature;
-	tz->temperature = temp;
-
-	for (count = 0; count < tz->trips; count++) {
-		tz->ops->get_trip_type(tz, count, &trip_type);
-		tz->ops->get_trip_temp(tz, count, &trip_temp);
-
-		switch (trip_type) {
-		case THERMAL_TRIP_CRITICAL:
-			if (temp >= trip_temp) {
-				if (tz->ops->notify)
-					ret = tz->ops->notify(tz, count,
-							      trip_type);
-				if (!ret) {
-					pr_emerg("Critical temperature reached (%ld C), shutting down\n",
-						 temp/1000);
-					orderly_poweroff(true);
-				}
-			}
-			break;
-		case THERMAL_TRIP_HOT:
-			if (temp >= trip_temp)
-				if (tz->ops->notify)
-					tz->ops->notify(tz, count, trip_type);
-			break;
-		case THERMAL_TRIP_ACTIVE:
-			thermal_zone_trip_update(tz, count, temp);
-			break;
-		case THERMAL_TRIP_PASSIVE:
-			if (temp >= trip_temp || tz->passive)
-				thermal_zone_trip_update(tz, count, temp);
-			break;
-		}
-	}
-
-	if (tz->forced_passive)
-		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE, temp);
-	thermal_zone_do_update(tz);
-
-leave:
-	if (tz->passive)
-		thermal_zone_device_set_polling(tz, tz->passive_delay);
-	else if (tz->polling_delay)
-		thermal_zone_device_set_polling(tz, tz->polling_delay);
-	else
-		thermal_zone_device_set_polling(tz, 0);
-	mutex_unlock(&tz->lock);
-}
-EXPORT_SYMBOL(thermal_zone_device_update);
-
 static int create_policy_attr(struct thermal_zone_device *tz)
 {
 	sysfs_attr_init(&tz->policy_attr.attr);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 60d2743..3bdf5f2 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -236,12 +236,12 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
 				     unsigned long, unsigned long);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
 				       struct thermal_cooling_device *);
-void thermal_zone_device_update(struct thermal_zone_device *);
 
 struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
 		const struct thermal_cooling_device_ops *);
 void thermal_cooling_device_unregister(struct thermal_cooling_device *);
 
+void thermal_zone_device_update(struct thermal_zone_device *);
 int get_tz_trend(struct thermal_zone_device *, int);
 struct thermal_cooling_device *get_cdev_by_name(const char *);
 struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
-- 
1.7.9.5

Re: [PATCH 10/13] Thermal: Remove throttling logic out of thermal_sys.c

[Zhang Rui]

On 四, 2012-08-09 at 18:16 +0530, Durgadoss R wrote:
> This patch removes the throttling logic out of
> thermal_sys.c; also refactors the code into smaller
> functions so that are easy to read/maintain.
>  * Seperates the handling of critical and non-critical trips
>  * Re-arranges the set_polling and device_check methods, so
>    that all related functions are arranged in one place.
>  * Removes the 'do_update' and 'trip_update' method, as part
>    of moving the throttling logic out of thermal_sys.c
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |  359 ++++++++++++++++-------------------------
>  include/linux/thermal.h       |    2 +-
>  2 files changed, 137 insertions(+), 224 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 748b12f..193d071 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -259,6 +259,142 @@ static void do_binding(struct thermal_zone_device *tz)
>  	}
>  }
>  
> +static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
> +					    int delay)
> +{
> +	cancel_delayed_work(&(tz->poll_queue));
> +
> +	if (!delay)
> +		return;
> +
> +	if (delay > 1000)
> +		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
> +				      round_jiffies(msecs_to_jiffies(delay)));
> +	else
> +		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
> +				      msecs_to_jiffies(delay));
> +}
> +
> +static void monitor_thermal_zone(struct thermal_zone_device *tz)
> +{
> +	mutex_lock(&tz->lock);
> +
> +	if (tz->passive)
> +		thermal_zone_device_set_polling(tz, tz->passive_delay);
> +	else if (tz->polling_delay)
> +		thermal_zone_device_set_polling(tz, tz->polling_delay);
> +	else
> +		thermal_zone_device_set_polling(tz, 0);
> +
> +	mutex_unlock(&tz->lock);
> +}
> +
> +static void notify_user_space(struct thermal_zone_device *tz, int trip)
> +{
> +	mutex_lock(&tz->lock);
> +
> +	kobject_uevent(&tz->device.kobj, KOBJ_CHANGE);
> +
> +	mutex_unlock(&tz->lock);
> +}
> +
> +static void handle_non_critical_trips(struct thermal_zone_device *tz,
> +			int trip, enum thermal_trip_type trip_type)
> +{
> +	int throttle_policy = THERMAL_STEP_WISE;
> +
> +	if (tz->tzp)
> +		throttle_policy = tz->tzp->throttle_policy;
> +
> +	switch (throttle_policy) {
> +	case THERMAL_FAIR_SHARE:
> +		fair_share_throttle(tz, trip);
> +		break;
> +	case THERMAL_STEP_WISE:
> +		step_wise_throttle(tz, trip);
> +		break;
> +	case THERMAL_USER_SPACE:
> +		notify_user_space(tz, trip);
> +		break;
> +	}

this is a little different from what I thought.
IMO, each policy should register its pointer to the thermal framework.
and tz->tzp points to the pointer of the policy using.
and then, in handle_non_critical_trips(), the code would be like this:
if(tz->tzp)
	tz->tzp->throttle(tz, trip);

But this is also okay for now, because we have only one callback for
each policy.

> +}
> +
> +static void handle_critical_trips(struct thermal_zone_device *tz,
> +				int trip, enum thermal_trip_type trip_type)
> +{
> +	long trip_temp;
> +
> +	tz->ops->get_trip_temp(tz, trip, &trip_temp);
> +
> +	/* If we have not crossed the trip_temp, we do not care. */
> +	if (tz->temperature < trip_temp)
> +		return;
> +
> +	if (tz->ops->notify)
> +		tz->ops->notify(tz, trip, trip_type);
> +
> +	if (trip_type == THERMAL_TRIP_CRITICAL) {
> +		pr_emerg("Critical temperature reached(%d C),shutting down\n",
> +			 tz->temperature / 1000);
> +		orderly_poweroff(true);
> +	}
> +}
> +
> +static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
> +{
> +	enum thermal_trip_type type;
> +
> +	tz->ops->get_trip_type(tz, trip, &type);
> +
> +	if (type == THERMAL_TRIP_CRITICAL || type == THERMAL_TRIP_HOT)
> +		handle_critical_trips(tz, trip, type);
> +	else
> +		handle_non_critical_trips(tz, trip, type);
> +	/*
> +	 * Alright, we handled this trip successfully.
> +	 * So, start monitoring again.
> +	 */
> +	monitor_thermal_zone(tz);
> +}
> +
> +static void update_temperature(struct thermal_zone_device *tz)
> +{
> +	long temp;
> +	int ret;
> +
> +	mutex_lock(&tz->lock);
> +
> +	ret = tz->ops->get_temp(tz, &temp);
> +	if (ret) {
> +		pr_warn("failed to read out thermal zone %d\n", tz->id);
> +		return;
> +	}
> +
> +	tz->last_temperature = tz->temperature;
> +	tz->temperature = temp;
> +
> +	mutex_unlock(&tz->lock);
> +}
> +
> +void thermal_zone_device_update(struct thermal_zone_device *tz)
> +{
> +	int count;
> +
> +	update_temperature(tz);
> +
> +	for (count = 0; count < tz->trips; count++)
> +		handle_thermal_trip(tz, count);
> +}
> +EXPORT_SYMBOL(thermal_zone_device_update);
> +
> +static void thermal_zone_device_check(struct work_struct *work)
> +{
> +	struct thermal_zone_device *tz = container_of(work, struct
> +						      thermal_zone_device,
> +						      poll_queue.work);
> +	thermal_zone_device_update(tz);
> +}
> +
>  /* sys I/F for thermal zone */
>  
>  #define to_thermal_zone(_dev) \
> @@ -878,30 +1014,6 @@ thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
>  }
>  #endif
>  
> -static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
> -					    int delay)
> -{
> -	cancel_delayed_work(&(tz->poll_queue));
> -
> -	if (!delay)
> -		return;
> -
> -	if (delay > 1000)
> -		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
> -				      round_jiffies(msecs_to_jiffies(delay)));
> -	else
> -		queue_delayed_work(system_freezable_wq, &(tz->poll_queue),
> -				      msecs_to_jiffies(delay));
> -}
> -
> -static void thermal_zone_device_check(struct work_struct *work)
> -{
> -	struct thermal_zone_device *tz = container_of(work, struct
> -						      thermal_zone_device,
> -						      poll_queue.work);
> -	thermal_zone_device_update(tz);
> -}
> -
>  /**
>   * thermal_zone_bind_cooling_device - bind a cooling device to a thermal zone
>   * @tz:		thermal zone device
> @@ -1197,205 +1309,6 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>  }
>  EXPORT_SYMBOL(thermal_cooling_device_unregister);
>  
> -static void thermal_cdev_do_update(struct thermal_cooling_device *cdev)
> -{
> -	struct thermal_instance *instance;
> -	unsigned long target = 0;
> -
> -	/* cooling device is updated*/
> -	if (cdev->updated)
> -		return;
> -
> -	mutex_lock(&cdev->lock);
> -	/* Make sure cdev enters the deepest cooling state */
> -	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
> -		if (instance->target == THERMAL_NO_TARGET)
> -			continue;
> -		if (instance->target > target)
> -			target = instance->target;
> -	}
> -	mutex_unlock(&cdev->lock);
> -	cdev->ops->set_cur_state(cdev, target);
> -	cdev->updated = true;
> -}
> -
> -static void thermal_zone_do_update(struct thermal_zone_device *tz)
> -{
> -	struct thermal_instance *instance;
> -
> -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> -		thermal_cdev_do_update(instance->cdev);
> -}
> -
> -/*
> - * Cooling algorithm for both active and passive cooling
> - *
> - * 1. if the temperature is higher than a trip point,
> - *    a. if the trend is THERMAL_TREND_RAISING, use higher cooling
> - *       state for this trip point
> - *    b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> - *       state for this trip point
> - *
> - * 2. if the temperature is lower than a trip point, use lower
> - *    cooling state for this trip point
> - *
> - * Note that this behaves the same as the previous passive cooling
> - * algorithm.
> - */
> -
> -static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> -				     int trip, long temp)
> -{
> -	struct thermal_instance *instance;
> -	struct thermal_cooling_device *cdev = NULL;
> -	unsigned long cur_state, max_state;
> -	long trip_temp;
> -	enum thermal_trip_type trip_type;
> -	enum thermal_trend trend;
> -
> -	if (trip == THERMAL_TRIPS_NONE) {
> -		trip_temp = tz->forced_passive;
> -		trip_type = THERMAL_TRIPS_NONE;
> -	} else {
> -		tz->ops->get_trip_temp(tz, trip, &trip_temp);
> -		tz->ops->get_trip_type(tz, trip, &trip_type);
> -	}
> -
> -	if (!tz->ops->get_trend || tz->ops->get_trend(tz, trip, &trend)) {
> -		/*
> -		 * compare the current temperature and previous temperature
> -		 * to get the thermal trend, if no special requirement
> -		 */
> -		if (tz->temperature > tz->last_temperature)
> -			trend = THERMAL_TREND_RAISING;
> -		else if (tz->temperature < tz->last_temperature)
> -			trend = THERMAL_TREND_DROPPING;
> -		else
> -			trend = THERMAL_TREND_STABLE;
> -	}
> -
> -	if (temp >= trip_temp) {
> -		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> -			if (instance->trip != trip)
> -				continue;
> -
> -			cdev = instance->cdev;
> -
> -			cdev->ops->get_cur_state(cdev, &cur_state);
> -			cdev->ops->get_max_state(cdev, &max_state);
> -
> -			if (trend == THERMAL_TREND_RAISING) {
> -				cur_state = cur_state < instance->upper ?
> -					    (cur_state + 1) : instance->upper;
> -			} else if (trend == THERMAL_TREND_DROPPING) {
> -				cur_state = cur_state > instance->lower ?
> -				    (cur_state - 1) : instance->lower;
> -			}
> -
> -			/* activate a passive thermal instance */
> -			if ((trip_type == THERMAL_TRIP_PASSIVE ||
> -			     trip_type == THERMAL_TRIPS_NONE) &&
> -			     instance->target == THERMAL_NO_TARGET)
> -				tz->passive++;
> -
> -			instance->target = cur_state;
> -			cdev->updated = false; /* cooling device needs update */
> -		}
> -	} else {	/* below trip */
> -		list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> -			if (instance->trip != trip)
> -				continue;
> -
> -			/* Do not use the inactive thermal instance */
> -			if (instance->target == THERMAL_NO_TARGET)
> -				continue;
> -			cdev = instance->cdev;
> -			cdev->ops->get_cur_state(cdev, &cur_state);
> -
> -			cur_state = cur_state > instance->lower ?
> -				    (cur_state - 1) : THERMAL_NO_TARGET;
> -
> -			/* deactivate a passive thermal instance */
> -			if ((trip_type == THERMAL_TRIP_PASSIVE ||
> -			     trip_type == THERMAL_TRIPS_NONE) &&
> -			     cur_state == THERMAL_NO_TARGET)
> -				tz->passive--;
> -			instance->target = cur_state;
> -			cdev->updated = false; /* cooling device needs update */
> -		}
> -	}
> -
> -	return;
> -}
> -/**
> - * thermal_zone_device_update - force an update of a thermal zone's state
> - * @ttz:	the thermal zone to update
> - */
> -
> -void thermal_zone_device_update(struct thermal_zone_device *tz)
> -{
> -	int count, ret = 0;
> -	long temp, trip_temp;
> -	enum thermal_trip_type trip_type;
> -
> -	mutex_lock(&tz->lock);
> -
> -	if (tz->ops->get_temp(tz, &temp)) {
> -		/* get_temp failed - retry it later */
> -		pr_warn("failed to read out thermal zone %d\n", tz->id);
> -		goto leave;
> -	}
> -
> -	tz->last_temperature = tz->temperature;
> -	tz->temperature = temp;
> -
> -	for (count = 0; count < tz->trips; count++) {
> -		tz->ops->get_trip_type(tz, count, &trip_type);
> -		tz->ops->get_trip_temp(tz, count, &trip_temp);
> -
> -		switch (trip_type) {
> -		case THERMAL_TRIP_CRITICAL:
> -			if (temp >= trip_temp) {
> -				if (tz->ops->notify)
> -					ret = tz->ops->notify(tz, count,
> -							      trip_type);
> -				if (!ret) {
> -					pr_emerg("Critical temperature reached (%ld C), shutting down\n",
> -						 temp/1000);
> -					orderly_poweroff(true);
> -				}
> -			}
> -			break;
> -		case THERMAL_TRIP_HOT:
> -			if (temp >= trip_temp)
> -				if (tz->ops->notify)
> -					tz->ops->notify(tz, count, trip_type);
> -			break;
> -		case THERMAL_TRIP_ACTIVE:
> -			thermal_zone_trip_update(tz, count, temp);
> -			break;
> -		case THERMAL_TRIP_PASSIVE:
> -			if (temp >= trip_temp || tz->passive)
> -				thermal_zone_trip_update(tz, count, temp);
> -			break;
> -		}
> -	}
> -
> -	if (tz->forced_passive)
> -		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE, temp);
> -	thermal_zone_do_update(tz);
> -
> -leave:
> -	if (tz->passive)
> -		thermal_zone_device_set_polling(tz, tz->passive_delay);
> -	else if (tz->polling_delay)
> -		thermal_zone_device_set_polling(tz, tz->polling_delay);
> -	else
> -		thermal_zone_device_set_polling(tz, 0);
> -	mutex_unlock(&tz->lock);
> -}
> -EXPORT_SYMBOL(thermal_zone_device_update);
> -
>  static int create_policy_attr(struct thermal_zone_device *tz)
>  {
>  	sysfs_attr_init(&tz->policy_attr.attr);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 60d2743..3bdf5f2 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -236,12 +236,12 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
>  				     unsigned long, unsigned long);
>  int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
>  				       struct thermal_cooling_device *);
> -void thermal_zone_device_update(struct thermal_zone_device *);
>  
>  struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
>  		const struct thermal_cooling_device_ops *);
>  void thermal_cooling_device_unregister(struct thermal_cooling_device *);
>  
> +void thermal_zone_device_update(struct thermal_zone_device *);
>  int get_tz_trend(struct thermal_zone_device *, int);
>  struct thermal_cooling_device *get_cdev_by_name(const char *);
>  struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 10/13] Thermal: Remove throttling logic out of thermal_sys.c

[R, Durgadoss]

Hi Rui,

[cut.]
> > +static void notify_user_space(struct thermal_zone_device *tz, int trip)
> > +{
> > +	mutex_lock(&tz->lock);
> > +
> > +	kobject_uevent(&tz->device.kobj, KOBJ_CHANGE);
> > +
> > +	mutex_unlock(&tz->lock);
> > +}
> > +
> > +static void handle_non_critical_trips(struct thermal_zone_device *tz,
> > +			int trip, enum thermal_trip_type trip_type)
> > +{
> > +	int throttle_policy = THERMAL_STEP_WISE;
> > +
> > +	if (tz->tzp)
> > +		throttle_policy = tz->tzp->throttle_policy;
> > +
> > +	switch (throttle_policy) {
> > +	case THERMAL_FAIR_SHARE:
> > +		fair_share_throttle(tz, trip);
> > +		break;
> > +	case THERMAL_STEP_WISE:
> > +		step_wise_throttle(tz, trip);
> > +		break;
> > +	case THERMAL_USER_SPACE:
> > +		notify_user_space(tz, trip);
> > +		break;
> > +	}
> 
> this is a little different from what I thought.
> IMO, each policy should register its pointer to the thermal framework.
> and tz->tzp points to the pointer of the policy using.
> and then, in handle_non_critical_trips(), the code would be like this:
> if(tz->tzp)
> 	tz->tzp->throttle(tz, trip);
> 
> But this is also okay for now, because we have only one callback for
> each policy.

I completely agree with you here. When I was doing my initial development,
I found that some policies need two arguments/ some need three etc..
That’s why could not do a function pointer implementation.

Will try to fix it up in the next version of the patches.

Thank you,
Durga

Re: [PATCH 10/13] Thermal: Remove throttling logic out of thermal_sys.c

[Eduardo Valentin]

Hello,

On Mon, Aug 13, 2012 at 08:04:03AM +0000, R, Durgadoss wrote:
> Hi Rui,
> 
> [cut.]
> > > +static void notify_user_space(struct thermal_zone_device *tz, int trip)
> > > +{
> > > +	mutex_lock(&tz->lock);
> > > +
> > > +	kobject_uevent(&tz->device.kobj, KOBJ_CHANGE);
> > > +
> > > +	mutex_unlock(&tz->lock);
> > > +}
> > > +
> > > +static void handle_non_critical_trips(struct thermal_zone_device *tz,
> > > +			int trip, enum thermal_trip_type trip_type)
> > > +{
> > > +	int throttle_policy = THERMAL_STEP_WISE;
> > > +
> > > +	if (tz->tzp)
> > > +		throttle_policy = tz->tzp->throttle_policy;
> > > +
> > > +	switch (throttle_policy) {
> > > +	case THERMAL_FAIR_SHARE:
> > > +		fair_share_throttle(tz, trip);
> > > +		break;
> > > +	case THERMAL_STEP_WISE:
> > > +		step_wise_throttle(tz, trip);
> > > +		break;
> > > +	case THERMAL_USER_SPACE:
> > > +		notify_user_space(tz, trip);
> > > +		break;
> > > +	}
> > 
> > this is a little different from what I thought.
> > IMO, each policy should register its pointer to the thermal framework.
> > and tz->tzp points to the pointer of the policy using.
> > and then, in handle_non_critical_trips(), the code would be like this:
> > if(tz->tzp)
> > 	tz->tzp->throttle(tz, trip);
> > 
> > But this is also okay for now, because we have only one callback for
> > each policy.
> 
> I completely agree with you here. When I was doing my initial development,
> I found that some policies need two arguments/ some need three etc..
> That’s why could not do a function pointer implementation.
> 
> Will try to fix it up in the next version of the patches.

Cool! I'd also prefer to use the ->ops type of design, if possible.

> 
> Thank you,
> Durga
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/13] Thermal: Add a notification API

[Durgadoss R]

This patch adds a notification API which the sensor drivers'
can use to notify the framework. The framework then takes
care of the throttling according to the configured policy.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/thermal/thermal_sys.c |   18 ++++++++++++++++++
 include/linux/thermal.h       |    1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 193d071..6931d81 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1309,6 +1309,24 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
 }
 EXPORT_SYMBOL(thermal_cooling_device_unregister);
 
+/**
+ * notify_thermal_framework - Sensor drivers use this API to notify framework
+ * @tz:		thermal zone device
+ * @trip:	indicates which trip point has been crossed
+ *
+ * This function handles the trip events from sensor drivers. It starts
+ * throttling the cooling devices according to the policy configured.
+ * For CRITICAL and HOT trip points, this notifies the respective drivers,
+ * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
+ * The throttling policy is based on the configured platform data; if no
+ * platform data is provided, this uses the step_wise throttling policy.
+ */
+void notify_thermal_framework(struct thermal_zone_device *tz, int trip)
+{
+	handle_thermal_trip(tz, trip);
+}
+EXPORT_SYMBOL(notify_thermal_framework);
+
 static int create_policy_attr(struct thermal_zone_device *tz)
 {
 	sysfs_attr_init(&tz->policy_attr.attr);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 3bdf5f2..a3a0144 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -246,6 +246,7 @@ int get_tz_trend(struct thermal_zone_device *, int);
 struct thermal_cooling_device *get_cdev_by_name(const char *);
 struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
 		struct thermal_cooling_device *, int);
+void notify_thermal_framework(struct thermal_zone_device *, int);
 
 /*
  * The platform layer shall define a 'function' that provides the
-- 
1.7.9.5

Re: [PATCH 11/13] Thermal: Add a notification API

[Zhang Rui]

On 四, 2012-08-09 at 18:16 +0530, Durgadoss R wrote:
> This patch adds a notification API which the sensor drivers'
> can use to notify the framework. The framework then takes
> care of the throttling according to the configured policy.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/thermal/thermal_sys.c |   18 ++++++++++++++++++
>  include/linux/thermal.h       |    1 +
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 193d071..6931d81 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -1309,6 +1309,24 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>  }
>  EXPORT_SYMBOL(thermal_cooling_device_unregister);
>  
> +/**
> + * notify_thermal_framework - Sensor drivers use this API to notify framework
> + * @tz:		thermal zone device
> + * @trip:	indicates which trip point has been crossed
> + *
> + * This function handles the trip events from sensor drivers. It starts
> + * throttling the cooling devices according to the policy configured.
> + * For CRITICAL and HOT trip points, this notifies the respective drivers,
> + * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
> + * The throttling policy is based on the configured platform data; if no
> + * platform data is provided, this uses the step_wise throttling policy.
> + */
> +void notify_thermal_framework(struct thermal_zone_device *tz, int trip)
> +{
> +	handle_thermal_trip(tz, trip);
> +}
> +EXPORT_SYMBOL(notify_thermal_framework);
> +
why thermal_zone_device_update() can not work here?

thanks,
rui
>  static int create_policy_attr(struct thermal_zone_device *tz)
>  {
>  	sysfs_attr_init(&tz->policy_attr.attr);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 3bdf5f2..a3a0144 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -246,6 +246,7 @@ int get_tz_trend(struct thermal_zone_device *, int);
>  struct thermal_cooling_device *get_cdev_by_name(const char *);
>  struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
>  		struct thermal_cooling_device *, int);
> +void notify_thermal_framework(struct thermal_zone_device *, int);
>  
>  /*
>   * The platform layer shall define a 'function' that provides the


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 11/13] Thermal: Add a notification API

[R, Durgadoss]

Hi Rui,


> -----Original Message-----
> From: Zhang, Rui
> Sent: Monday, August 13, 2012 12:33 PM
> To: R, Durgadoss
> Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> wni@nvidia.com
> Subject: Re: [PATCH 11/13] Thermal: Add a notification API
> 
> On 四, 2012-08-09 at 18:16 +0530, Durgadoss R wrote:
> > This patch adds a notification API which the sensor drivers'
> > can use to notify the framework. The framework then takes
> > care of the throttling according to the configured policy.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  drivers/thermal/thermal_sys.c |   18 ++++++++++++++++++
> >  include/linux/thermal.h       |    1 +
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 193d071..6931d81 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -1309,6 +1309,24 @@ void thermal_cooling_device_unregister(struct
> thermal_cooling_device *cdev)
> >  }
> >  EXPORT_SYMBOL(thermal_cooling_device_unregister);
> >
> > +/**
> > + * notify_thermal_framework - Sensor drivers use this API to notify
> framework
> > + * @tz:		thermal zone device
> > + * @trip:	indicates which trip point has been crossed
> > + *
> > + * This function handles the trip events from sensor drivers. It starts
> > + * throttling the cooling devices according to the policy configured.
> > + * For CRITICAL and HOT trip points, this notifies the respective drivers,
> > + * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
> > + * The throttling policy is based on the configured platform data; if no
> > + * platform data is provided, this uses the step_wise throttling policy.
> > + */
> > +void notify_thermal_framework(struct thermal_zone_device *tz, int trip)
> > +{
> > +	handle_thermal_trip(tz, trip);
> > +}
> > +EXPORT_SYMBOL(notify_thermal_framework);
> > +
> why thermal_zone_device_update() can not work here?

It can work. But thermal_zone_device_update() goes over the
list of all trip points whereas here we know 'trip' is the trip point
that we have to handle now. So, why do we need to iterate over all trip
points again ?
That’s why it is implemented this way.

Thanks,
Durga

Re: [PATCH 11/13] Thermal: Add a notification API

[Eduardo Valentin]

Hello,

On Mon, Aug 13, 2012 at 07:46:20AM +0000, R, Durgadoss wrote:
> Hi Rui,
> 
> 
> > -----Original Message-----
> > From: Zhang, Rui
> > Sent: Monday, August 13, 2012 12:33 PM
> > To: R, Durgadoss
> > Cc: lenb@kernel.org; rjw@sisk.pl; linux-acpi@vger.kernel.org; linux-
> > pm@vger.kernel.org; eduardo.valentin@ti.com; amit.kachhap@linaro.org;
> > wni@nvidia.com
> > Subject: Re: [PATCH 11/13] Thermal: Add a notification API
> > 
> > On 四, 2012-08-09 at 18:16 +0530, Durgadoss R wrote:
> > > This patch adds a notification API which the sensor drivers'
> > > can use to notify the framework. The framework then takes
> > > care of the throttling according to the configured policy.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > ---
> > >  drivers/thermal/thermal_sys.c |   18 ++++++++++++++++++
> > >  include/linux/thermal.h       |    1 +
> > >  2 files changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > > index 193d071..6931d81 100644
> > > --- a/drivers/thermal/thermal_sys.c
> > > +++ b/drivers/thermal/thermal_sys.c
> > > @@ -1309,6 +1309,24 @@ void thermal_cooling_device_unregister(struct
> > thermal_cooling_device *cdev)
> > >  }
> > >  EXPORT_SYMBOL(thermal_cooling_device_unregister);
> > >
> > > +/**
> > > + * notify_thermal_framework - Sensor drivers use this API to notify
> > framework
> > > + * @tz:		thermal zone device
> > > + * @trip:	indicates which trip point has been crossed
> > > + *
> > > + * This function handles the trip events from sensor drivers. It starts
> > > + * throttling the cooling devices according to the policy configured.
> > > + * For CRITICAL and HOT trip points, this notifies the respective drivers,
> > > + * and does actual throttling for other trip points i.e ACTIVE and PASSIVE.
> > > + * The throttling policy is based on the configured platform data; if no
> > > + * platform data is provided, this uses the step_wise throttling policy.
> > > + */
> > > +void notify_thermal_framework(struct thermal_zone_device *tz, int trip)
> > > +{
> > > +	handle_thermal_trip(tz, trip);
> > > +}
> > > +EXPORT_SYMBOL(notify_thermal_framework);
> > > +
> > why thermal_zone_device_update() can not work here?
> 
> It can work. But thermal_zone_device_update() goes over the
> list of all trip points whereas here we know 'trip' is the trip point
> that we have to handle now. So, why do we need to iterate over all trip
> points again ?
> That’s why it is implemented this way.

I see. But there is no example of usage of this API on this series?

> 
> Thanks,
> Durga
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/13] Thermal: Add documentation for platform layer data

[Durgadoss R]

This patch adds documentation for the structure
thermal_zone_params, and also shows an example of
how to populate them.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 Documentation/thermal/sysfs-api.txt |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index ca1a1a3..669720c 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -112,6 +112,36 @@ temperature) and throttle appropriate devices.
     trip: indicates which trip point the cooling devices is associated with
 	  in this thermal zone.
 
+1.4 Thermal Zone Parameters
+1.4.1 struct thermal_zone_params
+    This structure defines the platform level parameters for a thermal zone.
+    This data, for each thermal zone should come from the platform layer.
+    This is an optional feature where some platforms can choose not to
+    provide this data.
+1.4.2 struct thermal_zone_params attributes
+    .thermal_zone_name: Name of the thermal zone, for which these parameters
+			are being defined.
+    .num_cdevs: Number of cooling devices associated with this
+			  thermal zone.
+    .cdevs_name: Names of the cooling devices associated with this
+			   thermal zone.
+    .weights: This parameter defines the 'influence' of a particular cooling
+	      device on this thermal zone, on a percentage scale. The sum of
+	      all these weights cannot exceed 100. The order of values in
+	      this array should match with that of the cdevs_name.
+    .trip_mask: This is a bit mask that gives the binding relation between
+		this thermal zone and cdev, for a particular trip point.
+		If nth bit is set, then the cdev and thermal zone are bound
+		for trip point n.
+1.4.3 An example thermal_zone_params structure
+	struct thermal_zone_params tzp = {
+                .thermal_zone_name = "CPU",
+                .num_cdevs = 2,
+                .cdevs_name = {"CPU", "Memory"},
+                .weights = {70, 30},
+		.trip_mask = {0x0F, 0x08},
+        };
+
 2. sysfs attributes structure
 
 RO	read only value
-- 
1.7.9.5

Re: [PATCH 12/13] Thermal: Add documentation for platform layer data

[Eduardo Valentin]

hello,

On Thu, Aug 09, 2012 at 06:16:04PM +0530, Durgadoss R wrote:
> This patch adds documentation for the structure
> thermal_zone_params, and also shows an example of
> how to populate them.

Just a reminder, I believe you should also expose some documentation about
the new functions exported in this series.

> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  Documentation/thermal/sysfs-api.txt |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index ca1a1a3..669720c 100644
> --- a/Documentation/thermal/sysfs-api.txt
> +++ b/Documentation/thermal/sysfs-api.txt
> @@ -112,6 +112,36 @@ temperature) and throttle appropriate devices.
>      trip: indicates which trip point the cooling devices is associated with
>  	  in this thermal zone.
>  
> +1.4 Thermal Zone Parameters
> +1.4.1 struct thermal_zone_params
> +    This structure defines the platform level parameters for a thermal zone.
> +    This data, for each thermal zone should come from the platform layer.
> +    This is an optional feature where some platforms can choose not to
> +    provide this data.
> +1.4.2 struct thermal_zone_params attributes
> +    .thermal_zone_name: Name of the thermal zone, for which these parameters
> +			are being defined.
> +    .num_cdevs: Number of cooling devices associated with this
> +			  thermal zone.
> +    .cdevs_name: Names of the cooling devices associated with this
> +			   thermal zone.
> +    .weights: This parameter defines the 'influence' of a particular cooling
> +	      device on this thermal zone, on a percentage scale. The sum of
> +	      all these weights cannot exceed 100. The order of values in
> +	      this array should match with that of the cdevs_name.
> +    .trip_mask: This is a bit mask that gives the binding relation between
> +		this thermal zone and cdev, for a particular trip point.
> +		If nth bit is set, then the cdev and thermal zone are bound
> +		for trip point n.
> +1.4.3 An example thermal_zone_params structure
> +	struct thermal_zone_params tzp = {
> +                .thermal_zone_name = "CPU",
> +                .num_cdevs = 2,
> +                .cdevs_name = {"CPU", "Memory"},
> +                .weights = {70, 30},
> +		.trip_mask = {0x0F, 0x08},
> +        };
> +
>  2. sysfs attributes structure
>  
>  RO	read only value
> -- 
> 1.7.9.5
> 

RE: [PATCH 12/13] Thermal: Add documentation for platform layer data

[R, Durgadoss]

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> Sent: Tuesday, August 21, 2012 11:09 AM
> To: R, Durgadoss
> Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> amit.kachhap@linaro.org; wni@nvidia.com
> Subject: Re: [PATCH 12/13] Thermal: Add documentation for platform layer
> data
> 
> hello,
> 
> On Thu, Aug 09, 2012 at 06:16:04PM +0530, Durgadoss R wrote:
> > This patch adds documentation for the structure
> > thermal_zone_params, and also shows an example of
> > how to populate them.
> 
> Just a reminder, I believe you should also expose some documentation
> about
> the new functions exported in this series.

Thanks for the catch :-)
Will add in v2.
> 
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  Documentation/thermal/sysfs-api.txt |   30
> ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/Documentation/thermal/sysfs-api.txt
> b/Documentation/thermal/sysfs-api.txt
> > index ca1a1a3..669720c 100644
> > --- a/Documentation/thermal/sysfs-api.txt
> > +++ b/Documentation/thermal/sysfs-api.txt
> > @@ -112,6 +112,36 @@ temperature) and throttle appropriate devices.
> >      trip: indicates which trip point the cooling devices is associated with
> >  	  in this thermal zone.
> >
> > +1.4 Thermal Zone Parameters
> > +1.4.1 struct thermal_zone_params
> > +    This structure defines the platform level parameters for a thermal zone.
> > +    This data, for each thermal zone should come from the platform layer.
> > +    This is an optional feature where some platforms can choose not to
> > +    provide this data.
> > +1.4.2 struct thermal_zone_params attributes
> > +    .thermal_zone_name: Name of the thermal zone, for which these
> parameters
> > +			are being defined.
> > +    .num_cdevs: Number of cooling devices associated with this
> > +			  thermal zone.
> > +    .cdevs_name: Names of the cooling devices associated with this
> > +			   thermal zone.
> > +    .weights: This parameter defines the 'influence' of a particular cooling
> > +	      device on this thermal zone, on a percentage scale. The sum of
> > +	      all these weights cannot exceed 100. The order of values in
> > +	      this array should match with that of the cdevs_name.
> > +    .trip_mask: This is a bit mask that gives the binding relation between
> > +		this thermal zone and cdev, for a particular trip point.
> > +		If nth bit is set, then the cdev and thermal zone are bound
> > +		for trip point n.
> > +1.4.3 An example thermal_zone_params structure
> > +	struct thermal_zone_params tzp = {
> > +                .thermal_zone_name = "CPU",
> > +                .num_cdevs = 2,
> > +                .cdevs_name = {"CPU", "Memory"},
> > +                .weights = {70, 30},
> > +		.trip_mask = {0x0F, 0x08},
> > +        };
> > +
> >  2. sysfs attributes structure
> >
> >  RO	read only value
> > --
> > 1.7.9.5
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[Durgadoss R]

This patch shows how can we add platform specific thermal data
required by the thermal framework. This is just an example
patch, and _not_ for merge.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 arch/x86/platform/mrst/mrst.c |   42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/x86/platform/mrst/mrst.c b/arch/x86/platform/mrst/mrst.c
index fd41a92..0440db5 100644
--- a/arch/x86/platform/mrst/mrst.c
+++ b/arch/x86/platform/mrst/mrst.c
@@ -30,6 +30,7 @@
 #include <linux/mfd/intel_msic.h>
 #include <linux/gpio.h>
 #include <linux/i2c/tc35876x.h>
+#include <linux/thermal.h>
 
 #include <asm/setup.h>
 #include <asm/mpspec_def.h>
@@ -78,6 +79,30 @@ struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];
 EXPORT_SYMBOL_GPL(sfi_mrtc_array);
 int sfi_mrtc_num;
 
+#define MRST_THERMAL_ZONES	3
+struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
+	{ .thermal_zone_name = "CPU",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 2,
+	.cdevs_name = {"CPU", "Battery"},
+	.trip_mask = {0x0F, 0x08},
+	.weights = {80, 20}, },
+
+	{ .thermal_zone_name = "Battery",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 1,
+	.cdevs_name = {"Battery"},
+	.trip_mask = {0x0F},
+	.weights = {100}, },
+
+	{ .thermal_zone_name = "Skin",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 2,
+	.cdevs_name = {"Display", "Battery"},
+	.trip_mask = {0x0F, 0x0F},
+	.weights = {50, 50}, }
+};
+
 static void mrst_power_off(void)
 {
 }
@@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct sfi_table_header *table)
 	return 0;
 }
 
+static int mrst_get_thermal_params(struct thermal_zone_device *tz)
+{
+	int i;
+
+	for (i = 0; i < MRST_THERMAL_ZONES; i++) {
+		if (!strcmp(tzp[i].thermal_zone_name, tz->type)) {
+			tz->tzp = &tzp[i];
+			return 0;
+		}
+	}
+	return -ENODEV;
+}
+
 static int __init mrst_platform_init(void)
 {
 	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
 	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
+
+	/* Set platform thermal data pointer */
+	get_platform_thermal_params = mrst_get_thermal_params;
+
 	return 0;
 }
 arch_initcall(mrst_platform_init);
-- 
1.7.9.5

Re: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[Eduardo Valentin]

Hello,

On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> This patch shows how can we add platform specific thermal data
> required by the thermal framework. This is just an example
> patch, and _not_ for merge.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  arch/x86/platform/mrst/mrst.c |   42 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/arch/x86/platform/mrst/mrst.c b/arch/x86/platform/mrst/mrst.c
> index fd41a92..0440db5 100644
> --- a/arch/x86/platform/mrst/mrst.c
> +++ b/arch/x86/platform/mrst/mrst.c
> @@ -30,6 +30,7 @@
>  #include <linux/mfd/intel_msic.h>
>  #include <linux/gpio.h>
>  #include <linux/i2c/tc35876x.h>
> +#include <linux/thermal.h>
>  
>  #include <asm/setup.h>
>  #include <asm/mpspec_def.h>
> @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry sfi_mrtc_array[SFI_MRTC_MAX];
>  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
>  int sfi_mrtc_num;
>  
> +#define MRST_THERMAL_ZONES	3
> +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> +	{ .thermal_zone_name = "CPU",
> +	.throttle_policy = THERMAL_FAIR_SHARE,
> +	.num_cdevs = 2,
> +	.cdevs_name = {"CPU", "Battery"},
> +	.trip_mask = {0x0F, 0x08},
> +	.weights = {80, 20}, },
> +
> +	{ .thermal_zone_name = "Battery",
> +	.throttle_policy = THERMAL_FAIR_SHARE,
> +	.num_cdevs = 1,
> +	.cdevs_name = {"Battery"},
> +	.trip_mask = {0x0F},
> +	.weights = {100}, },
> +
> +	{ .thermal_zone_name = "Skin",
> +	.throttle_policy = THERMAL_FAIR_SHARE,
> +	.num_cdevs = 2,
> +	.cdevs_name = {"Display", "Battery"},
> +	.trip_mask = {0x0F, 0x0F},
> +	.weights = {50, 50}, }

Please consider the comment I sent on your data definition and also the comment I made on this patch on your RFC series.

> +};
> +
>  static void mrst_power_off(void)
>  {
>  }
> @@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct sfi_table_header *table)
>  	return 0;
>  }
>  
> +static int mrst_get_thermal_params(struct thermal_zone_device *tz)
> +{
> +	int i;
> +
> +	for (i = 0; i < MRST_THERMAL_ZONES; i++) {
> +		if (!strcmp(tzp[i].thermal_zone_name, tz->type)) {
> +			tz->tzp = &tzp[i];
> +			return 0;
> +		}
> +	}
> +	return -ENODEV;
> +}
> +
>  static int __init mrst_platform_init(void)
>  {
>  	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
>  	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
> +
> +	/* Set platform thermal data pointer */
> +	get_platform_thermal_params = mrst_get_thermal_params;
> +
>  	return 0;
>  }
>  arch_initcall(mrst_platform_init);
> -- 
> 1.7.9.5
> 

RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[R, Durgadoss]

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> Sent: Tuesday, August 21, 2012 11:10 AM
> To: R, Durgadoss
> Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> amit.kachhap@linaro.org; wni@nvidia.com
> Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to provide
> thermal data
> 
> Hello,
> 
> On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > This patch shows how can we add platform specific thermal data
> > required by the thermal framework. This is just an example
> > patch, and _not_ for merge.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  arch/x86/platform/mrst/mrst.c |   42
> +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/arch/x86/platform/mrst/mrst.c
> b/arch/x86/platform/mrst/mrst.c
> > index fd41a92..0440db5 100644
> > --- a/arch/x86/platform/mrst/mrst.c
> > +++ b/arch/x86/platform/mrst/mrst.c
> > @@ -30,6 +30,7 @@
> >  #include <linux/mfd/intel_msic.h>
> >  #include <linux/gpio.h>
> >  #include <linux/i2c/tc35876x.h>
> > +#include <linux/thermal.h>
> >
> >  #include <asm/setup.h>
> >  #include <asm/mpspec_def.h>
> > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> sfi_mrtc_array[SFI_MRTC_MAX];
> >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> >  int sfi_mrtc_num;
> >
> > +#define MRST_THERMAL_ZONES	3
> > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > +	{ .thermal_zone_name = "CPU",
> > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > +	.num_cdevs = 2,
> > +	.cdevs_name = {"CPU", "Battery"},
> > +	.trip_mask = {0x0F, 0x08},
> > +	.weights = {80, 20}, },
> > +
> > +	{ .thermal_zone_name = "Battery",
> > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > +	.num_cdevs = 1,
> > +	.cdevs_name = {"Battery"},
> > +	.trip_mask = {0x0F},
> > +	.weights = {100}, },
> > +
> > +	{ .thermal_zone_name = "Skin",
> > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > +	.num_cdevs = 2,
> > +	.cdevs_name = {"Display", "Battery"},
> > +	.trip_mask = {0x0F, 0x0F},
> > +	.weights = {50, 50}, }
> 
> Please consider the comment I sent on your data definition and also the
> comment I made on this patch on your RFC series.

Yes.. I don't know why/how I missed it.
Also, saw the same comment on one of the other patches also.

Will surely fix this thing in v2.

BTW, any suggestion for the 'name' of that structure ? :-)
Thanks,
Durga

> 
> > +};
> > +
> >  static void mrst_power_off(void)
> >  {
> >  }
> > @@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct
> sfi_table_header *table)
> >  	return 0;
> >  }
> >
> > +static int mrst_get_thermal_params(struct thermal_zone_device *tz)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MRST_THERMAL_ZONES; i++) {
> > +		if (!strcmp(tzp[i].thermal_zone_name, tz->type)) {
> > +			tz->tzp = &tzp[i];
> > +			return 0;
> > +		}
> > +	}
> > +	return -ENODEV;
> > +}
> > +
> >  static int __init mrst_platform_init(void)
> >  {
> >  	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
> >  	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
> > +
> > +	/* Set platform thermal data pointer */
> > +	get_platform_thermal_params = mrst_get_thermal_params;
> > +
> >  	return 0;
> >  }
> >  arch_initcall(mrst_platform_init);
> > --
> > 1.7.9.5
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[Zhang Rui]

On 一, 2012-08-20 at 23:52 -0600, R, Durgadoss wrote:
> 
> > -----Original Message-----
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> > Sent: Tuesday, August 21, 2012 11:10 AM
> > To: R, Durgadoss
> > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > amit.kachhap@linaro.org; wni@nvidia.com
> > Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to provide
> > thermal data
> > 
> > Hello,
> > 
> > On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > > This patch shows how can we add platform specific thermal data
> > > required by the thermal framework. This is just an example
> > > patch, and _not_ for merge.
> > >
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > ---
> > >  arch/x86/platform/mrst/mrst.c |   42
> > +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >
> > > diff --git a/arch/x86/platform/mrst/mrst.c
> > b/arch/x86/platform/mrst/mrst.c
> > > index fd41a92..0440db5 100644
> > > --- a/arch/x86/platform/mrst/mrst.c
> > > +++ b/arch/x86/platform/mrst/mrst.c
> > > @@ -30,6 +30,7 @@
> > >  #include <linux/mfd/intel_msic.h>
> > >  #include <linux/gpio.h>
> > >  #include <linux/i2c/tc35876x.h>
> > > +#include <linux/thermal.h>
> > >
> > >  #include <asm/setup.h>
> > >  #include <asm/mpspec_def.h>
> > > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> > sfi_mrtc_array[SFI_MRTC_MAX];
> > >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> > >  int sfi_mrtc_num;
> > >
> > > +#define MRST_THERMAL_ZONES	3
> > > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > > +	{ .thermal_zone_name = "CPU",
> > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > +	.num_cdevs = 2,
> > > +	.cdevs_name = {"CPU", "Battery"},
> > > +	.trip_mask = {0x0F, 0x08},
> > > +	.weights = {80, 20}, },
> > > +
> > > +	{ .thermal_zone_name = "Battery",
> > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > +	.num_cdevs = 1,
> > > +	.cdevs_name = {"Battery"},
> > > +	.trip_mask = {0x0F},
> > > +	.weights = {100}, },
> > > +
> > > +	{ .thermal_zone_name = "Skin",
> > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > +	.num_cdevs = 2,
> > > +	.cdevs_name = {"Display", "Battery"},
> > > +	.trip_mask = {0x0F, 0x0F},
> > > +	.weights = {50, 50}, }
> > 
> > Please consider the comment I sent on your data definition and also the
> > comment I made on this patch on your RFC series.
> 
> Yes.. I don't know why/how I missed it.
> Also, saw the same comment on one of the other patches also.
> 
> Will surely fix this thing in v2.
> 
> BTW, any suggestion for the 'name' of that structure ? :-)

hmmm,
do we still have thermal_zone_platforms in patch v2?
I do not think we need this if we only bind devices via .bind()
callback.

> Thanks,
> Durga
> 
> > 
> > > +};
> > > +
> > >  static void mrst_power_off(void)
> > >  {
> > >  }
> > > @@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct
> > sfi_table_header *table)
> > >  	return 0;
> > >  }
> > >
> > > +static int mrst_get_thermal_params(struct thermal_zone_device *tz)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < MRST_THERMAL_ZONES; i++) {
> > > +		if (!strcmp(tzp[i].thermal_zone_name, tz->type)) {
> > > +			tz->tzp = &tzp[i];
> > > +			return 0;
> > > +		}
> > > +	}
> > > +	return -ENODEV;
> > > +}
> > > +
> > >  static int __init mrst_platform_init(void)
> > >  {
> > >  	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
> > >  	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
> > > +
> > > +	/* Set platform thermal data pointer */
> > > +	get_platform_thermal_params = mrst_get_thermal_params;
> > > +
> > >  	return 0;
> > >  }
> > >  arch_initcall(mrst_platform_init);
> > > --
> > > 1.7.9.5
> > >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[R, Durgadoss]

Hi Rui,

> > > -----Original Message-----
> > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > > owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> > > Sent: Tuesday, August 21, 2012 11:10 AM
> > > To: R, Durgadoss
> > > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> > > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > > amit.kachhap@linaro.org; wni@nvidia.com
> > > Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to provide
> > > thermal data
> > >
> > > Hello,
> > >
> > > On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > > > This patch shows how can we add platform specific thermal data
> > > > required by the thermal framework. This is just an example
> > > > patch, and _not_ for merge.
> > > >
> > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > > ---
> > > >  arch/x86/platform/mrst/mrst.c |   42
> > > +++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/arch/x86/platform/mrst/mrst.c
> > > b/arch/x86/platform/mrst/mrst.c
> > > > index fd41a92..0440db5 100644
> > > > --- a/arch/x86/platform/mrst/mrst.c
> > > > +++ b/arch/x86/platform/mrst/mrst.c
> > > > @@ -30,6 +30,7 @@
> > > >  #include <linux/mfd/intel_msic.h>
> > > >  #include <linux/gpio.h>
> > > >  #include <linux/i2c/tc35876x.h>
> > > > +#include <linux/thermal.h>
> > > >
> > > >  #include <asm/setup.h>
> > > >  #include <asm/mpspec_def.h>
> > > > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> > > sfi_mrtc_array[SFI_MRTC_MAX];
> > > >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> > > >  int sfi_mrtc_num;
> > > >
> > > > +#define MRST_THERMAL_ZONES	3
> > > > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > > > +	{ .thermal_zone_name = "CPU",
> > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > +	.num_cdevs = 2,
> > > > +	.cdevs_name = {"CPU", "Battery"},
> > > > +	.trip_mask = {0x0F, 0x08},
> > > > +	.weights = {80, 20}, },
> > > > +
> > > > +	{ .thermal_zone_name = "Battery",
> > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > +	.num_cdevs = 1,
> > > > +	.cdevs_name = {"Battery"},
> > > > +	.trip_mask = {0x0F},
> > > > +	.weights = {100}, },
> > > > +
> > > > +	{ .thermal_zone_name = "Skin",
> > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > +	.num_cdevs = 2,
> > > > +	.cdevs_name = {"Display", "Battery"},
> > > > +	.trip_mask = {0x0F, 0x0F},
> > > > +	.weights = {50, 50}, }
> > >
> > > Please consider the comment I sent on your data definition and also the
> > > comment I made on this patch on your RFC series.
> >
> > Yes.. I don't know why/how I missed it.
> > Also, saw the same comment on one of the other patches also.
> >
> > Will surely fix this thing in v2.
> >
> > BTW, any suggestion for the 'name' of that structure ? :-)
> 
> hmmm,
> do we still have thermal_zone_platforms in patch v2?
> I do not think we need this if we only bind devices via .bind()
> callback.

We can bind devices via .bind call back, and that will take some load
off the framework code. But even then, we would need this structure
right ? Say, when we obtain platform data from a thermal driver, it
should know 'what format the platform data is' ..correct ?

I theoretically agree with you that individual platform drivers can
have data in their own format, but that will be a heavy loss on
standardization.

So,
I will remove the extra bind code I added to framework, and
(keep it the old way it was) but still prefer to have the structure
put in thermal.h.

Thanks,
Durga

RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[Zhang Rui]

On 二, 2012-08-21 at 00:41 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> > > > -----Original Message-----
> > > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > > > owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> > > > Sent: Tuesday, August 21, 2012 11:10 AM
> > > > To: R, Durgadoss
> > > > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> > > > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > > > amit.kachhap@linaro.org; wni@nvidia.com
> > > > Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to provide
> > > > thermal data
> > > >
> > > > Hello,
> > > >
> > > > On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > > > > This patch shows how can we add platform specific thermal data
> > > > > required by the thermal framework. This is just an example
> > > > > patch, and _not_ for merge.
> > > > >
> > > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > > > ---
> > > > >  arch/x86/platform/mrst/mrst.c |   42
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 42 insertions(+)
> > > > >
> > > > > diff --git a/arch/x86/platform/mrst/mrst.c
> > > > b/arch/x86/platform/mrst/mrst.c
> > > > > index fd41a92..0440db5 100644
> > > > > --- a/arch/x86/platform/mrst/mrst.c
> > > > > +++ b/arch/x86/platform/mrst/mrst.c
> > > > > @@ -30,6 +30,7 @@
> > > > >  #include <linux/mfd/intel_msic.h>
> > > > >  #include <linux/gpio.h>
> > > > >  #include <linux/i2c/tc35876x.h>
> > > > > +#include <linux/thermal.h>
> > > > >
> > > > >  #include <asm/setup.h>
> > > > >  #include <asm/mpspec_def.h>
> > > > > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> > > > sfi_mrtc_array[SFI_MRTC_MAX];
> > > > >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> > > > >  int sfi_mrtc_num;
> > > > >
> > > > > +#define MRST_THERMAL_ZONES	3
> > > > > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > > > > +	{ .thermal_zone_name = "CPU",
> > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > +	.num_cdevs = 2,
> > > > > +	.cdevs_name = {"CPU", "Battery"},
> > > > > +	.trip_mask = {0x0F, 0x08},
> > > > > +	.weights = {80, 20}, },
> > > > > +
> > > > > +	{ .thermal_zone_name = "Battery",
> > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > +	.num_cdevs = 1,
> > > > > +	.cdevs_name = {"Battery"},
> > > > > +	.trip_mask = {0x0F},
> > > > > +	.weights = {100}, },
> > > > > +
> > > > > +	{ .thermal_zone_name = "Skin",
> > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > +	.num_cdevs = 2,
> > > > > +	.cdevs_name = {"Display", "Battery"},
> > > > > +	.trip_mask = {0x0F, 0x0F},
> > > > > +	.weights = {50, 50}, }
> > > >
> > > > Please consider the comment I sent on your data definition and also the
> > > > comment I made on this patch on your RFC series.
> > >
> > > Yes.. I don't know why/how I missed it.
> > > Also, saw the same comment on one of the other patches also.
> > >
> > > Will surely fix this thing in v2.
> > >
> > > BTW, any suggestion for the 'name' of that structure ? :-)
> > 
> > hmmm,
> > do we still have thermal_zone_platforms in patch v2?
> > I do not think we need this if we only bind devices via .bind()
> > callback.
> 
> We can bind devices via .bind call back, and that will take some load
> off the framework code. But even then, we would need this structure
> right ?
why?
I'd prefer introduce something like this,
struct thermal_bind_params {
	int trip;
	unsigned long upper;
	unsinged long lower;
	int weight;
	int sample_period;
}

and use thermal_zone_bind_cooling_device(tz, cdev, thermal_bind_params),
throttle_policy should be set when invoking
thermal_zone_device_register.

is there any information in thermal_zone_params can not be convert to
thermal_bind_params?

thanks,
rui

>  Say, when we obtain platform data from a thermal driver, it
> should know 'what format the platform data is' ..correct ?
> 
> I theoretically agree with you that individual platform drivers can
> have data in their own format, but that will be a heavy loss on
> standardization.
> 
> So,
> I will remove the extra bind code I added to framework, and
> (keep it the old way it was) but still prefer to have the structure
> put in thermal.h.
> 
> Thanks,
> Durga


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[Eduardo Valentin]

Hello,

On Tue, Aug 21, 2012 at 02:52:34PM +0800, Zhang Rui wrote:
> On 二, 2012-08-21 at 00:41 -0600, R, Durgadoss wrote:
> > Hi Rui,
> > 
> > > > > -----Original Message-----
> > > > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > > > > owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> > > > > Sent: Tuesday, August 21, 2012 11:10 AM
> > > > > To: R, Durgadoss
> > > > > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> > > > > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > > > > amit.kachhap@linaro.org; wni@nvidia.com
> > > > > Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to provide
> > > > > thermal data
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > > > > > This patch shows how can we add platform specific thermal data
> > > > > > required by the thermal framework. This is just an example
> > > > > > patch, and _not_ for merge.
> > > > > >
> > > > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > > > > ---
> > > > > >  arch/x86/platform/mrst/mrst.c |   42
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 42 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/x86/platform/mrst/mrst.c
> > > > > b/arch/x86/platform/mrst/mrst.c
> > > > > > index fd41a92..0440db5 100644
> > > > > > --- a/arch/x86/platform/mrst/mrst.c
> > > > > > +++ b/arch/x86/platform/mrst/mrst.c
> > > > > > @@ -30,6 +30,7 @@
> > > > > >  #include <linux/mfd/intel_msic.h>
> > > > > >  #include <linux/gpio.h>
> > > > > >  #include <linux/i2c/tc35876x.h>
> > > > > > +#include <linux/thermal.h>
> > > > > >
> > > > > >  #include <asm/setup.h>
> > > > > >  #include <asm/mpspec_def.h>
> > > > > > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> > > > > sfi_mrtc_array[SFI_MRTC_MAX];
> > > > > >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> > > > > >  int sfi_mrtc_num;
> > > > > >
> > > > > > +#define MRST_THERMAL_ZONES	3
> > > > > > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > > > > > +	{ .thermal_zone_name = "CPU",
> > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > +	.num_cdevs = 2,
> > > > > > +	.cdevs_name = {"CPU", "Battery"},
> > > > > > +	.trip_mask = {0x0F, 0x08},
> > > > > > +	.weights = {80, 20}, },
> > > > > > +
> > > > > > +	{ .thermal_zone_name = "Battery",
> > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > +	.num_cdevs = 1,
> > > > > > +	.cdevs_name = {"Battery"},
> > > > > > +	.trip_mask = {0x0F},
> > > > > > +	.weights = {100}, },
> > > > > > +
> > > > > > +	{ .thermal_zone_name = "Skin",
> > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > +	.num_cdevs = 2,
> > > > > > +	.cdevs_name = {"Display", "Battery"},
> > > > > > +	.trip_mask = {0x0F, 0x0F},
> > > > > > +	.weights = {50, 50}, }
> > > > >
> > > > > Please consider the comment I sent on your data definition and also the
> > > > > comment I made on this patch on your RFC series.
> > > >
> > > > Yes.. I don't know why/how I missed it.
> > > > Also, saw the same comment on one of the other patches also.
> > > >
> > > > Will surely fix this thing in v2.
> > > >
> > > > BTW, any suggestion for the 'name' of that structure ? :-)
> > > 
> > > hmmm,
> > > do we still have thermal_zone_platforms in patch v2?
> > > I do not think we need this if we only bind devices via .bind()
> > > callback.
> > 
> > We can bind devices via .bind call back, and that will take some load
> > off the framework code. But even then, we would need this structure
> > right ?
> why?
> I'd prefer introduce something like this,
> struct thermal_bind_params {
> 	int trip;
> 	unsigned long upper;
> 	unsinged long lower;
> 	int weight;
> 	int sample_period;
> }
> 
> and use thermal_zone_bind_cooling_device(tz, cdev, thermal_bind_params),
> throttle_policy should be set when invoking
> thermal_zone_device_register.
> 
> is there any information in thermal_zone_params can not be convert to
> thermal_bind_params?

IMO, we need to think here carefully. Ideally, we should have a set of data
describing the thermal bindings. This way the code would look simpler and cleaner.
If we define a good way to describe the thermal bindings, I don't see why
we would need much complexity in the platform driver.

Assuming a good data structure design, the task of a platform driver would then be
to fetch the thermal info, either from bootloader, parameters, DT, etc, then
translate that into our binding descriptors, and pushing that data set forward
to the FW.

It think the above approach is much cleaner than writing for every platform
driver a set of function calls with static definitions telling what cooling
to do for each thermal zone.

What do you think?

> 
> thanks,
> rui
> 
> >  Say, when we obtain platform data from a thermal driver, it
> > should know 'what format the platform data is' ..correct ?
> > 
> > I theoretically agree with you that individual platform drivers can
> > have data in their own format, but that will be a heavy loss on
> > standardization.
> > 
> > So,
> > I will remove the extra bind code I added to framework, and
> > (keep it the old way it was) but still prefer to have the structure
> > put in thermal.h.
> > 
> > Thanks,
> > Durga
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[Zhang Rui]

On 二, 2012-08-21 at 11:51 +0300, Eduardo Valentin wrote:
> Hello,
> 
> On Tue, Aug 21, 2012 at 02:52:34PM +0800, Zhang Rui wrote:
> > On 二, 2012-08-21 at 00:41 -0600, R, Durgadoss wrote:
> > > Hi Rui,
> > > 
> > > > > > -----Original Message-----
> > > > > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > > > > > owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> > > > > > Sent: Tuesday, August 21, 2012 11:10 AM
> > > > > > To: R, Durgadoss
> > > > > > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
> > > > > > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > > > > > amit.kachhap@linaro.org; wni@nvidia.com
> > > > > > Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to provide
> > > > > > thermal data
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > > > > > > This patch shows how can we add platform specific thermal data
> > > > > > > required by the thermal framework. This is just an example
> > > > > > > patch, and _not_ for merge.
> > > > > > >
> > > > > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > > > > > ---
> > > > > > >  arch/x86/platform/mrst/mrst.c |   42
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 42 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/x86/platform/mrst/mrst.c
> > > > > > b/arch/x86/platform/mrst/mrst.c
> > > > > > > index fd41a92..0440db5 100644
> > > > > > > --- a/arch/x86/platform/mrst/mrst.c
> > > > > > > +++ b/arch/x86/platform/mrst/mrst.c
> > > > > > > @@ -30,6 +30,7 @@
> > > > > > >  #include <linux/mfd/intel_msic.h>
> > > > > > >  #include <linux/gpio.h>
> > > > > > >  #include <linux/i2c/tc35876x.h>
> > > > > > > +#include <linux/thermal.h>
> > > > > > >
> > > > > > >  #include <asm/setup.h>
> > > > > > >  #include <asm/mpspec_def.h>
> > > > > > > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> > > > > > sfi_mrtc_array[SFI_MRTC_MAX];
> > > > > > >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> > > > > > >  int sfi_mrtc_num;
> > > > > > >
> > > > > > > +#define MRST_THERMAL_ZONES	3
> > > > > > > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > > > > > > +	{ .thermal_zone_name = "CPU",
> > > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > > +	.num_cdevs = 2,
> > > > > > > +	.cdevs_name = {"CPU", "Battery"},
> > > > > > > +	.trip_mask = {0x0F, 0x08},
> > > > > > > +	.weights = {80, 20}, },
> > > > > > > +
> > > > > > > +	{ .thermal_zone_name = "Battery",
> > > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > > +	.num_cdevs = 1,
> > > > > > > +	.cdevs_name = {"Battery"},
> > > > > > > +	.trip_mask = {0x0F},
> > > > > > > +	.weights = {100}, },
> > > > > > > +
> > > > > > > +	{ .thermal_zone_name = "Skin",
> > > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > > +	.num_cdevs = 2,
> > > > > > > +	.cdevs_name = {"Display", "Battery"},
> > > > > > > +	.trip_mask = {0x0F, 0x0F},
> > > > > > > +	.weights = {50, 50}, }
> > > > > >
> > > > > > Please consider the comment I sent on your data definition and also the
> > > > > > comment I made on this patch on your RFC series.
> > > > >
> > > > > Yes.. I don't know why/how I missed it.
> > > > > Also, saw the same comment on one of the other patches also.
> > > > >
> > > > > Will surely fix this thing in v2.
> > > > >
> > > > > BTW, any suggestion for the 'name' of that structure ? :-)
> > > > 
> > > > hmmm,
> > > > do we still have thermal_zone_platforms in patch v2?
> > > > I do not think we need this if we only bind devices via .bind()
> > > > callback.
> > > 
> > > We can bind devices via .bind call back, and that will take some load
> > > off the framework code. But even then, we would need this structure
> > > right ?
> > why?
> > I'd prefer introduce something like this,
> > struct thermal_bind_params {
> > 	int trip;
> > 	unsigned long upper;
> > 	unsinged long lower;
> > 	int weight;
> > 	int sample_period;
> > }
> > 
> > and use thermal_zone_bind_cooling_device(tz, cdev, thermal_bind_params),
> > throttle_policy should be set when invoking
> > thermal_zone_device_register.
> > 
> > is there any information in thermal_zone_params can not be convert to
> > thermal_bind_params?
> 
> IMO, we need to think here carefully. Ideally, we should have a set of data
> describing the thermal bindings. This way the code would look simpler and cleaner.
> If we define a good way to describe the thermal bindings, I don't see why
> we would need much complexity in the platform driver.
> 
> Assuming a good data structure design, the task of a platform driver would then be
> to fetch the thermal info, either from bootloader, parameters, DT, etc, then
> translate that into our binding descriptors, and pushing that data set forward
> to the FW.
> 
> It think the above approach is much cleaner

agreed.

>  than writing for every platform
> driver a set of function calls with static definitions telling what cooling
> to do for each thermal zone.
> 
please define "a set of function calls".

thanks,
rui
> What do you think?
> 
> > 
> > thanks,
> > rui
> > 
> > >  Say, when we obtain platform data from a thermal driver, it
> > > should know 'what format the platform data is' ..correct ?
> > > 
> > > I theoretically agree with you that individual platform drivers can
> > > have data in their own format, but that will be a heavy loss on
> > > standardization.
> > > 
> > > So,
> > > I will remove the extra bind code I added to framework, and
> > > (keep it the old way it was) but still prefer to have the structure
> > > put in thermal.h.
> > > 
> > > Thanks,
> > > Durga
> > 
> > 


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[R, Durgadoss]

Hi Rui,

> 
> On 二, 2012-08-21 at 00:41 -0600, R, Durgadoss wrote:
> > Hi Rui,
> >
> > > > > -----Original Message-----
> > > > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > > > > owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> > > > > Sent: Tuesday, August 21, 2012 11:10 AM
> > > > > To: R, Durgadoss
> > > > > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-
> acpi@vger.kernel.org;
> > > > > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > > > > amit.kachhap@linaro.org; wni@nvidia.com
> > > > > Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to
> provide
> > > > > thermal data
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > > > > > This patch shows how can we add platform specific thermal data
> > > > > > required by the thermal framework. This is just an example
> > > > > > patch, and _not_ for merge.
> > > > > >
> > > > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > > > > ---
> > > > > >  arch/x86/platform/mrst/mrst.c |   42
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 42 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/x86/platform/mrst/mrst.c
> > > > > b/arch/x86/platform/mrst/mrst.c
> > > > > > index fd41a92..0440db5 100644
> > > > > > --- a/arch/x86/platform/mrst/mrst.c
> > > > > > +++ b/arch/x86/platform/mrst/mrst.c
> > > > > > @@ -30,6 +30,7 @@
> > > > > >  #include <linux/mfd/intel_msic.h>
> > > > > >  #include <linux/gpio.h>
> > > > > >  #include <linux/i2c/tc35876x.h>
> > > > > > +#include <linux/thermal.h>
> > > > > >
> > > > > >  #include <asm/setup.h>
> > > > > >  #include <asm/mpspec_def.h>
> > > > > > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> > > > > sfi_mrtc_array[SFI_MRTC_MAX];
> > > > > >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> > > > > >  int sfi_mrtc_num;
> > > > > >
> > > > > > +#define MRST_THERMAL_ZONES	3
> > > > > > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > > > > > +	{ .thermal_zone_name = "CPU",
> > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > +	.num_cdevs = 2,
> > > > > > +	.cdevs_name = {"CPU", "Battery"},
> > > > > > +	.trip_mask = {0x0F, 0x08},
> > > > > > +	.weights = {80, 20}, },
> > > > > > +
> > > > > > +	{ .thermal_zone_name = "Battery",
> > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > +	.num_cdevs = 1,
> > > > > > +	.cdevs_name = {"Battery"},
> > > > > > +	.trip_mask = {0x0F},
> > > > > > +	.weights = {100}, },
> > > > > > +
> > > > > > +	{ .thermal_zone_name = "Skin",
> > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > +	.num_cdevs = 2,
> > > > > > +	.cdevs_name = {"Display", "Battery"},
> > > > > > +	.trip_mask = {0x0F, 0x0F},
> > > > > > +	.weights = {50, 50}, }
> > > > >
> > > > > Please consider the comment I sent on your data definition and also
> the
> > > > > comment I made on this patch on your RFC series.
> > > >
> > > > Yes.. I don't know why/how I missed it.
> > > > Also, saw the same comment on one of the other patches also.
> > > >
> > > > Will surely fix this thing in v2.
> > > >
> > > > BTW, any suggestion for the 'name' of that structure ? :-)
> > >
> > > hmmm,
> > > do we still have thermal_zone_platforms in patch v2?
> > > I do not think we need this if we only bind devices via .bind()
> > > callback.
> >
> > We can bind devices via .bind call back, and that will take some load
> > off the framework code. But even then, we would need this structure
> > right ?
> why?
> I'd prefer introduce something like this,
> struct thermal_bind_params {
> 	int trip;
> 	unsigned long upper;
> 	unsinged long lower;
> 	int weight;
> 	int sample_period;
> }

Yes, this can work with little bit change like below:

struct thermal_zone_params {
	const char *zone_name;
	int num_cdevs;
	struct thermal_bind_params[num_cdevs];
}; 

Where struct thermal_bind_params will be like this:
	{
	.cdev_name = "CPU"
	.trip_mask = 0x0F
	.weight = 70
	.lower = 2
	.upper = 4
	.sample_period = 1000 (1 ms)
	};

Let me know what you think.

Thanks,
Durga

RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

[Zhang Rui]

On 二, 2012-08-21 at 03:28 -0600, R, Durgadoss wrote:
> Hi Rui,
> 
> > 
> > On 二, 2012-08-21 at 00:41 -0600, R, Durgadoss wrote:
> > > Hi Rui,
> > >
> > > > > > -----Original Message-----
> > > > > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> > > > > > owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> > > > > > Sent: Tuesday, August 21, 2012 11:10 AM
> > > > > > To: R, Durgadoss
> > > > > > Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-
> > acpi@vger.kernel.org;
> > > > > > linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
> > > > > > amit.kachhap@linaro.org; wni@nvidia.com
> > > > > > Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to
> > provide
> > > > > > thermal data
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:
> > > > > > > This patch shows how can we add platform specific thermal data
> > > > > > > required by the thermal framework. This is just an example
> > > > > > > patch, and _not_ for merge.
> > > > > > >
> > > > > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > > > > > ---
> > > > > > >  arch/x86/platform/mrst/mrst.c |   42
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 42 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/x86/platform/mrst/mrst.c
> > > > > > b/arch/x86/platform/mrst/mrst.c
> > > > > > > index fd41a92..0440db5 100644
> > > > > > > --- a/arch/x86/platform/mrst/mrst.c
> > > > > > > +++ b/arch/x86/platform/mrst/mrst.c
> > > > > > > @@ -30,6 +30,7 @@
> > > > > > >  #include <linux/mfd/intel_msic.h>
> > > > > > >  #include <linux/gpio.h>
> > > > > > >  #include <linux/i2c/tc35876x.h>
> > > > > > > +#include <linux/thermal.h>
> > > > > > >
> > > > > > >  #include <asm/setup.h>
> > > > > > >  #include <asm/mpspec_def.h>
> > > > > > > @@ -78,6 +79,30 @@ struct sfi_rtc_table_entry
> > > > > > sfi_mrtc_array[SFI_MRTC_MAX];
> > > > > > >  EXPORT_SYMBOL_GPL(sfi_mrtc_array);
> > > > > > >  int sfi_mrtc_num;
> > > > > > >
> > > > > > > +#define MRST_THERMAL_ZONES	3
> > > > > > > +struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
> > > > > > > +	{ .thermal_zone_name = "CPU",
> > > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > > +	.num_cdevs = 2,
> > > > > > > +	.cdevs_name = {"CPU", "Battery"},
> > > > > > > +	.trip_mask = {0x0F, 0x08},
> > > > > > > +	.weights = {80, 20}, },
> > > > > > > +
> > > > > > > +	{ .thermal_zone_name = "Battery",
> > > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > > +	.num_cdevs = 1,
> > > > > > > +	.cdevs_name = {"Battery"},
> > > > > > > +	.trip_mask = {0x0F},
> > > > > > > +	.weights = {100}, },
> > > > > > > +
> > > > > > > +	{ .thermal_zone_name = "Skin",
> > > > > > > +	.throttle_policy = THERMAL_FAIR_SHARE,
> > > > > > > +	.num_cdevs = 2,
> > > > > > > +	.cdevs_name = {"Display", "Battery"},
> > > > > > > +	.trip_mask = {0x0F, 0x0F},
> > > > > > > +	.weights = {50, 50}, }
> > > > > >
> > > > > > Please consider the comment I sent on your data definition and also
> > the
> > > > > > comment I made on this patch on your RFC series.
> > > > >
> > > > > Yes.. I don't know why/how I missed it.
> > > > > Also, saw the same comment on one of the other patches also.
> > > > >
> > > > > Will surely fix this thing in v2.
> > > > >
> > > > > BTW, any suggestion for the 'name' of that structure ? :-)
> > > >
> > > > hmmm,
> > > > do we still have thermal_zone_platforms in patch v2?
> > > > I do not think we need this if we only bind devices via .bind()
> > > > callback.
> > >
> > > We can bind devices via .bind call back, and that will take some load
> > > off the framework code. But even then, we would need this structure
> > > right ?
> > why?
> > I'd prefer introduce something like this,
> > struct thermal_bind_params {
> > 	int trip;
> > 	unsigned long upper;
> > 	unsinged long lower;
> > 	int weight;
> > 	int sample_period;
> > }
> 
> Yes, this can work with little bit change like below:
> 
> struct thermal_zone_params {
> 	const char *zone_name;
> 	int num_cdevs;
> 	struct thermal_bind_params[num_cdevs];
no, not num_cdevs.

Say CPU is used for both passive trip point 1 and passive trip point 2,
it has different lower/upper limit and weight.
and we need two thermal_bind_params here.

> }; 
> 
> Where struct thermal_bind_params will be like this:
> 	{
> 	.cdev_name = "CPU"
and the cooling device name must be unique in this case, which I do not
think it is reasonable.
because cooling devices are registered from different drivers, we do not
have a rule that followed by all these drivers.
> 	.trip_mask = 0x0F
> 	.weight = 70
> 	.lower = 2
> 	.upper = 4
> 	.sample_period = 1000 (1 ms)
> 	};
> 
if we want a way to register a couple of binding information to thermal
framework together with the thermal zone, I'd prefer something like
this,

struct thermal_zone_params {
 	struct thermal_zone_device *tz;
	int count;
 	struct thermal_bind_params *bindings;
}
struct thermal_bind_params {
	.id = 1;
	.match = platform_match_callback();
	.cdev = NULL;
	.weight = 70
	...
}

and in thermal_zone_device_register, we can have the code like this:

list_for_each_entry(cdev, &thermal_cdev_list, node) {
	for (i = 0; i < tz->params->count, i++) {
		if (tz->params->bindings[i].cdev)
			continue;
		/* check if this is the binding for this device */
		if (tz->params->bindings[i].match(tz, cdev, i))
			continue;
		tz->params->bindings[i]->cdev = cdev;
		thermal_zone_bind_cooling_device(tz, cdev, &tz->params->binding[i]);
	}
}

and we can have similar code in thermal_cdev_register().

you can do whatever in your platform_match_callback(), either strcmp or
checking devdata, or anything else.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html